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

[Ruslan <ruslichenko.r@xxxxxxxxx>]

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