Date: Tue, 7 May 2024 21:02:41 +0000
From: David Laight <David.Laight@...LAB.COM>
To: 'Christian Brauner' <brauner@...nel.org>, Linus Torvalds
	<torvalds@...ux-foundation.org>
CC: Al Viro <viro@...iv.linux.org.uk>, "keescook@...omium.org"
	<keescook@...omium.org>, "axboe@...nel.dk" <axboe@...nel.dk>,
	"christian.koenig@....com" <christian.koenig@....com>,
	"dri-devel@...ts.freedesktop.org" <dri-devel@...ts.freedesktop.org>,
	"io-uring@...r.kernel.org" <io-uring@...r.kernel.org>, "jack@...e.cz"
	<jack@...e.cz>, "laura@...bott.name" <laura@...bott.name>,
	"linaro-mm-sig@...ts.linaro.org" <linaro-mm-sig@...ts.linaro.org>,
	"linux-fsdevel@...r.kernel.org" <linux-fsdevel@...r.kernel.org>,
	"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
	"linux-media@...r.kernel.org" <linux-media@...r.kernel.org>,
	"minhquangbui99@...il.com" <minhquangbui99@...il.com>,
	"sumit.semwal@...aro.org" <sumit.semwal@...aro.org>,
	"syzbot+045b454ab35fd82a35fb@...kaller.appspotmail.com"
	<syzbot+045b454ab35fd82a35fb@...kaller.appspotmail.com>,
	"syzkaller-bugs@...glegroups.com" <syzkaller-bugs@...glegroups.com>
Subject: RE: [PATCH] epoll: try to be a _bit_ better about file lifetimes

From: Christian Brauner
> Sent: 06 May 2024 09:45
> 
> > The fact is, it's not dma-buf that is violating any rules. It's epoll.
> 
> I agree that epoll() not taking a reference on the file is at least
> unexpected and contradicts the usual code patterns for the sake of
> performance and that it very likely is the case that most callers of
> f_op->poll() don't know this.
> 
> Note, I cleary wrote upthread that I'm ok to do it like you suggested
> but raised two concerns a) there's currently only one instance of
> prolonged @file lifetime in f_op->poll() afaict and b) that there's
> possibly going to be some performance impact on epoll().
> 
> So it's at least worth discussing what's more important because epoll()
> is very widely used and it's not that we haven't favored performance
> before.
> 
> But you've already said that you aren't concerned with performance on
> epoll() upthread. So afaict then there's really not a lot more to
> discuss other than take the patch and see whether we get any complaints.

Surely there isn't a problem with epoll holding a reference to the file
structure - it isn't really any different to a dup().

'All' that needs to happen is that the 'magic' that makes epoll() remove
files on the last fput happen when the close is done.
I'm sure there are horrid locking issues it that code (separate from
it calling ->poll() after ->release()) eg if you call close() concurrently
with EPOLL_CTL_ADD.

I'm not at all sure it would have mattered if epoll kept the file open.
But it can't do that because it is documented not to.
As well as poll/select holding a reference to all their fd for the duration
of the system call, a successful mmap() holds a reference until the pages
are all unmapped - usually by process exit.

We (dayjob) have code that uses epoll() to monitor large numbers of UDP
sockets. I was doing some tests (trying to) receive RTP (audio) data
concurrently on 10000 sockets with typically one packet every 20ms.
There are 10000 associated RCTP sockets that are usually idle.
A more normal limit would be 1000 RTP sockets.
All the data needs to go into a single (multithreaded) process.
Just getting all the packets queued on the sockets was non-trivial.
epoll is about the only way to actually read the data.
(That needed multiple epoll fd so each thread could process all
the events from one epoll fd then look for another unprocessed fd.)

	David

-
Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
Registration No: 1397386 (Wales)

Powered by blists - more mailing lists