Re: [PATCH v2] [watchdog] combine nmi_watchdog and softlockup

Previous message: [thread] [date] [author]
Next message: [thread] [date] [author]
From: Frederic Weisbecker
Date: Thursday, April 15, 2010 - 6:47 pm

On Thu, Apr 15, 2010 at 05:25:10PM -0400, Don Zickus wrote:



In fact we also have the sample_freq thing that let you run
against a frequency (events per sec) rather than a sample
period.

We do some calculations that recompute the actual sample
period on top of this frequency in a regular base as the
events come.

It's rather unfortunate we can't have 0.xxxx frequencies
but may be we can work that out by adding a freq_unscale
field, which would basically produce the frequency like
this:

	freq = event->attr.sample_freq * (10 ^ -event->attr.freq_unscale)

This is not trivial though, so let's rather focus on the
real matter for now :)





That looks too general. It's more about the fact the arch supports
cpu cycle events and generates NMIs on overflow.


  



I'm confused, do we have two versions of the softlockup
detector now? You should drop the older one.






Well, softlockups detection doesn't use NMI.






Please use __get_cpu_var() instead so that we keep the preempt disabled
check from smp_processor_id()





Same here.






You should replace the call sites directly.







Why do you keep the software clock, I don't see how it can be
useful to detect hardlockups. It triggers in a regular irq
not an NMI.






May be have an arch spin lock there to update your cpu mask safely.






Hmm...this is probably not necessary.






Please avoid such ifdefs in the middle of the code.

It's better to gather hardlockup matters in a single ifdef block:

#ifdef CONFIG_PERF_EVENTS_NMI
define your functions here
#else
define (if needed) your off case functions here (static inline stubs)
#endif






You probably want a backtrace cpu mask here as well
(but better don't use the same than the hardlockup thing)






Please warn here instead, nothing seriously dangerous is going to
happen.






Especially such kind of idef in a function plus goto in the middle, we really
want to avoid that.

You want a watchdog_nmi_enable() call instead that does nothing
if !CONFIG_PERF_EVENTS_NMI.






Why do you need to touch watchdogs here?






Same here?





Please warn instead, there is nothing fatal here.






I'd suggest to drop the NMI prefix, this is not anymore
about detecting hard lockups only.

May be config WATCHDOG or config LOCKUP_DETECTOR if it's taken
already.


Also you should half-drop the DETECT_SOFTLOCKUP thing:
keep it's definition but drop the ability to choose it from
the prompt:

config DETECT_SOFTLOCKUP
	bool
	depends on DEBUG_KERNEL && !S390
	default y

This way we keep it for compatibility with def_configs, it will
enable the WATCHDOG by default if it is "y", we can schedule
its removal later.


Thanks.

--
Previous message: [thread] [date] [author]
Next message: [thread] [date] [author]

Messages in current thread:
Re: [PATCH v2] [watchdog] combine nmi_watchdog and softlockup, Frederic Weisbecker, (Thu Apr 15, 6:47 pm)
Re: [PATCH v2] [watchdog] combine nmi_watchdog and softlockup, Frederic Weisbecker, (Fri Apr 16, 7:43 am)
Re: [PATCH v2] [watchdog] combine nmi_watchdog and softlockup, Frederic Weisbecker, (Fri Apr 16, 7:46 am)
Re: [PATCH v2] [watchdog] combine nmi_watchdog and softlockup, Frederic Weisbecker, (Fri Apr 16, 7:59 am)
Re: [PATCH v2] [watchdog] combine nmi_watchdog and softlockup, Frederic Weisbecker, (Fri Apr 16, 8:32 am)
Re: [PATCH v2] [watchdog] combine nmi_watchdog and softlockup, Frederic Weisbecker, (Fri Apr 16, 9:24 am)