From: Alexander Sverdlin <alexander.sverdlin@nokia.com> To: Peter Zijlstra <peterz@infradead.org> Cc: Ingo Molnar <mingo@redhat.com>, Will Deacon <will@kernel.org>, linux-kernel@vger.kernel.org, Russell King <linux@armlinux.org.uk>, linux-arm-kernel@lists.infradead.org Subject: Re: [PATCH 1/2] qspinlock: Ensure writes are pushed out of core write buffer Date: Thu, 28 Jan 2021 08:42:03 +0100 [thread overview] Message-ID: <f1070b0f-7e3e-0f51-da7b-2ad9269b2ee6@nokia.com> (raw) In-Reply-To: <YBHsqIjop6X0Z+1c@hirez.programming.kicks-ass.net> Hi! On 27/01/2021 23:43, Peter Zijlstra wrote: > On Wed, Jan 27, 2021 at 09:01:08PM +0100, Alexander A Sverdlin wrote: >> From: Alexander Sverdlin <alexander.sverdlin@nokia.com> >> >> Ensure writes are pushed out of core write buffer to prevent waiting code >> on another cores from spinning longer than necessary. > Our smp_wmb() as defined does not have that property. You're relying on > some arch specific details which do not belong in common code. Yes, my intention was SYNCW on Octeon, which by accident is smp_wmb(). Do you think that the core write buffer is only Octeon feature and there will be no others? Should I re-implement arch_mcs_spin_lock_contended() for Octeon only, as it has been done for ARM? >> diff --git a/kernel/locking/mcs_spinlock.h b/kernel/locking/mcs_spinlock.h >> index 5e10153..10e497a 100644 >> --- a/kernel/locking/mcs_spinlock.h >> +++ b/kernel/locking/mcs_spinlock.h >> @@ -89,6 +89,11 @@ void mcs_spin_lock(struct mcs_spinlock **lock, struct mcs_spinlock *node) >> return; >> } >> WRITE_ONCE(prev->next, node); >> + /* >> + * This is necessary to make sure that the corresponding "while" in the >> + * mcs_spin_unlock() doesn't loop forever >> + */ > This comment is utterly inadequate, since it does not describe an > explicit ordering between two (or more) stores. > >> + smp_wmb(); >> >> /* Wait until the lock holder passes the lock down. */ >> arch_mcs_spin_lock_contended(&node->locked); >> diff --git a/kernel/locking/qspinlock.c b/kernel/locking/qspinlock.c >> index cbff6ba..577fe01 100644 >> --- a/kernel/locking/qspinlock.c >> +++ b/kernel/locking/qspinlock.c >> @@ -469,6 +469,12 @@ void queued_spin_lock_slowpath(struct qspinlock *lock, u32 val) >> >> /* Link @node into the waitqueue. */ >> WRITE_ONCE(prev->next, node); >> + /* >> + * This is necessary to make sure that the corresponding >> + * smp_cond_load_relaxed() below (running on another core) >> + * doesn't spin forever. >> + */ >> + smp_wmb(); > That's insane, cache coherency should not allow that to happen in the > first place. Our smp_wmb() cannot help with that. > -- Best regards, Alexander Sverdlin.
WARNING: multiple messages have this Message-ID (diff)
From: Alexander Sverdlin <alexander.sverdlin@nokia.com> To: Peter Zijlstra <peterz@infradead.org> Cc: Ingo Molnar <mingo@redhat.com>, Will Deacon <will@kernel.org>, linux-kernel@vger.kernel.org, linux-arm-kernel@lists.infradead.org, Russell King <linux@armlinux.org.uk> Subject: Re: [PATCH 1/2] qspinlock: Ensure writes are pushed out of core write buffer Date: Thu, 28 Jan 2021 08:42:03 +0100 [thread overview] Message-ID: <f1070b0f-7e3e-0f51-da7b-2ad9269b2ee6@nokia.com> (raw) In-Reply-To: <YBHsqIjop6X0Z+1c@hirez.programming.kicks-ass.net> Hi! On 27/01/2021 23:43, Peter Zijlstra wrote: > On Wed, Jan 27, 2021 at 09:01:08PM +0100, Alexander A Sverdlin wrote: >> From: Alexander Sverdlin <alexander.sverdlin@nokia.com> >> >> Ensure writes are pushed out of core write buffer to prevent waiting code >> on another cores from spinning longer than necessary. > Our smp_wmb() as defined does not have that property. You're relying on > some arch specific details which do not belong in common code. Yes, my intention was SYNCW on Octeon, which by accident is smp_wmb(). Do you think that the core write buffer is only Octeon feature and there will be no others? Should I re-implement arch_mcs_spin_lock_contended() for Octeon only, as it has been done for ARM? >> diff --git a/kernel/locking/mcs_spinlock.h b/kernel/locking/mcs_spinlock.h >> index 5e10153..10e497a 100644 >> --- a/kernel/locking/mcs_spinlock.h >> +++ b/kernel/locking/mcs_spinlock.h >> @@ -89,6 +89,11 @@ void mcs_spin_lock(struct mcs_spinlock **lock, struct mcs_spinlock *node) >> return; >> } >> WRITE_ONCE(prev->next, node); >> + /* >> + * This is necessary to make sure that the corresponding "while" in the >> + * mcs_spin_unlock() doesn't loop forever >> + */ > This comment is utterly inadequate, since it does not describe an > explicit ordering between two (or more) stores. > >> + smp_wmb(); >> >> /* Wait until the lock holder passes the lock down. */ >> arch_mcs_spin_lock_contended(&node->locked); >> diff --git a/kernel/locking/qspinlock.c b/kernel/locking/qspinlock.c >> index cbff6ba..577fe01 100644 >> --- a/kernel/locking/qspinlock.c >> +++ b/kernel/locking/qspinlock.c >> @@ -469,6 +469,12 @@ void queued_spin_lock_slowpath(struct qspinlock *lock, u32 val) >> >> /* Link @node into the waitqueue. */ >> WRITE_ONCE(prev->next, node); >> + /* >> + * This is necessary to make sure that the corresponding >> + * smp_cond_load_relaxed() below (running on another core) >> + * doesn't spin forever. >> + */ >> + smp_wmb(); > That's insane, cache coherency should not allow that to happen in the > first place. Our smp_wmb() cannot help with that. > -- Best regards, Alexander Sverdlin. _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
next prev parent reply other threads:[~2021-01-28 7:43 UTC|newest] Thread overview: 16+ messages / expand[flat|nested] mbox.gz Atom feed top 2021-01-27 20:01 [PATCH 1/2] qspinlock: Ensure writes are pushed out of core write buffer Alexander A Sverdlin 2021-01-27 20:01 ` Alexander A Sverdlin 2021-01-27 20:01 ` [PATCH 2/2] ARM: mcs_spinlock: Drop smp_wmb in arch_mcs_spin_lock_contended() Alexander A Sverdlin 2021-01-27 20:01 ` Alexander A Sverdlin 2021-01-27 22:22 ` Will Deacon 2021-01-27 22:22 ` Will Deacon 2021-01-27 22:21 ` [PATCH 1/2] qspinlock: Ensure writes are pushed out of core write buffer Will Deacon 2021-01-27 22:21 ` Will Deacon 2021-01-28 7:36 ` Alexander Sverdlin 2021-01-28 7:36 ` Alexander Sverdlin 2021-01-28 11:24 ` Peter Zijlstra 2021-01-28 11:24 ` Peter Zijlstra 2021-01-27 22:43 ` Peter Zijlstra 2021-01-27 22:43 ` Peter Zijlstra 2021-01-28 7:42 ` Alexander Sverdlin [this message] 2021-01-28 7:42 ` Alexander Sverdlin
Reply instructions: You may reply publicly to this message via plain-text email using any one of the following methods: * Save the following mbox file, import it into your mail client, and reply-to-all from there: mbox Avoid top-posting and favor interleaved quoting: https://en.wikipedia.org/wiki/Posting_style#Interleaved_style * Reply using the --to, --cc, and --in-reply-to switches of git-send-email(1): git send-email \ --in-reply-to=f1070b0f-7e3e-0f51-da7b-2ad9269b2ee6@nokia.com \ --to=alexander.sverdlin@nokia.com \ --cc=linux-arm-kernel@lists.infradead.org \ --cc=linux-kernel@vger.kernel.org \ --cc=linux@armlinux.org.uk \ --cc=mingo@redhat.com \ --cc=peterz@infradead.org \ --cc=will@kernel.org \ /path/to/YOUR_REPLY https://kernel.org/pub/software/scm/git/docs/git-send-email.html * If your mail client supports setting the In-Reply-To header via mailto: links, try the mailto: linkBe sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.