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" <cel@kernel.org>
Cc:     "Chuck Lever" <chuck.lever@oracle.com>,
        "Jeff Layton" <jlayton@kernel.org>, linux-nfs@vger.kernel.org
Subject: Re: [PATCH 10/14] SUNRPC: change svc_pool_wake_idle_thread() to
 return nothing.
In-reply-to: <ZLaagzqpB9MsQ5yb@bazille.1015granger.net>
References: <168966227838.11075.2974227708495338626.stgit@noble.brown>,
 <168966228866.11075.18415964365983945832.stgit@noble.brown>,
 <ZLaagzqpB9MsQ5yb@bazille.1015granger.net>
Date:   Wed, 19 Jul 2023 11:16:24 +1000
Message-id: <168972938409.11078.8409356274248659649@noble.neil.brown.name>
Precedence: bulk

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.

Thanks,
NeilBrown