Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1257166 > unrolled thread

[PATCH 3/4] x86,asm: Re-work smp_store_mb()

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  [PATCH 3/4] x86,asm: Re-work smp_store_mb() Davidlohr Bueso <dave@stgolabs.net> - 2015-10-27 21:00 +0100
    Re: [PATCH 3/4] x86,asm: Re-work smp_store_mb() Linus Torvalds <torvalds@linux-foundation.org> - 2015-10-27 22:40 +0100
      Re: [PATCH 3/4] x86,asm: Re-work smp_store_mb() Davidlohr Bueso <dave@stgolabs.net> - 2015-10-27 23:10 +0100
      Re: [PATCH 3/4] x86,asm: Re-work smp_store_mb() Peter Zijlstra <peterz@infradead.org> - 2015-10-28 01:20 +0100
        Re: [PATCH 3/4] x86,asm: Re-work smp_store_mb() Davidlohr Bueso <dave@stgolabs.net> - 2015-10-28 21:00 +0100

#1257166 — [PATCH 3/4] x86,asm: Re-work smp_store_mb()

With the exception of the recent rename of set_mb to smp_store_mb,
thus explicitly enforcing SMP ordering, the code is quite stale -
going back to 2002, afaict. Specifically, replace the implicit
barriers of xchg for more standard smp_mb() call instead. Thus,

(i) We need not re-define it for SMP and UP systems. The later
already converts the smp_mb() to a compiler barrier.

(ii) Like most other archs, avoid using ugly/hacky (void)xchg
patterns and simply add the smp_mb() call explicitly after the
assignment.

Note that this might affect callers that could/would rely on the
atomicity semantics, but there are no guarantees of that for
smp_store_mb() mentioned anywhere, plus most archs use this anyway.
Thus we continue to be consistent with the memory-barriers.txt file,
and more importantly, maintain the semantics of the smp_ nature.

Cc: x86@kernel.org
Signed-off-by: Davidlohr Bueso <dbueso@suse.de>
---
 arch/x86/include/asm/barrier.h | 8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)

diff --git a/arch/x86/include/asm/barrier.h b/arch/x86/include/asm/barrier.h
index 0681d25..09f817a 100644
--- a/arch/x86/include/asm/barrier.h
+++ b/arch/x86/include/asm/barrier.h
@@ -35,14 +35,18 @@
 #define smp_mb()	mb()
 #define smp_rmb()	dma_rmb()
 #define smp_wmb()	barrier()
-#define smp_store_mb(var, value) do { (void)xchg(&var, value); } while (0)
 #else /* !SMP */
 #define smp_mb()	barrier()
 #define smp_rmb()	barrier()
 #define smp_wmb()	barrier()
-#define smp_store_mb(var, value) do { WRITE_ONCE(var, value); barrier(); } while (0)
 #endif /* SMP */
 
+#define smp_store_mb(var, val)						\
+do {									\
+	WRITE_ONCE(var, val);						\
+	smp_mb();							\
+} while (0)
+
 #define read_barrier_depends()		do { } while (0)
 #define smp_read_barrier_depends()	do { } while (0)
 
-- 
2.1.4

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [next] | [standalone]

#1257299

On Wed, Oct 28, 2015 at 4:53 AM, Davidlohr Bueso <dave@stgolabs.net> wrote:
>
> Note that this might affect callers that could/would rely on the
> atomicity semantics, but there are no guarantees of that for
> smp_store_mb() mentioned anywhere, plus most archs use this anyway.
> Thus we continue to be consistent with the memory-barriers.txt file,
> and more importantly, maintain the semantics of the smp_ nature.

So I dislike this patch, mostly because it now makes it obvious that
smp_store_mb() seems to be totally pointless. Every single
implementation is now apparently WRITE_ONCE+smp_mb(), and there are
what, five users of it, so why not then open-code it?

But more importantly, is the "WRITE_ONCE()" even necessary? If there
are no atomicity guarantees, then why bother with WRTE_ONCE() either?

