Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1441923 > unrolled thread

Severe performance regression w/ 4.4+ on Android due to cgroup locking changes

Back to article view | Back to linux.kernel

Contents

  Severe performance regression w/ 4.4+ on Android due to cgroup  locking changes John Stultz <john.stultz@linaro.org> - 2016-07-13 02:10 +0200
    Re: Severe performance regression w/ 4.4+ on Android due to cgroup  locking changes Peter Zijlstra <peterz@infradead.org> - 2016-07-13 10:30 +0200
      Re: Severe performance regression w/ 4.4+ on Android due to cgroup  locking changes "Paul E. McKenney" <paulmck@linux.vnet.ibm.com> - 2016-07-13 16:50 +0200
        Re: Severe performance regression w/ 4.4+ on Android due to cgroup  locking changes "Paul E. McKenney" <paulmck@linux.vnet.ibm.com> - 2016-07-13 20:40 +0200
    Re: Severe performance regression w/ 4.4+ on Android due to cgroup  locking changes Tejun Heo <tj@kernel.org> - 2016-07-13 20:30 +0200
      Re: Severe performance regression w/ 4.4+ on Android due to cgroup  locking changes Tejun Heo <tj@kernel.org> - 2016-07-13 20:40 +0200
        Re: Severe performance regression w/ 4.4+ on Android due to cgroup  locking changes Tejun Heo <tj@kernel.org> - 2016-07-13 22:20 +0200
          Re: Severe performance regression w/ 4.4+ on Android due to cgroup  locking changes Peter Zijlstra <peterz@infradead.org> - 2016-07-13 22:30 +0200
            Re: Severe performance regression w/ 4.4+ on Android due to cgroup  locking changes Tejun Heo <tj@kernel.org> - 2016-07-13 22:50 +0200
              Re: Severe performance regression w/ 4.4+ on Android due to cgroup  locking changes Peter Zijlstra <peterz@infradead.org> - 2016-07-13 23:00 +0200
                Re: Severe performance regression w/ 4.4+ on Android due to cgroup  locking changes Tejun Heo <tj@kernel.org> - 2016-07-13 23:10 +0200
                  Re: Severe performance regression w/ 4.4+ on Android due to cgroup  locking changes "Paul E. McKenney" <paulmck@linux.vnet.ibm.com> - 2016-07-13 23:20 +0200
                    Re: Severe performance regression w/ 4.4+ on Android due to cgroup  locking changes "Paul E. McKenney" <paulmck@linux.vnet.ibm.com> - 2016-07-13 23:50 +0200
                      Re: Severe performance regression w/ 4.4+ on Android due to cgroup  locking changes John Stultz <john.stultz@linaro.org> - 2016-07-13 23:50 +0200
                        Re: Severe performance regression w/ 4.4+ on Android due to cgroup  locking changes "Paul E. McKenney" <paulmck@linux.vnet.ibm.com> - 2016-07-14 00:20 +0200
                          Re: Severe performance regression w/ 4.4+ on Android due to cgroup  locking changes John Stultz <john.stultz@linaro.org> - 2016-07-14 00:40 +0200
                            Re: Severe performance regression w/ 4.4+ on Android due to cgroup  locking changes "Paul E. McKenney" <paulmck@linux.vnet.ibm.com> - 2016-07-14 01:10 +0200
                              Re: Severe performance regression w/ 4.4+ on Android due to cgroup  locking changes "Paul E. McKenney" <paulmck@linux.vnet.ibm.com> - 2016-07-14 01:10 +0200
                                Re: Severe performance regression w/ 4.4+ on Android due to cgroup  locking changes Tejun Heo <tj@kernel.org> - 2016-07-14 13:40 +0200
                                  Re: Severe performance regression w/ 4.4+ on Android due to cgroup  locking changes Peter Zijlstra <peterz@infradead.org> - 2016-07-14 14:10 +0200
                                    Re: Severe performance regression w/ 4.4+ on Android due to cgroup  locking changes Tejun Heo <tj@kernel.org> - 2016-07-14 14:10 +0200
                                      Re: Severe performance regression w/ 4.4+ on Android due to cgroup  locking changes Peter Zijlstra <peterz@infradead.org> - 2016-07-14 14:30 +0200
                                        Re: Severe performance regression w/ 4.4+ on Android due to cgroup  locking changes Tejun Heo <tj@kernel.org> - 2016-07-14 17:10 +0200
                                          Re: Severe performance regression w/ 4.4+ on Android due to cgroup  locking changes Tejun Heo <tj@kernel.org> - 2016-07-14 17:30 +0200
                                          Re: Severe performance regression w/ 4.4+ on Android due to cgroup  locking changes Peter Zijlstra <peterz@infradead.org> - 2016-07-14 18:40 +0200
                                            Re: Severe performance regression w/ 4.4+ on Android due to cgroup  locking changes Oleg Nesterov <oleg@redhat.com> - 2016-07-14 19:40 +0200
                              Re: Severe performance regression w/ 4.4+ on Android due to cgroup  locking changes John Stultz <john.stultz@linaro.org> - 2016-07-14 19:00 +0200
                      Re: Severe performance regression w/ 4.4+ on Android due to cgroup  locking changes John Stultz <john.stultz@linaro.org> - 2016-07-14 00:30 +0200
                    Re: Severe performance regression w/ 4.4+ on Android due to cgroup  locking changes Tejun Heo <tj@kernel.org> - 2016-07-14 00:10 +0200
                      Re: Severe performance regression w/ 4.4+ on Android due to cgroup  locking changes "Paul E. McKenney" <paulmck@linux.vnet.ibm.com> - 2016-07-14 00:40 +0200
                      Re: Severe performance regression w/ 4.4+ on Android due to cgroup  locking changes Peter Zijlstra <peterz@infradead.org> - 2016-07-14 09:00 +0200
                        Re: Severe performance regression w/ 4.4+ on Android due to cgroup  locking changes Tejun Heo <tj@kernel.org> - 2016-07-14 13:30 +0200
                          Re: Severe performance regression w/ 4.4+ on Android due to cgroup  locking changes Peter Zijlstra <peterz@infradead.org> - 2016-07-14 14:20 +0200
                            Re: Severe performance regression w/ 4.4+ on Android due to cgroup  locking changes Tejun Heo <tj@kernel.org> - 2016-07-14 17:20 +0200
                Re: Severe performance regression w/ 4.4+ on Android due to cgroup  locking changes Tejun Heo <tj@kernel.org> - 2016-07-13 23:10 +0200
                Re: Severe performance regression w/ 4.4+ on Android due to cgroup  locking changes "Paul E. McKenney" <paulmck@linux.vnet.ibm.com> - 2016-07-13 23:10 +0200
                Re: Severe performance regression w/ 4.4+ on Android due to cgroup  locking changes Peter Zijlstra <peterz@infradead.org> - 2016-07-14 15:20 +0200
                  Re: Severe performance regression w/ 4.4+ on Android due to cgroup  locking changes Peter Zijlstra <peterz@infradead.org> - 2016-07-14 16:20 +0200
                  Re: Severe performance regression w/ 4.4+ on Android due to cgroup  locking changes Oleg Nesterov <oleg@redhat.com> - 2016-07-14 17:00 +0200
                    Re: Severe performance regression w/ 4.4+ on Android due to cgroup  locking changes Peter Zijlstra <peterz@infradead.org> - 2016-07-14 18:20 +0200
                    Re: Severe performance regression w/ 4.4+ on Android due to cgroup  locking changes Peter Zijlstra <peterz@infradead.org> - 2016-07-14 18:40 +0200
                      Re: Severe performance regression w/ 4.4+ on Android due to cgroup  locking changes Oleg Nesterov <oleg@redhat.com> - 2016-07-14 19:10 +0200
                  Re: Severe performance regression w/ 4.4+ on Android due to cgroup  locking changes "Paul E. McKenney" <paulmck@linux.vnet.ibm.com> - 2016-07-14 18:30 +0200
                    Re: Severe performance regression w/ 4.4+ on Android due to cgroup  locking changes Peter Zijlstra <peterz@infradead.org> - 2016-07-14 18:50 +0200
                      Re: Severe performance regression w/ 4.4+ on Android due to cgroup  locking changes "Paul E. McKenney" <paulmck@linux.vnet.ibm.com> - 2016-07-14 19:20 +0200
                  Re: Severe performance regression w/ 4.4+ on Android due to cgroup  locking changes John Stultz <john.stultz@linaro.org> - 2016-07-14 18:50 +0200
                    Re: Severe performance regression w/ 4.4+ on Android due to cgroup  locking changes Peter Zijlstra <peterz@infradead.org> - 2016-07-14 18:50 +0200
                      Re: Severe performance regression w/ 4.4+ on Android due to cgroup  locking changes John Stultz <john.stultz@linaro.org> - 2016-07-14 19:10 +0200
                        Re: Severe performance regression w/ 4.4+ on Android due to cgroup  locking changes Oleg Nesterov <oleg@redhat.com> - 2016-07-14 19:20 +0200
                          Re: Severe performance regression w/ 4.4+ on Android due to cgroup  locking changes John Stultz <john.stultz@linaro.org> - 2016-07-14 19:40 +0200
                            Re: Severe performance regression w/ 4.4+ on Android due to cgroup  locking changes Oleg Nesterov <oleg@redhat.com> - 2016-07-14 19:50 +0200
                              Re: Severe performance regression w/ 4.4+ on Android due to cgroup  locking changes John Stultz <john.stultz@linaro.org> - 2016-07-14 20:00 +0200
                  Re: Severe performance regression w/ 4.4+ on Android due to cgroup  locking changes Oleg Nesterov <oleg@redhat.com> - 2016-07-14 20:10 +0200
                    Re: Severe performance regression w/ 4.4+ on Android due to cgroup  locking changes Peter Zijlstra <peterz@infradead.org> - 2016-07-14 20:40 +0200
                      Re: Severe performance regression w/ 4.4+ on Android due to cgroup  locking changes Peter Zijlstra <peterz@infradead.org> - 2016-07-14 21:40 +0200
              Re: Severe performance regression w/ 4.4+ on Android due to cgroup  locking changes John Stultz <john.stultz@linaro.org> - 2016-07-13 23:00 +0200
            Re: Severe performance regression w/ 4.4+ on Android due to cgroup  locking changes Peter Zijlstra <peterz@infradead.org> - 2016-07-13 23:00 +0200
              Re: Severe performance regression w/ 4.4+ on Android due to cgroup  locking changes "Paul E. McKenney" <paulmck@linux.vnet.ibm.com> - 2016-07-13 23:10 +0200
            Re: Severe performance regression w/ 4.4+ on Android due to cgroup  locking changes "Paul E. McKenney" <paulmck@linux.vnet.ibm.com> - 2016-07-13 23:00 +0200
              Re: Severe performance regression w/ 4.4+ on Android due to cgroup  locking changes Dmitry Shmidt <dimitrysh@google.com> - 2016-07-13 23:10 +0200
                Re: Severe performance regression w/ 4.4+ on Android due to cgroup  locking changes "Paul E. McKenney" <paulmck@linux.vnet.ibm.com> - 2016-07-13 23:10 +0200
                Re: Severe performance regression w/ 4.4+ on Android due to cgroup  locking changes John Stultz <john.stultz@linaro.org> - 2016-07-13 23:10 +0200
        Re: Severe performance regression w/ 4.4+ on Android due to cgroup  locking changes John Stultz <john.stultz@linaro.org> - 2016-07-13 22:20 +0200
        Re: Severe performance regression w/ 4.4+ on Android due to cgroup  locking changes Dmitry Shmidt <dimitrysh@google.com> - 2016-07-13 22:40 +0200
      Re: Severe performance regression w/ 4.4+ on Android due to cgroup  locking changes Colin Cross <ccross@google.com> - 2016-07-13 22:50 +0200
        Re: Severe performance regression w/ 4.4+ on Android due to cgroup  locking changes Tejun Heo <tj@kernel.org> - 2016-07-13 23:00 +0200

