On Sat, May 13, 2017 at 11:37:06PM +0900, Akira Yokosawa wrote:
> On 2017/05/13 05:45:56 -0700, Paul E. McKenney wrote:
> > On Sat, May 13, 2017 at 09:04:38PM +0900, Akira Yokosawa wrote:
> >> Hi Jason & Paul,
> >>
> >> although this has already been applied, I have a comment.
> >>
> >> On 2017/05/11 23:03:41 +0800, Junchang Wang wrote:
> >>> Signed-off-by: Junchang Wang <junchangwang@xxxxxxxxx>
> >>> ---
> >>> CodeSamples/count/count_stat_eventual.c | 8 ++++----
> >>> 1 file changed, 4 insertions(+), 4 deletions(-)
> >>>
> >>> diff --git a/CodeSamples/count/count_stat_eventual.c b/CodeSamples/count/count_stat_eventual.c
> >>> index 059ab8b..cbde4aa 100644
> >>> --- a/CodeSamples/count/count_stat_eventual.c
> >>> +++ b/CodeSamples/count/count_stat_eventual.c
> >>> @@ -27,12 +27,12 @@ int stopflag;
> >>>
> >>> void inc_count(void)
> >>> {
> >>> - ACCESS_ONCE(__get_thread_var(counter))++;
> >>> + READ_ONCE(__get_thread_var(counter))++;
> >>
> >> This is OK because READ_ONCE() is defined as the same as ACCESS_ONCE()
> >> in CodeSamples. However, the definition in the current Linux kernel
> >> would not permit this.
> >>
> >> A read-modify-write access would need both READ_ONCE() and WRITE_ONCE().
> >> However, since "counter" is thread local and updated only by its owner,
> >> we don't need READ_ONCE() here. So:
> >>
> >> + WRITE_ONCE(__get_thread_var(counter), __get_thread_var(counter) + 1);
> >>
> >> should have been sufficient.
> >>
> >> Problem with this change is that the line gets too wide when applied to
> >> the code snippet in 2-column layout.
> >
> > Good point -- though renumbering the code is not all -that- hard.
> >
> > I clearly should have made a better READ_ONCE() that enforced the same
> > constraints as does the Linux kernel, perhaps something like this:
> >
> > #define READ_ONCE(x) ({ ACCESS_ONCE(x) })
> >
> > Thoughts?
>
> I assume you meant:
>
> #define READ_ONCE(x) ({ ACCESS_ONCE(x); })
Indeed! Good catch!!!
> I'm afraid this still permits uses such as:
>
> READ_ONCE(y)++;
>
> Looks like we need a complex definition which resembles that of
> include/linux/compiler.h. Hmm???
OK, how about this?
#define READ_ONCE(x) ({ typeof(x) ___x = ACCESS_ONCE(x); ___x; })
> And I have another pending question regarding 2/2 of this patch set.
> That might result in other addition of line to the code. I think
> I can send it tomorrow.
Looking forward to seeing it! ;-)
Thanx, Paul
> >> Hmm...
> >>
> >> Thanks, Akira
> >>
> >>> }
> >>>
> >>> unsigned long read_count(void)
> >>> {
> >>> - return ACCESS_ONCE(global_count);
> >>> + return READ_ONCE(global_count);
> >>> }
> >>>
> >>> void *eventual(void *arg)
> >>> @@ -43,8 +43,8 @@ void *eventual(void *arg)
> >>> while (stopflag < 3) {
> >>> sum = 0;
> >>> for_each_thread(t)
> >>> - sum += ACCESS_ONCE(per_thread(counter, t));
> >>> - ACCESS_ONCE(global_count) = sum;
> >>> + sum += READ_ONCE(per_thread(counter, t));
> >>> + WRITE_ONCE(global_count, sum);
> >>> poll(NULL, 0, 1);
> >>> if (stopflag) {
> >>> smp_mb();
> >>>
> >>
> >
> >
>
--
To unsubscribe from this list: send the line "unsubscribe perfbook" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
