Re: [lttng-dev] urcu 7ca7fe9c03 + _LGPL_SOURCE regression? - Eric Wong via lttng-dev

From: Eric Wong via lttng-dev <lttng-dev@lists.lttng.org>
To: Mathieu Desnoyers <mathieu.desnoyers@efficios.com>
Cc: lttng-dev <lttng-dev@lists.lttng.org>,
	Sam Saffron <sam.saffron@gmail.com>,
	Simon Marchi <simon.marchi@efficios.com>
Subject: Re: [lttng-dev] urcu 7ca7fe9c03 + _LGPL_SOURCE regression?
Date: Sat, 20 Aug 2022 18:51:43 +0000	[thread overview]
Message-ID: <20220820185143.GA5248@dcvr> (raw)
In-Reply-To: <2040534742.15368.1660848925552.JavaMail.zimbra@efficios.com>

Mathieu Desnoyers <mathieu.desnoyers@efficios.com> wrote:
> ----- On Aug 9, 2022, at 2:19 PM, Eric Wong via lttng-dev lttng-dev@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@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@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.

Thanks.

> 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.

Unfortunately, enterprise distro users are likely to be stuck
on 0.11 (or even older) for a long time :<

> > 
> > 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.

Noted.  However, I'm not exactly worried about losing reporting
for a few allocations in the early boot process, it's inevitable.
Allocations done by cds are the least of a Rubyist's problems.

The original code didn't even bother using rcu_assign_pointer
inside the resolve_malloc constructor :x

https://80x24.org/mwrap-public/20220815212217.GA11237@dcvr/

postimage w/ patch applied:
  https://80x24.org/mwrap-public/477b1cb/s/?b=ext/mwrap/mwrap.c#n139

All the malloc wrappers themselves are immune to cds_lfht totals
pointer being NULL.

Thanks.
_______________________________________________
lttng-dev mailing list
lttng-dev@lists.lttng.org
https://lists.lttng.org/cgi-bin/mailman/listinfo/lttng-dev