Page 3 of 4 — ← Prev page 1 2 [3] 4  Next page →

#1443587

On Thu, Jul 14, 2016 at 04:58:44PM +0200, Oleg Nesterov wrote:
> 
> But note that we do not need RCU_NONE. All we need is the trivial change
> below.

Hurm, maybe. So having that unbalanced keeps us in GP_PASSED state and
since we'll never drop gp_count back to 0 nothing will ever happen.

Cute, yes.

> Damn, I am trying to find my old rcu-sync patches which I didn't
> send, but can't... OK, this almost off-topic right now, just this "enter"
> is ugly and we can't switch the slow/fast modes dynamically.
> 
> The rest of you patch is "optimize the slow path" and we already discussed
> it before, I personally like it. Perhaps you can redo it without RCU_NONE
> part?

Indeed, I rebased that patch on top of the current tree and had to add
support for down_trylock() but otherwise much the same thing.

I can send it out again.

[toc] | [prev] | [next] | [standalone]

#1443602

On 07/14, Peter Zijlstra wrote:
>
> On Thu, Jul 14, 2016 at 04:58:44PM +0200, Oleg Nesterov wrote:
> >
> > But note that we do not need RCU_NONE. All we need is the trivial change
> > below.
>
> Hurm, maybe. So having that unbalanced keeps us in GP_PASSED state and
> since we'll never drop gp_count back to 0 nothing will ever happen.

Yes, exactly. Of course, this is a bit ugly, I'll try to send some
rcu-sync cleanups/simplifications later.

Perhaps we can even add a "cgroup migration is not that cold as it was
supposed" boot option for this rcu_sync_enter() for now, not sure.

> > Damn, I am trying to find my old rcu-sync patches which I didn't
> > send, but can't... OK, this almost off-topic right now, just this "enter"
> > is ugly and we can't switch the slow/fast modes dynamically.
> >
> > The rest of you patch is "optimize the slow path" and we already discussed
> > it before, I personally like it. Perhaps you can redo it without RCU_NONE
> > part?
>
> Indeed, I rebased that patch on top of the current tree and had to add
> support for down_trylock() but otherwise much the same thing.
>
> I can send it out again.

Great, thanks! This should fix the problem.

Oleg.

[toc] | [prev] | [next] | [standalone]

#1443576

On Thu, Jul 14, 2016 at 03:18:09PM +0200, Peter Zijlstra wrote:
> On Wed, Jul 13, 2016 at 10:51:02PM +0200, Peter Zijlstra wrote:
> > So, IIRC, the trade-off is a full memory barrier in read_lock and
> > read_unlock() vs sync_sched() in write.
> > 
> > Full memory barriers are expensive and while the combined cost might
> > well exceed the cost of the sync_sched() it doesn't suffer the latency
> > issues.
> > 
> > Not sure if we can frob the two in a single codebase, but I can have a
> > poke if Oleg or Paul doesn't beat me to it.
> 
> OK, not too horrible if I say so myself :-)
> 
> The below is a compile tested only first draft so far. I'll go give it
> some runtime next.


Hmmm...  How does this handle the following sequence of events for
the case where we are not biased towards the reader?

o	The per-CPU rwsem is set up with RCU_NONE and readers_slow
	(as opposed to readers_block).  The rcu_sync ->gp_state is
	GP_PENDING, which means that rcu_sync_is_idle() will always
	return true.

o	Task A on CPU 0 runs percpu_down_read() to completion, and remains
	within the critical section.  CPU 0's ->refcount is therefore 1.

o	Task B on CPU 1 does percpu_down_write(), which write-acquires
	->rw_sem, does rcu_sync_enter() (which is a no-op due to
	RCU_NONE), sets ->state to readers_block, and is just now going
	to wait for readers, which first invokes readers_active_check(),
	which starts summing the ->refcount for CPUs 0, 1, and 2,
	obtaining the value 1 so far.

o	Task C CPU 2 enters percpu_down_read(), disables preemption,
	increments CPU 2's ->refcount, sees rcu_sync_is_idle() return
	true (so skips __percpu_down_read()), enables preemption, and
	enters its critical section.

o	Task C migrates to CPU 3 and invokes percpu_up_read(), which
	disables preemption, sees rcu_sync_is_idle() return true, calls
	__this_cpu_dec() on CPU 3's ->refcount, and enables preemption.
	The value of CPU 3's ->refcount is thus (unsigned int)-1.

o	Task B on CPU 1 continues execution in readers_active_check(), with
	the full sum being zero.

So it looks to me like we have Task A as a writer at the same time that
Task A is a reader, which would not be so good.

So what am I missing here?

And a couple of checkpatch nits below.  Yes, I had to apply the patch to
figure out what it was doing.  ;-)

							Thanx, Paul

