[PATCH] CodeSamples/count: Remove unnecessary memory barriers

[PATCH] CodeSamples/count: Remove unnecessary memory barriers

[Alan Huang]

In count_lim_sig.c, there is only one ordering required, that is
writing to counter happens before setting theft to THEFT_READY in
add_count/sub_count's fast path. Therefore, partial memory barrier
will suffice.

Signed-off-by: Alan Huang <mmpgouride@gmail.com>
---
 CodeSamples/count/count_lim_sig.c | 10 +++-------
 count/count.tex                   | 12 ++++++------
 2 files changed, 9 insertions(+), 13 deletions(-)

diff --git a/CodeSamples/count/count_lim_sig.c b/CodeSamples/count/count_lim_sig.c
index 59da8077..c2f61197 100644
--- a/CodeSamples/count/count_lim_sig.c
+++ b/CodeSamples/count/count_lim_sig.c
@@ -56,12 +56,10 @@ static void flush_local_count_sig(int unused)	//\lnlbl{flush_sig:b}
 {
 	if (READ_ONCE(theft) != THEFT_REQ)	//\lnlbl{flush_sig:check:REQ}
 		return;				//\lnlbl{flush_sig:return:n}
-	smp_mb();				//\lnlbl{flush_sig:mb:1}
 	WRITE_ONCE(theft, THEFT_ACK);		//\lnlbl{flush_sig:set:ACK}
 	if (!counting) {			//\lnlbl{flush_sig:check:fast}
-		WRITE_ONCE(theft, THEFT_READY);	//\lnlbl{flush_sig:set:READY}
+		smp_store_release(&theft, THEFT_READY);	//\lnlbl{flush_sig:set:READY}
 	}
-	smp_mb();
 }						//\lnlbl{flush_sig:e}
 
 static void flush_local_count(void)			//\lnlbl{flush:b}
@@ -125,8 +123,7 @@ int add_count(unsigned long delta)			//\lnlbl{b}
 	WRITE_ONCE(counting, 0);			//\lnlbl{clearcnt}
 	barrier();					//\lnlbl{barrier:3}
 	if (READ_ONCE(theft) == THEFT_ACK) {		//\lnlbl{check:ACK}
-		smp_mb();				//\lnlbl{mb}
-		WRITE_ONCE(theft, THEFT_READY);		//\lnlbl{READY}
+		smp_store_release(&theft, THEFT_READY);		//\lnlbl{READY}
 	}
 	if (fastpath)
 		return 1;				//\lnlbl{return:fs}
@@ -164,8 +161,7 @@ int sub_count(unsigned long delta)
 	WRITE_ONCE(counting, 0);
 	barrier();
 	if (READ_ONCE(theft) == THEFT_ACK) {
-		smp_mb();
-		WRITE_ONCE(theft, THEFT_READY);
+		smp_store_release(&theft, THEFT_READY);
 	}
 	if (fastpath)
 		return 1;
diff --git a/count/count.tex b/count/count.tex
index 8ab67e2e..4c139d63 100644
--- a/count/count.tex
+++ b/count/count.tex
@@ -2425,12 +2425,11 @@ handler used in the theft process.
 \Clnref{check:REQ,return:n} check to see if
 the \co{theft} state is REQ, and, if not
 returns without change.
-\Clnref{mb:1} executes a \IX{memory barrier} to ensure that the sampling
-of the theft variable happens before any change to that variable.
 \Clnref{set:ACK} sets the \co{theft} state to ACK, and, if
 \clnref{check:fast} sees that
 this thread's fastpaths are not running, \clnref{set:READY} sets the \co{theft}
-state to READY\@.
+state to READY, with the release store ensuring any change to counter in
+the fastpath happens before the change of theft to READY\@.
 \end{fcvref}
 
 \begin{listing}
@@ -2595,9 +2594,10 @@ handlers to undertake theft.
 \Clnref{barrier:3} again disables compiler reordering, and then
 \clnref{check:ACK}
 checks to see if the signal handler deferred the \co{theft}
-state-change to READY, and, if so, \clnref{mb} executes a memory
-barrier to ensure that any CPU that sees \clnref{READY} setting state to
-READY also sees the effects of \clnref{add:f}.
+state-change to READY, and, if so, \clnref{READY} changes theft to
+READY with the release store ensuring that
+any CPU that sees the READY state also sees the effects
+of \clnref{add:f}.
 If the fastpath addition at \clnref{add:f} was executed, then
 \clnref{return:fs} returns
 success.
-- 
2.34.1

Re: [PATCH] CodeSamples/count: Remove unnecessary memory barriers

[Alan Huang]

Hi Paul and Akira,

This is the patch v3, I forgot to add a version tag…

And I think it may be better to add a question in a subsequent path.

Thanks,
Alan



