Re: [PATCH 03/16] xen/arm: introduce a separate struct for watchdog timers

[Mykola Kvach <xakep.amatop@xxxxxxxxx>]

Hi,

On Thu, Mar 13, 2025 at 5:27 PM Jan Beulich <jbeulich@xxxxxxxx> wrote:
>
> On 05.03.2025 10: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>
>
> A From: with no corresponding S-o-b: is potentially problematic. You also
> can't simply add one with her agreement, though.

Thank you for pointing that out! I'll revisit all commits and add the missing
Signed-off-by tags in the next version of patch series.

>
> > ---
> > This commit was introduced in patch series V2.
>
> Yet, btw, the whole series isn't tagged with a version.

Yes, I added a description of the versions in the cover letter and
followed the style
used in version 2 meaning I avoided using tags. Since years have passed between
the patch series, I thought including tags might confuse reviewers.
If you want I'll add a correct tag in the next version of this patch series,
i.e. V4 instead of V2.

>
> > --- 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 realize you mean to just do a mechanical replacement here, yet the use of
> u32 is not only against our style (should be uint32_t then), but it's also
> not clear to me that this subtraction can't ever yield a negative result.
> Hence the use of %d looks more correct to me than the cast to an unsigned
> type.
>
> In any event the already long line now grows too long and hence needs
> wrapping.

Maybe it would be better to send a separate patch for this. I'm not sure if such
changes are needed within the scope of this patch series

>
> > @@ -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];
>
> An alternative would be to have a separate array for the timeout values.
> This would also save some space, seeing that on 64-bit arches you
> introduce 32 bits of tail padding in the struct.

Maybe it will be enough to leave it as is and only change the order of
the timeout
value and the timer. This way, we will avoid potential padding issues
and still get
the benefits of using a single struct.

>
> If we go the struct watchdog_timer route, may I at least suggest to rename
> the field to just "watchdog", so things like &d->watchdog_timer[i].timer
> don't say "timer" twice?

I agree, I'll change the name of the fields to avoid duplication.

>
> > --- 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>
> > +
> > +struct watchdog_timer {
> > +    struct timer timer;
> > +    uint32_t timeout;
>
> This wants a brief comment mentioning the granularity.

Thanks for pointing that out, I'll add a comment.

>
> Jan