> ---
>  fs/super.c                    |   3 +-
>  include/linux/percpu-rwsem.h  |  96 +++++++++++++++--
>  include/linux/rcu_sync.h      |   2 +-
>  kernel/locking/percpu-rwsem.c | 243 +++++++++++++++++++++++++-----------------
>  kernel/rcu/sync.c             |  15 +++
>  5 files changed, 249 insertions(+), 110 deletions(-)
> 
> diff --git a/fs/super.c b/fs/super.c
> index d78b9847e6cb..8ff18af7703f 100644
> --- a/fs/super.c
> +++ b/fs/super.c
> @@ -195,7 +195,8 @@ static struct super_block *alloc_super(struct file_system_type *type, int flags)
>  	for (i = 0; i < SB_FREEZE_LEVELS; i++) {
>  		if (__percpu_init_rwsem(&s->s_writers.rw_sem[i],
>  					sb_writers_name[i],
> -					&type->s_writers_key[i]))
> +					&type->s_writers_key[i],
> +					PERCPU_RWSEM_READER))
>  			goto fail;
>  	}
>  	init_waitqueue_head(&s->s_writers.wait_unfrozen);
> diff --git a/include/linux/percpu-rwsem.h b/include/linux/percpu-rwsem.h
> index c2fa3ecb0dce..5e1c2b029e3a 100644
> --- a/include/linux/percpu-rwsem.h
> +++ b/include/linux/percpu-rwsem.h
> @@ -10,29 +10,107 @@
> 
>  struct percpu_rw_semaphore {
>  	struct rcu_sync		rss;
> -	unsigned int __percpu	*fast_read_ctr;
> +	unsigned int __percpu	*refcount;
>  	struct rw_semaphore	rw_sem;
> -	atomic_t		slow_read_ctr;
> -	wait_queue_head_t	write_waitq;
> +	wait_queue_head_t	writer;
> +	int			state;
>  };
> 
> -extern void percpu_down_read(struct percpu_rw_semaphore *);
> -extern int  percpu_down_read_trylock(struct percpu_rw_semaphore *);
> -extern void percpu_up_read(struct percpu_rw_semaphore *);
> +extern void __percpu_down_read(struct percpu_rw_semaphore *);
> +extern int  __percpu_down_read_trylock(struct percpu_rw_semaphore *);
> +extern void __percpu_up_read(struct percpu_rw_semaphore *);
> +
> +static inline void percpu_down_read(struct percpu_rw_semaphore *sem)
> +{
> +	might_sleep();
> +
> +	rwsem_acquire_read(&sem->rw_sem.dep_map, 0, 0, _RET_IP_);
> +
> +	preempt_disable();
> +	/*
> +	 * We are in an RCU-sched read-side critical section, so the writer
> +	 * cannot both change sem->state from readers_fast and start checking
> +	 * counters while we are here. So if we see !sem->state, we know that
> +	 * the writer won't be checking until we're past the preempt_enable()
> +	 * and that one the synchronize_sched() is done, the writer will see
> +	 * anything we did within this RCU-sched read-size critical section.
> +	 */
> +	__this_cpu_inc(*sem->refcount);
> +	if (unlikely(!rcu_sync_is_idle(&sem->rss)))
> +		__percpu_down_read(sem); /* Unconditional memory barrier */
> +	preempt_enable();
> +	/*
> +	 * The barrier() from preempt_enable() prevents the compiler from
> +	 * bleeding the critical section out.
> +	 */
> +}
> +
> +static inline int percpu_down_read_trylock(struct percpu_rw_semaphore *sem)
> +{
> +	int ret = 1;
> +
> +	preempt_disable();
> +	/*
> +	 * Same as in percpu_down_read().
> +	 */
> +	__this_cpu_inc(*sem->refcount);
> +	if (unlikely(!rcu_sync_is_idle(&sem->rss)))
> +		ret = __percpu_down_read_trylock(sem);
> +	preempt_enable();
> +	/*
> +	 * The barrier() from preempt_enable() prevents the compiler from
> +	 * bleeding the critical section out.
> +	 */
> +
> +	if (ret)
> +		rwsem_acquire_read(&sem->rw_sem.dep_map, 0, 1, _RET_IP_);
> +
> +	return ret;
> +}
> +
> +static inline void percpu_up_read(struct percpu_rw_semaphore *sem)
> +{
> +	/*
> +	 * The barrier() in preempt_disable() prevents the compiler from
> +	 * bleeding the critical section out.
> +	 */
> +	preempt_disable();
> +	/*
> +	 * Same as in percpu_down_read().
> +	 */
> +	if (likely(rcu_sync_is_idle(&sem->rss)))
> +		__this_cpu_dec(*sem->refcount);
> +	else
> +		__percpu_up_read(sem); /* Unconditional memory barrier */
> +	preempt_enable();
> +
> +	rwsem_release(&sem->rw_sem.dep_map, 1, _RET_IP_);
> +}
> 
>  extern void percpu_down_write(struct percpu_rw_semaphore *);
>  extern void percpu_up_write(struct percpu_rw_semaphore *);
> 
> +enum percpu_rwsem_bias { PERCPU_RWSEM_READER, PERCPU_RWSEM_WRITER };
> +
>  extern int __percpu_init_rwsem(struct percpu_rw_semaphore *,
> -				const char *, struct lock_class_key *);
> +				const char *, struct lock_class_key *,
> +				enum percpu_rwsem_bias bias);
> +
>  extern void percpu_free_rwsem(struct percpu_rw_semaphore *);
> 
> -#define percpu_init_rwsem(brw)	\
> +#define percpu_init_rwsem(sem)					\
>  ({								\
>  	static struct lock_class_key rwsem_key;			\
> -	__percpu_init_rwsem(brw, #brw, &rwsem_key);		\
> +	__percpu_init_rwsem(sem, #sem, &rwsem_key,		\
> +			    PERCPU_RWSEM_READER);		\
>  })
> 
> +#define percpu_init_rwsem_writer(sem)				\
> +({								\
> +	static struct lock_class_key rwsem_key;			\
> +	__percpu_init_rwsem(sem, #sem, &rwsem_key,i		\
> +			    PERCPU_RWSEM_WRITER);		\
> +})
> 
>  #define percpu_rwsem_is_held(sem) lockdep_is_held(&(sem)->rw_sem)
> 
> diff --git a/include/linux/rcu_sync.h b/include/linux/rcu_sync.h
> index a63a33e6196e..e556baaf785e 100644
> --- a/include/linux/rcu_sync.h
> +++ b/include/linux/rcu_sync.h
> @@ -26,7 +26,7 @@
>  #include <linux/wait.h>
>  #include <linux/rcupdate.h>
> 
> -enum rcu_sync_type { RCU_SYNC, RCU_SCHED_SYNC, RCU_BH_SYNC };
> +enum rcu_sync_type { RCU_SYNC, RCU_SCHED_SYNC, RCU_BH_SYNC, RCU_NONE };
> 
>  /* Structure to mediate between updaters and fastpath-using readers.  */
>  struct rcu_sync {
> diff --git a/kernel/locking/percpu-rwsem.c b/kernel/locking/percpu-rwsem.c
> index bec0b647f9cc..be37c7732b54 100644
> --- a/kernel/locking/percpu-rwsem.c
> +++ b/kernel/locking/percpu-rwsem.c
> @@ -8,152 +8,197 @@
>  #include <linux/sched.h>
>  #include <linux/errno.h>
> 
> -int __percpu_init_rwsem(struct percpu_rw_semaphore *brw,
> -			const char *name, struct lock_class_key *rwsem_key)
> +enum { readers_slow, readers_block };
> +
> +int __percpu_init_rwsem(struct percpu_rw_semaphore *sem,
> +			const char *name, struct lock_class_key *rwsem_key,
> +			enum percpu_rwsem_bias bias)
>  {
> -	brw->fast_read_ctr = alloc_percpu(int);
> -	if (unlikely(!brw->fast_read_ctr))
> +	sem->refcount = alloc_percpu(int);
> +	if (unlikely(!sem->refcount))
>  		return -ENOMEM;
> 
>  	/* ->rw_sem represents the whole percpu_rw_semaphore for lockdep */
> -	__init_rwsem(&brw->rw_sem, name, rwsem_key);
> -	rcu_sync_init(&brw->rss, RCU_SCHED_SYNC);
> -	atomic_set(&brw->slow_read_ctr, 0);
> -	init_waitqueue_head(&brw->write_waitq);
> +	rcu_sync_init(&sem->rss, bias == PERCPU_RWSEM_READER ? 
> +				 RCU_SCHED_SYNC : 

Whitespace complaint on prior two lines.

> +				 RCU_NONE);
> +	__init_rwsem(&sem->rw_sem, name, rwsem_key);
> +	init_waitqueue_head(&sem->writer);
> +	sem->state = readers_slow;
>  	return 0;
>  }
>  EXPORT_SYMBOL_GPL(__percpu_init_rwsem);
> 
> -void percpu_free_rwsem(struct percpu_rw_semaphore *brw)
> +void percpu_free_rwsem(struct percpu_rw_semaphore *sem)
>  {
>  	/*
>  	 * XXX: temporary kludge. The error path in alloc_super()
>  	 * assumes that percpu_free_rwsem() is safe after kzalloc().
>  	 */
> -	if (!brw->fast_read_ctr)
> +	if (!sem->refcount)
>  		return;
> 
> -	rcu_sync_dtor(&brw->rss);
> -	free_percpu(brw->fast_read_ctr);
> -	brw->fast_read_ctr = NULL; /* catch use after free bugs */
> +	rcu_sync_dtor(&sem->rss);
> +	free_percpu(sem->refcount);
> +	sem->refcount = NULL; /* catch use after free bugs */
>  }
>  EXPORT_SYMBOL_GPL(percpu_free_rwsem);
> 
> -/*
> - * This is the fast-path for down_read/up_read. If it succeeds we rely
> - * on the barriers provided by rcu_sync_enter/exit; see the comments in
> - * percpu_down_write() and percpu_up_write().
> - *
> - * If this helper fails the callers rely on the normal rw_semaphore and
> - * atomic_dec_and_test(), so in this case we have the necessary barriers.
> - */
> -static bool update_fast_ctr(struct percpu_rw_semaphore *brw, unsigned int val)
> +void __percpu_down_read(struct percpu_rw_semaphore *sem)
>  {
> -	bool success;
> +	/*
> +	 * Due to having preemption disabled the decrement happens on
> +	 * the same CPU as the increment, avoiding the
> +	 * increment-on-one-CPU-and-decrement-on-another problem.
> +	 *
> +	 * And yes, if the reader misses the writer's assignment of
> +	 * readers_block to sem->state, then the writer is
> +	 * guaranteed to see the reader's increment.  Conversely, any
> +	 * readers that increment their sem->refcount after the
> +	 * writer looks are guaranteed to see the readers_block value,
> +	 * which in turn means that they are guaranteed to immediately
> +	 * decrement their sem->refcount, so that it doesn't matter
> +	 * that the writer missed them.
> +	 */
> 
> -	preempt_disable();
> -	success = rcu_sync_is_idle(&brw->rss);
> -	if (likely(success))
> -		__this_cpu_add(*brw->fast_read_ctr, val);
> -	preempt_enable();
> +	smp_mb(); /* A matches D */
> 
> -	return success;
> -}
> +	/*
> +	 * If !readers_block the critical section starts here, matched by the
> +	 * release in percpu_up_write().
> +	 */
> +	if (likely(smp_load_acquire(&sem->state) != readers_block))
> +		return;
> 
> -/*
> - * Like the normal down_read() this is not recursive, the writer can
> - * come after the first percpu_down_read() and create the deadlock.
> - *
> - * Note: returns with lock_is_held(brw->rw_sem) == T for lockdep,
> - * percpu_up_read() does rwsem_release(). This pairs with the usage
> - * of ->rw_sem in percpu_down/up_write().
> - */
> -void percpu_down_read(struct percpu_rw_semaphore *brw)
> -{
> -	might_sleep();
> -	rwsem_acquire_read(&brw->rw_sem.dep_map, 0, 0, _RET_IP_);
> +	/*
> +	 * Per the above comment; we still have preemption disabled and
> +	 * will thus decrement on the same CPU as we incremented.
> +	 */
> +	__percpu_up_read(sem);
> 
> -	if (likely(update_fast_ctr(brw, +1)))
> -		return;
> +	/*
> +	 * We either call schedule() in the wait, or we'll fall through
> +	 * and reschedule on the preempt_enable() in percpu_down_read().
> +	 */
> +	preempt_enable_no_resched();
> +
> +	/*
> +	 * Avoid lockdep for the down/up_read() we already have them.
> +	 */
> +	__down_read(&sem->rw_sem);
> +	__this_cpu_inc(*sem->refcount);
> +	__up_read(&sem->rw_sem);
> 
> -	/* Avoid rwsem_acquire_read() and rwsem_release() */
> -	__down_read(&brw->rw_sem);
> -	atomic_inc(&brw->slow_read_ctr);
> -	__up_read(&brw->rw_sem);
> +	preempt_disable();
>  }
> -EXPORT_SYMBOL_GPL(percpu_down_read);
> +EXPORT_SYMBOL_GPL(__percpu_down_read);
> 
> -int percpu_down_read_trylock(struct percpu_rw_semaphore *brw)
> +int __percpu_down_read_trylock(struct percpu_rw_semaphore *sem)
>  {
> -	if (unlikely(!update_fast_ctr(brw, +1))) {
> -		if (!__down_read_trylock(&brw->rw_sem))
> -			return 0;
> -		atomic_inc(&brw->slow_read_ctr);
> -		__up_read(&brw->rw_sem);
> -	}
> -
> -	rwsem_acquire_read(&brw->rw_sem.dep_map, 0, 1, _RET_IP_);
> -	return 1;
> +	smp_mb(); /* A matches D */
> +
> +	if (likely(smp_load_acquire(&sem->state) != readers_block))
> +		return 1;
> +
> +	__percpu_up_read(sem);
> +	return 0;
>  }
> +EXPORT_SYMBOL_GPL(__percpu_down_read_trylock);
> 
> -void percpu_up_read(struct percpu_rw_semaphore *brw)
> +void __percpu_up_read(struct percpu_rw_semaphore *sem)
>  {
> -	rwsem_release(&brw->rw_sem.dep_map, 1, _RET_IP_);
> -
> -	if (likely(update_fast_ctr(brw, -1)))
> -		return;
> +	smp_mb(); /* B matches C */
> +	/*
> +	 * In other words, if they see our decrement (presumably to aggregate
> +	 * zero, as that is the only time it matters) they will also see our
> +	 * critical section.
> +	 */
> +	__this_cpu_dec(*sem->refcount);
> 
> -	/* false-positive is possible but harmless */
> -	if (atomic_dec_and_test(&brw->slow_read_ctr))
> -		wake_up_all(&brw->write_waitq);
> +	/* Prod writer to recheck readers_active */
> +	wake_up(&sem->writer);
>  }
> -EXPORT_SYMBOL_GPL(percpu_up_read);
> +EXPORT_SYMBOL_GPL(__percpu_up_read);
> +
> +#define per_cpu_sum(var)						\
> +({									\
> +	typeof(var) __sum = 0;						\
> +	int cpu;							\
> +	compiletime_assert_atomic_type(__sum);				\
> +	for_each_possible_cpu(cpu)					\
> +		__sum += per_cpu(var, cpu);				\
> +	__sum;								\
> +})
> 
> -static int clear_fast_ctr(struct percpu_rw_semaphore *brw)
> +/*
> + * Return true if the modular sum of the sem->refcount per-CPU variable is
> + * zero.  If this sum is zero, then it is stable due to the fact that if any
> + * newly arriving readers increment a given counter, they will immediately
> + * decrement that same counter.
> + */
> +static bool readers_active_check(struct percpu_rw_semaphore *sem)
>  {
> -	unsigned int sum = 0;
> -	int cpu;
> +	if (per_cpu_sum(*sem->refcount) != 0)
> +		return false;
> +
> +	/*
> +	 * If we observed the decrement; ensure we see the entire critical
> +	 * section.
> +	 */
> 
> -	for_each_possible_cpu(cpu) {
> -		sum += per_cpu(*brw->fast_read_ctr, cpu);
> -		per_cpu(*brw->fast_read_ctr, cpu) = 0;
> -	}
> +	smp_mb(); /* C matches B */
> 
> -	return sum;
> +	return true;
>  }
> 
> -void percpu_down_write(struct percpu_rw_semaphore *brw)
> +void percpu_down_write(struct percpu_rw_semaphore *sem)
>  {
> +	down_write(&sem->rw_sem);
> +
> +	/* Notify readers to take the slow path. */
> +	rcu_sync_enter(&sem->rss);
> +
>  	/*
> -	 * Make rcu_sync_is_idle() == F and thus disable the fast-path in
> -	 * percpu_down_read() and percpu_up_read(), and wait for gp pass.
> -	 *
> -	 * The latter synchronises us with the preceding readers which used
> -	 * the fast-past, so we can not miss the result of __this_cpu_add()
> -	 * or anything else inside their criticial sections.
> +	 * Notify new readers to block; up until now, and thus throughout the
> +	 * longish rcu_sync_enter() above, new readers could still come in.
>  	 */
> -	rcu_sync_enter(&brw->rss);
> +	sem->state = readers_block;
> 
> -	/* exclude other writers, and block the new readers completely */
> -	down_write(&brw->rw_sem);
> +	smp_mb(); /* D matches A */
> 
> -	/* nobody can use fast_read_ctr, move its sum into slow_read_ctr */
> -	atomic_add(clear_fast_ctr(brw), &brw->slow_read_ctr);
> +	/*
> +	 * If they don't see our writer of readers_block to sem->state,
> +	 * then we are guaranteed to see their sem->refcount increment, and
> +	 * therefore will wait for them.
> +	 */
> 
> -	/* wait for all readers to complete their percpu_up_read() */
> -	wait_event(brw->write_waitq, !atomic_read(&brw->slow_read_ctr));
> +	/* Wait for all now active readers to complete. */
> +	wait_event(sem->writer, readers_active_check(sem));
>  }
> -EXPORT_SYMBOL_GPL(percpu_down_write);
> 
> -void percpu_up_write(struct percpu_rw_semaphore *brw)
> +void percpu_up_write(struct percpu_rw_semaphore *sem)
>  {
> -	/* release the lock, but the readers can't use the fast-path */
> -	up_write(&brw->rw_sem);
>  	/*
> -	 * Enable the fast-path in percpu_down_read() and percpu_up_read()
> -	 * but only after another gp pass; this adds the necessary barrier
> -	 * to ensure the reader can't miss the changes done by us.
> +	 * Signal the writer is done, no fast path yet.
> +	 *
> +	 * One reason that we cannot just immediately flip to readers_fast is
> +	 * that new readers might fail to see the results of this writer's
> +	 * critical section.
> +	 *
> +	 * Therefore we force it through the slow path which guarantees an
> +	 * acquire and thereby guarantees the critical section's consistency.
> +	 */
> +	smp_store_release(&sem->state, readers_slow);
> +
> +	/*
> +	 * Release the write lock, this will allow readers back in the game.
> +	 */
> +	up_write(&sem->rw_sem);
> +
> +	/*
> +	 * Once this completes (at least one RCU grace period hence) the reader
> +	 * fast path will be available again. Safe to use outside the exclusive
> +	 * write lock because its counting.
>  	 */
> -	rcu_sync_exit(&brw->rss);
> +	rcu_sync_exit(&sem->rss);
>  }
> -EXPORT_SYMBOL_GPL(percpu_up_write);
> diff --git a/kernel/rcu/sync.c b/kernel/rcu/sync.c
> index be922c9f3d37..48055bf629af 100644
> --- a/kernel/rcu/sync.c
> +++ b/kernel/rcu/sync.c
> @@ -55,6 +55,7 @@ static const struct {
>  		.wait = rcu_barrier_bh,
>  		__INIT_HELD(rcu_read_lock_bh_held)
>  	},
> +	[RCU_NONE] = { },
>  };
> 
>  enum { GP_IDLE = 0, GP_PENDING, GP_PASSED };
> @@ -65,6 +66,9 @@ enum { CB_IDLE = 0, CB_PENDING, CB_REPLAY };
>  #ifdef CONFIG_PROVE_RCU
>  void rcu_sync_lockdep_assert(struct rcu_sync *rsp)
>  {
> +	if (rsp->gp_type == RCU_NONE)
> +		return;
> +
>  	RCU_LOCKDEP_WARN(!gp_ops[rsp->gp_type].held(),
>  			 "suspicious rcu_sync_is_idle() usage");
>  }
> @@ -80,6 +84,8 @@ void rcu_sync_init(struct rcu_sync *rsp, enum rcu_sync_type type)
>  	memset(rsp, 0, sizeof(*rsp));
>  	init_waitqueue_head(&rsp->gp_wait);
>  	rsp->gp_type = type;
> +	if (rsp->gp_type == RCU_NONE)
> +		rsp->gp_state = GP_PENDING; /* anything !0 */
>  }
> 
>  /**
> @@ -101,6 +107,9 @@ void rcu_sync_enter(struct rcu_sync *rsp)
>  {
>  	bool need_wait, need_sync;
> 
> +	if (rsp->gp_type == RCU_NONE)
> +		return;
> +
>  	spin_lock_irq(&rsp->rss_lock);
>  	need_wait = rsp->gp_count++;
>  	need_sync = rsp->gp_state == GP_IDLE;
> @@ -188,6 +197,9 @@ static void rcu_sync_func(struct rcu_head *rcu)
>   */
>  void rcu_sync_exit(struct rcu_sync *rsp)
>  {
> +	if (rsp->gp_type == RCU_NONE)
> +		return;
> +
>  	spin_lock_irq(&rsp->rss_lock);
>  	if (!--rsp->gp_count) {
>  		if (rsp->cb_state == CB_IDLE) {
> @@ -208,6 +220,9 @@ void rcu_sync_dtor(struct rcu_sync *rsp)
>  {
>  	int cb_state;
> 
> +	if (rsp->gp_type == RCU_NONE)
> +		return;
> +
>  	BUG_ON(rsp->gp_count);
> 
>  	spin_lock_irq(&rsp->rss_lock);
> 

[toc] | [prev] | [next] | [standalone]

#1443594

On Thu, Jul 14, 2016 at 09:23:55AM -0700, Paul E. McKenney wrote:
> Hmmm...  How does this handle the following sequence of events for
> the case where we are not biased towards the reader?
> 
> o	The per-CPU rwsem is set up with RCU_NONE and readers_slow
> 	(as opposed to readers_block).  The rcu_sync ->gp_state is
> 	GP_PENDING, which means that rcu_sync_is_idle() will always
> 	return true.

/false/, rcu_sync_is_idle() will always be false, to force us into the
slowpath.

> o	Task A on CPU 0 runs percpu_down_read() to completion, and remains
> 	within the critical section.  CPU 0's ->refcount is therefore 1.
> 
> o	Task B on CPU 1 does percpu_down_write(), which write-acquires
> 	->rw_sem, does rcu_sync_enter() (which is a no-op due to
> 	RCU_NONE), sets ->state to readers_block, and is just now going
> 	to wait for readers, which first invokes readers_active_check(),
> 	which starts summing the ->refcount for CPUs 0, 1, and 2,
> 	obtaining the value 1 so far.
> 
> o	Task C CPU 2 enters percpu_down_read(), disables preemption,
> 	increments CPU 2's ->refcount, sees rcu_sync_is_idle() return
> 	true (so skips __percpu_down_read()), enables preemption, and
> 	enters its critical section.

false, so does __percpu_down_read()

> 
> o	Task C migrates to CPU 3 and invokes percpu_up_read(), which
> 	disables preemption, sees rcu_sync_is_idle() return true, calls
> 	__this_cpu_dec() on CPU 3's ->refcount, and enables preemption.
> 	The value of CPU 3's ->refcount is thus (unsigned int)-1.

__percpu_up_read()

> 
> o	Task B on CPU 1 continues execution in readers_active_check(), with
> 	the full sum being zero.
> 
> So it looks to me like we have Task A as a writer at the same time that
> Task A is a reader, which would not be so good.
> 
> So what am I missing here?

for RCU_NONE we init rsp->gp_state to !0, which makes:

static inline rcu_sync_is_idle()'s

	return !rsp->gp_state (aka. rsp->gp_state == 0)

return false.

> And a couple of checkpatch nits below.  Yes, I had to apply the patch to
> figure out what it was doing.  ;-)

Yah, too much churn to read :-)

In any case, rest assured you've already gone over this part of the
patch several times. I repurposed an old percpu-rwsem optimization, Oleg
recognised it.

[toc] | [prev] | [next] | [standalone]

#1443610

On Thu, Jul 14, 2016 at 06:45:47PM +0200, Peter Zijlstra wrote:
> On Thu, Jul 14, 2016 at 09:23:55AM -0700, Paul E. McKenney wrote:
> > Hmmm...  How does this handle the following sequence of events for
> > the case where we are not biased towards the reader?
> > 
> > o	The per-CPU rwsem is set up with RCU_NONE and readers_slow
> > 	(as opposed to readers_block).  The rcu_sync ->gp_state is
> > 	GP_PENDING, which means that rcu_sync_is_idle() will always
> > 	return true.
> 
> /false/, rcu_sync_is_idle() will always be false, to force us into the
> slowpath.

Ah, got it...

> > o	Task A on CPU 0 runs percpu_down_read() to completion, and remains
> > 	within the critical section.  CPU 0's ->refcount is therefore 1.
> > 
> > o	Task B on CPU 1 does percpu_down_write(), which write-acquires
> > 	->rw_sem, does rcu_sync_enter() (which is a no-op due to
> > 	RCU_NONE), sets ->state to readers_block, and is just now going
> > 	to wait for readers, which first invokes readers_active_check(),
> > 	which starts summing the ->refcount for CPUs 0, 1, and 2,
> > 	obtaining the value 1 so far.
> > 
> > o	Task C CPU 2 enters percpu_down_read(), disables preemption,
> > 	increments CPU 2's ->refcount, sees rcu_sync_is_idle() return
> > 	true (so skips __percpu_down_read()), enables preemption, and
> > 	enters its critical section.
> 
> false, so does __percpu_down_read()
> 
> > 
> > o	Task C migrates to CPU 3 and invokes percpu_up_read(), which
> > 	disables preemption, sees rcu_sync_is_idle() return true, calls
> > 	__this_cpu_dec() on CPU 3's ->refcount, and enables preemption.
> > 	The value of CPU 3's ->refcount is thus (unsigned int)-1.
> 
> __percpu_up_read()
> 
> > 
> > o	Task B on CPU 1 continues execution in readers_active_check(), with
> > 	the full sum being zero.
> > 
> > So it looks to me like we have Task A as a writer at the same time that
> > Task A is a reader, which would not be so good.
> > 
> > So what am I missing here?
> 
> for RCU_NONE we init rsp->gp_state to !0, which makes:
> 
> static inline rcu_sync_is_idle()'s
> 
> 	return !rsp->gp_state (aka. rsp->gp_state == 0)
> 
> return false.
> 
> > And a couple of checkpatch nits below.  Yes, I had to apply the patch to
> > figure out what it was doing.  ;-)
> 
> Yah, too much churn to read :-)
> 
> In any case, rest assured you've already gone over this part of the
> patch several times. I repurposed an old percpu-rwsem optimization, Oleg
> recognised it.

