[PATCH RFC 0/5] rcu doc updates for whatisRCU and checklist

[PATCH RFC 0/5] rcu doc updates for whatisRCU and checklist

[Joel Fernandes (Google)]

Hi Paul,

Here are this week's rcu doc updates based on combing through whatisRCU and
checklists. Hopefully you agree with them. I left several old _bh and _sched
API references as is, since I don't think its a good idea to remove them till
the APIs themselves are removed, however I did remove several of them as well
(like in the first patch in this series) since I feel its better to "encourage"
new users not to use the old API.

Also do you think it makes sense for us to write coccinelle patches to check if
folks use them on new patches? Btw, I am new to coccinelle but I'd love to give
it a try, it looks exciting. I remember you saying you wanted to do that, so
that's something else you could potentially offload to me as you see fit ;-)

Thank you very much!

Joel Fernandes (Google) (5):
  doc: rcu: Update core and full API in whatisRCU
  doc: rcu: Add more rationale for using rcu_read_lock_sched in
    checklist
  doc: rcu: Remove obsolete suggestion from checklist
  doc: rcu: Remove obsolete checklist item about synchronize_rcu usage
  doc: rcu: Encourage use of rcu_barrier in checklist

 Documentation/RCU/checklist.txt | 49 +++++++----------------------
 Documentation/RCU/whatisRCU.txt | 55 +++++++++++++++++----------------
 2 files changed, 39 insertions(+), 65 deletions(-)

-- 
2.19.0.605.g01d371f741-goog

[PATCH RFC 1/5] doc: rcu: Update core and full API in whatisRCU

[Joel Fernandes (Google)]

RCU consolidation effort causes the update side of the RCU API to be
consistent across all the 3 RCU flavors (normal, sched, bh). Update the
full API in the whatisRCU document accordingly so we are encouraging
folks to use the consolidated API than using the ones for the individual
flavors (which if I understand correcty, could be removed in the
future).

Also rcu_dereference is documented to be the same for all 3 mechanisms
(even before the consolidation), however its actually different - as
using the right rcu_dereference primitive (such as rcu_dereference_bh
for bh) is needed to make lock debugging work correctly. This update
also corrects that.

Also, add local_bh_disable() and local_bh_enable() as softirq
protection primitives and correct a grammar error in a quiz answer.

Signed-off-by: Joel Fernandes (Google) <joel@joelfernandes.org>
---
 Documentation/RCU/whatisRCU.txt | 55 +++++++++++++++++----------------
 1 file changed, 28 insertions(+), 27 deletions(-)

diff --git a/Documentation/RCU/whatisRCU.txt b/Documentation/RCU/whatisRCU.txt
index 86d82f7f3500..2e8d1c0a824f 100644
--- a/Documentation/RCU/whatisRCU.txt
+++ b/Documentation/RCU/whatisRCU.txt
@@ -322,28 +322,27 @@ to their callers and (2) call_rcu() callbacks may be invoked.  Efficient
 implementations of the RCU infrastructure make heavy use of batching in
 order to amortize their overhead over many uses of the corresponding APIs.
 
-There are no fewer than three RCU mechanisms in the Linux kernel; the
-diagram above shows the first one, which is by far the most commonly used.
-The rcu_dereference() and rcu_assign_pointer() primitives are used for
-all three mechanisms, but different defer and protect primitives are
-used as follows:
+There are atleast three flavors of RCU usage in the Linux kernel. The diagram
+above shows the most common one. On the updater side, the rcu_assign_pointer(),
+sychronize_rcu() and call_rcu() primitives used are the same for all three
+flavors. However for protection (on the reader side), the primitives used vary
+depending on the flavor:
 
-	Defer			Protect
+a.	rcu_read_lock() / rcu_read_unlock()
+	rcu_dereference()
 
-a.	synchronize_rcu()	rcu_read_lock() / rcu_read_unlock()
-	call_rcu()		rcu_dereference()
+b.	rcu_read_lock_bh() / rcu_read_unlock_bh()
+	local_bh_disable() / local_bh_enable()
+	rcu_dereference_bh()
 
-b.	synchronize_rcu_bh()	rcu_read_lock_bh() / rcu_read_unlock_bh()
-	call_rcu_bh()		rcu_dereference_bh()
+c.	rcu_read_lock_sched() / rcu_read_unlock_sched()
+	preempt_disable() / preempt_enable()
+	local_irq_save() / local_irq_restore()
+	hardirq enter / hardirq exit
+	NMI enter / NMI exit
+	rcu_dereference_sched()
 
-c.	synchronize_sched()	rcu_read_lock_sched() / rcu_read_unlock_sched()
-	call_rcu_sched()	preempt_disable() / preempt_enable()
-				local_irq_save() / local_irq_restore()
-				hardirq enter / hardirq exit
-				NMI enter / NMI exit
-				rcu_dereference_sched()
-
-These three mechanisms are used as follows:
+These three flavors are used as follows:
 
 a.	RCU applied to normal data structures.
 
