From mboxrd@z Thu Jan  1 00:00:00 1970
From: Avi Kivity <avi@redhat.com>
Subject: Re: [patch 1/8] test: allow functions to execute on non-irq context
 remotely
Date: Sun, 28 Mar 2010 09:32:04 +0300
Message-ID: <4BAEF7E4.6060504@redhat.com>
References: <20100324212408.790319364@amt.cnet> <20100324212725.523800252@amt.cnet> <4BAB8E94.5090902@redhat.com> <20100325180727.GA23070@amt.cnet>
Mime-Version: 1.0
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 7bit
Cc: kvm@vger.kernel.org
To: Marcelo Tosatti <mtosatti@redhat.com>
Return-path: <kvm-owner@vger.kernel.org>
Received: from mx1.redhat.com ([209.132.183.28]:1027 "EHLO mx1.redhat.com"
	rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP
	id S1753605Ab0C1GcI (ORCPT <rfc822;kvm@vger.kernel.org>);
	Sun, 28 Mar 2010 02:32:08 -0400
Received: from int-mx02.intmail.prod.int.phx2.redhat.com (int-mx02.intmail.prod.int.phx2.redhat.com [10.5.11.12])
	by mx1.redhat.com (8.13.8/8.13.8) with ESMTP id o2S6W7Ns024905
	(version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-SHA bits=256 verify=OK)
	for <kvm@vger.kernel.org>; Sun, 28 Mar 2010 02:32:07 -0400
In-Reply-To: <20100325180727.GA23070@amt.cnet>
Sender: kvm-owner@vger.kernel.org
List-ID: <kvm.vger.kernel.org>

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.