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

From: Juergen Gross <jgross@suse.com>
To: Jan Beulich <jbeulich@suse.com>
Cc: "Tim Deegan" <tim@xen.org>,
	"Stefano Stabellini" <sstabellini@kernel.org>,
	"Wei Liu" <wl@xen.org>,
	"Konrad Rzeszutek Wilk" <konrad.wilk@oracle.com>,
	"George Dunlap" <George.Dunlap@eu.citrix.com>,
	"Andrew Cooper" <andrew.cooper3@citrix.com>,
	"Ian Jackson" <ian.jackson@eu.citrix.com>,
	"Dario Faggioli" <dfaggioli@suse.com>,
	"Julien Grall" <julien.grall@arm.com>,
	xen-devel@lists.xenproject.org,
	"Volodymyr Babchuk" <Volodymyr_Babchuk@epam.com>,
	"Roger Pau Monné" <roger.pau@citrix.com>
Subject: Re: [Xen-devel] [PATCH v2 29/48] xen/sched: add code to sync scheduling of all vcpus of a sched unit
Date: Fri, 13 Sep 2019 15:12:27 +0200	[thread overview]
Message-ID: <0a38b0cf-cc53-a672-f5ce-52169335f59c@suse.com> (raw)
In-Reply-To: <8d1e2ee8-a80f-2aa8-d10e-c549439bac87@suse.com>

On 10.09.19 17:36, Jan Beulich wrote:
> 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 ?

Yes.

> 
>> +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?

That's a bug I've already corrected in my version 3 (and the one which
is being tested by Sergey).

> 
>> -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?

Yes.

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

Especially patch 35 adds further using of SCHED_SLAVE_SOFTIRQ. I have
tried to avoid it, but the results were looking rather ugly and still
full of deadlocks.

Juergen

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