@@ -867,18 +866,20 @@ RCU:	Critical sections	Grace period		Barrier
 
 bh:	Critical sections	Grace period		Barrier
 
-	rcu_read_lock_bh	call_rcu_bh		rcu_barrier_bh
-	rcu_read_unlock_bh	synchronize_rcu_bh
-	rcu_dereference_bh	synchronize_rcu_bh_expedited
+	rcu_read_lock_bh	call_rcu		rcu_barrier
+	rcu_read_unlock_bh	synchronize_rcu
+	[local_bh_disable]	synchronize_rcu_expedited
+	[and friends]
+	rcu_dereference_bh
 	rcu_dereference_bh_check
 	rcu_dereference_bh_protected
 	rcu_read_lock_bh_held
 
 sched:	Critical sections	Grace period		Barrier
 
-	rcu_read_lock_sched	synchronize_sched	rcu_barrier_sched
-	rcu_read_unlock_sched	call_rcu_sched
-	[preempt_disable]	synchronize_sched_expedited
+	rcu_read_lock_sched	call_rcu		rcu_barrier
+	rcu_read_unlock_sched	synchronize_rcu
+	[preempt_disable]	synchronize_rcu_expedited
 	[and friends]
 	rcu_read_lock_sched_notrace
 	rcu_read_unlock_sched_notrace
@@ -890,8 +891,8 @@ sched:	Critical sections	Grace period		Barrier
 
 SRCU:	Critical sections	Grace period		Barrier
 
-	srcu_read_lock		synchronize_srcu	srcu_barrier
-	srcu_read_unlock	call_srcu
+	srcu_read_lock		call_srcu		srcu_barrier
+	srcu_read_unlock	synchronize_srcu
 	srcu_dereference	synchronize_srcu_expedited
 	srcu_dereference_check
 	srcu_read_lock_held
@@ -1034,7 +1035,7 @@ Answer:		Just as PREEMPT_RT permits preemption of spinlock
 		spinlocks blocking while in RCU read-side critical
 		sections.
 
-		Why the apparent inconsistency?  Because it is it
+		Why the apparent inconsistency?  Because it is
 		possible to use priority boosting to keep the RCU
 		grace periods short if need be (for example, if running
 		short of memory).  In contrast, if blocking waiting
-- 
2.19.0.605.g01d371f741-goog

[PATCH RFC 2/5] doc: rcu: Add more rationale for using rcu_read_lock_sched in checklist

[Joel Fernandes (Google)]

It could be clarified better why rcu_read_lock_sched is better than
using preempt_disable, add the same.

Signed-off-by: Joel Fernandes (Google) <joel@joelfernandes.org>
---
 Documentation/RCU/checklist.txt | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/Documentation/RCU/checklist.txt b/Documentation/RCU/checklist.txt
index 49747717d905..8860ab2a897a 100644
--- a/Documentation/RCU/checklist.txt
+++ b/Documentation/RCU/checklist.txt
@@ -63,7 +63,7 @@ over a rather long period of time, but improvements are always welcome!
 	pointer must be covered by rcu_read_lock(), rcu_read_lock_bh(),
 	rcu_read_lock_sched(), or by the appropriate update-side lock.
 	Disabling of preemption can serve as rcu_read_lock_sched(), but
-	is less readable.
+	is less readable and prevents lockdep from detecting locking issues.
 
 	Letting RCU-protected pointers "leak" out of an RCU read-side
 	critical section is every bid as bad as letting them leak out
-- 
2.19.0.605.g01d371f741-goog

[PATCH RFC 3/5] doc: rcu: Remove obsolete suggestion from checklist

[Joel Fernandes (Google)]

call_rcu_bh is now implemented in terms of call_rcu, so the suggestion
to use a different API for speed benefits is not accurate anymore.
Update the document accordingly.

Signed-off-by: Joel Fernandes (Google) <joel@joelfernandes.org>
---
 Documentation/RCU/checklist.txt | 6 +-----
 1 file changed, 1 insertion(+), 5 deletions(-)

diff --git a/Documentation/RCU/checklist.txt b/Documentation/RCU/checklist.txt
index 8860ab2a897a..cc22ce49618d 100644
--- a/Documentation/RCU/checklist.txt
+++ b/Documentation/RCU/checklist.txt
@@ -285,11 +285,7 @@ over a rather long period of time, but improvements are always welcome!
 		here is that superuser already has lots of ways to crash
 		the machine.
 
-	d.	Use call_rcu_bh() rather than call_rcu(), in order to take
-		advantage of call_rcu_bh()'s faster grace periods.  (This
-		is only a partial solution, though.)
-
-	e.	Periodically invoke synchronize_rcu(), permitting a limited
+	d.	Periodically invoke synchronize_rcu(), permitting a limited
 		number of updates per grace period.
 
 	The same cautions apply to call_rcu_bh(), call_rcu_sched(),
-- 
2.19.0.605.g01d371f741-goog

