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

From: Lihao Liang
Date: Fri Jan 26 2018 - 03:44:59 EST


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