So with this patch, the whole thing becomes pointless, I feel. (Ok, so
it may have been pointless before too, but at least before this patch
it generated special code, now it doesn't). So why carry it along at
all?

               Linus
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1257406

On Wed, 28 Oct 2015, Linus Torvalds wrote:

>On Wed, Oct 28, 2015 at 4:53 AM, Davidlohr Bueso <dave@stgolabs.net> wrote:
>>
>> Note that this might affect callers that could/would rely on the
>> atomicity semantics, but there are no guarantees of that for
>> smp_store_mb() mentioned anywhere, plus most archs use this anyway.
>> Thus we continue to be consistent with the memory-barriers.txt file,
>> and more importantly, maintain the semantics of the smp_ nature.
>
>So I dislike this patch, mostly because it now makes it obvious that
>smp_store_mb() seems to be totally pointless. Every single
>implementation is now apparently WRITE_ONCE+smp_mb(), and there are
>what, five users of it, so why not then open-code it?

So after having gone through pretty much all of smp_store_mb code, this
is a feeling I also share. However I justified its existence (as opposed
to dropping the call, updating all the callers/documenting the barriers
etc.) to at least encapsulate the store+mb logic, which apparently is a
pattern somewhat needed(?). Also, the name is obviously exactly what its
name implies.

But I have no strong preference either way. Now, if we should keep
smp_store_mb(), it should probably be made generic, instead of having
each arch define it.

>
>But more importantly, is the "WRITE_ONCE()" even necessary? If there
>are no atomicity guarantees, then why bother with WRTE_ONCE() either?

Agreed. Hmm, this was introduced by ab3f02fc237 (locking/arch: Add WRITE_ONCE()
to set_mb()), back when atomicity aspects were not clear yet.

>So with this patch, the whole thing becomes pointless, I feel. (Ok, so
>it may have been pointless before too, but at least before this patch
>it generated special code, now it doesn't). So why carry it along at
>all?

Ok, unless others are strongly against it, I'll send a series to drop the
call altogether.

Thanks,
Davidlohr
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1257497

On Wed, Oct 28, 2015 at 06:33:56AM +0900, Linus Torvalds wrote:
> On Wed, Oct 28, 2015 at 4:53 AM, Davidlohr Bueso <dave@stgolabs.net> wrote:
> >
> > Note that this might affect callers that could/would rely on the
> > atomicity semantics, but there are no guarantees of that for
> > smp_store_mb() mentioned anywhere, plus most archs use this anyway.
> > Thus we continue to be consistent with the memory-barriers.txt file,
> > and more importantly, maintain the semantics of the smp_ nature.
> 

> So with this patch, the whole thing becomes pointless, I feel. (Ok, so
> it may have been pointless before too, but at least before this patch
> it generated special code, now it doesn't). So why carry it along at
> all?

So I suppose this boils down to if: XCHG ends up being cheaper than
MOV+FENCE.

PeterA, any idea?
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1258392

On Tue, 27 Oct 2015, Peter Zijlstra wrote:

>On Wed, Oct 28, 2015 at 06:33:56AM +0900, Linus Torvalds wrote:
>> On Wed, Oct 28, 2015 at 4:53 AM, Davidlohr Bueso <dave@stgolabs.net> wrote:
>> >
>> > Note that this might affect callers that could/would rely on the
>> > atomicity semantics, but there are no guarantees of that for
>> > smp_store_mb() mentioned anywhere, plus most archs use this anyway.
>> > Thus we continue to be consistent with the memory-barriers.txt file,
>> > and more importantly, maintain the semantics of the smp_ nature.
>>
>
>> So with this patch, the whole thing becomes pointless, I feel. (Ok, so
>> it may have been pointless before too, but at least before this patch
>> it generated special code, now it doesn't). So why carry it along at
>> all?
>
>So I suppose this boils down to if: XCHG ends up being cheaper than
>MOV+FENCE.

If so, could this be the reasoning behind the mix and match of xchg and
MOV+FENCE? for different archs? This is from the days when set_mb() was
introduced. I wonder if it still even matters... I at least haven't seen
much difference in general workloads (I guess any difference would be
neglictible for practical matters). But could obviously be missing something.

>PeterA, any idea?

I suppose you're referring to hpa, Cc'ing him.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web