[PATCH RFC 4/5] doc: rcu: Remove obsolete checklist item about synchronize_rcu usage

[Joel Fernandes (Google)]

Since the RCU mechanisms have been consolidated, this checklist item
seems no longer useful or relevant. Probably even a bit misleading. For
example, synchronize_rcu will now guarantee that all interrupt disabled
regions have finished executing. So lets remove this checklist item.

Signed-off-by: Joel Fernandes (Google) <joel@joelfernandes.org>
---
 Documentation/RCU/checklist.txt | 37 +++++++--------------------------
 1 file changed, 7 insertions(+), 30 deletions(-)

diff --git a/Documentation/RCU/checklist.txt b/Documentation/RCU/checklist.txt
index cc22ce49618d..b90ad1b0665a 100644
--- a/Documentation/RCU/checklist.txt
+++ b/Documentation/RCU/checklist.txt
@@ -320,37 +320,14 @@ over a rather long period of time, but improvements are always welcome!
 	will break Alpha, cause aggressive compilers to generate bad code,
 	and confuse people trying to read your code.
 
-11.	Note that synchronize_rcu() -only- guarantees to wait until
-	all currently executing rcu_read_lock()-protected RCU read-side
-	critical sections complete.  It does -not- necessarily guarantee
-	that all currently running interrupts, NMIs, preempt_disable()
-	code, or idle loops will complete.  Therefore, if your
-	read-side critical sections are protected by something other
-	than rcu_read_lock(), do -not- use synchronize_rcu().
-
-	Similarly, disabling preemption is not an acceptable substitute
-	for rcu_read_lock().  Code that attempts to use preemption
-	disabling where it should be using rcu_read_lock() will break
-	in CONFIG_PREEMPT=y kernel builds.
-
-	If you want to wait for interrupt handlers, NMI handlers, and
-	code under the influence of preempt_disable(), you instead
-	need to use synchronize_irq() or synchronize_sched().
-
-	This same limitation also applies to synchronize_rcu_bh()
-	and synchronize_srcu(), as well as to the asynchronous and
-	expedited forms of the three primitives, namely call_rcu(),
-	call_rcu_bh(), call_srcu(), synchronize_rcu_expedited(),
-	synchronize_rcu_bh_expedited(), and synchronize_srcu_expedited().
-
-12.	Any lock acquired by an RCU callback must be acquired elsewhere
+11.	Any lock acquired by an RCU callback must be acquired elsewhere
 	with softirq disabled, e.g., via spin_lock_irqsave(),
 	spin_lock_bh(), etc.  Failing to disable irq on a given
 	acquisition of that lock will result in deadlock as soon as
 	the RCU softirq handler happens to run your RCU callback while
 	interrupting that acquisition's critical section.
 
-13.	RCU callbacks can be and are executed in parallel.  In many cases,
+12.	RCU callbacks can be and are executed in parallel.  In many cases,
 	the callback code simply wrappers around kfree(), so that this
 	is not an issue (or, more accurately, to the extent that it is
 	an issue, the memory-allocator locking handles it).  However,
@@ -366,7 +343,7 @@ over a rather long period of time, but improvements are always welcome!
 	not the case, a self-spawning RCU callback would prevent the
 	victim CPU from ever going offline.)
 
-14.	Unlike other forms of RCU, it -is- permissible to block in an
+13.	Unlike other forms of RCU, it -is- permissible to block in an
 	SRCU read-side critical section (demarked by srcu_read_lock()
 	and srcu_read_unlock()), hence the "SRCU": "sleepable RCU".
 	Please note that if you don't need to sleep in read-side critical
@@ -410,7 +387,7 @@ over a rather long period of time, but improvements are always welcome!
 	Note that rcu_dereference() and rcu_assign_pointer() relate to
 	SRCU just as they do to other forms of RCU.
 
-15.	The whole point of call_rcu(), synchronize_rcu(), and friends
+14.	The whole point of call_rcu(), synchronize_rcu(), and friends
 	is to wait until all pre-existing readers have finished before
 	carrying out some otherwise-destructive operation.  It is
 	therefore critically important to -first- remove any path
@@ -422,13 +399,13 @@ over a rather long period of time, but improvements are always welcome!
 	is the caller's responsibility to guarantee that any subsequent
 	readers will execute safely.
 
-16.	The various RCU read-side primitives do -not- necessarily contain
+15.	The various RCU read-side primitives do -not- necessarily contain
 	memory barriers.  You should therefore plan for the CPU
 	and the compiler to freely reorder code into and out of RCU
 	read-side critical sections.  It is the responsibility of the
 	RCU update-side primitives to deal with this.
 
-17.	Use CONFIG_PROVE_LOCKING, CONFIG_DEBUG_OBJECTS_RCU_HEAD, and the
+16.	Use CONFIG_PROVE_LOCKING, CONFIG_DEBUG_OBJECTS_RCU_HEAD, and the
 	__rcu sparse checks to validate your RCU code.	These can help
 	find problems as follows:
 
