Message-ID: <CAEQmJ=gCZBv1Mg6dxvWe5fRpFnrL9A_JM-1pB+eX9M-5v4xH7A@mail.gmail.com>
Date: Fri, 6 Sep 2024 18:03:33 +0800
From: Yuanhan Zhang <zyhtheonly@...il.com>
To: Doug Anderson <dianders@...omium.org>
Cc: 张元瀚 Tio Zhang <tiozhang@...iglobal.com>, 
	"akpm@...ux-foundation.org" <akpm@...ux-foundation.org>, 
	"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>, "tglx@...utronix.de" <tglx@...utronix.de>, 
	"zyhtheonly@...h.net" <zyhtheonly@...h.net>, 
	"john.ogness@...utronix.de" <john.ogness@...utronix.de>, 
	"lizhe.67@...edance.com" <lizhe.67@...edance.com>, "mcgrof@...nel.org" <mcgrof@...nel.org>, 
	"linux@...ssschuh.net" <linux@...ssschuh.net>, "kjlx@...pleofstupid.com" <kjlx@...pleofstupid.com>, 
	"mingo@...hat.com" <mingo@...hat.com>, "peterz@...radead.org" <peterz@...radead.org>, 
	"juri.lelli@...hat.com" <juri.lelli@...hat.com>, 
	"vincent.guittot@...aro.org" <vincent.guittot@...aro.org>, 
	"dietmar.eggemann@....com" <dietmar.eggemann@....com>, "rostedt@...dmis.org" <rostedt@...dmis.org>, 
	"bsegall@...gle.com" <bsegall@...gle.com>, "mgorman@...e.de" <mgorman@...e.de>, 
	"bristot@...hat.com" <bristot@...hat.com>, "vschneid@...hat.com" <vschneid@...hat.com>
Subject: Re: [PATCH] watchdog: when watchdog_enabled is 0, let
 (soft,nmi)switch remain 1 after we read them in proc

Hi Doug,

Doug Anderson <dianders@...omium.org> 于2024年9月5日周四 03:39写道：
>
> Hi,
>
> On Wed, Sep 4, 2024 at 2:23 AM 张元瀚 Tio Zhang <tiozhang@...iglobal.com> wrote:
> >
> > Hi,
> >
> > Ping :)
> >
> > // get_maintainer.pl does not tell who are the maintainers or reviewers of kernel/watchdog.c
> > Add commit signers and sched maintainers to the CC list.
>
> FWIW, people in the Linux community usually don't like "top posting"...
>
> In any case, I saw your original patch but I struggled to make sense
> of the explanation and so I left it for later and then never got back
> to it. I suspect that the explanation needs to be a little clearer.

Thanks for your time reviewing and sorry for my poor explanation,
I update the explanation with an example to reproduce the issue.
Please see [patch v2]

>
>
> > On 8/22/24 下午3:09, "张元瀚 Tio Zhang" <tiozhang@...iglobal.com <mailto:tiozhang@...iglobal.com>> wrote:
> >
> >
> > For users set "watchdog_user_enabled=0" but remaining
> > "(soft,nmi)watchdog_user_enabled=1". Watchdog threads(,nmi watchdog)
> > rework only if users reset "watchdog_user_enabled=1" without printing
> > those watchdog swicthes. Otherwise (soft,nmi)watchdog_user_enabled
> > will turn to 0 because of printing their values (It makes sense to print 0
> > since they do not work any more, but it does not make sense to let user's
> > swicthes change to 0 only by prints).
> >
> >
> > And after that, watchdog only should work again by doing:
> > (soft,nmi)watchdog_user_enabled=1
> > *** can't print, or everything go back to 0 again ***
> > watchdog_user_enabled=1
> >
> >
> > So this patch fixes this situation:
> >
> >
> > | name | value
> > |----------------------------|--------------------------
> > | watchdog_enabled | 0
> > |----------------------------|--------------------------
> > | nmi_watchdog_user_enabled | 1
> > |----------------------------|--------------------------
> > | soft_watchdog_user_enabled | 1
> > |----------------------------|--------------------------
> > | watchdog_user_enabled | 0
> > |----------------------------|--------------------------
> >
> >
> > cat /proc/sys/kernel/*watchdog
> > |
> > |
> > V
> > | name | value
> > |----------------------------|--------------------------
> > | watchdog_enabled | 0
> > |----------------------------|--------------------------
> > | nmi_watchdog_user_enabled | 0
> > |----------------------------|--------------------------
> > | soft_watchdog_user_enabled | 0
> > |----------------------------|--------------------------
> > | watchdog_user_enabled | 0
> > |----------------------------|--------------------------
> >
> >
> >
> >
> > Signed-off-by: Tio Zhang <tiozhang@...iglobal.com <mailto:tiozhang@...iglobal.com>>
> > ---
> > kernel/watchdog.c | 10 ++++++++++
> > 1 file changed, 10 insertions(+)
> >
> >
> > diff --git a/kernel/watchdog.c b/kernel/watchdog.c
> > index 51915b44ac73..42e69e83e76d 100644
> > --- a/kernel/watchdog.c
> > +++ b/kernel/watchdog.c
> > @@ -995,8 +995,18 @@ static int proc_watchdog_common(int which, struct ctl_table *table, int write,
> > * On read synchronize the userspace interface. This is a
> > * racy snapshot.
> > */
> > + old = READ_ONCE(*param);
>
> Given that the first thing that both the "if" and "else" case do now
> is to set "old" then it feels like it could move outside the "if"
> statement.

Done in [patch v2]

>
> Even though the existing code does it, I'm also a little doubtful that
> a READ_ONCE() is really needed here. You're protected by a mutex so
> you don't need to worry about other CPUs and it would be really
> confusing to me if the compiler could optimize things in a way that
> the READ_ONCE() was needed.

I'm also confused, the only reason I keep 'READ_ONCE' is to align with
existing code. I remove 'READ_ONCE' in [patch v2].

>
>
> > *param = (watchdog_enabled & which) != 0;
> > err = proc_dointvec_minmax(table, write, buffer, lenp, ppos);
> > + /*
> > + * When "old" is 1 and watchdog_enabled is 0,
> > + * it should not be change to 0 for printing
> > + * nmi_watchdog_user_enabled or soft_watchdog_user_enabled.
> > + * So after we print it as 0,
> > + * we should recover it to 1.
> > + */
> > + if (old && !watchdog_enabled)
> > + *param = old;
>
> I'm confused. Why all this extra logic? This should just
> _unconditionally_ restore "*param" to "old", right? The only reason
> the code hacked "*param" in the first place was so that it could use
> the common proc_dointvec_minmax() helper function. Aside from the
> common helper function working there is never a reason to muck with
> "*param" if "!write" so it should just always restore.

Yes actually the if statement is useless, my original purpose writing this
is trying to make the explanation more clear...
Remove in [patch v2]

>
> -Doug

Thanks,

Powered by blists - more mailing lists