Re: [Xen-devel] [PATCH RFC 01/49] xen/sched: call cpu_disable_scheduler() via cpu notifier

[Juergen Gross <jgross@xxxxxxxx>]

On 01/04/2019 16:01, Julien Grall wrote:
> Hi,
> 
> On 4/1/19 2:33 PM, Juergen Gross wrote:
>> On 01/04/2019 15:21, Julien Grall wrote:
>>> Hi Juergen,
>>>
>>> On 4/1/19 11:37 AM, Juergen Gross wrote:
>>>> On 01/04/2019 12:29, Julien Grall wrote:
>>>>> Hi,
>>>>>
>>>>> On 4/1/19 10:40 AM, Juergen Gross wrote:
>>>>>> On 01/04/2019 11:21, Julien Grall wrote:
>>>>>>> Hi,
>>>>>>>
>>>>>>> On 3/29/19 3:08 PM, Juergen Gross wrote:
>>>>>>>> cpu_disable_scheduler() is being called from __cpu_disable() today.
>>>>>>>> There is no need to execute it on the cpu just being disabled,
>>>>>>>> so use
>>>>>>>> the CPU_DEAD case of the cpu notifier chain. Moving the call out of
>>>>>>>> stop_machine() context is fine, as we just need to hold the domain
>>>>>>>> RCU
>>>>>>>> lock and need the scheduler percpu data to be still allocated.
>>>>>>>>
>>>>>>>> Add another hook for CPU_DOWN_PREPARE to bail out early in case
>>>>>>>> cpu_disable_scheduler() would fail. This will avoid crashes in rare
>>>>>>>> cases for cpu hotplug or suspend.
>>>>>>>>
>>>>>>>> While at it remove a superfluous smp_mb() in the ARM
>>>>>>>> __cpu_disable()
>>>>>>>> incarnation.
>>>>>>>
>>>>>>> This is not obvious why the smp_mb() is superfluous. Can you please
>>>>>>> provide more details on why this is not necessary?
>>>>>>
>>>>>> cpumask_clear_cpu() should already have the needed semantics, no?
>>>>>> It is based on clear_bit() which is defined to be atomic.
>>>>>
>>>>> atomicity does not mean the store/load cannot be re-ordered by the
>>>>> CPU.
>>>>> You would need a barrier to prevent re-ordering.
>>>>>
>>>>> cpumask_clear_cpu() and clear_bit() does not contain any barrier, so
>>>>> store/load can be re-ordered.
>>>>
>>>> Uh, couldn't this lead to problems, e.g. in vcpu_block()? The comment
>>>> there suggests the sequence of setting the blocked bit and doing the
>>>> test is important for avoiding a race...
>>>
>>> Hmmm... looking at the other usage (such as in do_poll), on non-x86
>>> platform, there is a smp_mb() between set_bit(...) and checking the
>>> event with a similar comment above.
>>>
>>> I don't know enough the scheduler code to know why the barrier is
>>> needed. But for consistency, it seems to me the smp_mb() would be
>>> required in vcpu_block() as well.
>>>
>>> Also, it is quite interesting that the barrier is not presence for x86.
>>> If I understand correctly the comment on top of set_bit/clear_bit, it
>>> could as well be re-ordered. So we seem to relying on the underlying
>>> implementation of set_bit/clear_bit.
>>
>> On x86 reads and writes can't be reordered with locked operations (SDM
>> Vol 3 8.2.2). So the barrier is really not needed AFAIU.
>>
>> include/asm-x86/bitops.h:
>>
>>   * clear_bit() is atomic and may not be reordered.
> 
> I interpreted the "may not" as you should not rely on the re-ordering to
> not happen.
> 
> In place were re-ordering should not happen (e.g test_and_set_bit) we
> use the wording "cannot".

The SDM is very clear here:

"Reads or writes cannot be reordered with I/O instructions, locked
 instructions, or serializing instructions."

>>> Wouldn't it make sense to try to uniformize the semantics? Maybe by
>>> introducing a new helper?
>>
>> Or adding the barrier on ARM for the atomic operations?
> 
> On which basis?  Why should we impact every users for fixing a bug in
> the scheduler?

I'm assuming there are more places like this either in common code or
code copied verbatim from arch/x86 to arch/arm with that problem.

So I take it you'd rather let me add that smp_mb() in __cpu_disable()
again.


Juergen

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