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

[Lihao Liang]

On Thu, Jan 25, 2018 at 6:20 AM, Paul E. McKenney
<paulmck@xxxxxxxxxxxxxxxxxx> wrote:
> On Tue, Jan 23, 2018 at 03:59:32PM +0800, lianglihao@xxxxxxxxxx wrote:
>> From: Lihao Liang <lianglihao@xxxxxxxxxx>
>>
>> This is PRCU's counterpart of RCU's call_rcu() API.
>>
>> Reviewed-by: Heng Zhang <heng.z@xxxxxxxxxx>
>> Signed-off-by: Lihao Liang <lianglihao@xxxxxxxxxx>
>> ---
>>  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.

Best,
Lihao.

>                                                         Thanx, Paul
>
>> +
>> +     head->func = func;
>> +     head->next = NULL;
>> +     vhp->next = NULL;
>> +
>> +     local_irq_save(flags);
>> +     local = this_cpu_ptr(&prcu_local);
>> +     vhp->version = local->version;
>> +     rclp = &local->cblist;
>> +     rclp->len++;
>> +     *rclp->tail = head;
>> +     rclp->tail = &head->next;
>> +     *rclp->version_tail = vhp;
>> +     rclp->version_tail = &vhp->next;
>> +     local_irq_restore(flags);
>> +}
>> +EXPORT_SYMBOL(call_prcu);
>> +
>> +void prcu_init_local_struct(int cpu)
>> +{
>> +     struct prcu_local_struct *local;
>> +
>> +     local = per_cpu_ptr(&prcu_local, cpu);
>> +     local->locked = 0;
>> +     local->online = 0;
>> +     local->version = 0;
>> +     prcu_cblist_init(&local->cblist);
>> +}
>> +
>> +void __init prcu_init(void)
>> +{
>> +     int cpu;
>> +
>> +     for_each_possible_cpu(cpu)
>> +             prcu_init_local_struct(cpu);
>> +}
>> --
>> 2.14.1.729.g59c0ea183
>>
>