OK, in that case I will hold off pending John Stultz's performance
checks.

							Thanx, Paul

[toc] | [prev] | [next] | [standalone]

#1443590

On Thu, Jul 14, 2016 at 6:18 AM, Peter Zijlstra <peterz@infradead.org> wrote:
> On Wed, Jul 13, 2016 at 10:51:02PM +0200, Peter Zijlstra wrote:
>> So, IIRC, the trade-off is a full memory barrier in read_lock and
>> read_unlock() vs sync_sched() in write.
>>
>> Full memory barriers are expensive and while the combined cost might
>> well exceed the cost of the sync_sched() it doesn't suffer the latency
>> issues.
>>
>> Not sure if we can frob the two in a single codebase, but I can have a
>> poke if Oleg or Paul doesn't beat me to it.
>
> OK, not too horrible if I say so myself :-)
>
> The below is a compile tested only first draft so far. I'll go give it
> some runtime next.

Unfortunately it didn't apply cleanly to the 4.4 based tree I'm
working with, so I had to manually apply the entirety of the
percpu-rwsem.c changes myself. Hopefully I didn't screw it up.

So running with this, I'm still seeing some pretty large delays. 80ms
peak, with lots of >20ms values as well.
So it doesn't seem to have the positive effect that Paul's change provided.

