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

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