'call_rcu' unstable?

'call_rcu' unstable?

[zs]

Hi list,

I found a big problem in my product, that use urcu 0.7.5. My program cost too mutch CPU in the funtion 'update_counter_and_wait:uruc.c:247', and I use gdb to see to *wait_loops*, it says -167777734. The CPU usage grows up from 1% to 100% in one day!


Here is the sample code to show how I use urcu library:

#include <urcu.h>

thread ()
{
rcu_register_thread();

for (;;) {
rcu_read_lock();
xxx
rcu_read_unlock();
}
}

main()
{
rcu_init();
  pthread_create(, , , , thread);

rcu_register_thread();
for (;;)
{
 if (xxx)
   call_rcu();
}

}

Re: 'call_rcu' unstable?

[Mathieu Desnoyers]

* zs (84500316@qq.com) wrote:
> Hi list,
> 
> I found a big problem in my product, that use urcu 0.7.5. My program cost too mutch CPU in the funtion 'update_counter_and_wait:uruc.c:247', and I use gdb to see to *wait_loops*, it says -167777734. The CPU usage grows up from 1% to 100% in one day!
> 
> 
> Here is the sample code to show how I use urcu library:
> 
> #include <urcu.h>
> 
> thread ()
> {
> rcu_register_thread();
> 
> for (;;) {
> rcu_read_lock();
> xxx
> rcu_read_unlock();

Please triple-check that all your rcu_read_lock() and rcu_read_unlock()
are balanced (no double-unlock, nor missing unlock for each lock taken).

The type of problem you get would happen in such a case.

Thanks,

Mathieu

> }
> }
> 
> main()
> {
> rcu_init();
>   pthread_create(, , , , thread);
> 
> rcu_register_thread();
> for (;;)
> {
>  if (xxx)
>    call_rcu();
> }
> 
> }
> _______________________________________________
> lttng-dev mailing list
> lttng-dev@lists.lttng.org
> http://lists.lttng.org/cgi-bin/mailman/listinfo/lttng-dev

-- 
Mathieu Desnoyers
Operating System Efficiency R&D Consultant
EfficiOS Inc.
http://www.efficios.com

Re: 'call_rcu' unstable?

[zs]

Thanks , BUT ...
I really check my code:

zs# find .|xargs grep rcu_read 

./sslvpn_ctx.c: rcu_read_lock();
./sslvpn_ctx.c:                 rcu_read_unlock();
./sslvpn_ctx.c: rcu_read_unlock();

AND in sslvpn_ctx.c:
void *sslvpn_lookup_ssl(unsigned long iip)
{
        struct sslvpn_ctx *ctx;
        int h;

        h = get_hash(iip, 0);

        rcu_read_lock();
        cds_list_for_each_entry_rcu(ctx, &g_sslvpnctxlist[h], cdlist) {
                if ((ctx->flags & SSL_CTX_ESTABLISHED) && ctx->iip && ctx->iip == iip) {

                        uatomic_add(&ctx->ssl_use, 1);
                        rcu_read_unlock();
                        return ctx;
                }
        }

        rcu_read_unlock();
        return NULL;
}

By the way, *sslvpn_lookup_ssl* called by 6 threads for TX.
the 7th thread only will call *call_rcu*:

int sslvpn_del_ctx(struct sslvpn_ctx *pctx)
{
        ...
        cds_list_del_rcu(&ctx->cdlist);
        ctx->flags |= SSL_CTX_DYING;
        call_rcu(&ctx->rcu, func);
        ...
}





------------------ Original ------------------
From:  "Mathieu Desnoyers"<mathieu.desnoyers@efficios.com>;
Date:  Wed, Dec 12, 2012 01:55 AM
To:  "zs"<84500316@qq.com>; 
Cc:  "lttng-dev"<lttng-dev@lists.lttng.org>; 
Subject:  Re: [lttng-dev] 'call_rcu' unstable?



* zs (84500316@qq.com) wrote:
> Hi list,
> 
> I found a big problem in my product, that use urcu 0.7.5. My program cost too mutch CPU in the funtion 'update_counter_and_wait:uruc.c:247', and I use gdb to see to *wait_loops*, it says -167777734. The CPU usage grows up from 1% to 100% in one day!
> 
> 
> Here is the sample code to show how I use urcu library:
> 
> #include <urcu.h>
> 
> thread ()
> {
> rcu_register_thread();
> 
> for (;;) {
> rcu_read_lock();
> xxx
> rcu_read_unlock();

Please triple-check that all your rcu_read_lock() and rcu_read_unlock()
are balanced (no double-unlock, nor missing unlock for each lock taken).

The type of problem you get would happen in such a case.

Thanks,

Mathieu

> }
> }
> 
> main()
> {
> rcu_init();
>   pthread_create(, , , , thread);
> 
> rcu_register_thread();
> for (;;)
> {
>  if (xxx)
>    call_rcu();
> }
> 
> }
> _______________________________________________
> lttng-dev mailing list
> lttng-dev@lists.lttng.org
> http://lists.lttng.org/cgi-bin/mailman/listinfo/lttng-dev

-- 
Mathieu Desnoyers
Operating System Efficiency R&D Consultant
EfficiOS Inc.
http://www.efficios.com

Re: 'call_rcu' unstable?

[Mathieu Desnoyers]

* zs (84500316@qq.com) wrote:
> Thanks , BUT ...
> I really check my code:
> 
> zs# find .|xargs grep rcu_read 
> 
> ./sslvpn_ctx.c: rcu_read_lock();
> ./sslvpn_ctx.c:                 rcu_read_unlock();
> ./sslvpn_ctx.c: rcu_read_unlock();

OK, in that case, continuing on the debugging checklist:

- Did you ensure that you issue rcu_register_thread() at thread start of
  each of your threads ? And rcu_register_thread before returning from
  each thread ?
- How is the list g_sslvpnctxlist[h] initialized ? Are there concurrent
  modifications of this list ? If yes, are they synchronized with a
  mutex ?
- When your application is a state where the call_rcu worker thread
  busy-waits on RCU read-side critical sections, it would be interesting
  to know on what read-side critical section it is waiting. In order to
  do so, from gdb attached to your process when it is hung:
  - we'd need to look at the urcu.c "registry" list. We'd need to figure
    out which list entries are keeping the busy-loop waiting.
  - then, we should look at each thread's "rcu_reader" TLS variable, to
    see its address and content.
  By comparing the content of the list and each active thread's
  rcu_reader TLS variable, we should be able to figure out what is
  keeping grace period to complete. If you can provide these dumps, it
  would let me help you digging further into your issue.

Thanks,

Mathieu


