[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [PATCH] x86/irq: Improve local_irq_restore() code generation and performance


  • To: Andrew Cooper <amc96@xxxxxxxx>, Andrew Cooper <andrew.cooper3@xxxxxxxxxx>
  • From: Jan Beulich <jbeulich@xxxxxxxx>
  • Date: Mon, 6 Dec 2021 16:15:21 +0100
  • Arc-authentication-results: i=1; mx.microsoft.com 1; spf=pass smtp.mailfrom=suse.com; dmarc=pass action=none header.from=suse.com; dkim=pass header.d=suse.com; arc=none
  • Arc-message-signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=microsoft.com; s=arcselector9901; h=From:Date:Subject:Message-ID:Content-Type:MIME-Version:X-MS-Exchange-AntiSpam-MessageData-ChunkCount:X-MS-Exchange-AntiSpam-MessageData-0:X-MS-Exchange-AntiSpam-MessageData-1; bh=0Xuibp43/JaJ8UQdYMRsGOxHv7rpHvG3g7rC77lZ7Ag=; b=J2tAHx4Qp0LyG227+eirwKPtCNGfSWI21a+CZy6tCMIiPbrpiMyM35z/Dt7jwExf8K0ylsHleV5SE6UCVtp7xfPBMwuZjZ/Nn++XJnh7/AI+1WDpUxYoqHeUWGJPfX55O8SIEwGGv9qiLV81Mpq3JkY+EOOGMfGe1kE1q9KkJRWh1Gv0xPSb1TQo8pxJh55wL9ZryrsX/6+B4Cs2qomsdI7p3Kxx3CCG9rd1xypeiZp+OvLEk1gy5no2oSqcp17OZqen917T3V1C42yNC4g0thy6mArAAGBBb6LcI7Zz8R3BTsduv3dyzbT2vGcqpf04Dt/qX/zEhwh7kOpw0xnZqw==
  • Arc-seal: i=1; a=rsa-sha256; s=arcselector9901; d=microsoft.com; cv=none; b=Td12/nKndSIL/LLNj+ew2zetkaBvTP1JNmiHoUkvTNRJxsXBo+0S0GcezU6SInGe+LrQ6UBL0vPVFkWLlS4itqLDqdiA/X/Bt2QR4fUSpbpBDurHXfHlju8mLCnsoeNBIq1rhBjaX55W3AifsaFV9FHGIhFXGU0yb1A+jjikM/mJLRqekmdoEG9Sy363GYN8KKwB7B7BuiH+IdV6FfbdBx0P7Ose35c4JtPMlP8lTWyqQK2ton8ptUG3UvJuxoexxTRizXCZp00QAgHPrfahP+wU9fpvQD2oYKhklFEtTE5JRiifv6pMA/3ODBDh0IAQZ4z0mB5EmAUM4FEOHRU3Bg==
  • Authentication-results: dkim=none (message not signed) header.d=none;dmarc=none action=none header.from=suse.com;
  • Cc: Roger Pau Monné <roger.pau@xxxxxxxxxx>, Wei Liu <wl@xxxxxxx>, Xen-devel <xen-devel@xxxxxxxxxxxxxxxxxxxx>
  • Delivery-date: Mon, 06 Dec 2021 15:15:36 +0000
  • List-id: Xen developer discussion <xen-devel.lists.xenproject.org>

On 06.12.2021 16:10, Andrew Cooper wrote:
> On 06/12/2021 14:07, Jan Beulich wrote:
>> On 06.12.2021 14:55, Andrew Cooper wrote:
>>> On 06/12/2021 13:38, Andrew Cooper wrote:
>>>> popf is a horribly expensive instruction, while sti is an optimised 
>>>> fastpath.
>>>> Switching popf for a conditional branch and sti caused an 8% perf 
>>>> improvement
>>>> in various linux measurements.
>>>>
>>>> Signed-off-by: Andrew Cooper <andrew.cooper3@xxxxxxxxxx>
>>>> ---
>>>> CC: Jan Beulich <JBeulich@xxxxxxxx>
>>>> CC: Roger Pau Monné <roger.pau@xxxxxxxxxx>
>>>> CC: Wei Liu <wl@xxxxxxx>
>>>> ---
>>>>  xen/include/asm-x86/system.h | 9 ++-------
>>>>  1 file changed, 2 insertions(+), 7 deletions(-)
>>>>
>>>> diff --git a/xen/include/asm-x86/system.h b/xen/include/asm-x86/system.h
>>>> index 65e63de69a67..4be235472ecd 100644
>>>> --- a/xen/include/asm-x86/system.h
>>>> +++ b/xen/include/asm-x86/system.h
>>>> @@ -267,13 +267,8 @@ static inline unsigned long 
>>>> array_index_mask_nospec(unsigned long index,
>>>>  })
>>>>  #define local_irq_restore(x)                                     \
>>>>  ({                                                               \
>>>> -    BUILD_BUG_ON(sizeof(x) != sizeof(long));                     \
>>>> -    asm volatile ( "pushfq\n\t"                                  \
>>>> -                   "andq %0, (%%rsp)\n\t"                        \
>>>> -                   "orq  %1, (%%rsp)\n\t"                        \
>>>> -                   "popfq"                                       \
>>>> -                   : : "i?r" ( ~X86_EFLAGS_IF ),                 \
>>>> -                       "ri" ( (x) & X86_EFLAGS_IF ) );           \
>>>> +    if ( (x) & X86_EFLAGS_IF )                                   \
>>>> +        local_irq_enable();                                      \
>>>>  })
>>> Bah.  There's still the one total abuse of local_irq_restore() to
>>> disable interrupts.
>> Question is whether that's really to be considered an abuse:
> 
> These are Linux's APIs, not ours, and they've spoken on the matter. 
> Furthermore, I agree with this being an abuse of the mechanism.
> 
>>  To me
>> "restore" doesn't mean only "maybe re-enable", but also "maybe
>> re-disable".
> 
> nor does "save" mean "save and disable", but that's what it does.
> 
> The naming may not be completely ideal, but the expected usage is very
> much one way.
> 
>>  And a conditional STI-or-CLI is likely still be better
>> than POPF.
> 
> It likely is better than popf, but for one single abuse which can be
> written in a better way anyway, it's really not worth it.

Fine with me as long as we can be very certain that's it's really only
one such case.

Jan




 


Rackspace

Lists.xenproject.org is hosted with RackSpace, monitoring our
servers 24x7x365 and backed by RackSpace's Fanatical Support®.