@@ -451,7 +428,7 @@ over a rather long period of time, but improvements are always welcome!
 	These debugging aids can help you find problems that are
 	otherwise extremely difficult to spot.
 
-18.	If you register a callback using call_rcu(), call_rcu_bh(),
+17.	If you register a callback using call_rcu(), call_rcu_bh(),
 	call_rcu_sched(), or call_srcu(), and pass in a function defined
 	within a loadable module, then it in necessary to wait for
 	all pending callbacks to be invoked after the last invocation
-- 
2.19.0.605.g01d371f741-goog

[PATCH RFC 5/5] doc: rcu: Encourage use of rcu_barrier in checklist

[Joel Fernandes (Google)]

The checklist suggests rcu_barrier_bh for RCU-bh and similarly for
sched, however these APIs are now implemented as rcu_barrier itself due
to the RCU consolidation. It may also be removed in the future, so lets
correct the doc to encourage use of the rcu_barrier() API itself.
Similar changes are made in previous patches.

Signed-off-by: Joel Fernandes (Google) <joel@joelfernandes.org>
---
 Documentation/RCU/checklist.txt | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/Documentation/RCU/checklist.txt b/Documentation/RCU/checklist.txt
index b90ad1b0665a..6f469864d9f5 100644
--- a/Documentation/RCU/checklist.txt
+++ b/Documentation/RCU/checklist.txt
@@ -442,8 +442,8 @@ over a rather long period of time, but improvements are always welcome!
 	You instead need to use one of the barrier functions:
 
 	o	call_rcu() -> rcu_barrier()
-	o	call_rcu_bh() -> rcu_barrier_bh()
-	o	call_rcu_sched() -> rcu_barrier_sched()
+	o	call_rcu_bh() -> rcu_barrier()
+	o	call_rcu_sched() -> rcu_barrier()
 	o	call_srcu() -> srcu_barrier()
 
 	However, these barrier functions are absolutely -not- guaranteed
-- 
2.19.0.605.g01d371f741-goog

Re: [PATCH RFC 0/5] rcu doc updates for whatisRCU and checklist

