RE: [PATCH 1/3] rcuwait: Support timeouts

[Dan Williams <dan.j.williams@intel.com>]

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
>