Re: [PATCH] Simple fix

[Koichiro Den <koichiro.den@canonical.com>]

On Wed, Jan 08, 2025 at 10:31:40AM +0800, Huacai Chen wrote:
> Hi, Koichiro,
> 
> On Wed, Jan 8, 2025 at 10:22 AM Koichiro Den <koichiro.den@canonical.com> wrote:
> >
> > On Tue, Jan 07, 2025 at 11:00:10AM +0000, Lorenzo Stoakes wrote:
> > > 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
> >
> > Sorry about late response. And thank you very much, Huacai. Your suggestion
> > looks great. Much simpler and intuitive.
> >
> > > can be no race here (I don't think so right?).
> >
> > IIUC, there is no race since smp_init() always runs before init_mm_internals().
> > At this stage (i.e. start_shepherd_timer()), for cpuhp scenario, the secondary
> > CPUs haven’t even been brought up yet. So the simple 'cpu_online' check looks
> > simple and the best.
> >
> > >
> > > Koichiro - could you check this and see whether it resolves the issue and
> > > whether you feel this is sensible?
> >
> > I tested it and it seems to work well both on booting and cpuhp events.
> >
> > (By the way, in my previous email comment [1], I forgot to mention that I also
> > applied other unmerged patches, [2] and [3], just to be able to run such random
> > transisioning test. This time here, I just tested by switching CPUHP_ONLINE <->
> > CPUHP_OFFLINE only.)
> >
> > [1]: https://lore.kernel.org/linux-mm/7ed97096-859e-46d0-8f27-16a2298a8914@lucifer.local/T/#m11d983715699d3cea295b8618aba7b6ccec4db55
> > [2]: https://lore.kernel.org/all/20241220134421.3809834-1-koichiro.den@canonical.com/
> > [3]: https://lore.kernel.org/all/20241220141538.4018232-1-koichiro.den@canonical.com/
> >
> > >
> > > Might be worth expanding comment to say that we disable on offline, enable
> > > on online and we're just providing symmetry here.
> >
> > Sounds good. Like this?
> >
> >     -       for_each_possible_cpu(cpu)
> >     +       for_each_possible_cpu(cpu) {
> >                     INIT_DEFERRABLE_WORK(per_cpu_ptr(&vmstat_work, cpu),
> >                             vmstat_update);
> >
> >     +               /*
> >     +                * For secondary CPUs during CPU hotplug scenarios,
> >     +                * vmstat_cpu_online() will enable the work.
> >     +                * mm/vmstat:online enables and disables vmstat_work
> >     +                * symmetrically during CPU hotplug events.
> >     +                */
> >     +               if (!cpu_online(cpu))
> >     +                       disable_delayed_work_sync(&per_cpu(vmstat_work, cpu));
> >     +       }
> >
> >
> > Before submitting a revised patch, I'd like to confirm:
> > Huacai, would you be comfortable with me adding your Signed-off-by to the
> > commit, or would you prefer a Suggested-by tag instead?
> Since the original V2 patch has been reverted in mainline, so I think
> you will send a V3 which integrate my suggestion, right?

I was considering sending a separate patch with a title distinct from the
original, while referencing both the original (v2) patch and the revert
commit. However, I'm fine with either approach.

If there’s any documentation that mentions a recommended method in this
kind of situation, please let me know. Personally, I’m not a fan of
multiple commits with identical titles appearing in a branch.

> 
> Both Signed-off-by and Suggested-by is OK for me.

Alright, thanks!

-Koichiro Den

> 
> Huacai
> 
> >
> > Thanks.
> >
> > -Koichiro Den
> >
> > >
> > > > > > +                     disable_delayed_work_sync(&per_cpu(vmstat_work, cpu));
> > > > > > +     }
> > > > > > +
> > > > > >       schedule_delayed_work(&shepherd,
> > > > > >               round_jiffies_relative(sysctl_stat_interval));
> > > > > >  }
> > > > > > --
> > > > > > 2.43.5
> > > > > >
> > > >
> > >
> > > Cheers, Lorenzo