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

Groups > linux.kernel > #1302740 > unrolled thread

[PATCH 1/2] kernel, timekeeping, add trylock option to ktime_get_with_offset()

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 1/2] kernel, timekeeping, add trylock option to ktime_get_with_offset() Prarit Bhargava <prarit@redhat.com> - 2016-01-06 14:10 +0100
    Re: [PATCH 1/2] kernel, timekeeping, add trylock option to  ktime_get_with_offset() Jiri Bohac <jbohac@suse.cz> - 2016-01-06 17:10 +0100
      Re: [PATCH 1/2] kernel, timekeeping, add trylock option to ktime_get_with_offset() Prarit Bhargava <prarit@redhat.com> - 2016-01-06 17:30 +0100
    Re: [PATCH 1/2] kernel, timekeeping, add trylock option to  ktime_get_with_offset() Petr Mladek <pmladek@suse.com> - 2016-01-06 17:30 +0100
    Re: [PATCH 1/2] kernel, timekeeping, add trylock option to ktime_get_with_offset() John Stultz <john.stultz@linaro.org> - 2016-01-06 18:30 +0100
      Re: [PATCH 1/2] kernel, timekeeping, add trylock option to  ktime_get_with_offset() Thomas Gleixner <tglx@linutronix.de> - 2016-01-06 18:40 +0100
        Re: [PATCH 1/2] kernel, timekeeping, add trylock option to ktime_get_with_offset() Prarit Bhargava <prarit@redhat.com> - 2016-01-06 19:10 +0100
          Re: [PATCH 1/2] kernel, timekeeping, add trylock option to  ktime_get_with_offset() Thomas Gleixner <tglx@linutronix.de> - 2016-01-06 19:20 +0100
        Re: [PATCH 1/2] kernel, timekeeping, add trylock option to ktime_get_with_offset() John Stultz <john.stultz@linaro.org> - 2016-01-06 20:10 +0100
          Re: [PATCH 1/2] kernel, timekeeping, add trylock option to ktime_get_with_offset() Prarit Bhargava <prarit@redhat.com> - 2016-01-06 20:10 +0100
      Re: [PATCH 1/2] kernel, timekeeping, add trylock option to ktime_get_with_offset() John Stultz <john.stultz@linaro.org> - 2016-01-06 18:40 +0100
        Re: [PATCH 1/2] kernel, timekeeping, add trylock option to ktime_get_with_offset() Prarit Bhargava <prarit@redhat.com> - 2016-01-06 19:10 +0100
          Re: [PATCH 1/2] kernel, timekeeping, add trylock option to  ktime_get_with_offset() Thomas Gleixner <tglx@linutronix.de> - 2016-01-06 19:20 +0100

#1302740 — [PATCH 1/2] kernel, timekeeping, add trylock option to ktime_get_with_offset()

This is a timekeeping staging patch for the printk() timestamp
functionality that adds a trylock option for the timekeeping_lock() to
ktime_get_with_offset().  When trylock is 1, calls to
ktime_get_with_offset() will return return a ktime of 0 if the
timekeeping_lock is locked.

This patch adds ktime_try_real(), ktime_try_boot(), and ktime_try_tai() as
wrapper functions around ktime_get_with_offset() with trylock = 1, and
modifies other callers to call ktime_get_with_offset() with trylock = 0.

Cc: John Stultz <john.stultz@linaro.org>
Cc: Thomas Gleixner <tglx@linutronix.de>
Cc: Ingo Molnar <mingo@kernel.org>
Cc: Xunlei Pang <pang.xunlei@linaro.org>
Cc: Peter Zijlstra <peterz@infradead.org>
Cc: Baolin Wang <baolin.wang@linaro.org>
Cc: Arnd Bergmann <arnd@arndb.de>
Signed-off-by: Prarit Bhargava <prarit@redhat.com>
---
 include/linux/timekeeping.h |   50 +++++++++++++++++++++++++++++++++++++++----
 kernel/time/timekeeping.c   |   15 ++++++++++++-
 2 files changed, 60 insertions(+), 5 deletions(-)

diff --git a/include/linux/timekeeping.h b/include/linux/timekeeping.h
index ec89d84..4f47352 100644
--- a/include/linux/timekeeping.h
+++ b/include/linux/timekeeping.h
@@ -166,7 +166,7 @@ enum tk_offsets {
 };
 
 extern ktime_t ktime_get(void);
-extern ktime_t ktime_get_with_offset(enum tk_offsets offs);
+extern ktime_t ktime_get_with_offset(enum tk_offsets offs, int trylock);
 extern ktime_t ktime_mono_to_any(ktime_t tmono, enum tk_offsets offs);
 extern ktime_t ktime_get_raw(void);
 extern u32 ktime_get_resolution_ns(void);
