linux-kernel - Re: [PATCH RFC tip/core/rcu] SRCU rewrite

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Thu, 17 Nov 2016 23:55:07 +0800
From:   Lai Jiangshan <jiangshanlai@...il.com>
To:     Boqun Feng <boqun.feng@...il.com>
Cc:     "Paul E. McKenney" <paulmck@...ux.vnet.ibm.com>,
        LKML <linux-kernel@...r.kernel.org>,
        Ingo Molnar <mingo@...nel.org>, dipankar@...ibm.com,
        akpm@...ux-foundation.org,
        Mathieu Desnoyers <mathieu.desnoyers@...icios.com>,
        Josh Triplett <josh@...htriplett.org>,
        Thomas Gleixner <tglx@...utronix.de>,
        Peter Zijlstra <peterz@...radead.org>,
        Steven Rostedt <rostedt@...dmis.org>,
        David Howells <dhowells@...hat.com>,
        Eric Dumazet <edumazet@...gle.com>, dvhart@...ux.intel.com,
        Frédéric Weisbecker <fweisbec@...il.com>,
        oleg@...hat.com, pranith kumar <bobby.prani@...il.com>,
        ldr709@...il.com
Subject: Re: [PATCH RFC tip/core/rcu] SRCU rewrite

On Thu, Nov 17, 2016 at 10:45 PM, Boqun Feng <boqun.feng@...il.com> wrote:
> On Thu, Nov 17, 2016 at 06:38:29AM -0800, Paul E. McKenney wrote:
>> On Thu, Nov 17, 2016 at 05:49:57AM -0800, Paul E. McKenney wrote:
>> > On Thu, Nov 17, 2016 at 08:18:51PM +0800, Lai Jiangshan wrote:
>> > > On Tue, Nov 15, 2016 at 10:37 PM, Paul E. McKenney
>> > > <paulmck@...ux.vnet.ibm.com> wrote:
>> > > > On Tue, Nov 15, 2016 at 09:44:45AM +0800, Boqun Feng wrote:
>> > >
>> > > >>
>> > > >> __srcu_read_lock() used to be called with preemption disabled. I guess
>> > > >> the reason was because we have two percpu variables to increase. So with
>> > > >> only one percpu right, could we remove the preempt_{dis,en}able() in
>> > > >> srcu_read_lock() and use this_cpu_inc() here?
>> > > >
>> > > > Quite possibly...
>> > > >
>> > >
>> > > it will be nicer if it is removed.
>> > >
>> > > The reason for the preemption-disabled was also because we
>> > > have to disallow any preemption between the fetching of the idx
>> > > and the increasement. so that we have at most NR_CPUS worth
>> > > of readers using the old index that haven't incremented the counters.
>> > >
>> > > if we remove the preempt_{dis,en}able(). we must change the
>> > > "NR_CPUS" in the comment into ULONG_MAX/4. (I assume
>> > > one on-going reader needs at least need 4bytes at the stack). it is still safe.
>> > >
>> > > but we still need to think more if we want to remove the preempt_{dis,en}able().
>> >
>> > Good points!  Agreed, any change in the preemption needs careful thought
>> > and needs to be a separate patch.
>>
>> And one area needing special thought is the call to __srcu_read_lock()
>> and __srcu_read_unlock() in do_exit().
>>
>
> So before commit 49f5903b473c5, we don't have the read of ->completed in
> preemption disable section?
>
> And following "git blame", I found commit 7a6b55e7108b3 ;-)

Ouch, it shows 7a6b55e7108b3 at least has a bug in the comments about NR_CPUS.

we should focus on the total number of all active readers instead the number
of the readers using the old index that haven't incremented the counters.
the later is smaller than the prior one which is smaller than the ULONG_MAX/4
or even smaller. so that we can simplify the comments.

+        * Note that the sum of the ->lock_count[]s cannot increment enough
+        * times to overflow and end up equal the sum of the ->unlock_count[]s,
+        * even too much readers using the old index that haven't incremented
+        * ->lock_count[] yet, as long as there are at most ULONG_MAX/4
+        * readers at a time.  Therefore, the only way that the return values of
+        * the two calls to srcu_readers_(un)lock_idx() can be equal is if there
+        * are no active readers using this index.

>
> Regards,
> Boqun
>
>>                                                       Thanx, Paul
>>