Re: [PATCH 4/6] rcusync: Introduce struct rcu_sync_ops

From: Lai Jiangshan
Date: Thu Oct 17 2013 - 21:19:16 EST

On 10/17/2013 11:42 PM, Paul E. McKenney wrote:
> On Thu, Oct 17, 2013 at 10:07:15AM +0800, Lai Jiangshan wrote:
>> On 10/08/2013 06:25 PM, Peter Zijlstra wrote:
>>> From: Oleg Nesterov <oleg@xxxxxxxxxx>
>>>
>>> Add the new struct rcu_sync_ops which holds sync/call methods, and
>>> turn the function pointers in rcu_sync_struct into an array of struct
>>> rcu_sync_ops.
>>
>> Hi, Paul
>>
>> I think this work should be done in rcupdate.[ch] side by introducing
>> struct rcu_flavor.
>
> I -do- have on my list to add an rcutorture test for rcu_sync, but
> what do you have in mind by adding struct rcu_flavor? I am guessing
> that you do not mean to try to create an rcu_state and a set of

No.
The thing what I need is just as same as Oleg Nesterov implemented.
It is just a structure with several function pointers for different RCU variants.
But it would be better if we implement in rcupdate.[ch],
and name it to struct rcu_flavor like the URCU.

After we have struct rcu_flavor, we can replace the following code
to a pointer to struct rcu_flavor.

struct rcu_state:
void (*call)(struct rcu_head *head, /* call_rcu() flavor. */
void (*func)(struct rcu_head *head));

struct rcu_torture_ops {
int (*readlock)(void);
void (*readunlock)(int idx);
void (*sync)(void);
void (*exp_sync)(void);
void (*call)(struct rcu_head *head, void (*func)(struct rcu_head *rcu));
void (*cb_barrier)(void);
};

and struct rcu_sync {
...
};


Thanks,
Lai.



> rcu_data structures for rcu_sync -- the write-side optimizations
> would not fit well into rcu_state/rcu_data.
>
> You might mean to simply put this code into rcupdate.[ch], which
> would be OK, but rcu_sync seems to me to be sufficiently different
> to deserve its own .c and .h files.
>
> So what did you have in mind?

I mean we need something like struct rcu_flavor as the URCU.


