[PATCH 0/4] xen/watchdog: Code and API improvements to the domain watchdog

[PATCH 0/4] xen/watchdog: Code and API improvements to the domain watchdog

[Andrew Cooper]

This is a mostly to address a corner case when using watchdogs, when a domain
wishes to cease fencing activites and cleanly reboot.

Patches 1 to 3 are just code improvements in domain_watchdog().  Patch 4
introduces the new functionality.

Andrew Cooper (4):
  xen/watchdog: Fold exit paths to have a single unlock
  xen/watchdog: Rearrange the logic to fold the timer-arming paths
  xen/watchdog: Drop all locked operations on the watchdog_inuse_map
  xen/watchdog: Support disable all watchdog timers in one go

 xen/common/schedule.c      | 47 +++++++++++++++++++++++++++++-----------------
 xen/include/public/sched.h |  6 ++++--
 2 files changed, 34 insertions(+), 19 deletions(-)

-- 
2.1.4


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

[Xen-devel] [PATCH 0/4] xen/watchdog: Code and API improvements to the domain watchdog

[Andrew Cooper]

This is a mostly to address a corner case when using watchdogs, when a domain
wishes to cease fencing activites and cleanly reboot.

Patches 1 to 3 are just code improvements in domain_watchdog().  Patch 4
introduces the new functionality.

Andrew Cooper (4):
  xen/watchdog: Fold exit paths to have a single unlock
  xen/watchdog: Rearrange the logic to fold the timer-arming paths
  xen/watchdog: Drop all locked operations on the watchdog_inuse_map
  xen/watchdog: Support disable all watchdog timers in one go

 xen/common/schedule.c      | 47 +++++++++++++++++++++++++++++-----------------
 xen/include/public/sched.h |  6 ++++--
 2 files changed, 34 insertions(+), 19 deletions(-)

-- 
2.1.4


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

[PATCH 1/4] xen/watchdog: Fold exit paths to have a single unlock

[Andrew Cooper]

This is mostly to simplify future logical changes, but it does come with a
modest redunction in compiled code size (-55, 345 => 290).

No functional change.

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, 9 insertions(+), 5 deletions(-)

diff --git a/xen/common/schedule.c b/xen/common/schedule.c
index 66f1e26..47f5d04 100644
--- a/xen/common/schedule.c
+++ b/xen/common/schedule.c
@@ -1050,6 +1050,8 @@ static void domain_watchdog_timeout(void *data)
 
 static long domain_watchdog(struct domain *d, uint32_t id, uint32_t timeout)
 {
+    long rc = 0;
+
     if ( id > NR_DOMAIN_WATCHDOG_TIMERS )
         return -EINVAL;
 
@@ -1064,15 +1066,15 @@ static long domain_watchdog(struct domain *d, uint32_t id, uint32_t timeout)
             set_timer(&d->watchdog_timer[id], NOW() + SECONDS(timeout));
             break;
         }
-        spin_unlock(&d->watchdog_lock);
-        return id == NR_DOMAIN_WATCHDOG_TIMERS ? -ENOSPC : id + 1;
+        rc = id == NR_DOMAIN_WATCHDOG_TIMERS ? -ENOSPC : id + 1;
+        goto unlock;
     }
 
     id -= 1;
     if ( !test_bit(id, &d->watchdog_inuse_map) )
     {
-        spin_unlock(&d->watchdog_lock);
-        return -EINVAL;
+        rc = -EINVAL;
+        goto unlock;
     }
 
     if ( timeout == 0 )
@@ -1085,8 +1087,10 @@ static long domain_watchdog(struct domain *d, uint32_t id, uint32_t timeout)
         set_timer(&d->watchdog_timer[id], NOW() + SECONDS(timeout));
     }
 
+ unlock:
     spin_unlock(&d->watchdog_lock);
-    return 0;
+
+    return rc;
 }
 
 void watchdog_domain_init(struct domain *d)
-- 
2.1.4


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

[Xen-devel] [PATCH 1/4] xen/watchdog: Fold exit paths to have a single unlock

[Andrew Cooper]

This is mostly to simplify future logical changes, but it does come with a
modest redunction in compiled code size (-55, 345 => 290).

No functional change.

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, 9 insertions(+), 5 deletions(-)

