Re: [PATCH v3 3/3] VT-d PI: restrict the vcpu number on a given pcpu

[Chao Gao <chao.gao@intel.com>]

On Fri, Jun 23, 2017 at 01:58:52AM -0600, Jan Beulich wrote:
>>>> On 23.06.17 at 06:22, <chao.gao@intel.com> wrote:
>> On Fri, Jun 16, 2017 at 09:09:13AM -0600, Jan Beulich wrote:
>>>>>> On 24.05.17 at 08:56, <chao.gao@intel.com> wrote:
>>>> +    {
>>>> +        pi_cpu = cpumask_cycle(pi_cpu, &cpu_online_map);
>>>
>>>With this, how could the CPU be offline by the time you make it
>>>back to the check above.
>> 
>> Thanks to point it out. It would incur a bug.
>
>I don't understand what you're trying to tell me here.
>

I agree with you that we might use an offline CPU's structure.
and I treat it as a bug.

>> I think we should do things like this:
>> 
>> IF pi_blocking_list of current pcpu doesn't over the limit:
>> 	add the vcpu to current pcpu.
>> ELSE
>> 	add the vcpu to another pcpu.
>
>But that's what supposedly the patch already tries to do?

yes. it is. but I want to unclose a key problem here is
we may add a vcpu to an offline pcpu's pi blocking list. Try to
avoid this we can introduce a _new_ lock to avoid racing with
vmx_pi_desc_fixup(). This version tried to use existing lock 
ie. per_cpu(vmx_pi_blocking, pi_cpu).lock to solve the key problem.
But as you pointed out, it may use a offline CPU's structure.

>
>> To add the vcpu to another pcpu, we should avoid concurrency with
>> vmx_pi_desc_fixup(). Thus, a lock (e.g. remote_pi_list_lock)
>
>Please use names which actually exist in source code, or make
>clear what exactly you're referring to. Talking of
>remote_pi_list_lock, which I can't find any instance of, does not
>help the discussion, as you leave me guessing whose lock you
>mean to acquire.

It is a new lock and I try to envision how it can take effect.

>
>> can solve this potential concurrency. Using this lock like below:
>> 
>> in vmx_vcpu_block():
>> 
>> IF pi_blocking_list of current pcpu doesn't over the limit:
>> 	add the vcpu to current pcpu
>> ELSE
>> 	acquire remote_pi_list_lock
>> 	choose another online pcpu	(don't worry this pcpu would goes
>> 					 offline for we hold the
>> 					 remote_pi_list_lock, which blocks
>> 					 calling vmx_pi_desc_fixup(),
>> 					 thus at least we can add this
>> 					 vcpu to the pi_blocking_list
>> 					 before cleanup)
>
>I can't see why you need to hold a lock to make sure a pCPU doesn't
>go offline - pCPU offlining happens in stop_machine context anyway.

In cpu_down(), the cpu_online_bitmap is cleared by this line:
	if ( (err = stop_machine_run(take_cpu_down, NULL, cpu)) < 0 )

And vmx_pi_desc_fixup() is called later by
	notifier_call_chain(&cpu_chain, CPU_DEAD, hcpu, NULL);

So I think if we acquire the new lock, namely remote_pi_list_lock before
choose another online pcpu, we can make sure the vcpu will be added to
the chosen pcpu's pi blocking list prior to some cleanup to the same pi
blocking list.

Thanks
Chao

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