Messages in this thread Patch in this message | | | Date | Fri, 26 Jan 2018 14:22:59 -0800 | From | "Paul E. McKenney" <> | 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@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
| |