diff --git a/xen/common/schedule.c b/xen/common/schedule.c
index 66f1e26..47f5d04 100644
--- a/xen/common/schedule.c
+++ b/xen/common/schedule.c
@@ -1050,6 +1050,8 @@ static void domain_watchdog_timeout(void *data)
 
 static long domain_watchdog(struct domain *d, uint32_t id, uint32_t timeout)
 {
+    long rc = 0;
+
     if ( id > NR_DOMAIN_WATCHDOG_TIMERS )
         return -EINVAL;
 
@@ -1064,15 +1066,15 @@ static long domain_watchdog(struct domain *d, uint32_t id, uint32_t timeout)
             set_timer(&d->watchdog_timer[id], NOW() + SECONDS(timeout));
             break;
         }
-        spin_unlock(&d->watchdog_lock);
-        return id == NR_DOMAIN_WATCHDOG_TIMERS ? -ENOSPC : id + 1;
+        rc = id == NR_DOMAIN_WATCHDOG_TIMERS ? -ENOSPC : id + 1;
+        goto unlock;
     }
 
     id -= 1;
     if ( !test_bit(id, &d->watchdog_inuse_map) )
     {
-        spin_unlock(&d->watchdog_lock);
-        return -EINVAL;
+        rc = -EINVAL;
+        goto unlock;
     }
 
     if ( timeout == 0 )
@@ -1085,8 +1087,10 @@ static long domain_watchdog(struct domain *d, uint32_t id, uint32_t timeout)
         set_timer(&d->watchdog_timer[id], NOW() + SECONDS(timeout));
     }
 
+ unlock:
     spin_unlock(&d->watchdog_lock);
-    return 0;
+
+    return rc;
 }
 
 void watchdog_domain_init(struct domain *d)
-- 
2.1.4


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

[PATCH 2/4] xen/watchdog: Rearrange the logic to fold the timer-arming paths

[Andrew Cooper]

By rearranging the logic, the timer allocation loop can reuse the common timer
arming/clearing logic.  This results in easier to follow code, and a modest
reduction in compiled code size (-64, 290 => 226).

For domains which use watchdogs, the overwhemling majoriy of hypercalls will
be re-arming an existing timer.  Arrange the fastpath to match.

This does cause one change in behaviour for a corner case.  Previously,
specifying id = 0, timeout = 0 would instantly kill the domain, as the timer
would fire before returning to the guest.  This corner case is going to be
reused for a different purpose in a later change.

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 | 32 ++++++++++++++++++--------------
 1 file changed, 18 insertions(+), 14 deletions(-)

diff --git a/xen/common/schedule.c b/xen/common/schedule.c
index 47f5d04..89aba88 100644
--- a/xen/common/schedule.c
+++ b/xen/common/schedule.c
@@ -1057,35 +1057,39 @@ static long domain_watchdog(struct domain *d, uint32_t id, uint32_t timeout)
 
     spin_lock(&d->watchdog_lock);
 
-    if ( id == 0 )
+    if ( likely(id != 0) ) /* Operate on a specific timer. */
+    {
+        id -= 1;
+        if ( !test_bit(id, &d->watchdog_inuse_map) )
+        {
+            rc = -EINVAL;
+            goto unlock;
+        }
+    }
+    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;
-            set_timer(&d->watchdog_timer[id], NOW() + SECONDS(timeout));
             break;
         }
-        rc = id == NR_DOMAIN_WATCHDOG_TIMERS ? -ENOSPC : id + 1;
-        goto unlock;
-    }
-
-    id -= 1;
-    if ( !test_bit(id, &d->watchdog_inuse_map) )
-    {
-        rc = -EINVAL;
-        goto unlock;
+        if ( id == NR_DOMAIN_WATCHDOG_TIMERS )
+        {
+            rc = -ENOSPC;
+            goto unlock;
+        }
+        rc = id + 1;
     }
 
-    if ( timeout == 0 )
+    /* (re-)arm, or clear a specific timer. */
+    if ( unlikely(timeout == 0) )
     {
         stop_timer(&d->watchdog_timer[id]);
         clear_bit(id, &d->watchdog_inuse_map);
     }
     else
-    {
         set_timer(&d->watchdog_timer[id], NOW() + SECONDS(timeout));
-    }
 
  unlock:
     spin_unlock(&d->watchdog_lock);
-- 
2.1.4


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

[Xen-devel] [PATCH 2/4] xen/watchdog: Rearrange the logic to fold the timer-arming paths

[Andrew Cooper]

