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

[Yunjeong Mun]

Hi Joshua, thanks for reviewing my patch and for your kind explanation.

On Tue,  4 Mar 2025 14:22:51 -0800 Joshua Hahn <joshua.hahnjy@xxxxxxxxx> wrote:
> 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@xxxxxxxxx> wrote:
> 
> > 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...]
> 			^^^^^^^^^^^^
> 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).

To prevent division by 0 errors, how about setting new_iw to 1 when it is first 
created, instead of setting it in the reduce function?

> 
> 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.

Thanks for considering my suggestion. I look forward to the v7.

Best regards,
Yunjeong
> 
> 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)
> 
>