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