Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
From:   Valentin Schneider <vschneid@redhat.com>
To:     Wang You <wangyoua@uniontech.com>, mingo@redhat.com,
        peterz@infradead.org, juri.lelli@redhat.com,
        vincent.guittot@linaro.org, dietmar.eggemann@arm.com,
        rostedt@goodmis.org, bsegall@google.com, mgorman@suse.de,
        bristot@redhat.com
Cc:     linux-kernel@vger.kernel.org, Wang You <wangyoua@uniontech.com>
Subject: Re: [RFC PATCH] sched/wait: Determine whether the wait queue is
 empty before waking up
In-Reply-To: <20230609053812.10230-1-wangyoua@uniontech.com>
References: <20230609053812.10230-1-wangyoua@uniontech.com>
Date:   Mon, 12 Jun 2023 18:09:36 +0100
Message-ID: <xhsmhfs6wy5db.mognet@vschneid.remote.csb>
MIME-Version: 1.0
Content-Type: text/plain
Precedence: bulk

On 09/06/23 13:38, Wang You wrote:
> When we did some benchmark tests (such as pipe tests), we found
> that the wake behavior was still triggered when the wait queue
> was empty, even though it would exit later.
>
> This processing consumes some unnecessary resources. Can we
> determine at the beginning of the wake up whether there are
> elements in the queue that need to be awakened? I think this
> judgment is probably good for performance and should be safe.
>
> Looking forward to your reply, thank you.
>
> Signed-off-by: Wang You <wangyoua@uniontech.com>
> ---
>  kernel/sched/wait.c | 3 +++
>  1 file changed, 3 insertions(+)
>
> diff --git a/kernel/sched/wait.c b/kernel/sched/wait.c
> index 133b74730738..17011780aa21 100644
> --- a/kernel/sched/wait.c
> +++ b/kernel/sched/wait.c
> @@ -205,6 +205,9 @@ void __wake_up_sync_key(struct wait_queue_head *wq_head, unsigned int mode,
>       if (unlikely(!wq_head))
>               return;
>
> +	if (unlikely(!wq_has_sleeper(wq_head)))
> +		return;
> +


Hm so I suppose that if we end up in wake_up*() then the update of the wait
condition has been done (so this shouldn't lead to missed wakeups), but
that still means an extra smp_mb() before grabbing the wq_head->lock.

I'd suggest benchmarking the change, this is going to cause unwanted
overhead when dealing with busy queues, and I'm not sure it's saving much
vs grabbing wq_head->lock before noticing the list is empty.


>       __wake_up_common_lock(wq_head, mode, 1, WF_SYNC, key);
>  }
>  EXPORT_SYMBOL_GPL(__wake_up_sync_key);
> --
> 2.20.1