Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1722097 > unrolled thread

Re: [PATCH] MIPS: Revert "MIPS: Fix race on setting and getting cpu_online_mask"

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  Re: [PATCH] MIPS: Revert "MIPS: Fix race on setting and getting cpu_online_mask" Huacai Chen <chenhuacai@gmail.com> - 2017-08-29 03:50 +0200
    Re: [PATCH] MIPS: Revert "MIPS: Fix race on setting and getting  cpu_online_mask" Matt Redfearn <matt.redfearn@imgtec.com> - 2017-08-30 15:30 +0200
      Re: [PATCH] MIPS: Revert "MIPS: Fix race on setting and getting cpu_online_mask" Huacai Chen <chenhuacai@gmail.com> - 2017-08-31 09:10 +0200

#1722097 — Re: [PATCH] MIPS: Revert "MIPS: Fix race on setting and getting cpu_online_mask"

I suggest to drop sync_r4k completely, because it is inaccurate. You
can use IPI to synchronize count/compare instead, as Loongson-3 does.

Huacai

On Mon, Aug 28, 2017 at 6:07 PM, Matija Glavinic Pecotic
<matija.glavinic-pecotic.ext@nokia.com> wrote:
> On 08/23/2017 10:21 AM, Matt Redfearn wrote:
>> As noted in the commit message, upstream differs in this area. The
>> hotplug code now waits on a completion event in bringup_wait_for_ap,
>> which is set by the starting CPU in cpuhp_online_idle once it calls
>> cpu_startup_entry. Thus there is no possibility of a race in upstream,
>> and this commit has only re-introduced the deadlock condition, which can
>> be observed on multiple platforms when running a heavy load test at the
>> same time as hotplugging CPUs. See commit 8f46cca1e6c06 ("MIPS: SMP: Fix
>> possibility of deadlock when bringing CPUs online") for details.
>
> I personally do not like the fact that synchronization is implicitly done by the callers, it is the reason why the patch was proposed. As noted before, it is enough someone checks cpu online mask somewhere in between and there is race again.
>
> How about moving synchronise_count_slave before setting the cpu online? Is there dependency it has to be done after completion?
>
> Regards,
>
> Matija
>

[toc] | [next] | [standalone]

#1723332 — Re: [PATCH] MIPS: Revert "MIPS: Fix race on setting and getting cpu_online_mask"

Hi Huacai,

On 29/08/17 02:43, Huacai Chen wrote:
> I suggest to drop sync_r4k completely, because it is inaccurate. You
> can use IPI to synchronize count/compare instead, as Loongson-3 does.

I am all for a better fix, such as this - but that would be a much more 
invasive change than what I propose. Currently 4.13 is broken by the 
patch that this is attempting to revert. It is easy to deadlock the 
system by hotplugging a CPU while it is busy. That was what my patch 
8f46cca1e6c06 originally fixed. Even though it is, perhaps, not 
stylistically great to have the synchronisation done by callers, the 
fact is that it *is* done (added in 8df3e07e7f21f), so the behavior for 
4.13 would be safe and deadlocks not possible. We can then look at more 
invasive changes that are acceptable to everyone during the 4.14 cycle.

Thanks,
Matt

>
> Huacai
>
> On Mon, Aug 28, 2017 at 6:07 PM, Matija Glavinic Pecotic
> <matija.glavinic-pecotic.ext@nokia.com> wrote:
>> On 08/23/2017 10:21 AM, Matt Redfearn wrote:
>>> As noted in the commit message, upstream differs in this area. The
>>> hotplug code now waits on a completion event in bringup_wait_for_ap,
>>> which is set by the starting CPU in cpuhp_online_idle once it calls
>>> cpu_startup_entry. Thus there is no possibility of a race in upstream,
>>> and this commit has only re-introduced the deadlock condition, which can
>>> be observed on multiple platforms when running a heavy load test at the
>>> same time as hotplugging CPUs. See commit 8f46cca1e6c06 ("MIPS: SMP: Fix
>>> possibility of deadlock when bringing CPUs online") for details.
>> I personally do not like the fact that synchronization is implicitly done by the callers, it is the reason why the patch was proposed. As noted before, it is enough someone checks cpu online mask somewhere in between and there is race again.
>>
>> How about moving synchronise_count_slave before setting the cpu online? Is there dependency it has to be done after completion?
>>
>> Regards,
>>
>> Matija
>>

[toc] | [prev] | [next] | [standalone]

#1723882

Yes, your original patch (8f46cca1e6c06) is needed in 4.12/4.13, but
they should be reverted in 4.1/4.4-stable branch.

Huacai

On Wed, Aug 30, 2017 at 9:24 PM, Matt Redfearn <matt.redfearn@imgtec.com> wrote:
> Hi Huacai,
>
> On 29/08/17 02:43, Huacai Chen wrote:
>>
>> I suggest to drop sync_r4k completely, because it is inaccurate. You
>> can use IPI to synchronize count/compare instead, as Loongson-3 does.
>
>
> I am all for a better fix, such as this - but that would be a much more
> invasive change than what I propose. Currently 4.13 is broken by the patch
> that this is attempting to revert. It is easy to deadlock the system by
> hotplugging a CPU while it is busy. That was what my patch 8f46cca1e6c06
> originally fixed. Even though it is, perhaps, not stylistically great to
> have the synchronisation done by callers, the fact is that it *is* done
> (added in 8df3e07e7f21f), so the behavior for 4.13 would be safe and
> deadlocks not possible. We can then look at more invasive changes that are
> acceptable to everyone during the 4.14 cycle.
>
> Thanks,
> Matt
>
>
>>
>> Huacai
>>
>> On Mon, Aug 28, 2017 at 6:07 PM, Matija Glavinic Pecotic
>> <matija.glavinic-pecotic.ext@nokia.com> wrote:
>>>
>>> On 08/23/2017 10:21 AM, Matt Redfearn wrote:
>>>>
>>>> As noted in the commit message, upstream differs in this area. The
>>>> hotplug code now waits on a completion event in bringup_wait_for_ap,
>>>> which is set by the starting CPU in cpuhp_online_idle once it calls
>>>> cpu_startup_entry. Thus there is no possibility of a race in upstream,
>>>> and this commit has only re-introduced the deadlock condition, which can
>>>> be observed on multiple platforms when running a heavy load test at the
>>>> same time as hotplugging CPUs. See commit 8f46cca1e6c06 ("MIPS: SMP: Fix
>>>> possibility of deadlock when bringing CPUs online") for details.
>>>
>>> I personally do not like the fact that synchronization is implicitly done
>>> by the callers, it is the reason why the patch was proposed. As noted
>>> before, it is enough someone checks cpu online mask somewhere in between and
>>> there is race again.
>>>
>>> How about moving synchronise_count_slave before setting the cpu online?
>>> Is there dependency it has to be done after completion?
>>>
>>> Regards,
>>>
>>> Matija
>>>
>

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web