Re: [Xen-devel] [PATCH v2] x86/cpu: Sync any remaining RCU callbacks after CPU up/down

[Igor Druzhinin <igor.druzhinin@xxxxxxxxxx>]

On 25/02/2020 12:22, Jan Beulich wrote:
> On 21.02.2020 20:26, Igor Druzhinin wrote:
>> On 21/02/2020 16:29, Jan Beulich wrote:
>>> On 19.02.2020 18:25, Igor Druzhinin wrote:
>>>> --- a/xen/arch/x86/sysctl.c
>>>> +++ b/xen/arch/x86/sysctl.c
>>>> @@ -78,8 +78,11 @@ static void l3_cache_get(void *arg)
>>>>  long cpu_up_helper(void *data)
>>>>  {
>>>>      unsigned int cpu = (unsigned long)data;
>>>> -    int ret = cpu_up(cpu);
>>>> +    int ret;
>>>>  
>>>> +    /* Flush potentially scheduled RCU work from preceding CPU offline */
>>>> +    rcu_barrier();
>>>> +    ret = cpu_up(cpu);
>>>>      if ( ret == -EBUSY )
>>>>      {
>>>>          /* On EBUSY, flush RCU work and have one more go. */
>>>> @@ -104,7 +107,11 @@ long cpu_up_helper(void *data)
>>>>  long cpu_down_helper(void *data)
>>>>  {
>>>>      int cpu = (unsigned long)data;
>>>> -    int ret = cpu_down(cpu);
>>>> +    int ret;
>>>> +
>>>> +    /* Flush potentially scheduled RCU work from preceding CPU online */
>>>> +    rcu_barrier();
>>>> +    ret = cpu_down(cpu);
>>>>      if ( ret == -EBUSY )
>>>>      {
>>>>          /* On EBUSY, flush RCU work and have one more go. */
>>>>
>>>
>>> There are more calls to cpu_up() / cpu_down(), most notably in
>>> core_parking.c. Wouldn't it be better to make the barrier part
>>> of the two functions? This would the also cover non-x86 in
>>> case an arch wants to support offlining/onlining of CPUs.
>>
>> Those functions are called from early init code and rcu_barrier() is
>> an expensive operation. I think it's better if caller is responsible
>> for syncing the state. This is the reason I moved rcu_barrier() in front
>> of cpu_up/down.
> 
> Well, there are two aspects here: One is to avoid the overhead where
> it's not needed. The other is, as observed on this patch, that by
> the chosen approach callers which in fact need amending may be
> forgotten. To find middle grounds, perhaps the solution is to have
> variants of cpu_{up,down}() which invoke the barrier, and which
> would be used by all runtime invocations?
> 
> The other question of course is why early init code is special in
> this regard. If it indeed was, perhaps the barrier invocation could
> also be tied to certain SYS_STATE_* values?

It's not special - in fact it starts after RCU is initialized. The issue
is, as you said, unnecessary overhead. I'd prefer to avoid any conditional
magic and instead call "nosync" version directly from setup code.

Igor

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