Re: [PATCH 2/2] softlockup: make detector be aware of task switch of processes hogging cpu

[Don Zickus <dzickus@redhat.com>]

On Wed, Aug 06, 2014 at 10:23:40AM +0800, Chai Wen wrote:
> On 08/05/2014 11:20 PM, Don Zickus wrote:
> 
> > On Tue, Aug 05, 2014 at 10:47:57AM +0800, Chai Wen wrote:
> >> On 08/04/2014 10:31 PM, Don Zickus wrote:
> >>
> >>> On Mon, Aug 04, 2014 at 03:36:19PM +0800, chai wen wrote:
> >>>>
> >>>> For now, soft lockup detector warns once for each case of process softlockup.
> >>>> But the thread 'watchdog/n' may can not always get cpu at the time slot between
> >>>> the task switch of two processes hogging that cpu.
> >>>> This case is a false negative of "warn only once for a process", as there may be
> >>>> a different process that is going to hog the cpu. Is is better for detector to
> >>>> be aware of it. 
> >>>
> >>> I am not sure I fully understand the problem resolved.
> >>>
> >>> >From the changelog I understood that two processes bouncing back and forth
> >>> could hog the cpu and could create a 'false negative' (a situation not
> >>> reported but should).
> >>
> >>
> >> Hi Don
> >>
> >> Thanks for your comment.
> >> Perhaps 'task-switch' is wrong and is some misleading here, sorry for that.
> >>
> >> Here I mean the very case that between a termination of an old cpu hogging
> >> process and a starting getting cpu of a new process hogging cpu.
> >> The case that two or more processes bouncing back and forth and the thread 'watchdog/n'
> >> getting no chance to run is rather difficult to be supposed. And I think this situation
> >> does not exist.
> >>
> >> When I am reading the code of warning once about a case, I think maybe it is
> >> not so reliable by judging a "soft_watchdog_warn". And I tried a simple test to see
> >> if it could handle the cased I mentioned above. Please see the comment and detail of
> >> the test below.
> > 
> > Thank you for your test case.  I understand the problem now.  If you have
> > multiple processes hogging the cpu and you kill the one reported by
> > the softlockup warning, you will never know about the other processes
> > because the soft_watchdog_warn variable never gets a chance to reset.
> > 
> > I am ok with your patch then.
> > 
> > Do you mind if I modify the changelog a little bit to maybe help explain
> > the problem better?  I am thinking of something like below:
> > 
> > "
> > For now, soft lockup detector warns once for each case of process softlockup.
> > But the thread 'watchdog/n' may not always get the cpu at the time slot between
> > the task switch of two processes hogging that cpu to reset
> > soft_watchdog_warn.
> > 
> > An example would be two processes hogging the cpu.  Process A causes the
> > softlockup warning and is killed manually by a user.  Process B
> > immediately becomes the new process hogging the cpu preventing the
> > softlockup code from resetting the soft_watchdog_warn variable.
> > 
> > This case is a false negative of "warn only once for a process", as there may be
> > a different process that is going to hog the cpu.  Resolve this by
> > saving/checking the pid of the hogging process and use that to reset
> > soft_watchdog_warn too.
> > "
> 
> 
> Your changelog and comment below is more specific for this case.
> Thanks for your work on this patch.
> Please feel free to do it like this.

Ok.  Great.  I'll repost with my signoff and updated changelog.

Cheers,
Don

