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@xxxxxxxxxx>
> ---
> 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