> 
> AND in sslvpn_ctx.c:
> void *sslvpn_lookup_ssl(unsigned long iip)
> {
>         struct sslvpn_ctx *ctx;
>         int h;
> 
>         h = get_hash(iip, 0);
> 
>         rcu_read_lock();
>         cds_list_for_each_entry_rcu(ctx, &g_sslvpnctxlist[h], cdlist) {
>                 if ((ctx->flags & SSL_CTX_ESTABLISHED) && ctx->iip && ctx->iip == iip) {
> 
>                         uatomic_add(&ctx->ssl_use, 1);
>                         rcu_read_unlock();
>                         return ctx;
>                 }
>         }
> 
>         rcu_read_unlock();
>         return NULL;
> }
> 
> By the way, *sslvpn_lookup_ssl* called by 6 threads for TX.
> the 7th thread only will call *call_rcu*:
> 
> int sslvpn_del_ctx(struct sslvpn_ctx *pctx)
> {
>         ...
>         cds_list_del_rcu(&ctx->cdlist);
>         ctx->flags |= SSL_CTX_DYING;
>         call_rcu(&ctx->rcu, func);
>         ...
> }
> 
> 
> 
> 
> 
> ------------------ Original ------------------
> From:  "Mathieu Desnoyers"<mathieu.desnoyers@efficios.com>;
> Date:  Wed, Dec 12, 2012 01:55 AM
> To:  "zs"<84500316@qq.com>; 
> Cc:  "lttng-dev"<lttng-dev@lists.lttng.org>; 
> Subject:  Re: [lttng-dev] 'call_rcu' unstable?
> 
> 
> 
> * zs (84500316@qq.com) wrote:
> > Hi list,
> > 
> > I found a big problem in my product, that use urcu 0.7.5. My program cost too mutch CPU in the funtion 'update_counter_and_wait:uruc.c:247', and I use gdb to see to *wait_loops*, it says -167777734. The CPU usage grows up from 1% to 100% in one day!
> > 
> > 
> > Here is the sample code to show how I use urcu library:
> > 
> > #include <urcu.h>
> > 
> > thread ()
> > {
> > rcu_register_thread();
> > 
> > for (;;) {
> > rcu_read_lock();
> > xxx
> > rcu_read_unlock();
> 
> Please triple-check that all your rcu_read_lock() and rcu_read_unlock()
> are balanced (no double-unlock, nor missing unlock for each lock taken).
> 
> The type of problem you get would happen in such a case.
> 
> Thanks,
> 
> Mathieu
> 
> > }
> > }
> > 
> > main()
> > {
> > rcu_init();
> >   pthread_create(, , , , thread);
> > 
> > rcu_register_thread();
> > for (;;)
> > {
> >  if (xxx)
> >    call_rcu();
> > }
> > 
> > }
> > _______________________________________________
> > lttng-dev mailing list
> > lttng-dev@lists.lttng.org
> > http://lists.lttng.org/cgi-bin/mailman/listinfo/lttng-dev
> 
> -- 
> Mathieu Desnoyers
> Operating System Efficiency R&D Consultant
> EfficiOS Inc.
> http://www.efficios.com
-- 
Mathieu Desnoyers
Operating System Efficiency R&D Consultant
EfficiOS Inc.
http://www.efficios.com

Re: 'call_rcu' unstable?

[zs]

thanks Mathieu Desnoyers for the patience.

>- Did you ensure that you issue rcu_register_thread() at thread start of
>  each of your threads ? And rcu_register_thread before returning from
>  each thread ?
Sure.

>- How is the list g_sslvpnctxlist[h] initialized ? 
Here, is the way:
I alloc and init 'g_sslvpnctxlist' in one process, and exec rcu_read_lock/unlock/call_rcu in another process(which create many rx/tx threads). 
        if (sslvpn_shm_alloc(&shm) == -1) {
                syslog(LOG_ERR, "alloc share stats memory failed %s\n", strerror(errno));
                exit(-1);
        }
        g_sslvpnctxlist = (void *)shm.addr;
        for (i = 0; i < sslvpn_max_users; i++)
                 CDS_INIT_LIST_HEAD(&g_sslvpnctxlist[i]);
It looks strange, that someone uses share-memory(created by mmap) to hold the 'g_sslvpnctxlist' (+_+);
Later I will re-coding this part, not use share-memory. 

Are there concurrent
>  modifications of this list ? If yes, are they synchronized with a
>  mutex ?
I do not use mutex, because all the add/del are executing in one thread. 

>- When your application is a state where the call_rcu worker thread
>  busy-waits on RCU read-side critical sections, it would be interesting
>  to know on what read-side critical section it is waiting. In order to
>  do so, from gdb attached to your process when it is hung:
>  - we'

  I did attach the process and check each thread, it says:
six threads block in 'msgrcv' IO, and one thread hangs in 'update_counter_and_wait:uruc.c:247'

  I do not have the chance to check the rcu_reader TLS values,
 because my customer will not allow the problem happlen again(I have replaced rcu_lock by pthread_mutex).

 I am trying to reproduce the problem in my testing workplace, if happens, I will give more.

thanks .
   



------------------ Original ------------------
From:  "Mathieu Desnoyers"<mathieu.desnoyers@efficios.com>;
Date:  Wed, Dec 12, 2012 10:41 AM
To:  "zs"<84500316@qq.com>; 
Cc:  "lttng-dev"<lttng-dev@lists.lttng.org>; 
Subject:  Re: [lttng-dev] 'call_rcu' unstable?



* zs (84500316@qq.com) wrote:
> Thanks , BUT ...
> I really check my code:
> 
> zs# find .|xargs grep rcu_read 
> 
> ./sslvpn_ctx.c: rcu_read_lock();
> ./sslvpn_ctx.c:                 rcu_read_unlock();
> ./sslvpn_ctx.c: rcu_read_unlock();

OK, in that case, continuing on the debugging checklist:

- Did you ensure that you issue rcu_register_thread() at thread start of
  each of your threads ? And rcu_register_thread before returning from
  each thread ?
- How is the list g_sslvpnctxlist[h] initialized ? Are there concurrent
  modifications of this list ? If yes, are they synchronized with a
  mutex ?
- When your application is a state where the call_rcu worker thread
  busy-waits on RCU read-side critical sections, it would be interesting
  to know on what read-side critical section it is waiting. In order to
  do so, from gdb attached to your process when it is hung:
  - we'd need to look at the urcu.c "registry" list. We'd need to figure
    out which list entries are keeping the busy-loop waiting.
  - then, we should look at each thread's "rcu_reader" TLS variable, to
    see its address and content.
  By comparing the content of the list and each active thread's
  rcu_reader TLS variable, we should be able to figure out what is
  keeping grace period to complete. If you can provide these dumps, it
  would let me help you digging further into your issue.

Thanks,

Mathieu