By rearranging the logic, the timer allocation loop can reuse the common timer
arming/clearing logic.  This results in easier to follow code, and a modest
reduction in compiled code size (-64, 290 => 226).

For domains which use watchdogs, the overwhemling majoriy of hypercalls will
be re-arming an existing timer.  Arrange the fastpath to match.

This does cause one change in behaviour for a corner case.  Previously,
specifying id = 0, timeout = 0 would instantly kill the domain, as the timer
would fire before returning to the guest.  This corner case is going to be
reused for a different purpose in a later change.

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 | 32 ++++++++++++++++++--------------
 1 file changed, 18 insertions(+), 14 deletions(-)

diff --git a/xen/common/schedule.c b/xen/common/schedule.c
index 47f5d04..89aba88 100644
--- a/xen/common/schedule.c
+++ b/xen/common/schedule.c
@@ -1057,35 +1057,39 @@ static long domain_watchdog(struct domain *d, uint32_t id, uint32_t timeout)
 
     spin_lock(&d->watchdog_lock);
 
-    if ( id == 0 )
+    if ( likely(id != 0) ) /* Operate on a specific timer. */
+    {
+        id -= 1;
+        if ( !test_bit(id, &d->watchdog_inuse_map) )
+        {
+            rc = -EINVAL;
+            goto unlock;
+        }
+    }
+    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;
-            set_timer(&d->watchdog_timer[id], NOW() + SECONDS(timeout));
             break;
         }
-        rc = id == NR_DOMAIN_WATCHDOG_TIMERS ? -ENOSPC : id + 1;
-        goto unlock;
-    }
-
-    id -= 1;
-    if ( !test_bit(id, &d->watchdog_inuse_map) )
-    {
-        rc = -EINVAL;
-        goto unlock;
+        if ( id == NR_DOMAIN_WATCHDOG_TIMERS )
+        {
+            rc = -ENOSPC;
+            goto unlock;
+        }
+        rc = id + 1;
     }
 
-    if ( timeout == 0 )
+    /* (re-)arm, or clear a specific timer. */
+    if ( unlikely(timeout == 0) )
     {
         stop_timer(&d->watchdog_timer[id]);
         clear_bit(id, &d->watchdog_inuse_map);
     }
     else
-    {
         set_timer(&d->watchdog_timer[id], NOW() + SECONDS(timeout));
-    }
 
  unlock:
     spin_unlock(&d->watchdog_lock);
-- 
2.1.4


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

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

[Andrew Cooper]

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

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

[Andrew Cooper]

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

[PATCH 4/4] xen/watchdog: Support disable all watchdog timers in one go

[Andrew Cooper]

For a domain which has been using watchdogs, but wants to cleanly reboot,
stopping all active timers is necessary to avoid crashing late during
shutdown.

The number of watchdogs isn't part of Xen's ABI, so to simplify cleanup and
error handling logic, support using id = 0, timeout = 0 to deactivate all
timers in one go.

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      | 9 ++++++++-
 xen/include/public/sched.h | 6 ++++--
 2 files changed, 12 insertions(+), 3 deletions(-)

diff --git a/xen/common/schedule.c b/xen/common/schedule.c
index 98c2c35..284f657 100644
--- a/xen/common/schedule.c
+++ b/xen/common/schedule.c
@@ -1066,7 +1066,7 @@ static long domain_watchdog(struct domain *d, uint32_t id, uint32_t timeout)
             goto unlock;
         }
     }
-    else /* Allocate the next available timer. */
+    else if ( timeout ) /* Allocate the next available timer. */
     {
         id = ffs(~d->watchdog_inuse_map) - 1;
 
@@ -1079,6 +1079,13 @@ static long domain_watchdog(struct domain *d, uint32_t id, uint32_t timeout)
         __set_bit(id, &d->watchdog_inuse_map);
         rc = id + 1;
     }
+    else /* id 0, timeout 0 => disable all timers. */
+    {
+        d->watchdog_inuse_map = 0;
+        for ( ; id < NR_DOMAIN_WATCHDOG_TIMERS; ++id )
+            stop_timer(&d->watchdog_timer[id]);
+        goto unlock;
+    }
 
     /* (re-)arm, or clear a specific timer. */
     if ( unlikely(timeout == 0) )