@@ -176,7 +176,16 @@ extern u32 ktime_get_resolution_ns(void);
  */
 static inline ktime_t ktime_get_real(void)
 {
-	return ktime_get_with_offset(TK_OFFS_REAL);
+	return ktime_get_with_offset(TK_OFFS_REAL, 0);
+}
+
+/**
+ * ktime_try_real - same as ktime_get_real, except return 0 if timekeeping is
+ * locked.
+ */
+static inline ktime_t ktime_try_real(void)
+{
+	return ktime_get_with_offset(TK_OFFS_REAL, 1);
 }
 
 /**
@@ -187,7 +196,16 @@ static inline ktime_t ktime_get_real(void)
  */
 static inline ktime_t ktime_get_boottime(void)
 {
-	return ktime_get_with_offset(TK_OFFS_BOOT);
+	return ktime_get_with_offset(TK_OFFS_BOOT, 0);
+}
+
+/**
+ * ktime_try_boottime - same as ktime_get_bootime, except return 0 if
+ * timekeeping is locked.
+ */
+static inline ktime_t ktime_try_boottime(void)
+{
+	return ktime_get_with_offset(TK_OFFS_BOOT, 1);
 }
 
 /**
@@ -195,7 +213,16 @@ static inline ktime_t ktime_get_boottime(void)
  */
 static inline ktime_t ktime_get_clocktai(void)
 {
-	return ktime_get_with_offset(TK_OFFS_TAI);
+	return ktime_get_with_offset(TK_OFFS_TAI, 0);
+}
+
+/**
+ * ktime_try_clocktai - same as ktime_get_clocktai, except return 0 if
+ * timekeeping is locked.
+ */
+static inline ktime_t ktime_try_clocktai(void)
+{
+	return ktime_get_with_offset(TK_OFFS_TAI, 1);
 }
 
 /**
@@ -216,16 +243,31 @@ static inline u64 ktime_get_real_ns(void)
 	return ktime_to_ns(ktime_get_real());
 }
 
+static inline u64 ktime_try_real_ns(void)
+{
+	return ktime_to_ns(ktime_try_real());
+}
+
 static inline u64 ktime_get_boot_ns(void)
 {
 	return ktime_to_ns(ktime_get_boottime());
 }
 
+static inline u64 ktime_try_boot_ns(void)
+{
+	return ktime_to_ns(ktime_try_boottime());
+}
+
 static inline u64 ktime_get_tai_ns(void)
 {
 	return ktime_to_ns(ktime_get_clocktai());
 }
 
+static inline u64 ktime_try_tai_ns(void)
+{
+	return ktime_to_ns(ktime_try_clocktai());
+}
+
 static inline u64 ktime_get_raw_ns(void)
 {
 	return ktime_to_ns(ktime_get_raw());
diff --git a/kernel/time/timekeeping.c b/kernel/time/timekeeping.c
index d563c19..6e2cbeb 100644
--- a/kernel/time/timekeeping.c
+++ b/kernel/time/timekeeping.c
@@ -44,6 +44,8 @@ static struct {
 static DEFINE_RAW_SPINLOCK(timekeeper_lock);
 static struct timekeeper shadow_timekeeper;
 
+/* printk may call ktime_get_with_offset() before timekeeping is initialized. */
+static int timekeeping_initialized;
 /**
  * struct tk_fast - NMI safe timekeeper
  * @seq:	Sequence counter for protecting updates. The lowest bit
@@ -705,15 +707,22 @@ static ktime_t *offsets[TK_OFFS_MAX] = {
 	[TK_OFFS_TAI]	= &tk_core.timekeeper.offs_tai,
 };
 
-ktime_t ktime_get_with_offset(enum tk_offsets offs)
+ktime_t ktime_get_with_offset(enum tk_offsets offs, int trylock)
 {
 	struct timekeeper *tk = &tk_core.timekeeper;
 	unsigned int seq;
 	ktime_t base, *offset = offsets[offs];
 	s64 nsecs;
+	unsigned long flags = 0;
+
+	if (unlikely(!timekeeping_initialized))
+		return ktime_set(0, 0);
 
 	WARN_ON(timekeeping_suspended);
 
+	if (trylock && !raw_spin_trylock_irqsave(&timekeeper_lock, flags))
+		return ktime_set(KTIME_MAX, 0);
+
 	do {
 		seq = read_seqcount_begin(&tk_core.seq);
 		base = ktime_add(tk->tkr_mono.base, *offset);
@@ -721,6 +730,9 @@ ktime_t ktime_get_with_offset(enum tk_offsets offs)
 
 	} while (read_seqcount_retry(&tk_core.seq, seq));
 
+	if (trylock)
+		raw_spin_unlock_irqrestore(&timekeeper_lock, flags);
+
 	return ktime_add_ns(base, nsecs);
 
 }
@@ -1255,6 +1267,7 @@ void __init timekeeping_init(void)
 
 	write_seqcount_end(&tk_core.seq);
 	raw_spin_unlock_irqrestore(&timekeeper_lock, flags);
+	timekeeping_initialized = 1;
 }
 
 /* time in seconds when suspend began for persistent clock */
