On 05.03.2025 10:11, Mykola Kvach wrote:
> From: Mykola Kvach <mykola_kv...@epam.com>
>
> This patch implements suspend/resume helpers for the watchdog.
> While a domain is suspended its watchdogs must be paused. Otherwise,
> if the domain stays in the suspend state for a longer period of time
> compared to the watchdog period, the domain would be shutdown on resume.
> Proper solution to this problem is to stop (suspend) the watchdog timers
> after the domain suspends and to restart (resume) the watchdog timers
> before the domain resumes. The suspend/resume of watchdog timers is done
> in Xen and is invisible to the guests.
>
> Signed-off-by: Mirela Simonovic <mirela.simono...@aggios.com>
From: != first S-o-b: is always raising the question of who's the original
author of a patch.
> --- a/xen/common/sched/core.c
> +++ b/xen/common/sched/core.c
> @@ -1605,6 +1605,45 @@ void watchdog_domain_destroy(struct domain *d)
> kill_timer(&d->watchdog_timer[i].timer);
> }
>
> +#ifdef CONFIG_SYSTEM_SUSPEND
> +
> +void watchdog_domain_suspend(struct domain *d)
> +{
> + unsigned int i;
> +
> + spin_lock(&d->watchdog_lock);
> +
> + for ( i = 0; i < NR_DOMAIN_WATCHDOG_TIMERS; i++ )
> + {
> + if ( test_bit(i, &d->watchdog_inuse_map) )
> + {
> + stop_timer(&d->watchdog_timer[i].timer);
> + }
We generally prefer to omit the braces if the body of an if() (or
whatever it is) is a single statement / line.
> + }
> +
> + spin_unlock(&d->watchdog_lock);
> +}
> +
> +void watchdog_domain_resume(struct domain *d)
> +{
> + unsigned int i;
> +
> + spin_lock(&d->watchdog_lock);
> +
> + for ( i = 0; i < NR_DOMAIN_WATCHDOG_TIMERS; i++ )
> + {
> + if ( test_bit(i, &d->watchdog_inuse_map) )
> + {
> + set_timer(&d->watchdog_timer[i].timer,
> + NOW() + SECONDS(d->watchdog_timer[i].timeout));
The timeout may have almost expired before suspending; restoring to the
full original period feels wrong. At the very least, if that's indeed
intended behavior, imo this needs spelling out explicitly.
> --- a/xen/include/xen/sched.h
> +++ b/xen/include/xen/sched.h
> @@ -1109,6 +1109,15 @@ void scheduler_disable(void);
> void watchdog_domain_init(struct domain *d);
> void watchdog_domain_destroy(struct domain *d);
>
> +#ifdef CONFIG_SYSTEM_SUSPEND
> +/*
> + * Suspend/resume watchdogs of domain (while the domain is suspended its
> + * watchdogs should be on pause)
> + */
> +void watchdog_domain_suspend(struct domain *d);
> +void watchdog_domain_resume(struct domain *d);
> +#endif /* CONFIG_SYSTEM_SUSPEND */
I don't think the #ifdef is strictly needed here?
Jan