> 
> 
> thanks
> chai wen
> 
> 
> > 
> >>
> >>>
> >>> But looking at the patch below I was a little confused on the
> >>> __touch_watchdog addition.  See below:
> >>>
> >>>>
> >>>> Signed-off-by: chai wen <chaiw.fnst@cn.fujitsu.com>
> >>>> ---
> >>>>  kernel/watchdog.c |   18 ++++++++++++++++--
> >>>>  1 files changed, 16 insertions(+), 2 deletions(-)
> >>>>
> >>>> diff --git a/kernel/watchdog.c b/kernel/watchdog.c
> >>>> index 4c2e11c..908050c 100644
> >>>> --- a/kernel/watchdog.c
> >>>> +++ b/kernel/watchdog.c
> >>>> @@ -42,6 +42,7 @@ static DEFINE_PER_CPU(bool, softlockup_touch_sync);
> >>>>  static DEFINE_PER_CPU(bool, soft_watchdog_warn);
> >>>>  static DEFINE_PER_CPU(unsigned long, hrtimer_interrupts);
> >>>>  static DEFINE_PER_CPU(unsigned long, soft_lockup_hrtimer_cnt);
> >>>> +static DEFINE_PER_CPU(pid_t, softlockup_warn_pid_saved);
> >>>>  #ifdef CONFIG_HARDLOCKUP_DETECTOR
> >>>>  static DEFINE_PER_CPU(bool, hard_watchdog_warn);
> >>>>  static DEFINE_PER_CPU(bool, watchdog_nmi_touch);
> >>>> @@ -317,6 +318,8 @@ static enum hrtimer_restart watchdog_timer_fn(struct hrtimer *hrtimer)
> >>>>  	 */
> >>>>  	duration = is_softlockup(touch_ts);
> >>>>  	if (unlikely(duration)) {
> >>>> +		pid_t pid = task_pid_nr(current);
> >>>> +
> >>>>  		/*
> >>>>  		 * If a virtual machine is stopped by the host it can look to
> >>>>  		 * the watchdog like a soft lockup, check to see if the host
> >>>> @@ -326,8 +329,18 @@ static enum hrtimer_restart watchdog_timer_fn(struct hrtimer *hrtimer)
> >>>>  			return HRTIMER_RESTART;
> >>>>  
> >>>>  		/* only warn once */
> >>>> -		if (__this_cpu_read(soft_watchdog_warn) == true)
> >>>> +		if (__this_cpu_read(soft_watchdog_warn) == true) {
> >>>> +			/*
> >>>> +			 * soft lockup detector should be aware of that there
> >>>> +			 * may be a task-swicth of two different processes
> >>>> +			 * hogging the cpu continously
> >>>> +			 */
> > 
> > Can I modify the comment above to something like:
> > 
> > 
> > +			/*
> > +			 * Handle the case where multiple processes are
> > +			 * causing softlockups but the duration is small
> > +			 * enough, the softlockup detector can not reset
> > +			 * itself in time.  Use pids to detect this.
> > +			 */
> > 
> > 
> > Cheers,
> > Don
> > 
> >>>
> >>> The above piece is what I am trying to understand.  Are you saying that
> >>> when two different processes are hogging the cpu, undo the
> >>> soft_watchdog_warn and allow the second pid to be reported?
> >>>
> >>
> >>
> >> Yes, Indeed.
> >>
> >>> Just trying to understand the problem and see if this is the right
> >>> approach (because 3 or more processes could cause the same problem???).
> >>>
> >>
> >>
> >> Only 2 processes is involved in this case as mentioned above, and it is a case about
> >> a termination of an old process and a starting of a new process.
> >>
> >> Here is my test about the case:
> >>
> >> 	stuck.c:
> >> 	#include <stdlib.h>
> >> 	#include <stdio.h>
> >>
> >> 	int main(int argc, char **argv)
> >> 	{
> >>     		while(1);
> >>     		exit(0);
> >> 	}
> >>
> >> 	# gcc -o stuck stuck.c
> >> 	# ./stuck &
> >> 	[1] 30309
> >> 	# ./stuck &
> >> 	[2] 30310
> >> 	# taskset -pc 3 30309
> >> 	pid 30309's current affinity list: 0-3
> >> 	pid 30309's new affinity list: 3
> >> 	# taskset -pc 3 30310
> >> 	pid 30310's current affinity list: 0-3
> >> 	pid 30310's new affinity list: 3
> >>
> >> 	Then change the schedule policy of 30309 and 30310 to be SCHED_FIFO with the MAX_RT_PRIO-1 priority.
> >> 	As the firstly changed to SCHED_FIFO process hogging cpu#3, and is reported after about ~20s.
> >> 	After it is killed or terminated, the process 30310 is going out and keeping hogging the cpu continuously
> >> 	But this process can not be always reported by the detector in this test.
> >> 	If removing the 'warn once' checking, pid change and rather big lockup duration could be found.
> >>
> >> thanks
> >> chai wen
> >>
> >>> Cheers,
> >>> Don
> >>>
> >>>>  			return HRTIMER_RESTART;
> >>>> +		}
> >>>>  
> >>>>  		if (softlockup_all_cpu_backtrace) {
> >>>>  			/* Prevent multiple soft-lockup reports if one cpu is already
> >>>> @@ -342,7 +355,8 @@ static enum hrtimer_restart watchdog_timer_fn(struct hrtimer *hrtimer)
> >>>>  
> >>>>  		printk(KERN_EMERG "BUG: soft lockup - CPU#%d stuck for %us! [%s:%d]\n",
> >>>>  			smp_processor_id(), duration,
> >>>> -			current->comm, task_pid_nr(current));
> >>>> +			current->comm, pid);
> >>>> +		__this_cpu_write(softlockup_warn_pid_saved, pid);
> >>>>  		print_modules();
> >>>>  		print_irqtrace_events(current);
> >>>>  		if (regs)
> >>>> -- 
> >>>> 1.7.1
> >>>>
> >>> .
> >>>
> >>
> >>
> >>
> >> -- 
> >> Regards
> >>
> >> Chai Wen
> > .
> > 
> 
> 
> 
> -- 
> Regards
> 
> Chai Wen