thanks
-john

[toc] | [prev] | [next] | [standalone]

#1443591

On Thu, Jul 14, 2016 at 09:43:40AM -0700, John Stultz wrote:
> On Thu, Jul 14, 2016 at 6:18 AM, Peter Zijlstra <peterz@infradead.org> wrote:
> > On Wed, Jul 13, 2016 at 10:51:02PM +0200, Peter Zijlstra wrote:
> >> So, IIRC, the trade-off is a full memory barrier in read_lock and
> >> read_unlock() vs sync_sched() in write.
> >>
> >> Full memory barriers are expensive and while the combined cost might
> >> well exceed the cost of the sync_sched() it doesn't suffer the latency
> >> issues.
> >>
> >> Not sure if we can frob the two in a single codebase, but I can have a
> >> poke if Oleg or Paul doesn't beat me to it.
> >
> > OK, not too horrible if I say so myself :-)
> >
> > The below is a compile tested only first draft so far. I'll go give it
> > some runtime next.
> 
> Unfortunately it didn't apply cleanly to the 4.4 based tree I'm
> working with, so I had to manually apply the entirety of the
> percpu-rwsem.c changes myself. Hopefully I didn't screw it up.
> 
> So running with this, I'm still seeing some pretty large delays. 80ms
> peak, with lots of >20ms values as well.
> So it doesn't seem to have the positive effect that Paul's change provided.

