[lttng-dev] [RFC] wfcqueue: allow defining CDS_WFCQ_WAIT_SLEEP to override `poll'

[lttng-dev] [RFC] wfcqueue: allow defining CDS_WFCQ_WAIT_SLEEP to override `poll'

[Eric Wong]

Users may want to use alternative sleeping behavior instead of
`poll'.  Make CDS_WFCQ_WAIT_SLEEP a macro which may be defined
before including wfcqueue.h.

This alternative behavior could include logging, performing
low-priority cleanup work, sleeping a shorter/longer interval
or any combination of that.

This will also make integration into glibc easier, as `poll'
linkage causes conformance test failures even when relegated
to an impossible code path:
https://public-inbox.org/libc-alpha/20180801092626.jrwyrojfye4avcis at whir/

Signed-off-by: Eric Wong <normalperson at yhbt.net>
---
 include/urcu/static/wfcqueue.h | 19 ++++++++++++++++++-
 1 file changed, 18 insertions(+), 1 deletion(-)

diff --git a/include/urcu/static/wfcqueue.h b/include/urcu/static/wfcqueue.h
index 67ac05f..3f56493 100644
--- a/include/urcu/static/wfcqueue.h
+++ b/include/urcu/static/wfcqueue.h
@@ -223,6 +223,23 @@ static inline bool _cds_wfcq_enqueue(cds_wfcq_head_ptr_t head,
 	return ___cds_wfcq_append(head, tail, new_tail, new_tail);
 }
 
