Re: [Xen-devel] [PATCH v2 29/48] xen/sched: add code to sync scheduling of all vcpus of a sched unit

[Jan Beulich <jbeulich@xxxxxxxx>]

On 09.08.2019 16:58, Juergen Gross wrote:
> +static bool sched_tasklet_check(unsigned int cpu)
> +{
> +    bool tasklet_work_scheduled = false;
> +    const cpumask_t *mask = get_sched_res(cpu)->cpus;
> +    int cpu_iter;

unsigned int ?

> +static void context_saved(struct vcpu *prev)
> +{
> +    struct sched_unit *unit = prev->sched_unit;
> +
> +    /* Clear running flag /after/ writing context to memory. */
> +    smp_wmb();
> +
> +    prev->is_running = 0;
> +    unit->is_running = 0;
> +    unit->state_entry_time = NOW();
> +
> +    /* Check for migration request /after/ clearing running flag. */
> +    smp_mb();
> +
> +    sched_context_saved(vcpu_scheduler(prev), unit);
> +
> +    sched_unit_migrate_finish(unit);
> +}
> +
> +/*
> + * Rendezvous on end of context switch.
> + * As no lock is protecting this rendezvous function we need to use atomic
> + * access functions on the counter.
> + * The counter will be 0 in case no rendezvous is needed. For the rendezvous
> + * case it is initialised to the number of cpus to rendezvous plus 1. Each
> + * member entering decrements the counter. The last one will decrement it to
> + * 1 and perform the final needed action in that case (call of 
> context_saved()
> + * if vcpu was switched), and then set the counter to zero. The other members
> + * will wait until the counter becomes zero until they proceed.
> + */
> +void sched_context_switched(struct vcpu *vprev, struct vcpu *vnext)
> +{
> +    struct sched_unit *next = vnext->sched_unit;
> +
> +    if ( atomic_read(&next->rendezvous_out_cnt) )
> +    {
> +        int cnt = atomic_dec_return(&next->rendezvous_out_cnt);
> +
> +        /* Call context_saved() before releasing other waiters. */
> +        if ( cnt == 1 )
> +        {
> +            if ( vprev != vnext )
> +                context_saved(vprev);
> +            atomic_set(&next->rendezvous_out_cnt, 0);
> +        }
> +        else
> +            while ( atomic_read(&next->rendezvous_out_cnt) )
> +                cpu_relax();

How come context_saved() is not called on this "else" branch? How
will vprev->is_running get cleared there? Or, since everything
else in the function is per-unit, does this clearing want to move
here?

> -void context_saved(struct vcpu *prev)
> +static void sched_slave(void)
>  {
> -    /* Clear running flag /after/ writing context to memory. */
> -    smp_wmb();
> +    struct vcpu          *vprev = current;
> +    struct sched_unit    *prev = vprev->sched_unit, *next;
> +    s_time_t              now;
> +    spinlock_t           *lock;
> +    int cpu = smp_processor_id();

unsigned int?

> @@ -1971,6 +2164,7 @@ void __init scheduler_init(void)
>      int i;
>  
>      open_softirq(SCHEDULE_SOFTIRQ, schedule);
> +    open_softirq(SCHED_SLAVE_SOFTIRQ, sched_slave);

Noticing the "we have a race" comment and code in schedule() I
wonder if there isn't enough state for schedule() to know
whether to call sched_slave(), rather than having this extra
softirq.

Jan

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