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

Avi Kivity
2010-03-28T06:32:08Z

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.