Well that is weird, did you put a tracepoint/printk in
synchronize_sched() to ensure we don't end up calling that?

[toc] | [prev] | [next] | [standalone]

#1443604

On Thu, Jul 14, 2016 at 9:49 AM, Peter Zijlstra <peterz@infradead.org> wrote:
> On Thu, Jul 14, 2016 at 09:43:40AM -0700, John Stultz wrote:
>> On Thu, Jul 14, 2016 at 6:18 AM, Peter Zijlstra <peterz@infradead.org> wrote:
>> > On Wed, Jul 13, 2016 at 10:51:02PM +0200, Peter Zijlstra wrote:
>> >> So, IIRC, the trade-off is a full memory barrier in read_lock and
>> >> read_unlock() vs sync_sched() in write.
>> >>
>> >> Full memory barriers are expensive and while the combined cost might
>> >> well exceed the cost of the sync_sched() it doesn't suffer the latency
>> >> issues.
>> >>
>> >> Not sure if we can frob the two in a single codebase, but I can have a
>> >> poke if Oleg or Paul doesn't beat me to it.
>> >
>> > OK, not too horrible if I say so myself :-)
>> >
>> > The below is a compile tested only first draft so far. I'll go give it
>> > some runtime next.
>>
>> Unfortunately it didn't apply cleanly to the 4.4 based tree I'm
>> working with, so I had to manually apply the entirety of the
>> percpu-rwsem.c changes myself. Hopefully I didn't screw it up.
>>
>> So running with this, I'm still seeing some pretty large delays. 80ms
>> peak, with lots of >20ms values as well.
>> So it doesn't seem to have the positive effect that Paul's change provided.
>
> Well that is weird, did you put a tracepoint/printk in
> synchronize_sched() to ensure we don't end up calling that?

So I am seeing synchronize_sched called, and its taking the
!rcu_gp_is_expedited path when I see the particularly bad latencies.

I wonder if I just mucked up applying the patch?

thanks
-john

[toc] | [prev] | [next] | [standalone]

#1443613

On 07/14, John Stultz wrote:
>
> So I am seeing synchronize_sched called, and its taking the
> !rcu_gp_is_expedited path when I see the particularly bad latencies.
>
> I wonder if I just mucked up applying the patch?

Probably yes...

Just in case, could you try the patch below? Of course, without other
optimizations from Peter, this change makes cgroup_threadgroup_rwsem
much worse than a plain rw_semaphore.

Oleg.

--- x/kernel/cgroup.c
+++ x/kernel/cgroup.c
@@ -5605,6 +5605,8 @@ int __init cgroup_init(void)
 	BUG_ON(cgroup_init_cftypes(NULL, cgroup_dfl_base_files));
 	BUG_ON(cgroup_init_cftypes(NULL, cgroup_legacy_base_files));
 
+	rcu_sync_enter(&cgroup_threadgroup_rwsem.rss);
+
 	get_user_ns(init_cgroup_ns.user_ns);
 
 	mutex_lock(&cgroup_mutex);

[toc] | [prev] | [next] | [standalone]

#1443624

On Thu, Jul 14, 2016 at 10:13 AM, Oleg Nesterov <oleg@redhat.com> wrote:
> On 07/14, John Stultz wrote:
>>
>> So I am seeing synchronize_sched called, and its taking the
>> !rcu_gp_is_expedited path when I see the particularly bad latencies.
>>
>> I wonder if I just mucked up applying the patch?
>
> Probably yes...

Hm. So I applied peterz patch to 4.7-rc7 and then diffed it to what I
had and it was just whitespace changes.

I've synched them up now, so I suspect my application isn't the issue
now. Just to be clear, I'm not supposed to be applying this on-top of
Paul's change, right?


> Just in case, could you try the patch below? Of course, without other
> optimizations from Peter, this change makes cgroup_threadgroup_rwsem
> much worse than a plain rw_semaphore.
>
> Oleg.
>
> --- x/kernel/cgroup.c
> +++ x/kernel/cgroup.c
> @@ -5605,6 +5605,8 @@ int __init cgroup_init(void)
>         BUG_ON(cgroup_init_cftypes(NULL, cgroup_dfl_base_files));
>         BUG_ON(cgroup_init_cftypes(NULL, cgroup_legacy_base_files));
>
> +       rcu_sync_enter(&cgroup_threadgroup_rwsem.rss);
> +


So adding this does make a huge difference ontop of Peter's patch. I'm
seeing sub 200us values for everything. The biggest spike in my basic
testing has been 138us.

I'm also not seeing synchronize_sched being called nearly as often,
and it doesn't seem to be being called in cgroup_procs_write path.

thanks
-john

[toc] | [prev] | [next] | [standalone]

#1443642

On 07/14, John Stultz wrote:
>
> I'm not supposed to be applying this on-top of
> Paul's change, right?

Right, unless I am totally confused,

> > Just in case, could you try the patch below? Of course, without other
> > optimizations from Peter, this change makes cgroup_threadgroup_rwsem
> > much worse than a plain rw_semaphore.
> >
> > Oleg.
> >
> > --- x/kernel/cgroup.c
> > +++ x/kernel/cgroup.c
> > @@ -5605,6 +5605,8 @@ int __init cgroup_init(void)
> >         BUG_ON(cgroup_init_cftypes(NULL, cgroup_dfl_base_files));
> >         BUG_ON(cgroup_init_cftypes(NULL, cgroup_legacy_base_files));
> >
> > +       rcu_sync_enter(&cgroup_threadgroup_rwsem.rss);
> > +
> 
> 
> So adding this does make a huge difference ontop of Peter's patch.

Ah, sorry for confusion. I meant, you could try this one-liner without
any other changes.

But we will need the "slow mode optimization" part from Peter's patch
anyway, otherwise percpu_rw_semaphore simply makes no sense for
cgroup_threadgroup_rwsem.

Oleg.

[toc] | [prev] | [next] | [standalone]

#1443652

On Thu, Jul 14, 2016 at 10:41 AM, Oleg Nesterov <oleg@redhat.com> wrote:
> On 07/14, John Stultz wrote:
>>
>> I'm not supposed to be applying this on-top of
>> Paul's change, right?
>
> Right, unless I am totally confused,
>
>> > Just in case, could you try the patch below? Of course, without other
>> > optimizations from Peter, this change makes cgroup_threadgroup_rwsem
>> > much worse than a plain rw_semaphore.
>> >
>> > Oleg.
>> >
>> > --- x/kernel/cgroup.c
>> > +++ x/kernel/cgroup.c
>> > @@ -5605,6 +5605,8 @@ int __init cgroup_init(void)
>> >         BUG_ON(cgroup_init_cftypes(NULL, cgroup_dfl_base_files));
>> >         BUG_ON(cgroup_init_cftypes(NULL, cgroup_legacy_base_files));
>> >
>> > +       rcu_sync_enter(&cgroup_threadgroup_rwsem.rss);
>> > +
>>
>>
>> So adding this does make a huge difference ontop of Peter's patch.
>
> Ah, sorry for confusion. I meant, you could try this one-liner without
> any other changes.

So the one-liner without other changes helps at a similar level as
Paul's change. From some simple testing I've got a 3.5ms spike, but
otherwise the values are under 200us.

> But we will need the "slow mode optimization" part from Peter's patch
> anyway, otherwise percpu_rw_semaphore simply makes no sense for
> cgroup_threadgroup_rwsem.

Yea, with Peter's patch it is further improved.

thanks
-john

[toc] | [prev] | [next] | [standalone]

#1443657

On 07/14, Peter Zijlstra wrote:
>
> The below is a compile tested only first draft so far. I'll go give it
> some runtime next.

So I will wait for the new version, but at first glance this matches the
code I already reviewed in the past (at least, tried hard to review ;)
and it looks correct.

Just a couple of almost cosmetic nits, feel free to ignore.