> 
> AND in sslvpn_ctx.c:
> void *sslvpn_lookup_ssl(unsigned long iip)
> {
>         struct sslvpn_ctx *ctx;
>         int h;
> 
>         h = get_hash(iip, 0);
> 
>         rcu_read_lock();
>         cds_list_for_each_entry_rcu(ctx, &g_sslvpnctxlist[h], cdlist) {
>                 if ((ctx->flags & SSL_CTX_ESTABLISHED) && ctx->iip && ctx->iip == iip) {
> 
>                         uatomic_add(&ctx->ssl_use, 1);
>                         rcu_read_unlock();
>                         return ctx;
>                 }
>         }
> 
>         rcu_read_unlock();
>         return NULL;
> }
> 
> By the way, *sslvpn_lookup_ssl* called by 6 threads for TX.
> the 7th thread only will call *call_rcu*:
> 
> int sslvpn_del_ctx(struct sslvpn_ctx *pctx)
> {
>         ...
>         cds_list_del_rcu(&ctx->cdlist);
>         ctx->flags |= SSL_CTX_DYING;
>         call_rcu(&ctx->rcu, func);
>         ...
> }
> 
> 
> 
> 
> 
> ------------------ Original ------------------
> From:  "Mathieu Desnoyers"<mathieu.desnoyers@efficios.com>;
> Date:  Wed, Dec 12, 2012 01:55 AM
> To:  "zs"<84500316@qq.com>; 
> Cc:  "lttng-dev"<lttng-dev@lists.lttng.org>; 
> Subject:  Re: [lttng-dev] 'call_rcu' unstable?
> 
> 
> 
> * zs (84500316@qq.com) wrote:
> > Hi list,
> > 
> > I found a big problem in my product, that use urcu 0.7.5. My program cost too mutch CPU in the funtion 'update_counter_and_wait:uruc.c:247', and I use gdb to see to *wait_loops*, it says -167777734. The CPU usage grows up from 1% to 100% in one day!
> > 
> > 
> > Here is the sample code to show how I use urcu library:
> > 
> > #include <urcu.h>
> > 
> > thread ()
> > {
> > rcu_register_thread();
> > 
> > for (;;) {
> > rcu_read_lock();
> > xxx
> > rcu_read_unlock();
> 
> Please triple-check that all your rcu_read_lock() and rcu_read_unlock()
> are balanced (no double-unlock, nor missing unlock for each lock taken).
> 
> The type of problem you get would happen in such a case.
> 
> Thanks,
> 
> Mathieu
> 
> > }
> > }
> > 
> > main()
> > {
> > rcu_init();
> >   pthread_create(, , , , thread);
> > 
> > rcu_register_thread();
> > for (;;)
> > {
> >  if (xxx)
> >    call_rcu();
> > }
> > 
> > }
> > _______________________________________________
> > lttng-dev mailing list
> > lttng-dev@lists.lttng.org
> > http://lists.lttng.org/cgi-bin/mailman/listinfo/lttng-dev
> 
> -- 
> Mathieu Desnoyers
> Operating System Efficiency R&D Consultant
> EfficiOS Inc.
> http://www.efficios.com
-- 
Mathieu Desnoyers
Operating System Efficiency R&D Consultant
EfficiOS Inc.
http://www.efficios.com

Re: 'call_rcu' unstable?

[Mathieu Desnoyers]

* zs (84500316@qq.com) wrote:
> thanks Mathieu Desnoyers for the patience.
> 
> >- Did you ensure that you issue rcu_register_thread() at thread start of
> >  each of your threads ? And rcu_register_thread before returning from
> >  each thread ?
> Sure.
> 
> >- How is the list g_sslvpnctxlist[h] initialized ? 
> Here, is the way:
> I alloc and init 'g_sslvpnctxlist' in one process, and exec rcu_read_lock/unlock/call_rcu in another process(which create many rx/tx threads). 
>         if (sslvpn_shm_alloc(&shm) == -1) {
>                 syslog(LOG_ERR, "alloc share stats memory failed %s\n", strerror(errno));
>                 exit(-1);
>         }
>         g_sslvpnctxlist = (void *)shm.addr;
>         for (i = 0; i < sslvpn_max_users; i++)
>                  CDS_INIT_LIST_HEAD(&g_sslvpnctxlist[i]);
> It looks strange, that someone uses share-memory(created by mmap) to
> hold the 'g_sslvpnctxlist' (+_+);
> Later I will re-coding this part, not use share-memory. 

A couple of things to keep in mind:

- the shm lists need to be initialized before being passed to the other
  process adding to them and deleting from them,
- you need to be aware that a synchronize_rcu() (or call_rcu())
  executing in the context of one process will not see the RCU read-side
  locks of another process. So while it should theoretically be OK
  to do cds_list_add_rcu() from one process and read data from another
  process, both data read and use of call_rcu/synchronize_rcu need to be
  performed within the same process, which might make reclaim more
  complicated. But I understand that you allocate list node, add,
  remove, and iterate all from the same process, and only initialize
  the list head from a different process, so this should be good.
> 
> Are there concurrent
> >  modifications of this list ? If yes, are they synchronized with a
> >  mutex ?
> I do not use mutex, because all the add/del are executing in one thread. 

Fair enough, as long as your list head init has been performed before
your first "add".

> 
> >- When your application is a state where the call_rcu worker thread
> >  busy-waits on RCU read-side critical sections, it would be interesting
> >  to know on what read-side critical section it is waiting. In order to
> >  do so, from gdb attached to your process when it is hung:
> >  - we'
> 
>   I did attach the process and check each thread, it says:
> six threads block in 'msgrcv' IO, and one thread hangs in
> 'update_counter_and_wait:uruc.c:247'

Hrm, interesting! I wonder what messages they are waiting on. This might
be the root cause of the issue: a blocking msgrcv that waits for data
that is not coming, and at least one of them would be holding the RCU
read-side lock while waiting. This would in turn also stop progress from
the call_rcu worker thread.

> 
>   I do not have the chance to check the rcu_reader TLS values,
>  because my customer will not allow the problem happlen again(I have
>  replaced rcu_lock by pthread_mutex).

I understand.

> 
>  I am trying to reproduce the problem in my testing workplace, if
>  happens, I will give more.

OK, that would be helpful! But maybe finding out what messages those
threads are waiting on might be enough. Is it expected by design that
those threads wait for a long time on msgrcv ? Is the rcu read-side lock
held across those msgrcv calls ?

Thanks,

Mathieu

