Re: [PATCH v2 8/9] sched: Don't reinvent the wheel but use existing llist API
On Mon, Feb 13, 2017 at 04:52:30PM +0100, Oleg Nesterov wrote: > On 02/13, Peter Zijlstra wrote: > > > > On Mon, Feb 13, 2017 at 04:21:08PM +0900, Byungchul Park wrote: > > > + llist_for_each_entry(p, llist, wake_entry) > > > + ttwu_do_activate(rq, p, p->sched_remote_wakeup ? WF_MIGRATED : > > > 0, ); > > > > I think this suffers the exact same problem the others did. After > > ttwu_do_activate() the llist entry can be reused, so doing list_next() > > after it is flaky. > > llist_for_each_entry_safe() should work, I guess. Yes. I will fix it. Thank you. > > Oleg.
Re: [PATCH v2 8/9] sched: Don't reinvent the wheel but use existing llist API
On Mon, Feb 13, 2017 at 04:52:30PM +0100, Oleg Nesterov wrote: > On 02/13, Peter Zijlstra wrote: > > > > On Mon, Feb 13, 2017 at 04:21:08PM +0900, Byungchul Park wrote: > > > + llist_for_each_entry(p, llist, wake_entry) > > > + ttwu_do_activate(rq, p, p->sched_remote_wakeup ? WF_MIGRATED : > > > 0, ); > > > > I think this suffers the exact same problem the others did. After > > ttwu_do_activate() the llist entry can be reused, so doing list_next() > > after it is flaky. > > llist_for_each_entry_safe() should work, I guess. Yes. I will fix it. Thank you. > > Oleg.
Re: [PATCH v2 8/9] sched: Don't reinvent the wheel but use existing llist API
On Mon, Feb 13, 2017 at 11:04:57AM +0100, Peter Zijlstra wrote: > On Mon, Feb 13, 2017 at 04:21:08PM +0900, Byungchul Park wrote: > > Although llist provides proper APIs, they are not used. Make them used. > > > > Signed-off-by: Byungchul Park> > --- > > kernel/sched/core.c | 13 ++--- > > 1 file changed, 2 insertions(+), 11 deletions(-) > > > > diff --git a/kernel/sched/core.c b/kernel/sched/core.c > > index d01f9d0..417060b 100644 > > --- a/kernel/sched/core.c > > +++ b/kernel/sched/core.c > > @@ -1783,17 +1783,8 @@ void sched_ttwu_pending(void) > > raw_spin_lock_irqsave(>lock, flags); > > rq_pin_lock(rq, ); > > > > - while (llist) { > > - int wake_flags = 0; > > - > > - p = llist_entry(llist, struct task_struct, wake_entry); > > - llist = llist_next(llist); > > - > > - if (p->sched_remote_wakeup) > > - wake_flags = WF_MIGRATED; > > - > > - ttwu_do_activate(rq, p, wake_flags, ); > > - } > > + llist_for_each_entry(p, llist, wake_entry) > > + ttwu_do_activate(rq, p, p->sched_remote_wakeup ? WF_MIGRATED : > > 0, ); > > I think this suffers the exact same problem the others did. After > ttwu_do_activate() the llist entry can be reused, so doing list_next() > after it is flaky. Indeed. I thought it's safe since it's within rq locked and cannot be reused. But I was wrong but it can be unlocked in ttwu_do_activate(). I will fix it. Thank you very much.
Re: [PATCH v2 8/9] sched: Don't reinvent the wheel but use existing llist API
On Mon, Feb 13, 2017 at 11:04:57AM +0100, Peter Zijlstra wrote: > On Mon, Feb 13, 2017 at 04:21:08PM +0900, Byungchul Park wrote: > > Although llist provides proper APIs, they are not used. Make them used. > > > > Signed-off-by: Byungchul Park > > --- > > kernel/sched/core.c | 13 ++--- > > 1 file changed, 2 insertions(+), 11 deletions(-) > > > > diff --git a/kernel/sched/core.c b/kernel/sched/core.c > > index d01f9d0..417060b 100644 > > --- a/kernel/sched/core.c > > +++ b/kernel/sched/core.c > > @@ -1783,17 +1783,8 @@ void sched_ttwu_pending(void) > > raw_spin_lock_irqsave(>lock, flags); > > rq_pin_lock(rq, ); > > > > - while (llist) { > > - int wake_flags = 0; > > - > > - p = llist_entry(llist, struct task_struct, wake_entry); > > - llist = llist_next(llist); > > - > > - if (p->sched_remote_wakeup) > > - wake_flags = WF_MIGRATED; > > - > > - ttwu_do_activate(rq, p, wake_flags, ); > > - } > > + llist_for_each_entry(p, llist, wake_entry) > > + ttwu_do_activate(rq, p, p->sched_remote_wakeup ? WF_MIGRATED : > > 0, ); > > I think this suffers the exact same problem the others did. After > ttwu_do_activate() the llist entry can be reused, so doing list_next() > after it is flaky. Indeed. I thought it's safe since it's within rq locked and cannot be reused. But I was wrong but it can be unlocked in ttwu_do_activate(). I will fix it. Thank you very much.
Re: [PATCH v2 8/9] sched: Don't reinvent the wheel but use existing llist API
On 02/13, Peter Zijlstra wrote: > > On Mon, Feb 13, 2017 at 04:21:08PM +0900, Byungchul Park wrote: > > + llist_for_each_entry(p, llist, wake_entry) > > + ttwu_do_activate(rq, p, p->sched_remote_wakeup ? WF_MIGRATED : > > 0, ); > > I think this suffers the exact same problem the others did. After > ttwu_do_activate() the llist entry can be reused, so doing list_next() > after it is flaky. llist_for_each_entry_safe() should work, I guess. Oleg.
Re: [PATCH v2 8/9] sched: Don't reinvent the wheel but use existing llist API
On 02/13, Peter Zijlstra wrote: > > On Mon, Feb 13, 2017 at 04:21:08PM +0900, Byungchul Park wrote: > > + llist_for_each_entry(p, llist, wake_entry) > > + ttwu_do_activate(rq, p, p->sched_remote_wakeup ? WF_MIGRATED : > > 0, ); > > I think this suffers the exact same problem the others did. After > ttwu_do_activate() the llist entry can be reused, so doing list_next() > after it is flaky. llist_for_each_entry_safe() should work, I guess. Oleg.
Re: [PATCH v2 8/9] sched: Don't reinvent the wheel but use existing llist API
On Mon, Feb 13, 2017 at 04:21:08PM +0900, Byungchul Park wrote: > Although llist provides proper APIs, they are not used. Make them used. > > Signed-off-by: Byungchul Park> --- > kernel/sched/core.c | 13 ++--- > 1 file changed, 2 insertions(+), 11 deletions(-) > > diff --git a/kernel/sched/core.c b/kernel/sched/core.c > index d01f9d0..417060b 100644 > --- a/kernel/sched/core.c > +++ b/kernel/sched/core.c > @@ -1783,17 +1783,8 @@ void sched_ttwu_pending(void) > raw_spin_lock_irqsave(>lock, flags); > rq_pin_lock(rq, ); > > - while (llist) { > - int wake_flags = 0; > - > - p = llist_entry(llist, struct task_struct, wake_entry); > - llist = llist_next(llist); > - > - if (p->sched_remote_wakeup) > - wake_flags = WF_MIGRATED; > - > - ttwu_do_activate(rq, p, wake_flags, ); > - } > + llist_for_each_entry(p, llist, wake_entry) > + ttwu_do_activate(rq, p, p->sched_remote_wakeup ? WF_MIGRATED : > 0, ); I think this suffers the exact same problem the others did. After ttwu_do_activate() the llist entry can be reused, so doing list_next() after it is flaky.
Re: [PATCH v2 8/9] sched: Don't reinvent the wheel but use existing llist API
On Mon, Feb 13, 2017 at 04:21:08PM +0900, Byungchul Park wrote: > Although llist provides proper APIs, they are not used. Make them used. > > Signed-off-by: Byungchul Park > --- > kernel/sched/core.c | 13 ++--- > 1 file changed, 2 insertions(+), 11 deletions(-) > > diff --git a/kernel/sched/core.c b/kernel/sched/core.c > index d01f9d0..417060b 100644 > --- a/kernel/sched/core.c > +++ b/kernel/sched/core.c > @@ -1783,17 +1783,8 @@ void sched_ttwu_pending(void) > raw_spin_lock_irqsave(>lock, flags); > rq_pin_lock(rq, ); > > - while (llist) { > - int wake_flags = 0; > - > - p = llist_entry(llist, struct task_struct, wake_entry); > - llist = llist_next(llist); > - > - if (p->sched_remote_wakeup) > - wake_flags = WF_MIGRATED; > - > - ttwu_do_activate(rq, p, wake_flags, ); > - } > + llist_for_each_entry(p, llist, wake_entry) > + ttwu_do_activate(rq, p, p->sched_remote_wakeup ? WF_MIGRATED : > 0, ); I think this suffers the exact same problem the others did. After ttwu_do_activate() the llist entry can be reused, so doing list_next() after it is flaky.
[PATCH v2 8/9] sched: Don't reinvent the wheel but use existing llist API
Although llist provides proper APIs, they are not used. Make them used. Signed-off-by: Byungchul Park--- kernel/sched/core.c | 13 ++--- 1 file changed, 2 insertions(+), 11 deletions(-) diff --git a/kernel/sched/core.c b/kernel/sched/core.c index d01f9d0..417060b 100644 --- a/kernel/sched/core.c +++ b/kernel/sched/core.c @@ -1783,17 +1783,8 @@ void sched_ttwu_pending(void) raw_spin_lock_irqsave(>lock, flags); rq_pin_lock(rq, ); - while (llist) { - int wake_flags = 0; - - p = llist_entry(llist, struct task_struct, wake_entry); - llist = llist_next(llist); - - if (p->sched_remote_wakeup) - wake_flags = WF_MIGRATED; - - ttwu_do_activate(rq, p, wake_flags, ); - } + llist_for_each_entry(p, llist, wake_entry) + ttwu_do_activate(rq, p, p->sched_remote_wakeup ? WF_MIGRATED : 0, ); rq_unpin_lock(rq, ); raw_spin_unlock_irqrestore(>lock, flags); -- 1.9.1
[PATCH v2 8/9] sched: Don't reinvent the wheel but use existing llist API
Although llist provides proper APIs, they are not used. Make them used. Signed-off-by: Byungchul Park --- kernel/sched/core.c | 13 ++--- 1 file changed, 2 insertions(+), 11 deletions(-) diff --git a/kernel/sched/core.c b/kernel/sched/core.c index d01f9d0..417060b 100644 --- a/kernel/sched/core.c +++ b/kernel/sched/core.c @@ -1783,17 +1783,8 @@ void sched_ttwu_pending(void) raw_spin_lock_irqsave(>lock, flags); rq_pin_lock(rq, ); - while (llist) { - int wake_flags = 0; - - p = llist_entry(llist, struct task_struct, wake_entry); - llist = llist_next(llist); - - if (p->sched_remote_wakeup) - wake_flags = WF_MIGRATED; - - ttwu_do_activate(rq, p, wake_flags, ); - } + llist_for_each_entry(p, llist, wake_entry) + ttwu_do_activate(rq, p, p->sched_remote_wakeup ? WF_MIGRATED : 0, ); rq_unpin_lock(rq, ); raw_spin_unlock_irqrestore(>lock, flags); -- 1.9.1