Re: [PATCH 2/3] rcu: Create rcu_sync infrastructure

["Paul E. McKenney" <paulmck@linux.vnet.ibm.com>]

On Thu, Oct 03, 2013 at 11:10:09PM +0200, Oleg Nesterov wrote:
> On 10/03, Oleg Nesterov wrote:
> >
> > So unless Peter objects I'll write the changelogs (always nontrivial task),
> > test, and send these 2 patches + "add ops->barr() / rcu_sync_wait_for_cb"
> > tomorrow.
> 
> And, can't resist, probably another patch below (incomplete, needs the
> obvious change in cpu.c and the new trivial __complete_locked() helper).
> 
> Hmm. But when I look at wait_for_completion() I think it is buggy. Will
> try to send the fix tomorrow.
> 
> Oleg.
> 
> rcusync: introduce rcu_sync_struct->exclusive mode
> 
> CHANGELOG.

Should the changelog really be in all caps?  (Sorry, couldn't resist...)

One confusion below (mine), otherwise cannot see any obvious problems
with it.  Not that this should count for much, getting a bit punch-drunk
reviewing this sort of code.  ;-)

							Thanx, Paul

> ---
> 
> diff --git a/include/linux/rcusync.h b/include/linux/rcusync.h
> index ab787c1..265875c 100644
> --- a/include/linux/rcusync.h
> +++ b/include/linux/rcusync.h
> @@ -1,8 +1,8 @@
>  #ifndef _LINUX_RCUSYNC_H_
>  #define _LINUX_RCUSYNC_H_
> 
> -#include <linux/wait.h>
>  #include <linux/rcupdate.h>
> +#include <linux/completion.h>
> 
>  struct rcu_sync_ops {
>  	void (*sync)(void);
> @@ -15,11 +15,12 @@ struct rcu_sync_ops {
>  struct rcu_sync_struct {
>  	int			gp_state;
>  	int			gp_count;
> -	wait_queue_head_t	gp_wait;
> +	struct completion	gp_comp;
> 
>  	int			cb_state;
>  	struct rcu_head		cb_head;
> 
> +	bool			exclusive;
>  	struct rcu_sync_ops	*ops;
>  };
> 
> @@ -33,31 +34,33 @@ static inline bool rcu_sync_is_idle(struct rcu_sync_struct *rss)
> 
>  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_init(struct rcu_sync_struct *,
> +				enum rcu_sync_type, bool excl);
>  extern void rcu_sync_enter(struct rcu_sync_struct *);
>  extern void rcu_sync_exit(struct rcu_sync_struct *);
> 
>  extern struct rcu_sync_ops rcu_sync_ops_array[];
> 
> -#define __RCU_SYNC_INITIALIZER(name, type) {				\
> +#define __RCU_SYNC_INITIALIZER(name, type, excl) {			\
>  		.gp_state = 0,						\
>  		.gp_count = 0,						\
> -		.gp_wait = __WAIT_QUEUE_HEAD_INITIALIZER(name.gp_wait),	\
> +		.gp_comp = COMPLETION_INITIALIZER(name.gp_comp),	\
>  		.cb_state = 0,						\
> +		.exclusive = excl,					\
>  		.ops = rcu_sync_ops_array + (type),			\
>  	}
> 
> -#define	__DEFINE_RCU_SYNC(name, type)	\
> -	struct rcu_sync_struct name = __RCU_SYNC_INITIALIZER(name, type)
> +#define	__DEFINE_RCU_SYNC(name, type, excl)	\
> +	struct rcu_sync_struct name = __RCU_SYNC_INITIALIZER(name, type, excl)
> 
> -#define DEFINE_RCU_SYNC(name)		\
> -	__DEFINE_RCU_SYNC(name, RCU_SYNC)
> +#define DEFINE_RCU_SYNC(name, excl)		\
> +	__DEFINE_RCU_SYNC(name, RCU_SYNC, excl)
> 
> -#define DEFINE_RCU_SCHED_SYNC(name)	\
> -	__DEFINE_RCU_SYNC(name, RCU_SCHED_SYNC)
> +#define DEFINE_RCU_SCHED_SYNC(name, excl)	\
> +	__DEFINE_RCU_SYNC(name, RCU_SCHED_SYNC, excl)
> 
> -#define DEFINE_RCU_BH_SYNC(name)	\
> -	__DEFINE_RCU_SYNC(name, RCU_BH_SYNC)
> +#define DEFINE_RCU_BH_SYNC(name, excl)		\
> +	__DEFINE_RCU_SYNC(name, RCU_BH_SYNC, excl)
> 
>  #endif /* _LINUX_RCUSYNC_H_ */
> 
> diff --git a/kernel/rcusync.c b/kernel/rcusync.c
> index 21cde9b..d8068df 100644
> --- a/kernel/rcusync.c
> +++ b/kernel/rcusync.c
> @@ -4,7 +4,7 @@
>  enum { GP_IDLE = 0, GP_PENDING, GP_PASSED };
>  enum { CB_IDLE = 0, CB_PENDING, CB_REPLAY };
> 
> -#define	rss_lock	gp_wait.lock
> +#define	rss_lock	gp_comp.wait.lock
> 
>  #ifdef CONFIG_PROVE_RCU
>  #define __INIT_HELD(func)	.held = func,
> @@ -30,11 +30,13 @@ struct rcu_sync_ops rcu_sync_ops_array[] = {
>  	},
>  };
> 
> -void rcu_sync_init(struct rcu_sync_struct *rss, enum rcu_sync_type type)
> +void rcu_sync_init(struct rcu_sync_struct *rss,
> +			enum rcu_sync_type type, bool excl)
>  {
>  	memset(rss, 0, sizeof(*rss));
> -	init_waitqueue_head(&rss->gp_wait);
> +	init_completion(&rss->gp_comp);
>  	rss->ops = rcu_sync_ops_array + type;
> +	rss->exclusive = excl;
>  }
> 
>  void rcu_sync_enter(struct rcu_sync_struct *rss)
> @@ -53,9 +55,13 @@ void rcu_sync_enter(struct rcu_sync_struct *rss)
>  	if (need_sync) {
>  		rss->ops->sync();
>  		rss->gp_state = GP_PASSED;
> -		wake_up_all(&rss->gp_wait);
> +		if (!rss->exclusive)
> +			wake_up_all(&rss->gp_comp.wait);

Not sure about the wake_up_all() on a completion, but if we are exclusive,
don't we need to complete() the completion here?

Oh, I guess gp_comp.wait is exactly a wait_queue_head_t, so I guess
you can do wake_up_all() on it...  And the thing we would wake up
is ourselves, and we are already awake.

Never mind!!!

>  	} else if (need_wait) {
> -		wait_event(rss->gp_wait, rss->gp_state == GP_PASSED);
> +		if (!rss->exclusive)
> +			wait_event(rss->gp_comp.wait, rss->gp_state == GP_PASSED);
> +		else
> +			wait_for_completion(&rss->gp_comp);
>  	} else {
>  		/*
>  		 * Possible when there's a pending CB from a rcu_sync_exit().
> @@ -110,6 +116,8 @@ void rcu_sync_exit(struct rcu_sync_struct *rss)
>  		} else if (rss->cb_state == CB_PENDING) {
>  			rss->cb_state = CB_REPLAY;
>  		}
> +	} else if (rss->exclusive) {
> +		__complete_locked(&rss->gp_comp);
>  	}
>  	spin_unlock_irq(&rss->rss_lock);
>  }
> 
> --
> 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/
>