diff --git a/xen/include/public/sched.h b/xen/include/public/sched.h
index 811bd87..994a0e5 100644
--- a/xen/include/public/sched.h
+++ b/xen/include/public/sched.h
@@ -112,8 +112,10 @@
 /*
  * Setup, poke and destroy a domain watchdog timer.
  * @arg == pointer to sched_watchdog_t structure.
- * With id == 0, setup a domain watchdog timer to cause domain shutdown
- *               after timeout, returns watchdog id.
+ * With id == 0 and timeout != 0, setup a domain watchdog timer to cause
+ *                                domain shutdown after timeout, returns
+ *                                watchdog id.
+ * With id == 0 and timeout == 0, destroy all watchdog timers.
  * With id != 0 and timeout == 0, destroy domain watchdog timer.
  * With id != 0 and timeout != 0, poke watchdog timer and set new timeout.
  */
-- 
2.1.4


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

[Xen-devel] [PATCH 4/4] xen/watchdog: Support disable all watchdog timers in one go

[Andrew Cooper]

For a domain which has been using watchdogs, but wants to cleanly reboot,
stopping all active timers is necessary to avoid crashing late during
shutdown.

The number of watchdogs isn't part of Xen's ABI, so to simplify cleanup and
error handling logic, support using id = 0, timeout = 0 to deactivate all
timers in one go.

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      | 9 ++++++++-
 xen/include/public/sched.h | 6 ++++--
 2 files changed, 12 insertions(+), 3 deletions(-)

diff --git a/xen/common/schedule.c b/xen/common/schedule.c
index 98c2c35..284f657 100644
--- a/xen/common/schedule.c
+++ b/xen/common/schedule.c
@@ -1066,7 +1066,7 @@ static long domain_watchdog(struct domain *d, uint32_t id, uint32_t timeout)
             goto unlock;
         }
     }
-    else /* Allocate the next available timer. */
+    else if ( timeout ) /* Allocate the next available timer. */
     {
         id = ffs(~d->watchdog_inuse_map) - 1;
 
@@ -1079,6 +1079,13 @@ static long domain_watchdog(struct domain *d, uint32_t id, uint32_t timeout)
         __set_bit(id, &d->watchdog_inuse_map);
         rc = id + 1;
     }
+    else /* id 0, timeout 0 => disable all timers. */
+    {
+        d->watchdog_inuse_map = 0;
+        for ( ; id < NR_DOMAIN_WATCHDOG_TIMERS; ++id )
+            stop_timer(&d->watchdog_timer[id]);
+        goto unlock;
+    }
 
     /* (re-)arm, or clear a specific timer. */
     if ( unlikely(timeout == 0) )
diff --git a/xen/include/public/sched.h b/xen/include/public/sched.h
index 811bd87..994a0e5 100644
--- a/xen/include/public/sched.h
+++ b/xen/include/public/sched.h
@@ -112,8 +112,10 @@
 /*
  * Setup, poke and destroy a domain watchdog timer.
  * @arg == pointer to sched_watchdog_t structure.
- * With id == 0, setup a domain watchdog timer to cause domain shutdown
- *               after timeout, returns watchdog id.
+ * With id == 0 and timeout != 0, setup a domain watchdog timer to cause
+ *                                domain shutdown after timeout, returns
+ *                                watchdog id.
+ * With id == 0 and timeout == 0, destroy all watchdog timers.
  * With id != 0 and timeout == 0, destroy domain watchdog timer.
  * With id != 0 and timeout != 0, poke watchdog timer and set new timeout.
  */
-- 
2.1.4


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

Re: [PATCH 1/4] xen/watchdog: Fold exit paths to have a single unlock

[Wei Liu]

On Fri, May 10, 2019 at 07:28:01PM +0100, Andrew Cooper wrote:
> This is mostly to simplify future logical changes, but it does come with a
> modest redunction in compiled code size (-55, 345 => 290).
> 
> No functional change.
> 
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>

Reviewed-by: Wei Liu <wei.liu2@citrix.com>

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

Re: [Xen-devel] [PATCH 1/4] xen/watchdog: Fold exit paths to have a single unlock

[Wei Liu]

On Fri, May 10, 2019 at 07:28:01PM +0100, Andrew Cooper wrote:
> This is mostly to simplify future logical changes, but it does come with a
> modest redunction in compiled code size (-55, 345 => 290).
> 
> No functional change.
> 
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>

Reviewed-by: Wei Liu <wei.liu2@citrix.com>

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

Re: [PATCH 2/4] xen/watchdog: Rearrange the logic to fold the timer-arming paths

[Andrew Cooper]

