Re: [Xen-devel] [PATCH 3/4] xen/watchdog: Drop all locked operations on the watchdog_inuse_map

[Andrew Cooper <andrew.cooper3@xxxxxxxxxx>]

On 13/05/2019 16:01, Jan Beulich wrote:
>>>> On 10.05.19 at 20:28, <andrew.cooper3@xxxxxxxxxx> wrote:
>> All modifications to the watchdog_inuse_map happen with d->watchdog_lock 
>> held,
>> so there are no concurrency problems to deal with.
> But concurrency problems can also occur for readers. While
> not a problem afaict, dump_domains() actually has such an
> example (and hence might be worth mentioning here).

Its only debugging, and would need to take the spinlock anyway to avoid
a TOCTOU race between watchdog_inuse_map and d->watchdog_timer[i].expires

>> --- a/xen/common/schedule.c
>> +++ b/xen/common/schedule.c
>> @@ -1068,17 +1068,15 @@ static long domain_watchdog(struct domain *d, 
>> uint32_t id, uint32_t timeout)
>>      }
>>      else /* Allocate the next available timer. */
>>      {
>> -        for ( id = 0; id < NR_DOMAIN_WATCHDOG_TIMERS; id++ )
>> -        {
>> -            if ( test_and_set_bit(id, &d->watchdog_inuse_map) )
>> -                continue;
>> -            break;
>> -        }
>> -        if ( id == NR_DOMAIN_WATCHDOG_TIMERS )
>> +        id = ffs(~d->watchdog_inuse_map) - 1;
> I'm surprised we have no (universally available) ffz(). I wonder
> though whether find_first_zero_bit() wouldn't be the better
> choice here anyway, as the result wouldn't be undefined in
> case NR_DOMAIN_WATCHDOG_TIMERS grew to 32.

Sadly no - find_first_zero_bit() takes unsigned long *, so its use here
a) requires a (void *) cast to compile, and b) is definitely UB.

I didn't fancy extending d->watchdog_inuse_map to unsigned long just to
make this work, nor do I think it is likely for this interface to gain
more timers.  From a usability point of view, it is rather terrible.

A far more useful interface (from a guests point of view) would be a
mechanism with a per-vcpu timer which injects an NMI on timeout, which
permits the guest far more flexibility about how to handle the timeout,
which might including dumping state or sending out a positive "I'm
fencing" broadcast.  For some HA scenarios, this is preferable to
forcing everyone else to wait for the system timeout before declaring
the dead entity to have fenced.

~Andrew

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel