Date: Mon, 21 Nov 2016 19:59:11 -0800
From: Davidlohr Bueso <dave@stgolabs.net>
To: Oleg Nesterov <oleg@redhat.com>
Cc: Peter Zijlstra <peterz@infradead.org>, mingo@kernel.org,
        john.stultz@linaro.org, dimitrysh@google.com,
        linux-kernel@vger.kernel.org, Davidlohr Bueso <dbueso@suse.de>
Subject: Re: [PATCH 3/3] locking/percpu-rwsem: Avoid unnecessary writer
 wakeups
Message-ID: <20161122035911.GA17027@linux-80c1.suse>
References: <1479495277-9075-1-git-send-email-dave@stgolabs.net>
 <1479495277-9075-4-git-send-email-dave@stgolabs.net>
 <20161121122343.GA635@redhat.com>
 <20161121122935.GD3092@twins.programming.kicks-ass.net>
 <20161121124722.GA1459@redhat.com>
 <20161121150722.GA7951@redhat.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii; format=flowed
Content-Disposition: inline
In-Reply-To: <20161121150722.GA7951@redhat.com>
User-Agent: Mutt/1.5.24 (2015-08-30)
Sender: linux-kernel-owner@vger.kernel.org
Content-Length: 1788
Lines: 48

On Mon, 21 Nov 2016, Oleg Nesterov wrote:

>On 11/21, Oleg Nesterov wrote:
>>
>> No, no, I meant that afaics both readers can see per_cpu_sum() != 0 and
>> thus the writer won't be woken up. Till the next down_read/up_read.
>>
>> Suppose that we have 2 CPU's, both counters == 1, both readers decrement.
>> its counter at the same time.
>>
>> 	READER_ON_CPU_0			READER_ON_CPU_1
>>
>> 	--ctr_0;			--ctr_1;
>>
>> 	if (ctr_0 + ctr_1)		if (ctr_0 + ctr_1)
>> 		wakeup();			wakeup();
>>
>> Why we can't miss a wakeup?

But the patch is really: if (!(ctr_0 + ctr_1)). wrt to stale values is this
like due to the data dependency we only see the real value of this_cpu ctr,
and no guarantee for the other cpus? If so I had not considered that scenario,
and yes we'd need stronger guarantees.

I'd have to wonder if other users of per_cpu_sum() would fall into a similar
trap. Hmm and each user seems to implement its own copy of the same thing.

>And in fact I am not sure this optimization makes sense... But it would be
>nice to avoid wake_up() when the writer sleeps in rcu_sync_enter(). Or this
>is the "slow mode" sem (cgroup_threadgroup_rwsem).

Why do you think using per_cpu_sum() does not make sense? As mentioned in the
changelog it optimizes for incoming readers while the writer is doing sync_enter
and getting the regular rwsem. What am I missing?

>
>I need to re-check, but what do you think about the change below?

While optimizing for multiple writers (rcu_sync_enter) is certainly valid
(at least considering the cgroups rwsem you mention), I think that my
heuristic covers the otherwise more common case. Could both optimizations
not work together?

Of course, the window of where readers_block == 1 is quite large, so there
can be a lot of false positives.

Thanks,
Davidlohr