Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
Date:   Wed, 6 Nov 2019 15:15:23 +0530
From:   Amol Grover <frextrite@gmail.com>
To:     Phong Tran <tranmanphong@gmail.com>
Cc:     paulmck@kernel.org, corbet@lwn.net, linux-doc@vger.kernel.org,
        jiangshanlai@gmail.com, josh@joshtriplett.org, rostedt@goodmis.org,
        linux-kernel@vger.kernel.org, rcu@vger.kernel.org,
        mathieu.desnoyers@efficios.com, joel@joelfernandes.org,
        linux-kernel-mentees@lists.linuxfoundation.org
Subject: Re: [PATCH] Doc: whatisRCU: Add more Markup
Message-ID: <20191106094523.GA1245@workstation-kernel-dev>
References: <20191105165938.GA10903@workstation>
 <20191105214234.17116-1-tranmanphong@gmail.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20191105214234.17116-1-tranmanphong@gmail.com>
User-Agent: Mutt/1.10.1 (2018-07-13)
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

On Wed, Nov 06, 2019 at 04:42:34AM +0700, Phong Tran wrote:
> o Adding more crossrefs.
> o Bold some words.
> o Add header levels.
> 
> Signed-off-by: Phong Tran <tranmanphong@gmail.com>
> ---
>  Documentation/RCU/whatisRCU.rst | 67 ++++++++++++++++++++-------------
>  1 file changed, 41 insertions(+), 26 deletions(-)
> 
> diff --git a/Documentation/RCU/whatisRCU.rst b/Documentation/RCU/whatisRCU.rst
> index ae40c8bcc56c..3e24e0155a91 100644
> --- a/Documentation/RCU/whatisRCU.rst
> +++ b/Documentation/RCU/whatisRCU.rst
> @@ -150,7 +150,7 @@ later.  See the kernel docbook documentation for more info, or look directly
>  at the function header comments.
>  
>  rcu_read_lock()
> -
> +^^^^^^^^^^^^^^^
>  	void rcu_read_lock(void);
>  
>  	Used by a reader to inform the reclaimer that the reader is
> @@ -164,7 +164,7 @@ rcu_read_lock()
>  	longer-term references to data structures.
>  
>  rcu_read_unlock()
> -
> +^^^^^^^^^^^^^^^^^
>  	void rcu_read_unlock(void);
>  
>  	Used by a reader to inform the reclaimer that the reader is
> @@ -172,13 +172,13 @@ rcu_read_unlock()
>  	read-side critical sections may be nested and/or overlapping.
>  
>  synchronize_rcu()
> -
> +^^^^^^^^^^^^^^^^^
>  	void synchronize_rcu(void);
>  
>  	Marks the end of updater code and the beginning of reclaimer
>  	code.  It does this by blocking until all pre-existing RCU
>  	read-side critical sections on all CPUs have completed.
> -	Note that synchronize_rcu() will -not- necessarily wait for
> +	Note that synchronize_rcu() will **not** necessarily wait for
>  	any subsequent RCU read-side critical sections to complete.
>  	For example, consider the following sequence of events::
>  
> @@ -196,7 +196,7 @@ synchronize_rcu()
>  	any that begin after synchronize_rcu() is invoked.
>  
>  	Of course, synchronize_rcu() does not necessarily return
> -	-immediately- after the last pre-existing RCU read-side critical
> +	**immediately** after the last pre-existing RCU read-side critical
>  	section completes.  For one thing, there might well be scheduling
>  	delays.  For another thing, many RCU implementations process
>  	requests in batches in order to improve efficiencies, which can
> @@ -225,10 +225,10 @@ synchronize_rcu()
>  	checklist.txt for some approaches to limiting the update rate.
>  
>  rcu_assign_pointer()
> -
> +^^^^^^^^^^^^^^^^^^^^
>  	void rcu_assign_pointer(p, typeof(p) v);
>  
> -	Yes, rcu_assign_pointer() -is- implemented as a macro, though it
> +	Yes, rcu_assign_pointer() **is** implemented as a macro, though it
>  	would be cool to be able to declare a function in this manner.
>  	(Compiler experts will no doubt disagree.)
>  
> @@ -245,7 +245,7 @@ rcu_assign_pointer()
>  	the _rcu list-manipulation primitives such as list_add_rcu().
>  
>  rcu_dereference()
> -
> +^^^^^^^^^^^^^^^^^
>  	typeof(p) rcu_dereference(p);
>  
>  	Like rcu_assign_pointer(), rcu_dereference() must be implemented
> @@ -280,8 +280,8 @@ rcu_dereference()
>  	unnecessary overhead on Alpha CPUs.
>  
>  	Note that the value returned by rcu_dereference() is valid
> -	only within the enclosing RCU read-side critical section [1].
> -	For example, the following is -not- legal::
> +	only within the enclosing RCU read-side critical section [1]_.
> +	For example, the following is **not** legal::
>  
>  		rcu_read_lock();
>  		p = rcu_dereference(head.next);
> @@ -304,9 +304,11 @@ rcu_dereference()
>  	at any time, including immediately after the rcu_dereference().
>  	And, again like rcu_assign_pointer(), rcu_dereference() is
>  	typically used indirectly, via the _rcu list-manipulation
> -	primitives, such as list_for_each_entry_rcu() [2].
> +	primitives, such as list_for_each_entry_rcu() [2]_.
> +
> +	.. [1]

