mm/hugetlb: hide nr_nodes in the internal of for_each_node_mask_to_[alloc|free]
diff mbox series

Message ID 20200714073404.84863-1-richard.weiyang@linux.alibaba.com
State New
Headers show
Series
  • mm/hugetlb: hide nr_nodes in the internal of for_each_node_mask_to_[alloc|free]
Related show

Commit Message

Wei Yang July 14, 2020, 7:34 a.m. UTC
The second parameter of for_each_node_mask_to_[alloc|free] is a loop
variant, which is not used outside of loop iteration.

Let's hide this.

Signed-off-by: Wei Yang <richard.weiyang@linux.alibaba.com>
---
 mm/hugetlb.c | 38 ++++++++++++++++++++------------------
 1 file changed, 20 insertions(+), 18 deletions(-)

Comments

Pankaj Gupta July 14, 2020, 8:37 a.m. UTC | #1
> The second parameter of for_each_node_mask_to_[alloc|free] is a loop
> variant, which is not used outside of loop iteration.
>
> Let's hide this.
>
> Signed-off-by: Wei Yang <richard.weiyang@linux.alibaba.com>
> ---
>  mm/hugetlb.c | 38 ++++++++++++++++++++------------------
>  1 file changed, 20 insertions(+), 18 deletions(-)
>
> diff --git a/mm/hugetlb.c b/mm/hugetlb.c
> index 57ece74e3aae..9c3d15fb317e 100644
> --- a/mm/hugetlb.c
> +++ b/mm/hugetlb.c
> @@ -1196,17 +1196,19 @@ static int hstate_next_node_to_free(struct hstate *h, nodemask_t *nodes_allowed)
>         return nid;
>  }
>
> -#define for_each_node_mask_to_alloc(hs, nr_nodes, node, mask)          \
> -       for (nr_nodes = nodes_weight(*mask);                            \
> -               nr_nodes > 0 &&                                         \
> +#define for_each_node_mask_to_alloc(hs, node, mask)                    \
> +       int __nr_nodes;                                                 \
> +       for (__nr_nodes = nodes_weight(*mask);                          \
> +               __nr_nodes > 0 &&                                       \
>                 ((node = hstate_next_node_to_alloc(hs, mask)) || 1);    \
> -               nr_nodes--)
> +               __nr_nodes--)
>
> -#define for_each_node_mask_to_free(hs, nr_nodes, node, mask)           \
> -       for (nr_nodes = nodes_weight(*mask);                            \
> -               nr_nodes > 0 &&                                         \
> +#define for_each_node_mask_to_free(hs, node, mask)                     \
> +       int __nr_nodes;                                                 \
> +       for (__nr_nodes = nodes_weight(*mask);                          \
> +               __nr_nodes > 0 &&                                       \
>                 ((node = hstate_next_node_to_free(hs, mask)) || 1);     \
> -               nr_nodes--)
> +               __nr_nodes--)
>
>  #ifdef CONFIG_ARCH_HAS_GIGANTIC_PAGE
>  static void destroy_compound_gigantic_page(struct page *page,
> @@ -1403,7 +1405,7 @@ static void __free_huge_page(struct page *page)
>          * reservation.  If the page was associated with a subpool, there
>          * would have been a page reserved in the subpool before allocation
>          * via hugepage_subpool_get_pages().  Since we are 'restoring' the
> -        * reservtion, do not call hugepage_subpool_put_pages() as this will
> +        * reservation, do not call hugepage_subpool_put_pages() as this will
>          * remove the reserved page from the subpool.
>          */
>         if (!restore_reserve) {
> @@ -1760,10 +1762,10 @@ static int alloc_pool_huge_page(struct hstate *h, nodemask_t *nodes_allowed,
>                                 nodemask_t *node_alloc_noretry)
>  {
>         struct page *page;
> -       int nr_nodes, node;
> +       int node;
>         gfp_t gfp_mask = htlb_alloc_mask(h) | __GFP_THISNODE;
>
> -       for_each_node_mask_to_alloc(h, nr_nodes, node, nodes_allowed) {
> +       for_each_node_mask_to_alloc(h, node, nodes_allowed) {
>                 page = alloc_fresh_huge_page(h, gfp_mask, node, nodes_allowed,
>                                                 node_alloc_noretry);
>                 if (page)
> @@ -1787,10 +1789,10 @@ static int alloc_pool_huge_page(struct hstate *h, nodemask_t *nodes_allowed,
>  static int free_pool_huge_page(struct hstate *h, nodemask_t *nodes_allowed,
>                                                          bool acct_surplus)
>  {
> -       int nr_nodes, node;
> +       int node;
>         int ret = 0;
>
> -       for_each_node_mask_to_free(h, nr_nodes, node, nodes_allowed) {
> +       for_each_node_mask_to_free(h, node, nodes_allowed) {
>                 /*
>                  * If we're returning unused surplus pages, only examine
>                  * nodes with surplus pages.
> @@ -2481,9 +2483,9 @@ int alloc_bootmem_huge_page(struct hstate *h)
>  int __alloc_bootmem_huge_page(struct hstate *h)
>  {
>         struct huge_bootmem_page *m;
> -       int nr_nodes, node;
> +       int node;
>
> -       for_each_node_mask_to_alloc(h, nr_nodes, node, &node_states[N_MEMORY]) {
> +       for_each_node_mask_to_alloc(h, node, &node_states[N_MEMORY]) {
>                 void *addr;
>
>                 addr = memblock_alloc_try_nid_raw(
> @@ -2662,17 +2664,17 @@ static inline void try_to_free_low(struct hstate *h, unsigned long count,
>  static int adjust_pool_surplus(struct hstate *h, nodemask_t *nodes_allowed,
>                                 int delta)
>  {
> -       int nr_nodes, node;
> +       int node;
>
>         VM_BUG_ON(delta != -1 && delta != 1);
>
>         if (delta < 0) {
> -               for_each_node_mask_to_alloc(h, nr_nodes, node, nodes_allowed) {
> +               for_each_node_mask_to_alloc(h, node, nodes_allowed) {
>                         if (h->surplus_huge_pages_node[node])
>                                 goto found;
>                 }
>         } else {
> -               for_each_node_mask_to_free(h, nr_nodes, node, nodes_allowed) {
> +               for_each_node_mask_to_free(h, node, nodes_allowed) {
>                         if (h->surplus_huge_pages_node[node] <
>                                         h->nr_huge_pages_node[node])
>                                 goto found;
> --
> 2.20.1 (Apple Git-117)

Acked-by: Pankaj Gupta <pankaj.gupta.linux@gmail.com>

>
>
Vlastimil Babka July 14, 2020, 9:13 a.m. UTC | #2
On 7/14/20 9:34 AM, Wei Yang wrote:
> The second parameter of for_each_node_mask_to_[alloc|free] is a loop
> variant, which is not used outside of loop iteration.
> 
> Let's hide this.
> 
> Signed-off-by: Wei Yang <richard.weiyang@linux.alibaba.com>
> ---
>  mm/hugetlb.c | 38 ++++++++++++++++++++------------------
>  1 file changed, 20 insertions(+), 18 deletions(-)
> 
> diff --git a/mm/hugetlb.c b/mm/hugetlb.c
> index 57ece74e3aae..9c3d15fb317e 100644
> --- a/mm/hugetlb.c
> +++ b/mm/hugetlb.c
> @@ -1196,17 +1196,19 @@ static int hstate_next_node_to_free(struct hstate *h, nodemask_t *nodes_allowed)
>  	return nid;
>  }
>  
> -#define for_each_node_mask_to_alloc(hs, nr_nodes, node, mask)		\
> -	for (nr_nodes = nodes_weight(*mask);				\
> -		nr_nodes > 0 &&						\
> +#define for_each_node_mask_to_alloc(hs, node, mask)			\
> +	int __nr_nodes;							\
> +	for (__nr_nodes = nodes_weight(*mask);				\

The problem with this is that if I use the macro twice in the same block, this
will redefine __nr_nodes and fail to compile, no?
In that case it's better to avoid setting up this trap, IMHO.

> +		__nr_nodes > 0 &&					\
>  		((node = hstate_next_node_to_alloc(hs, mask)) || 1);	\
> -		nr_nodes--)
> +		__nr_nodes--)
>  
> -#define for_each_node_mask_to_free(hs, nr_nodes, node, mask)		\
> -	for (nr_nodes = nodes_weight(*mask);				\
> -		nr_nodes > 0 &&						\
> +#define for_each_node_mask_to_free(hs, node, mask)			\
> +	int __nr_nodes;							\
> +	for (__nr_nodes = nodes_weight(*mask);				\
> +		__nr_nodes > 0 &&					\
>  		((node = hstate_next_node_to_free(hs, mask)) || 1);	\
> -		nr_nodes--)
> +		__nr_nodes--)
>  
>  #ifdef CONFIG_ARCH_HAS_GIGANTIC_PAGE
>  static void destroy_compound_gigantic_page(struct page *page,
> @@ -1403,7 +1405,7 @@ static void __free_huge_page(struct page *page)
>  	 * reservation.  If the page was associated with a subpool, there
>  	 * would have been a page reserved in the subpool before allocation
>  	 * via hugepage_subpool_get_pages().  Since we are 'restoring' the
> -	 * reservtion, do not call hugepage_subpool_put_pages() as this will
> +	 * reservation, do not call hugepage_subpool_put_pages() as this will
>  	 * remove the reserved page from the subpool.
>  	 */
>  	if (!restore_reserve) {
> @@ -1760,10 +1762,10 @@ static int alloc_pool_huge_page(struct hstate *h, nodemask_t *nodes_allowed,
>  				nodemask_t *node_alloc_noretry)
>  {
>  	struct page *page;
> -	int nr_nodes, node;
> +	int node;
>  	gfp_t gfp_mask = htlb_alloc_mask(h) | __GFP_THISNODE;
>  
> -	for_each_node_mask_to_alloc(h, nr_nodes, node, nodes_allowed) {
> +	for_each_node_mask_to_alloc(h, node, nodes_allowed) {
>  		page = alloc_fresh_huge_page(h, gfp_mask, node, nodes_allowed,
>  						node_alloc_noretry);
>  		if (page)
> @@ -1787,10 +1789,10 @@ static int alloc_pool_huge_page(struct hstate *h, nodemask_t *nodes_allowed,
>  static int free_pool_huge_page(struct hstate *h, nodemask_t *nodes_allowed,
>  							 bool acct_surplus)
>  {
> -	int nr_nodes, node;
> +	int node;
>  	int ret = 0;
>  
> -	for_each_node_mask_to_free(h, nr_nodes, node, nodes_allowed) {
> +	for_each_node_mask_to_free(h, node, nodes_allowed) {
>  		/*
>  		 * If we're returning unused surplus pages, only examine
>  		 * nodes with surplus pages.
> @@ -2481,9 +2483,9 @@ int alloc_bootmem_huge_page(struct hstate *h)
>  int __alloc_bootmem_huge_page(struct hstate *h)
>  {
>  	struct huge_bootmem_page *m;
> -	int nr_nodes, node;
> +	int node;
>  
> -	for_each_node_mask_to_alloc(h, nr_nodes, node, &node_states[N_MEMORY]) {
> +	for_each_node_mask_to_alloc(h, node, &node_states[N_MEMORY]) {
>  		void *addr;
>  
>  		addr = memblock_alloc_try_nid_raw(
> @@ -2662,17 +2664,17 @@ static inline void try_to_free_low(struct hstate *h, unsigned long count,
>  static int adjust_pool_surplus(struct hstate *h, nodemask_t *nodes_allowed,
>  				int delta)
>  {
> -	int nr_nodes, node;
> +	int node;
>  
>  	VM_BUG_ON(delta != -1 && delta != 1);
>  
>  	if (delta < 0) {
> -		for_each_node_mask_to_alloc(h, nr_nodes, node, nodes_allowed) {
> +		for_each_node_mask_to_alloc(h, node, nodes_allowed) {
>  			if (h->surplus_huge_pages_node[node])
>  				goto found;
>  		}
>  	} else {
> -		for_each_node_mask_to_free(h, nr_nodes, node, nodes_allowed) {
> +		for_each_node_mask_to_free(h, node, nodes_allowed) {
>  			if (h->surplus_huge_pages_node[node] <
>  					h->nr_huge_pages_node[node])
>  				goto found;
>
Vlastimil Babka July 14, 2020, 9:22 a.m. UTC | #3
On 7/14/20 11:13 AM, Vlastimil Babka wrote:
> On 7/14/20 9:34 AM, Wei Yang wrote:
>> The second parameter of for_each_node_mask_to_[alloc|free] is a loop
>> variant, which is not used outside of loop iteration.
>> 
>> Let's hide this.
>> 
>> Signed-off-by: Wei Yang <richard.weiyang@linux.alibaba.com>
>> ---
>>  mm/hugetlb.c | 38 ++++++++++++++++++++------------------
>>  1 file changed, 20 insertions(+), 18 deletions(-)
>> 
>> diff --git a/mm/hugetlb.c b/mm/hugetlb.c
>> index 57ece74e3aae..9c3d15fb317e 100644
>> --- a/mm/hugetlb.c
>> +++ b/mm/hugetlb.c
>> @@ -1196,17 +1196,19 @@ static int hstate_next_node_to_free(struct hstate *h, nodemask_t *nodes_allowed)
>>  	return nid;
>>  }
>>  
>> -#define for_each_node_mask_to_alloc(hs, nr_nodes, node, mask)		\
>> -	for (nr_nodes = nodes_weight(*mask);				\
>> -		nr_nodes > 0 &&						\
>> +#define for_each_node_mask_to_alloc(hs, node, mask)			\
>> +	int __nr_nodes;							\
>> +	for (__nr_nodes = nodes_weight(*mask);				\
> 
> The problem with this is that if I use the macro twice in the same block, this
> will redefine __nr_nodes and fail to compile, no?
> In that case it's better to avoid setting up this trap, IMHO.

Ah, and it will also generate the following warning, if the use of for_each*
macro is not the first thing after variable declarations, but there's another
statement before:

warning: ISO C90 forbids mixed declarations and code [-Wdeclaration-after-statement]

Instead we should switch to C99 and declare it as "for (int __nr_nodes" :P
Wei Yang July 14, 2020, 9:50 a.m. UTC | #4
On Tue, Jul 14, 2020 at 11:22:03AM +0200, Vlastimil Babka wrote:
>On 7/14/20 11:13 AM, Vlastimil Babka wrote:
>> On 7/14/20 9:34 AM, Wei Yang wrote:
>>> The second parameter of for_each_node_mask_to_[alloc|free] is a loop
>>> variant, which is not used outside of loop iteration.
>>> 
>>> Let's hide this.
>>> 
>>> Signed-off-by: Wei Yang <richard.weiyang@linux.alibaba.com>
>>> ---
>>>  mm/hugetlb.c | 38 ++++++++++++++++++++------------------
>>>  1 file changed, 20 insertions(+), 18 deletions(-)
>>> 
>>> diff --git a/mm/hugetlb.c b/mm/hugetlb.c
>>> index 57ece74e3aae..9c3d15fb317e 100644
>>> --- a/mm/hugetlb.c
>>> +++ b/mm/hugetlb.c
>>> @@ -1196,17 +1196,19 @@ static int hstate_next_node_to_free(struct hstate *h, nodemask_t *nodes_allowed)
>>>  	return nid;
>>>  }
>>>  
>>> -#define for_each_node_mask_to_alloc(hs, nr_nodes, node, mask)		\
>>> -	for (nr_nodes = nodes_weight(*mask);				\
>>> -		nr_nodes > 0 &&						\
>>> +#define for_each_node_mask_to_alloc(hs, node, mask)			\
>>> +	int __nr_nodes;							\
>>> +	for (__nr_nodes = nodes_weight(*mask);				\
>> 
>> The problem with this is that if I use the macro twice in the same block, this
>> will redefine __nr_nodes and fail to compile, no?
>> In that case it's better to avoid setting up this trap, IMHO.
>
>Ah, and it will also generate the following warning, if the use of for_each*
>macro is not the first thing after variable declarations, but there's another
>statement before:
>
>warning: ISO C90 forbids mixed declarations and code [-Wdeclaration-after-statement]
>
>Instead we should switch to C99 and declare it as "for (int __nr_nodes" :P

Thanks, I haven't notice the problem you mentioned.

Let me fix this.
Wei Yang July 14, 2020, 9:57 a.m. UTC | #5
On Tue, Jul 14, 2020 at 11:22:03AM +0200, Vlastimil Babka wrote:
>On 7/14/20 11:13 AM, Vlastimil Babka wrote:
>> On 7/14/20 9:34 AM, Wei Yang wrote:
>>> The second parameter of for_each_node_mask_to_[alloc|free] is a loop
>>> variant, which is not used outside of loop iteration.
>>> 
>>> Let's hide this.
>>> 
>>> Signed-off-by: Wei Yang <richard.weiyang@linux.alibaba.com>
>>> ---
>>>  mm/hugetlb.c | 38 ++++++++++++++++++++------------------
>>>  1 file changed, 20 insertions(+), 18 deletions(-)
>>> 
>>> diff --git a/mm/hugetlb.c b/mm/hugetlb.c
>>> index 57ece74e3aae..9c3d15fb317e 100644
>>> --- a/mm/hugetlb.c
>>> +++ b/mm/hugetlb.c
>>> @@ -1196,17 +1196,19 @@ static int hstate_next_node_to_free(struct hstate *h, nodemask_t *nodes_allowed)
>>>  	return nid;
>>>  }
>>>  
>>> -#define for_each_node_mask_to_alloc(hs, nr_nodes, node, mask)		\
>>> -	for (nr_nodes = nodes_weight(*mask);				\
>>> -		nr_nodes > 0 &&						\
>>> +#define for_each_node_mask_to_alloc(hs, node, mask)			\
>>> +	int __nr_nodes;							\
>>> +	for (__nr_nodes = nodes_weight(*mask);				\
>> 
>> The problem with this is that if I use the macro twice in the same block, this
>> will redefine __nr_nodes and fail to compile, no?
>> In that case it's better to avoid setting up this trap, IMHO.
>
>Ah, and it will also generate the following warning, if the use of for_each*
>macro is not the first thing after variable declarations, but there's another
>statement before:
>
>warning: ISO C90 forbids mixed declarations and code [-Wdeclaration-after-statement]
>
>Instead we should switch to C99 and declare it as "for (int __nr_nodes" :P

Hmm... I tried what you suggested, but compiler complains.

'for' loop initial declarations are only allowed in C99 or C11 mode
Vlastimil Babka July 14, 2020, 10:02 a.m. UTC | #6
On 7/14/20 11:57 AM, Wei Yang wrote:
> On Tue, Jul 14, 2020 at 11:22:03AM +0200, Vlastimil Babka wrote:
>>On 7/14/20 11:13 AM, Vlastimil Babka wrote:
>>> On 7/14/20 9:34 AM, Wei Yang wrote:
>>>> The second parameter of for_each_node_mask_to_[alloc|free] is a loop
>>>> variant, which is not used outside of loop iteration.
>>>> 
>>>> Let's hide this.
>>>> 
>>>> Signed-off-by: Wei Yang <richard.weiyang@linux.alibaba.com>
>>>> ---
>>>>  mm/hugetlb.c | 38 ++++++++++++++++++++------------------
>>>>  1 file changed, 20 insertions(+), 18 deletions(-)
>>>> 
>>>> diff --git a/mm/hugetlb.c b/mm/hugetlb.c
>>>> index 57ece74e3aae..9c3d15fb317e 100644
>>>> --- a/mm/hugetlb.c
>>>> +++ b/mm/hugetlb.c
>>>> @@ -1196,17 +1196,19 @@ static int hstate_next_node_to_free(struct hstate *h, nodemask_t *nodes_allowed)
>>>>  	return nid;
>>>>  }
>>>>  
>>>> -#define for_each_node_mask_to_alloc(hs, nr_nodes, node, mask)		\
>>>> -	for (nr_nodes = nodes_weight(*mask);				\
>>>> -		nr_nodes > 0 &&						\
>>>> +#define for_each_node_mask_to_alloc(hs, node, mask)			\
>>>> +	int __nr_nodes;							\
>>>> +	for (__nr_nodes = nodes_weight(*mask);				\
>>> 
>>> The problem with this is that if I use the macro twice in the same block, this
>>> will redefine __nr_nodes and fail to compile, no?
>>> In that case it's better to avoid setting up this trap, IMHO.
>>
>>Ah, and it will also generate the following warning, if the use of for_each*
>>macro is not the first thing after variable declarations, but there's another
>>statement before:
>>
>>warning: ISO C90 forbids mixed declarations and code [-Wdeclaration-after-statement]
>>
>>Instead we should switch to C99 and declare it as "for (int __nr_nodes" :P
> 
> Hmm... I tried what you suggested, but compiler complains.
> 
> 'for' loop initial declarations are only allowed in C99 or C11 mode

Yes, by "we should switch to C99" I meant that the kernel kbuild system would
need to switch. Not a trivial change...
Without that, I don't see how your patch is possible to do safely.
Mike Kravetz July 14, 2020, 9:12 p.m. UTC | #7
On 7/14/20 3:02 AM, Vlastimil Babka wrote:
> On 7/14/20 11:57 AM, Wei Yang wrote:
>> On Tue, Jul 14, 2020 at 11:22:03AM +0200, Vlastimil Babka wrote:
>>> On 7/14/20 11:13 AM, Vlastimil Babka wrote:
>>>> On 7/14/20 9:34 AM, Wei Yang wrote:
>>>>> The second parameter of for_each_node_mask_to_[alloc|free] is a loop
>>>>> variant, which is not used outside of loop iteration.
>>>>>
>>>>> Let's hide this.
>>>>>
>>>>> Signed-off-by: Wei Yang <richard.weiyang@linux.alibaba.com>
>>>>> ---
>>>>>  mm/hugetlb.c | 38 ++++++++++++++++++++------------------
>>>>>  1 file changed, 20 insertions(+), 18 deletions(-)
>>>>>
>>>>> diff --git a/mm/hugetlb.c b/mm/hugetlb.c
>>>>> index 57ece74e3aae..9c3d15fb317e 100644
>>>>> --- a/mm/hugetlb.c
>>>>> +++ b/mm/hugetlb.c
>>>>> @@ -1196,17 +1196,19 @@ static int hstate_next_node_to_free(struct hstate *h, nodemask_t *nodes_allowed)
>>>>>  	return nid;
>>>>>  }
>>>>>  
>>>>> -#define for_each_node_mask_to_alloc(hs, nr_nodes, node, mask)		\
>>>>> -	for (nr_nodes = nodes_weight(*mask);				\
>>>>> -		nr_nodes > 0 &&						\
>>>>> +#define for_each_node_mask_to_alloc(hs, node, mask)			\
>>>>> +	int __nr_nodes;							\
>>>>> +	for (__nr_nodes = nodes_weight(*mask);				\
>>>>
>>>> The problem with this is that if I use the macro twice in the same block, this
>>>> will redefine __nr_nodes and fail to compile, no?
>>>> In that case it's better to avoid setting up this trap, IMHO.
>>>
>>> Ah, and it will also generate the following warning, if the use of for_each*
>>> macro is not the first thing after variable declarations, but there's another
>>> statement before:
>>>
>>> warning: ISO C90 forbids mixed declarations and code [-Wdeclaration-after-statement]
>>>
>>> Instead we should switch to C99 and declare it as "for (int __nr_nodes" :P
>>
>> Hmm... I tried what you suggested, but compiler complains.
>>
>> 'for' loop initial declarations are only allowed in C99 or C11 mode
> 
> Yes, by "we should switch to C99" I meant that the kernel kbuild system would
> need to switch. Not a trivial change...
> Without that, I don't see how your patch is possible to do safely.

Vlastimil, thanks for pointing out future potential issues with this patch.
I likely would have missed that.

Wei, thanks for taking the time to put together the patch.  However, I tend
to agree with Vlastimil's assesment.  The cleanup is not worth the risk of
running into issues if someone uses multiple instances of the macro.
Wei Yang July 15, 2020, 3:46 a.m. UTC | #8
On Tue, Jul 14, 2020 at 02:12:03PM -0700, Mike Kravetz wrote:
>On 7/14/20 3:02 AM, Vlastimil Babka wrote:
>> On 7/14/20 11:57 AM, Wei Yang wrote:
>>> On Tue, Jul 14, 2020 at 11:22:03AM +0200, Vlastimil Babka wrote:
>>>> On 7/14/20 11:13 AM, Vlastimil Babka wrote:
>>>>> On 7/14/20 9:34 AM, Wei Yang wrote:
>>>>>> The second parameter of for_each_node_mask_to_[alloc|free] is a loop
>>>>>> variant, which is not used outside of loop iteration.
>>>>>>
>>>>>> Let's hide this.
>>>>>>
>>>>>> Signed-off-by: Wei Yang <richard.weiyang@linux.alibaba.com>
>>>>>> ---
>>>>>>  mm/hugetlb.c | 38 ++++++++++++++++++++------------------
>>>>>>  1 file changed, 20 insertions(+), 18 deletions(-)
>>>>>>
>>>>>> diff --git a/mm/hugetlb.c b/mm/hugetlb.c
>>>>>> index 57ece74e3aae..9c3d15fb317e 100644
>>>>>> --- a/mm/hugetlb.c
>>>>>> +++ b/mm/hugetlb.c
>>>>>> @@ -1196,17 +1196,19 @@ static int hstate_next_node_to_free(struct hstate *h, nodemask_t *nodes_allowed)
>>>>>>  	return nid;
>>>>>>  }
>>>>>>  
>>>>>> -#define for_each_node_mask_to_alloc(hs, nr_nodes, node, mask)		\
>>>>>> -	for (nr_nodes = nodes_weight(*mask);				\
>>>>>> -		nr_nodes > 0 &&						\
>>>>>> +#define for_each_node_mask_to_alloc(hs, node, mask)			\
>>>>>> +	int __nr_nodes;							\
>>>>>> +	for (__nr_nodes = nodes_weight(*mask);				\
>>>>>
>>>>> The problem with this is that if I use the macro twice in the same block, this
>>>>> will redefine __nr_nodes and fail to compile, no?
>>>>> In that case it's better to avoid setting up this trap, IMHO.
>>>>
>>>> Ah, and it will also generate the following warning, if the use of for_each*
>>>> macro is not the first thing after variable declarations, but there's another
>>>> statement before:
>>>>
>>>> warning: ISO C90 forbids mixed declarations and code [-Wdeclaration-after-statement]
>>>>
>>>> Instead we should switch to C99 and declare it as "for (int __nr_nodes" :P
>>>
>>> Hmm... I tried what you suggested, but compiler complains.
>>>
>>> 'for' loop initial declarations are only allowed in C99 or C11 mode
>> 
>> Yes, by "we should switch to C99" I meant that the kernel kbuild system would
>> need to switch. Not a trivial change...
>> Without that, I don't see how your patch is possible to do safely.
>
>Vlastimil, thanks for pointing out future potential issues with this patch.
>I likely would have missed that.
>
>Wei, thanks for taking the time to put together the patch.  However, I tend
>to agree with Vlastimil's assesment.  The cleanup is not worth the risk of
>running into issues if someone uses multiple instances of the macro.

Yep, thanks all for your feedback.

>-- 
>Mike Kravetz

Patch
diff mbox series

diff --git a/mm/hugetlb.c b/mm/hugetlb.c
index 57ece74e3aae..9c3d15fb317e 100644
--- a/mm/hugetlb.c
+++ b/mm/hugetlb.c
@@ -1196,17 +1196,19 @@  static int hstate_next_node_to_free(struct hstate *h, nodemask_t *nodes_allowed)
 	return nid;
 }
 
-#define for_each_node_mask_to_alloc(hs, nr_nodes, node, mask)		\
-	for (nr_nodes = nodes_weight(*mask);				\
-		nr_nodes > 0 &&						\
+#define for_each_node_mask_to_alloc(hs, node, mask)			\
+	int __nr_nodes;							\
+	for (__nr_nodes = nodes_weight(*mask);				\
+		__nr_nodes > 0 &&					\
 		((node = hstate_next_node_to_alloc(hs, mask)) || 1);	\
-		nr_nodes--)
+		__nr_nodes--)
 
-#define for_each_node_mask_to_free(hs, nr_nodes, node, mask)		\
-	for (nr_nodes = nodes_weight(*mask);				\
-		nr_nodes > 0 &&						\
+#define for_each_node_mask_to_free(hs, node, mask)			\
+	int __nr_nodes;							\
+	for (__nr_nodes = nodes_weight(*mask);				\
+		__nr_nodes > 0 &&					\
 		((node = hstate_next_node_to_free(hs, mask)) || 1);	\
-		nr_nodes--)
+		__nr_nodes--)
 
 #ifdef CONFIG_ARCH_HAS_GIGANTIC_PAGE
 static void destroy_compound_gigantic_page(struct page *page,
@@ -1403,7 +1405,7 @@  static void __free_huge_page(struct page *page)
 	 * reservation.  If the page was associated with a subpool, there
 	 * would have been a page reserved in the subpool before allocation
 	 * via hugepage_subpool_get_pages().  Since we are 'restoring' the
-	 * reservtion, do not call hugepage_subpool_put_pages() as this will
+	 * reservation, do not call hugepage_subpool_put_pages() as this will
 	 * remove the reserved page from the subpool.
 	 */
 	if (!restore_reserve) {
@@ -1760,10 +1762,10 @@  static int alloc_pool_huge_page(struct hstate *h, nodemask_t *nodes_allowed,
 				nodemask_t *node_alloc_noretry)
 {
 	struct page *page;
-	int nr_nodes, node;
+	int node;
 	gfp_t gfp_mask = htlb_alloc_mask(h) | __GFP_THISNODE;
 
-	for_each_node_mask_to_alloc(h, nr_nodes, node, nodes_allowed) {
+	for_each_node_mask_to_alloc(h, node, nodes_allowed) {
 		page = alloc_fresh_huge_page(h, gfp_mask, node, nodes_allowed,
 						node_alloc_noretry);
 		if (page)
@@ -1787,10 +1789,10 @@  static int alloc_pool_huge_page(struct hstate *h, nodemask_t *nodes_allowed,
 static int free_pool_huge_page(struct hstate *h, nodemask_t *nodes_allowed,
 							 bool acct_surplus)
 {
-	int nr_nodes, node;
+	int node;
 	int ret = 0;
 
-	for_each_node_mask_to_free(h, nr_nodes, node, nodes_allowed) {
+	for_each_node_mask_to_free(h, node, nodes_allowed) {
 		/*
 		 * If we're returning unused surplus pages, only examine
 		 * nodes with surplus pages.
@@ -2481,9 +2483,9 @@  int alloc_bootmem_huge_page(struct hstate *h)
 int __alloc_bootmem_huge_page(struct hstate *h)
 {
 	struct huge_bootmem_page *m;
-	int nr_nodes, node;
+	int node;
 
-	for_each_node_mask_to_alloc(h, nr_nodes, node, &node_states[N_MEMORY]) {
+	for_each_node_mask_to_alloc(h, node, &node_states[N_MEMORY]) {
 		void *addr;
 
 		addr = memblock_alloc_try_nid_raw(
@@ -2662,17 +2664,17 @@  static inline void try_to_free_low(struct hstate *h, unsigned long count,
 static int adjust_pool_surplus(struct hstate *h, nodemask_t *nodes_allowed,
 				int delta)
 {
-	int nr_nodes, node;
+	int node;
 
 	VM_BUG_ON(delta != -1 && delta != 1);
 
 	if (delta < 0) {
-		for_each_node_mask_to_alloc(h, nr_nodes, node, nodes_allowed) {
+		for_each_node_mask_to_alloc(h, node, nodes_allowed) {
 			if (h->surplus_huge_pages_node[node])
 				goto found;
 		}
 	} else {
-		for_each_node_mask_to_free(h, nr_nodes, node, nodes_allowed) {
+		for_each_node_mask_to_free(h, node, nodes_allowed) {
 			if (h->surplus_huge_pages_node[node] <
 					h->nr_huge_pages_node[node])
 				goto found;