+/*
+ * CDS_WFCQ_WAIT_SLEEP:
+ *
+ * By default, this sleeps for the given @msec milliseconds.
+ * This is a macro which LGPL users may #define themselves before
+ * including wfcqueue.h to override the default behavior (e.g.
+ * to log a warning or perform other background work).
+ */
+#ifndef CDS_WFCQ_WAIT_SLEEP
+#define CDS_WFCQ_WAIT_SLEEP(msec) ___cds_wfcq_wait_sleep((msec))
+#endif
+
+static inline void ___cds_wfcq_wait_sleep(int msec)
+{
+	(void) poll(NULL, 0, msec);
+}
+
 /*
  * ___cds_wfcq_busy_wait: adaptative busy-wait.
  *
@@ -234,7 +251,7 @@ ___cds_wfcq_busy_wait(int *attempt, int blocking)
 	if (!blocking)
 		return 1;
 	if (++(*attempt) >= WFCQ_ADAPT_ATTEMPTS) {
-		(void) poll(NULL, 0, WFCQ_WAIT);	/* Wait for 10ms */
+		CDS_WFCQ_WAIT_SLEEP(WFCQ_WAIT);		/* Wait for 10ms */
 		*attempt = 0;
 	} else {
 		caa_cpu_relax();
-- 
EW

[lttng-dev] [RFC] wfcqueue: allow defining CDS_WFCQ_WAIT_SLEEP to override `poll'

[Mathieu Desnoyers]

----- On Aug 1, 2018, at 2:54 PM, Eric Wong normalperson at yhbt.net wrote:

> Users may want to use alternative sleeping behavior instead of
> `poll'.  Make CDS_WFCQ_WAIT_SLEEP a macro which may be defined
> before including wfcqueue.h.
> 
> This alternative behavior could include logging, performing
> low-priority cleanup work, sleeping a shorter/longer interval
> or any combination of that.
> 
> This will also make integration into glibc easier, as `poll'
> linkage causes conformance test failures even when relegated
> to an impossible code path:
> https://public-inbox.org/libc-alpha/20180801092626.jrwyrojfye4avcis at whir/

Rather than introducing a macro here, can we extend the API to
pass a callback and a private pointer that would perform the
sleeping behavior requested by the caller ?

Thanks,

Mathieu

> 
> Signed-off-by: Eric Wong <normalperson at yhbt.net>
> ---
> include/urcu/static/wfcqueue.h | 19 ++++++++++++++++++-
> 1 file changed, 18 insertions(+), 1 deletion(-)
> 
> diff --git a/include/urcu/static/wfcqueue.h b/include/urcu/static/wfcqueue.h
> index 67ac05f..3f56493 100644
> --- a/include/urcu/static/wfcqueue.h
> +++ b/include/urcu/static/wfcqueue.h
> @@ -223,6 +223,23 @@ static inline bool _cds_wfcq_enqueue(cds_wfcq_head_ptr_t
> head,
> 	return ___cds_wfcq_append(head, tail, new_tail, new_tail);
> }
> 
> +/*
> + * CDS_WFCQ_WAIT_SLEEP:
> + *
> + * By default, this sleeps for the given @msec milliseconds.
> + * This is a macro which LGPL users may #define themselves before
> + * including wfcqueue.h to override the default behavior (e.g.
> + * to log a warning or perform other background work).
> + */
> +#ifndef CDS_WFCQ_WAIT_SLEEP
> +#define CDS_WFCQ_WAIT_SLEEP(msec) ___cds_wfcq_wait_sleep((msec))
> +#endif
> +
> +static inline void ___cds_wfcq_wait_sleep(int msec)
> +{
> +	(void) poll(NULL, 0, msec);
> +}
> +
> /*
>  * ___cds_wfcq_busy_wait: adaptative busy-wait.
>  *
> @@ -234,7 +251,7 @@ ___cds_wfcq_busy_wait(int *attempt, int blocking)
> 	if (!blocking)
> 		return 1;
> 	if (++(*attempt) >= WFCQ_ADAPT_ATTEMPTS) {
> -		(void) poll(NULL, 0, WFCQ_WAIT);	/* Wait for 10ms */
> +		CDS_WFCQ_WAIT_SLEEP(WFCQ_WAIT);		/* Wait for 10ms */
> 		*attempt = 0;
> 	} else {
> 		caa_cpu_relax();
> --
> EW

-- 
Mathieu Desnoyers
EfficiOS Inc.
http://www.efficios.com

[lttng-dev] [RFC] wfcqueue: allow defining CDS_WFCQ_WAIT_SLEEP to override `poll'

[Eric Wong]

Mathieu Desnoyers <mathieu.desnoyers at efficios.com> wrote:
> ----- On Aug 1, 2018, at 2:54 PM, Eric Wong normalperson at yhbt.net wrote:
> 
> > Users may want to use alternative sleeping behavior instead of
> > `poll'.  Make CDS_WFCQ_WAIT_SLEEP a macro which may be defined
> > before including wfcqueue.h.
> > 
> > This alternative behavior could include logging, performing
> > low-priority cleanup work, sleeping a shorter/longer interval
> > or any combination of that.
> > 
> > This will also make integration into glibc easier, as `poll'
> > linkage causes conformance test failures even when relegated
> > to an impossible code path:
> > https://public-inbox.org/libc-alpha/20180801092626.jrwyrojfye4avcis at whir/
> 
> Rather than introducing a macro here, can we extend the API to
> pass a callback and a private pointer that would perform the
> sleeping behavior requested by the caller ?

Given this is deep in the call stack, it would require modifying
a lot of callers.  So it seems like a compatibility/migration
nightmare.  I fear it's too much work for a small improvement.

For glibc, we can simply define `poll' to `__poll' to avoid
those test failures, at least

[lttng-dev] [RFC] wfcqueue: allow defining CDS_WFCQ_WAIT_SLEEP to override `poll'

[Mathieu Desnoyers]

----- On Aug 17, 2018, at 4:31 PM, Eric Wong normalperson at yhbt.net wrote:

> Mathieu Desnoyers <mathieu.desnoyers at efficios.com> wrote:
>> ----- On Aug 1, 2018, at 2:54 PM, Eric Wong normalperson at yhbt.net wrote:
>> 
>> > Users may want to use alternative sleeping behavior instead of
>> > `poll'.  Make CDS_WFCQ_WAIT_SLEEP a macro which may be defined
>> > before including wfcqueue.h.
>> > 
>> > This alternative behavior could include logging, performing
>> > low-priority cleanup work, sleeping a shorter/longer interval
>> > or any combination of that.
>> > 
>> > This will also make integration into glibc easier, as `poll'
>> > linkage causes conformance test failures even when relegated
>> > to an impossible code path:
>> > https://public-inbox.org/libc-alpha/20180801092626.jrwyrojfye4avcis at whir/
>> 
>> Rather than introducing a macro here, can we extend the API to
>> pass a callback and a private pointer that would perform the
>> sleeping behavior requested by the caller ?
> 
> Given this is deep in the call stack, it would require modifying
> a lot of callers.  So it seems like a compatibility/migration
> nightmare.  I fear it's too much work for a small improvement.
> 
> For glibc, we can simply define `poll' to `__poll' to avoid
> those test failures, at least

I merged the patch you proposed, it makes sense considering the
context.

Thanks!

Mathieu


-- 
Mathieu Desnoyers
EfficiOS Inc.
http://www.efficios.com

[lttng-dev] [RFC] wfcqueue: allow defining CDS_WFCQ_WAIT_SLEEP to override `poll'

[Mathieu Desnoyers]

----- On Aug 1, 2018, at 2:54 PM, Eric Wong normalperson at yhbt.net wrote:

> Users may want to use alternative sleeping behavior instead of
> `poll'.  Make CDS_WFCQ_WAIT_SLEEP a macro which may be defined
> before including wfcqueue.h.
> 
> This alternative behavior could include logging, performing
> low-priority cleanup work, sleeping a shorter/longer interval
> or any combination of that.
> 
> This will also make integration into glibc easier, as `poll'
> linkage causes conformance test failures even when relegated
> to an impossible code path:
> https://public-inbox.org/libc-alpha/20180801092626.jrwyrojfye4avcis at whir/
> 
> Signed-off-by: Eric Wong <normalperson at yhbt.net>
> ---
> include/urcu/static/wfcqueue.h | 19 ++++++++++++++++++-
> 1 file changed, 18 insertions(+), 1 deletion(-)
> 
> diff --git a/include/urcu/static/wfcqueue.h b/include/urcu/static/wfcqueue.h
> index 67ac05f..3f56493 100644
> --- a/include/urcu/static/wfcqueue.h
> +++ b/include/urcu/static/wfcqueue.h
> @@ -223,6 +223,23 @@ static inline bool _cds_wfcq_enqueue(cds_wfcq_head_ptr_t
> head,
> 	return ___cds_wfcq_append(head, tail, new_tail, new_tail);
> }
> 
> +/*
> + * CDS_WFCQ_WAIT_SLEEP:
> + *
> + * By default, this sleeps for the given @msec milliseconds.
> + * This is a macro which LGPL users may #define themselves before
> + * including wfcqueue.h to override the default behavior (e.g.
> + * to log a warning or perform other background work).
> + */
> +#ifndef CDS_WFCQ_WAIT_SLEEP
> +#define CDS_WFCQ_WAIT_SLEEP(msec) ___cds_wfcq_wait_sleep((msec))

Removing the extra () around msec, they are not needed.

Otherwise, merged into liburcu master branch, thanks!

Mathieu

> +#endif
> +
> +static inline void ___cds_wfcq_wait_sleep(int msec)
> +{
> +	(void) poll(NULL, 0, msec);
> +}
> +
> /*
>  * ___cds_wfcq_busy_wait: adaptative busy-wait.
>  *
> @@ -234,7 +251,7 @@ ___cds_wfcq_busy_wait(int *attempt, int blocking)
> 	if (!blocking)
> 		return 1;
> 	if (++(*attempt) >= WFCQ_ADAPT_ATTEMPTS) {
> -		(void) poll(NULL, 0, WFCQ_WAIT);	/* Wait for 10ms */
> +		CDS_WFCQ_WAIT_SLEEP(WFCQ_WAIT);		/* Wait for 10ms */
> 		*attempt = 0;
> 	} else {
> 		caa_cpu_relax();
> --
> EW

-- 
Mathieu Desnoyers
EfficiOS Inc.
http://www.efficios.com