| Message ID | 20230501185240.352642-1-info@bnoordhuis.nl (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | io_uring: undeprecate epoll_ctl support | expand |
On 5/1/23 19:52, Ben Noordhuis wrote: > Libuv recently started using it so there is at least one consumer now. It was rather deprecated because io_uring controlling epoll is a bad idea and should never be used. One reason is that it means libuv still uses epoll but not io_uring, and so the use of io_uring wouldn't seem to make much sense. You're welcome to prove me wrong on that, why libuv decided to use a deprecated API in the first place? Sorry, but the warning is going to stay and libuv should revert the use of epol_ctl requests. > Link: https://github.com/libuv/libuv/pull/3979 > --- > io_uring/epoll.c | 4 ---- > 1 file changed, 4 deletions(-) > > diff --git a/io_uring/epoll.c b/io_uring/epoll.c > index 9aa74d2c80bc..89bff2068a19 100644 > --- a/io_uring/epoll.c > +++ b/io_uring/epoll.c > @@ -25,10 +25,6 @@ int io_epoll_ctl_prep(struct io_kiocb *req, const struct io_uring_sqe *sqe) > { > struct io_epoll *epoll = io_kiocb_to_cmd(req, struct io_epoll); > > - pr_warn_once("%s: epoll_ctl support in io_uring is deprecated and will " > - "be removed in a future Linux kernel version.\n", > - current->comm); > - > if (sqe->buf_index || sqe->splice_fd_in) > return -EINVAL; >
On Tue, May 2, 2023 at 2:51 PM Pavel Begunkov <asml.silence@gmail.com> wrote: > > On 5/1/23 19:52, Ben Noordhuis wrote: > > Libuv recently started using it so there is at least one consumer now. > > It was rather deprecated because io_uring controlling epoll is a bad > idea and should never be used. One reason is that it means libuv still > uses epoll but not io_uring, and so the use of io_uring wouldn't seem > to make much sense. You're welcome to prove me wrong on that, why libuv > decided to use a deprecated API in the first place? > Sorry, but the warning is going to stay and libuv should revert the use > of epol_ctl requests. Why use a deprecated API? Because it was only recently deprecated. Distro kernels don't warn about it yet. I only found out because of kernel source code spelunking. Why combine io_uring and epoll? Libuv uses level-triggered I/O for reasons (I can go into detail but they're not material) so it's very profitable to batch epoll_ctl syscalls; it's the epoll_ctlv() syscall people have been asking for since practically forever. Why not switch to io_uring wholesale? Libuv can't drop support for epoll because of old kernels, and io_uring isn't always clearly faster than epoll in the first place. As to the warning: according to the commit that introduced it, it was added because no one was using IORING_OP_EPOLL_CTL. Well, now someone is using it. Saying it's a bad API feels like post-hoc rationalization. I kindly ask you merge this patch. I'd be happy to keep an eye on io_uring/epoll.c if you're worried about maintenance burden.
On 5/3/23 2:58?AM, Ben Noordhuis wrote: > On Tue, May 2, 2023 at 2:51?PM Pavel Begunkov <asml.silence@gmail.com> wrote: >> >> On 5/1/23 19:52, Ben Noordhuis wrote: >>> Libuv recently started using it so there is at least one consumer now. >> >> It was rather deprecated because io_uring controlling epoll is a bad >> idea and should never be used. One reason is that it means libuv still >> uses epoll but not io_uring, and so the use of io_uring wouldn't seem >> to make much sense. You're welcome to prove me wrong on that, why libuv >> decided to use a deprecated API in the first place? >> Sorry, but the warning is going to stay and libuv should revert the use >> of epol_ctl requests. > > Why use a deprecated API? Because it was only recently deprecated. > Distro kernels don't warn about it yet. I only found out because of > kernel source code spelunking. > > Why combine io_uring and epoll? Libuv uses level-triggered I/O for > reasons (I can go into detail but they're not material) so it's very > profitable to batch epoll_ctl syscalls; it's the epoll_ctlv() syscall > people have been asking for since practically forever. > > Why not switch to io_uring wholesale? Libuv can't drop support for > epoll because of old kernels, and io_uring isn't always clearly faster > than epoll in the first place. > > As to the warning: according to the commit that introduced it, it was > added because no one was using IORING_OP_EPOLL_CTL. Well, now someone > is using it. Saying it's a bad API feels like post-hoc > rationalization. I kindly ask you merge this patch. I'd be happy to > keep an eye on io_uring/epoll.c if you're worried about maintenance > burden. This is obviously mostly our fault, as the deprecation patch should've obviously been backported to stable. Just adding it to the current kernel defeated the purpose, as it added a long period where older kernels quite happily accepted epoll use cases. So I do agree, the only sane course of action here is to un-deprecate it.
On 5/3/23 09:58, Ben Noordhuis wrote: > On Tue, May 2, 2023 at 2:51 PM Pavel Begunkov <asml.silence@gmail.com> wrote: >> >> On 5/1/23 19:52, Ben Noordhuis wrote: >>> Libuv recently started using it so there is at least one consumer now. >> >> It was rather deprecated because io_uring controlling epoll is a bad >> idea and should never be used. One reason is that it means libuv still >> uses epoll but not io_uring, and so the use of io_uring wouldn't seem >> to make much sense. You're welcome to prove me wrong on that, why libuv >> decided to use a deprecated API in the first place? >> Sorry, but the warning is going to stay and libuv should revert the use >> of epol_ctl requests. > > Why use a deprecated API? Because it was only recently deprecated. > Distro kernels don't warn about it yet. I only found out because of > kernel source code spelunking. I see. Looks it was introduced ~9 months ago, it's not as new, but as Jens mentioned, it's really a shame it hasn't been backported. > Why combine io_uring and epoll? Libuv uses level-triggered I/O for > reasons (I can go into detail but they're not material) so it's very > profitable to batch epoll_ctl syscalls; it's the epoll_ctlv() syscall > people have been asking for since practically forever. > > Why not switch to io_uring wholesale? Libuv can't drop support for > epoll because of old kernels, and io_uring isn't always clearly faster > than epoll in the first place. > > As to the warning: according to the commit that introduced it, it was > added because no one was using IORING_OP_EPOLL_CTL. Well, now someone > is using it. Saying it's a bad API feels like post-hoc Well, the commit message is wrong, or rather incomplete. There was a discussion about that, but not having users was obviously a requirement for deprecation at the time. > rationalization. I kindly ask you merge this patch. I'd be happy to > keep an eye on io_uring/epoll.c if you're worried about maintenance > burden.
On 5/3/23 13:49, Jens Axboe wrote: > On 5/3/23 2:58?AM, Ben Noordhuis wrote: >> On Tue, May 2, 2023 at 2:51?PM Pavel Begunkov <asml.silence@gmail.com> wrote: >>> >>> On 5/1/23 19:52, Ben Noordhuis wrote: >>>> Libuv recently started using it so there is at least one consumer now. >>> >>> It was rather deprecated because io_uring controlling epoll is a bad >>> idea and should never be used. One reason is that it means libuv still >>> uses epoll but not io_uring, and so the use of io_uring wouldn't seem >>> to make much sense. You're welcome to prove me wrong on that, why libuv >>> decided to use a deprecated API in the first place? >>> Sorry, but the warning is going to stay and libuv should revert the use >>> of epol_ctl requests. >> >> Why use a deprecated API? Because it was only recently deprecated. >> Distro kernels don't warn about it yet. I only found out because of >> kernel source code spelunking. >> >> Why combine io_uring and epoll? Libuv uses level-triggered I/O for >> reasons (I can go into detail but they're not material) so it's very >> profitable to batch epoll_ctl syscalls; it's the epoll_ctlv() syscall >> people have been asking for since practically forever. >> >> Why not switch to io_uring wholesale? Libuv can't drop support for >> epoll because of old kernels, and io_uring isn't always clearly faster >> than epoll in the first place. >> >> As to the warning: according to the commit that introduced it, it was >> added because no one was using IORING_OP_EPOLL_CTL. Well, now someone >> is using it. Saying it's a bad API feels like post-hoc >> rationalization. I kindly ask you merge this patch. I'd be happy to >> keep an eye on io_uring/epoll.c if you're worried about maintenance >> burden. > > This is obviously mostly our fault, as the deprecation patch should've > obviously been backported to stable. Just adding it to the current > kernel defeated the purpose, as it added a long period where older > kernels quite happily accepted epoll use cases. > > So I do agree, the only sane course of action here is to un-deprecate > it. nack, keeping piling rubbish is not a great course of action at all. Has libuv already released it? Because it seems the patches were just merged.
On 5/3/23 7:21?AM, Pavel Begunkov wrote: > On 5/3/23 13:49, Jens Axboe wrote: >> On 5/3/23 2:58?AM, Ben Noordhuis wrote: >>> On Tue, May 2, 2023 at 2:51?PM Pavel Begunkov <asml.silence@gmail.com> wrote: >>>> >>>> On 5/1/23 19:52, Ben Noordhuis wrote: >>>>> Libuv recently started using it so there is at least one consumer now. >>>> >>>> It was rather deprecated because io_uring controlling epoll is a bad >>>> idea and should never be used. One reason is that it means libuv still >>>> uses epoll but not io_uring, and so the use of io_uring wouldn't seem >>>> to make much sense. You're welcome to prove me wrong on that, why libuv >>>> decided to use a deprecated API in the first place? >>>> Sorry, but the warning is going to stay and libuv should revert the use >>>> of epol_ctl requests. >>> >>> Why use a deprecated API? Because it was only recently deprecated. >>> Distro kernels don't warn about it yet. I only found out because of >>> kernel source code spelunking. >>> >>> Why combine io_uring and epoll? Libuv uses level-triggered I/O for >>> reasons (I can go into detail but they're not material) so it's very >>> profitable to batch epoll_ctl syscalls; it's the epoll_ctlv() syscall >>> people have been asking for since practically forever. >>> >>> Why not switch to io_uring wholesale? Libuv can't drop support for >>> epoll because of old kernels, and io_uring isn't always clearly faster >>> than epoll in the first place. >>> >>> As to the warning: according to the commit that introduced it, it was >>> added because no one was using IORING_OP_EPOLL_CTL. Well, now someone >>> is using it. Saying it's a bad API feels like post-hoc >>> rationalization. I kindly ask you merge this patch. I'd be happy to >>> keep an eye on io_uring/epoll.c if you're worried about maintenance >>> burden. >> >> This is obviously mostly our fault, as the deprecation patch should've >> obviously been backported to stable. Just adding it to the current >> kernel defeated the purpose, as it added a long period where older >> kernels quite happily accepted epoll use cases. >> >> So I do agree, the only sane course of action here is to un-deprecate >> it. > > nack, keeping piling rubbish is not a great course of action at all. > > Has libuv already released it? Because it seems the patches were > just merged. This is not a NAK situation. Fact is that the code is out there, and libuv isn't the first to discover this by accident. We messed up not getting this to stable, but I think it's a reasonable assumption that there are likely others there as most folks run distro and/or stable kernels and are not on the bleeding edge. The deprecation patch was in 6.0, so anyone running kernels before that could be using the epoll support and have no idea that it would be going away. Outside of that, it's also quite easy to miss a single dmesg blurp on this unless you're actively looking for it or just happen to come across it. Unless there are reasons beyond "I'd love to remove this code", then it will be reinstated. We don't get to make up special rules for io_uring code that are counter to what the kernel generally guarantees, most notable that you cannot remove an API that is out there and in use. This isn't really about libuv in particular, as the io_uring support there is rather new and they could change course. It's more about other projects out there that already have it in production.
On 5/1/23 12:52 PM, Ben Noordhuis wrote: > Libuv recently started using it so there is at least one consumer now. > > Link: https://github.com/libuv/libuv/pull/3979 If we're going to apply this, we also need to have your Signed-off-by line added. And then I think it'd be a good idea to also add: Cc: stable@vger.kernel.org Fixes: 61a2732af4b0 ("io_uring: deprecate epoll_ctl support") so it would wind up in the stable releases that have the original deprecation patch.
diff --git a/io_uring/epoll.c b/io_uring/epoll.c index 9aa74d2c80bc..89bff2068a19 100644 --- a/io_uring/epoll.c +++ b/io_uring/epoll.c @@ -25,10 +25,6 @@ int io_epoll_ctl_prep(struct io_kiocb *req, const struct io_uring_sqe *sqe) { struct io_epoll *epoll = io_kiocb_to_cmd(req, struct io_epoll); - pr_warn_once("%s: epoll_ctl support in io_uring is deprecated and will " - "be removed in a future Linux kernel version.\n", - current->comm); - if (sqe->buf_index || sqe->splice_fd_in) return -EINVAL;