[Theodore Y. Ts'o]

On Fri, Oct 05, 2018 at 04:18:09PM -0700, Joel Fernandes (Google) wrote:
> 
> Here are this week's rcu doc updates based on combing through whatisRCU and
> checklists. Hopefully you agree with them. I left several old _bh and _sched
> API references as is, since I don't think its a good idea to remove them till
> the APIs themselves are removed, however I did remove several of them as well
> (like in the first patch in this series) since I feel its better to "encourage"
> new users not to use the old API.

Hi Joel,

As it so happens, I just recently wrote my first RCU patch[1] (file
systems, especially on-disk data structures, generally tend not to be
good candidates for RCU semantics).

[1] http://patchwork.ozlabs.org/patch/979779/

So if you are working on improving RCU documentation, I thought I
would give two comments on the RCU docs from the perspective of a
developer trying to use RCU for the first time.

* whatisRCU is great, but one the example in Section 3 uses
  rcu_dereference_protected() without explaining it.  Given that using
  that function seems to be considered best practice, maybe a few more
  words there would be in order?  That function isn't mentioned in
  rcu.txt either, BTW.

* lockdep.txt *does* explain what rcu_dereference_protected() does,
  but it doesn't really describe lockdep_is_held().  You can mostly
  figure it out from context, but it wasn't obvious to me what locks
  it could be used against, and in the case of a rw_semaphore, whether
  it applied to shared as well as exclusive locks.  That's a lockdep
  abstraction, and not a RCU abstraction, but lockdep isn't
  particularly well documented, so I ended up spending 20-30 minutes
  or so looking at the lockdep implementation before I was sure it
  actually worked the way I thought it was going to.

Anyway, I was going to put submitting a patch to improve whatisRCU on
my (vastly over-long) TODO list, but when I saw your patch set, I
couldn't resist trying to see if I could fob it off on you.  If you
don't think that's fair (and it probably isn't really), just let me
know, and I'll put it back on my todo list.  :-)

Cheers,

					- Ted

Re: [PATCH RFC 0/5] rcu doc updates for whatisRCU and checklist

[Joel Fernandes]

On Fri, Oct 05, 2018 at 07:46:28PM -0400, Theodore Y. Ts'o wrote:
> On Fri, Oct 05, 2018 at 04:18:09PM -0700, Joel Fernandes (Google) wrote:
> > 
> > Here are this week's rcu doc updates based on combing through whatisRCU and
> > checklists. Hopefully you agree with them. I left several old _bh and _sched
> > API references as is, since I don't think its a good idea to remove them till
> > the APIs themselves are removed, however I did remove several of them as well
> > (like in the first patch in this series) since I feel its better to "encourage"
> > new users not to use the old API.
> 
> Hi Joel,

Hi Ted,

> 
> As it so happens, I just recently wrote my first RCU patch[1] (file
> systems, especially on-disk data structures, generally tend not to be
> good candidates for RCU semantics).
> 
> [1] http://patchwork.ozlabs.org/patch/979779/
> 
> So if you are working on improving RCU documentation, I thought I
> would give two comments on the RCU docs from the perspective of a
> developer trying to use RCU for the first time.
> 
> * whatisRCU is great, but one the example in Section 3 uses
>   rcu_dereference_protected() without explaining it.  Given that using
>   that function seems to be considered best practice, maybe a few more
>   words there would be in order?  That function isn't mentioned in
>   rcu.txt either, BTW.

I actually felt the same about rcu_dereference_protected while reading and
then looked at the comment above the implementation. The code comments are
pretty detailed, but I agree the example should mention a few words about it
since it uses it. I could look into improving that, no problem.

> * lockdep.txt *does* explain what rcu_dereference_protected() does,
>   but it doesn't really describe lockdep_is_held().  You can mostly
>   figure it out from context, but it wasn't obvious to me what locks
>   it could be used against, and in the case of a rw_semaphore, whether
>   it applied to shared as well as exclusive locks.  That's a lockdep
>   abstraction, and not a RCU abstraction, but lockdep isn't
>   particularly well documented, so I ended up spending 20-30 minutes
>   or so looking at the lockdep implementation before I was sure it
>   actually worked the way I thought it was going to.

Ok, makes sense to improve it. Since I haven't yet looked through lockdep.txt
yet (as a part of my broader documentation effort for the RCU consolidation),
I can take up improving that based on your suggestions since I have to look
into it anyway :). I'll look into that next week and CC you on this.

> Anyway, I was going to put submitting a patch to improve whatisRCU on
> my (vastly over-long) TODO list, but when I saw your patch set, I
> couldn't resist trying to see if I could fob it off on you.  If you
> don't think that's fair (and it probably isn't really), just let me
> know, and I'll put it back on my todo list.  :-)

Its Ok :) I'm happy to help! thanks for letting me know.

Best,

 - Joel

Re: [PATCH RFC 0/5] rcu doc updates for whatisRCU and checklist

[Paul E. McKenney]

On Fri, Oct 05, 2018 at 07:46:28PM -0400, Theodore Y. Ts'o wrote:
> On Fri, Oct 05, 2018 at 04:18:09PM -0700, Joel Fernandes (Google) wrote:
> > 
> > Here are this week's rcu doc updates based on combing through whatisRCU and
> > checklists. Hopefully you agree with them. I left several old _bh and _sched
> > API references as is, since I don't think its a good idea to remove them till
> > the APIs themselves are removed, however I did remove several of them as well
> > (like in the first patch in this series) since I feel its better to "encourage"
> > new users not to use the old API.
> 
> Hi Joel,
> 
> As it so happens, I just recently wrote my first RCU patch[1] (file
> systems, especially on-disk data structures, generally tend not to be
> good candidates for RCU semantics).
> 
> [1] http://patchwork.ozlabs.org/patch/979779/

Very cool!

One question...  In the following hunk:

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

@@ -5353,9 +5362,13 @@  static int ext4_remount(struct super_block *sb, int *flags, char *data)
 #ifdef CONFIG_QUOTA
 	sbi->s_jquota_fmt = old_opts.s_jquota_fmt;
 	for (i = 0; i < EXT4_MAXQUOTAS; i++) {
-		kfree(sbi->s_qf_names[i]);
-		sbi->s_qf_names[i] = old_opts.s_qf_names[i];
+		to_free[i] = rcu_dereference_protected(sbi->s_qf_names[i],
+						       &sb->s_umount);
+		rcu_assign_pointer(sbi->s_qf_names[i], old_opts.s_qf_names[i]);
 	}
+	for (i = 0; i < EXT4_MAXQUOTAS; i++)
+		kfree(to_free[i]);
+	synchronize_rcu();
 #endif
 	kfree(orig_data);
 	return err;

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

Shouldn't the synchronize_rcu() precede the loop doing the kfree()
calls?  Or am I missing something subtle?

Otherwise, looks good!  I was worried that seq_show_option() might
sleep, but it looks like it is just putting characters into an
array.  If there is lingering concern, CONFIG_PROVE_LOCKING will
usually catch that sort of thing.

							Thanx, Paul

> So if you are working on improving RCU documentation, I thought I
> would give two comments on the RCU docs from the perspective of a
> developer trying to use RCU for the first time.
> 
> * whatisRCU is great, but one the example in Section 3 uses
>   rcu_dereference_protected() without explaining it.  Given that using
>   that function seems to be considered best practice, maybe a few more
>   words there would be in order?  That function isn't mentioned in
>   rcu.txt either, BTW.
> 
> * lockdep.txt *does* explain what rcu_dereference_protected() does,
>   but it doesn't really describe lockdep_is_held().  You can mostly
>   figure it out from context, but it wasn't obvious to me what locks
>   it could be used against, and in the case of a rw_semaphore, whether
>   it applied to shared as well as exclusive locks.  That's a lockdep
>   abstraction, and not a RCU abstraction, but lockdep isn't
>   particularly well documented, so I ended up spending 20-30 minutes
>   or so looking at the lockdep implementation before I was sure it
>   actually worked the way I thought it was going to.
> 
> Anyway, I was going to put submitting a patch to improve whatisRCU on
> my (vastly over-long) TODO list, but when I saw your patch set, I
> couldn't resist trying to see if I could fob it off on you.  If you
> don't think that's fair (and it probably isn't really), just let me
> know, and I'll put it back on my todo list.  :-)
> 
> Cheers,
> 
> 					- Ted
> 

Re: [PATCH RFC 0/5] rcu doc updates for whatisRCU and checklist

[Joel Fernandes]

On Fri, Oct 05, 2018 at 08:45:40PM -0700, Paul E. McKenney wrote:
> On Fri, Oct 05, 2018 at 07:46:28PM -0400, Theodore Y. Ts'o wrote:
> > On Fri, Oct 05, 2018 at 04:18:09PM -0700, Joel Fernandes (Google) wrote:
> > > 
> > > Here are this week's rcu doc updates based on combing through whatisRCU and
> > > checklists. Hopefully you agree with them. I left several old _bh and _sched
> > > API references as is, since I don't think its a good idea to remove them till
> > > the APIs themselves are removed, however I did remove several of them as well
> > > (like in the first patch in this series) since I feel its better to "encourage"
> > > new users not to use the old API.
> > 
> > Hi Joel,
> > 
> > As it so happens, I just recently wrote my first RCU patch[1] (file
> > systems, especially on-disk data structures, generally tend not to be
> > good candidates for RCU semantics).
> > 
> > [1] http://patchwork.ozlabs.org/patch/979779/
> 
> Very cool!
> 
> One question...  In the following hunk:
> 
> ------------------------------------------------------------------------
> 
> @@ -5353,9 +5362,13 @@  static int ext4_remount(struct super_block *sb, int *flags, char *data)
>  #ifdef CONFIG_QUOTA
>  	sbi->s_jquota_fmt = old_opts.s_jquota_fmt;
>  	for (i = 0; i < EXT4_MAXQUOTAS; i++) {
> -		kfree(sbi->s_qf_names[i]);
> -		sbi->s_qf_names[i] = old_opts.s_qf_names[i];

Could you annotate this pointer (sbi->s_qf_names) with __rcu so it can be
checked by sparse for proper usage? Its also point #16 in the checklist.txt
RCU document. I enclosed a diff to do this below.

I also saw a bunch of places in super.c where the pointer isn't accessed from
an rcu read section or rcu_dereference, but it was a quick look so sorry if I
missed something. If its true, then are you planning to convert these to use
rcu_dereference and wrapped by an rcu_read_lock/unlock as well?

> +		to_free[i] = rcu_dereference_protected(sbi->s_qf_names[i],
> +						       &sb->s_umount);

Also should this be the following?
		to_free[i] = rcu_dereference_protected(sbi->s_qf_names[i],
					       lockdep_is_held(&sb->s_umount));

> +		rcu_assign_pointer(sbi->s_qf_names[i], old_opts.s_qf_names[i]);
>  	}
> +	for (i = 0; i < EXT4_MAXQUOTAS; i++)
> +		kfree(to_free[i]);
> +	synchronize_rcu();

I had same concern as Paul here about synchronize_rcu done before the kfree.

thanks,

 - Joel

diff --git a/fs/ext4/super.c b/fs/ext4/super.c
index 5863fd22e90b..eec1b3090d04 100644
--- a/fs/ext4/super.c
+++ b/fs/ext4/super.c
@@ -5083,7 +5083,7 @@ struct ext4_mount_options {
 	u32 s_min_batch_time, s_max_batch_time;
 #ifdef CONFIG_QUOTA
 	int s_jquota_fmt;
-	char *s_qf_names[EXT4_MAXQUOTAS];
+	char __rcu *s_qf_names[EXT4_MAXQUOTAS];
 #endif
 };
 

Re: [PATCH RFC 0/5] rcu doc updates for whatisRCU and checklist

[Theodore Y. Ts'o]

On Fri, Oct 05, 2018 at 09:40:57PM -0700, Joel Fernandes wrote:
> 
> Could you annotate this pointer (sbi->s_qf_names) with __rcu so it can be
> checked by sparse for proper usage? Its also point #16 in the checklist.txt
> RCU document. I enclosed a diff to do this below.

Sure.

> I also saw a bunch of places in super.c where the pointer isn't accessed from
> an rcu read section or rcu_dereference, but it was a quick look so sorry if I
> missed something. If its true, then are you planning to convert these to use
> rcu_dereference and wrapped by an rcu_read_lock/unlock as well?

These are places where we are holding s_umount, so there's no way
s_qf_names can change out from under us.  So the conversion should be
to use rcu_dereference_protected().

> > +		to_free[i] = rcu_dereference_protected(sbi->s_qf_names[i],
> > +						       &sb->s_umount);
> 
> Also should this be the following?
> 		to_free[i] = rcu_dereference_protected(sbi->s_qf_names[i],
> 					       lockdep_is_held(&sb->s_umount));

Yep, good catch, thanks.

						- Ted

Re: [PATCH RFC 0/5] rcu doc updates for whatisRCU and checklist

[Joel Fernandes]

On Fri, Oct 05, 2018 at 08:45:40PM -0700, Paul E. McKenney wrote:
> On Fri, Oct 05, 2018 at 07:46:28PM -0400, Theodore Y. Ts'o wrote:
> > On Fri, Oct 05, 2018 at 04:18:09PM -0700, Joel Fernandes (Google) wrote:
> > > 
> > > Here are this week's rcu doc updates based on combing through whatisRCU and
> > > checklists. Hopefully you agree with them. I left several old _bh and _sched
> > > API references as is, since I don't think its a good idea to remove them till
> > > the APIs themselves are removed, however I did remove several of them as well
> > > (like in the first patch in this series) since I feel its better to "encourage"
> > > new users not to use the old API.
> > 
> > Hi Joel,
> > 
> > As it so happens, I just recently wrote my first RCU patch[1] (file
> > systems, especially on-disk data structures, generally tend not to be
> > good candidates for RCU semantics).
> > 
> > [1] http://patchwork.ozlabs.org/patch/979779/
> 
> Very cool!
> 
> One question...  In the following hunk:
> 
> ------------------------------------------------------------------------
> 
> @@ -5353,9 +5362,13 @@  static int ext4_remount(struct super_block *sb, int *flags, char *data)
>  #ifdef CONFIG_QUOTA
>  	sbi->s_jquota_fmt = old_opts.s_jquota_fmt;
>  	for (i = 0; i < EXT4_MAXQUOTAS; i++) {
> -		kfree(sbi->s_qf_names[i]);
> -		sbi->s_qf_names[i] = old_opts.s_qf_names[i];
> +		to_free[i] = rcu_dereference_protected(sbi->s_qf_names[i],
> +						       &sb->s_umount);
> +		rcu_assign_pointer(sbi->s_qf_names[i], old_opts.s_qf_names[i]);
>  	}
> +	for (i = 0; i < EXT4_MAXQUOTAS; i++)
> +		kfree(to_free[i]);
> +	synchronize_rcu();
>  #endif
>  	kfree(orig_data);
>  	return err;
> 
> ------------------------------------------------------------------------
> 
> Shouldn't the synchronize_rcu() precede the loop doing the kfree()
> calls?  Or am I missing something subtle?
> 
> Otherwise, looks good!  I was worried that seq_show_option() might
> sleep, but it looks like it is just putting characters into an
> array.  If there is lingering concern, CONFIG_PROVE_LOCKING will
> usually catch that sort of thing.

Also I was wondering if the "if (sbi->s_qf_names[USRQUOTA])" in the patch
should be "if (rcu_dereference(sbi->s_qf_names[USRQUOTA]))". I don't think
the compiler could optimize the access in this case, bit IMO using the
rcu_dereference would serve to document that its an RCU protected pointer
anyway.

thanks,

 - Joel

Re: [PATCH RFC 0/5] rcu doc updates for whatisRCU and checklist

[Theodore Y. Ts'o]

On Fri, Oct 05, 2018 at 08:45:40PM -0700, Paul E. McKenney wrote:
> 
> Shouldn't the synchronize_rcu() precede the loop doing the kfree()
> calls?  Or am I missing something subtle?

No, that was a cut and paste error on my part.  I was removing the
rcu_read_unlock() before the kfree loop, and accidentally removed the
synchronize_rcu().  Then when I put it back, I put it back in the
right place.

The longer version:

I originally used rcu_read_lock() and rcu_read_unlock() around setting
up to_free[] --- since whatisRCU.txt didn't talk about
rcu_derefence_proctected(), just rcu_dereference() in Section 2: "What
is RCU's Core API?"   

Then when I looked at the example in Section 3, I was surprised when I
didn't see the rcu_read_[un]lock() on the updater side, and spent some
time trying to figure out how to use rcu_dereference_protected().

Then when I did the transumation from
rcu_read_lock/rcu_dereference_protected/rcu_read_unlock to
rcu_dereference_protected, I bobbled the location of
synchronize_rcu().

						- Ted

P.S.  Pedagogically, it might make sense to show an example that only
uses the RCU core API --- I assume using rcu_read_[un]lock() and
rcu_dereference() does work; it's just non-optimal, right?  --- and
then introduce the use of rcu_dereference_protected() afterwards.

Re: [PATCH RFC 0/5] rcu doc updates for whatisRCU and checklist

[Paul E. McKenney]

On Sat, Oct 06, 2018 at 01:34:46AM -0400, Theodore Y. Ts'o wrote:
> On Fri, Oct 05, 2018 at 08:45:40PM -0700, Paul E. McKenney wrote:
> > 
> > Shouldn't the synchronize_rcu() precede the loop doing the kfree()
> > calls?  Or am I missing something subtle?
> 
> No, that was a cut and paste error on my part.  I was removing the
> rcu_read_unlock() before the kfree loop, and accidentally removed the
> synchronize_rcu().  Then when I put it back, I put it back in the
> right place.

Been there, done that!  ;-)

> The longer version:
> 
> I originally used rcu_read_lock() and rcu_read_unlock() around setting
> up to_free[] --- since whatisRCU.txt didn't talk about
> rcu_derefence_proctected(), just rcu_dereference() in Section 2: "What
> is RCU's Core API?"   
> 
> Then when I looked at the example in Section 3, I was surprised when I
> didn't see the rcu_read_[un]lock() on the updater side, and spent some
> time trying to figure out how to use rcu_dereference_protected().
> 
> Then when I did the transumation from
> rcu_read_lock/rcu_dereference_protected/rcu_read_unlock to
> rcu_dereference_protected, I bobbled the location of
> synchronize_rcu().
> 
> 						- Ted
> 
> P.S.  Pedagogically, it might make sense to show an example that only
> uses the RCU core API --- I assume using rcu_read_[un]lock() and
> rcu_dereference() does work; it's just non-optimal, right?  --- and
> then introduce the use of rcu_dereference_protected() afterwards.

Yes, you can use rcu_dereference() on the update side and dispense
with rcu_dereference_protected(), but that will require you to add an
otherwise useless rcu_read_lock()/rcu_read_unlock() pair when accessing
the pointer on the update side.

Furthermore, if you are OK leaking memory rather than freeing it (which
is admittedly quite rare, but does sometimes happen), then yes, you
don't need call_rcu(), synchronize_rcu(), and friends.

So it is as you say, functional but non-optimal.

							Thanx, Paul

Re: [PATCH RFC 0/5] rcu doc updates for whatisRCU and checklist

[Paul E. McKenney]

On Fri, Oct 05, 2018 at 04:18:09PM -0700, Joel Fernandes (Google) wrote:
> Hi Paul,
> 
> Here are this week's rcu doc updates based on combing through whatisRCU and
> checklists. Hopefully you agree with them. I left several old _bh and _sched
> API references as is, since I don't think its a good idea to remove them till
> the APIs themselves are removed, however I did remove several of them as well
> (like in the first patch in this series) since I feel its better to "encourage"
> new users not to use the old API.
> 
> Also do you think it makes sense for us to write coccinelle patches to check if
> folks use them on new patches? Btw, I am new to coccinelle but I'd love to give
> it a try, it looks exciting. I remember you saying you wanted to do that, so
> that's something else you could potentially offload to me as you see fit ;-)
> 
> Thank you very much!

Good catches, applied and pushed, thank you!

I updated the commit log and a couple of the patches a bit, so could
you please double-check them?

							Thanx, Paul

> Joel Fernandes (Google) (5):
>   doc: rcu: Update core and full API in whatisRCU
>   doc: rcu: Add more rationale for using rcu_read_lock_sched in
>     checklist
>   doc: rcu: Remove obsolete suggestion from checklist
>   doc: rcu: Remove obsolete checklist item about synchronize_rcu usage
>   doc: rcu: Encourage use of rcu_barrier in checklist
> 
>  Documentation/RCU/checklist.txt | 49 +++++++----------------------
>  Documentation/RCU/whatisRCU.txt | 55 +++++++++++++++++----------------
>  2 files changed, 39 insertions(+), 65 deletions(-)
> 
> -- 
> 2.19.0.605.g01d371f741-goog
> 

Re: [PATCH RFC 0/5] rcu doc updates for whatisRCU and checklist

[Joel Fernandes]

On Fri, Oct 05, 2018 at 05:13:26PM -0700, Paul E. McKenney wrote:
> On Fri, Oct 05, 2018 at 04:18:09PM -0700, Joel Fernandes (Google) wrote:
> > Hi Paul,
> > 
> > Here are this week's rcu doc updates based on combing through whatisRCU and
> > checklists. Hopefully you agree with them. I left several old _bh and _sched
> > API references as is, since I don't think its a good idea to remove them till
> > the APIs themselves are removed, however I did remove several of them as well
> > (like in the first patch in this series) since I feel its better to "encourage"
> > new users not to use the old API.
> > 
> > Also do you think it makes sense for us to write coccinelle patches to check if
> > folks use them on new patches? Btw, I am new to coccinelle but I'd love to give
> > it a try, it looks exciting. I remember you saying you wanted to do that, so
> > that's something else you could potentially offload to me as you see fit ;-)
> > 
> > Thank you very much!
> 
> Good catches, applied and pushed, thank you!
> 
> I updated the commit log and a couple of the patches a bit, so could
> you please double-check them?

Yes I checked and it looks good, thanks!

 - Joel