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

Andrew Cooper posted 4 patches 1 year, 11 months ago

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

Posted by Andrew Cooper 1 year, 11 months ago
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

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

Posted by Jan Beulich 1 year, 11 months ago
>>> 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

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

Posted by Andrew Cooper 1 year, 11 months ago
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

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

Posted by Jan Beulich 1 year, 11 months ago
>>> 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