Re: [PATCH] Simple fix

[Lorenzo Stoakes <lorenzo.stoakes@oracle.com>]

On Tue, Jan 07, 2025 at 06:29:03PM +0800, Huacai Chen wrote:
> Hi, Lorenzo,
>
> On Tue, Jan 7, 2025 at 4:48 PM Lorenzo Stoakes
> <lorenzo.stoakes@oracle.com> wrote:
> >
> > On Tue, Jan 07, 2025 at 09:18:48AM +0800, Huacai Chen wrote:
> > > Hi, all, I like simple fixes, so is this acceptable (based on an early
> > > version from Koichiro Den)?
> >
> > No not at all. This is bizarre - in the mail you are replying to Koichiro
> > agrees with me that the approach of his code that you've sent here (I don't
> > know why) is fundamentally broken and suggest another.
> >
> > I am at a loss as to why you've sent this? Perhaps a miscommunication
> > somewhere? :)
> >
> > In any case, please don't send '[PATCH] xxx' mails that aren't intended to
> > be patches, a better form of this would be to say 'oh can we just do...'
> > then to put this code in the mail underneath, without any '[PATCH]' prefix.
> I wasn't in the CC list, and I also found the bug yesterday, so I can
> only reply to this email with "git send-email --in-reply-to". This is
> the reason why my reply looks so stranne.
>
> >
> > But please do review the discussion - the below is insufficient as simple
> > as it is (sadly) because the boot CPU's delayed work will never be
> > executed.
> Koichiro's simple fix causes the boot CPU's delayed work to never be
> executed, this is obvious, and of course I have read the early
> discussion. And so I improve it, with a "cpu_online()" checking, then
> the boot CPU is unaffected.

With respect Haucai, this is not how you engage in kernel discussion. You
could simply have replied to the mail or given more information, you now
have two people telling you this, please take it on board.

And it caused me to miss your actually quite valuable suggestion so this is
beneficial for all! :)

ANYWAY, moving on to the technical side:

>
> Huacai
>
> >
> > I will take a look at Koichiro's new approach as soon as I am able.
> >
> > Cheers!
> >
> > > ---
> > >  mm/vmstat.c | 7 ++++++-
> > >  1 file changed, 6 insertions(+), 1 deletion(-)
> > >
> > > diff --git a/mm/vmstat.c b/mm/vmstat.c
> > > index 0889b75cef14..1badc24a21ff 100644
> > > --- a/mm/vmstat.c
> > > +++ b/mm/vmstat.c
> > > @@ -2122,10 +2122,15 @@ static void __init start_shepherd_timer(void)
> > >  {
> > >       int cpu;
> > >
> > > -     for_each_possible_cpu(cpu)
> > > +     for_each_possible_cpu(cpu) {
> > >               INIT_DEFERRABLE_WORK(per_cpu_ptr(&vmstat_work, cpu),
> > >                       vmstat_update);
> > >
> > > +             /* Will be enabled on vmstat_cpu_online() */
> > > +             if (!cpu_online(cpu))

This might actually be workable as something simpler, on assumption there
can be no race here (I don't think so right?).

Koichiro - could you check this and see whether it resolves the issue and
whether you feel this is sensible?

Might be worth expanding comment to say that we disable on offline, enable
on online and we're just providing symmetry here.

> > > +                     disable_delayed_work_sync(&per_cpu(vmstat_work, cpu));
> > > +     }
> > > +
> > >       schedule_delayed_work(&shepherd,
> > >               round_jiffies_relative(sysctl_stat_interval));
> > >  }
> > > --
> > > 2.43.5
> > >
>

Cheers, Lorenzo