> 
> thanks .
>    
> 
> 
> 
> ------------------ Original ------------------
> From:  "Mathieu Desnoyers"<mathieu.desnoyers@efficios.com>;
> Date:  Wed, Dec 12, 2012 10:41 AM
> To:  "zs"<84500316@qq.com>; 
> Cc:  "lttng-dev"<lttng-dev@lists.lttng.org>; 
> Subject:  Re: [lttng-dev] 'call_rcu' unstable?
> 
> 
> 
> * zs (84500316@qq.com) wrote:
> > Thanks , BUT ...
> > I really check my code:
> > 
> > zs# find .|xargs grep rcu_read 
> > 
> > ./sslvpn_ctx.c: rcu_read_lock();
> > ./sslvpn_ctx.c:                 rcu_read_unlock();
> > ./sslvpn_ctx.c: rcu_read_unlock();
> 
> OK, in that case, continuing on the debugging checklist:
> 
> - Did you ensure that you issue rcu_register_thread() at thread start of
>   each of your threads ? And rcu_register_thread before returning from
>   each thread ?
> - How is the list g_sslvpnctxlist[h] initialized ? Are there concurrent
>   modifications of this list ? If yes, are they synchronized with a
>   mutex ?
> - When your application is a state where the call_rcu worker thread
>   busy-waits on RCU read-side critical sections, it would be interesting
>   to know on what read-side critical section it is waiting. In order to
>   do so, from gdb attached to your process when it is hung:
>   - we'd need to look at the urcu.c "registry" list. We'd need to figure
>     out which list entries are keeping the busy-loop waiting.
>   - then, we should look at each thread's "rcu_reader" TLS variable, to
>     see its address and content.
>   By comparing the content of the list and each active thread's
>   rcu_reader TLS variable, we should be able to figure out what is
>   keeping grace period to complete. If you can provide these dumps, it
>   would let me help you digging further into your issue.
> 
> Thanks,
> 
> Mathieu
> 
> 
> > 
> > AND in sslvpn_ctx.c:
> > void *sslvpn_lookup_ssl(unsigned long iip)
> > {
> >         struct sslvpn_ctx *ctx;
> >         int h;
> > 
> >         h = get_hash(iip, 0);
> > 
> >         rcu_read_lock();
> >         cds_list_for_each_entry_rcu(ctx, &g_sslvpnctxlist[h], cdlist) {
> >                 if ((ctx->flags & SSL_CTX_ESTABLISHED) && ctx->iip && ctx->iip == iip) {
> > 
> >                         uatomic_add(&ctx->ssl_use, 1);
> >                         rcu_read_unlock();
> >                         return ctx;
> >                 }
> >         }
> > 
> >         rcu_read_unlock();
> >         return NULL;
> > }
> > 
> > By the way, *sslvpn_lookup_ssl* called by 6 threads for TX.
> > the 7th thread only will call *call_rcu*:
> > 
> > int sslvpn_del_ctx(struct sslvpn_ctx *pctx)
> > {
> >         ...
> >         cds_list_del_rcu(&ctx->cdlist);
> >         ctx->flags |= SSL_CTX_DYING;
> >         call_rcu(&ctx->rcu, func);
> >         ...
> > }
> > 
> > 
> > 
> > 
> > 
> > ------------------ Original ------------------
> > From:  "Mathieu Desnoyers"<mathieu.desnoyers@efficios.com>;
> > Date:  Wed, Dec 12, 2012 01:55 AM
> > To:  "zs"<84500316@qq.com>; 
> > Cc:  "lttng-dev"<lttng-dev@lists.lttng.org>; 
> > Subject:  Re: [lttng-dev] 'call_rcu' unstable?
> > 
> > 
> > 
> > * zs (84500316@qq.com) wrote:
> > > Hi list,
> > > 
> > > I found a big problem in my product, that use urcu 0.7.5. My program cost too mutch CPU in the funtion 'update_counter_and_wait:uruc.c:247', and I use gdb to see to *wait_loops*, it says -167777734. The CPU usage grows up from 1% to 100% in one day!
> > > 
> > > 
> > > Here is the sample code to show how I use urcu library:
> > > 
> > > #include <urcu.h>
> > > 
> > > thread ()
> > > {
> > > rcu_register_thread();
> > > 
> > > for (;;) {
> > > rcu_read_lock();
> > > xxx
> > > rcu_read_unlock();
> > 
> > Please triple-check that all your rcu_read_lock() and rcu_read_unlock()
> > are balanced (no double-unlock, nor missing unlock for each lock taken).
> > 
> > The type of problem you get would happen in such a case.
> > 
> > Thanks,
> > 
> > Mathieu
> > 
> > > }
> > > }
> > > 
> > > main()
> > > {
> > > rcu_init();
> > >   pthread_create(, , , , thread);
> > > 
> > > rcu_register_thread();
> > > for (;;)
> > > {
> > >  if (xxx)
> > >    call_rcu();
> > > }
> > > 
> > > }
> > > _______________________________________________
> > > lttng-dev mailing list
> > > lttng-dev@lists.lttng.org
> > > http://lists.lttng.org/cgi-bin/mailman/listinfo/lttng-dev
> > 
> > -- 
> > Mathieu Desnoyers
> > Operating System Efficiency R&D Consultant
> > EfficiOS Inc.
> > http://www.efficios.com
> -- 
> Mathieu Desnoyers
> Operating System Efficiency R&D Consultant
> EfficiOS Inc.
> http://www.efficios.com
-- 
Mathieu Desnoyers
Operating System Efficiency R&D Consultant
EfficiOS Inc.
http://www.efficios.com

Re: 'call_rcu' unstable?

[Mathieu Desnoyers]

* Mathieu Desnoyers (mathieu.desnoyers@efficios.com) wrote:
> * zs (84500316@qq.com) wrote:
> > thanks Mathieu Desnoyers for the patience.
> > 
> > >- Did you ensure that you issue rcu_register_thread() at thread start of
> > >  each of your threads ? And rcu_register_thread before returning from
> > >  each thread ?
> > Sure.
> > 
> > >- How is the list g_sslvpnctxlist[h] initialized ? 
> > Here, is the way:
> > I alloc and init 'g_sslvpnctxlist' in one process, and exec rcu_read_lock/unlock/call_rcu in another process(which create many rx/tx threads). 
> >         if (sslvpn_shm_alloc(&shm) == -1) {
> >                 syslog(LOG_ERR, "alloc share stats memory failed %s\n", strerror(errno));
> >                 exit(-1);
> >         }
> >         g_sslvpnctxlist = (void *)shm.addr;
> >         for (i = 0; i < sslvpn_max_users; i++)
> >                  CDS_INIT_LIST_HEAD(&g_sslvpnctxlist[i]);
> > It looks strange, that someone uses share-memory(created by mmap) to
> > hold the 'g_sslvpnctxlist' (+_+);
> > Later I will re-coding this part, not use share-memory. 
> 
> A couple of things to keep in mind:
> 
> - the shm lists need to be initialized before being passed to the other
>   process adding to them and deleting from them,
> - you need to be aware that a synchronize_rcu() (or call_rcu())
>   executing in the context of one process will not see the RCU read-side
>   locks of another process. So while it should theoretically be OK
>   to do cds_list_add_rcu() from one process and read data from another
>   process, both data read and use of call_rcu/synchronize_rcu need to be
>   performed within the same process, which might make reclaim more
>   complicated. But I understand that you allocate list node, add,
>   remove, and iterate all from the same process, and only initialize
>   the list head from a different process, so this should be good.
> > 
> > Are there concurrent
> > >  modifications of this list ? If yes, are they synchronized with a
> > >  mutex ?
> > I do not use mutex, because all the add/del are executing in one thread. 
> 
> Fair enough, as long as your list head init has been performed before
> your first "add".
> 
> > 
> > >- When your application is a state where the call_rcu worker thread
> > >  busy-waits on RCU read-side critical sections, it would be interesting
> > >  to know on what read-side critical section it is waiting. In order to
> > >  do so, from gdb attached to your process when it is hung:
> > >  - we'
> > 
> >   I did attach the process and check each thread, it says:
> > six threads block in 'msgrcv' IO, and one thread hangs in
> > 'update_counter_and_wait:uruc.c:247'
> 
> Hrm, interesting! I wonder what messages they are waiting on. This might
> be the root cause of the issue: a blocking msgrcv that waits for data
> that is not coming, and at least one of them would be holding the RCU
> read-side lock while waiting. This would in turn also stop progress from
> the call_rcu worker thread.
> 
> > 
> >   I do not have the chance to check the rcu_reader TLS values,
> >  because my customer will not allow the problem happlen again(I have
> >  replaced rcu_lock by pthread_mutex).
> 
> I understand.
> 
> > 
> >  I am trying to reproduce the problem in my testing workplace, if
> >  happens, I will give more.
> 
> OK, that would be helpful! But maybe finding out what messages those
> threads are waiting on might be enough. Is it expected by design that
> those threads wait for a long time on msgrcv ? Is the rcu read-side lock
> held across those msgrcv calls ?

One more question about the nature of your application: is it using a
"fork()" (or clone()) not followed by exec() ?

This design pattern is quite common in daemons using multi-process
worker threads, and it's very likely in your case since you are using
shared memory between processes. This has important impacts on the way
threads are handled (see the pthread_atfork(3) manpage). Given that URCU
keeps track of all reader threads, if any of those vanish at fork(), due
to Linux implementation limitations, they need to be cleared from the
urcu registry before a fork() not followed by exec().

For more info, please refer to the Userspace RCU README file, under
section "Interaction with fork()".

Especially the part:

"Most liburcu implementations require that all registrations (as reader,
defer_rcu and call_rcu threads) should be released before a fork() is
performed, except for the rather common scenario where fork() is
immediately followed by exec() in the child process."

So one possible explanation for the scenario you are observing is that
the parent process still has RCU readers registered while performing
fork(), and that the child process is unable to complete grace periods
due to this stale list entry.

One thing to keep in mind is that this commit:

