linux-kernel - Re: [PATCH 1/1] epoll: remove wrong assert that ep_poll

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <2cec506322600c3d2b59f2ca9bcd59dc@suse.de>
Date:   Tue, 08 Jan 2019 17:07:23 +0100
From:   Roman Penyaev <rpenyaev@...e.de>
To:     Davidlohr Bueso <dbueso@...e.de>
Cc:     Jason Baron <jbaron@...mai.com>, Al Viro <viro@...iv.linux.org.uk>,
        Andrew Morton <akpm@...ux-foundation.org>,
        Linus Torvalds <torvalds@...ux-foundation.org>,
        linux-fsdevel@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH 1/1] epoll: remove wrong assert that ep_poll_callback is
 always called with irqs off

On 2019-01-08 16:16, Davidlohr Bueso wrote:
> On 2019-01-08 04:42, Roman Penyaev wrote:
>> What we can do:
>> 
>> a) disable irqs if we are not in interrupt.
>> b) revert the patch completely.
>> 
>> David, is it really crucial in terms of performance to avoid double
>> local_irq_save() on Xen on this ep_poll_callback() hot path?
> 
> Note that such optimizations are also relevant for baremetal, ie: x86
> PUSHF + POPF can be pretty expensive because of insn dependencies.
> 
>> 
>> For example why not to do the following:
>> 
>>   if (!in_interrupt())
>>        local_irq_save(flags);
>>   read_lock(ep->lock);
>> 
>> with huge comment explaining performance number.
>> 
>> Or just give up and simply revert the original patch completely
>> and always call read_lock_irqsave().
> 
> Yeah so the reason why I had done the other epoll lock irq
> optimizations was because they were painfully obvious.
> ep_poll_callback(), however is a different beast, as you've
> encountered. I vote for not shooting ourselves in the foot and just
> dropping this patch -- most large performance benefits will come from
> microbenches anyway.

Then I will send another patch which changes read_lock() on 
read_lock_irqsave(),
since simple revert of the original patch won't work.

Thanks.

--
Roman