All modifications to the watchdog_inuse_map happen with d->watchdog_lock held,
so there are no concurrency problems to deal with.
Furthermore, there is no need to use a loop to locate the next available
watchdog. As the bitmap is currently 2 bits wide and is stored in a uint32_t,
the next available timer can be located in O(1) time using bit-scanning
instructions.
No change in behaviour, but should have less cache-coherency impact.
Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
---
CC: Jan Beulich <JBeulich@suse.com>
CC: Wei Liu <wei.liu2@citrix.com>
CC: Roger Pau Monné <roger.pau@citrix.com>
CC: Stefano Stabellini <sstabellini@kernel.org>
CC: Julien Grall <julien.grall@arm.com>
CC: George Dunlap <george.dunlap@eu.citrix.com>
CC: Edwin Török <edvin.torok@citrix.com>
CC: Christian Lindig <christian.lindig@citrix.com>
CC: Pau Ruiz Safont <pau.safont@citrix.com>
---
xen/common/schedule.c | 14 ++++++--------
1 file changed, 6 insertions(+), 8 deletions(-)
diff --git a/xen/common/schedule.c b/xen/common/schedule.c
index 89aba88..98c2c35 100644
--- 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;
+
+ if ( unlikely(id >= NR_DOMAIN_WATCHDOG_TIMERS) )
{
rc = -ENOSPC;
goto unlock;
}
+
+ __set_bit(id, &d->watchdog_inuse_map);
rc = id + 1;
}
@@ -1086,7 +1084,7 @@ static long domain_watchdog(struct domain *d, uint32_t id, uint32_t timeout)
if ( unlikely(timeout == 0) )
{
stop_timer(&d->watchdog_timer[id]);
- clear_bit(id, &d->watchdog_inuse_map);
+ __clear_bit(id, &d->watchdog_inuse_map);
}
else
set_timer(&d->watchdog_timer[id], NOW() + SECONDS(timeout));
--
2.1.4
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel>>> On 10.05.19 at 20:28, <andrew.cooper3@citrix.com> 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).
> --- 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.
Also while this looks to be logically independent of patch 2, it
doesn't look like it would apply on its own.
Jan
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
On 13/05/2019 16:01, Jan Beulich wrote:
>>>> On 10.05.19 at 20:28, <andrew.cooper3@citrix.com> 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@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
>>> On 13.05.19 at 17:17, <andrew.cooper3@citrix.com> wrote:
> On 13/05/2019 16:01, Jan Beulich wrote:
>>>>> On 10.05.19 at 20:28, <andrew.cooper3@citrix.com> 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
Right, hence my suggestion to just mention it here.
>>> --- 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.
Oh, right, it works with a pointer. Would you mind adding a
BUILD_BUG_ON() then to exclude the UB case of ffs()? With
that then
Reviewed-by: Jan Beulich <jbeulich@suse.com>
Jan
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
© 2016 - 2026 Red Hat, Inc.