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 <[email protected]>
---
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)
> 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 <[email protected]>
> ---
> 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 <[email protected]>
>
>
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 <[email protected]>
> ---
> 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;
>
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 <[email protected]>
>> ---
>> 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
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 <[email protected]>
>>> ---
>>> 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
Help you, Help me
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 <[email protected]>
>>> ---
>>> 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
--
Wei Yang
Help you, Help me
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 <[email protected]>
>>>> ---
>>>> 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.
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 <[email protected]>
>>>>> ---
>>>>> 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.
--
Mike Kravetz
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 <[email protected]>
>>>>>> ---
>>>>>> 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
--
Wei Yang
Help you, Help me