Re:Re: [PATCH V3] watchdog: mtk: support pre-timeout when the bark irq is available
>On 4/14/21 4:48 AM, Wang Qing wrote:
>> Use the bark interrupt as the pretimeout notifier if available.
>>
>> By default, the pretimeout notification shall occur one second earlier
>> than the timeout.
>>
>> V2:
>> - panic() by default if WATCHDOG_PRETIMEOUT_GOV is not enabled.
>>
>> V3:
>> - Modify the pretimeout behavior, manually reset after the pretimeout
>> - is processed and wait until timeout.
>>
>> Signed-off-by: Wang Qing
>> ---
>> drivers/watchdog/mtk_wdt.c | 62
>> ++
>> 1 file changed, 57 insertions(+), 5 deletions(-)
>>
>> diff --git a/drivers/watchdog/mtk_wdt.c b/drivers/watchdog/mtk_wdt.c
>> index 97ca993..7bef1e3
>> --- a/drivers/watchdog/mtk_wdt.c
>> +++ b/drivers/watchdog/mtk_wdt.c
>> @@ -25,6 +25,7 @@
>> #include
>> #include
>> #include
>> +#include
>>
>> #define WDT_MAX_TIMEOUT 31
>> #define WDT_MIN_TIMEOUT 1
>> @@ -234,18 +235,46 @@ static int mtk_wdt_start(struct watchdog_device
>> *wdt_dev)
>> void __iomem *wdt_base = mtk_wdt->wdt_base;
>> int ret;
>>
>> -ret = mtk_wdt_set_timeout(wdt_dev, wdt_dev->timeout);
>> +ret = mtk_wdt_set_timeout(wdt_dev, wdt_dev->timeout -
>> wdt_dev->pretimeout);
>> if (ret < 0)
>> return ret;
>>
>> reg = ioread32(wdt_base + WDT_MODE);
>> -reg &= ~(WDT_MODE_IRQ_EN | WDT_MODE_DUAL_EN);
>> +reg &= ~WDT_MODE_IRQ_EN;
>> +if (wdt_dev->pretimeout)
>> +reg |= WDT_MODE_IRQ_EN;
>> +else
>> +reg &= ~WDT_MODE_IRQ_EN;
>> reg |= (WDT_MODE_EN | WDT_MODE_KEY);
>> iowrite32(reg, wdt_base + WDT_MODE);
>>
>> return 0;
>> }
>>
>> +static int mtk_wdt_set_pretimeout(struct watchdog_device *wdd,
>> + unsigned int timeout)
>> +{
>> +wdd->pretimeout = timeout;
>> +return mtk_wdt_start(wdd);
>
>The watchdog is not necessarily active here.
>
>> +}
>> +
>> +static irqreturn_t mtk_wdt_isr(int irq, void *arg)
>> +{
>> +struct watchdog_device *wdd = arg;
>> +struct mtk_wdt_dev *mtk_wdt = watchdog_get_drvdata(wdd);
>> +void __iomem *wdt_base = mtk_wdt->wdt_base;
>> +
>> +watchdog_notify_pretimeout(wdd);
>> +/*
>> + * Guaranteed to be reset when the timeout
>> + * expires under any situations
>> + */
>> +mdelay(1000*wdd->pretimeout);
>
>That is not how this is supposed to work. The idea with a pretimeout is that
>the
>real watchdog reset will happen under all circumstances, and that executing
>the pretimeout (and changing some hardware registers) is not a prerequisite
>for the real timeout to happen. After all, the system could be stuck hard, with
>interrupts disabled.
>
>On top of that, just sleeping here while waiting for the real timeout and
>then resetting the system isn't the idea either. On a single core system this
>will just hang. On a multi-core system, who knows if userspace managed to ping
>the watchdog in the meantime.
>
>Unless there is a means to trigger the watchdog twice, without intervention,
>the first time generating an interrupt and the second time resetting the
>system,
>there is no way for this to work. I don't see how this chip really supports
>pretimeout. It seems that it supports either a hard reset or generating an
>interrupt on watchdog timeout, and there is only a single timeout.
>
>If you have a use case for generating an interrupt and resetting the system via
>software (ie panic) _instead_ of having it generate a hard reset, please feel
>free to submit a patch along that line, together with a description of its use
>case.
>
>Thanks,
>Guenter
>
Yes, as mentioned before, the behavior of WDT_MODE_IRQ_EN is use irq instead of
reset, so we must use WDT_MODE_IRQ_EN | WDT_MODE_DUAL_EN if like you said
"the first time generating an interrupt and the second time resetting the
system" .
The Dual mode is mentioned in the MTK datasheet:
In this mode, the watchdog will be AUTO-RESTART after interrupt is triggered.
AP need to clear WDT_STA after receiving interrupt from TOPRGU, or system reset
will be triggered after watchdog timer expires.
Instructions for use：
Set wdt_en = 1'b1.
Set dual_mode = 1'b1.
Set wdt_irq = 1'b1.
We can use Dual mode to achieve pretimeout behavior, only in this way can we
get more information during pretimeout processing, instead of directly
resetting.
Thanks,
Qing
Re:Re: [PATCH] watchdog: mtk: support pre-timeout when the bark irq is available
>On 4/9/21 8:11 PM, 王擎 wrote: >> >>> On 4/9/21 7:42 PM, 王擎 wrote: >>>> >>>>> On 4/9/21 2:55 AM, Wang Qing wrote: >>>>>> Use the bark interrupt as the pretimeout notifier if available. >>>>>> >>>>>> By default, the pretimeout notification shall occur one second earlier >>>>>> than the timeout. >>>>>> >>>>>> Signed-off-by: Wang Qing >>>>>> --- >>>>>> drivers/watchdog/mtk_wdt.c | 47 >>>>>> +++--- >>>>>> 1 file changed, 44 insertions(+), 3 deletions(-) >>>>>> >>>>>> diff --git a/drivers/watchdog/mtk_wdt.c b/drivers/watchdog/mtk_wdt.c >>>>>> index 97ca993..8b919cc >>>>>> --- a/drivers/watchdog/mtk_wdt.c >>>>>> +++ b/drivers/watchdog/mtk_wdt.c >>>>>> @@ -25,6 +25,7 @@ >>>>>> #include >>>>>> #include >>>>>> #include >>>>>> +#include >>>>>> >>>>>> #define WDT_MAX_TIMEOUT 31 >>>>>> #define WDT_MIN_TIMEOUT 1 >>>>>> @@ -234,18 +235,35 @@ static int mtk_wdt_start(struct watchdog_device >>>>>> *wdt_dev) >>>>>> void __iomem *wdt_base = mtk_wdt->wdt_base; >>>>>> int ret; >>>>>> >>>>>> -ret = mtk_wdt_set_timeout(wdt_dev, wdt_dev->timeout); >>>>>> +ret = mtk_wdt_set_timeout(wdt_dev, wdt_dev->timeout - >>>>>> wdt_dev->pretimeout); >>>>> >>>>> That looks suspiciously like the real watchdog won't happen at all. >>>>> What will happen if the pretimeout governor is set to none ? >>>>> >>>>> Guenter >>>>> >>>> The pretimeout governor is panic by default. If pretimeout is enabled and >>>> the governor is >>>> set to none, it means the timeout behavior does not need to be processed, >>>> only printing. >>>> >>> >>> That was not my question. My question was if the real timeout happens in >>> that case. >>> >>> Guenter >>> >> Yes, the real timeout will happen. After WDT timeout, IRQ is sent out >> instead of >> reset signal first. In order to ensure that CPU does not get stuck after IRQ >> is sent out, >> WDT will time again and send reset signal to reset. >> > >When will that be, or in other words how does the chip know when to time out ? >After all, only a single timeout value is written into the chip. I don't see >how >it would know to reset the chip after wdt_dev->timeout. > >Guenter > > Sorry I made a mistake, the IRQ is sent out first, then time again and reset only when WDT_MODE_IRQ_EN|WDT_MODE_DUAL_EN is enabled. WDT_MODE_DUAL_EN mode is a special mode, depending on the specific chip design. If WDT_MODE_IRQ_EN|~WDT_MODE_DUAL_EN as described in my patch, it just sent out IRQ instead of reset signal, the real timeout will not happen if the pretimeout governor is set to none. Also, ~(WDT_MODE_IRQ_EN | WDT_MODE_DUAL_EN) means send reset signal directly. It does not support pretimeout processing. Thanks Qing Wang
Re:Re: [PATCH] watchdog: mtk: support pre-timeout when the bark irq is available
>On 4/9/21 7:42 PM, 王擎 wrote: >> >>> On 4/9/21 2:55 AM, Wang Qing wrote: >>>> Use the bark interrupt as the pretimeout notifier if available. >>>> >>>> By default, the pretimeout notification shall occur one second earlier >>>> than the timeout. >>>> >>>> Signed-off-by: Wang Qing >>>> --- >>>> drivers/watchdog/mtk_wdt.c | 47 >>>> +++--- >>>> 1 file changed, 44 insertions(+), 3 deletions(-) >>>> >>>> diff --git a/drivers/watchdog/mtk_wdt.c b/drivers/watchdog/mtk_wdt.c >>>> index 97ca993..8b919cc >>>> --- a/drivers/watchdog/mtk_wdt.c >>>> +++ b/drivers/watchdog/mtk_wdt.c >>>> @@ -25,6 +25,7 @@ >>>> #include >>>> #include >>>> #include >>>> +#include >>>> >>>> #define WDT_MAX_TIMEOUT 31 >>>> #define WDT_MIN_TIMEOUT 1 >>>> @@ -234,18 +235,35 @@ static int mtk_wdt_start(struct watchdog_device >>>> *wdt_dev) >>>>void __iomem *wdt_base = mtk_wdt->wdt_base; >>>>int ret; >>>> >>>> - ret = mtk_wdt_set_timeout(wdt_dev, wdt_dev->timeout); >>>> + ret = mtk_wdt_set_timeout(wdt_dev, wdt_dev->timeout - >>>> wdt_dev->pretimeout); >>> >>> That looks suspiciously like the real watchdog won't happen at all. >>> What will happen if the pretimeout governor is set to none ? >>> >>> Guenter >>> >> The pretimeout governor is panic by default. If pretimeout is enabled and >> the governor is >> set to none, it means the timeout behavior does not need to be processed, >> only printing. >> > >That was not my question. My question was if the real timeout happens in that >case. > >Guenter > Yes, the real timeout will happen. After WDT timeout, IRQ is sent out instead of reset signal first. In order to ensure that CPU does not get stuck after IRQ is sent out, WDT will time again and send reset signal to reset. Thanks. Qing Wang.
Re:Re: [PATCH] softdog: make pretimeout available when SOFT_WATCHDOG_PRETIMEOUT enabled
>On 4/6/21 2:44 AM, Wang Qing wrote:
>> Although softdog supports pretimeout, there is no way to set pretimeout,
>> so pretimeout will never be processed in softdog_ping().
>>
>This is wrong. The pretimeout can be set using WDIOC_SETPRETIMEOUT, as with
>every other driver supporting it. There is no need for a module parameter.
>
>Guenter
>
I saw it, but if I use softdog and enable pretimeout, I hope to get pre
processing
by default instead of fire. Can we give pretimeout a minimum default value?
Thanks.
Qing wang.
>> Here add the configuration mechanism for pretimeout and the default value
>> is 1 second, so when CONFIG_SOFT_WATCHDOG_PRETIMEOUT is enabled, the
>> pretimeout function defaults available.
>>
>> Signed-off-by: Wang Qing
>> ---
>> drivers/watchdog/softdog.c | 11 +++
>> 1 file changed, 11 insertions(+)
>>
>> diff --git a/drivers/watchdog/softdog.c b/drivers/watchdog/softdog.c
>> index 7a10962..79e52791
>> --- a/drivers/watchdog/softdog.c
>> +++ b/drivers/watchdog/softdog.c
>> @@ -35,6 +35,14 @@ MODULE_PARM_DESC(soft_margin,
>> "Watchdog soft_margin in seconds. (0 < soft_margin < 65536, default="
>> __MODULE_STRING(TIMER_MARGIN) ")");
>>
>> +#ifdef CONFIG_SOFT_WATCHDOG_PRETIMEOUT
>> +#define PRE_TIMER_MARGIN1 /* Default is 1 seconds */
>> +static unsigned int soft_pretimeout = PRE_TIMER_MARGIN; /* in seconds */
>> +module_param(soft_pretimeout, uint, 0);
>> +MODULE_PARM_DESC(soft_pretimeout,
>> +"Watchdog soft_pretimeout in seconds. (0 < soft_pretimeout <
>> soft_margin, default=1)");
>> +#endif
>> +
>> static bool nowayout = WATCHDOG_NOWAYOUT;
>> module_param(nowayout, bool, 0);
>> MODULE_PARM_DESC(nowayout,
>> @@ -177,6 +185,9 @@ static struct watchdog_device softdog_dev = {
>> .min_timeout = 1,
>> .max_timeout = 65535,
>> .timeout = TIMER_MARGIN,
>> +#ifdef CONFIG_SOFT_WATCHDOG_PRETIMEOUT
>> +.pretimeout = PRE_TIMER_MARGIN,
>> +#endif
>> };
>>
>> static int __init softdog_init(void)
>>
>
Re:Re: [PATCH] watchdog: mtk: support pre-timeout when the bark irq is available
>On 4/9/21 2:55 AM, Wang Qing wrote: >> Use the bark interrupt as the pretimeout notifier if available. >> >> By default, the pretimeout notification shall occur one second earlier >> than the timeout. >> >> Signed-off-by: Wang Qing >> --- >> drivers/watchdog/mtk_wdt.c | 47 >> +++--- >> 1 file changed, 44 insertions(+), 3 deletions(-) >> >> diff --git a/drivers/watchdog/mtk_wdt.c b/drivers/watchdog/mtk_wdt.c >> index 97ca993..8b919cc >> --- a/drivers/watchdog/mtk_wdt.c >> +++ b/drivers/watchdog/mtk_wdt.c >> @@ -25,6 +25,7 @@ >> #include >> #include >> #include >> +#include >> >> #define WDT_MAX_TIMEOUT 31 >> #define WDT_MIN_TIMEOUT 1 >> @@ -234,18 +235,35 @@ static int mtk_wdt_start(struct watchdog_device >> *wdt_dev) >> void __iomem *wdt_base = mtk_wdt->wdt_base; >> int ret; >> >> -ret = mtk_wdt_set_timeout(wdt_dev, wdt_dev->timeout); >> +ret = mtk_wdt_set_timeout(wdt_dev, wdt_dev->timeout - >> wdt_dev->pretimeout); > >That looks suspiciously like the real watchdog won't happen at all. >What will happen if the pretimeout governor is set to none ? > >Guenter > The pretimeout governor is panic by default. If pretimeout is enabled and the governor is set to none, it means the timeout behavior does not need to be processed, only printing. Thanks. Qing Wang.
Re:[PATCH V3,RESEND] workqueue/watchdog: Make unbound workqueues aware of touch_softlockup_watchdog()
>V3:
>- Modify the commit message clearly according to Petr's suggestion.
>
>Signed-off-by: Wang Qing
>---
> kernel/watchdog.c | 5 +++--
> kernel/workqueue.c | 17 ++---
> 2 files changed, 9 insertions(+), 13 deletions(-)
>
>diff --git a/kernel/watchdog.c b/kernel/watchdog.c
>index 7110906..107bc38
>--- a/kernel/watchdog.c
>+++ b/kernel/watchdog.c
>@@ -278,9 +278,10 @@ void touch_all_softlockup_watchdogs(void)
>* update as well, the only side effect might be a cycle delay for
>* the softlockup check.
>*/
>- for_each_cpu(cpu, _allowed_mask)
>+ for_each_cpu(cpu, _allowed_mask) {
> per_cpu(watchdog_touch_ts, cpu) = SOFTLOCKUP_RESET;
>- wq_watchdog_touch(-1);
>+ wq_watchdog_touch(cpu);
>+ }
> }
>
> void touch_softlockup_watchdog_sync(void)
>diff --git a/kernel/workqueue.c b/kernel/workqueue.c
>index 0d150da..be08295
>--- a/kernel/workqueue.c
>+++ b/kernel/workqueue.c
>@@ -5787,22 +5787,17 @@ static void wq_watchdog_timer_fn(struct timer_list
>*unused)
> continue;
>
> /* get the latest of pool and touched timestamps */
>+ if (pool->cpu >= 0)
>+ touched = READ_ONCE(per_cpu(wq_watchdog_touched_cpu,
>pool->cpu));
>+ else
>+ touched = READ_ONCE(wq_watchdog_touched);
> pool_ts = READ_ONCE(pool->watchdog_ts);
>- touched = READ_ONCE(wq_watchdog_touched);
>
> if (time_after(pool_ts, touched))
> ts = pool_ts;
> else
> ts = touched;
>
>- if (pool->cpu >= 0) {
>- unsigned long cpu_touched =
>- READ_ONCE(per_cpu(wq_watchdog_touched_cpu,
>-pool->cpu));
>- if (time_after(cpu_touched, ts))
>- ts = cpu_touched;
>- }
>-
> /* did we stall? */
> if (time_after(jiffies, ts + thresh)) {
> lockup_detected = true;
>@@ -5826,8 +5821,8 @@ notrace void wq_watchdog_touch(int cpu)
> {
> if (cpu >= 0)
> per_cpu(wq_watchdog_touched_cpu, cpu) = jiffies;
>- else
>- wq_watchdog_touched = jiffies;
>+
>+ wq_watchdog_touched = jiffies;
> }
>
> static void wq_watchdog_set_thresh(unsigned long thresh)
>--
>2.7.4
>
Hi Petr:
Can you give a reviewed tag, or pick it up to workqueue tree?
Thanks,
Qing
Re:Re: Re: Re: [PATCH V2] workqueue: watchdog: update wq_watchdog_touched for unbound lockup checking
>On Wed 2021-03-24 10:16:46, 王擎 wrote:
>>
>> >On Tue 2021-03-23 20:37:35, 王擎 wrote:
>> >>
>> >> >On Fri 2021-03-19 16:00:36, Wang Qing wrote:
>> >> >> When touch_softlockup_watchdog() is called, only
>> >> >> wq_watchdog_touched_cpu
>> >> >> updated, while the unbound worker_pool running on its core uses
>> >> >> wq_watchdog_touched to determine whether locked up. This may be
>> >> >> mischecked.
>> >> >
>> >> >By other words, unbound workqueues are not aware of the more common
>> >> >touch_softlockup_watchdog() because it updates only
>> >> >wq_watchdog_touched_cpu for the affected CPU. As a result,
>> >> >the workqueue watchdog might report lockup in unbound workqueue
>> >> >even though it is blocked by a known slow code.
>> >>
>> >> Yes, this is the problem I'm talking about.
>> >
>> >I thought more about it. This patch prevents a false positive.
>> >Could it bring an opposite problem and hide real problems?
>> >
>> >I mean that an unbound workqueue might get blocked on CPU A
>> >because of a real softlockup. But we might not notice it because
>> >CPU B is touched. Well, there are other ways how to detect
>> >this situation, e.g. the softlockup watchdog.
>> >
>> >
>> >> >> My suggestion is to update both when touch_softlockup_watchdog() is
>> >> >> called,
>> >> >> use wq_watchdog_touched_cpu to check bound, and use
>> >> >> wq_watchdog_touched
>> >> >> to check unbound worker_pool.
>> >> >>
>> >> >> Signed-off-by: Wang Qing
>> >> >> ---
>> >> >> kernel/watchdog.c | 5 +++--
>> >> >> kernel/workqueue.c | 17 ++---
>> >> >> 2 files changed, 9 insertions(+), 13 deletions(-)
>> >> >>
>> >> >> diff --git a/kernel/watchdog.c b/kernel/watchdog.c
>> >> >> index 7110906..107bc38
>> >> >> --- a/kernel/watchdog.c
>> >> >> +++ b/kernel/watchdog.c
>> >> >> @@ -278,9 +278,10 @@ void touch_all_softlockup_watchdogs(void)
>> >> >> * update as well, the only side effect might be a cycle delay
>> >> >> for
>> >> >> * the softlockup check.
>> >> >> */
>> >> >> - for_each_cpu(cpu, _allowed_mask)
>> >> >> + for_each_cpu(cpu, _allowed_mask) {
>> >> >>per_cpu(watchdog_touch_ts, cpu) = SOFTLOCKUP_RESET;
>> >> >> - wq_watchdog_touch(-1);
>> >> >> + wq_watchdog_touch(cpu);
>> >> >
>> >> >Note that wq_watchdog_touch(cpu) newly always updates
>> >> >wq_watchdog_touched. This cycle will set the same jiffies
>> >> >value cpu-times to the same variable.
>> >> >
>> >> Although there is a bit of redundancy here, but the most concise way of
>> >> implementation, and it is certain that it will not affect performance.
>> >>
>> Another way to implement is wq_watchdog_touch() remain unchanged, but need
>> to modify touch_softlockup_watchdog() and touch_all_softlockup_watchdogs():
>> notrace void touch_softlockup_watchdog(void)
>> {
>> touch_softlockup_watchdog_sched();
>> wq_watchdog_touch(raw_smp_processor_id());
>> + wq_watchdog_touch(-1);
>> }
>> void touch_all_softlockup_watchdogs(void)
>> * update as well, the only side effect might be a cycle delay for
>> * the softlockup check.
>> */
>> - for_each_cpu(cpu, _allowed_mask)
>> +for_each_cpu(cpu, _allowed_mask) {
>> per_cpu(watchdog_touch_ts, cpu) = SOFTLOCKUP_RESET;
>> + wq_watchdog_touch(cpu);
>> + }
>> wq_watchdog_touch(-1);
>> }
>> So wq_watchdog_touched will not get updated many times,
>> which do you think is better, Petr?
>
>I actually prefer the original patch. It makes wq_watchdog_touch()
>easy to use. The complexity is hidden in wq-specific code.
>
>The alternative way updates each timestamp only once but the use
>is more complicated. IMHO, it is more error prone.
I agree, so I will just modify the commit log based on V2 and resend.
Thanks,
Qing
>
>Best Regards,
>Petr
Re:Re: Re: [PATCH V2] workqueue: watchdog: update wq_watchdog_touched for unbound lockup checking
>On Tue 2021-03-23 20:37:35, 王擎 wrote:
>>
>> >On Fri 2021-03-19 16:00:36, Wang Qing wrote:
>> >> When touch_softlockup_watchdog() is called, only wq_watchdog_touched_cpu
>> >> updated, while the unbound worker_pool running on its core uses
>> >> wq_watchdog_touched to determine whether locked up. This may be
>> >> mischecked.
>> >
>> >By other words, unbound workqueues are not aware of the more common
>> >touch_softlockup_watchdog() because it updates only
>> >wq_watchdog_touched_cpu for the affected CPU. As a result,
>> >the workqueue watchdog might report lockup in unbound workqueue
>> >even though it is blocked by a known slow code.
>>
>> Yes, this is the problem I'm talking about.
>
>I thought more about it. This patch prevents a false positive.
>Could it bring an opposite problem and hide real problems?
>
>I mean that an unbound workqueue might get blocked on CPU A
>because of a real softlockup. But we might not notice it because
>CPU B is touched. Well, there are other ways how to detect
>this situation, e.g. the softlockup watchdog.
>
>
>> >> My suggestion is to update both when touch_softlockup_watchdog() is
>> >> called,
>> >> use wq_watchdog_touched_cpu to check bound, and use wq_watchdog_touched
>> >> to check unbound worker_pool.
>> >>
>> >> Signed-off-by: Wang Qing
>> >> ---
>> >> kernel/watchdog.c | 5 +++--
>> >> kernel/workqueue.c | 17 ++---
>> >> 2 files changed, 9 insertions(+), 13 deletions(-)
>> >>
>> >> diff --git a/kernel/watchdog.c b/kernel/watchdog.c
>> >> index 7110906..107bc38
>> >> --- a/kernel/watchdog.c
>> >> +++ b/kernel/watchdog.c
>> >> @@ -278,9 +278,10 @@ void touch_all_softlockup_watchdogs(void)
>> >>* update as well, the only side effect might be a cycle delay for
>> >>* the softlockup check.
>> >>*/
>> >> - for_each_cpu(cpu, _allowed_mask)
>> >> + for_each_cpu(cpu, _allowed_mask) {
>> >> per_cpu(watchdog_touch_ts, cpu) = SOFTLOCKUP_RESET;
>> >> - wq_watchdog_touch(-1);
>> >> + wq_watchdog_touch(cpu);
>> >
>> >Note that wq_watchdog_touch(cpu) newly always updates
>> >wq_watchdog_touched. This cycle will set the same jiffies
>> >value cpu-times to the same variable.
>> >
>> Although there is a bit of redundancy here, but the most concise way of
>> implementation, and it is certain that it will not affect performance.
>>
Another way to implement is wq_watchdog_touch() remain unchanged, but need
to modify touch_softlockup_watchdog() and touch_all_softlockup_watchdogs():
notrace void touch_softlockup_watchdog(void)
{
touch_softlockup_watchdog_sched();
wq_watchdog_touch(raw_smp_processor_id());
+ wq_watchdog_touch(-1);
}
void touch_all_softlockup_watchdogs(void)
* update as well, the only side effect might be a cycle delay for
* the softlockup check.
*/
- for_each_cpu(cpu, _allowed_mask)
+ for_each_cpu(cpu, _allowed_mask) {
per_cpu(watchdog_touch_ts, cpu) = SOFTLOCKUP_RESET;
+ wq_watchdog_touch(cpu);
+ }
wq_watchdog_touch(-1);
}
So wq_watchdog_touched will not get updated many times,
which do you think is better, Petr?
>> >>
>> >> void touch_softlockup_watchdog_sync(void)
>> >> diff --git a/kernel/workqueue.c b/kernel/workqueue.c
>> >> index 0d150da..be08295
>> >> --- a/kernel/workqueue.c
>> >> +++ b/kernel/workqueue.c
>> >> @@ -5826,8 +5821,8 @@ notrace void wq_watchdog_touch(int cpu)
>> >> {
>> >> if (cpu >= 0)
>> >> per_cpu(wq_watchdog_touched_cpu, cpu) = jiffies;
>> >> - else
>> >> - wq_watchdog_touched = jiffies;
>> >> +
>> >> + wq_watchdog_touched = jiffies;
>> >> }
>> >>
>> >> static void wq_watchdog_set_thresh(unsigned long thresh)
>> >
>> >This last hunk is enough to fix the problem. wq_watchdog_touched will
>> >get updated also from cpu-specific touch_softlockup_watchdog().
>>
>> Not enough in fact, because wq_watchdog_touched was updated in
>> wq_watchdog_touch(),
>> so wq_watchdog_touched can no longer be used to judge the bound pool, we
>> must update
>> every wq_watchdog_touched_cpu in touch_all_softlockup_watchdogs() for bound
>> judgment.
>
>I see. Your patch makes sense.
>
>I would just improve the
Re:Re: [PATCH V2] workqueue: watchdog: update wq_watchdog_touched for unbound lockup checking
>On Fri 2021-03-19 16:00:36, Wang Qing wrote:
>> When touch_softlockup_watchdog() is called, only wq_watchdog_touched_cpu
>> updated, while the unbound worker_pool running on its core uses
>> wq_watchdog_touched to determine whether locked up. This may be mischecked.
>
>By other words, unbound workqueues are not aware of the more common
>touch_softlockup_watchdog() because it updates only
>wq_watchdog_touched_cpu for the affected CPU. As a result,
>the workqueue watchdog might report lockup in unbound workqueue
>even though it is blocked by a known slow code.
Yes, this is the problem I'm talking about.
>
>> My suggestion is to update both when touch_softlockup_watchdog() is called,
>> use wq_watchdog_touched_cpu to check bound, and use wq_watchdog_touched
>> to check unbound worker_pool.
>>
>> Signed-off-by: Wang Qing
>> ---
>> kernel/watchdog.c | 5 +++--
>> kernel/workqueue.c | 17 ++---
>> 2 files changed, 9 insertions(+), 13 deletions(-)
>>
>> diff --git a/kernel/watchdog.c b/kernel/watchdog.c
>> index 7110906..107bc38
>> --- a/kernel/watchdog.c
>> +++ b/kernel/watchdog.c
>> @@ -278,9 +278,10 @@ void touch_all_softlockup_watchdogs(void)
>> * update as well, the only side effect might be a cycle delay for
>> * the softlockup check.
>> */
>> -for_each_cpu(cpu, _allowed_mask)
>> +for_each_cpu(cpu, _allowed_mask) {
>> per_cpu(watchdog_touch_ts, cpu) = SOFTLOCKUP_RESET;
>> -wq_watchdog_touch(-1);
>> +wq_watchdog_touch(cpu);
>
>Note that wq_watchdog_touch(cpu) newly always updates
>wq_watchdog_touched. This cycle will set the same jiffies
>value cpu-times to the same variable.
>
Although there is a bit of redundancy here, but the most concise way of
implementation, and it is certain that it will not affect performance.
>> +}
>> }
>>
>> void touch_softlockup_watchdog_sync(void)
>> diff --git a/kernel/workqueue.c b/kernel/workqueue.c
>> index 0d150da..be08295
>> --- a/kernel/workqueue.c
>> +++ b/kernel/workqueue.c
>> @@ -5787,22 +5787,17 @@ static void wq_watchdog_timer_fn(struct timer_list
>> *unused)
>> continue;
>>
>> /* get the latest of pool and touched timestamps */
>> +if (pool->cpu >= 0)
>> +touched = READ_ONCE(per_cpu(wq_watchdog_touched_cpu,
>> pool->cpu));
>> +else
>> +touched = READ_ONCE(wq_watchdog_touched);
>> pool_ts = READ_ONCE(pool->watchdog_ts);
>> -touched = READ_ONCE(wq_watchdog_touched);
>>
>> if (time_after(pool_ts, touched))
>> ts = pool_ts;
>> else
>> ts = touched;
>>
>> -if (pool->cpu >= 0) {
>> -unsigned long cpu_touched =
>> -READ_ONCE(per_cpu(wq_watchdog_touched_cpu,
>> - pool->cpu));
>> -if (time_after(cpu_touched, ts))
>> -ts = cpu_touched;
>> -}
>> -
>> /* did we stall? */
>> if (time_after(jiffies, ts + thresh)) {
>> lockup_detected = true;
>> @@ -5826,8 +5821,8 @@ notrace void wq_watchdog_touch(int cpu)
>> {
>> if (cpu >= 0)
>> per_cpu(wq_watchdog_touched_cpu, cpu) = jiffies;
>> -else
>> -wq_watchdog_touched = jiffies;
>> +
>> +wq_watchdog_touched = jiffies;
>> }
>>
>> static void wq_watchdog_set_thresh(unsigned long thresh)
>
>This last hunk is enough to fix the problem. wq_watchdog_touched will
>get updated also from cpu-specific touch_softlockup_watchdog().
Not enough in fact, because wq_watchdog_touched was updated in
wq_watchdog_touch(),
so wq_watchdog_touched can no longer be used to judge the bound pool, we must
update
every wq_watchdog_touched_cpu in touch_all_softlockup_watchdogs() for bound
judgment.
Thanks,
Qing Wang
>
>The original patch simplified the logic of wq_watchdog_timer_fn().
>But it added un-necessary assignments into
>touch_all_softlockup_watchdogs(void).
>
>I do not have strong opinion what solution is better. I slightly
>prefer to keep only this last hunk.
>
>Best Regards,
>Petr
Re:Re: [PATCH V2] workqueue: watchdog: update wq_watchdog_touched for unbound lockup checking
>On Fri, Mar 19, 2021 at 04:00:36PM +0800, Wang Qing wrote: >> When touch_softlockup_watchdog() is called, only wq_watchdog_touched_cpu >> updated, while the unbound worker_pool running on its core uses >> wq_watchdog_touched to determine whether locked up. This may be mischecked. > >Can you please elaborate here, preferably with a concrete scenario where the >new code is better? The previous code is problematic for judging whether the unbound pool is locked up: When the expected single cpu stall occurs, only wq_watchdog_touched_cpu is updated, However, the unbound pool uses wq_watchdog_touched to determine whether it is locked up, so when the unbound pool is running in a scenario where the cpu stall is expected, a misjudgment will occur, because wq_watchdog_touched only be update when all the cpu stall as expect. So we need to update wq_watchdog_touched in touch_softlockup_watchdog(), and it is only used for unbound pool, it will not affect the bound pool in any way. Thanks, Qing > >Thanks. > >-- >tejun
Re:Re: [PATCH] futex: use wake_up_process() instead of wake_up_state()
>> On Fri, 2021-03-19 at 10:59 +0800, Wang Qing wrote: >> > Using wake_up_process() is more simpler and friendly, >> > and it is more convenient for analysis and statistics >> >> I likely needn't bother, and don't have a NAK to paste on this thing, >> but here's another copy of my NOPE for yet another gratuitous change >> with complete BS justification. > >Let me try a bit softer tone. I think you're trying to help, but >ignoring feedback is not the way to achieve that goal. My feedback was >and remains that your change is not an improvement, it's churn, but >more importantly, that changes require technical justification, which >you did not provide. You were subsequently handed the justification >you lacked by none other than the maintainer of the code you were >modifying. He told you that your change could become a tiny kernel >size optimization by converting like instances all in one patch.. which >you promptly ignored, instead submitting multiple patches with zero >justification. That is not the path to success. Thank you for your reply. There are two reasons for sending patch again. One is that I think this is only an improvement in format and has no substantial impact, so no verification is required. The second one is that I want to hear more opinions from the maintainer. Because the entire kernel may have similar problems, I have to figure out whether this is a tacit behavior. Also, I don't understand what you mean by "your change could become a tiny kernel size optimization by converting like instances all in one patch". Thanks, WangQing. > >> >> > >> > Signed-off-by: Wang Qing >> > --- >> > kernel/futex.c | 2 +- >> > 1 file changed, 1 insertion(+), 1 deletion(-) >> > >> > diff --git a/kernel/futex.c b/kernel/futex.c >> > index e68db77..078a1f9 >> > --- a/kernel/futex.c >> > +++ b/kernel/futex.c >> > @@ -1820,7 +1820,7 @@ void requeue_pi_wake_futex(struct futex_q *q, union >> > futex_key *key, >> > >> >q->lock_ptr = >lock; >> > >> > - wake_up_state(q->task, TASK_NORMAL); >> > + wake_up_process(q->task); >> > } >> > >> > /** >> >
Re:Re: [PATCH] dma-buf: use wake_up_process() instead of wake_up_state()
>> Using wake_up_process() is more simpler and friendly, >> and it is more convenient for analysis and statistics >> >> Signed-off-by: Wang Qing > >Reviewed-by: Christian König > >Should I pick it up or do you want to push it through some other tree >than DRM? Pick it up just fine, thanks, WangQing. > >Thanks, >Christian.
Re:Re: [PATCH] sched: swait: use wake_up_process() instead of wake_up_state()
>> >> * Mike Galbraith wrote: >> >> > On Tue, 2021-03-16 at 19:20 +0800, Wang Qing wrote: >> > > Why not just use wake_up_process(). >> > >> > IMO this is not an improvement. There are other places where explicit >> > TASK_NORMAL is used as well, and they're all perfectly clear as is. >> >> Arguably those could all be converted to wake_up_process() as well. >> It's a very small kernel code size optimization. There's about 3 such >> places, could be converted in a single patch. > >It's still pointless churn IMO. Using wake_up_process() is more simpler and friendly for beginners, and it is more convenient for analysis and statistics.
Re:Re: [PATCH] arc: kernel: Return -EFAULT if copy_to_user() fails
Subject: Re: [PATCH] arc: kernel: Return -EFAULT if copy_to_user() fails>On 3/1/21 4:05 AM, Wang Qing wrote: >> The copy_to_user() function returns the number of bytes remaining to be >> copied, but we want to return -EFAULT if the copy doesn't complete. >> >> Signed-off-by: Wang Qing > >Acked-by: Vineet Gupta > >Do you want me to pick this up via ARC tree ? Yes, thank you for picking this up. Wangqing > >Thx, >-Vineet
Re:Re: [PATCH V4 net-bugfixs] net/ethernet: Update ret when ptp_clock is ERROR
>On Thu, 12 Nov 2020 09:15:05 +0800 (GMT+08:00) 王擎 wrote: >> >Grygorii, would you mind sending a correct patch in so Wang Qing can >> >see how it's done? I've been asking for a fixes tag multiple times >> >already :( >> >> I still don't quite understand what a fixes tag means， >> can you tell me how to do this, thanks. > >Please read: Documentation/process/submitting-patches.rst > >You can search for "Fixes:" I see, but this bug is not caused by a specific patch, it exists at the beginning, so there is no need to add a fixes tag. Please point out if I understand it incorrectly,thanks! Wang Qing
Re:Re: [PATCH V4 net-bugfixs] net/ethernet: Update ret when ptp_clock is ERROR
>> On Wed, Nov 11, 2020 at 03:24:33PM +0200, Grygorii Strashko wrote: >> > >> > Following Richard's comments v1 of the patch has to be applied [1]. >> > I've also added my Reviewed-by there. >> > >> > [1] https://lore.kernel.org/patchwork/patch/1334067/ >> >> +1 >> >> Jakub, can you please take the original v1 of this patch? > >I don't think v1 builds cleanly folks (not 100% sure, cpts is not >compiled on x86): > > ret = cpts->ptp_clock ? cpts->ptp_clock : (-ENODEV); > >ptp_clock is a pointer, ret is an integer, right? yeah, I will modify like: ret = cpts->ptp_clock ? PTR_ERR(cpts->ptp_clock) : -ENODEV; > >Grygorii, would you mind sending a correct patch in so Wang Qing can >see how it's done? I've been asking for a fixes tag multiple times >already :( I still don't quite understand what a fixes tag means， can you tell me how to do this, thanks. Wang Qing
Re:Re: [PATCH] sched/rt, powerpc: Prepare for PREEMPT_RT
>Quoting Wang Qing :
>
>> Add PREEMPT_RT output to die().
>>
>> Signed-off-by: Wang Qing
>> ---
>> arch/powerpc/kernel/traps.c | 10 +-
>> 1 file changed, 9 insertions(+), 1 deletion(-)
>>
>> diff --git a/arch/powerpc/kernel/traps.c b/arch/powerpc/kernel/traps.c
>> index 5006dcb..6dfe567
>> --- a/arch/powerpc/kernel/traps.c
>> +++ b/arch/powerpc/kernel/traps.c
>> @@ -258,6 +258,14 @@ static char *get_mmu_str(void)
>> return "";
>> }
>>
>> +#ifdef CONFIG_PREEMPT
>> +#define S_PREEMPT " PREEMPT"
>> +#elif defined(CONFIG_PREEMPT_RT)
>> +#define S_PREEMPT " PREEMPT_RT"
>> +#else
>> +#define S_PREEMPT ""
>> +#endif
>
>I don't like too much that forest of #ifdefs. IS_ENABLED() is prefered
>whenever possible.
>
>> +
>> static int __die(const char *str, struct pt_regs *regs, long err)
>> {
>> printk("Oops: %s, sig: %ld [#%d]\n", str, err, ++die_counter);
>> @@ -265,7 +273,7 @@ static int __die(const char *str, struct pt_regs
>> *regs, long err)
>> printk("%s PAGE_SIZE=%luK%s%s%s%s%s%s %s\n",
>> IS_ENABLED(CONFIG_CPU_LITTLE_ENDIAN) ? "LE" : "BE",
>> PAGE_SIZE / 1024, get_mmu_str(),
>> - IS_ENABLED(CONFIG_PREEMPT) ? " PREEMPT" : "",
>> + S_PREEMPT,
>> IS_ENABLED(CONFIG_SMP) ? " SMP" : "",
>> IS_ENABLED(CONFIG_SMP) ? (" NR_CPUS=" __stringify(NR_CPUS)) : "",
>> debug_pagealloc_enabled() ? " DEBUG_PAGEALLOC" : "",
>> --
>> 2.7.4
>
>I'd prefer to remain in line with the existing and use IS_ENABLED()
>instead of #ifdefs, see below:
>
>diff --git a/arch/powerpc/kernel/traps.c b/arch/powerpc/kernel/traps.c
>index 5006dcbe1d9f..dec7b81c72a4 100644
>--- a/arch/powerpc/kernel/traps.c
>+++ b/arch/powerpc/kernel/traps.c
>@@ -262,10 +262,11 @@ static int __die(const char *str, struct pt_regs
>*regs, long err)
> {
> printk("Oops: %s, sig: %ld [#%d]\n", str, err, ++die_counter);
>
>- printk("%s PAGE_SIZE=%luK%s%s%s%s%s%s %s\n",
>+ printk("%s PAGE_SIZE=%luK%s%s%s%s%s%s%s %s\n",
> IS_ENABLED(CONFIG_CPU_LITTLE_ENDIAN) ? "LE" : "BE",
> PAGE_SIZE / 1024, get_mmu_str(),
> IS_ENABLED(CONFIG_PREEMPT) ? " PREEMPT" : "",
>+ IS_ENABLED(CONFIG_PREEMPT_RT) ? " PREEMPT_RT" : "",
> IS_ENABLED(CONFIG_SMP) ? " SMP" : "",
> IS_ENABLED(CONFIG_SMP) ? (" NR_CPUS=" __stringify(NR_CPUS)) : "",
> debug_pagealloc_enabled() ? " DEBUG_PAGEALLOC" : "",
>---
>Christophe
Yeah, I agree with it.
Wang Qing
Re:Re: [PATCH] usb: Assign NULL to phy that may be returned
>> Assign initial values to local variables that may be returned
>>
>> Signed-off-by: Wang Qing
>
>Your subject, and body of text, seem to have 2 "odd" characters in it,
>please fix up.
>
>Also, your subject and changelog body here are identical, please be much
>more verbose in the body explaining why you are doing something, not
>just what you are doing.
>
>And your subject line should also match other patches for this file, and
>have "usb: phy: ..." in the beginning.
Yeah, I got it.
>
>> ---
>> drivers/usb/phy/phy.c | 2 +-
>> 1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/drivers/usb/phy/phy.c b/drivers/usb/phy/phy.c
>> index b47285f..de21967
>> --- a/drivers/usb/phy/phy.c
>> +++ b/drivers/usb/phy/phy.c
>> @@ -59,7 +59,7 @@ static struct usb_phy *__usb_find_phy(struct list_head
>> *list,
>>
>> static struct usb_phy *__of_usb_find_phy(struct device_node *node)
>> {
>> -struct usb_phy *phy;
>> +struct usb_phy *phy = NULL;
>
>Why isn't the compiler complaining about this today? Are you sure this
>is needed?
Sorry, I didn't look at it carefully, because __usb_find_phy has an initial
value,
and I was affected.. You don't need to modify it, In fact.
thanks,
Wang Qing
>
>thanks,
>
>greg k-h
Re:Re: [PATCH V4] doc: zh_CN: add translatation for btrfs
主题：Re: [PATCH V4] doc: zh_CN: add translatation for btrfs> > >在 2020/9/25 下午4:19, Alex Shi 写道: >> Reviewed-by: Alex Shi > >Sorry, this patch still has format issue and can not be applied. >I would take back the reviewed-by. > >Thanks >Alex Sorry for wasting your time, I sent the patch through send-email. The previous problem was caused by encoding. This patch format should be good, the format issue you mentioned should be caused by another patch I submitted (doc: zh_CN: add translatation for tmpfs) depend on this patch, I will update that patch again. ,Please review this patch again. Thanks Qing >> >> 在 2020/9/25 下午3:22, Wang Qing 写道: >>> Translate Documentation/filesystems/btrfs.rst into Chinese. >>> >>> Signed-off-by: Wang Qing >>> --- >>> .../translations/zh_CN/filesystems/btrfs.rst | 37 >>> ++ >>> .../translations/zh_CN/filesystems/index.rst | 1 + >>> 2 files changed, 38 insertions(+) >>> create mode 100644 Documentation/translations/zh_CN/filesystems/btrfs.rst >>> >>> diff --git a/Documentation/translations/zh_CN/filesystems/btrfs.rst >>> b/Documentation/translations/zh_CN/filesystems/btrfs.rst >>> new file mode 100644 >>> index 000..8b8cca2 >>> --- /dev/null >>> +++ b/Documentation/translations/zh_CN/filesystems/btrfs.rst >>> @@ -0,0 +1,37 @@ >>> +.. SPDX-License-Identifier: GPL-2.0 >>> + >>> +.. include:: ../disclaimer-zh_CN.rst >>> + >>> +:Original: :ref:`Documentation/filesystems/ext3.rst ` >>> + >>> +translated by 王擎 Wang Qing >>> + >>> += >>> +BTRFS >>> += >>> + >>> +Btrfs是一个写时复制更新的文件系统，它注重容错、修复和易于管理。 >>> +Btrfs由多家公司联合开发，并获得GPL许可，免费开放给所有人。 >>> + >>> +Btrfs的主要功能包括： >>> + >>> +*扩展大小的文件存储（文件最大支持2^64） >>> +*填充方式使小文件更节省空间 >>> +*索引目录的方式更节省空间 >>> +*动态的索引节点分配方式 >>> +*可写快照的特性 >>> +*支持子卷（独立的内部根文件系统） >>> +*对象级别的镜像克隆 >>> +*基于数据和元数据的校验和（支持多种算法） >>> +*支持压缩 >>> +*內建多种磁盘阵列算法，支持多种设备 >>> +*支持离线的文件系统检查 >>> +*高效的增量备份和文件系统镜像 >>> +*在线文件系统碎片整理 >>> + >>> +更多有关信息，请参阅Wiki >>> + >>> + https://btrfs.wiki.kernel.org >>> + >>> +维护信息包含管理任务、常见问题、用例、挂载选项、变更日志、 >>> +特性、手册、源码仓、联系人等。 >>> diff --git a/Documentation/translations/zh_CN/filesystems/index.rst >>> b/Documentation/translations/zh_CN/filesystems/index.rst >>> index 186501d..47e86397 >>> --- a/Documentation/translations/zh_CN/filesystems/index.rst >>> +++ b/Documentation/translations/zh_CN/filesystems/index.rst >>> @@ -25,4 +25,5 @@ Linux Kernel中的文件系统 >>> >>> virtiofs >>> debugfs >>> + btrfs >>> >>>
Re:Re: [PATCH] drivers\block: Use kobj_to_dev() API
Subject: Re: [PATCH] drivers\block: Use kobj_to_dev() API>On Fri, Jun 12, 2020
at 03:10:56PM +0800, Wang Qing wrote:
>> Use kobj_to_dev() API instead of container_of().
>>
>> Signed-off-by: Wang Qing
>> ---
>> drivers/block/virtio_blk.c | 2 +-
>> 1 file changed, 1 insertion(+), 1 deletion(-)
>> mode change 100644 => 100755 drivers/block/virtio_blk.c
>
>
>Subject should probably use "/". Besides that - trivial tree?
Sorry, I will modify subject using "/". But what do you mean about trivial tree?
>
>>
>> diff --git a/drivers/block/virtio_blk.c b/drivers/block/virtio_blk.c
>> index 9d21bf0..c808405
>> --- a/drivers/block/virtio_blk.c
>> +++ b/drivers/block/virtio_blk.c
>> @@ -630,7 +630,7 @@ static struct attribute *virtblk_attrs[] = {
>> static umode_t virtblk_attrs_are_visible(struct kobject *kobj,
>> struct attribute *a, int n)
>> {
>> -struct device *dev = container_of(kobj, struct device, kobj);
>> +struct device *dev = kobj_to_dev(kobj);
>> struct gendisk *disk = dev_to_disk(dev);
>> struct virtio_blk *vblk = disk->private_data;
>> struct virtio_device *vdev = vblk->vdev;
>> --
>> 2.7.4
>
