[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [PATCH 03/16] xen/arm: introduce a separate struct for watchdog timers
Hi, On Wed, Mar 19, 2025 at 6:14 PM Grygorii Strashko <grygorii_strashko@xxxxxxxx> wrote: > > > > On 05.03.25 11:11, Mykola Kvach wrote: > > From: Mirela Simonovic <mirela.simonovic@xxxxxxxxxx> > > > > Introduce a separate struct for watchdog timers. It is needed to properly > > implement the suspend/resume actions for the watchdog timers. To be able > > to restart watchdog timer after suspend we need to remember their > > frequency somewhere. To not bloat the struct timer a new struct > > watchdog_timer is introduced, containing the original timer and the last > > set timeout. > > > > Signed-off-by: Mykyta Poturai <mykyta_poturai@xxxxxxxx> > > Signed-off-by: Mykola Kvach <mykola_kvach@xxxxxxxx> > > --- > > This commit was introduced in patch series V2. > > --- > > xen/common/keyhandler.c | 2 +- > > xen/common/sched/core.c | 11 ++++++----- > > xen/include/xen/sched.h | 3 ++- > > xen/include/xen/watchdog.h | 6 ++++++ > > 4 files changed, 15 insertions(+), 7 deletions(-) > > > > diff --git a/xen/common/keyhandler.c b/xen/common/keyhandler.c > > index 0bb842ec00..caf614c0c2 100644 > > --- a/xen/common/keyhandler.c > > +++ b/xen/common/keyhandler.c > > @@ -305,7 +305,7 @@ static void cf_check dump_domains(unsigned char key) > > for ( i = 0 ; i < NR_DOMAIN_WATCHDOG_TIMERS; i++ ) > > if ( test_bit(i, &d->watchdog_inuse_map) ) > > printk(" watchdog %d expires in %d seconds\n", > > - i, (u32)((d->watchdog_timer[i].expires - NOW()) >> > > 30)); > > + i, (u32)((d->watchdog_timer[i].timer.expires - > > NOW()) >> 30)); > > I'd like to propose to add watchdog API wrapper here, like > > watchdog_domain_expires_sec(d,id) > > or > > watchdog_domain_dump(d) > > and so hide implementation internals. It was already proposed by Jan Beulich. I'll do it. > > > > > arch_dump_domain_info(d); > > > > diff --git a/xen/common/sched/core.c b/xen/common/sched/core.c > > index d6296d99fd..b1c6b6b9fa 100644 > > --- a/xen/common/sched/core.c > > +++ b/xen/common/sched/core.c > > @@ -1556,7 +1556,8 @@ static long domain_watchdog(struct domain *d, > > uint32_t id, uint32_t timeout) > > { > > if ( test_and_set_bit(id, &d->watchdog_inuse_map) ) > > continue; > > - set_timer(&d->watchdog_timer[id], NOW() + SECONDS(timeout)); > > + d->watchdog_timer[id].timeout = timeout; > > + set_timer(&d->watchdog_timer[id].timer, NOW() + > > SECONDS(timeout)); > > break; > > } > > spin_unlock(&d->watchdog_lock); > > @@ -1572,12 +1573,12 @@ static long domain_watchdog(struct domain *d, > > uint32_t id, uint32_t timeout) > > > > if ( timeout == 0 ) > > { > > - stop_timer(&d->watchdog_timer[id]); > > + stop_timer(&d->watchdog_timer[id].timer); > > clear_bit(id, &d->watchdog_inuse_map); > > } > > else > > { > > - set_timer(&d->watchdog_timer[id], NOW() + SECONDS(timeout)); > > + set_timer(&d->watchdog_timer[id].timer, NOW() + SECONDS(timeout)); > > } > > > > spin_unlock(&d->watchdog_lock); > > @@ -1593,7 +1594,7 @@ void watchdog_domain_init(struct domain *d) > > d->watchdog_inuse_map = 0; > > > > for ( i = 0; i < NR_DOMAIN_WATCHDOG_TIMERS; i++ ) > > - init_timer(&d->watchdog_timer[i], domain_watchdog_timeout, d, 0); > > + init_timer(&d->watchdog_timer[i].timer, domain_watchdog_timeout, > > d, 0); > > } > > > > void watchdog_domain_destroy(struct domain *d) > > @@ -1601,7 +1602,7 @@ void watchdog_domain_destroy(struct domain *d) > > unsigned int i; > > > > for ( i = 0; i < NR_DOMAIN_WATCHDOG_TIMERS; i++ ) > > - kill_timer(&d->watchdog_timer[i]); > > + kill_timer(&d->watchdog_timer[i].timer); > > } > > > > /* > > diff --git a/xen/include/xen/sched.h b/xen/include/xen/sched.h > > index 177784e6da..d0d10612ce 100644 > > --- a/xen/include/xen/sched.h > > +++ b/xen/include/xen/sched.h > > @@ -24,6 +24,7 @@ > > #include <asm/current.h> > > #include <xen/vpci.h> > > #include <xen/wait.h> > > +#include <xen/watchdog.h> > > #include <public/xen.h> > > #include <public/domctl.h> > > #include <public/sysctl.h> > > I think struct watchdog_timer (or whatever you going to add) need to be moved > in sched.h > because... > > > @@ -569,7 +570,7 @@ struct domain > > #define NR_DOMAIN_WATCHDOG_TIMERS 2 > > spinlock_t watchdog_lock; > > uint32_t watchdog_inuse_map; > > - struct timer watchdog_timer[NR_DOMAIN_WATCHDOG_TIMERS]; > > + struct watchdog_timer watchdog_timer[NR_DOMAIN_WATCHDOG_TIMERS]; > > > > struct rcu_head rcu; > > > > diff --git a/xen/include/xen/watchdog.h b/xen/include/xen/watchdog.h > > index 4c2840bd91..2b7169632d 100644 > > --- a/xen/include/xen/watchdog.h > > +++ b/xen/include/xen/watchdog.h > > @@ -8,6 +8,12 @@ > > #define __XEN_WATCHDOG_H__ > > > > #include <xen/types.h> > > +#include <xen/timer.h> > > ...this interface is not related to domain's watchdogs. > From x86 code, it seems like some sort of HW watchdog used to check pCPUs > state > and not domains/vcpu. And it's Not enabled for Arm now. Sorry, but maybe I missed something. However, this struct and the previous watchdog timer are used as fields of the domain struct and correspond to a particular domain. Also, take a look at some functions where the watchdog timer field is used: domain_watchdog, watchdog_domain_init, and watchdog_domain_destroy. I see a direct connection with a domain.. > > > + > > +struct watchdog_timer { > > + struct timer timer; > > + uint32_t timeout; > > +}; > > > > #ifdef CONFIG_WATCHDOG > > Best regards, Mykola
|
![]() |
Lists.xenproject.org is hosted with RackSpace, monitoring our |