> --- a/include/linux/percpu-rwsem.h
> +++ b/include/linux/percpu-rwsem.h
> @@ -10,29 +10,107 @@
>  
>  struct percpu_rw_semaphore {
>  	struct rcu_sync		rss;
> -	unsigned int __percpu	*fast_read_ctr;
> +	unsigned int __percpu	*refcount;
>  	struct rw_semaphore	rw_sem;
> -	atomic_t		slow_read_ctr;
> -	wait_queue_head_t	write_waitq;
> +	wait_queue_head_t	writer;
> +	int			state;
        ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

I think that this "int state" and "enum { readers_slow, readers_block }"
just add a bit of complication/confusion.

All we need is the simple "bool readers_block" in percpu_rw_semaphore,
no?

> +void percpu_down_write(struct percpu_rw_semaphore *sem)
>  {
> +	down_write(&sem->rw_sem);
> +
> +	/* Notify readers to take the slow path. */
> +	rcu_sync_enter(&sem->rss);

I'd suggest to call rcu_sync_enter() before down_write(). This can help
when we wait for another writer which holds this lock.

Oleg.

[toc] | [prev] | [next] | [standalone]

#1443678

On Thu, Jul 14, 2016 at 08:09:52PM +0200, Oleg Nesterov wrote:
> On 07/14, Peter Zijlstra wrote:
> >
> > The below is a compile tested only first draft so far. I'll go give it
> > some runtime next.
> 
> So I will wait for the new version, but at first glance this matches the
> code I already reviewed in the past (at least, tried hard to review ;)
> and it looks correct.
> 
> Just a couple of almost cosmetic nits, feel free to ignore.

Drat, I just mailed out the patches.. I can do a second version later.

> > --- a/include/linux/percpu-rwsem.h
> > +++ b/include/linux/percpu-rwsem.h
> > @@ -10,29 +10,107 @@
> >  
> >  struct percpu_rw_semaphore {
> >  	struct rcu_sync		rss;
> > -	unsigned int __percpu	*fast_read_ctr;
> > +	unsigned int __percpu	*refcount;
> >  	struct rw_semaphore	rw_sem;
> > -	atomic_t		slow_read_ctr;
> > -	wait_queue_head_t	write_waitq;
> > +	wait_queue_head_t	writer;
> > +	int			state;
>         ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
> 
> I think that this "int state" and "enum { readers_slow, readers_block }"
> just add a bit of complication/confusion.
> 
> All we need is the simple "bool readers_block" in percpu_rw_semaphore,
> no?

So I detest bool in structures because sizeof(bool) isn't defined.
Obviously an implementation needs to pick a size, but this is typically
architecture ABIs, so sizes can differ between architectures.

But yes, I suppose "int readers_block" will do just fine.

IIRC, earlier version had more states, but that all went away.

> > +void percpu_down_write(struct percpu_rw_semaphore *sem)
> >  {
> > +	down_write(&sem->rw_sem);
> > +
> > +	/* Notify readers to take the slow path. */
> > +	rcu_sync_enter(&sem->rss);
> 
> I'd suggest to call rcu_sync_enter() before down_write(). This can help
> when we wait for another writer which holds this lock.

Hurm, I think I figured that might have issues, but I cannot seem to
think of any just now :-), yes can do.

[toc] | [prev] | [next] | [standalone]

#1443720

On Thu, Jul 14, 2016 at 08:36:09PM +0200, Peter Zijlstra wrote:
> On Thu, Jul 14, 2016 at 08:09:52PM +0200, Oleg Nesterov wrote:
> > On 07/14, Peter Zijlstra wrote:
> > >
> > > The below is a compile tested only first draft so far. I'll go give it
> > > some runtime next.
> > 
> > So I will wait for the new version, but at first glance this matches the
> > code I already reviewed in the past (at least, tried hard to review ;)
> > and it looks correct.
> > 
> > Just a couple of almost cosmetic nits, feel free to ignore.
> 
> Drat, I just mailed out the patches.. I can do a second version later.

How about so on top of what I sent?


--- a/include/linux/percpu-rwsem.h
+++ b/include/linux/percpu-rwsem.h
@@ -13,11 +13,10 @@ struct percpu_rw_semaphore {
 	unsigned int __percpu	*read_count;
 	struct rw_semaphore	rw_sem;
 	wait_queue_head_t	writer;
-	int			state;
+	int			readers_block;
 };
 
-extern void __percpu_down_read(struct percpu_rw_semaphore *);
-extern int  __percpu_down_read_trylock(struct percpu_rw_semaphore *);
+extern int __percpu_down_read(struct percpu_rw_semaphore *, int);
 extern void __percpu_up_read(struct percpu_rw_semaphore *);
 
 static inline void percpu_down_read(struct percpu_rw_semaphore *sem)
@@ -37,7 +36,7 @@ static inline void percpu_down_read(stru
 	 */
 	__this_cpu_inc(*sem->read_count);
 	if (unlikely(!rcu_sync_is_idle(&sem->rss)))
-		__percpu_down_read(sem); /* Unconditional memory barrier */
+		__percpu_down_read(sem, false); /* Unconditional memory barrier */
 	preempt_enable();
 	/*
 	 * The barrier() from preempt_enable() prevents the compiler from
@@ -55,7 +54,7 @@ static inline int percpu_down_read_trylo
 	 */
 	__this_cpu_inc(*sem->read_count);
 	if (unlikely(!rcu_sync_is_idle(&sem->rss)))
-		ret = __percpu_down_read_trylock(sem);
+		ret = __percpu_down_read(sem, true); /* Unconditional memory barrier */
 	preempt_enable();
 	/*
 	 * The barrier() from preempt_enable() prevents the compiler from
--- a/kernel/locking/percpu-rwsem.c
+++ b/kernel/locking/percpu-rwsem.c
@@ -8,8 +8,6 @@
 #include <linux/sched.h>
 #include <linux/errno.h>
 
-enum { readers_slow, readers_block };
-
 int __percpu_init_rwsem(struct percpu_rw_semaphore *sem,
 			const char *name, struct lock_class_key *rwsem_key)
 {
@@ -21,7 +19,7 @@ int __percpu_init_rwsem(struct percpu_rw
 	rcu_sync_init(&sem->rss, RCU_SCHED_SYNC);
 	__init_rwsem(&sem->rw_sem, name, rwsem_key);
 	init_waitqueue_head(&sem->writer);
-	sem->state = readers_slow;
+	sem->readers_block = 0;
 	return 0;
 }
 EXPORT_SYMBOL_GPL(__percpu_init_rwsem);
@@ -41,21 +39,21 @@ void percpu_free_rwsem(struct percpu_rw_
 }
 EXPORT_SYMBOL_GPL(percpu_free_rwsem);
 
-void __percpu_down_read(struct percpu_rw_semaphore *sem)
+int __percpu_down_read(struct percpu_rw_semaphore *sem, int try)
 {
 	/*
 	 * Due to having preemption disabled the decrement happens on
 	 * the same CPU as the increment, avoiding the
 	 * increment-on-one-CPU-and-decrement-on-another problem.
 	 *
-	 * And yes, if the reader misses the writer's assignment of
-	 * readers_block to sem->state, then the writer is
-	 * guaranteed to see the reader's increment.  Conversely, any
-	 * readers that increment their sem->read_count after the
-	 * writer looks are guaranteed to see the readers_block value,
+	 * If the reader misses the writer's assignment of readers_block, then
+	 * the writer is guaranteed to see the reader's increment.
+	 *
+	 * Conversely, any readers that increment their sem->read_count after
+	 * the writer looks are guaranteed to see the readers_block value,
 	 * which in turn means that they are guaranteed to immediately
-	 * decrement their sem->read_count, so that it doesn't matter
-	 * that the writer missed them.
+	 * decrement their sem->read_count, so that it doesn't matter that the
+	 * writer missed them.
 	 */
 
 	smp_mb(); /* A matches D */
@@ -64,8 +62,8 @@ void __percpu_down_read(struct percpu_rw
 	 * If !readers_block the critical section starts here, matched by the
 	 * release in percpu_up_write().
 	 */
-	if (likely(smp_load_acquire(&sem->state) != readers_block))
-		return;
+	if (likely(!smp_load_acquire(&sem->readers_block)))
+		return 1;
 
 	/*
 	 * Per the above comment; we still have preemption disabled and
@@ -73,6 +71,9 @@ void __percpu_down_read(struct percpu_rw
 	 */
 	__percpu_up_read(sem);
 
+	if (try)
+		return 0;
+
 	/*
 	 * We either call schedule() in the wait, or we'll fall through
 	 * and reschedule on the preempt_enable() in percpu_down_read().
@@ -87,21 +88,10 @@ void __percpu_down_read(struct percpu_rw
 	__up_read(&sem->rw_sem);
 
 	preempt_disable();
+	return 1;
 }
 EXPORT_SYMBOL_GPL(__percpu_down_read);
 
-int __percpu_down_read_trylock(struct percpu_rw_semaphore *sem)
-{
-	smp_mb(); /* A matches D */
-
-	if (likely(smp_load_acquire(&sem->state) != readers_block))
-		return 1;
-
-	__percpu_up_read(sem);
-	return 0;
-}
-EXPORT_SYMBOL_GPL(__percpu_down_read_trylock);
-
 void __percpu_up_read(struct percpu_rw_semaphore *sem)
 {
 	smp_mb(); /* B matches C */
@@ -150,23 +140,23 @@ static bool readers_active_check(struct
 
 void percpu_down_write(struct percpu_rw_semaphore *sem)
 {
-	down_write(&sem->rw_sem);
-
 	/* Notify readers to take the slow path. */
 	rcu_sync_enter(&sem->rss);
 
+	down_write(&sem->rw_sem);
+
 	/*
 	 * Notify new readers to block; up until now, and thus throughout the
 	 * longish rcu_sync_enter() above, new readers could still come in.
 	 */
-	WRITE_ONCE(sem->state, readers_block);
+	WRITE_ONCE(sem->readers_block, 1);
 
 	smp_mb(); /* D matches A */
 
 	/*
-	 * If they don't see our writer of readers_block to sem->state,
-	 * then we are guaranteed to see their sem->read_count increment, and
-	 * therefore will wait for them.
+	 * If they don't see our writer of readers_block, then we are
+	 * guaranteed to see their sem->read_count increment, and therefore
+	 * will wait for them.
 	 */
 
 	/* Wait for all now active readers to complete. */
@@ -186,7 +176,7 @@ void percpu_up_write(struct percpu_rw_se
 	 * Therefore we force it through the slow path which guarantees an
 	 * acquire and thereby guarantees the critical section's consistency.
 	 */
-	smp_store_release(&sem->state, readers_slow);
+	smp_store_release(&sem->readers_block, 0);
 
 	/*
 	 * Release the write lock, this will allow readers back in the game.
@@ -194,9 +184,9 @@ void percpu_up_write(struct percpu_rw_se
 	up_write(&sem->rw_sem);
 
 	/*
-	 * Once this completes (at least one RCU grace period hence) the reader
-	 * fast path will be available again. Safe to use outside the exclusive
-	 * write lock because its counting.
+	 * Once this completes (at least one RCU-sched grace period hence) the
+	 * reader fast path will be available again. Safe to use outside the
+	 * exclusive write lock because its counting.
 	 */
 	rcu_sync_exit(&sem->rss);
 }

[toc] | [prev] | [next] | [standalone]

#1442851

On Wed, Jul 13, 2016 at 1:39 PM, Tejun Heo <tj@kernel.org> wrote:
> Hello,
>
> On Wed, Jul 13, 2016 at 10:26:57PM +0200, Peter Zijlstra wrote:
>> > So, it's a percpu rwsem issue then.  I haven't really followed the
>> > perpcpu rwsem changes closely.  Oleg, are multi-milisec delay expected
>> > on down write expected with the current implementation of
>> > percpu_rwsem?
>>
>> There is a synchronize_sched() in there, so sorta. That thing is heavily
>> geared towards readers, as is the only 'sane' choice for global locks.
>
> It used to use the expedited variant until 001dac627ff3
> ("locking/percpu-rwsem: Make use of the rcu_sync infrastructure"), so
> it might have been okay before then.

Just as a datapoint, just reverting 001dac627ff3
("locking/percpu-rwsem: Make use of the rcu_sync infrastructure"),
doesn't seem to help as much as your prior patch going to a normal
rwsem.

I'm consistently seeing values in the 100-550us range (with occasional
spikes around 6ms), with a max value at 36ms.

thanks
-john

[toc] | [prev] | [next] | [standalone]

#1442842

On Wed, Jul 13, 2016 at 01:52:11PM -0700, Paul E. McKenney wrote:
> Not a particularly real-time-friendly fix, but certainly a good check
> on our various assumptions.

Not only RT, but also HPC and all the other RDMA userspace polling
freaks ;-)

But yes, it would confirm that this is indeed the issue.

[toc] | [prev] | [next] | [standalone]

#1442856

On Wed, Jul 13, 2016 at 10:57:12PM +0200, Peter Zijlstra wrote:
> On Wed, Jul 13, 2016 at 01:52:11PM -0700, Paul E. McKenney wrote:
> > Not a particularly real-time-friendly fix, but certainly a good check
> > on our various assumptions.
> 
> Not only RT, but also HPC and all the other RDMA userspace polling
> freaks ;-)

If Tejun's lglock-like approach doesn't pan out, then it would quite
easy to make a boot parameter that selected expedited vs. non-expedited
behavior.

							Thanx, Paul

[toc] | [prev] | [next] | [standalone]

#1442850

On Wed, Jul 13, 2016 at 10:26:57PM +0200, Peter Zijlstra wrote:
> On Wed, Jul 13, 2016 at 04:18:23PM -0400, Tejun Heo wrote:
> > Hello, John.
> > 
> > On Wed, Jul 13, 2016 at 01:13:11PM -0700, John Stultz wrote:
> > > On Wed, Jul 13, 2016 at 11:33 AM, Tejun Heo <tj@kernel.org> wrote:
> > > > On Wed, Jul 13, 2016 at 02:21:02PM -0400, Tejun Heo wrote:
> > > >> One interesting thing to try would be replacing it with a regular
> > > >> non-percpu rwsem and see how it behaves.  That should easily tell us
> > > >> whether this is from actual contention or artifacts from percpu_rwsem
> > > >> implementation.
> > > >
> > > > So, something like the following.  Can you please see whether this
> > > > makes any difference?
> > > 
> > > Yea. So this brings it down for me closer to what we're seeing with
> > > the Dmitry's patch reverting the two problematic commits, usually
> > > 10-50us with one early spike at 18ms.
> > 
> > So, it's a percpu rwsem issue then.  I haven't really followed the
> > perpcpu rwsem changes closely.  Oleg, are multi-milisec delay expected
> > on down write expected with the current implementation of
> > percpu_rwsem?
> 
> There is a synchronize_sched() in there, so sorta. That thing is heavily
> geared towards readers, as is the only 'sane' choice for global locks.

Then one diagnostic step to take would be to replace that
synchronize_sched() with synchronize_sched_expedited(), and see if that
gets rid of the delays.

Not a particularly real-time-friendly fix, but certainly a good check
on our various assumptions.

							Thanx, Paul

------------------------------------------------------------------------

diff --git a/kernel/rcu/sync.c b/kernel/rcu/sync.c
index be922c9f3d37..211acddc7e21 100644
--- a/kernel/rcu/sync.c
+++ b/kernel/rcu/sync.c
@@ -38,19 +38,19 @@ static const struct {
 #endif
 } gp_ops[] = {
 	[RCU_SYNC] = {
-		.sync = synchronize_rcu,
+		.sync = synchronize_rcu_expedited,
 		.call = call_rcu,
 		.wait = rcu_barrier,
 		__INIT_HELD(rcu_read_lock_held)
 	},
 	[RCU_SCHED_SYNC] = {
-		.sync = synchronize_sched,
+		.sync = synchronize_sched_expedited,
 		.call = call_rcu_sched,
 		.wait = rcu_barrier_sched,
 		__INIT_HELD(rcu_read_lock_sched_held)
 	},
 	[RCU_BH_SYNC] = {
-		.sync = synchronize_rcu_bh,
+		.sync = synchronize_rcu_bh_expedited,
 		.call = call_rcu_bh,
 		.wait = rcu_barrier_bh,
 		__INIT_HELD(rcu_read_lock_bh_held)

[toc] | [prev] | [next] | [standalone]

#1442860

On Wed, Jul 13, 2016 at 1:52 PM, Paul E. McKenney
<paulmck@linux.vnet.ibm.com> wrote:
> On Wed, Jul 13, 2016 at 10:26:57PM +0200, Peter Zijlstra wrote:
>> On Wed, Jul 13, 2016 at 04:18:23PM -0400, Tejun Heo wrote:
>> > Hello, John.
>> >
>> > On Wed, Jul 13, 2016 at 01:13:11PM -0700, John Stultz wrote:
>> > > On Wed, Jul 13, 2016 at 11:33 AM, Tejun Heo <tj@kernel.org> wrote:
>> > > > On Wed, Jul 13, 2016 at 02:21:02PM -0400, Tejun Heo wrote:
>> > > >> One interesting thing to try would be replacing it with a regular
>> > > >> non-percpu rwsem and see how it behaves.  That should easily tell us
>> > > >> whether this is from actual contention or artifacts from percpu_rwsem
>> > > >> implementation.
>> > > >
>> > > > So, something like the following.  Can you please see whether this
>> > > > makes any difference?
>> > >
>> > > Yea. So this brings it down for me closer to what we're seeing with
>> > > the Dmitry's patch reverting the two problematic commits, usually
>> > > 10-50us with one early spike at 18ms.
>> >
>> > So, it's a percpu rwsem issue then.  I haven't really followed the
>> > perpcpu rwsem changes closely.  Oleg, are multi-milisec delay expected
>> > on down write expected with the current implementation of
>> > percpu_rwsem?
>>
>> There is a synchronize_sched() in there, so sorta. That thing is heavily
>> geared towards readers, as is the only 'sane' choice for global locks.
>
> Then one diagnostic step to take would be to replace that
> synchronize_sched() with synchronize_sched_expedited(), and see if that
> gets rid of the delays.
>
> Not a particularly real-time-friendly fix, but certainly a good check
> on our various assumptions.

All delays <200 us, but one that is 3 ms.

>                                                         Thanx, Paul
>
> ------------------------------------------------------------------------
>
> diff --git a/kernel/rcu/sync.c b/kernel/rcu/sync.c
> index be922c9f3d37..211acddc7e21 100644
> --- a/kernel/rcu/sync.c
> +++ b/kernel/rcu/sync.c
> @@ -38,19 +38,19 @@ static const struct {
>  #endif
>  } gp_ops[] = {
>         [RCU_SYNC] = {
> -               .sync = synchronize_rcu,
> +               .sync = synchronize_rcu_expedited,
>                 .call = call_rcu,
>                 .wait = rcu_barrier,
>                 __INIT_HELD(rcu_read_lock_held)
>         },
>         [RCU_SCHED_SYNC] = {
> -               .sync = synchronize_sched,
> +               .sync = synchronize_sched_expedited,
>                 .call = call_rcu_sched,
>                 .wait = rcu_barrier_sched,
>                 __INIT_HELD(rcu_read_lock_sched_held)
>         },
>         [RCU_BH_SYNC] = {
> -               .sync = synchronize_rcu_bh,
> +               .sync = synchronize_rcu_bh_expedited,
>                 .call = call_rcu_bh,
>                 .wait = rcu_barrier_bh,
>                 __INIT_HELD(rcu_read_lock_bh_held)
>

[toc] | [prev] | [next] | [standalone]

Page 3 of 4 — ← Prev page 1 2 [3] 4  Next page →

Back to top | Article view | linux.kernel

csiph-web