-- 
1.7.9.3

--
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]

#1302868 — Re: [PATCH 1/2] kernel, timekeeping, add trylock option to ktime_get_with_offset()

On Wed, Jan 06, 2016 at 08:00:33AM -0500, Prarit Bhargava wrote:
> -ktime_t ktime_get_with_offset(enum tk_offsets offs)
> +ktime_t ktime_get_with_offset(enum tk_offsets offs, int trylock)
>  {
>  	struct timekeeper *tk = &tk_core.timekeeper;
>  	unsigned int seq;
>  	ktime_t base, *offset = offsets[offs];
>  	s64 nsecs;
> +	unsigned long flags = 0;
> +
> +	if (unlikely(!timekeeping_initialized))
> +		return ktime_set(0, 0);
>  
>  	WARN_ON(timekeeping_suspended);
>  
> +	if (trylock && !raw_spin_trylock_irqsave(&timekeeper_lock, flags))
> +		return ktime_set(KTIME_MAX, 0);
> +

Are you trying to avoid a deadlock caused by calling printk() with
timekeeper_lock locked?

I believe this is already unsafe, as explained in the commit log
of 6d9bcb62 (timekeeping: use printk_deferred when holding
timekeeping seqlock).

So directly calling ktime_get() from printk would just turn a
rare deadlock into a certain one - perhaps a good thing?


-- 
Jiri Bohac <jbohac@suse.cz>
SUSE Labs, SUSE CZ

--
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]

#1302880

On 01/06/2016 11:04 AM, Jiri Bohac wrote:
> On Wed, Jan 06, 2016 at 08:00:33AM -0500, Prarit Bhargava wrote:
>> -ktime_t ktime_get_with_offset(enum tk_offsets offs)
>> +ktime_t ktime_get_with_offset(enum tk_offsets offs, int trylock)
>>  {
>>  	struct timekeeper *tk = &tk_core.timekeeper;
>>  	unsigned int seq;
>>  	ktime_t base, *offset = offsets[offs];
>>  	s64 nsecs;
>> +	unsigned long flags = 0;
>> +
>> +	if (unlikely(!timekeeping_initialized))
>> +		return ktime_set(0, 0);
>>  
>>  	WARN_ON(timekeeping_suspended);
>>  
>> +	if (trylock && !raw_spin_trylock_irqsave(&timekeeper_lock, flags))
>> +		return ktime_set(KTIME_MAX, 0);
>> +
> 
> Are you trying to avoid a deadlock caused by calling printk() with
> timekeeper_lock locked?

Not exactly.  When I initially sent this as a RFE to jstultz he pointed out that
if CPU A had acquired the timekeeper_lock (and therefore incremented tk_core.seq
for a write), and CPU B panicked, no output would occur because the reads of
tk_core.seq would spin indefinitely.

> 
> I believe this is already unsafe, as explained in the commit log
> of 6d9bcb62 (timekeeping: use printk_deferred when holding
> timekeeping seqlock).

Hmm ... John Stultz, any suggestions here?

P.

> 
> So directly calling ktime_get() from printk would just turn a
> rare deadlock into a certain one - perhaps a good thing?
> 
> 
--
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]

#1302882 — Re: [PATCH 1/2] kernel, timekeeping, add trylock option to ktime_get_with_offset()

On Wed 2016-01-06 08:00:33, Prarit Bhargava wrote:
> This is a timekeeping staging patch for the printk() timestamp
> functionality that adds a trylock option for the timekeeping_lock() to
> ktime_get_with_offset().  When trylock is 1, calls to
> ktime_get_with_offset() will return return a ktime of 0 if the
> timekeeping_lock is locked.

If I get it correctly, it returns 0 when timekeeping is not
initialized. But it returns TIME_MAX_NS when the lock is taken.
Where TIME_MAX_NS is defined in the 2nd patch.

