On Wed, Jul 15, 2020 at 10:05:07AM +0800, Leo Yan wrote:
> From: Peter Zijlstra <[email protected]>
>
...
>
> Provide struct clock_read_data and two (seqcount) helpers so that
> architectures (arm64 in specific) can expose the numbers to userspace.
>
...
>
> +struct clock_read_data *sched_clock_read_begin(unsigned int *seq)
> +{
> + *seq = raw_read_seqcount(&cd.seq);
> + return cd.read_data + (*seq & 1);
> +}
> +
...
Hmm, this seqcount_t is actually a latch seqcount. I know the original
code also used raw_read_seqcount(), but while at it, let's use the
proper read API for seqcount_t latchers: raw_read_seqcount_latch().
raw_read_seqcount_latch() has no read memory barrier though, and a
suspicious claim that READ_ONCE() pairs with an smp_wmb() (??). But if
its implementation is wrong, let's fix it there instead.
Thanks,
--
Ahmed S. Darwish
Linutronix GmbH
On Wed, Jul 15, 2020 at 07:56:50AM +0200, Ahmed S. Darwish wrote:
> On Wed, Jul 15, 2020 at 10:05:07AM +0800, Leo Yan wrote:
> > From: Peter Zijlstra <[email protected]>
> >
> ...
> >
> > Provide struct clock_read_data and two (seqcount) helpers so that
> > architectures (arm64 in specific) can expose the numbers to userspace.
> >
> ...
> >
> > +struct clock_read_data *sched_clock_read_begin(unsigned int *seq)
> > +{
> > + *seq = raw_read_seqcount(&cd.seq);
> > + return cd.read_data + (*seq & 1);
> > +}
> > +
> ...
>
> Hmm, this seqcount_t is actually a latch seqcount. I know the original
> code also used raw_read_seqcount(), but while at it, let's use the
> proper read API for seqcount_t latchers: raw_read_seqcount_latch().
>
> raw_read_seqcount_latch() has no read memory barrier though, and a
> suspicious claim that READ_ONCE() pairs with an smp_wmb() (??). But if
> its implementation is wrong, let's fix it there instead.
It's supposed to be a dependent load, so READ_ONCE() is sufficient.
Except, of course, the C standard has other ideas, so a compiler is
allowed to wreck that, but they mostly don't :-)
On Wed, Jul 15, 2020 at 10:12:22AM +0200, Peter Zijlstra wrote:
> On Wed, Jul 15, 2020 at 07:56:50AM +0200, Ahmed S. Darwish wrote:
> > On Wed, Jul 15, 2020 at 10:05:07AM +0800, Leo Yan wrote:
> > > From: Peter Zijlstra <[email protected]>
> > >
> > ...
> > >
> > > Provide struct clock_read_data and two (seqcount) helpers so that
> > > architectures (arm64 in specific) can expose the numbers to userspace.
> > >
> > ...
> > >
> > > +struct clock_read_data *sched_clock_read_begin(unsigned int *seq)
> > > +{
> > > + *seq = raw_read_seqcount(&cd.seq);
> > > + return cd.read_data + (*seq & 1);
> > > +}
> > > +
> > ...
> >
> > Hmm, this seqcount_t is actually a latch seqcount. I know the original
> > code also used raw_read_seqcount(), but while at it, let's use the
> > proper read API for seqcount_t latchers: raw_read_seqcount_latch().
> >
> > raw_read_seqcount_latch() has no read memory barrier though, and a
> > suspicious claim that READ_ONCE() pairs with an smp_wmb() (??). But if
> > its implementation is wrong, let's fix it there instead.
>
> It's supposed to be a dependent load, so READ_ONCE() is sufficient.
> Except, of course, the C standard has other ideas, so a compiler is
> allowed to wreck that, but they mostly don't :-)
Also see:
https://lkml.kernel.org/r/[email protected]
On Wed, Jul 15, 2020 at 10:14:43AM +0200, [email protected] wrote:
> On Wed, Jul 15, 2020 at 10:12:22AM +0200, Peter Zijlstra wrote:
> > On Wed, Jul 15, 2020 at 07:56:50AM +0200, Ahmed S. Darwish wrote:
> > > On Wed, Jul 15, 2020 at 10:05:07AM +0800, Leo Yan wrote:
> > > > From: Peter Zijlstra <[email protected]>
> > > >
> > > ...
> > > >
> > > > Provide struct clock_read_data and two (seqcount) helpers so that
> > > > architectures (arm64 in specific) can expose the numbers to userspace.
> > > >
> > > ...
> > > >
> > > > +struct clock_read_data *sched_clock_read_begin(unsigned int *seq)
> > > > +{
> > > > + *seq = raw_read_seqcount(&cd.seq);
> > > > + return cd.read_data + (*seq & 1);
> > > > +}
> > > > +
> > > ...
> > >
> > > Hmm, this seqcount_t is actually a latch seqcount. I know the original
> > > code also used raw_read_seqcount(), but while at it, let's use the
> > > proper read API for seqcount_t latchers: raw_read_seqcount_latch().
> > >
> > > raw_read_seqcount_latch() has no read memory barrier though, and a
> > > suspicious claim that READ_ONCE() pairs with an smp_wmb() (??). But if
> > > its implementation is wrong, let's fix it there instead.
> >
> > It's supposed to be a dependent load, so READ_ONCE() is sufficient.
> > Except, of course, the C standard has other ideas, so a compiler is
> > allowed to wreck that, but they mostly don't :-)
>
> Also see:
>
> https://lkml.kernel.org/r/[email protected]
Oh, spot on.
Can we then please replace the raw_read_seqcount(), in the original
patch which started this discussion, with raw_read_seqcount_latch()?
I see that you already did something similar for timekeeping.c:
7fc26327b756 ("seqlock: Introduce raw_read_seqcount_latch()").
Confession time: I have an internal patch series which introduces
seqcount_latch_t. Having a standardized set of accessors for the
seqcount latch read and write paths will make everything much more
streamlined :-)
Thanks,
--
Ahmed S. Darwish
Linutronix GmbH
On Wed, Jul 15, 2020 at 11:23:45AM +0200, Ahmed S. Darwish wrote:
>
> Can we then please replace the raw_read_seqcount(), in the original
> patch which started this discussion, with raw_read_seqcount_latch()?
Separate patch please, but ACK for making the change.
sched_clock uses seqcount_t latching to switch between two storage
places protected by the sequence counter. This allows it to have
interruptible, NMI-safe, seqcount_t write side critical sections.
Since 7fc26327b756 ("seqlock: Introduce raw_read_seqcount_latch()"),
raw_read_seqcount_latch() became the standardized way for seqcount_t
latch read paths. Due to the dependent load, it also has one read
memory barrier less than the currently used raw_read_seqcount() API.
Use raw_read_seqcount_latch() for the seqcount_t latch read path.
Link: https://lkml.kernel.org/r/[email protected]
Link: https://lkml.kernel.org/r/[email protected]
References: 1809bfa44e10 ("timers, sched/clock: Avoid deadlock during read from NMI")
Signed-off-by: Ahmed S. Darwish <[email protected]>
---
kernel/time/sched_clock.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/kernel/time/sched_clock.c b/kernel/time/sched_clock.c
index fa3f800d7d76..ea007928d681 100644
--- a/kernel/time/sched_clock.c
+++ b/kernel/time/sched_clock.c
@@ -100,7 +100,7 @@ unsigned long long notrace sched_clock(void)
struct clock_read_data *rd;
do {
- seq = raw_read_seqcount(&cd.seq);
+ seq = raw_read_seqcount_latch(&cd.seq);
rd = cd.read_data + (seq & 1);
cyc = (rd->read_sched_clock() - rd->epoch_cyc) &
--
2.20.1
On Wed, Jul 15, 2020 at 11:29:26PM +0800, Leo Yan wrote:
> Hi Peter, Ahmed,
>
> On Wed, Jul 15, 2020 at 01:59:01PM +0200, Ahmed S. Darwish wrote:
> > sched_clock uses seqcount_t latching to switch between two storage
> > places protected by the sequence counter. This allows it to have
> > interruptible, NMI-safe, seqcount_t write side critical sections.
> >
> > Since 7fc26327b756 ("seqlock: Introduce raw_read_seqcount_latch()"),
> > raw_read_seqcount_latch() became the standardized way for seqcount_t
> > latch read paths. Due to the dependent load, it also has one read
> > memory barrier less than the currently used raw_read_seqcount() API.
> >
> > Use raw_read_seqcount_latch() for the seqcount_t latch read path.
> >
> > Link: https://lkml.kernel.org/r/[email protected]
> > Link: https://lkml.kernel.org/r/[email protected]
> > References: 1809bfa44e10 ("timers, sched/clock: Avoid deadlock during read from NMI")
> > Signed-off-by: Ahmed S. Darwish <[email protected]>
> > ---
> > kernel/time/sched_clock.c | 2 +-
> > 1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/kernel/time/sched_clock.c b/kernel/time/sched_clock.c
> > index fa3f800d7d76..ea007928d681 100644
> > --- a/kernel/time/sched_clock.c
> > +++ b/kernel/time/sched_clock.c
> > @@ -100,7 +100,7 @@ unsigned long long notrace sched_clock(void)
> > struct clock_read_data *rd;
> >
> > do {
> > - seq = raw_read_seqcount(&cd.seq);
> > + seq = raw_read_seqcount_latch(&cd.seq);
>
> Understand this is doing the same thing with __ktime_get_fast_ns() and
> I saw Peter acked to make change for this.
>
> Just want to confirm, since this patch introduces conflict with the
> patch set "arm64: perf: Proper cap_user_time* support" [1], I should
> rebase the patch set on top of this patch, right?
Or rebase this patch on top of yours and include it, either way.
The following commit has been merged into the locking/core branch of tip:
Commit-ID: 58faf20a086bd34f91983609e26eac3d5fe76be3
Gitweb: https://git.kernel.org/tip/58faf20a086bd34f91983609e26eac3d5fe76be3
Author: Ahmed S. Darwish <[email protected]>
AuthorDate: Thu, 27 Aug 2020 13:40:37 +02:00
Committer: Peter Zijlstra <[email protected]>
CommitterDate: Thu, 10 Sep 2020 11:19:28 +02:00
time/sched_clock: Use raw_read_seqcount_latch() during suspend
sched_clock uses seqcount_t latching to switch between two storage
places protected by the sequence counter. This allows it to have
interruptible, NMI-safe, seqcount_t write side critical sections.
Since 7fc26327b756 ("seqlock: Introduce raw_read_seqcount_latch()"),
raw_read_seqcount_latch() became the standardized way for seqcount_t
latch read paths. Due to the dependent load, it has one read memory
barrier less than the currently used raw_read_seqcount() API.
Use raw_read_seqcount_latch() for the suspend path.
Commit aadd6e5caaac ("time/sched_clock: Use raw_read_seqcount_latch()")
missed changing that instance of raw_read_seqcount().
References: 1809bfa44e10 ("timers, sched/clock: Avoid deadlock during read from NMI")
Signed-off-by: Ahmed S. Darwish <[email protected]>
Signed-off-by: Peter Zijlstra (Intel) <[email protected]>
Link: https://lkml.kernel.org/r/[email protected]
---
kernel/time/sched_clock.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/kernel/time/sched_clock.c b/kernel/time/sched_clock.c
index 1c03eec..8c6b5fe 100644
--- a/kernel/time/sched_clock.c
+++ b/kernel/time/sched_clock.c
@@ -258,7 +258,7 @@ void __init generic_sched_clock_init(void)
*/
static u64 notrace suspended_sched_clock_read(void)
{
- unsigned int seq = raw_read_seqcount(&cd.seq);
+ unsigned int seq = raw_read_seqcount_latch(&cd.seq);
return cd.read_data[seq & 1].epoch_cyc;
}