> 2023年4月8日 上午1:58，Alan Huang <mmpgouride@gmail.com> 写道：
> 
> In count_lim_sig.c, there is only one ordering required, that is
> writing to counter happens before setting theft to THEFT_READY in
> add_count/sub_count's fast path. Therefore, partial memory barrier
> will suffice.
> 
> Signed-off-by: Alan Huang <mmpgouride@gmail.com>
> ---
> CodeSamples/count/count_lim_sig.c | 10 +++-------
> count/count.tex                   | 12 ++++++------
> 2 files changed, 9 insertions(+), 13 deletions(-)
> 
> diff --git a/CodeSamples/count/count_lim_sig.c b/CodeSamples/count/count_lim_sig.c
> index 59da8077..c2f61197 100644
> --- a/CodeSamples/count/count_lim_sig.c
> +++ b/CodeSamples/count/count_lim_sig.c
> @@ -56,12 +56,10 @@ static void flush_local_count_sig(int unused)	//\lnlbl{flush_sig:b}
> {
> 	if (READ_ONCE(theft) != THEFT_REQ)	//\lnlbl{flush_sig:check:REQ}
> 		return;				//\lnlbl{flush_sig:return:n}
> -	smp_mb();				//\lnlbl{flush_sig:mb:1}
> 	WRITE_ONCE(theft, THEFT_ACK);		//\lnlbl{flush_sig:set:ACK}
> 	if (!counting) {			//\lnlbl{flush_sig:check:fast}
> -		WRITE_ONCE(theft, THEFT_READY);	//\lnlbl{flush_sig:set:READY}
> +		smp_store_release(&theft, THEFT_READY);	//\lnlbl{flush_sig:set:READY}
> 	}
> -	smp_mb();
> }						//\lnlbl{flush_sig:e}
> 
> static void flush_local_count(void)			//\lnlbl{flush:b}
> @@ -125,8 +123,7 @@ int add_count(unsigned long delta)			//\lnlbl{b}
> 	WRITE_ONCE(counting, 0);			//\lnlbl{clearcnt}
> 	barrier();					//\lnlbl{barrier:3}
> 	if (READ_ONCE(theft) == THEFT_ACK) {		//\lnlbl{check:ACK}
> -		smp_mb();				//\lnlbl{mb}
> -		WRITE_ONCE(theft, THEFT_READY);		//\lnlbl{READY}
> +		smp_store_release(&theft, THEFT_READY);		//\lnlbl{READY}
> 	}
> 	if (fastpath)
> 		return 1;				//\lnlbl{return:fs}
> @@ -164,8 +161,7 @@ int sub_count(unsigned long delta)
> 	WRITE_ONCE(counting, 0);
> 	barrier();
> 	if (READ_ONCE(theft) == THEFT_ACK) {
> -		smp_mb();
> -		WRITE_ONCE(theft, THEFT_READY);
> +		smp_store_release(&theft, THEFT_READY);
> 	}
> 	if (fastpath)
> 		return 1;
> diff --git a/count/count.tex b/count/count.tex
> index 8ab67e2e..4c139d63 100644
> --- a/count/count.tex
> +++ b/count/count.tex
> @@ -2425,12 +2425,11 @@ handler used in the theft process.
> \Clnref{check:REQ,return:n} check to see if
> the \co{theft} state is REQ, and, if not
> returns without change.
> -\Clnref{mb:1} executes a \IX{memory barrier} to ensure that the sampling
> -of the theft variable happens before any change to that variable.
> \Clnref{set:ACK} sets the \co{theft} state to ACK, and, if
> \clnref{check:fast} sees that
> this thread's fastpaths are not running, \clnref{set:READY} sets the \co{theft}
> -state to READY\@.
> +state to READY, with the release store ensuring any change to counter in
> +the fastpath happens before the change of theft to READY\@.
> \end{fcvref}
> 
> \begin{listing}
> @@ -2595,9 +2594,10 @@ handlers to undertake theft.
> \Clnref{barrier:3} again disables compiler reordering, and then
> \clnref{check:ACK}
> checks to see if the signal handler deferred the \co{theft}
> -state-change to READY, and, if so, \clnref{mb} executes a memory
> -barrier to ensure that any CPU that sees \clnref{READY} setting state to
> -READY also sees the effects of \clnref{add:f}.
> +state-change to READY, and, if so, \clnref{READY} changes theft to
> +READY with the release store ensuring that
> +any CPU that sees the READY state also sees the effects
> +of \clnref{add:f}.
> If the fastpath addition at \clnref{add:f} was executed, then
> \clnref{return:fs} returns
> success.
> -- 
> 2.34.1
> 

Re: [PATCH] CodeSamples/count: Remove unnecessary memory barriers

[Akira Yokosawa]

Hi Alan,

Maybe I was not clear enough, but there are minor inconsistencies in
PDF rendering.  Please see below.

