Re: [patch 1/8] test: allow functions to execute on non-irq context remotely

[Avi Kivity <avi@redhat.com>]

On 03/25/2010 08:07 PM, Marcelo Tosatti wrote:
> On Thu, Mar 25, 2010 at 06:25:56PM +0200, Avi Kivity wrote:
>    
>> On 03/24/2010 11:24 PM, Marcelo Tosatti wrote:
>>      
>>> Which allows code to execute on remote cpus while receiving interrupts.
>>>
>>> Also move late smp initialization to common code, and the smp loop
>>> to C code.
>>>        
>>
>>      
>>> +
>>> +void smp_loop(void)
>>> +{
>>> +    void (*fn)(void *data);
>>> +    void *data;
>>> +
>>> +    asm volatile ("hlt");
>>>        
>> Racy.  The interrupt can happen before the hlt, which will kill the
>> cpu.
>>      
> Why would it kill the cpu? Only miss the event AFAICS. See patch below.
>    

If the other cpu won't send it a new event until this one is completed.


>> Needs to be
>>
>>      cli
>>      while not smp_function():
>>          sti; hlt
>>          cli
>>      sti
>>      smp_function()(smp_data())
>>
>> Also need to make sure two on_cpu_noipi()s don't stomp on each other.
>>      
> Only cpu0 requests this ATM, and the IPIs to set function/data are
> protected by a spinlock which serializes between cpu0<->target.
>
> Its responsability of the caller to synchronization with termination.
>
> Are you OK with this:
>    

> +
> +static void irq_disable(void)
> +{
> +    asm volatile("cli");
> +}
> +
> +static void irq_enable(void)
> +{
> +    asm volatile("sti");
> +}
> +
> +void smp_loop(void)
> +{
> +    void (*fn)(void *data);
> +
> +    irq_disable();
> +    fn = smp_function();
> +    if (fn) {
> +        setup_smp_function(0);
> +        irq_enable();
> +        fn(smp_data());
> +        irq_disable();
> +    }
> +
> +    irq_enable();
> +    asm volatile ("hlt");
> +    irq_disable();
>    

sti; hlt need to be in one asm statement so the compiler does't insert 
anything (for example a call insn) or the race window opens again.

Even then, can't two cpus overwrite a third cpu's smp_function()?

The most worrying bit is that this follows no known model so the reader 
doesn't know what to expect.  on_cpu() follows Linux, but on_cpu_noipi 
has no parallel.

What about switching to a thread model?  Have a single global runqueue, 
round robin threads (no preemption, only on calls to schedule()), use 
cpus_allowed to pin a thread to a cpu.


-- 
Do not meddle in the internals of kernels, for they are subtle and quick to panic.