Hey Phong,
I just checked the patch but this change doesn't seem quite right.
It just creates an empty footnote. Something on the lines of
..	[1] The variant rcu_dereference_protected()...
should work.

>  
> -	[1] The variant rcu_dereference_protected() can be used outside
> +	The variant rcu_dereference_protected() can be used outside
>  	of an RCU read-side critical section as long as the usage is
>  	protected by locks acquired by the update-side code.  This variant
>  	avoids the lockdep warning that would happen when using (for
> @@ -319,7 +321,9 @@ rcu_dereference()
>  	a lockdep splat is emitted.  See Documentation/RCU/Design/Requirements/Requirements.rst
>  	and the API's code comments for more details and example usage.
>  
> -	[2] If the list_for_each_entry_rcu() instance might be used by
> +	.. [2]

Similarly for this.

> +
> +	If the list_for_each_entry_rcu() instance might be used by
>  	update-side code as well as by RCU readers, then an additional
>  	lockdep expression can be added to its list of arguments.
>  	For example, given an additional "lock_is_held(&mylock)" argument,
> @@ -459,22 +463,22 @@ uses of RCU may be found in :ref:`listRCU.rst <list_rcu_doc>`,
>  
>  So, to sum up:
>  
> -o	Use rcu_read_lock() and rcu_read_unlock() to guard RCU
> +-	Use rcu_read_lock() and rcu_read_unlock() to guard RCU
>  	read-side critical sections.
>  
> -o	Within an RCU read-side critical section, use rcu_dereference()
> +-	Within an RCU read-side critical section, use rcu_dereference()
>  	to dereference RCU-protected pointers.
>  
> -o	Use some solid scheme (such as locks or semaphores) to
> +-	Use some solid scheme (such as locks or semaphores) to
>  	keep concurrent updates from interfering with each other.
>  
> -o	Use rcu_assign_pointer() to update an RCU-protected pointer.
> +-	Use rcu_assign_pointer() to update an RCU-protected pointer.
>  	This primitive protects concurrent readers from the updater,
> -	-not- concurrent updates from each other!  You therefore still
> +	**not** concurrent updates from each other!  You therefore still
>  	need to use locking (or something similar) to keep concurrent
>  	rcu_assign_pointer() primitives from interfering with each other.
>  
> -o	Use synchronize_rcu() -after- removing a data element from an
> +-	Use synchronize_rcu() **after** removing a data element from an
>  	RCU-protected data structure, but -before- reclaiming/freeing

And this -before- is feeling lonely aswell.


Thanks
Amol

>  	the data element, in order to wait for the completion of all
>  	RCU read-side critical sections that might be referencing that
> @@ -566,7 +570,7 @@ namely foo_reclaim().
>  The summary of advice is the same as for the previous section, except
>  that we are now using call_rcu() rather than synchronize_rcu():
>  
> -o	Use call_rcu() -after- removing a data element from an
> +-	Use call_rcu() **after** removing a data element from an
>  	RCU-protected data structure in order to register a callback
>  	function that will be invoked after the completion of all RCU
>  	read-side critical sections that might be referencing that
> @@ -603,7 +607,7 @@ more details on the current implementation as of early 2004.
>  
>  
>  5A.  "TOY" IMPLEMENTATION #1: LOCKING
> -
> +^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
>  This section presents a "toy" RCU implementation that is based on
>  familiar locking primitives.  Its overhead makes it a non-starter for
>  real-life use, as does its lack of scalability.  It is also unsuitable
> @@ -671,6 +675,8 @@ that the only thing that can block rcu_read_lock() is a synchronize_rcu().
>  But synchronize_rcu() does not acquire any locks while holding rcu_gp_mutex,
>  so there can be no deadlock cycle.
>  
> +.. _quiz_1:
> +
>  Quick Quiz #1:
>  		Why is this argument naive?  How could a deadlock
>  		occur when using this algorithm in a real-world Linux
> @@ -679,7 +685,7 @@ Quick Quiz #1:
>  :ref:`Answers to Quick Quiz <8_whatisRCU>`
>  
>  5B.  "TOY" EXAMPLE #2: CLASSIC RCU
> -
> +^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
>  This section presents a "toy" RCU implementation that is based on
>  "classic RCU".  It is also short on performance (but only for updates) and
>  on features such as hotplug CPU and the ability to run in CONFIG_PREEMPT
> @@ -710,14 +716,14 @@ CPU in turn.  The run_on() primitive can be implemented straightforwardly
>  in terms of the sched_setaffinity() primitive.  Of course, a somewhat less
>  "toy" implementation would restore the affinity upon completion rather
>  than just leaving all tasks running on the last CPU, but when I said
> -"toy", I meant -toy-!
> +"toy", I meant **toy**!
>  
>  So how the heck is this supposed to work???
>  
>  Remember that it is illegal to block while in an RCU read-side critical
>  section.  Therefore, if a given CPU executes a context switch, we know
>  that it must have completed all preceding RCU read-side critical sections.
> -Once -all- CPUs have executed a context switch, then -all- preceding
> +Once **all** CPUs have executed a context switch, then **all** preceding
>  RCU read-side critical sections will have completed.
>  
>  So, suppose that we remove a data item from its structure and then invoke
> @@ -725,12 +731,16 @@ synchronize_rcu().  Once synchronize_rcu() returns, we are guaranteed
>  that there are no RCU read-side critical sections holding a reference
>  to that data item, so we can safely reclaim it.
>  
> +.. _quiz_2:
> +
>  Quick Quiz #2:
>  		Give an example where Classic RCU's read-side
> -		overhead is -negative-.
> +		overhead is **negative**.
>  
>  :ref:`Answers to Quick Quiz <8_whatisRCU>`
>  
> +.. _quiz_3:
> +
>  Quick Quiz #3:
>  		If it is illegal to block in an RCU read-side
>  		critical section, what the heck do you do in
> @@ -1076,9 +1086,11 @@ Answer:
>  		approach where tasks in RCU read-side critical sections
>  		cannot be blocked by tasks executing synchronize_rcu().
>  
> +:ref:`Back to Quick Quiz #1 <quiz_1>`
> +
>  Quick Quiz #2:
>  		Give an example where Classic RCU's read-side
> -		overhead is -negative-.
> +		overhead is **negative**.
>  
>  Answer:
>  		Imagine a single-CPU system with a non-CONFIG_PREEMPT
> @@ -1103,6 +1115,8 @@ Answer:
>  		even the theoretical possibility of negative overhead for
>  		a synchronization primitive is a bit unexpected.  ;-)
>  
> +:ref:`Back to Quick Quiz #2 <quiz_2>`
> +
>  Quick Quiz #3:
>  		If it is illegal to block in an RCU read-side
>  		critical section, what the heck do you do in
> @@ -1128,6 +1142,7 @@ Answer:
>  		Besides, how does the computer know what pizza parlor
>  		the human being went to???
>  
> +:ref:`Back to Quick Quiz #3 <quiz_3>`
>  
>  ACKNOWLEDGEMENTS
>  
> -- 
> 2.20.1
>