Re: [PATCH v3 02/12] mm/khugepaged: add struct collapse_control

[Peter Xu <peterx@redhat.com>]

On Tue, Apr 26, 2022 at 07:44:02AM -0700, Zach O'Keefe wrote:
> Modularize hugepage collapse by introducing struct collapse_control.
> This structure serves to describe the properties of the requested
> collapse, as well as serve as a local scratch pad to use during the
> collapse itself.

Reasonable, but IMHO worth mentioning that "we're introducing the context
by first moving the per-node statistics into ...", or something like that.

> 
> Signed-off-by: Zach O'Keefe <zokeefe@google.com>
> ---
>  mm/khugepaged.c | 79 ++++++++++++++++++++++++++++---------------------
>  1 file changed, 46 insertions(+), 33 deletions(-)
> 
> diff --git a/mm/khugepaged.c b/mm/khugepaged.c
> index 2933b13fc975..9d42fa330812 100644
> --- a/mm/khugepaged.c
> +++ b/mm/khugepaged.c
> @@ -86,6 +86,14 @@ static struct kmem_cache *mm_slot_cache __read_mostly;
>  
>  #define MAX_PTE_MAPPED_THP 8
>  
> +struct collapse_control {
> +	/* Num pages scanned per node */
> +	int node_load[MAX_NUMNODES];
> +
> +	/* Last target selected in khugepaged_find_target_node() for this scan */

Not really important, but.. iiuc this is not only for this scan but for all
scans of the khugepaged thread.

> +	int last_target_node;
> +};

[...]

> @@ -829,28 +835,28 @@ static inline gfp_t alloc_hugepage_khugepaged_gfpmask(void)
>  }
>  
>  #ifdef CONFIG_NUMA
> -static int khugepaged_find_target_node(void)
> +static int khugepaged_find_target_node(struct collapse_control *cc)
>  {
> -	static int last_khugepaged_target_node = NUMA_NO_NODE;
>  	int nid, target_node = 0, max_value = 0;
>  
>  	/* find first node with max normal pages hit */
>  	for (nid = 0; nid < MAX_NUMNODES; nid++)
> -		if (khugepaged_node_load[nid] > max_value) {
> -			max_value = khugepaged_node_load[nid];
> +		if (cc->node_load[nid] > max_value) {
> +			max_value = cc->node_load[nid];
>  			target_node = nid;
>  		}
>  
>  	/* do some balance if several nodes have the same hit record */
> -	if (target_node <= last_khugepaged_target_node)
> -		for (nid = last_khugepaged_target_node + 1; nid < MAX_NUMNODES;
> -				nid++)
> -			if (max_value == khugepaged_node_load[nid]) {
> +	if (target_node <= cc->last_target_node)
> +		for (nid = cc->last_target_node + 1; nid < MAX_NUMNODES;
> +		     nid++) {
> +			if (max_value == cc->node_load[nid]) {
>  				target_node = nid;
>  				break;
>  			}
> +		}

I'm not sure what's the coding style for this case, but IIUC we could
either add both (to both outer "if" and "for") or none; adding one pair of
brackets seems a bit odd.

>  
> -	last_khugepaged_target_node = target_node;
> +	cc->last_target_node = target_node;
>  	return target_node;
>  }

-- 
Peter Xu