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

[Joshua Hahn <joshua.hahnjy@gmail.com>]

Hi Yunjeong, sorry for the noise, but I have discovered another potential
concern that your patch introduces, which I have explained below. 

On Tue,  4 Mar 2025 13:56:11 -0800 Joshua Hahn <joshua.hahnjy@gmail.com> wrote:

> On Fri, 28 Feb 2025 15:39:55 +0900 Yunjeong Mun <yunjeong.mun@sk.com> 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...]
			^^^^^^^^^^^^
When I was originally writing the response, I missed reviewing the contents
inside this snipped section, which looks like this:
		if (!node_state(nid, N_MEMORY)) {
			new_iw[nid] = 1;
			continue;
		}
I introduced this check in v6 because without this, we end up with the
possibility of memoryless nodes having a 0 in the table, which can lead to some
problems down the line (e.g. div by 0 in alloc_pages_bulk_weighted_interleave).

Respectfully, I would prefer to write my own version that takes your
suggestion, as opposed to applying this patch directly on top of mine so that
we do not introduce the build error or the potential div0. However, v7 will
include your suggestion, so it will go through only one loop as opposed to two.

Thank you for your feedback again. I hope you have a great day!
Joshua

> > -		/*
> > -		 * 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).

[...snip...]

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