Re: [PATCH RT 1/2] tasklet: Address a race resulting in double-enqueue
On Thu, Apr 23, 2020 at 11:55 PM wrote:
>
> From: Zhang Xiao
>
> v4.19.115-rt49-rc1 stable review patch.
> If anyone has any objections, please let me know.
>
> ---
>
>
> The kernel bugzilla has the following race condition reported:
>
> CPU0CPU1CPU2
>
> test_set SCHED
> test_set RUN
>if SCHED
> add_list
> raise
> clear RUN
>
> test_set RUN
> test_clear SCHED
> ->func
> test_set SCHED
> tasklet_try_unlock ->0
> test_clear SCHED
> test_set SCHED
> ->func
> tasklet_try_unlock ->1
> test_set RUN
> if SCHED
> add list
> raise
> clear RUN
> test_set RUN
> if SCHED
> add list
> raise
> clear RUN
>
> As a result the tasklet is enqueued on both CPUs and run on both CPUs. Due
> to the nature of the list used here, it is possible that further
> (different) tasklets, which are enqueued after this double-enqueued
> tasklet, are scheduled on CPU2 but invoked on CPU1. It is also possible
> that these tasklets won't be invoked at all, because during the second
> enqueue process the t->next pointer is set to NULL - dropping everything
> from the list.
>
> This race will trigger one or two of the WARN_ON() in
> tasklet_action_common().
> The problem is that the tasklet may be invoked multiple times and clear
> SCHED bit on each invocation. This makes it possible to enqueue the
> very same tasklet on different CPUs.
>
> Current RT-devel is using the upstream implementation which does not
> re-run tasklets if they have SCHED set again and so it does not clear
> the SCHED bit multiple times on a single invocation.
>
> Introduce the CHAINED flag. The tasklet will only be enqueued if the
> CHAINED flag has been set successfully.
> If it is possible to exchange the flags (CHAINED | RUN) -> 0 then the
> tasklet won't be re-run. Otherwise the possible SCHED flag is removed
> and the tasklet is re-run again.
>
> Bugzilla: https://bugzilla.kernel.org/show_bug.cgi?id=61451
> Not-signed-off-by: Zhang Xiao
> [bigeasy: patch description]
> Signed-off-by: Sebastian Andrzej Siewior
>
> Signed-off-by: Tom Zanussi
> ---
> include/linux/interrupt.h | 5 -
> kernel/softirq.c | 6 +-
> 2 files changed, 9 insertions(+), 2 deletions(-)
>
> diff --git a/include/linux/interrupt.h b/include/linux/interrupt.h
> index 97d9ba26915e..a3b5edb26bc5 100644
> --- a/include/linux/interrupt.h
> +++ b/include/linux/interrupt.h
> @@ -579,12 +579,15 @@ enum
> {
> TASKLET_STATE_SCHED,/* Tasklet is scheduled for execution */
> TASKLET_STATE_RUN, /* Tasklet is running (SMP only) */
> - TASKLET_STATE_PENDING /* Tasklet is pending */
> + TASKLET_STATE_PENDING, /* Tasklet is pending */
> + TASKLET_STATE_CHAINED /* Tasklet is chained */
> };
>
> #define TASKLET_STATEF_SCHED (1 << TASKLET_STATE_SCHED)
> #define TASKLET_STATEF_RUN (1 << TASKLET_STATE_RUN)
> #define TASKLET_STATEF_PENDING (1 << TASKLET_STATE_PENDING)
> +#define TASKLET_STATEF_CHAINED (1 << TASKLET_STATE_CHAINED)
> +#define TASKLET_STATEF_RC (TASKLET_STATEF_RUN | TASKLET_STATEF_CHAINED)
>
> #if defined(CONFIG_SMP) || defined(CONFIG_PREEMPT_RT_FULL)
> static inline int tasklet_trylock(struct tasklet_struct *t)
> diff --git a/kernel/softirq.c b/kernel/softirq.c
> index 25bcf2f2714b..73dae64bfc9c 100644
> --- a/kernel/softirq.c
> +++ b/kernel/softirq.c
> @@ -947,6 +947,10 @@ static void __tasklet_schedule_common(struct
> tasklet_struct *t,
> * is locked before adding it to the list.
> */
> if (test_bit(TASKLET_STATE_SCHED, &t->state)) {
> + if (test_and_set_bit(TASKLET_STATE_CHAINED, &t->state)) {
> + tasklet_unlock(t);
> + return;
> + }
> t->next = NULL;
> *head->tail = t;
> head->tail = &(t->next);
> @@ -1040,7 +1044,7 @@ static void tasklet_action_common(struct softirq_action
> *a,
> again:
> t->func(t->data);
>
> - while (!tasklet_tryunlock(t)) {
> + while (cmpxchg(&t->state, TASKLET_STATEF_RC, 0) !=
> TASKLET_STATEF_RC) {
> /*
> * If it got disabled meanwhile, bail out:
> */
> --
> 2.17.1
>
Hi, This patch introduced a regression in our kernel
(v4.19.124-rt53-rebase), It occurs when we're jumping to crush kernel
using kexec, in the initialization of the emmc driver.
I'm still debugging the root cause, but I thought of mentioning this
in the mailing list if you have a
Re: [PATCH RT 1/2] tasklet: Address a race resulting in double-enqueue
Hi Ramon,
On Thu, 2020-06-04 at 19:04 +0300, Ramon Fried wrote:
> On Thu, Apr 23, 2020 at 11:55 PM wrote:
> >
> > From: Zhang Xiao
> >
> > v4.19.115-rt49-rc1 stable review patch.
> > If anyone has any objections, please let me know.
> >
> > ---
> >
> >
> > The kernel bugzilla has the following race condition reported:
> >
> > CPU0CPU1CPU2
> >
> > test_set SCHED
> > test_set RUN
> >if SCHED
> > add_list
> > raise
> > clear RUN
> >
> > test_set RUN
> > test_clear SCHED
> > ->func
> > test_set SCHED
> > tasklet_try_unlock ->0
> > test_clear SCHED
> > test_set SCHED
> > ->func
> > tasklet_try_unlock ->1
> > test_set RUN
> > if SCHED
> > add list
> > raise
> > clear RUN
> > test_set RUN
> > if SCHED
> > add list
> > raise
> > clear RUN
> >
> > As a result the tasklet is enqueued on both CPUs and run on both
> > CPUs. Due
> > to the nature of the list used here, it is possible that further
> > (different) tasklets, which are enqueued after this double-enqueued
> > tasklet, are scheduled on CPU2 but invoked on CPU1. It is also
> > possible
> > that these tasklets won't be invoked at all, because during the
> > second
> > enqueue process the t->next pointer is set to NULL - dropping
> > everything
> > from the list.
> >
> > This race will trigger one or two of the WARN_ON() in
> > tasklet_action_common().
> > The problem is that the tasklet may be invoked multiple times and
> > clear
> > SCHED bit on each invocation. This makes it possible to enqueue the
> > very same tasklet on different CPUs.
> >
> > Current RT-devel is using the upstream implementation which does
> > not
> > re-run tasklets if they have SCHED set again and so it does not
> > clear
> > the SCHED bit multiple times on a single invocation.
> >
> > Introduce the CHAINED flag. The tasklet will only be enqueued if
> > the
> > CHAINED flag has been set successfully.
> > If it is possible to exchange the flags (CHAINED | RUN) -> 0 then
> > the
> > tasklet won't be re-run. Otherwise the possible SCHED flag is
> > removed
> > and the tasklet is re-run again.
> >
> > Bugzilla: https://bugzilla.kernel.org/show_bug.cgi?id=61451
> > Not-signed-off-by: Zhang Xiao
> > [bigeasy: patch description]
> > Signed-off-by: Sebastian Andrzej Siewior
> >
> > Signed-off-by: Tom Zanussi
> > ---
> > include/linux/interrupt.h | 5 -
> > kernel/softirq.c | 6 +-
> > 2 files changed, 9 insertions(+), 2 deletions(-)
> >
> > diff --git a/include/linux/interrupt.h b/include/linux/interrupt.h
> > index 97d9ba26915e..a3b5edb26bc5 100644
> > --- a/include/linux/interrupt.h
> > +++ b/include/linux/interrupt.h
> > @@ -579,12 +579,15 @@ enum
> > {
> > TASKLET_STATE_SCHED,/* Tasklet is scheduled for
> > execution */
> > TASKLET_STATE_RUN, /* Tasklet is running (SMP only) */
> > - TASKLET_STATE_PENDING /* Tasklet is pending */
> > + TASKLET_STATE_PENDING, /* Tasklet is pending */
> > + TASKLET_STATE_CHAINED /* Tasklet is chained */
> > };
> >
> > #define TASKLET_STATEF_SCHED (1 << TASKLET_STATE_SCHED)
> > #define TASKLET_STATEF_RUN (1 << TASKLET_STATE_RUN)
> > #define TASKLET_STATEF_PENDING (1 << TASKLET_STATE_PENDING)
> > +#define TASKLET_STATEF_CHAINED (1 << TASKLET_STATE_CHAINED)
> > +#define TASKLET_STATEF_RC (TASKLET_STATEF_RUN |
> > TASKLET_STATEF_CHAINED)
> >
> > #if defined(CONFIG_SMP) || defined(CONFIG_PREEMPT_RT_FULL)
> > static inline int tasklet_trylock(struct tasklet_struct *t)
> > diff --git a/kernel/softirq.c b/kernel/softirq.c
> > index 25bcf2f2714b..73dae64bfc9c 100644
> > --- a/kernel/softirq.c
> > +++ b/kernel/softirq.c
> > @@ -947,6 +947,10 @@ static void __tasklet_schedule_common(struct
> > tasklet_struct *t,
> > * is locked before adding it to the list.
> > */
> > if (test_bit(TASKLET_STATE_SCHED, &t->state)) {
> > + if (test_and_set_bit(TASKLET_STATE_CHAINED, &t-
> > >state)) {
> > + tasklet_unlock(t);
> > + return;
> > + }
> > t->next = NULL;
> > *head->tail = t;
> > head->tail = &(t->next);
> > @@ -1040,7 +1044,7 @@ static void tasklet_action_common(struct
> > softirq_action *a,
> > again:
> > t->func(t->data);
> >
> > - while (!tasklet_tryunlock(t)) {
> > + while (cmpxchg(&t->state, TASKLET_STATEF_RC, 0) !=
> > TASKLET_STATEF_RC) {
> > /*
> >
Re: [PATCH RT 1/2] tasklet: Address a race resulting in double-enqueue
On 2020-06-04 15:51:14 [-0500], Tom Zanussi wrote: > > > > Hi, This patch introduced a regression in our kernel > > (v4.19.124-rt53-rebase), It occurs when we're jumping to crush kernel > > using kexec, in the initialization of the emmc driver. > > I'm still debugging the root cause, but I thought of mentioning this > > in the mailing list if you have any idea why this could occur. > > The issue doesn't happen on normal boot, only when I specifically > > crash the kernel into the crash kernel. > > Thanks, > > Ramon. > > I'm not very familiar with crashing the kernel into the crash kernel. > Can you explain in enough detail how to set things up to reproduce this > and how to trigger it? Does it happen every time? > > >From looking at the backtrace, it's hitting the WARN_ON() in the > cmpxchg() loop below, because TASKLET_STATE is just > TASKLET_STATE_CHAINED. > > It seems that the only way to turn off TASKLET_STATE_CHAINED is via > this cmpxchg(), but TASKLET_STATE_RUN can be independently turned off > elsewhere (tasklet_unlock() and tasklet_tryunlock()), so if that > happens and this loop is hit, you could loop until loops runs out and > hit this warning. But clearing TASKLET_STATE_RUN independently happens by the task, that set it / part of tasklet_schedule(). tasklet_tryunlock() does a cmpxchg() with only the RUN bit so it won't work if the additional CHAINED bit is set. The tasklet itself (which may run on another CPU) sets the RUN bit at the begin and clears it at the end via cmpxchg() together with the CHAINED bit. I've been staring at it for sometime and I don't see how this can happen. Sebastian
Re: [PATCH RT 1/2] tasklet: Address a race resulting in double-enqueue
Hi Sebastian, On Tue, 2020-06-09 at 17:47 +0200, Sebastian Andrzej Siewior wrote: > On 2020-06-04 15:51:14 [-0500], Tom Zanussi wrote: > > > > > > Hi, This patch introduced a regression in our kernel > > > (v4.19.124-rt53-rebase), It occurs when we're jumping to crush > > > kernel > > > using kexec, in the initialization of the emmc driver. > > > I'm still debugging the root cause, but I thought of mentioning > > > this > > > in the mailing list if you have any idea why this could occur. > > > The issue doesn't happen on normal boot, only when I specifically > > > crash the kernel into the crash kernel. > > > Thanks, > > > Ramon. > > > > I'm not very familiar with crashing the kernel into the crash > > kernel. > > Can you explain in enough detail how to set things up to reproduce > > this > > and how to trigger it? Does it happen every time? > > > > > From looking at the backtrace, it's hitting the WARN_ON() in the > > > > cmpxchg() loop below, because TASKLET_STATE is just > > TASKLET_STATE_CHAINED. > > > > It seems that the only way to turn off TASKLET_STATE_CHAINED is via > > this cmpxchg(), but TASKLET_STATE_RUN can be independently turned > > off > > elsewhere (tasklet_unlock() and tasklet_tryunlock()), so if that > > happens and this loop is hit, you could loop until loops runs out > > and > > hit this warning. > > But clearing TASKLET_STATE_RUN independently happens by the task, > that > set it / part of tasklet_schedule(). > tasklet_tryunlock() does a cmpxchg() with only the RUN bit so it > won't > work if the additional CHAINED bit is set. > > The tasklet itself (which may run on another CPU) sets the RUN bit at > the > begin and clears it at the end via cmpxchg() together with the > CHAINED > bit. > > I've been staring at it for sometime and I don't see how this can > happen. > I did find a problem with the patch when configured as !SMP since in that case the RUN flag is never set (will send a patch for that shortly), but that wouldn't be the case here. It would help to be able to reproduce it, but I haven't been able to yet. Tom > Sebastian
Re: [PATCH RT 1/2] tasklet: Address a race resulting in double-enqueue
On 2020-06-09 11:17:53 [-0500], Tom Zanussi wrote:
> Hi Sebastian,
Hi Tom,
> I did find a problem with the patch when configured as !SMP since in
> that case the RUN flag is never set (will send a patch for that
> shortly), but that wouldn't be the case here.
How?
| #if defined(CONFIG_SMP) || defined(CONFIG_PREEMPT_RT_FULL)
| static inline int tasklet_trylock(struct tasklet_struct *t)
| {
| return !test_and_set_bit(TASKLET_STATE_RUN, &(t)->state);
| }
I can't tell from the backtrace if he runs with RT or without but I
assumed RT. But yes, for !SMP && !RT it would explain it.
> It would help to be able to reproduce it, but I haven't been able to
> yet.
>
> Tom
>
Sebastian
Re: [PATCH RT 1/2] tasklet: Address a race resulting in double-enqueue
On June 9, 2020 7:34:46 PM GMT+03:00, Sebastian Andrzej Siewior
wrote:
>On 2020-06-09 11:17:53 [-0500], Tom Zanussi wrote:
>> Hi Sebastian,
>Hi Tom,
>
>> I did find a problem with the patch when configured as !SMP since in
>> that case the RUN flag is never set (will send a patch for that
>> shortly), but that wouldn't be the case here.
>
>How?
>
>| #if defined(CONFIG_SMP) || defined(CONFIG_PREEMPT_RT_FULL)
>| static inline int tasklet_trylock(struct tasklet_struct *t)
>| {
>| return !test_and_set_bit(TASKLET_STATE_RUN, &(t)->state);
>| }
>
>I can't tell from the backtrace if he runs with RT or without but I
>assumed RT. But yes, for !SMP && !RT it would explain it.
PREEMT_FULL is enabled.
I'm working on getting symbols for this trace, this is a crash kernel so
everything is stripped naked.
Thanks, Ramon
>
>> It would help to be able to reproduce it, but I haven't been able to
>> yet.
>>
>> Tom
>>
>Sebastian
--
Sent from my Android device with K-9 Mail. Please excuse my brevity.
Re: [PATCH RT 1/2] tasklet: Address a race resulting in double-enqueue
On June 9, 2020 7:37:31 PM GMT+03:00, Ramon Fried wrote:
>
>
>On June 9, 2020 7:34:46 PM GMT+03:00, Sebastian Andrzej Siewior
> wrote:
>>On 2020-06-09 11:17:53 [-0500], Tom Zanussi wrote:
>>> Hi Sebastian,
>>Hi Tom,
>>
>>> I did find a problem with the patch when configured as !SMP since in
>>> that case the RUN flag is never set (will send a patch for that
>>> shortly), but that wouldn't be the case here.
>>
>>How?
>>
>>| #if defined(CONFIG_SMP) || defined(CONFIG_PREEMPT_RT_FULL)
>>| static inline int tasklet_trylock(struct tasklet_struct *t)
>>| {
>>| return !test_and_set_bit(TASKLET_STATE_RUN, &(t)->state);
>>| }
>>
>>I can't tell from the backtrace if he runs with RT or without but I
>>assumed RT. But yes, for !SMP && !RT it would explain it.
>PREEMT_FULL is enabled.
>I'm working on getting symbols for this trace, this is a crash kernel
>so everything is stripped naked.
>Thanks, Ramon
Correction. normal kernel is running with RT enabled, crash kernel without.
>>
>>> It would help to be able to reproduce it, but I haven't been able to
>>> yet.
>>>
>>> Tom
>>>
>>Sebastian
--
Sent from my Android device with K-9 Mail. Please excuse my brevity.
Re: [PATCH RT 1/2] tasklet: Address a race resulting in double-enqueue
On 2020-06-09 19:40:03 [+0300], Ramon Fried wrote: > Correction. normal kernel is running with RT enabled, crash kernel without. no RT and no SMP in your crash kernel? So this information in your first email would have saved me some time… Sebastian
Re: [PATCH RT 1/2] tasklet: Address a race resulting in double-enqueue
Hi Sebastian,
On Tue, 2020-06-09 at 18:34 +0200, Sebastian Andrzej Siewior wrote:
> On 2020-06-09 11:17:53 [-0500], Tom Zanussi wrote:
> > Hi Sebastian,
>
> Hi Tom,
>
> > I did find a problem with the patch when configured as !SMP since
> > in
> > that case the RUN flag is never set (will send a patch for that
> > shortly), but that wouldn't be the case here.
>
> How?
>
My test machine with !SMP and !RT doesn't boot, and in that case we
have:
#define tasklet_trylock(t) 1
#define tasklet_tryunlock(t)1
instead of setting/clearing the RUN flag.
So the cmpxchg with RUN+CHAIN can never work and we hit the loop.
> > #if defined(CONFIG_SMP) || defined(CONFIG_PREEMPT_RT_FULL)
> > static inline int tasklet_trylock(struct tasklet_struct *t)
> > {
> > return !test_and_set_bit(TASKLET_STATE_RUN, &(t)->state);
> > }
>
> I can't tell from the backtrace if he runs with RT or without but I
> assumed RT. But yes, for !SMP && !RT it would explain it.
>
Yeah, for me !SMP and RT works, but !SMP and !RT doesn't.
I had assumed he was talking about the samely configured kernel, but
apparently it's not.
Tom
> > It would help to be able to reproduce it, but I haven't been able
> > to
> > yet.
> >
> > Tom
> >
>
> Sebastian
Re: [PATCH RT 1/2] tasklet: Address a race resulting in double-enqueue
On June 9, 2020 7:42:13 PM GMT+03:00, Sebastian Andrzej Siewior wrote: >On 2020-06-09 19:40:03 [+0300], Ramon Fried wrote: >> Correction. normal kernel is running with RT enabled, crash kernel >without. > >no RT and no SMP in your crash kernel? So this information in your >first >email would have saved me some time… Indeed I'm truly sorry, I thought our crash kernel is configured as RT as well. so, as I understand, if I build the RT kernel without preempt enabled I can hit this bug? > >Sebastian -- Sent from my Android device with K-9 Mail. Please excuse my brevity.
Re: [PATCH RT 1/2] tasklet: Address a race resulting in double-enqueue
On 2020-06-09 20:07:06 [+0300], Ramon Fried wrote: > Indeed > I'm truly sorry, I thought our crash kernel is configured as RT as well. > so, as I understand, if I build the RT kernel without preempt enabled I can > hit this bug? Don't worry, I should have been better with the details in the log. So you should _always_ hit the warning/bug if you compile a kernel without SMP and RT. If you enable one of these then everything should be fine. Sebastian
Re: [PATCH RT 1/2] tasklet: Address a race resulting in double-enqueue
On June 9, 2020 8:10:43 PM GMT+03:00, Sebastian Andrzej Siewior wrote: >On 2020-06-09 20:07:06 [+0300], Ramon Fried wrote: >> Indeed >> I'm truly sorry, I thought our crash kernel is configured as RT as >well. >> so, as I understand, if I build the RT kernel without preempt enabled >I can hit this bug? > >Don't worry, I should have been better with the details in the log. > >So you should _always_ hit the warning/bug if you compile a kernel >without SMP and RT. If you enable one of these then everything should >be >fine. Would there be a fix for that? > >Sebastian -- Sent from my Android device with K-9 Mail. Please excuse my brevity.
Re: [PATCH RT 1/2] tasklet: Address a race resulting in double-enqueue
Hi Ramon,
On Tue, 2020-06-09 at 20:14 +0300, Ramon Fried wrote:
>
> On June 9, 2020 8:10:43 PM GMT+03:00, Sebastian Andrzej Siewior <
> bige...@linutronix.de> wrote:
> > On 2020-06-09 20:07:06 [+0300], Ramon Fried wrote:
> > > Indeed
> > > I'm truly sorry, I thought our crash kernel is configured as RT
> > > as
> >
> > well.
> > > so, as I understand, if I build the RT kernel without preempt
> > > enabled
> >
> > I can hit this bug?
> >
> > Don't worry, I should have been better with the details in the log.
> >
> > So you should _always_ hit the warning/bug if you compile a kernel
> > without SMP and RT. If you enable one of these then everything
> > should
> > be
> > fine.
>
> Would there be a fix for that?
I haven't tested the fix yet, but can you try the below patch and see
if it fixes your broken case?
[PATCH] tasklet: Fix UP case for tasklet CHAINED state
commit 62d0a2a30cd0 (tasklet: Address a race resulting in
double-enqueue) addresses a problem that can result in a tasklet being
enqueued on two cpus at the same time by combining the RUN flag with a
new CHAINED flag, and relies on the combination to be present in order
to zero it out, which can never happen on (!SMP and !PREEMPT_RT_FULL)
because the RUN flag is SMP/PREEMPT_RT_FULL-only.
So make sure the above commit is only applied for the SMP ||
PREEMPT_RT_FULL case.
Signed-off-by: Tom Zanussi
---
kernel/softirq.c | 6 ++
1 file changed, 6 insertions(+)
diff --git a/kernel/softirq.c b/kernel/softirq.c
index 73dae64bfc9c..9bad7a16dc61 100644
--- a/kernel/softirq.c
+++ b/kernel/softirq.c
@@ -947,10 +947,12 @@ static void __tasklet_schedule_common(struct
tasklet_struct *t,
* is locked before adding it to the list.
*/
if (test_bit(TASKLET_STATE_SCHED, &t->state)) {
+#if defined(CONFIG_SMP) || defined(CONFIG_PREEMPT_RT_FULL)
if (test_and_set_bit(TASKLET_STATE_CHAINED, &t->state))
{
tasklet_unlock(t);
return;
}
+#endif
t->next = NULL;
*head->tail = t;
head->tail = &(t->next);
@@ -1044,7 +1046,11 @@ static void tasklet_action_common(struct
softirq_action *a,
again:
t->func(t->data);
+#if defined(CONFIG_SMP) || defined(CONFIG_PREEMPT_RT_FULL)
while (cmpxchg(&t->state, TASKLET_STATEF_RC, 0) !=
TASKLET_STATEF_RC) {
+#else
+ while (!tasklet_tryunlock(t)) {
+#endif
/*
* If it got disabled meanwhile, bail out:
*/
--
2.17.1
Re: [PATCH RT 1/2] tasklet: Address a race resulting in double-enqueue
On Tue, Jun 9, 2020 at 8:20 PM Tom Zanussi wrote:
>
> Hi Ramon,
>
> On Tue, 2020-06-09 at 20:14 +0300, Ramon Fried wrote:
> >
> > On June 9, 2020 8:10:43 PM GMT+03:00, Sebastian Andrzej Siewior <
> > bige...@linutronix.de> wrote:
> > > On 2020-06-09 20:07:06 [+0300], Ramon Fried wrote:
> > > > Indeed
> > > > I'm truly sorry, I thought our crash kernel is configured as RT
> > > > as
> > >
> > > well.
> > > > so, as I understand, if I build the RT kernel without preempt
> > > > enabled
> > >
> > > I can hit this bug?
> > >
> > > Don't worry, I should have been better with the details in the log.
> > >
> > > So you should _always_ hit the warning/bug if you compile a kernel
> > > without SMP and RT. If you enable one of these then everything
> > > should
> > > be
> > > fine.
> >
> > Would there be a fix for that?
>
> I haven't tested the fix yet, but can you try the below patch and see
> if it fixes your broken case?
>
> [PATCH] tasklet: Fix UP case for tasklet CHAINED state
>
> commit 62d0a2a30cd0 (tasklet: Address a race resulting in
> double-enqueue) addresses a problem that can result in a tasklet being
> enqueued on two cpus at the same time by combining the RUN flag with a
> new CHAINED flag, and relies on the combination to be present in order
> to zero it out, which can never happen on (!SMP and !PREEMPT_RT_FULL)
> because the RUN flag is SMP/PREEMPT_RT_FULL-only.
>
> So make sure the above commit is only applied for the SMP ||
> PREEMPT_RT_FULL case.
>
> Signed-off-by: Tom Zanussi
> ---
> kernel/softirq.c | 6 ++
> 1 file changed, 6 insertions(+)
>
> diff --git a/kernel/softirq.c b/kernel/softirq.c
> index 73dae64bfc9c..9bad7a16dc61 100644
> --- a/kernel/softirq.c
> +++ b/kernel/softirq.c
> @@ -947,10 +947,12 @@ static void __tasklet_schedule_common(struct
> tasklet_struct *t,
> * is locked before adding it to the list.
> */
> if (test_bit(TASKLET_STATE_SCHED, &t->state)) {
> +#if defined(CONFIG_SMP) || defined(CONFIG_PREEMPT_RT_FULL)
> if (test_and_set_bit(TASKLET_STATE_CHAINED, &t->state))
> {
> tasklet_unlock(t);
> return;
> }
> +#endif
> t->next = NULL;
> *head->tail = t;
> head->tail = &(t->next);
> @@ -1044,7 +1046,11 @@ static void tasklet_action_common(struct
> softirq_action *a,
> again:
> t->func(t->data);
>
> +#if defined(CONFIG_SMP) || defined(CONFIG_PREEMPT_RT_FULL)
> while (cmpxchg(&t->state, TASKLET_STATEF_RC, 0) !=
> TASKLET_STATEF_RC) {
> +#else
> + while (!tasklet_tryunlock(t)) {
> +#endif
> /*
> * If it got disabled meanwhile, bail out:
> */
> --
> 2.17.1
>
>
Tested-By: Ramon Fried
Working, thanks a lot.
Re: [PATCH RT 1/2] tasklet: Address a race resulting in double-enqueue
On Tue, 2020-06-09 at 23:03 +0300, Ramon Fried wrote:
> On Tue, Jun 9, 2020 at 8:20 PM Tom Zanussi
> wrote:
> >
> > Hi Ramon,
> >
> > On Tue, 2020-06-09 at 20:14 +0300, Ramon Fried wrote:
> > >
> > > On June 9, 2020 8:10:43 PM GMT+03:00, Sebastian Andrzej Siewior <
> > > bige...@linutronix.de> wrote:
> > > > On 2020-06-09 20:07:06 [+0300], Ramon Fried wrote:
> > > > > Indeed
> > > > > I'm truly sorry, I thought our crash kernel is configured as
> > > > > RT
> > > > > as
> > > >
> > > > well.
> > > > > so, as I understand, if I build the RT kernel without preempt
> > > > > enabled
> > > >
> > > > I can hit this bug?
> > > >
> > > > Don't worry, I should have been better with the details in the
> > > > log.
> > > >
> > > > So you should _always_ hit the warning/bug if you compile a
> > > > kernel
> > > > without SMP and RT. If you enable one of these then everything
> > > > should
> > > > be
> > > > fine.
> > >
> > > Would there be a fix for that?
> >
> > I haven't tested the fix yet, but can you try the below patch and
> > see
> > if it fixes your broken case?
> >
> > [PATCH] tasklet: Fix UP case for tasklet CHAINED state
> >
> > commit 62d0a2a30cd0 (tasklet: Address a race resulting in
> > double-enqueue) addresses a problem that can result in a tasklet
> > being
> > enqueued on two cpus at the same time by combining the RUN flag
> > with a
> > new CHAINED flag, and relies on the combination to be present in
> > order
> > to zero it out, which can never happen on (!SMP and
> > !PREEMPT_RT_FULL)
> > because the RUN flag is SMP/PREEMPT_RT_FULL-only.
> >
> > So make sure the above commit is only applied for the SMP ||
> > PREEMPT_RT_FULL case.
> >
> > Signed-off-by: Tom Zanussi
> > ---
> > kernel/softirq.c | 6 ++
> > 1 file changed, 6 insertions(+)
> >
> > diff --git a/kernel/softirq.c b/kernel/softirq.c
> > index 73dae64bfc9c..9bad7a16dc61 100644
> > --- a/kernel/softirq.c
> > +++ b/kernel/softirq.c
> > @@ -947,10 +947,12 @@ static void __tasklet_schedule_common(struct
> > tasklet_struct *t,
> > * is locked before adding it to the list.
> > */
> > if (test_bit(TASKLET_STATE_SCHED, &t->state)) {
> > +#if defined(CONFIG_SMP) || defined(CONFIG_PREEMPT_RT_FULL)
> > if (test_and_set_bit(TASKLET_STATE_CHAINED, &t-
> > >state))
> > {
> > tasklet_unlock(t);
> > return;
> > }
> > +#endif
> > t->next = NULL;
> > *head->tail = t;
> > head->tail = &(t->next);
> > @@ -1044,7 +1046,11 @@ static void tasklet_action_common(struct
> > softirq_action *a,
> > again:
> > t->func(t->data);
> >
> > +#if defined(CONFIG_SMP) || defined(CONFIG_PREEMPT_RT_FULL)
> > while (cmpxchg(&t->state, TASKLET_STATEF_RC, 0) !=
> > TASKLET_STATEF_RC) {
> > +#else
> > + while (!tasklet_tryunlock(t)) {
> > +#endif
> > /*
> > * If it got disabled meanwhile, bail out:
> > */
> > --
> > 2.17.1
> >
> >
>
> Tested-By: Ramon Fried
>
> Working, thanks a lot.
OK, great, thanks for testing and reporting this.
Tom
