Hi Alan,
On Tue, 4 Apr 2023 12:47:53 -0400, Alan Huang wrote:
> The original memory barriers in count_stat_eventual.c ensure
> writing to global_count happens before writing to stopflag and
> reading from stopflag happens before later reading from global_count.
>
> Thus, smp_load_acquire and smp_store_release will suffice.
So far, looks good.
But those smp_mb()s are mentioned in Section 5.2.4.
Please see inline comments below:
>
> 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.
I'm kind of not 100% sure of your changes in count_lim_sig.c.
I'll comment in another mail. That might take a while.
> Therefore, partial memory barrier
> will suffice.
>
> Signed-off-by: Alan Huang <mmpgouride@xxxxxxxxx>
> ---
> CodeSamples/count/count_lim_sig.c | 10 +++-------
> CodeSamples/count/count_stat_eventual.c | 6 ++----
> count/count.tex | 9 ++++-----
> 3 files changed, 9 insertions(+), 16 deletions(-)
>
[...]
> diff --git a/CodeSamples/count/count_stat_eventual.c b/CodeSamples/count/count_stat_eventual.c
> index 967644de..7157ee0e 100644
> --- a/CodeSamples/count/count_stat_eventual.c
> +++ b/CodeSamples/count/count_stat_eventual.c
> @@ -51,8 +51,7 @@ void *eventual(void *arg) //\lnlbl{eventual:b}
> WRITE_ONCE(global_count, sum);
> poll(NULL, 0, 1);
> if (READ_ONCE(stopflag)) {
> - smp_mb();
> - WRITE_ONCE(stopflag, stopflag + 1);
> + smp_store_release(&stopflag, stopflag + 1);
> }
> }
> return NULL;
> @@ -73,9 +72,8 @@ void count_init(void) //\lnlbl{init:b}
> void count_cleanup(void) //\lnlbl{cleanup:b}
> {
> WRITE_ONCE(stopflag, 1);
> - while (READ_ONCE(stopflag) < 3)
> + while (smp_load_acquire(&stopflag) < 3)
> poll(NULL, 0, 1);
> - smp_mb();
> } //\lnlbl{cleanup:e}
> //\end{snippet}
So, you'd also need to update a paragraph in Section 5.2.4,
which currently reads:
The count_cleanup() function on lines 48--54 coordinates termination.
The calls to smp_mb() here and in eventual() ensure that all updates to
global_count are visible to code following the call to count_cleanup().
And I think splitting the changes in count_stat_eventual.c and
count_lim_sig.c into individual patches would ease the review on
our side.
So, can you send a patch concerning count_stat_eventual.c
first ?
I'll comment on count_lim_sig.c later, that is, if Paul won't beat
me to it.
Thanks, Akira
