[lttng-dev] urcu 7ca7fe9c03 + _LGPL_SOURCE regression?

[Mathieu Desnoyers]

----- On Aug 9, 2022, at 2:19 PM, Eric Wong via lttng-dev lttng-dev at lists.lttng.org wrote:

> Hello, I've noticed liburcu v0.11.4+ and later fails with the
> "mwrap" LD_PRELOAD malloc wrapper which initializes an rculfhash
> in a constructor.
> 
> The problem happens when using _LGPL_SOURCE + rcu_dereference on
> a cds_lfht pointer:
> 
>  In file included from /tmp/b/include/urcu/pointer.h:39,
>                   from /tmp/b/include/urcu/urcu-bp.h:58,
>                   from /tmp/b/include/urcu-bp.h:2,
>                   from ../7ca7fe9c_regression.c:10:
>  ../7ca7fe9c_regression.c: In function ‘main’:
>  /tmp/b/include/urcu/static/pointer.h:101:18: error: invalid use of undefined
>  type ‘struct cds_lfht’
>    101 |     __typeof__(p + 0) _________p1;    \
>        |                  ^
>  /tmp/b/include/urcu/pointer.h:47:26: note: in expansion of macro
>  ‘_rcu_dereference’
>     47 | #define rcu_dereference  _rcu_dereference
>        |                          ^~~~~~~~~~~~~~~~
>  ../7ca7fe9c_regression.c:26:23: note: in expansion of macro ‘rcu_dereference’
>     26 |  struct cds_lfht *t = rcu_dereference(totals);
>        |                       ^~~~~~~~~~~~~~~
> 
> Removing _LGPL_SOURCE avoids the problem, but I'd rather not
> introduce performance regressions.

Fixed by commits:

commit 2d466a6397dbc7af397d0fc10e327cc6cac76a5a
Author: Simon Marchi <simon.marchi at efficios.com>
Date:   Wed Aug 17 11:24:25 2022 -0400

    Fix: change method used by _rcu_dereference to strip type constness

and

commit fada682f0d6e680f18e3243aa0af607dfafcbd32 (review/master)
Author: Mathieu Desnoyers <mathieu.desnoyers at efficios.com>
Date:   Wed Aug 17 15:32:38 2022 -0400

    Fix: add missing unused attribute to _rcu_dereference

Those will be released in the 0.12 and 0.13 stable branches of liburcu.

Note that the 0.11 liburcu branch is end of life and will not have any
further release, so you may want to upgrade if you still use it.

> 
> In retrospect, my use of rcu_dereference seems unnecessary since
> constructor functions should always fire before any threads are
> created.
> 
> That means I can safely replace the assignment w/ CMM_STORE_SHARED,
> and rcu_dereference with CMM_LOAD_SHARED, correct?

I would not recommend it. Note that cds_lfht_new() starts a worker
thread (from a constructor) in your code. So there is concurrent
access after that hash table creation, even if you access it from
constructor code.

Thanks,

Mathieu

> 
> Sample code:
> /*
> * shows a regression with _LGPL_SOURCE + urcu v0.11.4, commit:
> * 7ca7fe9c03217813 (Make temporary variable in _rcu_dereference non-const,
> 2021-07-29)
> * $(CC) -o x -I/path/to/include/ -L/path/to/lib/ __FILE__ -lurcu-bp -lurcu-cds
> *
> * Originally noticed in an LD_PRELOAD malloc wrapper:
> * https://80x24.org/mwrap.git (Ruby 2.6+)
> * https://80x24.org/mwrap-perl.git (Perl5)
> */
> #define _LGPL_SOURCE
> #include <urcu-bp.h>
> #include <urcu/rculfhash.h>
> static struct cds_lfht *totals; /* immortal */
> 
> __attribute__((constructor)) static void init_lfht(void)
> {
>	/* XXX replace with CMM_STORE_SHARED? */
>	totals = cds_lfht_new(16384, 1, 0, CDS_LFHT_AUTO_RESIZE, 0);
> }
> 
> int main(void) /* this is meant to be a malloc-family function in mwrap */
> {
>	/* XXX replace rcu_dereference with CMM_LOAD_SHARED? */
>	struct cds_lfht *t = rcu_dereference(totals);
>	return t ? 0 : 1;
> }
> ---
> Thanks for many years of urcu + cds!
> _______________________________________________
> lttng-dev mailing list
> lttng-dev at lists.lttng.org
> https://lists.lttng.org/cgi-bin/mailman/listinfo/lttng-dev

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