Re: [RFC PATCH -perfbook 2/2] count: Update QQ 5.46

On Thu, Jan 11, 2024 at 11:05:57PM +0900, Akira Yokosawa wrote:
> Commit cee469f9a7aa ("api: Switch x86 atomics to gcc C11-like
> atomics") changed the definition of atomic_set() as follows:
> 
>     #define atomic_set(v, i) \
> 	__atomic_store_n(&(v)->counter, (i), __ATOMIC_RELAXED)
> 
> Previously, it was defined for x86 as follows:
> 
>     #define atomic_set(v, i)	(((v)->counter) = (i))
> 
> QQ 5.46 assumed the old definition and it can confuse those who have
> followed the definition under CodeSamples/.
> 
> Reword QQ 5.46 to prevent such confusions.
> 
> Add a related QQ asking the need of atomic operation for atomic_set().
> 
> Reported-by: Hao Lee <[email protected]>
> Link: https://lore.kernel.org/perfbook/[email protected]/
> Signed-off-by: Akira Yokosawa <[email protected]>

Queued and pushed both, thank you, and please accept my apologies for
the delay!

							Thanx, Paul

> ---
>  count/count.tex | 36 ++++++++++++++++++++++++++++++------
>  1 file changed, 30 insertions(+), 6 deletions(-)
> 
> diff --git a/count/count.tex b/count/count.tex
> index c82e7333..e8248b95 100644
> --- a/count/count.tex
> +++ b/count/count.tex
> @@ -2257,19 +2257,43 @@ as it is with the \co{count_register_thread()} function starting on
>  \clnref{unregister:b}.
>  \end{fcvref}
>  
> -\QuickQuiz{
> -	Given that the \co{atomic_set()} primitive does a simple
> -	store to the specified \co{atomic_t}, how can
> +\QuickQuizSeries{%
> +\QuickQuizB{
> +	How can
>  	\clnrefr{ln:count:count_lim_atomic:utility2:balance:atmcset} of
>  	\co{balance_count()} in
>  	\cref{lst:count:Atomic Limit Counter Utility Functions 2}
>  	work correctly in face of concurrent \co{flush_local_count()}
>  	updates to this variable?
> -}\QuickQuizAnswer{
> -	The caller of both \co{balance_count()} and
> +}\QuickQuizAnswerB{
> +	It wouldn't work if they were executed concurrently!
> +	But no worries.
> +	The callers of both \co{balance_count()} and
>  	\co{flush_local_count()} hold \co{gblcnt_mutex}, so
>  	only one may be executing at a given time.
> -}\QuickQuizEnd
> +}\QuickQuizEndB
> +%
> +\QuickQuizE{
> +	Does the \co{atomic_set()} primitive in \co{balance_count()}
> +	really need to be atomic?
> +}\QuickQuizAnswerE{
> +	No, it does not.
> + 	As far as the code in question is concerned,
> +	the \co{atomic_set()} updates the per-thread variable
> +	\co{counterandmax} of its own, whose value can only be updated
> +	either from its own thread's fastpaths or from slowpaths of all
> +	updaters.
> +	As the \co{atomic_set()} is in the slowpath, it can never
> +	race with other atomic operations.
> +
> +	So, even if this code is ported to an ISA where a plain store
> +	to an \co{atomic_t} can interfere with atomic read-modify-write
> +	operations, a non-atomic \co{atomic_set()} should work.
> +
> +	That said, such an optimization in slowpaths which is effective
> +	only for those peculiar ISAs might not be worth bothering.
> +}\QuickQuizEndE
> +}			% End of \QuickQuizSeries
>  
>  The next section qualitatively evaluates this design.
>  \ebFloatBarrier
> -- 
> 2.34.1
> 
>