Date:   Fri, 26 Jan 2018 14:22:59 -0800
From:   "Paul E. McKenney" <paulmck@...ux.vnet.ibm.com>
To:     Lihao Liang <lihao.liang@...il.com>
Cc:     "Guohanjun (Hanjun Guo)" <guohanjun@...wei.com>, heng.z@...wei.com,
        hb.chen@...wei.com, linux-kernel@...r.kernel.org
Subject: Re: [PATCH RFC 07/16] prcu: Implement call_prcu() API

On Fri, Jan 26, 2018 at 08:44:50AM +0000, Lihao Liang wrote:
> On Thu, Jan 25, 2018 at 6:20 AM, Paul E. McKenney
> <paulmck@...ux.vnet.ibm.com> wrote:
> > On Tue, Jan 23, 2018 at 03:59:32PM +0800, lianglihao@...wei.com wrote:
> >> From: Lihao Liang <lianglihao@...wei.com>
> >>
> >> This is PRCU's counterpart of RCU's call_rcu() API.
> >>
> >> Reviewed-by: Heng Zhang <heng.z@...wei.com>
> >> Signed-off-by: Lihao Liang <lianglihao@...wei.com>
> >> ---
> >>  include/linux/prcu.h | 25 ++++++++++++++++++++
> >>  init/main.c          |  2 ++
> >>  kernel/rcu/prcu.c    | 67 +++++++++++++++++++++++++++++++++++++++++++++++++---
> >>  3 files changed, 91 insertions(+), 3 deletions(-)
> >>
> >> diff --git a/include/linux/prcu.h b/include/linux/prcu.h
> >> index 653b4633..e5e09c9b 100644
> >> --- a/include/linux/prcu.h
> >> +++ b/include/linux/prcu.h
> >> @@ -2,15 +2,36 @@
> >>  #define __LINUX_PRCU_H
> >>
> >>  #include <linux/atomic.h>
> >> +#include <linux/types.h>
> >>  #include <linux/mutex.h>
> >>  #include <linux/wait.h>
> >>
> >>  #define CONFIG_PRCU
> >>
> >> +struct prcu_version_head {
> >> +     unsigned long long version;
> >> +     struct prcu_version_head *next;
> >> +};
> >> +
> >> +/* Simple unsegmented callback list for PRCU. */
> >> +struct prcu_cblist {
> >> +     struct rcu_head *head;
> >> +     struct rcu_head **tail;
> >> +     struct prcu_version_head *version_head;
> >> +     struct prcu_version_head **version_tail;
> >> +     long len;
> >> +};
> >> +
> >> +#define PRCU_CBLIST_INITIALIZER(n) { \
> >> +     .head = NULL, .tail = &n.head, \
> >> +     .version_head = NULL, .version_tail = &n.version_head, \
> >> +}
> >> +
> >>  struct prcu_local_struct {
> >>       unsigned int locked;
> >>       unsigned int online;
> >>       unsigned long long version;
> >> +     struct prcu_cblist cblist;
> >>  };
> >>
> >>  struct prcu_struct {
> >> @@ -24,6 +45,8 @@ struct prcu_struct {
> >>  void prcu_read_lock(void);
> >>  void prcu_read_unlock(void);
> >>  void synchronize_prcu(void);
> >> +void call_prcu(struct rcu_head *head, rcu_callback_t func);
> >> +void prcu_init(void);
> >>  void prcu_note_context_switch(void);
> >>
> >>  #else /* #ifdef CONFIG_PRCU */
> >> @@ -31,6 +54,8 @@ void prcu_note_context_switch(void);
> >>  #define prcu_read_lock() do {} while (0)
> >>  #define prcu_read_unlock() do {} while (0)
> >>  #define synchronize_prcu() do {} while (0)
> >> +#define call_prcu() do {} while (0)
> >> +#define prcu_init() do {} while (0)
> >>  #define prcu_note_context_switch() do {} while (0)
> >>
> >>  #endif /* #ifdef CONFIG_PRCU */
> >> diff --git a/init/main.c b/init/main.c
> >> index f8665104..4925964e 100644
> >> --- a/init/main.c
> >> +++ b/init/main.c
> >> @@ -38,6 +38,7 @@
> >>  #include <linux/smp.h>
> >>  #include <linux/profile.h>
> >>  #include <linux/rcupdate.h>
> >> +#include <linux/prcu.h>
> >>  #include <linux/moduleparam.h>
> >>  #include <linux/kallsyms.h>
> >>  #include <linux/writeback.h>
> >> @@ -574,6 +575,7 @@ asmlinkage __visible void __init start_kernel(void)
> >>       workqueue_init_early();
> >>
> >>       rcu_init();
> >> +     prcu_init();
> >>
> >>       /* Trace events are available after this */
> >>       trace_init();
> >> diff --git a/kernel/rcu/prcu.c b/kernel/rcu/prcu.c
> >> index a00b9420..f198285c 100644
> >> --- a/kernel/rcu/prcu.c
> >> +++ b/kernel/rcu/prcu.c
> >> @@ -1,11 +1,12 @@
> >>  #include <linux/smp.h>
> >> -#include <linux/prcu.h>
> >>  #include <linux/percpu.h>
> >> -#include <linux/compiler.h>
> >> +#include <linux/prcu.h>
> >>  #include <linux/sched.h>
> >> -
> >> +#include <linux/slab.h>
> >>  #include <asm/barrier.h>
> >>
> >> +#include "rcu.h"
> >> +
> >>  DEFINE_PER_CPU_SHARED_ALIGNED(struct prcu_local_struct, prcu_local);
> >>
> >>  struct prcu_struct global_prcu = {
> >> @@ -16,6 +17,16 @@ struct prcu_struct global_prcu = {
> >>  };
> >>  struct prcu_struct *prcu = &global_prcu;
> >>
> >> +/* Initialize simple callback list. */
> >> +static void prcu_cblist_init(struct prcu_cblist *rclp)
> >> +{
> >> +     rclp->head = NULL;
> >> +     rclp->tail = &rclp->head;
> >> +     rclp->version_head = NULL;
> >> +     rclp->version_tail = &rclp->version_head;
> >> +     rclp->len = 0;
> >> +}
> >> +
> >>  static inline void prcu_report(struct prcu_local_struct *local)
> >>  {
> >>       unsigned long long global_version;
> >> @@ -123,3 +134,53 @@ void prcu_note_context_switch(void)
> >>       prcu_report(local);
> >>       put_cpu_ptr(&prcu_local);
> >>  }
> >> +
> >> +void call_prcu(struct rcu_head *head, rcu_callback_t func)
> >> +{
> >> +     unsigned long flags;
> >> +     struct prcu_local_struct *local;
> >> +     struct prcu_cblist *rclp;
> >> +     struct prcu_version_head *vhp;
> >> +
> >> +     debug_rcu_head_queue(head);
> >> +
> >> +     /* Use GFP_ATOMIC with IRQs disabled */
> >> +     vhp = kmalloc(sizeof(struct prcu_version_head), GFP_ATOMIC);
> >> +     if (!vhp)
> >> +             return;
> >
> > Silently failing to post the callback can cause system hangs.  I suggest
> > finding some way to avoid allocating on the call_prcu() code path.
> >
> 
> You're absolutely right. We were also thinking of changing the
> function return type from void to int to indicate whether the memory
> allocation is successful or not.

Suppose that you are a user of such a function.  When it returns indicating
failure, what are you supposed to do?  What would be the complexity of
the resulting failure-handling code?

Having it simply unconditionally succeed is much friendlier to the user,
especially given that it is not all that hard to make it do so.

							Thanx, Paul

Powered by blists - more mailing lists