commit 765f3eadad5647e6fa853414fc652670f9e00966
Author: Mathieu Desnoyers <mathieu.desnoyers@efficios.com>
Date:   Sat Sep 10 22:02:58 2011 -0700

    call_rcu: register work threads as rcu readers

makes all call_rcu worker threads register as RCU readers. Since the
default call_rcu thread is never cleaned up, it becomes hard for the
application to ensure fork() correctness.

Am I on the right track ?

Thanks,

Mathieu

> 
> Thanks,
> 
> Mathieu
> 
> > 
> > thanks .
> >    
> > 
> > 
> > 
> > ------------------ Original ------------------
> > From:  "Mathieu Desnoyers"<mathieu.desnoyers@efficios.com>;
> > Date:  Wed, Dec 12, 2012 10:41 AM
> > To:  "zs"<84500316@qq.com>; 
> > Cc:  "lttng-dev"<lttng-dev@lists.lttng.org>; 
> > Subject:  Re: [lttng-dev] 'call_rcu' unstable?
> > 
> > 
> > 
> > * zs (84500316@qq.com) wrote:
> > > Thanks , BUT ...
> > > I really check my code:
> > > 
> > > zs# find .|xargs grep rcu_read 
> > > 
> > > ./sslvpn_ctx.c: rcu_read_lock();
> > > ./sslvpn_ctx.c:                 rcu_read_unlock();
> > > ./sslvpn_ctx.c: rcu_read_unlock();
> > 
> > OK, in that case, continuing on the debugging checklist:
> > 
> > - Did you ensure that you issue rcu_register_thread() at thread start of
> >   each of your threads ? And rcu_register_thread before returning from
> >   each thread ?
> > - How is the list g_sslvpnctxlist[h] initialized ? Are there concurrent
> >   modifications of this list ? If yes, are they synchronized with a
> >   mutex ?
> > - When your application is a state where the call_rcu worker thread
> >   busy-waits on RCU read-side critical sections, it would be interesting
> >   to know on what read-side critical section it is waiting. In order to
> >   do so, from gdb attached to your process when it is hung:
> >   - we'd need to look at the urcu.c "registry" list. We'd need to figure
> >     out which list entries are keeping the busy-loop waiting.
> >   - then, we should look at each thread's "rcu_reader" TLS variable, to
> >     see its address and content.
> >   By comparing the content of the list and each active thread's
> >   rcu_reader TLS variable, we should be able to figure out what is
> >   keeping grace period to complete. If you can provide these dumps, it
> >   would let me help you digging further into your issue.
> > 
> > Thanks,
> > 
> > Mathieu
> > 
> > 
> > > 
> > > AND in sslvpn_ctx.c:
> > > void *sslvpn_lookup_ssl(unsigned long iip)
> > > {
> > >         struct sslvpn_ctx *ctx;
> > >         int h;
> > > 
> > >         h = get_hash(iip, 0);
> > > 
> > >         rcu_read_lock();
> > >         cds_list_for_each_entry_rcu(ctx, &g_sslvpnctxlist[h], cdlist) {
> > >                 if ((ctx->flags & SSL_CTX_ESTABLISHED) && ctx->iip && ctx->iip == iip) {
> > > 
> > >                         uatomic_add(&ctx->ssl_use, 1);
> > >                         rcu_read_unlock();
> > >                         return ctx;
> > >                 }
> > >         }
> > > 
> > >         rcu_read_unlock();
> > >         return NULL;
> > > }
> > > 
> > > By the way, *sslvpn_lookup_ssl* called by 6 threads for TX.
> > > the 7th thread only will call *call_rcu*:
> > > 
> > > int sslvpn_del_ctx(struct sslvpn_ctx *pctx)
> > > {
> > >         ...
> > >         cds_list_del_rcu(&ctx->cdlist);
> > >         ctx->flags |= SSL_CTX_DYING;
> > >         call_rcu(&ctx->rcu, func);
> > >         ...
> > > }
> > > 
> > > 
> > > 
> > > 
> > > 
> > > ------------------ Original ------------------
> > > From:  "Mathieu Desnoyers"<mathieu.desnoyers@efficios.com>;
> > > Date:  Wed, Dec 12, 2012 01:55 AM
> > > To:  "zs"<84500316@qq.com>; 
> > > Cc:  "lttng-dev"<lttng-dev@lists.lttng.org>; 
> > > Subject:  Re: [lttng-dev] 'call_rcu' unstable?
> > > 
> > > 
> > > 
> > > * zs (84500316@qq.com) wrote:
> > > > Hi list,
> > > > 
> > > > I found a big problem in my product, that use urcu 0.7.5. My program cost too mutch CPU in the funtion 'update_counter_and_wait:uruc.c:247', and I use gdb to see to *wait_loops*, it says -167777734. The CPU usage grows up from 1% to 100% in one day!
> > > > 
> > > > 
> > > > Here is the sample code to show how I use urcu library:
> > > > 
> > > > #include <urcu.h>
> > > > 
> > > > thread ()
> > > > {
> > > > rcu_register_thread();
> > > > 
> > > > for (;;) {
> > > > rcu_read_lock();
> > > > xxx
> > > > rcu_read_unlock();
> > > 
> > > Please triple-check that all your rcu_read_lock() and rcu_read_unlock()
> > > are balanced (no double-unlock, nor missing unlock for each lock taken).
> > > 
> > > The type of problem you get would happen in such a case.
> > > 
> > > Thanks,
> > > 
> > > Mathieu
> > > 
> > > > }
> > > > }
> > > > 
> > > > main()
> > > > {
> > > > rcu_init();
> > > >   pthread_create(, , , , thread);
> > > > 
> > > > rcu_register_thread();
> > > > for (;;)
> > > > {
> > > >  if (xxx)
> > > >    call_rcu();
> > > > }
> > > > 
> > > > }
> > > > _______________________________________________
> > > > lttng-dev mailing list
> > > > lttng-dev@lists.lttng.org
> > > > http://lists.lttng.org/cgi-bin/mailman/listinfo/lttng-dev
> > > 
> > > -- 
> > > Mathieu Desnoyers
> > > Operating System Efficiency R&D Consultant
> > > EfficiOS Inc.
> > > http://www.efficios.com
> > -- 
> > Mathieu Desnoyers
> > Operating System Efficiency R&D Consultant
> > EfficiOS Inc.
> > http://www.efficios.com
> -- 
> Mathieu Desnoyers
> Operating System Efficiency R&D Consultant
> EfficiOS Inc.
> http://www.efficios.com
> 
> _______________________________________________
> lttng-dev mailing list
> lttng-dev@lists.lttng.org
> http://lists.lttng.org/cgi-bin/mailman/listinfo/lttng-dev

-- 
Mathieu Desnoyers
Operating System Efficiency R&D Consultant
EfficiOS Inc.
http://www.efficios.com

Re: 'call_rcu' unstable?

[Mathieu Desnoyers]

