* [PATCH 1/2] sched/swait: allow swake_up() to return [not found] <20171109091854.24367-1-peterx@redhat.com> @ 2017-11-09 9:18 ` Peter Xu 2017-11-09 10:06 ` Paolo Bonzini 2017-11-09 10:23 ` Peter Zijlstra 0 siblings, 2 replies; 8+ messages in thread From: Peter Xu @ 2017-11-09 9:18 UTC (permalink / raw) To: kvm Cc: Paolo Bonzini, peterx, Radim Krčmář, Ingo Molnar, Peter Zijlstra, linux-kernel Let swake_up() to return whether any of the waiters is waked up. One use case of it would be: if (swait_active(wq)) { swake_up(wq); // do something when waiter is waked up waked_up++; } Logically it's possible that when reaching swake_up() the wait queue is not active any more, and here doing something like waked_up++ would be inaccurate. To correct it, we need an atomic version of it. With this patch, we can simply re-write it into: if (swake_up(wq)) { // do something when waiter is waked up waked_up++; } After all we are checking swait_active() inside swake_up() too. CC: Ingo Molnar <mingo@redhat.com> CC: Peter Zijlstra <peterz@infradead.org> CC: Peter Xu <peterx@redhat.com> CC: linux-kernel@vger.kernel.org Signed-off-by: Peter Xu <peterx@redhat.com> --- include/linux/swait.h | 4 ++-- kernel/sched/swait.c | 21 ++++++++++++++++----- 2 files changed, 18 insertions(+), 7 deletions(-) diff --git a/include/linux/swait.h b/include/linux/swait.h index c1f9c62a8a50..c55171280ac8 100644 --- a/include/linux/swait.h +++ b/include/linux/swait.h @@ -84,9 +84,9 @@ static inline int swait_active(struct swait_queue_head *q) return !list_empty(&q->task_list); } -extern void swake_up(struct swait_queue_head *q); +extern bool swake_up(struct swait_queue_head *q); extern void swake_up_all(struct swait_queue_head *q); -extern void swake_up_locked(struct swait_queue_head *q); +extern bool swake_up_locked(struct swait_queue_head *q); extern void __prepare_to_swait(struct swait_queue_head *q, struct swait_queue *wait); extern void prepare_to_swait(struct swait_queue_head *q, struct swait_queue *wait, int state); diff --git a/kernel/sched/swait.c b/kernel/sched/swait.c index 3d5610dcce11..31f8d677c690 100644 --- a/kernel/sched/swait.c +++ b/kernel/sched/swait.c @@ -16,29 +16,40 @@ EXPORT_SYMBOL(__init_swait_queue_head); * If for some reason it would return 0, that means the previously waiting * task is already running, so it will observe condition true (or has already). */ -void swake_up_locked(struct swait_queue_head *q) +bool swake_up_locked(struct swait_queue_head *q) { struct swait_queue *curr; if (list_empty(&q->task_list)) - return; + return false; curr = list_first_entry(&q->task_list, typeof(*curr), task_list); wake_up_process(curr->task); list_del_init(&curr->task_list); + return true; } EXPORT_SYMBOL(swake_up_locked); -void swake_up(struct swait_queue_head *q) +/** + * swake_up - Wake up one process on the waiting list + * @q: the waitqueue to wake up + * + * Returns true if some process is waked up, otherwise false if there + * is no waiter to wake up. + */ +bool swake_up(struct swait_queue_head *q) { + bool ret; unsigned long flags; if (!swait_active(q)) - return; + return false; raw_spin_lock_irqsave(&q->lock, flags); - swake_up_locked(q); + ret = swake_up_locked(q); raw_spin_unlock_irqrestore(&q->lock, flags); + + return ret; } EXPORT_SYMBOL(swake_up); -- 2.13.6 ^ permalink raw reply related [flat|nested] 8+ messages in thread
* Re: [PATCH 1/2] sched/swait: allow swake_up() to return 2017-11-09 9:18 ` [PATCH 1/2] sched/swait: allow swake_up() to return Peter Xu @ 2017-11-09 10:06 ` Paolo Bonzini 2017-11-10 7:12 ` Peter Xu 2017-11-09 10:23 ` Peter Zijlstra 1 sibling, 1 reply; 8+ messages in thread From: Paolo Bonzini @ 2017-11-09 10:06 UTC (permalink / raw) To: Peter Xu, kvm Cc: Radim Krčmář, Ingo Molnar, Peter Zijlstra, linux-kernel On 09/11/2017 10:18, Peter Xu wrote: > Let swake_up() to return whether any of the waiters is waked up. One use > case of it would be: > > if (swait_active(wq)) { > swake_up(wq); > // do something when waiter is waked up > waked_up++; > } > > Logically it's possible that when reaching swake_up() the wait queue is > not active any more, and here doing something like waked_up++ would be > inaccurate. To correct it, we need an atomic version of it. > > With this patch, we can simply re-write it into: > > if (swake_up(wq)) { > // do something when waiter is waked up > waked_up++; > } > > After all we are checking swait_active() inside swake_up() too. Better subject: sched/swait: make swake_up() return whether there were any waiters I like this patch. > > CC: Ingo Molnar <mingo@redhat.com> > CC: Peter Zijlstra <peterz@infradead.org> > CC: Peter Xu <peterx@redhat.com> > CC: linux-kernel@vger.kernel.org > Signed-off-by: Peter Xu <peterx@redhat.com> > --- > include/linux/swait.h | 4 ++-- > kernel/sched/swait.c | 21 ++++++++++++++++----- > 2 files changed, 18 insertions(+), 7 deletions(-) > > diff --git a/include/linux/swait.h b/include/linux/swait.h > index c1f9c62a8a50..c55171280ac8 100644 > --- a/include/linux/swait.h > +++ b/include/linux/swait.h > @@ -84,9 +84,9 @@ static inline int swait_active(struct swait_queue_head *q) > return !list_empty(&q->task_list); > } > > -extern void swake_up(struct swait_queue_head *q); > +extern bool swake_up(struct swait_queue_head *q); > extern void swake_up_all(struct swait_queue_head *q); > -extern void swake_up_locked(struct swait_queue_head *q); > +extern bool swake_up_locked(struct swait_queue_head *q); > > extern void __prepare_to_swait(struct swait_queue_head *q, struct swait_queue *wait); > extern void prepare_to_swait(struct swait_queue_head *q, struct swait_queue *wait, int state); > diff --git a/kernel/sched/swait.c b/kernel/sched/swait.c > index 3d5610dcce11..31f8d677c690 100644 > --- a/kernel/sched/swait.c > +++ b/kernel/sched/swait.c > @@ -16,29 +16,40 @@ EXPORT_SYMBOL(__init_swait_queue_head); > * If for some reason it would return 0, that means the previously waiting > * task is already running, so it will observe condition true (or has already). > */ > -void swake_up_locked(struct swait_queue_head *q) > +bool swake_up_locked(struct swait_queue_head *q) > { > struct swait_queue *curr; > > if (list_empty(&q->task_list)) > - return; > + return false; > > curr = list_first_entry(&q->task_list, typeof(*curr), task_list); > wake_up_process(curr->task); > list_del_init(&curr->task_list); > + return true; > } > EXPORT_SYMBOL(swake_up_locked); > > -void swake_up(struct swait_queue_head *q) > +/** > + * swake_up - Wake up one process on the waiting list > + * @q: the waitqueue to wake up > + * > + * Returns true if some process is waked up, otherwise false if there > + * is no waiter to wake up. > + */ > +bool swake_up(struct swait_queue_head *q) > { > + bool ret; > unsigned long flags; > > if (!swait_active(q)) > - return; > + return false; > > raw_spin_lock_irqsave(&q->lock, flags); > - swake_up_locked(q); > + ret = swake_up_locked(q); > raw_spin_unlock_irqrestore(&q->lock, flags); > + > + return ret; > } > EXPORT_SYMBOL(swake_up); > > ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH 1/2] sched/swait: allow swake_up() to return 2017-11-09 10:06 ` Paolo Bonzini @ 2017-11-10 7:12 ` Peter Xu 0 siblings, 0 replies; 8+ messages in thread From: Peter Xu @ 2017-11-10 7:12 UTC (permalink / raw) To: Paolo Bonzini Cc: kvm, Radim Krčmář, Ingo Molnar, Peter Zijlstra, linux-kernel On Thu, Nov 09, 2017 at 11:06:53AM +0100, Paolo Bonzini wrote: > On 09/11/2017 10:18, Peter Xu wrote: > > Let swake_up() to return whether any of the waiters is waked up. One use > > case of it would be: > > > > if (swait_active(wq)) { > > swake_up(wq); > > // do something when waiter is waked up > > waked_up++; > > } > > > > Logically it's possible that when reaching swake_up() the wait queue is > > not active any more, and here doing something like waked_up++ would be > > inaccurate. To correct it, we need an atomic version of it. > > > > With this patch, we can simply re-write it into: > > > > if (swake_up(wq)) { > > // do something when waiter is waked up > > waked_up++; > > } > > > > After all we are checking swait_active() inside swake_up() too. > > Better subject: > > sched/swait: make swake_up() return whether there were any waiters > > I like this patch. I'll see how PeterZ would like me to do next, or I can drop this patch and send another clean up which is part of patch 2. Thanks for the positive feedback and commenting. :-) -- Peter Xu ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH 1/2] sched/swait: allow swake_up() to return 2017-11-09 9:18 ` [PATCH 1/2] sched/swait: allow swake_up() to return Peter Xu 2017-11-09 10:06 ` Paolo Bonzini @ 2017-11-09 10:23 ` Peter Zijlstra 2017-11-10 7:10 ` Peter Xu 1 sibling, 1 reply; 8+ messages in thread From: Peter Zijlstra @ 2017-11-09 10:23 UTC (permalink / raw) To: Peter Xu; +Cc: kvm, Paolo Bonzini, Radim Kr??m????, Ingo Molnar, linux-kernel On Thu, Nov 09, 2017 at 05:18:53PM +0800, Peter Xu wrote: > Let swake_up() to return whether any of the waiters is waked up. One use > case of it would be: > > if (swait_active(wq)) { > swake_up(wq); > // do something when waiter is waked up > waked_up++; > } The word is 'woken', and no that doesn't work. All it says is that there was a waiter, not that you were to one to wake it. Another concurrent wakeup might have done so. > > Logically it's possible that when reaching swake_up() the wait queue is > not active any more, and here doing something like waked_up++ would be > inaccurate. To correct it, we need an atomic version of it. > > With this patch, we can simply re-write it into: > > if (swake_up(wq)) { > // do something when waiter is waked up > waked_up++; > } > > After all we are checking swait_active() inside swake_up() too. We're not in fact; you've been staring at old code; see commit: 35a2897c2a30 ("sched/wait: Remove the lockless swait_active() check in swake_up*()") Also, you're changing the interface relative to the regular wait interface. The two should be similar wherever possible. ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH 1/2] sched/swait: allow swake_up() to return 2017-11-09 10:23 ` Peter Zijlstra @ 2017-11-10 7:10 ` Peter Xu 2017-11-10 8:05 ` Peter Zijlstra 0 siblings, 1 reply; 8+ messages in thread From: Peter Xu @ 2017-11-10 7:10 UTC (permalink / raw) To: Peter Zijlstra Cc: kvm, Paolo Bonzini, Radim Kr??m????, Ingo Molnar, linux-kernel On Thu, Nov 09, 2017 at 11:23:03AM +0100, Peter Zijlstra wrote: > On Thu, Nov 09, 2017 at 05:18:53PM +0800, Peter Xu wrote: > > Let swake_up() to return whether any of the waiters is waked up. One use > > case of it would be: > > > > if (swait_active(wq)) { > > swake_up(wq); > > // do something when waiter is waked up > > waked_up++; > > } > > The word is 'woken', and no that doesn't work. All it says is that there > was a waiter, not that you were to one to wake it. Another concurrent > wakeup might have done so. Yes. Or IIUC the waiter can be calling finish_swait() somehow so it removed itself from the list before being woken. > > > > > Logically it's possible that when reaching swake_up() the wait queue is > > not active any more, and here doing something like waked_up++ would be > > inaccurate. To correct it, we need an atomic version of it. > > > > With this patch, we can simply re-write it into: > > > > if (swake_up(wq)) { > > // do something when waiter is waked up > > waked_up++; > > } > > > > After all we are checking swait_active() inside swake_up() too. > > We're not in fact; you've been staring at old code; see commit: > > 35a2897c2a30 ("sched/wait: Remove the lockless swait_active() check in swake_up*()") I thought the tree was new enough, but obviously I was wrong... Thanks for the pointer. > > > Also, you're changing the interface relative to the regular wait > interface. The two should be similar wherever possible. Indeed. I came to this when reading kvm_vcpu_wake_up(), so that only affects some statistic which may not be that critical. However I don't know whether there would be any other real use case that we would like to know exactly whether a call to [s]wake_up() has really done something or just returned with a NOP. Anyway, please let me know if you think the same change to wake_up() would be meaningful, otherwise I can drop this patch and post another KVM-only one to clean up the redundant callers of swait_active(), since even if we dropped that list check in 35a2897c2a30, we'll do that again in swake_up_locked(). And after knowing 35a2897c2a30, I do think that calling swait_active() before swake_up() is not good since that call is without a lock as well, just like what can happen before 35a2897c2a30. (I am not 100% sure whether I fully understand the problem mentioned in 35a2897c2a30, but I think it's the memory barrier in the lock/unlock that matters.) Thanks, -- Peter Xu ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH 1/2] sched/swait: allow swake_up() to return 2017-11-10 7:10 ` Peter Xu @ 2017-11-10 8:05 ` Peter Zijlstra 2017-11-13 3:33 ` Peter Xu 0 siblings, 1 reply; 8+ messages in thread From: Peter Zijlstra @ 2017-11-10 8:05 UTC (permalink / raw) To: Peter Xu; +Cc: kvm, Paolo Bonzini, Radim Kr??m????, Ingo Molnar, linux-kernel On Fri, Nov 10, 2017 at 03:10:17PM +0800, Peter Xu wrote: > I came to this when reading kvm_vcpu_wake_up(), so that only affects > some statistic which may not be that critical. However I don't know > whether there would be any other real use case that we would like to > know exactly whether a call to [s]wake_up() has really done something > or just returned with a NOP. > > Anyway, please let me know if you think the same change to wake_up() > would be meaningful, otherwise I can drop this patch and post another > KVM-only one to clean up the redundant callers of swait_active(), > since even if we dropped that list check in 35a2897c2a30, we'll do > that again in swake_up_locked(). See commits: 8cd641e3c7cb ("sched/wait: Add swq_has_sleeper()") 5e0018b3e39e ("kvm: Serialize wq active checks in kvm_vcpu_wake_up()") In any case, I don't think we want the change you propose. The numbers don't mean much and there's no point in making all the callers in the kernel slower for it. ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH 1/2] sched/swait: allow swake_up() to return 2017-11-10 8:05 ` Peter Zijlstra @ 2017-11-13 3:33 ` Peter Xu 2017-11-13 5:19 ` Peter Xu 0 siblings, 1 reply; 8+ messages in thread From: Peter Xu @ 2017-11-13 3:33 UTC (permalink / raw) To: Peter Zijlstra Cc: kvm, Paolo Bonzini, Radim Kr??m????, Ingo Molnar, linux-kernel On Fri, Nov 10, 2017 at 09:05:20AM +0100, Peter Zijlstra wrote: > On Fri, Nov 10, 2017 at 03:10:17PM +0800, Peter Xu wrote: > > I came to this when reading kvm_vcpu_wake_up(), so that only affects > > some statistic which may not be that critical. However I don't know > > whether there would be any other real use case that we would like to > > know exactly whether a call to [s]wake_up() has really done something > > or just returned with a NOP. > > > > Anyway, please let me know if you think the same change to wake_up() > > would be meaningful, otherwise I can drop this patch and post another > > KVM-only one to clean up the redundant callers of swait_active(), > > since even if we dropped that list check in 35a2897c2a30, we'll do > > that again in swake_up_locked(). > > See commits: > > 8cd641e3c7cb ("sched/wait: Add swq_has_sleeper()") > 5e0018b3e39e ("kvm: Serialize wq active checks in kvm_vcpu_wake_up()") > > > In any case, I don't think we want the change you propose. The numbers > don't mean much and there's no point in making all the callers in the > kernel slower for it. I see. And also we can introduce a new API for that if really needed. I'll repost with KVM only changes. Thanks for reviewing. -- Peter Xu ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH 1/2] sched/swait: allow swake_up() to return 2017-11-13 3:33 ` Peter Xu @ 2017-11-13 5:19 ` Peter Xu 0 siblings, 0 replies; 8+ messages in thread From: Peter Xu @ 2017-11-13 5:19 UTC (permalink / raw) To: Peter Zijlstra Cc: kvm, Paolo Bonzini, Radim Kr??m????, Ingo Molnar, linux-kernel On Mon, Nov 13, 2017 at 11:33:43AM +0800, Peter Xu wrote: > On Fri, Nov 10, 2017 at 09:05:20AM +0100, Peter Zijlstra wrote: > > On Fri, Nov 10, 2017 at 03:10:17PM +0800, Peter Xu wrote: > > > I came to this when reading kvm_vcpu_wake_up(), so that only affects > > > some statistic which may not be that critical. However I don't know > > > whether there would be any other real use case that we would like to > > > know exactly whether a call to [s]wake_up() has really done something > > > or just returned with a NOP. > > > > > > Anyway, please let me know if you think the same change to wake_up() > > > would be meaningful, otherwise I can drop this patch and post another > > > KVM-only one to clean up the redundant callers of swait_active(), > > > since even if we dropped that list check in 35a2897c2a30, we'll do > > > that again in swake_up_locked(). > > > > See commits: > > > > 8cd641e3c7cb ("sched/wait: Add swq_has_sleeper()") > > 5e0018b3e39e ("kvm: Serialize wq active checks in kvm_vcpu_wake_up()") > > > > > > In any case, I don't think we want the change you propose. The numbers > > don't mean much and there's no point in making all the callers in the > > kernel slower for it. > > I see. And also we can introduce a new API for that if really needed. > > I'll repost with KVM only changes. Thanks for reviewing. Wait... I see that https://lkml.org/lkml/2017/9/5/622 seems to have fixed all the occurences. So I'll drop the series. Thanks, -- Peter Xu ^ permalink raw reply [flat|nested] 8+ messages in thread
end of thread, other threads:[~2017-11-13 5:19 UTC | newest] Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- [not found] <20171109091854.24367-1-peterx@redhat.com> 2017-11-09 9:18 ` [PATCH 1/2] sched/swait: allow swake_up() to return Peter Xu 2017-11-09 10:06 ` Paolo Bonzini 2017-11-10 7:12 ` Peter Xu 2017-11-09 10:23 ` Peter Zijlstra 2017-11-10 7:10 ` Peter Xu 2017-11-10 8:05 ` Peter Zijlstra 2017-11-13 3:33 ` Peter Xu 2017-11-13 5:19 ` Peter Xu
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).