Re: [PATCH v4 2/2] misc: Add a mechanism to detect stalls on guest vCPUs

From: Guenter Roeck <linux@roeck-us.net>
To: Greg Kroah-Hartman <gregkh@linuxfoundation.org>,
	Sebastian Ene <sebastianene@google.com>
Cc: Rob Herring <robh+dt@kernel.org>, Arnd Bergmann <arnd@arndb.de>,
	Dragan Cvetic <dragan.cvetic@xilinx.com>,
	linux-kernel@vger.kernel.org, devicetree@vger.kernel.org,
	maz@kernel.org, will@kernel.org, qperret@google.com
Subject: Re: [PATCH v4 2/2] misc: Add a mechanism to detect stalls on guest vCPUs
Date: Fri, 29 Apr 2022 09:51:51 -0700	[thread overview]
Message-ID: <e94fdd89-7b8e-eec3-4b2f-dcea55c7f0bc@roeck-us.net> (raw)
In-Reply-To: <YmumSgiTrQUWoXsb@kroah.com>

On 4/29/22 01:48, Greg Kroah-Hartman wrote:
> On Fri, Apr 29, 2022 at 08:30:33AM +0000, Sebastian Ene wrote:
>> This driver creates per-cpu hrtimers which are required to do the
>> periodic 'pet' operation. On a conventional watchdog-core driver, the
>> userspace is responsible for delivering the 'pet' events by writing to
>> the particular /dev/watchdogN node. In this case we require a strong
>> thread affinity to be able to account for lost time on a per vCPU.
>>
>> This part of the driver is the 'frontend' which is reponsible for
>> delivering the periodic 'pet' events, configuring the virtual peripheral
>> and listening for cpu hotplug events. The other part of the driver
>> handles the peripheral emulation and this part accounts for lost time by
>> looking at the /proc/{}/task/{}/stat entries and is located here:
>> https://chromium-review.googlesource.com/c/chromiumos/platform/crosvm/+/3548817
>>
>> Signed-off-by: Sebastian Ene <sebastianene@google.com>
>> ---
>>   drivers/misc/Kconfig       |  12 +++
>>   drivers/misc/Makefile      |   1 +
>>   drivers/misc/vm-watchdog.c | 206 +++++++++++++++++++++++++++++++++++++
>>   3 files changed, 219 insertions(+)
>>   create mode 100644 drivers/misc/vm-watchdog.c
>>
>> diff --git a/drivers/misc/Kconfig b/drivers/misc/Kconfig
>> index 2b9572a6d114..26c3a99e269c 100644
>> --- a/drivers/misc/Kconfig
>> +++ b/drivers/misc/Kconfig
>> @@ -493,6 +493,18 @@ config OPEN_DICE
>>   
>>   	  If unsure, say N.
>>   
>> +config VM_WATCHDOG
>> +	tristate "Virtual Machine Watchdog"
>> +	select LOCKUP_DETECTOR
>> +	help
>> +	  Detect CPU locks on the virtual machine. This driver relies on the
>> +	  hrtimers which are CPU-binded to do the 'pet' operation. When a vCPU
>> +	  has to do a 'pet', it exits the guest through MMIO write and the
>> +	  backend driver takes into account the lost ticks for this particular
>> +	  CPU.
>> +	  To compile this driver as a module, choose M here: the
>> +	  module will be called vm-wdt.
> 
> You forgot to name the module properly here based on the Makefile change
> you made.
> 
> And again, as this is called a "watchdog", it seems crazy that it is not
> in drivers/watchdog/
> 

I disagree. It is not a watchdog driver in the traditional sense (it does
not use, want to use, or need to use the watchdog driver API or ABI).
Its functionality is similar to the functionality of kernel/watchdog.c,
which doesn't belong into drivers/watchdog either.

Guenter