linux-kernel - Re: [PATCH RESEND v4] fs/epoll: Remove unnecessary wakeups of nested epoll that in ET mode

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] [thread-next>] [day] [month] [year] [list]

Message-ID: <f2083f2862f9c2197576900ae0771e32@suse.de>
Date:   Tue, 08 Oct 2019 11:55:58 +0200
From:   Roman Penyaev <rpenyaev@...e.de>
To:     Jason Baron <jbaron@...mai.com>
Cc:     Andrew Morton <akpm@...ux-foundation.org>, hev <r@....cc>,
        linux-fsdevel@...r.kernel.org, Al Viro <viro@...iv.linux.org.uk>,
        Davide Libenzi <davidel@...ilserver.org>,
        Davidlohr Bueso <dave@...olabs.net>,
        Dominik Brodowski <linux@...inikbrodowski.net>,
        Eric Wong <e@...24.org>,
        Linus Torvalds <torvalds@...ux-foundation.org>,
        Sridhar Samudrala <sridhar.samudrala@...el.com>,
        linux-kernel@...r.kernel.org
Subject: Re: [PATCH RESEND v4] fs/epoll: Remove unnecessary wakeups of nested
 epoll that in ET mode

On 2019-10-07 20:43, Jason Baron wrote:

[...]

>> But what if to make this wakeup explicit if we have more events to 
>> process?
>> (nothing is tested, just a guess)
>> 
>> @@ -255,6 +255,7 @@ struct ep_pqueue {
>>  struct ep_send_events_data {
>>         int maxevents;
>>         struct epoll_event __user *events;
>> +       bool have_more;
>>         int res;
>>  };
>> @@ -1783,14 +1768,17 @@ static __poll_t ep_send_events_proc(struct
>> eventpoll *ep, struct list_head *head
>>  }
>> 
>>  static int ep_send_events(struct eventpoll *ep,
>> -                         struct epoll_event __user *events, int 
>> maxevents)
>> +                         struct epoll_event __user *events, int 
>> maxevents,
>> +                         bool *have_more)
>>  {
>> -       struct ep_send_events_data esed;
>> -
>> -       esed.maxevents = maxevents;
>> -       esed.events = events;
>> +       struct ep_send_events_data esed = {
>> +               .maxevents = maxevents,
>> +               .events = events,
>> +       };
>> 
>>         ep_scan_ready_list(ep, ep_send_events_proc, &esed, 0, false);
>> +       *have_more = esed.have_more;
>> +
>>         return esed.res;
>>  }
>> 
>> @@ -1827,7 +1815,7 @@ static int ep_poll(struct eventpoll *ep, struct
>> epoll_event __user *events,
>>  {
>>         int res = 0, eavail, timed_out = 0;
>>         u64 slack = 0;
>> -       bool waiter = false;
>> +       bool waiter = false, have_more;
>>         wait_queue_entry_t wait;
>>         ktime_t expires, *to = NULL;
>> 
>> @@ -1927,7 +1915,8 @@ static int ep_poll(struct eventpoll *ep, struct
>> epoll_event __user *events,
>>          * more luck.
>>          */
>>         if (!res && eavail &&
>> -           !(res = ep_send_events(ep, events, maxevents)) && 
>> !timed_out)
>> +           !(res = ep_send_events(ep, events, maxevents, &have_more)) 
>> &&
>> +           !timed_out)
>>                 goto fetch_events;
>> 
>>         if (waiter) {
>> @@ -1935,6 +1924,12 @@ static int ep_poll(struct eventpoll *ep, struct
>> epoll_event __user *events,
>>                 __remove_wait_queue(&ep->wq, &wait);
>>                 spin_unlock_irq(&ep->wq.lock);
>>         }
>> +       /*
>> +        * We were not able to process all the events, so immediately
>> +        * wakeup other waiter.
>> +        */
>> +       if (res > 0 && have_more && waitqueue_active(&ep->wq))
>> +               wake_up(&ep->wq);
>> 
>>         return res;
>>  }
>> 
>> 
> 

[...]

> And I think the above change can go in separately (if we decide we want 
> it).

Hi Jason,

I did measurements using Eric's test http://yhbt.net/eponeshotmt.c
(8 writers, 8 waiters;  1 writer, 8 waiters) and tested the impact
of outrunning wakeup: I do not see any difference. Since write events
are constantly coming, next waiter will be woken up anyway by the
following write event.  In order to have some perf gain probably writes
should happen with some interval: produce bunch of events, sleep,
produce bunch of events, sleep, etc, which seems can bring something
only if writer is accidentally synchronized with waiters. Not a clean
way of perf improvement.

--
Roman