public inbox for [email protected]
 help / color / mirror / Atom feed
* [PATCH] io_uring: undeprecate epoll_ctl support
@ 2023-05-01 18:52 Ben Noordhuis
  2023-05-02 12:47 ` Pavel Begunkov
  2023-05-03 14:55 ` Jens Axboe
  0 siblings, 2 replies; 12+ messages in thread
From: Ben Noordhuis @ 2023-05-01 18:52 UTC (permalink / raw)
  To: io-uring; +Cc: Ben Noordhuis

Libuv recently started using it so there is at least one consumer now.

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;
 
-- 
2.37.2


^ permalink raw reply related	[flat|nested] 12+ messages in thread

* Re: [PATCH] io_uring: undeprecate epoll_ctl support
  2023-05-01 18:52 [PATCH] io_uring: undeprecate epoll_ctl support Ben Noordhuis
@ 2023-05-02 12:47 ` Pavel Begunkov
  2023-05-03  8:58   ` Ben Noordhuis
  2023-05-03 14:55 ` Jens Axboe
  1 sibling, 1 reply; 12+ messages in thread
From: Pavel Begunkov @ 2023-05-02 12:47 UTC (permalink / raw)
  To: Ben Noordhuis, io-uring

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;
>   

-- 
Pavel Begunkov

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH] io_uring: undeprecate epoll_ctl support
  2023-05-02 12:47 ` Pavel Begunkov
@ 2023-05-03  8:58   ` Ben Noordhuis
  2023-05-03 12:49     ` Jens Axboe
  2023-05-03 13:13     ` Pavel Begunkov
  0 siblings, 2 replies; 12+ messages in thread
From: Ben Noordhuis @ 2023-05-03  8:58 UTC (permalink / raw)
  To: Pavel Begunkov; +Cc: io-uring

On Tue, May 2, 2023 at 2:51 PM Pavel Begunkov <[email protected]> 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.

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH] io_uring: undeprecate epoll_ctl support
  2023-05-03  8:58   ` Ben Noordhuis
@ 2023-05-03 12:49     ` Jens Axboe
  2023-05-03 13:21       ` Pavel Begunkov
  2023-05-03 13:13     ` Pavel Begunkov
  1 sibling, 1 reply; 12+ messages in thread
From: Jens Axboe @ 2023-05-03 12:49 UTC (permalink / raw)
  To: Ben Noordhuis, Pavel Begunkov; +Cc: io-uring

On 5/3/23 2:58?AM, Ben Noordhuis wrote:
> On Tue, May 2, 2023 at 2:51?PM Pavel Begunkov <[email protected]> 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.

-- 
Jens Axboe


^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH] io_uring: undeprecate epoll_ctl support
  2023-05-03  8:58   ` Ben Noordhuis
  2023-05-03 12:49     ` Jens Axboe
@ 2023-05-03 13:13     ` Pavel Begunkov
  1 sibling, 0 replies; 12+ messages in thread
From: Pavel Begunkov @ 2023-05-03 13:13 UTC (permalink / raw)
  To: Ben Noordhuis; +Cc: io-uring

On 5/3/23 09:58, Ben Noordhuis wrote:
> On Tue, May 2, 2023 at 2:51 PM Pavel Begunkov <[email protected]> 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.

-- 
Pavel Begunkov

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH] io_uring: undeprecate epoll_ctl support
  2023-05-03 12:49     ` Jens Axboe
@ 2023-05-03 13:21       ` Pavel Begunkov
  2023-05-03 13:42         ` Jens Axboe
  0 siblings, 1 reply; 12+ messages in thread
From: Pavel Begunkov @ 2023-05-03 13:21 UTC (permalink / raw)
  To: Jens Axboe, Ben Noordhuis; +Cc: io-uring

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 <[email protected]> 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.

-- 
Pavel Begunkov

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH] io_uring: undeprecate epoll_ctl support
  2023-05-03 13:21       ` Pavel Begunkov
@ 2023-05-03 13:42         ` Jens Axboe
  0 siblings, 0 replies; 12+ messages in thread
From: Jens Axboe @ 2023-05-03 13:42 UTC (permalink / raw)
  To: Pavel Begunkov, Ben Noordhuis; +Cc: io-uring

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 <[email protected]> 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.

-- 
Jens Axboe


^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH] io_uring: undeprecate epoll_ctl support
  2023-05-01 18:52 [PATCH] io_uring: undeprecate epoll_ctl support Ben Noordhuis
  2023-05-02 12:47 ` Pavel Begunkov
@ 2023-05-03 14:55 ` Jens Axboe
  2023-05-06  9:55   ` Ben Noordhuis
  1 sibling, 1 reply; 12+ messages in thread
From: Jens Axboe @ 2023-05-03 14:55 UTC (permalink / raw)
  To: Ben Noordhuis, io-uring

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: [email protected]
Fixes: 61a2732af4b0 ("io_uring: deprecate epoll_ctl support")

