Re: [PATCH v2 08/16] mm: add ability to take further action in vm_area_desc

[Lorenzo Stoakes <lorenzo.stoakes@oracle.com>]

On Mon, Sep 15, 2025 at 09:11:12AM -0300, Jason Gunthorpe wrote:
> On Wed, Sep 10, 2025 at 09:22:03PM +0100, Lorenzo Stoakes wrote:
> > +static inline void mmap_action_remap(struct mmap_action *action,
> > +		unsigned long addr, unsigned long pfn, unsigned long size,
> > +		pgprot_t pgprot)
> > +{
> > +	action->type = MMAP_REMAP_PFN;
> > +
> > +	action->remap.addr = addr;
> > +	action->remap.pfn = pfn;
> > +	action->remap.size = size;
> > +	action->remap.pgprot = pgprot;
> > +}
>
> These helpers drivers are supposed to call really should have kdocs.
>
> Especially since 'addr' is sort of ambigous.

OK.

>
> And I'm wondering why they don't take in the vm_area_desc? Eg shouldn't
> we be strongly discouraging using anything other than
> vma->vm_page_prot as the last argument?

I need to abstract desc from action so custom handlers can perform
sub-actions. It's unfortunate but there we go.

There'd be horrible confusion passing around a desc that has an action in
it that you then ignore, otherwise. Better to abstract the concept of
action altogether.

>
> I'd probably also have a small helper wrapper for the very common case
> of whole vma:
>
> /* Fill the entire VMA with pfns starting at pfn. Caller must have
>  * already checked desc has an appropriate size */
> mmap_action_remap_full(struct vm_area_desc *desc, unsigned long pfn)

See above re: desc vs. action.



>
> It is not normal for a driver to partially populate a VMA, lets call
> those out as something weird.
>
> > +struct page **mmap_action_mixedmap_pages(struct mmap_action *action,
> > +		unsigned long addr, unsigned long num_pages)
> > +{
> > +	struct page **pages;
> > +
> > +	pages = kmalloc_array(num_pages, sizeof(struct page *), GFP_KERNEL);
> > +	if (!pages)
> > +		return NULL;
>
> This allocation seems like a shame, I doubt many places actually need
> it .. A callback to get each pfn would be better?

It'd be hard to know how to get the context right that'd need to be supplied to
the callback.

In kcov's case it'd be kcov->area + an offset.

So we'd need an offset parameter, the struct file *, whatever else to be
passed.

And then we'll find a driver where that doesn't work and we're screwed.

I don't think optimising for mmap setup is really important.

We can always go back and refactor things later once this pattern is
established.

And again with ~230 odd drivers to update, I'd rather keep things as simple
as possible for now.

>
> Jason

Cheers, Lorenzo