linux-kernel - Re: [PATCH] mm/hugetlb: hide nr_nodes in the internal of for_each_node_mask_to

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <cd1651a0-35c6-7535-5782-ce7e31cc26e8@suse.cz>
Date:   Tue, 14 Jul 2020 11:13:14 +0200
From:   Vlastimil Babka <vbabka@...e.cz>
To:     Wei Yang <richard.weiyang@...ux.alibaba.com>,
        mike.kravetz@...cle.com, akpm@...ux-foundation.org
Cc:     linux-mm@...ck.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH] mm/hugetlb: hide nr_nodes in the internal of
 for_each_node_mask_to_[alloc|free]

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@...ux.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;
>