> On Jul 19, 2023, at 7:44 PM, Chuck Lever III <chuck.lever@xxxxxxxxxx> wrote: > > > >> On Jul 19, 2023, at 7:20 PM, NeilBrown <neilb@xxxxxxx> wrote: >> >> On Wed, 19 Jul 2023, Chuck Lever III wrote: >>> >>>> On Jul 18, 2023, at 9:16 PM, NeilBrown <neilb@xxxxxxx> 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. > > 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. Pushed to https://git.kernel.org/pub/scm/linux/kernel/git/cel/linux.git in branch topic-sunrpc-thread-scheduling >> I think it is worth logging whether an event triggered a wake up or not, >> and which event did that. > > Agreed. I have some experimental code that records _RET_IP_ of the caller > of svc_xprt_enqueue(), but again it's questionable whether that is of > long term value. > > >> 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 > > > -- > Chuck Lever -- Chuck Lever
