| Message ID | 20230502171841.21317-2-dave@stgolabs.net |
|---|---|
| State | Superseded |
| Headers | show |
| Series | cxl: Handle background commands | expand |
Davidlohr Bueso wrote: > Introduce rcuwait_wait_event_timeout(), with semantics equivalent > to calls for the simple and regular waitqueue counterparts. So my first reaction to this is "what is rcuwait, never heard of that before?", and "how did Davidlohr find this facility and why does he think it suitable here?". Then I go to read the history to get smarter about this thing and the author is: Davidlohr Bueso <dave@stgolabs.net>, aha! The critical insight for me was: https://lore.kernel.org/all/1484148146-14210-2-git-send-email-dave@stgolabs.net/ ...i.e. that rcuwait is suitable for waiting for a condition to fire while holding another exclusive lock like a mutex() as is the case with the CXL mbox code. Did I get that right? My recommendation would be to include some text like "Recall that rcuwait is suitable in this scenario because..." reminders at least until more driver people understand when and how to use it. > Cc: peterz@infradead.org > Cc: mingo@redhat.com > Signed-off-by: Davidlohr Bueso <dave@stgolabs.net> > --- > Only cc'ing sched folks this patch to avoid spamming. fyi the actual user > comes up in patch 3 (cxl/mbox: Add background cmd handling machinery), > but found a few other potential users in various drivers, so more could > be added. FWIW this looks like a straightforward addition of timeout support to rcuwait_wait_event() to me. What acks are required to take this through the CXL tree? Or are you the rcuwait maintainer? > > include/linux/rcuwait.h | 23 ++++++++++++++++++++--- > 1 file changed, 20 insertions(+), 3 deletions(-) > > diff --git a/include/linux/rcuwait.h b/include/linux/rcuwait.h > index 8052d34da782..27343424225c 100644 > --- a/include/linux/rcuwait.h > +++ b/include/linux/rcuwait.h > @@ -49,9 +49,9 @@ static inline void prepare_to_rcuwait(struct rcuwait *w) > > extern void finish_rcuwait(struct rcuwait *w); > > -#define rcuwait_wait_event(w, condition, state) \ > +#define ___rcuwait_wait_event(w, condition, state, ret, cmd) \ > ({ \ > - int __ret = 0; \ > + long __ret = ret; \ > prepare_to_rcuwait(w); \ > for (;;) { \ > /* \ > @@ -67,10 +67,27 @@ extern void finish_rcuwait(struct rcuwait *w); > break; \ > } \ > \ > - schedule(); \ > + cmd; \ > } \ > finish_rcuwait(w); \ > __ret; \ > }) > > +#define rcuwait_wait_event(w, condition, state) \ > + ___rcuwait_wait_event(w, condition, state, 0, schedule()) > + > +#define __rcuwait_wait_event_timeout(w, condition, state, timeout) \ > + ___rcuwait_wait_event(w, ___wait_cond_timeout(condition), \ > + state, timeout, \ > + __ret = schedule_timeout(__ret)) > + > +#define rcuwait_wait_event_timeout(w, condition, state, timeout) \ > +({ \ > + long __ret = timeout; \ > + if (!___wait_cond_timeout(condition)) \ > + __ret = __rcuwait_wait_event_timeout(w, condition, \ > + state, timeout); \ > + __ret; \ > +}) > + > #endif /* _LINUX_RCUWAIT_H_ */ > -- > 2.40.1 >
On Fri, 19 May 2023, Dan Williams wrote: >Davidlohr Bueso wrote: >> Introduce rcuwait_wait_event_timeout(), with semantics equivalent >> to calls for the simple and regular waitqueue counterparts. > >So my first reaction to this is "what is rcuwait, never heard of that >before?", and "how did Davidlohr find this facility and why does he >think it suitable here?". Then I go to read the history to get smarter >about this thing and the author is: Davidlohr Bueso <dave@stgolabs.net>, >aha! > >The critical insight for me was: > >https://lore.kernel.org/all/1484148146-14210-2-git-send-email-dave@stgolabs.net/ Just a general note that per 154abafc68b (tasks, sched/core: With a grace period after finish_task_switch(), remove unnecessary code) ... that the exit_notify() caveat no longer applies. >...i.e. that rcuwait is suitable for waiting for a condition to fire >while holding another exclusive lock like a mutex() as is the case with >the CXL mbox code. Did I get that right? Yes. For this particular patchset it made sense to go this way in terms of semantics (this use-case is obviously not a performance sensitive path, nor do any of the rt benefits of not having to deal with a q->lock). >My recommendation would be to include some text like "Recall that >rcuwait is suitable in this scenario because..." reminders at least >until more driver people understand when and how to use it. Sure. >> Cc: peterz@infradead.org >> Cc: mingo@redhat.com >> Signed-off-by: Davidlohr Bueso <dave@stgolabs.net> >> --- >> Only cc'ing sched folks this patch to avoid spamming. fyi the actual user >> comes up in patch 3 (cxl/mbox: Add background cmd handling machinery), >> but found a few other potential users in various drivers, so more could >> be added. > >FWIW this looks like a straightforward addition of timeout support to >rcuwait_wait_event() to me. > >What acks are required to take this through the CXL tree? Or are you >the rcuwait maintainer? No, I am not the maintainer, changes have been routed usually through tip folks. Conceptually at least I think Peter was ok with it, but ultimately an explicit ack/nack would be required. Thanks, Davidlohr
On Fri, May 19, 2023 at 03:55:04PM -0700, Davidlohr Bueso wrote: > On Fri, 19 May 2023, Dan Williams wrote: > > What acks are required to take this through the CXL tree? Or are you > > the rcuwait maintainer? > > No, I am not the maintainer, changes have been routed usually through > tip folks. Conceptually at least I think Peter was ok with it, but > ultimately an explicit ack/nack would be required. Yeah, would normally be me routing it through the sched tree -- that also takes all the other wait (wait and swait) crud. If this needs to go elsewhere, Acked-by: Peter Zijlstra (Intel) <peterz@infradead.org>
diff --git a/include/linux/rcuwait.h b/include/linux/rcuwait.h index 8052d34da782..27343424225c 100644 --- a/include/linux/rcuwait.h +++ b/include/linux/rcuwait.h @@ -49,9 +49,9 @@ static inline void prepare_to_rcuwait(struct rcuwait *w) extern void finish_rcuwait(struct rcuwait *w); -#define rcuwait_wait_event(w, condition, state) \ +#define ___rcuwait_wait_event(w, condition, state, ret, cmd) \ ({ \ - int __ret = 0; \ + long __ret = ret; \ prepare_to_rcuwait(w); \ for (;;) { \ /* \ @@ -67,10 +67,27 @@ extern void finish_rcuwait(struct rcuwait *w); break; \ } \ \ - schedule(); \ + cmd; \ } \ finish_rcuwait(w); \ __ret; \ }) +#define rcuwait_wait_event(w, condition, state) \ + ___rcuwait_wait_event(w, condition, state, 0, schedule()) + +#define __rcuwait_wait_event_timeout(w, condition, state, timeout) \ + ___rcuwait_wait_event(w, ___wait_cond_timeout(condition), \ + state, timeout, \ + __ret = schedule_timeout(__ret)) + +#define rcuwait_wait_event_timeout(w, condition, state, timeout) \ +({ \ + long __ret = timeout; \ + if (!___wait_cond_timeout(condition)) \ + __ret = __rcuwait_wait_event_timeout(w, condition, \ + state, timeout); \ + __ret; \ +}) + #endif /* _LINUX_RCUWAIT_H_ */
Introduce rcuwait_wait_event_timeout(), with semantics equivalent to calls for the simple and regular waitqueue counterparts. Cc: peterz@infradead.org Cc: mingo@redhat.com Signed-off-by: Davidlohr Bueso <dave@stgolabs.net> --- Only cc'ing sched folks this patch to avoid spamming. fyi the actual user comes up in patch 3 (cxl/mbox: Add background cmd handling machinery), but found a few other potential users in various drivers, so more could be added. include/linux/rcuwait.h | 23 ++++++++++++++++++++--- 1 file changed, 20 insertions(+), 3 deletions(-)