> On Jul 24, 2023, at 12:44 AM, NeilBrown <neilb@xxxxxxx> wrote: > > On Fri, 21 Jul 2023, Chuck Lever III wrote: >> >>> 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 > > 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() So, set CONGESTED whenever an idle thread cannot be found, no matter the caller. > - 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. Maybe that should be done in a separate patch. Can you give it a try? > So maybe it can return void after all. -- Chuck Lever
