diff mbox series

[v2,22/48] xen/sched: switch schedule() from vcpus to sched_units

Message ID 20190809145833.1020-23-jgross@suse.com (mailing list archive)
State Superseded
Headers show
Series xen: add core scheduling support | expand

Commit Message

Jürgen Groß Aug. 9, 2019, 2:58 p.m. UTC
Use sched_units instead of vcpus in schedule(). This includes the
introduction of sched_unit_runstate_change() as a replacement of
vcpu_runstate_change() in schedule().

Signed-off-by: Juergen Gross <jgross@suse.com>
---
 xen/common/schedule.c | 68 +++++++++++++++++++++++++++++----------------------
 1 file changed, 39 insertions(+), 29 deletions(-)

Comments

Jan Beulich Sept. 9, 2019, 2:35 p.m. UTC | #1
On 09.08.2019 16:58, Juergen Gross wrote:
> --- a/xen/common/schedule.c
> +++ b/xen/common/schedule.c
> @@ -248,6 +248,20 @@ static inline void vcpu_runstate_change(
>      v->runstate.state = new_state;
>  }
>  
> +static inline void sched_unit_runstate_change(struct sched_unit *unit,
> +    bool running, s_time_t new_entry_time)
> +{
> +    struct vcpu *v = unit->vcpu_list;
> +
> +    if ( running )
> +        vcpu_runstate_change(v, RUNSTATE_running, new_entry_time);
> +    else
> +        vcpu_runstate_change(v,
> +            ((v->pause_flags & VPF_blocked) ? RUNSTATE_blocked :
> +             (vcpu_runnable(v) ? RUNSTATE_runnable : RUNSTATE_offline)),
> +            new_entry_time);
> +}

I find it puzzling that this gets introduced, but won't survive till
the end of the series. I can only guess that you can't do without the
separation intermediately. Making such transient state more apparent
from the description would be nice imo.

Jan
Jürgen Groß Sept. 12, 2019, 1:44 p.m. UTC | #2
On 09.09.19 16:35, Jan Beulich wrote:
> On 09.08.2019 16:58, Juergen Gross wrote:
>> --- a/xen/common/schedule.c
>> +++ b/xen/common/schedule.c
>> @@ -248,6 +248,20 @@ static inline void vcpu_runstate_change(
>>       v->runstate.state = new_state;
>>   }
>>   
>> +static inline void sched_unit_runstate_change(struct sched_unit *unit,
>> +    bool running, s_time_t new_entry_time)
>> +{
>> +    struct vcpu *v = unit->vcpu_list;
>> +
>> +    if ( running )
>> +        vcpu_runstate_change(v, RUNSTATE_running, new_entry_time);
>> +    else
>> +        vcpu_runstate_change(v,
>> +            ((v->pause_flags & VPF_blocked) ? RUNSTATE_blocked :
>> +             (vcpu_runnable(v) ? RUNSTATE_runnable : RUNSTATE_offline)),
>> +            new_entry_time);
>> +}
> 
> I find it puzzling that this gets introduced, but won't survive till
> the end of the series. I can only guess that you can't do without the
> separation intermediately. Making such transient state more apparent
> from the description would be nice imo.

The functionality will stay, but it will be subsumed in patch 35. I
don't think I should mention that in the commit message, so do you want
me to just add it below the "---" marker?


Juergen
Jan Beulich Sept. 12, 2019, 2:34 p.m. UTC | #3
On 12.09.2019 15:44, Juergen Gross wrote:
> On 09.09.19 16:35, Jan Beulich wrote:
>> On 09.08.2019 16:58, Juergen Gross wrote:
>>> --- a/xen/common/schedule.c
>>> +++ b/xen/common/schedule.c
>>> @@ -248,6 +248,20 @@ static inline void vcpu_runstate_change(
>>>       v->runstate.state = new_state;
>>>   }
>>>   
>>> +static inline void sched_unit_runstate_change(struct sched_unit *unit,
>>> +    bool running, s_time_t new_entry_time)
>>> +{
>>> +    struct vcpu *v = unit->vcpu_list;
>>> +
>>> +    if ( running )
>>> +        vcpu_runstate_change(v, RUNSTATE_running, new_entry_time);
>>> +    else
>>> +        vcpu_runstate_change(v,
>>> +            ((v->pause_flags & VPF_blocked) ? RUNSTATE_blocked :
>>> +             (vcpu_runnable(v) ? RUNSTATE_runnable : RUNSTATE_offline)),
>>> +            new_entry_time);
>>> +}
>>
>> I find it puzzling that this gets introduced, but won't survive till
>> the end of the series. I can only guess that you can't do without the
>> separation intermediately. Making such transient state more apparent
>> from the description would be nice imo.
> 
> The functionality will stay, but it will be subsumed in patch 35. I
> don't think I should mention that in the commit message, so do you want
> me to just add it below the "---" marker?

Don't know, to be honest. Apparently odd things like this one are
making review more difficult. I don't typically resort to looking at
the final resulting code, but if even that ends up confusing, it's
certainly something where a hint somewhere would be nice.

Jan
diff mbox series

Patch

diff --git a/xen/common/schedule.c b/xen/common/schedule.c
index d8402878d4..b87aec74b7 100644
--- a/xen/common/schedule.c
+++ b/xen/common/schedule.c
@@ -248,6 +248,20 @@  static inline void vcpu_runstate_change(
     v->runstate.state = new_state;
 }
 
