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

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

Hi Yunjeong,

On Wed,  5 Mar 2025 18:49:11 +0900 Yunjeong Mun <yunjeong.mun@sk.com> wrote:

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

[...snip...]

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

I think this makes sense. The original motivation for including it in
reduce_interleave_weights is because this function is usually called on newly
allocated tables, so I thought I would just combine the functionality of
initializing the table and reducing weights into one function. Howver, I now
see that there are actually a few spots when either a table is initialized
but this function isn't called, or when an already-initialized table is
given to this function.

The other rationale was that it seems a bit silly to go through and set all
weights to 1, and then immediately overwrite them with the reduced interleave
weights. With that said, none of this code is in any critical section, I'm
sure that going through one more iteration and setting the weights to 1 is
not unreasonable. 

[...snip...]

> > 
> > 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 again for your suggestions, Yunjeong! I'll re-write the code
to incorporate them in v7. I hope you have a great day!
Joshua

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