> This patch adds ktime_try_real(), ktime_try_boot(), and ktime_try_tai() as
> wrapper functions around ktime_get_with_offset() with trylock = 1, and
> modifies other callers to call ktime_get_with_offset() with trylock = 0.
> 
> ---
>  include/linux/timekeeping.h |   50 +++++++++++++++++++++++++++++++++++++++----
>  kernel/time/timekeeping.c   |   15 ++++++++++++-
>  2 files changed, 60 insertions(+), 5 deletions(-)
> 
> diff --git a/include/linux/timekeeping.h b/include/linux/timekeeping.h
> index ec89d84..4f47352 100644
> --- a/include/linux/timekeeping.h
> +++ b/include/linux/timekeeping.h
> @@ -166,7 +166,7 @@ enum tk_offsets {
>  };
>  
>  extern ktime_t ktime_get(void);
> -extern ktime_t ktime_get_with_offset(enum tk_offsets offs);
> +extern ktime_t ktime_get_with_offset(enum tk_offsets offs, int trylock);
>  extern ktime_t ktime_mono_to_any(ktime_t tmono, enum tk_offsets offs);
>  extern ktime_t ktime_get_raw(void);
>  extern u32 ktime_get_resolution_ns(void);
> @@ -176,7 +176,16 @@ extern u32 ktime_get_resolution_ns(void);
>   */
>  static inline ktime_t ktime_get_real(void)
>  {
> -	return ktime_get_with_offset(TK_OFFS_REAL);
> +	return ktime_get_with_offset(TK_OFFS_REAL, 0);
> +}
> +
> +/**
> + * ktime_try_real - same as ktime_get_real, except return 0 if timekeeping is
> + * locked.
> + */
> +static inline ktime_t ktime_try_real(void)

I would call this ktime_try_get_real() to make it clear.


> +{
> +	return ktime_get_with_offset(TK_OFFS_REAL, 1);
>  }
>  
[...]

