| Message ID | 168966228866.11075.18415964365983945832.stgit@noble.brown (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Refactor SUNRPC svc thread code, and use llist | expand |
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. > One caller wants to know if anything was woken to set SP_CONGESTED, so > set that inside the function instead. > > Now svc_pool_wake_idle_thread() can "return" void. > > Signed-off-by: NeilBrown <neilb@suse.de> > --- > include/linux/sunrpc/svc.h | 2 +- > net/sunrpc/svc.c | 13 +++++-------- > net/sunrpc/svc_xprt.c | 18 +++--------------- > 3 files changed, 9 insertions(+), 24 deletions(-) > > diff --git a/include/linux/sunrpc/svc.h b/include/linux/sunrpc/svc.h > index ea3ce1315416..b39c613fbe06 100644 > --- a/include/linux/sunrpc/svc.h > +++ b/include/linux/sunrpc/svc.h > @@ -454,7 +454,7 @@ int svc_register(const struct svc_serv *, struct net *, const int, > > void svc_wake_up(struct svc_serv *); > void svc_reserve(struct svc_rqst *rqstp, int space); > -struct svc_rqst *svc_pool_wake_idle_thread(struct svc_serv *serv, > +void svc_pool_wake_idle_thread(struct svc_serv *serv, > struct svc_pool *pool); > struct svc_pool *svc_pool_for_cpu(struct svc_serv *serv); > char * svc_print_addr(struct svc_rqst *, char *, size_t); > diff --git a/net/sunrpc/svc.c b/net/sunrpc/svc.c > index b18175ef74ec..fd49e7b12c94 100644 > --- a/net/sunrpc/svc.c > +++ b/net/sunrpc/svc.c > @@ -696,13 +696,10 @@ svc_prepare_thread(struct svc_serv *serv, struct svc_pool *pool, int node) > * @serv: RPC service > * @pool: service thread pool > * > - * Returns an idle service thread (now marked BUSY), or NULL > - * if no service threads are available. Finding an idle service > - * thread and marking it BUSY is atomic with respect to other > - * calls to svc_pool_wake_idle_thread(). > + * Wake an idle thread if there is one, else mark the pool as congested. > */ > -struct svc_rqst *svc_pool_wake_idle_thread(struct svc_serv *serv, > - struct svc_pool *pool) > +void svc_pool_wake_idle_thread(struct svc_serv *serv, > + struct svc_pool *pool) > { > struct svc_rqst *rqstp; > > @@ -715,13 +712,13 @@ struct svc_rqst *svc_pool_wake_idle_thread(struct svc_serv *serv, > wake_up_process(rqstp->rq_task); > rcu_read_unlock(); > percpu_counter_inc(&pool->sp_threads_woken); > - return rqstp; > + return; > } > rcu_read_unlock(); > > trace_svc_pool_starved(serv, pool); > percpu_counter_inc(&pool->sp_threads_starved); > - return NULL; > + set_bit(SP_CONGESTED, &pool->sp_flags); > } > EXPORT_SYMBOL_GPL(svc_pool_wake_idle_thread); > > diff --git a/net/sunrpc/svc_xprt.c b/net/sunrpc/svc_xprt.c > index 948605e7043b..964c97dbb36c 100644 > --- a/net/sunrpc/svc_xprt.c > +++ b/net/sunrpc/svc_xprt.c > @@ -456,7 +456,6 @@ static bool svc_xprt_ready(struct svc_xprt *xprt) > */ > void svc_xprt_enqueue(struct svc_xprt *xprt) > { > - struct svc_rqst *rqstp; > struct svc_pool *pool; > > if (!svc_xprt_ready(xprt)) > @@ -477,13 +476,7 @@ void svc_xprt_enqueue(struct svc_xprt *xprt) > list_add_tail(&xprt->xpt_ready, &pool->sp_sockets); > spin_unlock_bh(&pool->sp_lock); > > - rqstp = svc_pool_wake_idle_thread(xprt->xpt_server, pool); > - if (!rqstp) { > - set_bit(SP_CONGESTED, &pool->sp_flags); > - return; > - } > - > - trace_svc_xprt_enqueue(xprt, rqstp); > + svc_pool_wake_idle_thread(xprt->xpt_server, pool); > } > EXPORT_SYMBOL_GPL(svc_xprt_enqueue); > > @@ -587,14 +580,9 @@ static void svc_xprt_release(struct svc_rqst *rqstp) > void svc_wake_up(struct svc_serv *serv) > { > struct svc_pool *pool = &serv->sv_pools[0]; > - struct svc_rqst *rqstp; > > - rqstp = svc_pool_wake_idle_thread(serv, pool); > - if (!rqstp) { > - set_bit(SP_TASK_PENDING, &pool->sp_flags); > - smp_wmb(); > - return; > - } > + set_bit(SP_TASK_PENDING, &pool->sp_flags); > + svc_pool_wake_idle_thread(serv, pool); > } > EXPORT_SYMBOL_GPL(svc_wake_up); > > >
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
> 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. 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
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 > > >
> On Jul 19, 2023, at 7:20 PM, NeilBrown <neilb@suse.de> wrote: > > 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. 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. > 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
> On Jul 19, 2023, at 7:44 PM, Chuck Lever III <chuck.lever@oracle.com> wrote: > > > >> On Jul 19, 2023, at 7:20 PM, NeilBrown <neilb@suse.de> wrote: >> >> 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. > > 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
On Fri, 21 Jul 2023, Chuck Lever III wrote: > > > On Jul 19, 2023, at 7:44 PM, Chuck Lever III <chuck.lever@oracle.com> wrote: > > > > > > > >> On Jul 19, 2023, at 7:20 PM, NeilBrown <neilb@suse.de> wrote: > >> > >> 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. > > > > 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 > Thanks. One little thing to fix. In SUNRPC: Report when no service thread is available. you now have - return false; + + trace_svc_pool_starved(serv, pool); + percpu_counter_inc(&pool->sp_threads_starved); + return NULL; That should still be "return false" at the end; NeilBrown
On Fri, 21 Jul 2023, Chuck Lever III wrote: > > > On Jul 19, 2023, at 7:44 PM, Chuck Lever III <chuck.lever@oracle.com> wrote: > > > > > > > >> On Jul 19, 2023, at 7:20 PM, NeilBrown <neilb@suse.de> wrote: > >> > >> 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. > > > > 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() - 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
> On Jul 24, 2023, at 12:44 AM, NeilBrown <neilb@suse.de> wrote: > > On Fri, 21 Jul 2023, Chuck Lever III wrote: >> >>> On Jul 19, 2023, at 7:44 PM, Chuck Lever III <chuck.lever@oracle.com> wrote: >>> >>> >>> >>>> On Jul 19, 2023, at 7:20 PM, NeilBrown <neilb@suse.de> wrote: >>>> >>>> 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. >>> >>> 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
diff --git a/include/linux/sunrpc/svc.h b/include/linux/sunrpc/svc.h index ea3ce1315416..b39c613fbe06 100644 --- a/include/linux/sunrpc/svc.h +++ b/include/linux/sunrpc/svc.h @@ -454,7 +454,7 @@ int svc_register(const struct svc_serv *, struct net *, const int, void svc_wake_up(struct svc_serv *); void svc_reserve(struct svc_rqst *rqstp, int space); -struct svc_rqst *svc_pool_wake_idle_thread(struct svc_serv *serv, +void svc_pool_wake_idle_thread(struct svc_serv *serv, struct svc_pool *pool); struct svc_pool *svc_pool_for_cpu(struct svc_serv *serv); char * svc_print_addr(struct svc_rqst *, char *, size_t); diff --git a/net/sunrpc/svc.c b/net/sunrpc/svc.c index b18175ef74ec..fd49e7b12c94 100644 --- a/net/sunrpc/svc.c +++ b/net/sunrpc/svc.c @@ -696,13 +696,10 @@ svc_prepare_thread(struct svc_serv *serv, struct svc_pool *pool, int node) * @serv: RPC service * @pool: service thread pool * - * Returns an idle service thread (now marked BUSY), or NULL - * if no service threads are available. Finding an idle service - * thread and marking it BUSY is atomic with respect to other - * calls to svc_pool_wake_idle_thread(). + * Wake an idle thread if there is one, else mark the pool as congested. */ -struct svc_rqst *svc_pool_wake_idle_thread(struct svc_serv *serv, - struct svc_pool *pool) +void svc_pool_wake_idle_thread(struct svc_serv *serv, + struct svc_pool *pool) { struct svc_rqst *rqstp; @@ -715,13 +712,13 @@ struct svc_rqst *svc_pool_wake_idle_thread(struct svc_serv *serv, wake_up_process(rqstp->rq_task); rcu_read_unlock(); percpu_counter_inc(&pool->sp_threads_woken); - return rqstp; + return; } rcu_read_unlock(); trace_svc_pool_starved(serv, pool); percpu_counter_inc(&pool->sp_threads_starved); - return NULL; + set_bit(SP_CONGESTED, &pool->sp_flags); } EXPORT_SYMBOL_GPL(svc_pool_wake_idle_thread); diff --git a/net/sunrpc/svc_xprt.c b/net/sunrpc/svc_xprt.c index 948605e7043b..964c97dbb36c 100644 --- a/net/sunrpc/svc_xprt.c +++ b/net/sunrpc/svc_xprt.c @@ -456,7 +456,6 @@ static bool svc_xprt_ready(struct svc_xprt *xprt) */ void svc_xprt_enqueue(struct svc_xprt *xprt) { - struct svc_rqst *rqstp; struct svc_pool *pool; if (!svc_xprt_ready(xprt)) @@ -477,13 +476,7 @@ void svc_xprt_enqueue(struct svc_xprt *xprt) list_add_tail(&xprt->xpt_ready, &pool->sp_sockets); spin_unlock_bh(&pool->sp_lock); - rqstp = svc_pool_wake_idle_thread(xprt->xpt_server, pool); - if (!rqstp) { - set_bit(SP_CONGESTED, &pool->sp_flags); - return; - } - - trace_svc_xprt_enqueue(xprt, rqstp); + svc_pool_wake_idle_thread(xprt->xpt_server, pool); } EXPORT_SYMBOL_GPL(svc_xprt_enqueue); @@ -587,14 +580,9 @@ static void svc_xprt_release(struct svc_rqst *rqstp) void svc_wake_up(struct svc_serv *serv) { struct svc_pool *pool = &serv->sv_pools[0]; - struct svc_rqst *rqstp; - rqstp = svc_pool_wake_idle_thread(serv, pool); - if (!rqstp) { - set_bit(SP_TASK_PENDING, &pool->sp_flags); - smp_wmb(); - return; - } + set_bit(SP_TASK_PENDING, &pool->sp_flags); + svc_pool_wake_idle_thread(serv, pool); } EXPORT_SYMBOL_GPL(svc_wake_up);
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. One caller wants to know if anything was woken to set SP_CONGESTED, so set that inside the function instead. Now svc_pool_wake_idle_thread() can "return" void. Signed-off-by: NeilBrown <neilb@suse.de> --- include/linux/sunrpc/svc.h | 2 +- net/sunrpc/svc.c | 13 +++++-------- net/sunrpc/svc_xprt.c | 18 +++--------------- 3 files changed, 9 insertions(+), 24 deletions(-)