| Message ID | 20240529-fuse-uring-for-6-9-rfc2-out-v1-15-d149476b1d65@ddn.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | fuse: fuse-over-io-uring | expand |
On Wed, May 29, 2024 at 08:00:50PM +0200, Bernd Schubert wrote: > This is needed by fuse-over-io-uring to wake up the waiting > application thread on the core it was submitted from. > Avoiding core switching is actually a major factor for > fuse performance improvements of fuse-over-io-uring. > > Signed-off-by: Bernd Schubert <bschubert@ddn.com> > Cc: Ingo Molnar <mingo@redhat.com> > Cc: Peter Zijlstra <peterz@infradead.org> > Cc: Andrei Vagin <avagin@google.com> Reviewed-by: Josef Bacik <josef@toxicpanda.com> Probably best to submit this as a one-off so the sched guys can take it and it's not in the middle of a fuse patchset they may be ignoring. Thanks, Josef
On Wed, May 29, 2024 at 08:00:50PM +0200, Bernd Schubert wrote: > This is needed by fuse-over-io-uring to wake up the waiting > application thread on the core it was submitted from. > Avoiding core switching is actually a major factor for > fuse performance improvements of fuse-over-io-uring. Then maybe split that into a separate enhancement? > --- a/kernel/sched/wait.c > +++ b/kernel/sched/wait.c > @@ -132,6 +132,7 @@ void __wake_up_on_current_cpu(struct wait_queue_head *wq_head, unsigned int mode > { > __wake_up_common_lock(wq_head, mode, 1, WF_CURRENT_CPU, key); > } > +EXPORT_SYMBOL(__wake_up_on_current_cpu); I'll leave it to the scheduler maintainers if they want this exported at all and if yes with the __-prefix and without a kerneldoc comment explaining the API, but anything this low-level should be EXPORT_SYMBOL_GPL for sure.
On Thu, May 30, 2024 at 04:37:29PM -0400, Josef Bacik wrote: > On Wed, May 29, 2024 at 08:00:50PM +0200, Bernd Schubert wrote: > > This is needed by fuse-over-io-uring to wake up the waiting > > application thread on the core it was submitted from. > > Avoiding core switching is actually a major factor for > > fuse performance improvements of fuse-over-io-uring. > > > > Signed-off-by: Bernd Schubert <bschubert@ddn.com> > > Cc: Ingo Molnar <mingo@redhat.com> > > Cc: Peter Zijlstra <peterz@infradead.org> > > Cc: Andrei Vagin <avagin@google.com> > > Reviewed-by: Josef Bacik <josef@toxicpanda.com> > > Probably best to submit this as a one-off so the sched guys can take it and it's > not in the middle of a fuse patchset they may be ignoring. Thanks, On its own its going to not be applied. Never merge an EXPORT without a user. As is, I don't have enough of the series to even see the user, so yeah, not happy :/ And as hch said, this very much needs to be a GPL export.
On 6/4/24 11:26, Peter Zijlstra wrote: > On Thu, May 30, 2024 at 04:37:29PM -0400, Josef Bacik wrote: >> On Wed, May 29, 2024 at 08:00:50PM +0200, Bernd Schubert wrote: >>> This is needed by fuse-over-io-uring to wake up the waiting >>> application thread on the core it was submitted from. >>> Avoiding core switching is actually a major factor for >>> fuse performance improvements of fuse-over-io-uring. >>> >>> Signed-off-by: Bernd Schubert <bschubert@ddn.com> >>> Cc: Ingo Molnar <mingo@redhat.com> >>> Cc: Peter Zijlstra <peterz@infradead.org> >>> Cc: Andrei Vagin <avagin@google.com> >> >> Reviewed-by: Josef Bacik <josef@toxicpanda.com> >> >> Probably best to submit this as a one-off so the sched guys can take it and it's >> not in the middle of a fuse patchset they may be ignoring. Thanks, > > On its own its going to not be applied. Never merge an EXPORT without a > user. > > As is, I don't have enough of the series to even see the user, so yeah, > not happy :/ > > And as hch said, this very much needs to be a GPL export. Sorry, accidentally done without the _GPL. What is the right way to get this merged? First merge the entire fuse-io-uring series and then add on this? I already have these optimization patches at the end of the series... The user for this is in the next patch [PATCH RFC v2 16/19] fuse: {uring} Wake requests on the the current cpu diff --git a/fs/fuse/dev.c b/fs/fuse/dev.c index c7fd3849a105..851c5fa99946 100644 --- a/fs/fuse/dev.c +++ b/fs/fuse/dev.c @@ -333,7 +333,10 @@ void fuse_request_end(struct fuse_req *req) spin_unlock(&fc->bg_lock); } else { /* Wake up waiter sleeping in request_wait_answer() */ - wake_up(&req->waitq); + if (fuse_per_core_queue(fc)) + __wake_up_on_current_cpu(&req->waitq, TASK_NORMAL, NULL); + else + wake_up(&req->waitq); } if (test_bit(FR_ASYNC, &req->flags)) Thank, Bernd
On Tue, Jun 04, 2024 at 09:36:08AM +0000, Bernd Schubert wrote: > On 6/4/24 11:26, Peter Zijlstra wrote: > > On Thu, May 30, 2024 at 04:37:29PM -0400, Josef Bacik wrote: > >> On Wed, May 29, 2024 at 08:00:50PM +0200, Bernd Schubert wrote: > >>> This is needed by fuse-over-io-uring to wake up the waiting > >>> application thread on the core it was submitted from. > >>> Avoiding core switching is actually a major factor for > >>> fuse performance improvements of fuse-over-io-uring. > >>> > >>> Signed-off-by: Bernd Schubert <bschubert@ddn.com> > >>> Cc: Ingo Molnar <mingo@redhat.com> > >>> Cc: Peter Zijlstra <peterz@infradead.org> > >>> Cc: Andrei Vagin <avagin@google.com> > >> > >> Reviewed-by: Josef Bacik <josef@toxicpanda.com> > >> > >> Probably best to submit this as a one-off so the sched guys can take it and it's > >> not in the middle of a fuse patchset they may be ignoring. Thanks, > > > > On its own its going to not be applied. Never merge an EXPORT without a > > user. > > > > As is, I don't have enough of the series to even see the user, so yeah, > > not happy :/ > > > > And as hch said, this very much needs to be a GPL export. > > Sorry, accidentally done without the _GPL. What is the right way to get this merged? > First merge the entire fuse-io-uring series and then add on this? I already have these > optimization patches at the end of the series... The user for this is in the next patch Yeah, but you didn't send me the next patch, did you? So I have no clue.. :-) Anyway, if you could add a wee comment to __wake_up_con_current_cpu() along with the EXPORT_SYMBOL_GPL() that might be nice. I suppose you can copy paste from __wake_up() and then edit a wee bit. > [PATCH RFC v2 16/19] fuse: {uring} Wake requests on the the current cpu > > diff --git a/fs/fuse/dev.c b/fs/fuse/dev.c > index c7fd3849a105..851c5fa99946 100644 > --- a/fs/fuse/dev.c > +++ b/fs/fuse/dev.c > @@ -333,7 +333,10 @@ void fuse_request_end(struct fuse_req *req) > spin_unlock(&fc->bg_lock); > } else { > /* Wake up waiter sleeping in request_wait_answer() */ > - wake_up(&req->waitq); > + if (fuse_per_core_queue(fc)) > + __wake_up_on_current_cpu(&req->waitq, TASK_NORMAL, NULL); > + else > + wake_up(&req->waitq); > } > > if (test_bit(FR_ASYNC, &req->flags)) Fair enough, although do we want a helper like wake_up() -- something like wake_up_on_current_cpu() ?
On 6/4/24 21:27, Peter Zijlstra wrote: > On Tue, Jun 04, 2024 at 09:36:08AM +0000, Bernd Schubert wrote: >> On 6/4/24 11:26, Peter Zijlstra wrote: >>> On Thu, May 30, 2024 at 04:37:29PM -0400, Josef Bacik wrote: >>>> On Wed, May 29, 2024 at 08:00:50PM +0200, Bernd Schubert wrote: >>>>> This is needed by fuse-over-io-uring to wake up the waiting >>>>> application thread on the core it was submitted from. >>>>> Avoiding core switching is actually a major factor for >>>>> fuse performance improvements of fuse-over-io-uring. >>>>> >>>>> Signed-off-by: Bernd Schubert <bschubert@ddn.com> >>>>> Cc: Ingo Molnar <mingo@redhat.com> >>>>> Cc: Peter Zijlstra <peterz@infradead.org> >>>>> Cc: Andrei Vagin <avagin@google.com> >>>> >>>> Reviewed-by: Josef Bacik <josef@toxicpanda.com> >>>> >>>> Probably best to submit this as a one-off so the sched guys can take it and it's >>>> not in the middle of a fuse patchset they may be ignoring. Thanks, >>> >>> On its own its going to not be applied. Never merge an EXPORT without a >>> user. >>> >>> As is, I don't have enough of the series to even see the user, so yeah, >>> not happy :/ >>> >>> And as hch said, this very much needs to be a GPL export. >> >> Sorry, accidentally done without the _GPL. What is the right way to get this merged? >> First merge the entire fuse-io-uring series and then add on this? I already have these >> optimization patches at the end of the series... The user for this is in the next patch > > Yeah, but you didn't send me the next patch, did you? So I have no > clue.. :-) > > Anyway, if you could add a wee comment to __wake_up_con_current_cpu() > along with the EXPORT_SYMBOL_GPL() that might be nice. I suppose you can > copy paste from __wake_up() and then edit a wee bit. > >> [PATCH RFC v2 16/19] fuse: {uring} Wake requests on the the current cpu >> >> diff --git a/fs/fuse/dev.c b/fs/fuse/dev.c >> index c7fd3849a105..851c5fa99946 100644 >> --- a/fs/fuse/dev.c >> +++ b/fs/fuse/dev.c >> @@ -333,7 +333,10 @@ void fuse_request_end(struct fuse_req *req) >> spin_unlock(&fc->bg_lock); >> } else { >> /* Wake up waiter sleeping in request_wait_answer() */ >> - wake_up(&req->waitq); >> + if (fuse_per_core_queue(fc)) >> + __wake_up_on_current_cpu(&req->waitq, TASK_NORMAL, NULL); >> + else >> + wake_up(&req->waitq); >> } >> >> if (test_bit(FR_ASYNC, &req->flags)) > > Fair enough, although do we want a helper like wake_up() -- something > like wake_up_on_current_cpu() ? Thank you and yes sure! I remove the patch and optimization from RFCv3, we first need to agree on the taken approach and get that merged. Will send submit this optimization immediately after. Thanks, Bernd
diff --git a/kernel/sched/wait.c b/kernel/sched/wait.c index 51e38f5f4701..6576a1ef5d43 100644 --- a/kernel/sched/wait.c +++ b/kernel/sched/wait.c @@ -132,6 +132,7 @@ void __wake_up_on_current_cpu(struct wait_queue_head *wq_head, unsigned int mode { __wake_up_common_lock(wq_head, mode, 1, WF_CURRENT_CPU, key); } +EXPORT_SYMBOL(__wake_up_on_current_cpu); /* * Same as __wake_up but called with the spinlock in wait_queue_head_t held.
This is needed by fuse-over-io-uring to wake up the waiting application thread on the core it was submitted from. Avoiding core switching is actually a major factor for fuse performance improvements of fuse-over-io-uring. Signed-off-by: Bernd Schubert <bschubert@ddn.com> Cc: Ingo Molnar <mingo@redhat.com> Cc: Peter Zijlstra <peterz@infradead.org> Cc: Andrei Vagin <avagin@google.com> --- kernel/sched/wait.c | 1 + 1 file changed, 1 insertion(+)