On Fri,  7 Apr 2023 13:58:13 -0400, Alan Huang wrote:
> In count_lim_sig.c, there is only one ordering required, that is
> writing to counter happens before setting theft to THEFT_READY in
> add_count/sub_count's fast path. Therefore, partial memory barrier
> will suffice.
> 
> Signed-off-by: Alan Huang <mmpgouride@gmail.com>
> ---
>  CodeSamples/count/count_lim_sig.c | 10 +++-------
>  count/count.tex                   | 12 ++++++------
>  2 files changed, 9 insertions(+), 13 deletions(-)
> 
> diff --git a/CodeSamples/count/count_lim_sig.c b/CodeSamples/count/count_lim_sig.c
> index 59da8077..c2f61197 100644
> --- a/CodeSamples/count/count_lim_sig.c
> +++ b/CodeSamples/count/count_lim_sig.c
> @@ -56,12 +56,10 @@ static void flush_local_count_sig(int unused)	//\lnlbl{flush_sig:b}
>  {
>  	if (READ_ONCE(theft) != THEFT_REQ)	//\lnlbl{flush_sig:check:REQ}
>  		return;				//\lnlbl{flush_sig:return:n}
> -	smp_mb();				//\lnlbl{flush_sig:mb:1}
>  	WRITE_ONCE(theft, THEFT_ACK);		//\lnlbl{flush_sig:set:ACK}
>  	if (!counting) {			//\lnlbl{flush_sig:check:fast}
> -		WRITE_ONCE(theft, THEFT_READY);	//\lnlbl{flush_sig:set:READY}
> +		smp_store_release(&theft, THEFT_READY);	//\lnlbl{flush_sig:set:READY}
>  	}
> -	smp_mb();
>  }						//\lnlbl{flush_sig:e}
>  
>  static void flush_local_count(void)			//\lnlbl{flush:b}
> @@ -125,8 +123,7 @@ int add_count(unsigned long delta)			//\lnlbl{b}
>  	WRITE_ONCE(counting, 0);			//\lnlbl{clearcnt}
>  	barrier();					//\lnlbl{barrier:3}
>  	if (READ_ONCE(theft) == THEFT_ACK) {		//\lnlbl{check:ACK}
> -		smp_mb();				//\lnlbl{mb}
> -		WRITE_ONCE(theft, THEFT_READY);		//\lnlbl{READY}
> +		smp_store_release(&theft, THEFT_READY);		//\lnlbl{READY}
>  	}
>  	if (fastpath)
>  		return 1;				//\lnlbl{return:fs}
> @@ -164,8 +161,7 @@ int sub_count(unsigned long delta)
>  	WRITE_ONCE(counting, 0);
>  	barrier();
>  	if (READ_ONCE(theft) == THEFT_ACK) {
> -		smp_mb();
> -		WRITE_ONCE(theft, THEFT_READY);
> +		smp_store_release(&theft, THEFT_READY);
>  	}
>  	if (fastpath)
>  		return 1;
> diff --git a/count/count.tex b/count/count.tex
> index 8ab67e2e..4c139d63 100644
> --- a/count/count.tex
> +++ b/count/count.tex
> @@ -2425,12 +2425,11 @@ handler used in the theft process.
>  \Clnref{check:REQ,return:n} check to see if
>  the \co{theft} state is REQ, and, if not
>  returns without change.
> -\Clnref{mb:1} executes a \IX{memory barrier} to ensure that the sampling
> -of the theft variable happens before any change to that variable.
>  \Clnref{set:ACK} sets the \co{theft} state to ACK, and, if
>  \clnref{check:fast} sees that
>  this thread's fastpaths are not running, \clnref{set:READY} sets the \co{theft}
> -state to READY\@.
> +state to READY, with the release store ensuring any change to counter in
                                                                 \co{counter}

> +the fastpath happens before the change of theft to READY\@.
                                             \co{theft}

>  \end{fcvref}
>  
>  \begin{listing}
> @@ -2595,9 +2594,10 @@ handlers to undertake theft.
>  \Clnref{barrier:3} again disables compiler reordering, and then
>  \clnref{check:ACK}
>  checks to see if the signal handler deferred the \co{theft}
> -state-change to READY, and, if so, \clnref{mb} executes a memory
> -barrier to ensure that any CPU that sees \clnref{READY} setting state to
> -READY also sees the effects of \clnref{add:f}.
> +state-change to READY, and, if so, \clnref{READY} changes theft to
                                                             \co{theft}

> +READY with the release store ensuring that
> +any CPU that sees the READY state also sees the effects
> +of \clnref{add:f}.
>  If the fastpath addition at \clnref{add:f} was executed, then
>  \clnref{return:fs} returns
>  success.

With those addressed,

Reviewed-by: Akira Yokosawa <akiyks@gmail.com>

        Thanks, Akira

Re: [PATCH] CodeSamples/count: Remove unnecessary memory barriers

[Alan Huang]

