linux-kernel - Re: [PATCH tip/core/rcu 16/27] rcu: Add comment documenting how rcu_seq

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]

Message-ID: <20180628051028.GB79165@joelaf.mtv.corp.google.com>
Date:   Wed, 27 Jun 2018 22:10:28 -0700
From:   Joel Fernandes <joel@...lfernandes.org>
To:     "Paul E. McKenney" <paulmck@...ux.vnet.ibm.com>
Cc:     Peter Zijlstra <peterz@...radead.org>,
        linux-kernel@...r.kernel.org, mingo@...nel.org,
        jiangshanlai@...il.com, dipankar@...ibm.com,
        akpm@...ux-foundation.org, mathieu.desnoyers@...icios.com,
        josh@...htriplett.org, tglx@...utronix.de, rostedt@...dmis.org,
        dhowells@...hat.com, edumazet@...gle.com, fweisbec@...il.com,
        oleg@...hat.com, kernel-team@...roid.com
Subject: Re: [PATCH tip/core/rcu 16/27] rcu: Add comment documenting how
 rcu_seq_snap works

On Wed, Jun 27, 2018 at 11:27:26AM -0700, Joel Fernandes wrote:
[..]
> > > > 	s = __ALIGN_MASK(s, RCU_SEQ_STATE_MASK);
> > > > 
> > > 
> > > I agree with Peter's suggestions for both the verbiage reduction in the
> > > comments in the header, as the new code he is proposing is more
> > > self-documenting. I believe I proposed a big comment just because the code
> > > wasn't self-documenting or obvious previously so needed an explanation.
> > > 
> > > How would you like to proceed? Let me know what you guys decide, I am really
> > > Ok with anything. If you guys agree, should I write a follow-up patch with
> > > Peter's suggestion that applies on top of this one?  Or do we want to drop
> > > this one in favor of Peter's suggestion?
> > 
> > Shortening the comment would be good, so please do that.

Paul,

Do you want to fold the below patch into the original one? Or do you prefer I
resent the original patch fixed up?

Following is the patch ontop of current 'dev' branch in your tree, with the
excessive comments removed.

Thanks to Peter for suggesting!

---8<-----------------------

From: "Joel Fernandes (Google)" <joel@...lfernandes.org>
Subject: [PATCH] rcu: Remove excessive commentary on rcu_seq_snap

There isn't a strong need to explain in excessive detail about
rcu_seq_snap with an example. Remove unnecessary and redundant comments.

Suggested-by: Peter Zijlstra <peterz@...radead.org>
Fixes: 9701945dd79e ("rcu: Add comment documenting how rcu_seq_snap works")
Signed-off-by: Joel Fernandes (Google) <joel@...lfernandes.org>
---
 kernel/rcu/rcu.h | 22 ----------------------
 1 file changed, 22 deletions(-)

diff --git a/kernel/rcu/rcu.h b/kernel/rcu/rcu.h
index 0af6ce6d8b66..4d04683c31b2 100644
--- a/kernel/rcu/rcu.h
+++ b/kernel/rcu/rcu.h
@@ -101,28 +101,6 @@ static inline void rcu_seq_end(unsigned long *sp)
  * current time. This value is the current grace-period number plus two to the
  * power of the number of low-order bits reserved for state, then rounded up to
  * the next value in which the state bits are all zero.
- *
- * In the current design, RCU_SEQ_STATE_MASK=3 and the least significant bit of
- * the seq is used to track if a GP is in progress or not. Given this, it is
- * sufficient if we add (6+1) and mask with ~3 to get the next GP. Let's see
- * why with an example:
- *
- * Say the current seq is 12 which is 0b1100 (GP is 3 and state bits are 0b00).
- * To get to the next GP number of 4, we have to add 0b100 to this (0x1 << 2)
- * to account for the shift due to 2 state bits. Now, if the current seq is
- * 13 (GP is 3 and state bits are 0b01), then it means the current grace period
- * is already in progress so the next GP that a future call back will be queued
- * to run at is GP+2 = 5, not 4. To account for the extra +1, we just overflow
- * the 2 lower bits by adding 0b11. In case the lower bit was set, the overflow
- * will cause the extra +1 to the GP, along with the usual +1 explained before.
- * This gives us GP+2. Finally we mask the lower to bits by ~0x3 in case the
- * overflow didn't occur. This masking is needed because in case RCU was idle
- * (no GP in progress so lower 2 bits are 0b00), then the overflow of the lower
- * 2 state bits wouldn't occur, so we mask to zero out those lower 2 bits.
- *
- * In other words, the next seq can be obtained by (0b11 + 0b100) & (~0b11)
- * which can be generalized to:
- * seq + (RCU_SEQ_STATE_MASK + (RCU_SEQ_STATE_MASK + 1)) & (~RCU_SEQ_STATE_MASK)
  */
 static inline unsigned long rcu_seq_snap(unsigned long *sp)
 {
-- 
2.18.0.rc2.346.g013aa6912e-goog