+static inline void sched_unit_runstate_change(struct sched_unit *unit,
+    bool running, s_time_t new_entry_time)
+{
+    struct vcpu *v = unit->vcpu_list;
+
+    if ( running )
+        vcpu_runstate_change(v, RUNSTATE_running, new_entry_time);
+    else
+        vcpu_runstate_change(v,
+            ((v->pause_flags & VPF_blocked) ? RUNSTATE_blocked :
+             (vcpu_runnable(v) ? RUNSTATE_runnable : RUNSTATE_offline)),
+            new_entry_time);
+}
+
 void vcpu_runstate_get(struct vcpu *v, struct vcpu_runstate_info *runstate)
 {
     spinlock_t *lock = likely(v == current)
@@ -1613,7 +1627,7 @@  static void vcpu_periodic_timer_work(struct vcpu *v)
  */
 static void schedule(void)
 {
-    struct vcpu          *prev = current, *next = NULL;
+    struct sched_unit    *prev = current->sched_unit, *next = NULL;
     s_time_t              now;
     struct scheduler     *sched;
     unsigned long        *tasklet_work = &this_cpu(tasklet_work_to_do);
@@ -1657,9 +1671,9 @@  static void schedule(void)
     sched = this_cpu(scheduler);
     next_slice = sched->do_schedule(sched, now, tasklet_work_scheduled);
 
-    next = next_slice.task->vcpu_list;
+    next = next_slice.task;
 
-    sd->curr = next->sched_unit;
+    sd->curr = next;
 
     if ( next_slice.time >= 0 ) /* -ve means no limit */
         set_timer(&sd->s_timer, now + next_slice.time);
@@ -1668,59 +1682,55 @@  static void schedule(void)
     {
         pcpu_schedule_unlock_irq(lock, cpu);
         TRACE_4D(TRC_SCHED_SWITCH_INFCONT,
-                 next->domain->domain_id, next->vcpu_id,
-                 now - prev->runstate.state_entry_time,
+                 next->domain->domain_id, next->unit_id,
+                 now - prev->state_entry_time,
                  next_slice.time);
-        trace_continue_running(next);
-        return continue_running(prev);
+        trace_continue_running(next->vcpu_list);
+        return continue_running(prev->vcpu_list);
     }
 
     TRACE_3D(TRC_SCHED_SWITCH_INFPREV,
-             prev->domain->domain_id, prev->vcpu_id,
-             now - prev->runstate.state_entry_time);
+             prev->domain->domain_id, prev->unit_id,
+             now - prev->state_entry_time);
     TRACE_4D(TRC_SCHED_SWITCH_INFNEXT,
-             next->domain->domain_id, next->vcpu_id,
-             (next->runstate.state == RUNSTATE_runnable) ?
-             (now - next->runstate.state_entry_time) : 0,
+             next->domain->domain_id, next->unit_id,
+             (next->vcpu_list->runstate.state == RUNSTATE_runnable) ?
+             (now - next->state_entry_time) : 0,
              next_slice.time);
 
-    ASSERT(prev->runstate.state == RUNSTATE_running);
+    ASSERT(prev->vcpu_list->runstate.state == RUNSTATE_running);
 
     TRACE_4D(TRC_SCHED_SWITCH,
-             prev->domain->domain_id, prev->vcpu_id,
-             next->domain->domain_id, next->vcpu_id);
+             prev->domain->domain_id, prev->unit_id,
+             next->domain->domain_id, next->unit_id);
 
-    vcpu_runstate_change(
-        prev,
-        ((prev->pause_flags & VPF_blocked) ? RUNSTATE_blocked :
-         (vcpu_runnable(prev) ? RUNSTATE_runnable : RUNSTATE_offline)),
-        now);
+    sched_unit_runstate_change(prev, false, now);
 
-    ASSERT(next->runstate.state != RUNSTATE_running);
-    vcpu_runstate_change(next, RUNSTATE_running, now);
+    ASSERT(next->vcpu_list->runstate.state != RUNSTATE_running);
+    sched_unit_runstate_change(next, true, now);
 
     /*
      * NB. Don't add any trace records from here until the actual context
      * switch, else lost_records resume will not work properly.
      */
 
-    ASSERT(!next->sched_unit->is_running);
+    ASSERT(!next->is_running);
+    next->vcpu_list->is_running = 1;
     next->is_running = 1;
-    next->sched_unit->is_running = 1;
-    next->sched_unit->state_entry_time = now;
+    next->state_entry_time = now;
 
     pcpu_schedule_unlock_irq(lock, cpu);
 
     SCHED_STAT_CRANK(sched_ctx);
 
-    stop_timer(&prev->periodic_timer);
+    stop_timer(&prev->vcpu_list->periodic_timer);
 
     if ( next_slice.migrated )
-        sched_move_irqs(next);
+        sched_move_irqs(next->vcpu_list);
 
-    vcpu_periodic_timer_work(next);
+    vcpu_periodic_timer_work(next->vcpu_list);
 
-    context_switch(prev, next);
+    context_switch(prev->vcpu_list, next->vcpu_list);
 }
 
 void context_saved(struct vcpu *prev)