KVM: some swait cleanups

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 <[email protected]>

CC Peter Zijlstra <[email protected]>

CC Peter Xu <[email protected]>

CC [email protected]

Signed-off-by Peter Xu <[email protected]>

---
 include/linux/swait.h |  4 ++--
 kernel/sched/swait.c  | 21 ++++++++++++++++-----
 2 files changed, 18 insertions(+), 7 deletions(-)

include/linux/swait.h +2 -2

index c1f9c62a8a50..c55171280ac8 100644
@@ -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);

kernel/sched/swait.c +16 -5

index 3d5610dcce11..31f8d677c690 100644
@@ -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);
 

Quoted 20 lines · Peter Xu

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++;
  }

Quoted 1 line

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.

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.

Quoted 8 lines · Peter Xu

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.

Quoted 13 lines

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++;
  }

Quoted 1 line

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.

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().

Quoted 11 lines · Peter Xu

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.

Quoted 19 lines · Peter Zijlstra

Quoted 11 lines · Peter Xu

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()")

Quoted 3 lines

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.