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

[Lai Jiangshan <laijs@cn.fujitsu.com>]

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@redhat.com>
>>>
>>> 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@infradead.org>
>>> Link: http://lkml.kernel.org/r/20131005171746.GA17664@redhat.com
>>> ---
>>>
>>> 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@vger.kernel.org
>>> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>>> Please read the FAQ at  http://www.tux.org/lkml/
>>>
>>
>