Re: [PATCH RFC 07/16] prcu: Implement call_prcu() API

["Paul E. McKenney" <paulmck@linux.vnet.ibm.com>]

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@linux.vnet.ibm.com> wrote:
> > On Tue, Jan 23, 2018 at 03:59:32PM +0800, lianglihao@huawei.com wrote:
> >> From: Lihao Liang <lianglihao@huawei.com>
> >>
> >> This is PRCU's counterpart of RCU's call_rcu() API.
> >>
> >> Reviewed-by: Heng Zhang <heng.z@huawei.com>
> >> Signed-off-by: Lihao Liang <lianglihao@huawei.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