>
> Thanx, Paul
>
>> Thanks,
>> Lai
>>
>>>
>>> This simplifies the "init" helpers, and this way it is simpler to add
>>> the new methods we need, especially ifdef'ed.
>>>
>>> Signed-off-by: Peter Zijlstra <peterz@xxxxxxxxxxxxx>
>>> Link: http://lkml.kernel.org/r/20131005171746.GA17664@xxxxxxxxxx
>>> ---
>>>
>>> diff --git a/include/linux/rcusync.h b/include/linux/rcusync.h
>>> index 7858491..988ec33 100644
>>> --- a/include/linux/rcusync.h
>>> +++ b/include/linux/rcusync.h
>>> @@ -4,6 +4,8 @@
>>> #include <linux/wait.h>
>>> #include <linux/rcupdate.h>
>>>
>>> +enum rcu_sync_type { RCU_SYNC, RCU_SCHED_SYNC, RCU_BH_SYNC };
>>> +
>>> struct rcu_sync_struct {
>>> int gp_state;
>>> int gp_count;
>>> @@ -12,53 +14,37 @@ struct rcu_sync_struct {
>>> int cb_state;
>>> struct rcu_head cb_head;
>>>
>>> - void (*sync)(void);
>>> - void (*call)(struct rcu_head *, void (*)(struct rcu_head *));
>>> + enum rcu_sync_type gp_type;
>>> };
>>>
>>> -#define ___RCU_SYNC_INIT(name) \
>>> - .gp_state = 0, \
>>> - .gp_count = 0, \
>>> - .gp_wait = __WAIT_QUEUE_HEAD_INITIALIZER(name.gp_wait), \
>>> - .cb_state = 0
>>> -
>>> -#define __RCU_SCHED_SYNC_INIT(name) { \
>>> - ___RCU_SYNC_INIT(name), \
>>> - .sync = synchronize_sched, \
>>> - .call = call_rcu_sched, \
>>> -}
>>> -
>>> -#define __RCU_BH_SYNC_INIT(name) { \
>>> - ___RCU_SYNC_INIT(name), \
>>> - .sync = synchronize_rcu_bh, \
>>> - .call = call_rcu_bh, \
>>> -}
>>> -
>>> -#define __RCU_SYNC_INIT(name) { \
>>> - ___RCU_SYNC_INIT(name), \
>>> - .sync = synchronize_rcu, \
>>> - .call = call_rcu, \
>>> -}
>>> -
>>> -#define DEFINE_RCU_SCHED_SYNC(name) \
>>> - struct rcu_sync_struct name = __RCU_SCHED_SYNC_INIT(name)
>>> -
>>> -#define DEFINE_RCU_BH_SYNC(name) \
>>> - struct rcu_sync_struct name = __RCU_BH_SYNC_INIT(name)
>>> -
>>> -#define DEFINE_RCU_SYNC(name) \
>>> - struct rcu_sync_struct name = __RCU_SYNC_INIT(name)
>>> -
>>> static inline bool rcu_sync_is_idle(struct rcu_sync_struct *rss)
>>> {
>>> return !rss->gp_state; /* GP_IDLE */
>>> }
>>>
>>> -enum rcu_sync_type { RCU_SYNC, RCU_SCHED_SYNC, RCU_BH_SYNC };
>>> -
>>> extern void rcu_sync_init(struct rcu_sync_struct *, enum rcu_sync_type);
>>> extern void rcu_sync_enter(struct rcu_sync_struct *);
>>> extern void rcu_sync_exit(struct rcu_sync_struct *);
>>>
>>> +#define __RCU_SYNC_INITIALIZER(name, type) { \
>>> + .gp_state = 0, \
>>> + .gp_count = 0, \
>>> + .gp_wait = __WAIT_QUEUE_HEAD_INITIALIZER(name.gp_wait), \
>>> + .cb_state = 0, \
>>> + .gp_type = type, \
>>> + }
>>> +
>>> +#define __DEFINE_RCU_SYNC(name, type) \
>>> + struct rcu_sync_struct name = __RCU_SYNC_INITIALIZER(name, type)
>>> +
>>> +#define DEFINE_RCU_SYNC(name) \
>>> + __DEFINE_RCU_SYNC(name, RCU_SYNC)
>>> +
>>> +#define DEFINE_RCU_SCHED_SYNC(name) \
>>> + __DEFINE_RCU_SYNC(name, RCU_SCHED_SYNC)
>>> +
>>> +#define DEFINE_RCU_BH_SYNC(name) \
>>> + __DEFINE_RCU_SYNC(name, RCU_BH_SYNC)
>>> +
>>> #endif /* _LINUX_RCUSYNC_H_ */
>>>
>>> diff --git a/kernel/rcusync.c b/kernel/rcusync.c
>>> index f84176a..99051b7 100644
>>> --- a/kernel/rcusync.c
>>> +++ b/kernel/rcusync.c
>>> @@ -1,7 +1,24 @@
>>> -
>>> #include <linux/rcusync.h>
>>> #include <linux/sched.h>
>>>
>>> +static const struct {
>>> + void (*sync)(void);
>>> + void (*call)(struct rcu_head *, void (*)(struct rcu_head *));
>>> +} gp_ops[] = {
>>> + [RCU_SYNC] = {
>>> + .sync = synchronize_rcu,
>>> + .call = call_rcu,
>>> + },
>>> + [RCU_SCHED_SYNC] = {
>>> + .sync = synchronize_sched,
>>> + .call = call_rcu_sched,
>>> + },
>>> + [RCU_BH_SYNC] = {
>>> + .sync = synchronize_rcu_bh,
>>> + .call = call_rcu_bh,
>>> + },
>>> +};
>>> +
>>> enum { GP_IDLE = 0, GP_PENDING, GP_PASSED };
>>> enum { CB_IDLE = 0, CB_PENDING, CB_REPLAY };
>>>
>>> @@ -11,23 +28,7 @@ void rcu_sync_init(struct rcu_sync_struct *rss, enum rcu_sync_type type)
>>> {
>>> memset(rss, 0, sizeof(*rss));
>>> init_waitqueue_head(&rss->gp_wait);
>>> -
>>> - switch (type) {
>>> - case RCU_SYNC:
>>> - rss->sync = synchronize_rcu;
>>> - rss->call = call_rcu;
>>> - break;
>>> -
>>> - case RCU_SCHED_SYNC:
>>> - rss->sync = synchronize_sched;
>>> - rss->call = call_rcu_sched;
>>> - break;
>>> -
>>> - case RCU_BH_SYNC:
>>> - rss->sync = synchronize_rcu_bh;
>>> - rss->call = call_rcu_bh;
>>> - break;
>>> - }
>>> + rss->gp_type = type;
>>> }
>>>
>>> void rcu_sync_enter(struct rcu_sync_struct *rss)
>>> @@ -44,7 +45,7 @@ void rcu_sync_enter(struct rcu_sync_struct *rss)
>>> BUG_ON(need_wait && need_sync);
>>>
>>> if (need_sync) {
>>> - rss->sync();
>>> + gp_ops[rss->gp_type].sync();
>>> rss->gp_state = GP_PASSED;
>>> wake_up_all(&rss->gp_wait);
>>> } else if (need_wait) {
>>> @@ -81,7 +82,7 @@ static void rcu_sync_func(struct rcu_head *rcu)
>>> * to catch a later GP.
>>> */
>>> rss->cb_state = CB_PENDING;
>>> - rss->call(&rss->cb_head, rcu_sync_func);
>>> + gp_ops[rss->gp_type].call(&rss->cb_head, rcu_sync_func);
>>> } else {
>>> /*
>>> * We're at least a GP after rcu_sync_exit(); eveybody will now
>>> @@ -99,7 +100,7 @@ void rcu_sync_exit(struct rcu_sync_struct *rss)
>>> if (!--rss->gp_count) {
>>> if (rss->cb_state == CB_IDLE) {
>>> rss->cb_state = CB_PENDING;
>>> - rss->call(&rss->cb_head, rcu_sync_func);
>>> + gp_ops[rss->gp_type].call(&rss->cb_head, rcu_sync_func);
>>> } else if (rss->cb_state == CB_PENDING) {
>>> rss->cb_state = CB_REPLAY;
>>> }
>>>
>>>
>>>
>>> --
>>> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
>>> the body of a message to majordomo@xxxxxxxxxxxxxxx
>>> More majordomo info at http://vger.kernel.org/majordomo-info.html
>>> Please read the FAQ at http://www.tux.org/lkml/
>>>
>>
>

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/