* Mathieu Desnoyers (mathieu.desnoyers@efficios.com) wrote:
[...]
> 
> One more question about the nature of your application: is it using a
> "fork()" (or clone()) not followed by exec() ?
> 
> This design pattern is quite common in daemons using multi-process
> worker threads, and it's very likely in your case since you are using
> shared memory between processes. This has important impacts on the way
> threads are handled (see the pthread_atfork(3) manpage). Given that URCU
> keeps track of all reader threads, if any of those vanish at fork(), due
> to Linux implementation limitations, they need to be cleared from the
> urcu registry before a fork() not followed by exec().
> 
> For more info, please refer to the Userspace RCU README file, under
> section "Interaction with fork()".
> 
> Especially the part:
> 
> "Most liburcu implementations require that all registrations (as reader,
> defer_rcu and call_rcu threads) should be released before a fork() is
> performed, except for the rather common scenario where fork() is
> immediately followed by exec() in the child process."
> 
> So one possible explanation for the scenario you are observing is that
> the parent process still has RCU readers registered while performing
> fork(), and that the child process is unable to complete grace periods
> due to this stale list entry.
> 
> One thing to keep in mind is that this commit:
> 
> commit 765f3eadad5647e6fa853414fc652670f9e00966
> Author: Mathieu Desnoyers <mathieu.desnoyers@efficios.com>
> Date:   Sat Sep 10 22:02:58 2011 -0700
> 
>     call_rcu: register work threads as rcu readers
> 
> makes all call_rcu worker threads register as RCU readers. Since the
> default call_rcu thread is never cleaned up, it becomes hard for the
> application to ensure fork() correctness.
> 
> Am I on the right track ?

I created a small patch that reproduces your faulty behavior by simply
using fork() in a program (without following exec()). The child process
gets stucked in wait_for_readers() eating 100% of one CPU. More to come
soon.

Comments are welcome,

Thanks,

Mathieu

---
diff --git a/tests/Makefile.am b/tests/Makefile.am
index f8b4c67..0e15a4c 100644
--- a/tests/Makefile.am
+++ b/tests/Makefile.am
@@ -20,7 +20,8 @@ noinst_PROGRAMS = test_urcu test_urcu_dynamic_link test_urcu_timing \
 	test_urcu_wfcq_dynlink \
 	test_urcu_lfq_dynlink test_urcu_lfs_dynlink test_urcu_hash \
 	test_urcu_lfs_rcu_dynlink \
-	test_urcu_multiflavor test_urcu_multiflavor_dynlink
+	test_urcu_multiflavor test_urcu_multiflavor_dynlink \
+	test_urcu_fork
 noinst_HEADERS = rcutorture.h test_urcu_multiflavor.h
 
 if COMPAT_ARCH
@@ -85,6 +86,7 @@ test_urcu_signal_timing_CFLAGS= -DRCU_SIGNAL $(AM_CFLAGS)
 test_urcu_signal_yield_SOURCES = test_urcu.c $(URCU_SIGNAL)
 test_urcu_signal_yield_CFLAGS = -DRCU_SIGNAL -DDEBUG_YIELD $(AM_CFLAGS)
 
+test_urcu_fork_SOURCES = test_urcu_fork.c $(URCU)
 
 test_rwlock_timing_SOURCES = test_rwlock_timing.c $(URCU_SIGNAL)
 
diff --git a/tests/test_urcu_fork.c b/tests/test_urcu_fork.c
new file mode 100644
index 0000000..07c521a
--- /dev/null
+++ b/tests/test_urcu_fork.c
@@ -0,0 +1,141 @@
+/*
+ * test_urcu_fork.c
+ *
+ * Userspace RCU library - test program (fork)
+ *
+ * Copyright February 2012 - Mathieu Desnoyers <mathieu.desnoyers@efficios.com>
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation; either version 2 of the License, or
+ * (at your option) any later version.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License along
+ * with this program; if not, write to the Free Software Foundation, Inc.,
+ * 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA.
+ */
+
+#define _GNU_SOURCE
+#include "../config.h"
+#include <stdio.h>
+#include <pthread.h>
+#include <stdlib.h>
+#include <string.h>
+#include <sys/types.h>
+#include <sys/wait.h>
+#include <unistd.h>
+#include <stdio.h>
+#include <assert.h>
+#include <sched.h>
+#include <errno.h>
+
+#include <urcu/arch.h>
+#include <urcu/tls-compat.h>
+
+#ifndef DYNAMIC_LINK_TEST
+#define _LGPL_SOURCE
+#else
+#define rcu_debug_yield_read()
+#endif
+#include <urcu.h>
+
+struct test_node {
+	int somedata;
+	struct rcu_head head;
+};
+
+static void cb(struct rcu_head *head)
+{
+	struct test_node *node;
+
+	fprintf(stderr, "rcu callback invoked in pid: %d\n",
+		(int) getpid());
+	node = caa_container_of(head, struct test_node, head);
+	free(node);
+}
+
+static void test_rcu(void)
+{
+	struct test_node *node;
+
+	rcu_register_thread();
+
+	synchronize_rcu();
+
+	rcu_read_lock();
+	rcu_read_unlock();
+
+	node = malloc(sizeof(*node));
+	assert(node);
+
+	call_rcu(&node->head, cb);
+
+	synchronize_rcu();
+
+	rcu_unregister_thread();
+}
+
+int main(int argc, char **argv)
+{
+	pid_t pid;
+	int ret;
+
+	ret = pthread_atfork(call_rcu_before_fork,
+		call_rcu_after_fork_parent,
+		call_rcu_after_fork_child);
+	if (ret) {
+		errno = ret;
+		perror("pthread_atfork");
+		exit(EXIT_FAILURE);
+	}
+
+	test_rcu();
+
+	synchronize_rcu();
+
+	fprintf(stderr, "%s parent pid: %d, before fork\n",
+		argv[0], (int) getpid());
+
+	pid = fork();
+
+	if (pid == 0) {
+		/* child */
+		fprintf(stderr, "%s child pid: %d, after fork\n",
+			argv[0], (int) getpid());
+		test_rcu();
+		fprintf(stderr, "%s child pid: %d, after rcu test\n",
+			argv[0], (int) getpid());
+	} else if (pid > 0) {
+		int status;
+
+		/* parent */
+		fprintf(stderr, "%s parent pid: %d, after fork\n",
+			argv[0], (int) getpid());
+		test_rcu();
+		fprintf(stderr, "%s parent pid: %d, after rcu test\n",
+			argv[0], (int) getpid());
+		for (;;) {
+			pid = wait(&status);
+			if (WIFEXITED(status)) {
+				fprintf(stderr, "child %u exited normally with status %u\n",
+					pid, WEXITSTATUS(status));
+				break;
+			} else if (WIFSIGNALED(status)) {
+				fprintf(stderr, "child %u was terminated by signal %u\n",
+					pid, WTERMSIG(status));
+				break;
+			} else {
+				continue;
+			}
+		}
+	} else {
+		perror("fork");
+		exit(EXIT_FAILURE);
+	}
+	exit(EXIT_SUCCESS);
+}


-- 
Mathieu Desnoyers
Operating System Efficiency R&D Consultant
EfficiOS Inc.
http://www.efficios.com

[RFC PATCH] Fix call_rcu fork handling

[Mathieu Desnoyers]

Fix call_rcu fork handling by putting all call_rcu threads in a
quiescent state before fork (paused state), and unpausing them when the
parent returns from fork.

On the child, everything will run fine as long as we don't issue fork()
from a call_rcu callback.

Side-note: pthread_atfork is not appropriate when playing with
multithread and malloc/free. The glibc malloc implementation sadly
expects that all malloc/free are executed from the context of a single
thread while pthread atfork handlers are running, which leads to
interesting hang in glibc.

Signed-off-by: Mathieu Desnoyers <mathieu.desnoyers@efficios.com>
---
diff --git a/tests/test_urcu_fork.c b/tests/test_urcu_fork.c
index 07c521a..6e454b5 100644
--- a/tests/test_urcu_fork.c
+++ b/tests/test_urcu_fork.c
@@ -85,6 +85,8 @@ int main(int argc, char **argv)
 	pid_t pid;
 	int ret;
 
