On Tue, Feb 26, 2019 at 04:03:23PM -0800, Mike Kravetz wrote: > I was just going to update the comments and send you a new patch, but > but your comment got me thinking about this situation. I did not really > change the way this code operates. As a reminder, the original code is like: > > NODEMASK_ALLOC(nodemask_t, nodes_allowed, GFP_KERNEL | __GFP_NORETRY); > > if (nid == NUMA_NO_NODE) { > /* do something */ > } else if (nodes_allowed) { > /* do something else */ > } else { > nodes_allowed = &node_states[N_MEMORY]; > } > > So, the only way we get to that final else if if we can not allocate > a node mask (kmalloc a few words). Right? I wonder why we should > even try to continue in this case. Why not just return right there? > > The specified count value is either a request to increase number of > huge pages or decrease. If we can't allocate a few words, we certainly > are not going to find memory to create huge pages. There 'might' be > surplus pages which can be converted to permanent pages. But remember > this is a 'node specific' request and we can't allocate a mask to pass > down to the conversion routines. So, chances are good we would operate > on the wrong node. The same goes for a request to 'free' huge pages. > Since, we can't allocate a node mask we are likely to free them from > the wrong node. > > Unless my reasoning above is incorrect, I think that final else block > in __nr_hugepages_store_common() is wrong. > > Any additional thoughts? Could not we kill the NODEMASK_ALLOC there? __nr_hugepages_store_common() should be called from a rather shallow stack, and unless I am wrong, the maximum value we can get for a nodemask_t is 128bytes (1024 NUMA nodes). -- Oscar Salvador SUSE L3