netdev - Re: [PATCH v6] epoll: use refcount to reduce ep

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: <20230326040605.qmnu7zi2qx6glfy2@offworld>
Date:   Sat, 25 Mar 2023 21:06:05 -0700
From:   Davidlohr Bueso <dave@...olabs.net>
To:     Paolo Abeni <pabeni@...hat.com>
Cc:     netdev@...r.kernel.org, Soheil Hassas Yeganeh <soheil@...gle.com>,
        Al Viro <viro@...iv.linux.org.uk>,
        Carlos Maiolino <cmaiolino@...hat.com>,
        Eric Biggers <ebiggers@...nel.org>,
        Jacob Keller <jacob.e.keller@...el.com>,
        Andrew Morton <akpm@...ux-foundation.org>,
        Jens Axboe <axboe@...nel.dk>,
        Christian Brauner <brauner@...nel.org>,
        linux-fsdevel@...r.kernel.org, Eric Dumazet <edumazet@...gle.com>
Subject: Re: [PATCH v6] epoll: use refcount to reduce ep_mutex contention

On Wed, 22 Mar 2023, Paolo Abeni wrote:

>We are observing huge contention on the epmutex during an http
>connection/rate test:
>
> 83.17% 0.25%  nginx            [kernel.kallsyms]         [k] entry_SYSCALL_64_after_hwframe
>[...]
>           |--66.96%--__fput
>                      |--60.04%--eventpoll_release_file
>                                 |--58.41%--__mutex_lock.isra.6
>                                           |--56.56%--osq_lock
>
>The application is multi-threaded, creates a new epoll entry for
>each incoming connection, and does not delete it before the
>connection shutdown - that is, before the connection's fd close().
>
>Many different threads compete frequently for the epmutex lock,
>affecting the overall performance.
>
>To reduce the contention this patch introduces explicit reference counting
>for the eventpoll struct. Each registered event acquires a reference,
>and references are released at ep_remove() time.
>
>The eventpoll struct is released by whoever - among EP file close() and
>and the monitored file close() drops its last reference.
>
>Additionally, this introduces a new 'dying' flag to prevent races between
>the EP file close() and the monitored file close().
>ep_eventpoll_release() marks, under f_lock spinlock, each epitem as dying
>before removing it, while EP file close() does not touch dying epitems.
>
>The above is needed as both close operations could run concurrently and
>drop the EP reference acquired via the epitem entry. Without the above
>flag, the monitored file close() could reach the EP struct via the epitem
>list while the epitem is still listed and then try to put it after its
>disposal.
>
>An alternative could be avoiding touching the references acquired via
>the epitems at EP file close() time, but that could leave the EP struct
>alive for potentially unlimited time after EP file close(), with nasty
>side effects.
>
>With all the above in place, we can drop the epmutex usage at disposal time.
>
>Overall this produces a significant performance improvement in the
>mentioned connection/rate scenario: the mutex operations disappear from
>the topmost offenders in the perf report, and the measured connections/rate
>grows by ~60%.

Nice, and now just a single nested use-case for the global lock. It should
probably renamed to 'epnested_mutex'.

>To make the change more readable this additionally renames ep_free() to
>ep_clear_and_put(), and moves the actual memory cleanup in a separate
>ep_free() helper.
>

Reviewed-by: Davidlohr Bueso <dave@...olabs.net>