Re: [PATCH v6 03/10] mm,memory_hotplug: Implement numa node notifier

[Oscar Salvador]

On Tue, Jun 10, 2025 at 10:10:21AM +0200, David Hildenbrand wrote:
> On 09.06.25 11:21, Oscar Salvador wrote:
> > +The first argument of the callback function (self) is a pointer to the block
> > +of the notifier chain that points to the callback function itself.
> > +The second argument (action) is one of the event types described above.
> > +The third argument (arg) passes a pointer of struct node_notify::
> > +
> > +        struct node_notify {
> > +                int nid;
> > +        }
> > +
> > +- nid is the node we are adding or removing memory to.
> > +
> > +  If nid >= 0, callback should create/discard structures for the
> > +  node if necessary.
> 
> Likely that should be removed?

Yes, indeed.

> 
> It' probably worth mentioning that one might get notified about
> NODE_CANCEL_ADDING_FIRST_MEMORY even though never notified for
> NODE_ADDING_FIRST_MEMORY. (same for removing)
> 
> I recall this can happen if one of the NODE_ADDING_FIRST_MEMORY notifiers
> fails.
> 
> (same applies to MEM_CANCEL_*)
> 
> Consequently, we might simplify the cancel_mem_notifier_on_err etc stuff,
> simply unconditionally calling the cancel counterparts.

So, I managed to do another respin with all feedback included, but I
left this one for the end, and here I'm.

It's true, currently users can get notified about e.g: MEM_CANCE_ONLINE without
going through MEM_GOING_ONLINE if another user fails for the latter, but I'm
trying to workaround the fact why that's not a problem.

Because assume you have a user of MEM_CANCEL_ONLINE, who thinks it got called
for MEM_GOING_ONLINE, while in fact it didn't because some other user fail on
it, and it tries to free some memory it thinks it initialized during MEM_GOING_ONLINE.

Isn't this a bit shaky? I mean, yes, I guess we can put the burden on the users of
the notifiers to not assume anything, but then yes, I think we should document this
as it can lead to potential misbeliefs.


-- 
Oscar Salvador
SUSE Labs