so it would wind up in the stable releases that have the original
deprecation patch.

-- 
Jens Axboe



^ permalink raw reply	[flat|nested] 12+ messages in thread

* [PATCH] io_uring: undeprecate epoll_ctl support
  2023-05-03 14:55 ` Jens Axboe
@ 2023-05-06  9:55   ` Ben Noordhuis
  2023-05-27  1:48     ` Sam James
  2023-05-27  2:22     ` Jens Axboe
  0 siblings, 2 replies; 12+ messages in thread
From: Ben Noordhuis @ 2023-05-06  9:55 UTC (permalink / raw)
  To: io-uring; +Cc: Ben Noordhuis, stable

Libuv recently started using it so there is at least one consumer now.

Cc: [email protected]
Fixes: 61a2732af4b0 ("io_uring: deprecate epoll_ctl support")
Link: https://github.com/libuv/libuv/pull/3979
Signed-off-by: Ben Noordhuis <[email protected]>
---
 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;
 
-- 
2.39.2


^ permalink raw reply related	[flat|nested] 12+ messages in thread

* Re: [PATCH] io_uring: undeprecate epoll_ctl support
  2023-05-06  9:55   ` Ben Noordhuis
@ 2023-05-27  1:48     ` Sam James
  2023-05-27  2:22       ` Jens Axboe
  2023-05-27  2:22     ` Jens Axboe
  1 sibling, 1 reply; 12+ messages in thread
From: Sam James @ 2023-05-27  1:48 UTC (permalink / raw)
  To: info, Jens Axboe; +Cc: io-uring, stable, kernel

[-- Attachment #1: Type: text/plain, Size: 342 bytes --]

Hi,

New libuv is indeed in the wild now and CMake uses it, so I end up
getting
a tonne of:
```
[ 1763.697364] isc-net-0000: epoll_ctl support in io_uring is deprecated
and will be removed in a future Linux kernel version.
```

Could you confirm if this patch looks likely to land so we know if it's
OK
to backport it downstream?

Thank you!

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 377 bytes --]

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH] io_uring: undeprecate epoll_ctl support
  2023-05-27  1:48     ` Sam James
@ 2023-05-27  2:22       ` Jens Axboe
  0 siblings, 0 replies; 12+ messages in thread
From: Jens Axboe @ 2023-05-27  2:22 UTC (permalink / raw)
  To: Sam James, info; +Cc: io-uring, stable, kernel

On 5/26/23 7:48 PM, Sam James wrote:
> Hi,
> 
> New libuv is indeed in the wild now and CMake uses it, so I end up
> getting
> a tonne of:
> ```
> [ 1763.697364] isc-net-0000: epoll_ctl support in io_uring is deprecated
> and will be removed in a future Linux kernel version.
> ```
> 
> Could you confirm if this patch looks likely to land so we know if it's
> OK
> to backport it downstream?

Yeah, I think this is the best plan. I was out of town for a bit
around when it got reposted and hence missed it, I'll queue it
up for this release.

-- 
Jens Axboe



^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH] io_uring: undeprecate epoll_ctl support
  2023-05-06  9:55   ` Ben Noordhuis
  2023-05-27  1:48     ` Sam James
@ 2023-05-27  2:22     ` Jens Axboe
  1 sibling, 0 replies; 12+ messages in thread
From: Jens Axboe @ 2023-05-27  2:22 UTC (permalink / raw)
  To: io-uring, Ben Noordhuis; +Cc: stable


On Sat, 06 May 2023 11:55:02 +0200, Ben Noordhuis wrote:
> Libuv recently started using it so there is at least one consumer now.
> 
> 

Applied, thanks!

[1/1] io_uring: undeprecate epoll_ctl support
      commit: 4ea0bf4b98d66a7a790abb285539f395596bae92

Best regards,
-- 
Jens Axboe




^ permalink raw reply	[flat|nested] 12+ messages in thread

end of thread, other threads:[~2023-05-27  2:23 UTC | newest]

Thread overview: 12+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2023-05-01 18:52 [PATCH] io_uring: undeprecate epoll_ctl support Ben Noordhuis
2023-05-02 12:47 ` Pavel Begunkov
2023-05-03  8:58   ` Ben Noordhuis
2023-05-03 12:49     ` Jens Axboe
2023-05-03 13:21       ` Pavel Begunkov
2023-05-03 13:42         ` Jens Axboe
2023-05-03 13:13     ` Pavel Begunkov
2023-05-03 14:55 ` Jens Axboe
2023-05-06  9:55   ` Ben Noordhuis
2023-05-27  1:48     ` Sam James
2023-05-27  2:22       ` Jens Axboe
2023-05-27  2:22     ` Jens Axboe

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox