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: quoted-printable
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: <3A9F5306-EAEE-427C-80D2-E0CD81212238@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>,
 <168980881867.11078.6059884952065090216@noble.neil.brown.name>,
 <E93923C4-080B-4B43-9A3B-28A233BF5DFC@oracle.com>,
 <3A9F5306-EAEE-427C-80D2-E0CD81212238@oracle.com>
Date:   Mon, 24 Jul 2023 14:44:39 +1000
Message-id: <169017387952.11078.1482563019296445946@noble.neil.brown.name>
Precedence: bulk

On Fri, 21 Jul 2023, Chuck Lever III wrote:
>=20
> > On Jul 19, 2023, at 7:44 PM, Chuck Lever III <chuck.lever@oracle.com> wro=
te:
> >=20
> >=20
> >=20
> >> On Jul 19, 2023, at 7:20 PM, NeilBrown <neilb@suse.de> wrote:
> >>=20
> >> On Wed, 19 Jul 2023, Chuck Lever III wrote:
> >>>=20
> >>>> On Jul 18, 2023, at 9:16 PM, NeilBrown <neilb@suse.de> wrote:
> >>>>=20
> >>>> 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 woke=
n -
> >>>>>> except one that wants to trace the wakeup.  For now we drop that
> >>>>>> tracepoint.
> >>>>>=20
> >>>>> That's an important tracepoint, IMO.
> >>>>>=20
> >>>>> 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.
> >>>>=20
> >>>> Mostly I agree.
> >>>>=20
> >>>> 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.
> >>>>=20
> >>>> 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.
> >>>>=20
> >>>> 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.
> >>>=20
> >>> I'll come up with something that rearranges the tracepoints so that
> >>> svc_pool_wake_idle_thread() can return void.
> >>=20
> >> My current draft code has svc_pool_wake_idle_thread() returning bool -
> >> if it found something to wake up - purely for logging.
> >=20
> > This is also where I have ended up. I'll post an update probably tomorrow
> > my time. Too much other stuff going on to finish it today.
>=20
> Pushed to https://git.kernel.org/pub/scm/linux/kernel/git/cel/linux.git
> in branch topic-sunrpc-thread-scheduling

Another thing.
You have made svc_pool_wake_idle_thread() return, but for different
reasons than me.

I wanted bool so I could trace a wake up due to enqueuing an xprt
differently from a wakeup due to a call to svc_wake_up().  I thought the
difference might be important.

You have it returning a bool so that:
- in one case you can set SP_CONGESTED - but that can be safely set
  inside svc_pool_wake_idle_thread()
- in another case so SP_TASK_PENDING can be set.  But I think it is
  best to set that anyway, and clear it when svc_recv() wakes up.

So maybe it can return void after all.

Thanks,
NeilBrown