Re: [PATCH 1/2 v6] mm/mempolicy: Weighted Interleave Auto-tuning

[Joshua Hahn]

On Fri, 28 Feb 2025 15:39:55 +0900 Yunjeong Mun <yunjeong.mun@xxxxxx> wrote:

Hi Yunjeong,

While applying your patch, I realized that it re-introduces a build error
that was fixed in v6, which I am noting below. 

> Hi, Joshua. 

[...snip...]
 
> In my understanding, new_iw[nid] values are scaled twice, first to 100 and then to a 
> weightines value of 32. I think this scaling can be done just once, directly 
> to weightness value as follows:
> 
> diff --git a/mm/mempolicy.c b/mm/mempolicy.c
> index 50cbb7c047fa..65a7e2baf161 100644
> --- a/mm/mempolicy.c
> +++ b/mm/mempolicy.c
> @@ -176,47 +176,22 @@ static u8 get_il_weight(int node)
>  static void reduce_interleave_weights(unsigned int *bw, u8 *new_iw)
>  {
> 	u64 sum_bw = 0;
> -	unsigned int cast_sum_bw, sum_iw = 0;
> -	unsigned int scaling_factor = 1, iw_gcd = 1;
> +	unsigned int scaling_factor = 1, iw_gcd = 0;
> 	int nid;
> 
> 	/* Recalculate the bandwidth distribution given the new info */
> 	for_each_node_state(nid, N_MEMORY)
> 		sum_bw += bw[nid];
> 
> -       for (nid = 0; nid < nr_node_ids; nid++) {
>  			[...snip...]
> -		/*
> -		 * Try not to perform 64-bit division.
> -		 * If sum_bw < scaling_factor, then sum_bw < U32_MAX.
> -		 * If sum_bw > scaling_factor, then bw[nid] is less than
> -		 * 1% of the total bandwidth. Round up to 1%.
> -		 */
>  			[...snip...]

We cannot remove this part here, since this is what allows us to divide
in the next for loop below. sum_bw is a u64, so performing division
by this value will create a build error for 32-bit machines. I've gone and
re-added this comment and parts to the bottom part; the logic should not
change at all from the patch that you proposed (except for the build error).

It's not a big deal, but I just wanted to note that this patch was not applied
in its entirety in the v7 of my patch, in case you look at v7 and see
that the code looks different from your branch.

Honggyu also suggested that I add a Co-developed-by tag, which I am happy
to do. However, this requires a subsequent Signed-off-by tag as well, as
per the kerneldoc on patches [1]. I just wanted to have your explicit
signed-off-by so that I could add it to the patch.

> -		sum_iw += new_iw[nid];
> -	}
> -
>      
> 	/*
> 	 * Scale each node's share of the total bandwidth from percentages
> 	 * to whole numbers in the range [1, weightiness]
> 	 */
> 	for_each_node_state(nid, N_MEMORY) {
> -		scaling_factor = weightiness * new_iw[nid];
> -		new_iw[nid] = max(scaling_factor / sum_iw, 1);
> -		if (nid == 0)
> -			iw_gcd = new_iw[0];
> +		scaling_factor = weightiness * bw[nid];
> +		new_iw[nid] = max(scaling_factor / sum_bw, 1);
						 ^^^^^^^^
						This causes a build error for
						32 bit machines

[...snip...]

> Please let me know how you think about this.
> 
> Best regards,
> Yunjeong

Thanks again Yunjeong, I hope you have a great day!
Joshua

[1] https://www.kernel.org/doc/html/v4.17/process/submitting-patches.html#when-to-use-acked-by-cc-and-co-developed-by

Sent using hkml (https://github.com/sjp38/hackermail)