+#if 0
+	/* pthread_atfork does not work with malloc/free in callbacks */
 	ret = pthread_atfork(call_rcu_before_fork,
 		call_rcu_after_fork_parent,
 		call_rcu_after_fork_child);
@@ -93,6 +95,7 @@ int main(int argc, char **argv)
 		perror("pthread_atfork");
 		exit(EXIT_FAILURE);
 	}
+#endif
 
 	test_rcu();
 
@@ -101,10 +104,12 @@ int main(int argc, char **argv)
 	fprintf(stderr, "%s parent pid: %d, before fork\n",
 		argv[0], (int) getpid());
 
+	call_rcu_before_fork();
 	pid = fork();
 
 	if (pid == 0) {
 		/* child */
+		call_rcu_after_fork_child();
 		fprintf(stderr, "%s child pid: %d, after fork\n",
 			argv[0], (int) getpid());
 		test_rcu();
@@ -114,6 +119,7 @@ int main(int argc, char **argv)
 		int status;
 
 		/* parent */
+		call_rcu_after_fork_parent();
 		fprintf(stderr, "%s parent pid: %d, after fork\n",
 			argv[0], (int) getpid());
 		test_rcu();
diff --git a/urcu-call-rcu-impl.h b/urcu-call-rcu-impl.h
index 6580397..f6625ba 100644
--- a/urcu-call-rcu-impl.h
+++ b/urcu-call-rcu-impl.h
@@ -75,8 +75,9 @@ static CDS_LIST_HEAD(call_rcu_data_list);
 
 static DEFINE_URCU_TLS(struct call_rcu_data *, thread_call_rcu_data);
 
-/* Guard call_rcu thread creation. */
-
+/*
+ * Guard call_rcu thread creation and atfork handlers.
+ */
 static pthread_mutex_t call_rcu_mutex = PTHREAD_MUTEX_INITIALIZER;
 
 /* If a given thread does not have its own call_rcu thread, this is default. */
@@ -254,6 +255,21 @@ static void *call_rcu_thread(void *arg)
 		struct cds_wfcq_node *cbs, *cbs_tmp_n;
 		enum cds_wfcq_ret splice_ret;
 
+		if (uatomic_read(&crdp->flags) & URCU_CALL_RCU_PAUSE) {
+			/*
+			 * Pause requested. Become quiescent: remove
+			 * ourself from all global lists, and don't
+			 * process any callback. The callback lists may
+			 * still be non-empty though.
+			 */
+			rcu_unregister_thread();
+			cmm_smp_mb__before_uatomic_or();
+			uatomic_or(&crdp->flags, URCU_CALL_RCU_PAUSED);
+			while ((uatomic_read(&crdp->flags) & URCU_CALL_RCU_PAUSE) != 0)
+				poll(NULL, 0, 1);
+			rcu_register_thread();
+		}
+
 		cds_wfcq_init(&cbs_tmp_head, &cbs_tmp_tail);
 		splice_ret = __cds_wfcq_splice_blocking(&cbs_tmp_head,
 			&cbs_tmp_tail, &crdp->cbs_head, &crdp->cbs_tail);
@@ -705,12 +721,25 @@ void free_all_cpu_call_rcu_data(void)
 
 /*
  * Acquire the call_rcu_mutex in order to ensure that the child sees
- * all of the call_rcu() data structures in a consistent state.
+ * all of the call_rcu() data structures in a consistent state. Ensure
+ * that all call_rcu threads are in a quiescent state across fork.
  * Suitable for pthread_atfork() and friends.
  */
 void call_rcu_before_fork(void)
 {
+	struct call_rcu_data *crdp;
+
 	call_rcu_lock(&call_rcu_mutex);
+
+	cds_list_for_each_entry(crdp, &call_rcu_data_list, list) {
+		uatomic_or(&crdp->flags, URCU_CALL_RCU_PAUSE);
+		cmm_smp_mb__after_uatomic_or();
+		wake_call_rcu_thread(crdp);
+	}
+	cds_list_for_each_entry(crdp, &call_rcu_data_list, list) {
+		while ((uatomic_read(&crdp->flags) & URCU_CALL_RCU_PAUSED) == 0)
+			poll(NULL, 0, 1);
+	}
 }
 
 /*
@@ -720,6 +749,10 @@ void call_rcu_before_fork(void)
  */
 void call_rcu_after_fork_parent(void)
 {
+	struct call_rcu_data *crdp;
+
+	cds_list_for_each_entry(crdp, &call_rcu_data_list, list)
+		uatomic_and(&crdp->flags, ~URCU_CALL_RCU_PAUSE);
 	call_rcu_unlock(&call_rcu_mutex);
 }
 
@@ -752,7 +785,11 @@ void call_rcu_after_fork_child(void)
 	rcu_set_pointer(&per_cpu_call_rcu_data, NULL);
 	URCU_TLS(thread_call_rcu_data) = NULL;
 
-	/* Dispose of all of the rest of the call_rcu_data structures. */
+	/*
+	 * Dispose of all of the rest of the call_rcu_data structures.
+	 * Leftover call_rcu callbacks will be merged into the new
+	 * default call_rcu thread queue.
+	 */
 	cds_list_for_each_entry_safe(crdp, next, &call_rcu_data_list, list) {
 		if (crdp == default_call_rcu_data)
 			continue;
diff --git a/urcu-call-rcu.h b/urcu-call-rcu.h
index 1dad0e2..997bb2f 100644
--- a/urcu-call-rcu.h
+++ b/urcu-call-rcu.h
@@ -44,10 +44,12 @@ struct call_rcu_data;
 
 /* Flag values. */
 
-#define URCU_CALL_RCU_RT	0x1
-#define URCU_CALL_RCU_RUNNING	0x2
-#define URCU_CALL_RCU_STOP	0x4
-#define URCU_CALL_RCU_STOPPED	0x8
+#define URCU_CALL_RCU_RT	(1U << 0)
+#define URCU_CALL_RCU_RUNNING	(1U << 1)
+#define URCU_CALL_RCU_STOP	(1U << 2)
+#define URCU_CALL_RCU_STOPPED	(1U << 3)
+#define URCU_CALL_RCU_PAUSE	(1U << 4)
+#define URCU_CALL_RCU_PAUSED	(1U << 5)
 
 /*
  * The rcu_head data structure is placed in the structure to be freed
-- 
Mathieu Desnoyers
Operating System Efficiency R&D Consultant
EfficiOS Inc.
http://www.efficios.com

Re: [RFC PATCH] Fix call_rcu fork handling

[Mathieu Desnoyers]

* Mathieu Desnoyers (mathieu.desnoyers@efficios.com) wrote:
> Fix call_rcu fork handling by putting all call_rcu threads in a
> quiescent state before fork (paused state), and unpausing them when the
> parent returns from fork.
> 
> On the child, everything will run fine as long as we don't issue fork()
> from a call_rcu callback.
> 
> Side-note: pthread_atfork is not appropriate when playing with
> multithread and malloc/free. The glibc malloc implementation sadly
> expects that all malloc/free are executed from the context of a single
> thread while pthread atfork handlers are running, which leads to
> interesting hang in glibc.

I'll rework the patch so the modification to tests/test_urcu_fork.c is
moved to a separate patch, and merge the fix into the master branch,
since it solves a reproduceable problem.

Thanks,

Mathieu

> 
> Signed-off-by: Mathieu Desnoyers <mathieu.desnoyers@efficios.com>
> ---
> diff --git a/tests/test_urcu_fork.c b/tests/test_urcu_fork.c
> index 07c521a..6e454b5 100644
> --- a/tests/test_urcu_fork.c
> +++ b/tests/test_urcu_fork.c
> @@ -85,6 +85,8 @@ int main(int argc, char **argv)
>  	pid_t pid;
>  	int ret;
>  
> +#if 0
> +	/* pthread_atfork does not work with malloc/free in callbacks */
>  	ret = pthread_atfork(call_rcu_before_fork,
>  		call_rcu_after_fork_parent,
>  		call_rcu_after_fork_child);
> @@ -93,6 +95,7 @@ int main(int argc, char **argv)
>  		perror("pthread_atfork");
>  		exit(EXIT_FAILURE);
>  	}
> +#endif
>  
>  	test_rcu();
>  
> @@ -101,10 +104,12 @@ int main(int argc, char **argv)
>  	fprintf(stderr, "%s parent pid: %d, before fork\n",
>  		argv[0], (int) getpid());
>  
> +	call_rcu_before_fork();
>  	pid = fork();
>  
>  	if (pid == 0) {
>  		/* child */
> +		call_rcu_after_fork_child();
>  		fprintf(stderr, "%s child pid: %d, after fork\n",
>  			argv[0], (int) getpid());
>  		test_rcu();
> @@ -114,6 +119,7 @@ int main(int argc, char **argv)
>  		int status;
>  
>  		/* parent */
> +		call_rcu_after_fork_parent();
>  		fprintf(stderr, "%s parent pid: %d, after fork\n",
>  			argv[0], (int) getpid());
>  		test_rcu();
> diff --git a/urcu-call-rcu-impl.h b/urcu-call-rcu-impl.h
> index 6580397..f6625ba 100644
> --- a/urcu-call-rcu-impl.h
> +++ b/urcu-call-rcu-impl.h
> @@ -75,8 +75,9 @@ static CDS_LIST_HEAD(call_rcu_data_list);
>  
>  static DEFINE_URCU_TLS(struct call_rcu_data *, thread_call_rcu_data);
>  
> -/* Guard call_rcu thread creation. */
> -
> +/*
> + * Guard call_rcu thread creation and atfork handlers.
> + */
>  static pthread_mutex_t call_rcu_mutex = PTHREAD_MUTEX_INITIALIZER;
>  
>  /* If a given thread does not have its own call_rcu thread, this is default. */
> @@ -254,6 +255,21 @@ static void *call_rcu_thread(void *arg)
>  		struct cds_wfcq_node *cbs, *cbs_tmp_n;
>  		enum cds_wfcq_ret splice_ret;
>  
> +		if (uatomic_read(&crdp->flags) & URCU_CALL_RCU_PAUSE) {
> +			/*
> +			 * Pause requested. Become quiescent: remove
> +			 * ourself from all global lists, and don't
> +			 * process any callback. The callback lists may
> +			 * still be non-empty though.
> +			 */
> +			rcu_unregister_thread();
> +			cmm_smp_mb__before_uatomic_or();
> +			uatomic_or(&crdp->flags, URCU_CALL_RCU_PAUSED);
> +			while ((uatomic_read(&crdp->flags) & URCU_CALL_RCU_PAUSE) != 0)
> +				poll(NULL, 0, 1);
> +			rcu_register_thread();
> +		}
> +
>  		cds_wfcq_init(&cbs_tmp_head, &cbs_tmp_tail);
>  		splice_ret = __cds_wfcq_splice_blocking(&cbs_tmp_head,
>  			&cbs_tmp_tail, &crdp->cbs_head, &crdp->cbs_tail);
> @@ -705,12 +721,25 @@ void free_all_cpu_call_rcu_data(void)
>  
>  /*
>   * Acquire the call_rcu_mutex in order to ensure that the child sees
> - * all of the call_rcu() data structures in a consistent state.
> + * all of the call_rcu() data structures in a consistent state. Ensure
> + * that all call_rcu threads are in a quiescent state across fork.
>   * Suitable for pthread_atfork() and friends.
>   */
>  void call_rcu_before_fork(void)
>  {
> +	struct call_rcu_data *crdp;
> +
>  	call_rcu_lock(&call_rcu_mutex);
> +
> +	cds_list_for_each_entry(crdp, &call_rcu_data_list, list) {
> +		uatomic_or(&crdp->flags, URCU_CALL_RCU_PAUSE);
> +		cmm_smp_mb__after_uatomic_or();
> +		wake_call_rcu_thread(crdp);
> +	}
> +	cds_list_for_each_entry(crdp, &call_rcu_data_list, list) {
> +		while ((uatomic_read(&crdp->flags) & URCU_CALL_RCU_PAUSED) == 0)
> +			poll(NULL, 0, 1);
> +	}
>  }
>  
>  /*
> @@ -720,6 +749,10 @@ void call_rcu_before_fork(void)
>   */
>  void call_rcu_after_fork_parent(void)
>  {
> +	struct call_rcu_data *crdp;
> +
> +	cds_list_for_each_entry(crdp, &call_rcu_data_list, list)
> +		uatomic_and(&crdp->flags, ~URCU_CALL_RCU_PAUSE);
>  	call_rcu_unlock(&call_rcu_mutex);
>  }
>  
> @@ -752,7 +785,11 @@ void call_rcu_after_fork_child(void)
>  	rcu_set_pointer(&per_cpu_call_rcu_data, NULL);
>  	URCU_TLS(thread_call_rcu_data) = NULL;
>  
> -	/* Dispose of all of the rest of the call_rcu_data structures. */
> +	/*
> +	 * Dispose of all of the rest of the call_rcu_data structures.
> +	 * Leftover call_rcu callbacks will be merged into the new
> +	 * default call_rcu thread queue.
> +	 */
>  	cds_list_for_each_entry_safe(crdp, next, &call_rcu_data_list, list) {
>  		if (crdp == default_call_rcu_data)
>  			continue;
> diff --git a/urcu-call-rcu.h b/urcu-call-rcu.h
> index 1dad0e2..997bb2f 100644
> --- a/urcu-call-rcu.h
> +++ b/urcu-call-rcu.h
> @@ -44,10 +44,12 @@ struct call_rcu_data;
>  
>  /* Flag values. */
>  
> -#define URCU_CALL_RCU_RT	0x1
> -#define URCU_CALL_RCU_RUNNING	0x2
> -#define URCU_CALL_RCU_STOP	0x4
> -#define URCU_CALL_RCU_STOPPED	0x8
> +#define URCU_CALL_RCU_RT	(1U << 0)
> +#define URCU_CALL_RCU_RUNNING	(1U << 1)
> +#define URCU_CALL_RCU_STOP	(1U << 2)
> +#define URCU_CALL_RCU_STOPPED	(1U << 3)
> +#define URCU_CALL_RCU_PAUSE	(1U << 4)
> +#define URCU_CALL_RCU_PAUSED	(1U << 5)
>  
>  /*
>   * The rcu_head data structure is placed in the structure to be freed
> -- 
> Mathieu Desnoyers
> Operating System Efficiency R&D Consultant
> EfficiOS Inc.
> http://www.efficios.com
> 
> _______________________________________________
> lttng-dev mailing list
> lttng-dev@lists.lttng.org
> http://lists.lttng.org/cgi-bin/mailman/listinfo/lttng-dev

-- 
Mathieu Desnoyers
Operating System Efficiency R&D Consultant
EfficiOS Inc.
http://www.efficios.com