Received-SPF: pass (google.com: domain of linux-nfs-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Content-Type: text/plain; charset="utf-8"
Content-Transfer-Encoding: 7bit
MIME-Version: 1.0
From:   "NeilBrown" <neilb@suse.de>
To:     "Chuck Lever III" <chuck.lever@oracle.com>
Cc:     "Chuck Lever" <cel@kernel.org>, "Jeff Layton" <jlayton@kernel.org>,
        "Linux NFS Mailing List" <linux-nfs@vger.kernel.org>
Subject: Re: [PATCH 10/14] SUNRPC: change svc_pool_wake_idle_thread() to
 return nothing.
In-reply-to: <9EEE82A6-6D25-4939-A4F5-BAC8E9986FF3@oracle.com>
References: <168966227838.11075.2974227708495338626.stgit@noble.brown>,
 <168966228866.11075.18415964365983945832.stgit@noble.brown>,
 <ZLaagzqpB9MsQ5yb@bazille.1015granger.net>,
 <168972938409.11078.8409356274248659649@noble.neil.brown.name>,
 <9EEE82A6-6D25-4939-A4F5-BAC8E9986FF3@oracle.com>
Date:   Thu, 20 Jul 2023 09:20:18 +1000
Message-id: <168980881867.11078.6059884952065090216@noble.neil.brown.name>
Precedence: bulk

On Wed, 19 Jul 2023, Chuck Lever III wrote:
> 
> > On Jul 18, 2023, at 9:16 PM, NeilBrown <neilb@suse.de> wrote:
> > 
> > On Tue, 18 Jul 2023, Chuck Lever wrote:
> >> On Tue, Jul 18, 2023 at 04:38:08PM +1000, NeilBrown wrote:
> >>> No callers of svc_pool_wake_idle_thread() care which thread was woken -
> >>> except one that wants to trace the wakeup.  For now we drop that
> >>> tracepoint.
> >> 
> >> That's an important tracepoint, IMO.
> >> 
> >> It might be better to have svc_pool_wake_idle_thread() return void
> >> right from it's introduction, and move the tracepoint into that
> >> function. I can do that and respin if you agree.
> > 
> > Mostly I agree.
> > 
> > It isn't clear to me how you would handle trace_svc_xprt_enqueue(),
> > as there would be no code that can see both the trigger xprt, and the
> > woken rqst.
> > 
> > I also wonder if having the trace point when the wake-up is requested
> > makes any sense, as there is no guarantee that thread with handle that
> > xprt.
> > 
> > Maybe the trace point should report when the xprt is dequeued.  i.e.
> > maybe trace_svc_pool_awoken() should report the pid, and we could have
> > trace_svc_xprt_enqueue() only report the xprt, not the rqst.
> 
> I'll come up with something that rearranges the tracepoints so that
> svc_pool_wake_idle_thread() can return void.

My current draft code has svc_pool_wake_idle_thread() returning bool -
if it found something to wake up - purely for logging.
I think it is worth logging whether an event triggered a wake up or not,
and which event did that.  I'm less you that the pid is relevant.  But
as you say - this will probably become clearer as the code settles down.

Thanks,
NeilBrown


> 
> svc_pool_wake_idle_thread() can save the waker's PID in svc_rqst
> somewhere, for example. The dequeue tracepoint can then report that
> (if it's still interesting when we're all done with this work).
> 
> 
> --
> Chuck Lever
> 
> 
>