[PATCH V2 fix 5/6] mm: hugetlb: add a new function to allocate a new gigantic page

[Vlastimil Babka]

On 11/29/2016 10:03 AM, Huang Shijie wrote:
> On Mon, Nov 28, 2016 at 03:17:28PM +0100, Vlastimil Babka wrote:
>> On 11/16/2016 07:55 AM, Huang Shijie wrote:
>> > +static struct page *__hugetlb_alloc_gigantic_page(struct hstate *h,
>> > +		struct vm_area_struct *vma, unsigned long addr, int nid)
>> > +{
>> > +	NODEMASK_ALLOC(nodemask_t, nodes_allowed, GFP_KERNEL | __GFP_NORETRY);
>>
>> What if the allocation fails and nodes_allowed is NULL?
>> It might work fine now, but it's rather fragile, so I'd rather see an
> Yes.
>
>> explicit check.
> See the comment below.
>
>>
>> BTW same thing applies to __nr_hugepages_store_common().
>>
>> > +	struct page *page = NULL;
>> > +
>> > +	/* Not NUMA */
>> > +	if (!IS_ENABLED(CONFIG_NUMA)) {
>> > +		if (nid == NUMA_NO_NODE)
>> > +			nid = numa_mem_id();
>> > +
>> > +		page = alloc_gigantic_page(nid, huge_page_order(h));
>> > +		if (page)
>> > +			prep_compound_gigantic_page(page, huge_page_order(h));
>> > +
>> > +		NODEMASK_FREE(nodes_allowed);
>> > +		return page;
>> > +	}
>> > +
>> > +	/* NUMA && !vma */
>> > +	if (!vma) {
>> > +		if (nid == NUMA_NO_NODE) {
>> > +			if (!init_nodemask_of_mempolicy(nodes_allowed)) {
>> > +				NODEMASK_FREE(nodes_allowed);
>> > +				nodes_allowed = &node_states[N_MEMORY];
>> > +			}
>> > +		} else if (nodes_allowed) {
> The check is here.

It's below a possible usage of nodes_allowed as an argument of 
init_nodemask_of_mempolicy(mask). Which does

         if (!(mask && current->mempolicy))
                 return false;

which itself looks like an error at first sight :)

> Do we really need to re-arrange the code here for the explicit check? :)

We don't need it *now* to be correct, but I still find it fragile. Also it
mixes up the semantic of NULL as a conscious "default" value, and NULL as
a side-effect of memory allocation failure. Nothing good can come from that in 
the long term :)

> Thanks
> Huang Shijie
>> > +			init_nodemask_of_node(nodes_allowed, nid);
>> > +		} else {
>> > +			nodes_allowed = &node_states[N_MEMORY];
>> > +		}
>> > +
>> > +		page = alloc_fresh_gigantic_page(h, nodes_allowed, true);
>> > +
>