On 10/05/2019 19:28, Andrew Cooper wrote:
> By rearranging the logic, the timer allocation loop can reuse the common timer
> arming/clearing logic.  This results in easier to follow code, and a modest
> reduction in compiled code size (-64, 290 => 226).
>
> For domains which use watchdogs, the overwhemling majoriy of hypercalls will
> be re-arming an existing timer.  Arrange the fastpath to match.
>
> This does cause one change in behaviour for a corner case.  Previously,
> specifying id = 0, timeout = 0 would instantly kill the domain, as the timer
> would fire before returning to the guest.  This corner case is going to be
> reused for a different purpose in a later change.

Actually, it turns out that this corner case is used for deliberately
fencing in some cases.

I'll have to invent some different way of clearing all timers.

~Andrew

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

Re: [Xen-devel] [PATCH 2/4] xen/watchdog: Rearrange the logic to fold the timer-arming paths

[Andrew Cooper]

On 10/05/2019 19:28, Andrew Cooper wrote:
> By rearranging the logic, the timer allocation loop can reuse the common timer
> arming/clearing logic.  This results in easier to follow code, and a modest
> reduction in compiled code size (-64, 290 => 226).
>
> For domains which use watchdogs, the overwhemling majoriy of hypercalls will
> be re-arming an existing timer.  Arrange the fastpath to match.
>
> This does cause one change in behaviour for a corner case.  Previously,
> specifying id = 0, timeout = 0 would instantly kill the domain, as the timer
> would fire before returning to the guest.  This corner case is going to be
> reused for a different purpose in a later change.

Actually, it turns out that this corner case is used for deliberately
fencing in some cases.

I'll have to invent some different way of clearing all timers.

~Andrew

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

Re: [PATCH 1/4] xen/watchdog: Fold exit paths to have a single unlock

[Jan Beulich]