>  static inline u64 ktime_get_raw_ns(void)
>  {
>  	return ktime_to_ns(ktime_get_raw());
> diff --git a/kernel/time/timekeeping.c b/kernel/time/timekeeping.c
> index d563c19..6e2cbeb 100644
> --- a/kernel/time/timekeeping.c
> +++ b/kernel/time/timekeeping.c
> @@ -44,6 +44,8 @@ static struct {
>  static DEFINE_RAW_SPINLOCK(timekeeper_lock);
>  static struct timekeeper shadow_timekeeper;
>  
> +/* printk may call ktime_get_with_offset() before timekeeping is initialized. */
> +static int timekeeping_initialized;
>  /**
>   * struct tk_fast - NMI safe timekeeper
>   * @seq:	Sequence counter for protecting updates. The lowest bit
> @@ -705,15 +707,22 @@ static ktime_t *offsets[TK_OFFS_MAX] = {
>  	[TK_OFFS_TAI]	= &tk_core.timekeeper.offs_tai,
>  };
>  
> -ktime_t ktime_get_with_offset(enum tk_offsets offs)
> +ktime_t ktime_get_with_offset(enum tk_offsets offs, int trylock)
>  {
>  	struct timekeeper *tk = &tk_core.timekeeper;
>  	unsigned int seq;
>  	ktime_t base, *offset = offsets[offs];
>  	s64 nsecs;
> +	unsigned long flags = 0;
> +
> +	if (unlikely(!timekeeping_initialized))
> +		return ktime_set(0, 0);



>  	WARN_ON(timekeeping_suspended);
>  
> +	if (trylock && !raw_spin_trylock_irqsave(&timekeeper_lock, flags))
> +		return ktime_set(KTIME_MAX, 0);
> +

I guess that you want to avoid a deadlock with this. I mean that you
want to survive when you call, for example, ktime_try_tai_ns() from
inside timekeeping_set_tai_offset(). Am I right?

One problem is that it will fail even when the lock is taken from
another CPU and the deadlock is not real. It probably is not a big
issue for printk() because currently used local_clock() is far from perfect
but...

Another problem is that it will block writers. This might be solved
if you try only one while cycle instead of taking the lock.
I mean to do something like:

ktime_t ktime_get_with_offset(enum tk_offsets offs, int try_once)
{
	struct timekeeper *tk = &tk_core.timekeeper;
	unsigned int seq;
	int retry;
	ktime_t base, *offset = offsets[offs];
	s64 nsecs;

	WARN_ON(timekeeping_suspended);

	do {
		seq = read_seqcount_begin(&tk_core.seq);
		base = ktime_add(tk->tkr_mono.base, *offset);
		nsecs = timekeeping_get_ns(&tk->tkr_mono);
		retry = read_seqcount_retry(&tk_core.seq, seq));
	} while (retry && !try_once);

	if (try_once && retry)
		return ktime_set(KTIME_MAX, 0);

	return ktime_add_ns(base, nsecs);

}

Another question is if you really need to distinguish between
non-initialized and locked state. You might always return zero
time if you do not know. It will things easier.


>  	do {
>  		seq = read_seqcount_begin(&tk_core.seq);
>  		base = ktime_add(tk->tkr_mono.base, *offset);
> @@ -721,6 +730,9 @@ ktime_t ktime_get_with_offset(enum tk_offsets offs)
>  
>  	} while (read_seqcount_retry(&tk_core.seq, seq));
>  
> +	if (trylock)
> +		raw_spin_unlock_irqrestore(&timekeeper_lock, flags);
> +
>  	return ktime_add_ns(base, nsecs);
>  

Best Regards,
Petr
--
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]

#1302909

On Wed, Jan 6, 2016 at 5:00 AM, Prarit Bhargava <prarit@redhat.com> wrote:
> -ktime_t ktime_get_with_offset(enum tk_offsets offs)
> +ktime_t ktime_get_with_offset(enum tk_offsets offs, int trylock)
>  {
>         struct timekeeper *tk = &tk_core.timekeeper;
>         unsigned int seq;
>         ktime_t base, *offset = offsets[offs];
>         s64 nsecs;
> +       unsigned long flags = 0;
> +
> +       if (unlikely(!timekeeping_initialized))
> +               return ktime_set(0, 0);
>
>         WARN_ON(timekeeping_suspended);
>
> +       if (trylock && !raw_spin_trylock_irqsave(&timekeeper_lock, flags))
> +               return ktime_set(KTIME_MAX, 0);

Wait.. this doesn't make sense. The timekeeper lock is only for reading.

What I was suggesting to you off line is to have something that avoids
spinning on the seqcounter should if a bug occurs and we IPI all the
cpus, that we don't deadlock or block any printk messages.


> +
>         do {
>                 seq = read_seqcount_begin(&tk_core.seq);
>                 base = ktime_add(tk->tkr_mono.base, *offset);
> @@ -721,6 +730,9 @@ ktime_t ktime_get_with_offset(enum tk_offsets offs)
>
>         } while (read_seqcount_retry(&tk_core.seq, seq));

So instead of the do/while() loop above... Something closer to:

int __ktime_get_with_offset(enum tk_offsets offs, ktime_t* base, s64 *nsec)
{
       unsigned int seq;

       seq = read_seqcount_begin(&tk_core.seq);
       *base = ktime_add(tk->tkr_mono.base, *offset);
       *nsecs = timekeeping_get_ns(&tk->tkr_mono);

       return read_seqcount_retry(&tk_core.seq, seq);
}

Then ktime_get_with_offset() would just call:
      ...
      while(__ktime_get_with_offset(offs, &base, &nsecs))
                          /*spin*/;
      return ktime_add_ns(base,nsecs);

Then you add a simple:
int ktime_try_get_with_offset(enum tk_offsets offs, ktime_t* ret)
{
   ...
    if (__ktime_get_with_offset(offs, &base, &nsecs))
           return -EAGAIN;
    *ret = ktime_add_ns(base,nsecs);
     return 0;
}


thanks
-john
--
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]

#1302912 — Re: [PATCH 1/2] kernel, timekeeping, add trylock option to ktime_get_with_offset()

On Wed, 6 Jan 2016, John Stultz wrote:
> On Wed, Jan 6, 2016 at 5:00 AM, Prarit Bhargava <prarit@redhat.com> wrote:
> > -ktime_t ktime_get_with_offset(enum tk_offsets offs)
> > +ktime_t ktime_get_with_offset(enum tk_offsets offs, int trylock)
> >  {
> >         struct timekeeper *tk = &tk_core.timekeeper;
> >         unsigned int seq;
> >         ktime_t base, *offset = offsets[offs];
> >         s64 nsecs;
> > +       unsigned long flags = 0;
> > +
> > +       if (unlikely(!timekeeping_initialized))
> > +               return ktime_set(0, 0);
> >
> >         WARN_ON(timekeeping_suspended);
> >
> > +       if (trylock && !raw_spin_trylock_irqsave(&timekeeper_lock, flags))
> > +               return ktime_set(KTIME_MAX, 0);
> 
> Wait.. this doesn't make sense. The timekeeper lock is only for reading.
> 
> What I was suggesting to you off line is to have something that avoids
> spinning on the seqcounter should if a bug occurs and we IPI all the
> cpus, that we don't deadlock or block any printk messages.

We could also extend the fast timekeeper with boot/real/tai extensions and use
that for printk. You can use ktime_get_mono_fast_ns() today.

Thanks,

	tglx

--
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]

#1302940

On 01/06/2016 12:34 PM, Thomas Gleixner wrote:
> On Wed, 6 Jan 2016, John Stultz wrote:
>> On Wed, Jan 6, 2016 at 5:00 AM, Prarit Bhargava <prarit@redhat.com> wrote:
>>> -ktime_t ktime_get_with_offset(enum tk_offsets offs)
>>> +ktime_t ktime_get_with_offset(enum tk_offsets offs, int trylock)
>>>  {
>>>         struct timekeeper *tk = &tk_core.timekeeper;
>>>         unsigned int seq;
>>>         ktime_t base, *offset = offsets[offs];
>>>         s64 nsecs;
>>> +       unsigned long flags = 0;
>>> +
>>> +       if (unlikely(!timekeeping_initialized))
>>> +               return ktime_set(0, 0);
>>>
>>>         WARN_ON(timekeeping_suspended);
>>>
>>> +       if (trylock && !raw_spin_trylock_irqsave(&timekeeper_lock, flags))
>>> +               return ktime_set(KTIME_MAX, 0);
>>
>> Wait.. this doesn't make sense. The timekeeper lock is only for reading.
>>
>> What I was suggesting to you off line is to have something that avoids
>> spinning on the seqcounter should if a bug occurs and we IPI all the
>> cpus, that we don't deadlock or block any printk messages.
> 
> We could also extend the fast timekeeper with boot/real/tai extensions and use
> that for printk. You can use ktime_get_mono_fast_ns() today.
> 

Thanks tglx -- I thought about doing that but was put off by the comments
in __ktime_get_fast_ns() which point out that we could see backwards time
stamps.  But I see your point -- I could do the same "last_time_stamp" check
and use "??" in the output.

That's a far better approach here, and unless John has any objections I'll
go with that.

P.


> Thanks,
> 
> 	tglx
> 
--
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]

#1302956 — Re: [PATCH 1/2] kernel, timekeeping, add trylock option to ktime_get_with_offset()

On Wed, 6 Jan 2016, Prarit Bhargava wrote:
> On 01/06/2016 12:34 PM, Thomas Gleixner wrote:
> > On Wed, 6 Jan 2016, John Stultz wrote:
> >> On Wed, Jan 6, 2016 at 5:00 AM, Prarit Bhargava <prarit@redhat.com> wrote:
> >>> -ktime_t ktime_get_with_offset(enum tk_offsets offs)
> >>> +ktime_t ktime_get_with_offset(enum tk_offsets offs, int trylock)
> >>>  {
> >>>         struct timekeeper *tk = &tk_core.timekeeper;
> >>>         unsigned int seq;
> >>>         ktime_t base, *offset = offsets[offs];
> >>>         s64 nsecs;
> >>> +       unsigned long flags = 0;
> >>> +
> >>> +       if (unlikely(!timekeeping_initialized))
> >>> +               return ktime_set(0, 0);
> >>>
> >>>         WARN_ON(timekeeping_suspended);
> >>>
> >>> +       if (trylock && !raw_spin_trylock_irqsave(&timekeeper_lock, flags))
> >>> +               return ktime_set(KTIME_MAX, 0);
> >>
> >> Wait.. this doesn't make sense. The timekeeper lock is only for reading.
> >>
> >> What I was suggesting to you off line is to have something that avoids
> >> spinning on the seqcounter should if a bug occurs and we IPI all the
> >> cpus, that we don't deadlock or block any printk messages.
> > 
> > We could also extend the fast timekeeper with boot/real/tai extensions and use
> > that for printk. You can use ktime_get_mono_fast_ns() today.
> > 
> 
> Thanks tglx -- I thought about doing that but was put off by the comments
> in __ktime_get_fast_ns() which point out that we could see backwards time
> stamps.  But I see your point -- I could do the same "last_time_stamp" check
> and use "??" in the output.

We talk about single digit nanoseconds here and in the case of a crash/bug we
really do not care about that at all.

Thanks,

	tglx
--
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]

#1303008

On Wed, Jan 6, 2016 at 9:34 AM, Thomas Gleixner <tglx@linutronix.de> wrote:
> On Wed, 6 Jan 2016, John Stultz wrote:
>> On Wed, Jan 6, 2016 at 5:00 AM, Prarit Bhargava <prarit@redhat.com> wrote:
>> > -ktime_t ktime_get_with_offset(enum tk_offsets offs)
>> > +ktime_t ktime_get_with_offset(enum tk_offsets offs, int trylock)
>> >  {
>> >         struct timekeeper *tk = &tk_core.timekeeper;
>> >         unsigned int seq;
>> >         ktime_t base, *offset = offsets[offs];
>> >         s64 nsecs;
>> > +       unsigned long flags = 0;
>> > +
>> > +       if (unlikely(!timekeeping_initialized))
>> > +               return ktime_set(0, 0);
>> >
>> >         WARN_ON(timekeeping_suspended);
>> >
>> > +       if (trylock && !raw_spin_trylock_irqsave(&timekeeper_lock, flags))
>> > +               return ktime_set(KTIME_MAX, 0);
>>
>> Wait.. this doesn't make sense. The timekeeper lock is only for reading.
>>
>> What I was suggesting to you off line is to have something that avoids
>> spinning on the seqcounter should if a bug occurs and we IPI all the
>> cpus, that we don't deadlock or block any printk messages.
>
> We could also extend the fast timekeeper with boot/real/tai extensions and use
> that for printk. You can use ktime_get_mono_fast_ns() today.

Ack. There'd be a chance for odd values around when the time is set,
but for debug printks I think its not critical.

thanks
-john
--
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]

#1303011

On 01/06/2016 02:04 PM, John Stultz wrote:
> On Wed, Jan 6, 2016 at 9:34 AM, Thomas Gleixner <tglx@linutronix.de> wrote:
>> On Wed, 6 Jan 2016, John Stultz wrote:
>>> On Wed, Jan 6, 2016 at 5:00 AM, Prarit Bhargava <prarit@redhat.com> wrote:
>>>> -ktime_t ktime_get_with_offset(enum tk_offsets offs)
>>>> +ktime_t ktime_get_with_offset(enum tk_offsets offs, int trylock)
>>>>  {
>>>>         struct timekeeper *tk = &tk_core.timekeeper;
>>>>         unsigned int seq;
>>>>         ktime_t base, *offset = offsets[offs];
>>>>         s64 nsecs;
>>>> +       unsigned long flags = 0;
>>>> +
>>>> +       if (unlikely(!timekeeping_initialized))
>>>> +               return ktime_set(0, 0);
>>>>
>>>>         WARN_ON(timekeeping_suspended);
>>>>
>>>> +       if (trylock && !raw_spin_trylock_irqsave(&timekeeper_lock, flags))
>>>> +               return ktime_set(KTIME_MAX, 0);
>>>
>>> Wait.. this doesn't make sense. The timekeeper lock is only for reading.
>>>
>>> What I was suggesting to you off line is to have something that avoids
>>> spinning on the seqcounter should if a bug occurs and we IPI all the
>>> cpus, that we don't deadlock or block any printk messages.
>>
>> We could also extend the fast timekeeper with boot/real/tai extensions and use
>> that for printk. You can use ktime_get_mono_fast_ns() today.
> 
> Ack. There'd be a chance for odd values around when the time is set,
> but for debug printks I think its not critical.

I'll convert to this in v2.

Thanks for the input everyone!

P.

> 
> thanks
> -john
> 
--
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]

#1302914

On Wed, Jan 6, 2016 at 9:28 AM, John Stultz <john.stultz@linaro.org> wrote:
> On Wed, Jan 6, 2016 at 5:00 AM, Prarit Bhargava <prarit@redhat.com> wrote:
>> -ktime_t ktime_get_with_offset(enum tk_offsets offs)
>> +ktime_t ktime_get_with_offset(enum tk_offsets offs, int trylock)
>>  {
>>         struct timekeeper *tk = &tk_core.timekeeper;
>>         unsigned int seq;
>>         ktime_t base, *offset = offsets[offs];
>>         s64 nsecs;
>> +       unsigned long flags = 0;
>> +
>> +       if (unlikely(!timekeeping_initialized))
>> +               return ktime_set(0, 0);
>>
>>         WARN_ON(timekeeping_suspended);
>>
>> +       if (trylock && !raw_spin_trylock_irqsave(&timekeeper_lock, flags))
>> +               return ktime_set(KTIME_MAX, 0);
>
> Wait.. this doesn't make sense. The timekeeper lock is only for reading.

Only for writing.. sorry.. still drinking my coffee.

> What I was suggesting to you off line is to have something that avoids
> spinning on the seqcounter should if a bug occurs and we IPI all the
> cpus, that we don't deadlock or block any printk messages.

And more clearly here, if a cpu takes a write on the seqcounter in
update_wall_time() and at that point another cpu hits a bug, and IPIs
the cpus, the system would deadlock. That's really what I want to
avoid.

thanks
-john
--
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]

#1302938

On 01/06/2016 12:33 PM, John Stultz wrote:
> On Wed, Jan 6, 2016 at 9:28 AM, John Stultz <john.stultz@linaro.org> wrote:
>> On Wed, Jan 6, 2016 at 5:00 AM, Prarit Bhargava <prarit@redhat.com> wrote:
>>> -ktime_t ktime_get_with_offset(enum tk_offsets offs)
>>> +ktime_t ktime_get_with_offset(enum tk_offsets offs, int trylock)
>>>  {
>>>         struct timekeeper *tk = &tk_core.timekeeper;
>>>         unsigned int seq;
>>>         ktime_t base, *offset = offsets[offs];
>>>         s64 nsecs;
>>> +       unsigned long flags = 0;
>>> +
>>> +       if (unlikely(!timekeeping_initialized))
>>> +               return ktime_set(0, 0);
>>>
>>>         WARN_ON(timekeeping_suspended);
>>>
>>> +       if (trylock && !raw_spin_trylock_irqsave(&timekeeper_lock, flags))
>>> +               return ktime_set(KTIME_MAX, 0);
>>
>> Wait.. this doesn't make sense. The timekeeper lock is only for reading.
> 
> Only for writing.. sorry.. still drinking my coffee.
> 
>> What I was suggesting to you off line is to have something that avoids
>> spinning on the seqcounter should if a bug occurs and we IPI all the
>> cpus, that we don't deadlock or block any printk messages.
> 
> And more clearly here, if a cpu takes a write on the seqcounter in
> update_wall_time() and at that point another cpu hits a bug, and IPIs
> the cpus, the system would deadlock. That's really what I want to
> avoid.

Right -- but the only time that the seq_lock is taken for writing is when the
timekeeper_lock is acquired (including update_wall_time()).  This means that

if (!raw_spin_trylock_irqsave(&timekeeper_lock, flags))

is equivalent to

if (tk_core.seq & 1) // sequence_t is odd when writing

The problem with the latter is that it is possible that there is no
protection from a writer setting tk_core.seq odd AFTER I've read it,
and the protection for that AFAICT comes from the timekeeper_lock.

That means I need to check to see if the timekeeper_lock is locked.  And
the patch does exactly that -- checks to see if the lock is available, and
if not avoids spinning on the seq_lock.

P.



> 
> thanks
> -john
> 
--
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]

#1302955 — Re: [PATCH 1/2] kernel, timekeeping, add trylock option to ktime_get_with_offset()

On Wed, 6 Jan 2016, Prarit Bhargava wrote:
> On 01/06/2016 12:33 PM, John Stultz wrote:
> > On Wed, Jan 6, 2016 at 9:28 AM, John Stultz <john.stultz@linaro.org> wrote:
> >> On Wed, Jan 6, 2016 at 5:00 AM, Prarit Bhargava <prarit@redhat.com> wrote:
> >>> -ktime_t ktime_get_with_offset(enum tk_offsets offs)
> >>> +ktime_t ktime_get_with_offset(enum tk_offsets offs, int trylock)
> >>>  {
> >>>         struct timekeeper *tk = &tk_core.timekeeper;
> >>>         unsigned int seq;
> >>>         ktime_t base, *offset = offsets[offs];
> >>>         s64 nsecs;
> >>> +       unsigned long flags = 0;
> >>> +
> >>> +       if (unlikely(!timekeeping_initialized))
> >>> +               return ktime_set(0, 0);
> >>>
> >>>         WARN_ON(timekeeping_suspended);
> >>>
> >>> +       if (trylock && !raw_spin_trylock_irqsave(&timekeeper_lock, flags))
> >>> +               return ktime_set(KTIME_MAX, 0);
> >>
> >> Wait.. this doesn't make sense. The timekeeper lock is only for reading.
> > 
> > Only for writing.. sorry.. still drinking my coffee.
> > 
> >> What I was suggesting to you off line is to have something that avoids
> >> spinning on the seqcounter should if a bug occurs and we IPI all the
> >> cpus, that we don't deadlock or block any printk messages.
> > 
> > And more clearly here, if a cpu takes a write on the seqcounter in
> > update_wall_time() and at that point another cpu hits a bug, and IPIs
> > the cpus, the system would deadlock. That's really what I want to
> > avoid.
> 
> Right -- but the only time that the seq_lock is taken for writing is when the
> timekeeper_lock is acquired (including update_wall_time()).  This means that
> 
> if (!raw_spin_trylock_irqsave(&timekeeper_lock, flags))
> 
> is equivalent to
> 
> if (tk_core.seq & 1) // sequence_t is odd when writing
> 
> The problem with the latter is that it is possible that there is no
> protection from a writer setting tk_core.seq odd AFTER I've read it,
> and the protection for that AFAICT comes from the timekeeper_lock.
> 
> That means I need to check to see if the timekeeper_lock is locked.  And
> the patch does exactly that -- checks to see if the lock is available, and
> if not avoids spinning on the seq_lock.

And no, we don't want that in every code path.

We already have the concept of the fast timekeeper, which is lockless and NMI
safe. It's useable for tracing and perf, so it can be used for printk as well.

It supports clock monotonic today, which is good enough for printk, but it
could be extended to other clocks if really required.

Thanks,

	tglx
--
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