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 10:11:42AM -0300, Jason Gunthorpe wrote:
> On Mon, Sep 15, 2025 at 01:54:05PM +0100, Lorenzo Stoakes wrote:
> > > Just mark the functions as manipulating the action using the 'action'
> > > in the fuction name.
> >
> > Because now sub-callers that partially map using one method and partially map
> > using another now need to have a desc too that they have to 'just know' which
> > fields to update or artificially set up.
>
> Huh? There is only on desc->action, how can you have more than one
> action with this scheme?

Because you use a custom hook that can in turn perform actions? As I've
implemented for vmcore?

>
> One action is the right thing anyhow, we can't meaningfully mix
> different action types in the same VMA. That's nonsense.

OK, except that's how 'true' mixed maps work though right? As vmcore is doing?

>
> You may need more flexible ways to get the address lists down the road
> because not every driver will be contiguous, but that should still be
> one action.
>
> > The vmcore case does something like this.
>
> vmcore is a true MIXEDMAP, it isn't doing two actions. These mixedmap
> helpers just aren't good for what mixedmap needs.. Mixed map need a
> list of physical pfns with a bit indicating if they are "special" or
> not. If you do it with a callback or a kmalloc allocation it doesn't
> matter.

Well it's a mix of actions to accomodate PFNs and normal pages as
implemented via a custom hook that can invoke each.

>
> vmcore would then populate that list with its mixture of special and
> non-sepcial memory and do a single mixedmem action.

I'm confused as to why you say a helper would be no good here, then go on
to delineate how a helper could work...

>
> I think this series should drop the mixedmem stuff, it is the most
> complicated action type. A vmalloc_user action is better for kcov.

Fine, I mean if we could find a way to explicitly just give a list of stuff
to map that'd be _great_ vs. having a custom hook.

If we can avoid custom hooks altogether that'd be ideal.

Anyway I'll drop the mixed map stuff, fine.

>
> And maybe that is just a comment overall. This would be nicer if each
> series focused on adding one action with a three-four mmap users
> converted to use it as an example case.

In future series I'll try to group by the action type.

This series is _setting up this to be a possibility at all_.

The idea was that I could put fundamentals in that should cover most cases,
I could then go on to implement them in (relative) peace...

I mean once I drop the mixed map stuff, and refactor to vmalloc_user(),
then we are pretty much doing that, modulo a single vmalloc_user() case.

So maybe I should drop the vmalloc_user() bits too and make this a
remap-only change...

But I don't want to tackle _all_ remap cases here.

I want to add this functionality in and have it ready for next cycle (yeah
not so sure about that now...) so I can then do follow up work.

Am trying to do it before Kernel Recipes which I'll be at and then a (very
very very needed) couple weeks vacaation.

Anyway maybe if I simplify there's still a shot at this landing in time...

>
> Eg there are not that many places calling vmalloc_user(), a single
> series could convert alot of them.
>
> If you did it this way we'd discover that there are already
> helpers for vmalloc_user():
>
> 	return remap_vmalloc_range(vma, mdev_state->memblk, 0);
>
> And kcov looks buggy to not be using it already. The above gets the
> VMA type right and doesn't force mixedmap :)

Right, I mean maybe.

If I can take care of low hanging fruit relatively easily then maybe it'll
be more practical to refactor the 'odd ones out'.

>
> Then the series goals are a bit better we can actually fully convert
> and remove things like remap_vmalloc_range() in single series. That
> looks feasible to me.

Right.

I'd love to drop unused stuff earlier, so _that_ is not an unreasonable
requirement.

>
> Jason

I guess I'll do a respin then as per above.

Cheers, Lorenzo