[-- Attachment #1: Type: text/plain, Size: 4362 bytes --]

Hi Akira,

Thanks for pointing that out,  I have sent patch v4.

Thanks,
Alan



> 2023年4月8日 下午12:25，Akira Yokosawa <akiyks@gmail.com> 写道：
> 
> Hi Alan,
> 
> Maybe I was not clear enough, but there are minor inconsistencies in
> PDF rendering.  Please see below.
> 
> On Fri,  7 Apr 2023 13:58:13 -0400, Alan Huang wrote:
>> In count_lim_sig.c, there is only one ordering required, that is
>> writing to counter happens before setting theft to THEFT_READY in
>> add_count/sub_count's fast path. Therefore, partial memory barrier
>> will suffice.
>> 
>> Signed-off-by: Alan Huang <mmpgouride@gmail.com>
>> ---
>> CodeSamples/count/count_lim_sig.c | 10 +++-------
>> count/count.tex                   | 12 ++++++------
>> 2 files changed, 9 insertions(+), 13 deletions(-)
>> 
>> diff --git a/CodeSamples/count/count_lim_sig.c b/CodeSamples/count/count_lim_sig.c
>> index 59da8077..c2f61197 100644
>> --- a/CodeSamples/count/count_lim_sig.c
>> +++ b/CodeSamples/count/count_lim_sig.c
>> @@ -56,12 +56,10 @@ static void flush_local_count_sig(int unused)	//\lnlbl{flush_sig:b}
>> {
>> 	if (READ_ONCE(theft) != THEFT_REQ)	//\lnlbl{flush_sig:check:REQ}
>> 		return;				//\lnlbl{flush_sig:return:n}
>> -	smp_mb();				//\lnlbl{flush_sig:mb:1}
>> 	WRITE_ONCE(theft, THEFT_ACK);		//\lnlbl{flush_sig:set:ACK}
>> 	if (!counting) {			//\lnlbl{flush_sig:check:fast}
>> -		WRITE_ONCE(theft, THEFT_READY);	//\lnlbl{flush_sig:set:READY}
>> +		smp_store_release(&theft, THEFT_READY);	//\lnlbl{flush_sig:set:READY}
>> 	}
>> -	smp_mb();
>> }						//\lnlbl{flush_sig:e}
>> 
>> static void flush_local_count(void)			//\lnlbl{flush:b}
>> @@ -125,8 +123,7 @@ int add_count(unsigned long delta)			//\lnlbl{b}
>> 	WRITE_ONCE(counting, 0);			//\lnlbl{clearcnt}
>> 	barrier();					//\lnlbl{barrier:3}
>> 	if (READ_ONCE(theft) == THEFT_ACK) {		//\lnlbl{check:ACK}
>> -		smp_mb();				//\lnlbl{mb}
>> -		WRITE_ONCE(theft, THEFT_READY);		//\lnlbl{READY}
>> +		smp_store_release(&theft, THEFT_READY);		//\lnlbl{READY}
>> 	}
>> 	if (fastpath)
>> 		return 1;				//\lnlbl{return:fs}
>> @@ -164,8 +161,7 @@ int sub_count(unsigned long delta)
>> 	WRITE_ONCE(counting, 0);
>> 	barrier();
>> 	if (READ_ONCE(theft) == THEFT_ACK) {
>> -		smp_mb();
>> -		WRITE_ONCE(theft, THEFT_READY);
>> +		smp_store_release(&theft, THEFT_READY);
>> 	}
>> 	if (fastpath)
>> 		return 1;
>> diff --git a/count/count.tex b/count/count.tex
>> index 8ab67e2e..4c139d63 100644
>> --- a/count/count.tex
>> +++ b/count/count.tex
>> @@ -2425,12 +2425,11 @@ handler used in the theft process.
>> \Clnref{check:REQ,return:n} check to see if
>> the \co{theft} state is REQ, and, if not
>> returns without change.
>> -\Clnref{mb:1} executes a \IX{memory barrier} to ensure that the sampling
>> -of the theft variable happens before any change to that variable.
>> \Clnref{set:ACK} sets the \co{theft} state to ACK, and, if
>> \clnref{check:fast} sees that
>> this thread's fastpaths are not running, \clnref{set:READY} sets the \co{theft}
>> -state to READY\@.
>> +state to READY, with the release store ensuring any change to counter in
>                                                                 \co{counter}
> 
>> +the fastpath happens before the change of theft to READY\@.
>                                             \co{theft}
> 
>> \end{fcvref}
>> 
>> \begin{listing}
>> @@ -2595,9 +2594,10 @@ handlers to undertake theft.
>> \Clnref{barrier:3} again disables compiler reordering, and then
>> \clnref{check:ACK}
>> checks to see if the signal handler deferred the \co{theft}
>> -state-change to READY, and, if so, \clnref{mb} executes a memory
>> -barrier to ensure that any CPU that sees \clnref{READY} setting state to
>> -READY also sees the effects of \clnref{add:f}.
>> +state-change to READY, and, if so, \clnref{READY} changes theft to
>                                                             \co{theft}
> 
>> +READY with the release store ensuring that
>> +any CPU that sees the READY state also sees the effects
>> +of \clnref{add:f}.
>> If the fastpath addition at \clnref{add:f} was executed, then
>> \clnref{return:fs} returns
>> success.
> 
> With those addressed,
> 
> Reviewed-by: Akira Yokosawa <akiyks@gmail.com <mailto:akiyks@gmail.com>>
> 
>        Thanks, Akira


[-- Attachment #2: Type: text/html, Size: 24731 bytes --]

Re: [PATCH] CodeSamples/count: Remove unnecessary memory barriers

[Alan Huang]

Hi Akira,

Thanks for pointing that out,  I have sent patch v4.

Thanks,
Alan



> 2023年4月8日 下午12:25，Akira Yokosawa <akiyks@gmail.com> 写道：
> 
> Hi Alan,
> 
> Maybe I was not clear enough, but there are minor inconsistencies in
> PDF rendering.  Please see below.
> 
> On Fri,  7 Apr 2023 13:58:13 -0400, Alan Huang wrote:
>> In count_lim_sig.c, there is only one ordering required, that is
>> writing to counter happens before setting theft to THEFT_READY in
>> add_count/sub_count's fast path. Therefore, partial memory barrier
>> will suffice.
>> 
>> Signed-off-by: Alan Huang <mmpgouride@gmail.com>
>> ---
>> CodeSamples/count/count_lim_sig.c | 10 +++-------
>> count/count.tex                   | 12 ++++++------
>> 2 files changed, 9 insertions(+), 13 deletions(-)
>> 
>> diff --git a/CodeSamples/count/count_lim_sig.c b/CodeSamples/count/count_lim_sig.c
>> index 59da8077..c2f61197 100644
>> --- a/CodeSamples/count/count_lim_sig.c
>> +++ b/CodeSamples/count/count_lim_sig.c
>> @@ -56,12 +56,10 @@ static void flush_local_count_sig(int unused)	//\lnlbl{flush_sig:b}
>> {
>> 	if (READ_ONCE(theft) != THEFT_REQ)	//\lnlbl{flush_sig:check:REQ}
>> 		return;				//\lnlbl{flush_sig:return:n}
>> -	smp_mb();				//\lnlbl{flush_sig:mb:1}
>> 	WRITE_ONCE(theft, THEFT_ACK);		//\lnlbl{flush_sig:set:ACK}
>> 	if (!counting) {			//\lnlbl{flush_sig:check:fast}
>> -		WRITE_ONCE(theft, THEFT_READY);	//\lnlbl{flush_sig:set:READY}
>> +		smp_store_release(&theft, THEFT_READY);	//\lnlbl{flush_sig:set:READY}
>> 	}
>> -	smp_mb();
>> }						//\lnlbl{flush_sig:e}
>> 
>> static void flush_local_count(void)			//\lnlbl{flush:b}
>> @@ -125,8 +123,7 @@ int add_count(unsigned long delta)			//\lnlbl{b}
>> 	WRITE_ONCE(counting, 0);			//\lnlbl{clearcnt}
>> 	barrier();					//\lnlbl{barrier:3}
>> 	if (READ_ONCE(theft) == THEFT_ACK) {		//\lnlbl{check:ACK}
>> -		smp_mb();				//\lnlbl{mb}
>> -		WRITE_ONCE(theft, THEFT_READY);		//\lnlbl{READY}
>> +		smp_store_release(&theft, THEFT_READY);		//\lnlbl{READY}
>> 	}
>> 	if (fastpath)
>> 		return 1;				//\lnlbl{return:fs}
>> @@ -164,8 +161,7 @@ int sub_count(unsigned long delta)
>> 	WRITE_ONCE(counting, 0);
>> 	barrier();
>> 	if (READ_ONCE(theft) == THEFT_ACK) {
>> -		smp_mb();
>> -		WRITE_ONCE(theft, THEFT_READY);
>> +		smp_store_release(&theft, THEFT_READY);
>> 	}
>> 	if (fastpath)
>> 		return 1;
>> diff --git a/count/count.tex b/count/count.tex
>> index 8ab67e2e..4c139d63 100644
>> --- a/count/count.tex
>> +++ b/count/count.tex
>> @@ -2425,12 +2425,11 @@ handler used in the theft process.
>> \Clnref{check:REQ,return:n} check to see if
>> the \co{theft} state is REQ, and, if not
>> returns without change.
>> -\Clnref{mb:1} executes a \IX{memory barrier} to ensure that the sampling
>> -of the theft variable happens before any change to that variable.
>> \Clnref{set:ACK} sets the \co{theft} state to ACK, and, if
>> \clnref{check:fast} sees that
>> this thread's fastpaths are not running, \clnref{set:READY} sets the \co{theft}
>> -state to READY\@.
>> +state to READY, with the release store ensuring any change to counter in
>                                                                 \co{counter}
> 
>> +the fastpath happens before the change of theft to READY\@.
>                                             \co{theft}
> 
>> \end{fcvref}
>> 
>> \begin{listing}
>> @@ -2595,9 +2594,10 @@ handlers to undertake theft.
>> \Clnref{barrier:3} again disables compiler reordering, and then
>> \clnref{check:ACK}
>> checks to see if the signal handler deferred the \co{theft}
>> -state-change to READY, and, if so, \clnref{mb} executes a memory
>> -barrier to ensure that any CPU that sees \clnref{READY} setting state to
>> -READY also sees the effects of \clnref{add:f}.
>> +state-change to READY, and, if so, \clnref{READY} changes theft to
>                                                             \co{theft}
> 
>> +READY with the release store ensuring that
>> +any CPU that sees the READY state also sees the effects
>> +of \clnref{add:f}.
>> If the fastpath addition at \clnref{add:f} was executed, then
>> \clnref{return:fs} returns
>> success.
> 
> With those addressed,
> 
> Reviewed-by: Akira Yokosawa <akiyks@gmail.com>
> 
>        Thanks, Akira

Re: [PATCH] CodeSamples/count: Remove unnecessary memory barriers

[Akira Yokosawa]

Hi,

On Sat, 8 Apr 2023 02:04:24 +0800, Alan Huang wrote:
> Hi Paul and Akira,
> 
> This is the patch v3, I forgot to add a version tag…
> 
> And I think it may be better to add a question in a subsequent path.

So I'm looking at Quick Quizzes on count_lim_sig.c.
It looks to me Quick Quiz 5.50 lost its context due to the
removal of smp_mb() and the use of smp_store_release().

A band-aide fix would be to change the quiz to:

    In Listing 5.18's function flush_local_count_sig(), why
    are there READ_ONCE(), WRITE_ONCE(), and smp_store_release()
    wrappers around the uses of the theft per-thread variable?

, and to adjust the line count in its Answer.

However, smp_store_release() is by no means a simple wrapper.
So I'm wondering if it is worthwhile to keep this quiz.

Paul, how about replacing the quiz with the new quiz asking:

     In Listing 5.18, doesn't flush_local_count_sig() need
     a stronger memory barrier or two?

, and picking Alan's reasoning in the answer?

        Thanks, Akira

> 
> Thanks,
> Alan

Re: [PATCH] CodeSamples/count: Remove unnecessary memory barriers

[Paul E. McKenney]

On Sun, Apr 09, 2023 at 09:11:58AM +0900, Akira Yokosawa wrote:
> Hi,
> 
> On Sat, 8 Apr 2023 02:04:24 +0800, Alan Huang wrote:
> > Hi Paul and Akira,
> > 
> > This is the patch v3, I forgot to add a version tag…
> > 
> > And I think it may be better to add a question in a subsequent path.
> 
> So I'm looking at Quick Quizzes on count_lim_sig.c.
> It looks to me Quick Quiz 5.50 lost its context due to the
> removal of smp_mb() and the use of smp_store_release().
> 
> A band-aide fix would be to change the quiz to:
> 
>     In Listing 5.18's function flush_local_count_sig(), why
>     are there READ_ONCE(), WRITE_ONCE(), and smp_store_release()
>     wrappers around the uses of the theft per-thread variable?
> 
> , and to adjust the line count in its Answer.
> 
> However, smp_store_release() is by no means a simple wrapper.
> So I'm wondering if it is worthwhile to keep this quiz.
> 
> Paul, how about replacing the quiz with the new quiz asking:
> 
>      In Listing 5.18, doesn't flush_local_count_sig() need
>      a stronger memory barrier or two?
> 
> , and picking Alan's reasoning in the answer?

How about something like this?

							Thanx, Paul

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

diff --git a/count/count.tex b/count/count.tex
index 2aade053..ccc2e839 100644
--- a/count/count.tex
+++ b/count/count.tex
@@ -2441,28 +2441,11 @@ the fastpath happens before this change of \co{theft} to READY\@.
 
 \QuickQuiz{
 	In \cref{lst:count:Signal-Theft Limit Counter Value-Migration Functions}'s
-	function \co{flush_local_count_sig()}, why are there
-	\co{READ_ONCE()} and \co{WRITE_ONCE()} wrappers around
-	the uses of the
-	\co{theft} per-thread variable?
+	doesn't \co{flush_local_count_sig()} need stronger memory barriers?
 }\QuickQuizAnswer{
-	\begin{fcvref}[ln:count:count_lim_sig:migration:flush_sig]
-	The first one (on \clnref{check:REQ}) can be argued to be unnecessary.
-	The last two (\clnref{set:ACK,set:READY}) are important.
-	If these are removed, the compiler would be within its rights
-	to rewrite \clnrefrange{set:ACK}{set:READY} as follows:
-	\end{fcvref}
-
-\begin{VerbatimN}[firstnumber=14]
-theft = THEFT_READY;
-if (counting) {
-	theft = THEFT_ACK;
-}
-\end{VerbatimN}
-
-	This would be fatal, as the slowpath might see the transient
-	value of \co{THEFT_READY}, and start stealing before the
-	corresponding thread was ready.
+	No, that \co{smp_store_release()} suffices because this code
+	communicates only with \co{flush_local_count()}, and there is
+	no need for store-to-load ordering.
 }\QuickQuizEnd
 
 \begin{fcvref}[ln:count:count_lim_sig:migration:flush]

Re: [PATCH] CodeSamples/count: Remove unnecessary memory barriers

[Alan Huang]

The signal-theft counter reminds me of the signal-based user space RCU implementation, the signal hander there 
has  two full memory barriers around it, how about mentioning that a little?

Thanks,
Alan



> 2023年4月11日 上午3:06，Paul E. McKenney <paulmck@kernel.org> 写道：
> 
> On Sun, Apr 09, 2023 at 09:11:58AM +0900, Akira Yokosawa wrote:
>> Hi,
>> 
>> On Sat, 8 Apr 2023 02:04:24 +0800, Alan Huang wrote:
>>> Hi Paul and Akira,
>>> 
>>> This is the patch v3, I forgot to add a version tag…
>>> 
>>> And I think it may be better to add a question in a subsequent path.
>> 
>> So I'm looking at Quick Quizzes on count_lim_sig.c.
>> It looks to me Quick Quiz 5.50 lost its context due to the
>> removal of smp_mb() and the use of smp_store_release().
>> 
>> A band-aide fix would be to change the quiz to:
>> 
>>    In Listing 5.18's function flush_local_count_sig(), why
>>    are there READ_ONCE(), WRITE_ONCE(), and smp_store_release()
>>    wrappers around the uses of the theft per-thread variable?
>> 
>> , and to adjust the line count in its Answer.
>> 
>> However, smp_store_release() is by no means a simple wrapper.
>> So I'm wondering if it is worthwhile to keep this quiz.
>> 
>> Paul, how about replacing the quiz with the new quiz asking:
>> 
>>     In Listing 5.18, doesn't flush_local_count_sig() need
>>     a stronger memory barrier or two?
>> 
>> , and picking Alan's reasoning in the answer?
> 
> How about something like this?
> 
> 							Thanx, Paul
> 
> ------------------------------------------------------------------------
> 
> diff --git a/count/count.tex b/count/count.tex
> index 2aade053..ccc2e839 100644
> --- a/count/count.tex
> +++ b/count/count.tex
> @@ -2441,28 +2441,11 @@ the fastpath happens before this change of \co{theft} to READY\@.
> 
> \QuickQuiz{
> 	In \cref{lst:count:Signal-Theft Limit Counter Value-Migration Functions}'s
> -	function \co{flush_local_count_sig()}, why are there
> -	\co{READ_ONCE()} and \co{WRITE_ONCE()} wrappers around
> -	the uses of the
> -	\co{theft} per-thread variable?
> +	doesn't \co{flush_local_count_sig()} need stronger memory barriers?
> }\QuickQuizAnswer{
> -	\begin{fcvref}[ln:count:count_lim_sig:migration:flush_sig]
> -	The first one (on \clnref{check:REQ}) can be argued to be unnecessary.
> -	The last two (\clnref{set:ACK,set:READY}) are important.
> -	If these are removed, the compiler would be within its rights
> -	to rewrite \clnrefrange{set:ACK}{set:READY} as follows:
> -	\end{fcvref}
> -
> -\begin{VerbatimN}[firstnumber=14]
> -theft = THEFT_READY;
> -if (counting) {
> -	theft = THEFT_ACK;
> -}
> -\end{VerbatimN}
> -
> -	This would be fatal, as the slowpath might see the transient
> -	value of \co{THEFT_READY}, and start stealing before the
> -	corresponding thread was ready.
> +	No, that \co{smp_store_release()} suffices because this code
> +	communicates only with \co{flush_local_count()}, and there is
> +	no need for store-to-load ordering.
> }\QuickQuizEnd
> 
> \begin{fcvref}[ln:count:count_lim_sig:migration:flush]

Re: [PATCH] CodeSamples/count: Remove unnecessary memory barriers

[Elad Lahav]

I may be missing something, but why do you need memory barriers at all
in the signal handler? At least on the systems I know both setting up
the signal handler and restoring the context require kernel calls. On
x86 this doesn't matter as it is strongly ordered, but on ARM the
kernel calls provide full synchronization points.
Are there architectures/OS combinations where that is not true?

--Elad

On Tue, Apr 11, 2023 at 12:43 PM Alan Huang <mmpgouride@gmail.com> wrote:
>
> The signal-theft counter reminds me of the signal-based user space RCU implementation, the signal hander there
> has  two full memory barriers around it, how about mentioning that a little?
>
> Thanks,
> Alan
>
>
>
> > 2023年4月11日 上午3:06，Paul E. McKenney <paulmck@kernel.org> 写道：
> >
> > On Sun, Apr 09, 2023 at 09:11:58AM +0900, Akira Yokosawa wrote:
> >> Hi,
> >>
> >> On Sat, 8 Apr 2023 02:04:24 +0800, Alan Huang wrote:
> >>> Hi Paul and Akira,
> >>>
> >>> This is the patch v3, I forgot to add a version tag…
> >>>
> >>> And I think it may be better to add a question in a subsequent path.
> >>
> >> So I'm looking at Quick Quizzes on count_lim_sig.c.
> >> It looks to me Quick Quiz 5.50 lost its context due to the
> >> removal of smp_mb() and the use of smp_store_release().
> >>
> >> A band-aide fix would be to change the quiz to:
> >>
> >>    In Listing 5.18's function flush_local_count_sig(), why
> >>    are there READ_ONCE(), WRITE_ONCE(), and smp_store_release()
> >>    wrappers around the uses of the theft per-thread variable?
> >>
> >> , and to adjust the line count in its Answer.
> >>
> >> However, smp_store_release() is by no means a simple wrapper.
> >> So I'm wondering if it is worthwhile to keep this quiz.
> >>
> >> Paul, how about replacing the quiz with the new quiz asking:
> >>
> >>     In Listing 5.18, doesn't flush_local_count_sig() need
> >>     a stronger memory barrier or two?
> >>
> >> , and picking Alan's reasoning in the answer?
> >
> > How about something like this?
> >
> >                                                       Thanx, Paul
> >
> > ------------------------------------------------------------------------
> >
> > diff --git a/count/count.tex b/count/count.tex
> > index 2aade053..ccc2e839 100644
> > --- a/count/count.tex
> > +++ b/count/count.tex
> > @@ -2441,28 +2441,11 @@ the fastpath happens before this change of \co{theft} to READY\@.
> >
> > \QuickQuiz{
> >       In \cref{lst:count:Signal-Theft Limit Counter Value-Migration Functions}'s
> > -     function \co{flush_local_count_sig()}, why are there
> > -     \co{READ_ONCE()} and \co{WRITE_ONCE()} wrappers around
> > -     the uses of the
> > -     \co{theft} per-thread variable?
> > +     doesn't \co{flush_local_count_sig()} need stronger memory barriers?
> > }\QuickQuizAnswer{
> > -     \begin{fcvref}[ln:count:count_lim_sig:migration:flush_sig]
> > -     The first one (on \clnref{check:REQ}) can be argued to be unnecessary.
> > -     The last two (\clnref{set:ACK,set:READY}) are important.
> > -     If these are removed, the compiler would be within its rights
> > -     to rewrite \clnrefrange{set:ACK}{set:READY} as follows:
> > -     \end{fcvref}
> > -
> > -\begin{VerbatimN}[firstnumber=14]
> > -theft = THEFT_READY;
> > -if (counting) {
> > -     theft = THEFT_ACK;
> > -}
> > -\end{VerbatimN}
> > -
> > -     This would be fatal, as the slowpath might see the transient
> > -     value of \co{THEFT_READY}, and start stealing before the
> > -     corresponding thread was ready.
> > +     No, that \co{smp_store_release()} suffices because this code
> > +     communicates only with \co{flush_local_count()}, and there is
> > +     no need for store-to-load ordering.
> > }\QuickQuizEnd
> >
> > \begin{fcvref}[ln:count:count_lim_sig:migration:flush]
>

Re: [PATCH] CodeSamples/count: Remove unnecessary memory barriers

[Paul E. McKenney]

On Wed, Apr 12, 2023 at 07:32:45AM -0400, Elad Lahav wrote:
> I may be missing something, but why do you need memory barriers at all
> in the signal handler? At least on the systems I know both setting up
> the signal handler and restoring the context require kernel calls. On
> x86 this doesn't matter as it is strongly ordered, but on ARM the
> kernel calls provide full synchronization points.
> Are there architectures/OS combinations where that is not true?

It is quite possible that there is still more memory ordering in this
code than is absolutely required.

I would have some difficulty forcing myself to rely on implicit ordering
due to signal entry and exit, but then again, I have also some difficulty
forcing myself to rely on signals.  ;-)

But if there is some signal entry/exit guarantee in (say) POSIX, we should
be OK relying on it.

Any thoughts on interrupt handlers in kernels?

							Thanx, Paul

> --Elad
> 
> On Tue, Apr 11, 2023 at 12:43 PM Alan Huang <mmpgouride@gmail.com> wrote:
> >
> > The signal-theft counter reminds me of the signal-based user space RCU implementation, the signal hander there
> > has  two full memory barriers around it, how about mentioning that a little?
> >
> > Thanks,
> > Alan
> >
> >
> >
> > > 2023年4月11日 上午3:06，Paul E. McKenney <paulmck@kernel.org> 写道：
> > >
> > > On Sun, Apr 09, 2023 at 09:11:58AM +0900, Akira Yokosawa wrote:
> > >> Hi,
> > >>
> > >> On Sat, 8 Apr 2023 02:04:24 +0800, Alan Huang wrote:
> > >>> Hi Paul and Akira,
> > >>>
> > >>> This is the patch v3, I forgot to add a version tag…
> > >>>
> > >>> And I think it may be better to add a question in a subsequent path.
> > >>
> > >> So I'm looking at Quick Quizzes on count_lim_sig.c.
> > >> It looks to me Quick Quiz 5.50 lost its context due to the
> > >> removal of smp_mb() and the use of smp_store_release().
> > >>
> > >> A band-aide fix would be to change the quiz to:
> > >>
> > >>    In Listing 5.18's function flush_local_count_sig(), why
> > >>    are there READ_ONCE(), WRITE_ONCE(), and smp_store_release()
> > >>    wrappers around the uses of the theft per-thread variable?
> > >>
> > >> , and to adjust the line count in its Answer.
> > >>
> > >> However, smp_store_release() is by no means a simple wrapper.
> > >> So I'm wondering if it is worthwhile to keep this quiz.
> > >>
> > >> Paul, how about replacing the quiz with the new quiz asking:
> > >>
> > >>     In Listing 5.18, doesn't flush_local_count_sig() need
> > >>     a stronger memory barrier or two?
> > >>
> > >> , and picking Alan's reasoning in the answer?
> > >
> > > How about something like this?
> > >
> > >                                                       Thanx, Paul
> > >
> > > ------------------------------------------------------------------------
> > >
> > > diff --git a/count/count.tex b/count/count.tex
> > > index 2aade053..ccc2e839 100644
> > > --- a/count/count.tex
> > > +++ b/count/count.tex
> > > @@ -2441,28 +2441,11 @@ the fastpath happens before this change of \co{theft} to READY\@.
> > >
> > > \QuickQuiz{
> > >       In \cref{lst:count:Signal-Theft Limit Counter Value-Migration Functions}'s
> > > -     function \co{flush_local_count_sig()}, why are there
> > > -     \co{READ_ONCE()} and \co{WRITE_ONCE()} wrappers around
> > > -     the uses of the
> > > -     \co{theft} per-thread variable?
> > > +     doesn't \co{flush_local_count_sig()} need stronger memory barriers?
> > > }\QuickQuizAnswer{
> > > -     \begin{fcvref}[ln:count:count_lim_sig:migration:flush_sig]
> > > -     The first one (on \clnref{check:REQ}) can be argued to be unnecessary.
> > > -     The last two (\clnref{set:ACK,set:READY}) are important.
> > > -     If these are removed, the compiler would be within its rights
> > > -     to rewrite \clnrefrange{set:ACK}{set:READY} as follows:
> > > -     \end{fcvref}
> > > -
> > > -\begin{VerbatimN}[firstnumber=14]
> > > -theft = THEFT_READY;
> > > -if (counting) {
> > > -     theft = THEFT_ACK;
> > > -}
> > > -\end{VerbatimN}
> > > -
> > > -     This would be fatal, as the slowpath might see the transient
> > > -     value of \co{THEFT_READY}, and start stealing before the
> > > -     corresponding thread was ready.
> > > +     No, that \co{smp_store_release()} suffices because this code
> > > +     communicates only with \co{flush_local_count()}, and there is
> > > +     no need for store-to-load ordering.
> > > }\QuickQuizEnd
> > >
> > > \begin{fcvref}[ln:count:count_lim_sig:migration:flush]
> >