Re: [Xen-devel] [PATCH v1 2/2] xen/arm: Fix deadlock in on_selected_cpus function

[Oleksandr Tyshchenko <oleksandr.tyshchenko@xxxxxxxxxxxxxxx>]

On Tue, Jan 28, 2014 at 3:58 PM, Stefano Stabellini
<stefano.stabellini@xxxxxxxxxxxxx> wrote:
> On Mon, 27 Jan 2014, Oleksandr Tyshchenko wrote:
>> This patch is needed to avoid possible deadlocks in case of simultaneous
>> occurrence cross-interrupts.
>>
>> Change-Id: I574b496442253a7b67a27e2edd793526c8131284
>> Signed-off-by: Oleksandr Tyshchenko <oleksandr.tyshchenko@xxxxxxxxxxxxxxx>
>> Signed-off-by: Andrii Tseglytskyi <andrii.tseglytskyi@xxxxxxxxxxxxxxx>
>> ---
>>  xen/common/smp.c |    6 +++++-
>>  1 file changed, 5 insertions(+), 1 deletion(-)
>>
>> diff --git a/xen/common/smp.c b/xen/common/smp.c
>> index 2700bd7..46d2fc6 100644
>> --- a/xen/common/smp.c
>> +++ b/xen/common/smp.c
>> @@ -55,7 +55,11 @@ void on_selected_cpus(
>>
>>      ASSERT(local_irq_is_enabled());
>>
>> -    spin_lock(&call_lock);
>> +    if (!spin_trylock(&call_lock)) {
>> +        if (smp_call_function_interrupt())
>> +            return;
>
> If smp_call_function_interrupt returns -EPERM, shouldn't we go back to
> spinning on call_lock?
> Also there is a race condition between spin_lock, cpumask_copy and
> smp_call_function_interrupt: smp_call_function_interrupt could be called
> on cpu1 after cpu0 acquired the lock, but before cpu0 set
> call_data.selected.
>
> I think the correct implemention would be:
>
>
> while ( unlikely(!spin_trylock(&call_lock)) )
>     smp_call_function_interrupt();
I completely agree
>
>
>
>> +        spin_lock(&call_lock);
>> +    }
>>
>>      cpumask_copy(&call_data.selected, selected);
>>
>> --
>> 1.7.9.5
>>
>>
>> _______________________________________________
>> Xen-devel mailing list
>> Xen-devel@xxxxxxxxxxxxx
>> http://lists.xen.org/xen-devel
>>

-- 

Oleksandr Tyshchenko | Embedded Developer
GlobalLogic

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