[PATCH] xen/sched: refactor urgent count update

[PATCH] xen/sched: refactor urgent count update

[Ruslan Ruslichenko]

From: Ruslan Ruslichenko <Ruslan_Ruslichenko@epam.com>

With current implementation whenever vCPU blocks on
waiting for event channel via SCHEDOP_poll hypercall,
scheduler sets 'is_urgent' flag. The flag is cleared
on vCPU runstate change, if it is no longer in
'VPF_blocked' state or its vcpu_id is not in poll_mask.

However, the only way for vCPU runstate to change in
polled state is when it's going to wakeup.

Thus when 'is_urgent' is set, inner condition will always be:

if ( unlikely(v->is_urgent) )
{
    if ( !(v->pause_flags & VPF_blocked) ||            // Always True
         !test_bit(v->vcpu_id, v->domain->poll_mask) ) // Never evaluated

This makes the second condition dead code.

Thus simplify overall code structure and remove dead code.

The patch has no functional change.

Signed-off-by: Ruslan Ruslichenko <Ruslan_Ruslichenko@epam.com>
---
 xen/common/sched/core.c | 26 +++++++++++---------------
 1 file changed, 11 insertions(+), 15 deletions(-)

diff --git a/xen/common/sched/core.c b/xen/common/sched/core.c
index 77cbfd5226..22b3cd1007 100644
--- a/xen/common/sched/core.c
+++ b/xen/common/sched/core.c
@@ -241,26 +241,22 @@ static inline void trace_continue_running(const struct vcpu *v)
 
 static inline void vcpu_urgent_count_update(struct vcpu *v)
 {
+    bool cur_urgent;
+
     if ( is_idle_vcpu(v) )
         return;
 
-    if ( unlikely(v->is_urgent) )
-    {
-        if ( !(v->pause_flags & VPF_blocked) ||
-             !test_bit(v->vcpu_id, v->domain->poll_mask) )
-        {
-            v->is_urgent = 0;
-            atomic_dec(&per_cpu(sched_urgent_count, v->processor));
-        }
-    }
-    else
+    cur_urgent = (v->pause_flags & VPF_blocked) &&
+                 test_bit(v->vcpu_id, v->domain->poll_mask);
+
+    if ( unlikely(v->is_urgent != cur_urgent) )
     {
-        if ( unlikely(v->pause_flags & VPF_blocked) &&
-             unlikely(test_bit(v->vcpu_id, v->domain->poll_mask)) )
-        {
-            v->is_urgent = 1;
+        v->is_urgent = cur_urgent;
+
+        if ( cur_urgent )
             atomic_inc(&per_cpu(sched_urgent_count, v->processor));
-        }
+        else
+            atomic_dec(&per_cpu(sched_urgent_count, v->processor));
     }
 }
 
-- 
2.43.0

Re: [PATCH] xen/sched: refactor urgent count update

[Jan Beulich]

On 25.09.2026 23:28, Ruslan Ruslichenko wrote:
> From: Ruslan Ruslichenko <Ruslan_Ruslichenko@epam.com>
> 
> With current implementation whenever vCPU blocks on
> waiting for event channel via SCHEDOP_poll hypercall,
> scheduler sets 'is_urgent' flag. The flag is cleared
> on vCPU runstate change, if it is no longer in
> 'VPF_blocked' state or its vcpu_id is not in poll_mask.
> 
> However, the only way for vCPU runstate to change in
> polled state is when it's going to wakeup.
> 
> Thus when 'is_urgent' is set, inner condition will always be:
> 
> if ( unlikely(v->is_urgent) )
> {
>     if ( !(v->pause_flags & VPF_blocked) ||            // Always True
>          !test_bit(v->vcpu_id, v->domain->poll_mask) ) // Never evaluated

Maybe this invariant indeed applies, but then there must be more to it.
Simply from taking the "else" branch here

        if ( unlikely(v->pause_flags & VPF_blocked) &&
             unlikely(test_bit(v->vcpu_id, v->domain->poll_mask)) )
        {
            v->is_urgent = 1;
            atomic_inc(&per_cpu(sched_urgent_count, v->processor));
        }

clearly immediately afterwards the invariant you name does not hold.

> --- a/xen/common/sched/core.c
> +++ b/xen/common/sched/core.c
> @@ -241,26 +241,22 @@ static inline void trace_continue_running(const struct vcpu *v)
>  
>  static inline void vcpu_urgent_count_update(struct vcpu *v)
>  {
> +    bool cur_urgent;
> +
>      if ( is_idle_vcpu(v) )
>          return;
>  
> -    if ( unlikely(v->is_urgent) )
> -    {
> -        if ( !(v->pause_flags & VPF_blocked) ||
> -             !test_bit(v->vcpu_id, v->domain->poll_mask) )
> -        {
> -            v->is_urgent = 0;
> -            atomic_dec(&per_cpu(sched_urgent_count, v->processor));
> -        }
> -    }
> -    else
> +    cur_urgent = (v->pause_flags & VPF_blocked) &&
> +                 test_bit(v->vcpu_id, v->domain->poll_mask);
> +
> +    if ( unlikely(v->is_urgent != cur_urgent) )
>      {
> -        if ( unlikely(v->pause_flags & VPF_blocked) &&
> -             unlikely(test_bit(v->vcpu_id, v->domain->poll_mask)) )
> -        {
> -            v->is_urgent = 1;
> +        v->is_urgent = cur_urgent;
> +
> +        if ( cur_urgent )
>              atomic_inc(&per_cpu(sched_urgent_count, v->processor));
> -        }
> +        else
> +            atomic_dec(&per_cpu(sched_urgent_count, v->processor));

Assuming the transformation is valid to make (scheduler maintainers will
need to judge), may I suggest to fold these two into a single atomic_add(),
passing in either +1 or -1?

Jan

[RESEND] Re: [PATCH] xen/sched: refactor urgent count update

[Ruslan]

Resending since Google web client mangled the formatting of previous mail.


On 9/29/26 17:35, Jan Beulich wrote:
> On 25.09.2026 23:28, Ruslan Ruslichenko wrote:
>> From: Ruslan Ruslichenko <Ruslan_Ruslichenko@epam.com>
>>
>> With current implementation whenever vCPU blocks on
>> waiting for event channel via SCHEDOP_poll hypercall,
>> scheduler sets 'is_urgent' flag. The flag is cleared
>> on vCPU runstate change, if it is no longer in
>> 'VPF_blocked' state or its vcpu_id is not in poll_mask.
>>
>> However, the only way for vCPU runstate to change in
>> polled state is when it's going to wakeup.
>>
>> Thus when 'is_urgent' is set, inner condition will always be:
>>
>> if ( unlikely(v->is_urgent) )
>> {
>>     if ( !(v->pause_flags & VPF_blocked) ||            // Always True
>>          !test_bit(v->vcpu_id, v->domain->poll_mask) ) // Never evaluated
> 
> Maybe this invariant indeed applies, but then there must be more to it.
> Simply from taking the "else" branch here
> 
>         if ( unlikely(v->pause_flags & VPF_blocked) &&
>              unlikely(test_bit(v->vcpu_id, v->domain->poll_mask)) )
>         {
>             v->is_urgent = 1;
>             atomic_inc(&per_cpu(sched_urgent_count, v->processor));
>         }
> 
> clearly immediately afterwards the invariant you name does not hold.
> 

I should have described the preconditions for this. vcpu_urgent_count_update()
only called by vcpu_runstate_change(), which returns early if
'v->runstate.state == new_state'.
v->is_urgent set means vCPU is in RUNSTATE_blocked, so the only change
from RUNSTATE_blocked
is to RUNSTATE_runnable or RUNSTATE_offline, and both require
_VPF_blocked clear.
Also vcpu_unblock() clears _VPF_blocked first and it is used by wakeup
sources: event_channel, irqs and poll_timer itself.

I will refine this in the commit message.

>> --- a/xen/common/sched/core.c
>> +++ b/xen/common/sched/core.c
>> @@ -241,26 +241,22 @@ static inline void trace_continue_running(const struct vcpu *v)
>>  
>>  static inline void vcpu_urgent_count_update(struct vcpu *v)
>>  {
>> +    bool cur_urgent;
>> +
>>      if ( is_idle_vcpu(v) )
>>          return;
>>  
>> -    if ( unlikely(v->is_urgent) )
>> -    {
>> -        if ( !(v->pause_flags & VPF_blocked) ||
>> -             !test_bit(v->vcpu_id, v->domain->poll_mask) )
>> -        {
>> -            v->is_urgent = 0;
>> -            atomic_dec(&per_cpu(sched_urgent_count, v->processor));
>> -        }
>> -    }
>> -    else
>> +    cur_urgent = (v->pause_flags & VPF_blocked) &&
>> +                 test_bit(v->vcpu_id, v->domain->poll_mask);
>> +
>> +    if ( unlikely(v->is_urgent != cur_urgent) )
>>      {
>> -        if ( unlikely(v->pause_flags & VPF_blocked) &&
>> -             unlikely(test_bit(v->vcpu_id, v->domain->poll_mask)) )
>> -        {
>> -            v->is_urgent = 1;
>> +        v->is_urgent = cur_urgent;
>> +
>> +        if ( cur_urgent )
>>              atomic_inc(&per_cpu(sched_urgent_count, v->processor));
>> -        }
>> +        else
>> +            atomic_dec(&per_cpu(sched_urgent_count, v->processor));
> 
> Assuming the transformation is valid to make (scheduler maintainers will
> need to judge), may I suggest to fold these two into a single atomic_add(),
> passing in either +1 or -1?
> 

This looks like a good improvement, thanks. I will update for v2.

--
Best Regards,
Ruslan

> Jan