>>> On 10.05.19 at 20:28, <andrew.cooper3@citrix.com> wrote:
> --- a/xen/common/schedule.c
> +++ b/xen/common/schedule.c
> @@ -1050,6 +1050,8 @@ static void domain_watchdog_timeout(void *data)
>  
>  static long domain_watchdog(struct domain *d, uint32_t id, uint32_t timeout)
>  {
> +    long rc = 0;

I'm wondering why this isn't plain int. Not a big deal, but I'm
curious anyway.

As per your own response to patch 2 I understand that the
other patches in this series don#t need looking at until you
send a v2.

Jan



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

Re: [Xen-devel] [PATCH 1/4] xen/watchdog: Fold exit paths to have a single unlock

[Jan Beulich]

>>> On 10.05.19 at 20:28, <andrew.cooper3@citrix.com> wrote:
> --- a/xen/common/schedule.c
> +++ b/xen/common/schedule.c
> @@ -1050,6 +1050,8 @@ static void domain_watchdog_timeout(void *data)
>  
>  static long domain_watchdog(struct domain *d, uint32_t id, uint32_t timeout)
>  {
> +    long rc = 0;

I'm wondering why this isn't plain int. Not a big deal, but I'm
curious anyway.

As per your own response to patch 2 I understand that the
other patches in this series don#t need looking at until you
send a v2.

Jan



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

Re: [PATCH 1/4] xen/watchdog: Fold exit paths to have a single unlock

[Andrew Cooper]

On 13/05/2019 14:47, Jan Beulich wrote:
>>>> On 10.05.19 at 20:28, <andrew.cooper3@citrix.com> wrote:
>> --- a/xen/common/schedule.c
>> +++ b/xen/common/schedule.c
>> @@ -1050,6 +1050,8 @@ static void domain_watchdog_timeout(void *data)
>>  
>>  static long domain_watchdog(struct domain *d, uint32_t id, uint32_t timeout)
>>  {
>> +    long rc = 0;
> I'm wondering why this isn't plain int. Not a big deal, but I'm
> curious anyway.

To match the return value.  This function is compiled twice AFAICT.

>
> As per your own response to patch 2 I understand that the
> other patches in this series don#t need looking at until you
> send a v2.

patch 3 is independent of the ABI changes, so fine in principle to
review now.

Patches 2 and 4 will definitely need changing.

~Andrew

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

Re: [Xen-devel] [PATCH 1/4] xen/watchdog: Fold exit paths to have a single unlock

[Andrew Cooper]

On 13/05/2019 14:47, Jan Beulich wrote:
>>>> On 10.05.19 at 20:28, <andrew.cooper3@citrix.com> wrote:
>> --- a/xen/common/schedule.c
>> +++ b/xen/common/schedule.c
>> @@ -1050,6 +1050,8 @@ static void domain_watchdog_timeout(void *data)
>>  
>>  static long domain_watchdog(struct domain *d, uint32_t id, uint32_t timeout)
>>  {
>> +    long rc = 0;
> I'm wondering why this isn't plain int. Not a big deal, but I'm
> curious anyway.

To match the return value.  This function is compiled twice AFAICT.

>
> As per your own response to patch 2 I understand that the
> other patches in this series don#t need looking at until you
> send a v2.

patch 3 is independent of the ABI changes, so fine in principle to
review now.

Patches 2 and 4 will definitely need changing.

~Andrew

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

Re: [PATCH 1/4] xen/watchdog: Fold exit paths to have a single unlock

[Jan Beulich]

>>> On 13.05.19 at 15:51, <andrew.cooper3@citrix.com> wrote:
> On 13/05/2019 14:47, Jan Beulich wrote:
>>>>> On 10.05.19 at 20:28, <andrew.cooper3@citrix.com> wrote:
>>> --- a/xen/common/schedule.c
>>> +++ b/xen/common/schedule.c
>>> @@ -1050,6 +1050,8 @@ static void domain_watchdog_timeout(void *data)
>>>  
>>>  static long domain_watchdog(struct domain *d, uint32_t id, uint32_t timeout)
>>>  {
>>> +    long rc = 0;
>> I'm wondering why this isn't plain int. Not a big deal, but I'm
>> curious anyway.
> 
> To match the return value.

But the compiler will happily sign-extend the value at the return statement.

>  This function is compiled twice AFAICT.

I have no idea how this matters.

>> As per your own response to patch 2 I understand that the
>> other patches in this series don#t need looking at until you
>> send a v2.
> 
> patch 3 is independent of the ABI changes, so fine in principle to
> review now.

Okay.

Jan



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

Re: [Xen-devel] [PATCH 1/4] xen/watchdog: Fold exit paths to have a single unlock

[Jan Beulich]

>>> On 13.05.19 at 15:51, <andrew.cooper3@citrix.com> wrote:
> On 13/05/2019 14:47, Jan Beulich wrote:
>>>>> On 10.05.19 at 20:28, <andrew.cooper3@citrix.com> wrote:
>>> --- a/xen/common/schedule.c
>>> +++ b/xen/common/schedule.c
>>> @@ -1050,6 +1050,8 @@ static void domain_watchdog_timeout(void *data)
>>>  
>>>  static long domain_watchdog(struct domain *d, uint32_t id, uint32_t timeout)
>>>  {
>>> +    long rc = 0;
>> I'm wondering why this isn't plain int. Not a big deal, but I'm
>> curious anyway.
> 
> To match the return value.

But the compiler will happily sign-extend the value at the return statement.

>  This function is compiled twice AFAICT.

I have no idea how this matters.

>> As per your own response to patch 2 I understand that the
>> other patches in this series don#t need looking at until you
>> send a v2.
> 
> patch 3 is independent of the ABI changes, so fine in principle to
> review now.

Okay.

Jan



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

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

[Jan Beulich]

>>> 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

[Jan Beulich]

>>> 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: [PATCH 3/4] xen/watchdog: Drop all locked operations on the watchdog_inuse_map

[Andrew Cooper]

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

[Andrew Cooper]

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: [PATCH 1/4] xen/watchdog: Fold exit paths to have a single unlock

[George Dunlap]

On 5/10/19 7:28 PM, Andrew Cooper wrote:
> This is mostly to simplify future logical changes, but it does come with a
> modest redunction in compiled code size (-55, 345 => 290).
> 
> No functional change.
> 
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>

Reviewed-by: George Dunlap <george.dunlap@citrix.com>

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

Re: [Xen-devel] [PATCH 1/4] xen/watchdog: Fold exit paths to have a single unlock

[George Dunlap]

On 5/10/19 7:28 PM, Andrew Cooper wrote:
> This is mostly to simplify future logical changes, but it does come with a
> modest redunction in compiled code size (-55, 345 => 290).
> 
> No functional change.
> 
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>

Reviewed-by: George Dunlap <george.dunlap@citrix.com>

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

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

[Jan Beulich]

>>> 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

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

[Jan Beulich]

>>> 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