Re: [RFC PATCH lttng-modules] Fix: use timekeeping_is_busy() to fixktime_get() hard lockup

[Thomas Gleixner]

On Wed, 11 Sep 2013, Mathieu Desnoyers wrote:
> * John Stultz (john.stultz@xxxxxxxxxx) wrote:
> > On 09/11/2013 08:12 AM, Mathieu Desnoyers wrote:
> > > LTTng uses ktime to have the same time-base across kernel and
> > > user-space, so traces gathered from LTTng-modules and LTTng-UST can be
> > > correlated. We plan on using ktime until a fast, scalable, and
> > > fine-grained time-source for tracing that can be used across kernel and
> > > user-space, and which does not rely on read seqlock for kernel-level
> > > synchronization, makes its way into the kernel.
> > >
> > > Cc: Thomas Gleixner <tglx@xxxxxxxxxxxxx>
> > > Cc: Richard Cochran <richardcochran@xxxxxxxxx>
> > > Cc: Prarit Bhargava <prarit@xxxxxxxxxx>
> > > Cc: John Stultz <john.stultz@xxxxxxxxxx>
> > > Cc: Greg Kroah-Hartman <gregkh@xxxxxxxxxxxxxxxxxxx>
> > > Cc: Peter Zijlstra <peterz@xxxxxxxxxxxxx>
> > > Cc: Steven Rostedt <rostedt@xxxxxxxxxxx>
> > > Cc: Ingo Molnar <mingo@xxxxxxx>
> > > Signed-off-by: Mathieu Desnoyers <mathieu.desnoyers@xxxxxxxxxxxx>
> > > ---
> > > diff --git a/wrapper/trace-clock.h b/wrapper/trace-clock.h
> > > index bced61c..2f9df7a 100644
> > > --- a/wrapper/trace-clock.h
> > > +++ b/wrapper/trace-clock.h
> > > @@ -32,6 +32,7 @@
> > >  #include <linux/ktime.h>
> > >  #include <linux/time.h>
> > >  #include <linux/hrtimer.h>
> > > +#include <linux/version.h>
> > >  #include "random.h"
> > >  
> > >  static inline u64 trace_clock_monotonic_wrapper(void)
> > > @@ -45,6 +46,10 @@ static inline u64 trace_clock_monotonic_wrapper(void)
> > >  	if (in_nmi())
> > >  		return (u64) -EIO;
> > >  
> > > +#if (LINUX_VERSION_CODE >= KERNEL_VERSION(3,10,0))
> > > +	if (timekeeping_is_busy())
> > > +		return (u64) -EIO;
> > > +#endif
> > >  	ktime = ktime_get();
> > >  	return ktime_to_ns(ktime);
> > >  }
> > 
> > 
> > I guess the other question here is should this functionality be pushed
> > down into the timekeeping accessors themselves?
> > 
> > I know any extra checks would probably be considered overhead in some
> > uses, but if we do the check only when we hit contention then it might
> > not be so bad.
> 
> I thought about the exact same thing, but wanted to keep my initial
> kernel patch minimal, so I chose not to touch the fast paths initially.
> 
> Indeed, if we only do this check after the seqretry has failed, we
> should be able to add this check without touching the fast-path.
> 
> It might be cleaner to make ktime_get() return an error rather than
> cause a hard lockup in those cases. Especially if it can be done without
> performance regression.

Nope. ktime_get() is not going to fail ever. We want to deadlock when
its called from inside xtime_lock held code. Simply because it's wrong
to do so.

If there are special use cases, i.e. tracing, which need this kind of
check, then we rather add a new interface, e.g. ktime_get_tracetime(),
than adding a tasteless bogosity like timekeeping_busy().

Thanks,

	tglx




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