Re: [PATCH v3 3/4] mm/memory_hotplug: Refactor unregister_mem_sect_under_nodes

[Andrew Morton]

On Wed, 15 Aug 2018 16:42:18 +0200 Oscar Salvador <osalvador@xxxxxxxxxxxxxxxxxx> wrote:

> From: Oscar Salvador <osalvador@xxxxxxx>
> 
> unregister_mem_sect_under_nodes() tries to allocate a nodemask_t
> in order to check whithin the loop which nodes have already been unlinked,
> so we do not repeat the operation on them.
> 
> NODEMASK_ALLOC calls kmalloc() if NODES_SHIFT > 8, otherwise
> it just declares a nodemask_t variable whithin the stack.
> 
> Since kamlloc() can fail, we actually check whether NODEMASK_ALLOC failed
> or not, and we return -ENOMEM accordingly.
> remove_memory_section() does not check for the return value though.
> 
> The problem with this is that if we return -ENOMEM, it means that
> unregister_mem_sect_under_nodes will not be able to remove the symlinks,
> but since we do not check the return value, we go ahead and we call
> unregister_memory(), which will remove all the mem_blks directories.
> 
> This will leave us with dangled symlinks.
> 
> The easiest way to overcome this is to fallback by calling
> sysfs_remove_link() unconditionally in case NODEMASK_ALLOC failed.
> This means that we will call sysfs_remove_link on nodes that have been
> already unlinked, but nothing wrong happens as sysfs_remove_link()
> backs off somewhere down the chain in case the link has already been
> removed.
> 
> I think that this is better than
> 
> a) dangled symlinks
> b) having to recovery from such error in remove_memory_section
> 
> Since from now on we will not need to care about return values, we can make
> the function void.
> 
> As we have a safe fallback, one thing that could also be done is to add
> __GFP_NORETRY in the flags when calling NODEMASK_ALLOC, so we do not retry.
> 

Oh boy, lots of things.

That small GFP_KERNEL allocation will basically never fail.  In the
exceedingly-rare-basically-never-happens case, simply bailing out of
unregister_mem_sect_under_nodes() seems acceptable.  But I guess that
addressing it is a reasonable thing to do, if it can be done sanely.

But given that your new unregister_mem_sect_under_nodes() can proceed
happily even if the allocation failed, what's the point in allocating
the nodemask at all?  Why not just go ahead and run sysfs_remove_link()
against possibly-absent sysfs objects every time?  That produces
simpler code and avoids having this basically-never-executed-or-tested
code path in there.

Incidentally, do we have locking in place to prevent
unregister_mem_sect_under_nodes() from accidentally removing sysfs
nodes which were added 2 nanoseconds ago by a concurrent thread?

Also, this stuff in nodemask.h:

: /*
:  * For nodemask scrach area.
:  * NODEMASK_ALLOC(type, name) allocates an object with a specified type and
:  * name.
:  */
: #if NODES_SHIFT > 8 /* nodemask_t > 256 bytes */
: #define NODEMASK_ALLOC(type, name, gfp_flags)	\
:			type *name = kmalloc(sizeof(*name), gfp_flags)
: #define NODEMASK_FREE(m)			kfree(m)
: #else
: #define NODEMASK_ALLOC(type, name, gfp_flags)	type _##name, *name = &_##name
: #define NODEMASK_FREE(m)			do {} while (0)
: #endif

a) s/scrach/scratch/

b) The comment is wrong, isn't it?  "NODES_SHIFT > 8" means
   "nodemask_t > 32 bytes"?

c) If "yes" then we can surely bump that up a bit - "NODES_SHIFT >
   11", say.

d) What's the maximum number of nodes, ever?  Perhaps we can always
   fit a nodemask_t onto the stack, dunno.