Re: [PATCH 2/4] watchdog: introduce watchdog_suspend() and watchdog_resume()

[Ulrich Obergfell <uobergfe@redhat.com>]

> ----- Original Message -----
> From: "Andrew Morton" <akpm@linux-foundation.org>
> ...
> On Sat,  1 Aug 2015 14:49:23 +0200 Ulrich Obergfell <uobergfe@redhat.com> wrote:
>
>> This interface can be utilized to deactivate the hard and soft lockup
>> detector temporarily. Callers are expected to minimize the duration of
>> deactivation. Multiple deactivations are allowed to occur in parallel
>> but should be rare in practice.
>>
>> ...
>>
>> --- a/include/linux/nmi.h
>> +++ b/include/linux/nmi.h
>> @@ -80,6 +80,8 @@ extern int proc_watchdog_thresh(struct ctl_table *, int ,
>>                                  void __user *, size_t *, loff_t *);
>>  extern int proc_watchdog_cpumask(struct ctl_table *, int,
>>                                   void __user *, size_t *, loff_t *);
>> +extern int watchdog_suspend(void);
>> +extern void watchdog_resume(void);
>>  #endif
>>  
>>  #ifdef CONFIG_HAVE_ACPI_APEI_NMI
>> diff --git a/kernel/watchdog.c b/kernel/watchdog.c
>> index 5571f20..98d44b1 100644
>> --- a/kernel/watchdog.c
>> +++ b/kernel/watchdog.c
>> @@ -67,6 +67,7 @@ unsigned long *watchdog_cpumask_bits = cpumask_bits(&watchdog_cpumask);
>>  #define for_each_watchdog_cpu(cpu) \
>>          for_each_cpu_and((cpu), cpu_online_mask, &watchdog_cpumask)
>>  
>> +static int __read_mostly watchdog_suspended = 0;
>
> With my compiler the "= 0" increases the size of watchdog.o data.  For
> some reason by 16 bytes(!).

I see that you already fixed this. Many Thanks.

>>  static int __read_mostly watchdog_running;
>>  static u64 __read_mostly sample_period;
>
> The relationship between watchdog_running and watchdog_suspended hurts
> my brain a bit.  It appears that all watchdog_running transitions
> happen under watchdog_proc_mutex so I don't think it's racy, but I
> wonder if things would be simpler if these were folded into a single
> up/down counter.

The 'watchdog_running' variable indicates whether watchdog threads exist.
[Whether they have been launched via smpboot_register_percpu_thread().]

The 'watchdog_suspended' variable indicates whether existing threads are
currently parked, and whether multiple requests to suspend the watchdog
have been made by different callers.

I think folding them into one variable would not improve the readability
of the code, because instead of two variables with distinct semantics we
would then have one variable with multiple semantics (which I think would
also make code more complex). If we would want to improve the readability
I'd prefer to either just add a comment block to explain the semantics of
the variables or to rename them -for example- like:

  watchdog_running   to watchdog_threads_exist
  watchdog_suspended to watchdog_suspend_count

Please let me know if you would want any changes in this regard.

I also received these suggestions:

- rename the watchdog_{suspend|resume} functions as discussed in
  http://marc.info/?l=linux-kernel&m=143844050610220&w=2

- use pr_debug() instead of pr_info() as discussed in
  http://marc.info/?l=linux-kernel&m=143869949229461&w=2

Please let me know if you want me to post a 'version 2' of the patch set
or if you want me to post these changes as separate follow-up patches.

Regards,

Uli