SQPOLL question

SQPOLL question

[Josef]

Hi,

I'm trying to implement SQPOLL in netty at the moment, basically the
fd are registered by io_uring_register(2), which returns 0, but the
write event seems to fail with bad file descriptor error(-9) when
SQPOLL flag is enabled


small example to reproduce it:
https://gist.github.com/1Jo1/171790d549134b5b81ee51b23fb15cd0

what exactly am I doing wrong here? :)

---
Josef

Re: SQPOLL question

[Jens Axboe]

On 9/6/20 9:44 AM, Josef wrote:
> Hi,
> 
> I'm trying to implement SQPOLL in netty at the moment, basically the
> fd are registered by io_uring_register(2), which returns 0, but the
> write event seems to fail with bad file descriptor error(-9) when
> SQPOLL flag is enabled
> 
> 
> small example to reproduce it:
> https://gist.github.com/1Jo1/171790d549134b5b81ee51b23fb15cd0
> 
> what exactly am I doing wrong here? :)

You're using the 'fd' as the file descriptor, for registered files
you want to use the index instead. Since it's the only fd you
registered, the index would be 0 and that's what you should use.

It's worth mentioning that for 5.10 and on, SQPOLL will no longer
require registered files.

-- 
Jens Axboe

Re: SQPOLL question

[Josef]

> You're using the 'fd' as the file descriptor, for registered files
> you want to use the index instead. Since it's the only fd you
> registered, the index would be 0 and that's what you should use.

oh..yeah it works, thanks :)

> It's worth mentioning that for 5.10 and on, SQPOLL will no longer
> require registered files.

that's awesome, it would be really handy as I just implemented a kind
of workaround in netty :)

Re: SQPOLL question

[Jens Axboe]

On 9/6/20 10:24 AM, Josef wrote:
>> You're using the 'fd' as the file descriptor, for registered files
>> you want to use the index instead. Since it's the only fd you
>> registered, the index would be 0 and that's what you should use.
> 
> oh..yeah it works, thanks :)

Great!

>> It's worth mentioning that for 5.10 and on, SQPOLL will no longer
>> require registered files.
> 
> that's awesome, it would be really handy as I just implemented a kind
> of workaround in netty :)

On top of that, capabilities will also be reduced from root to
CAP_SYS_NICE instead, and sharing across rings for the SQPOLL thread
will be supported. So it'll be a lot more useful/flexible in general.

-- 
Jens Axboe

Re: SQPOLL question

[Josef]

> On top of that, capabilities will also be reduced from root to
> CAP_SYS_NICE instead, and sharing across rings for the SQPOLL thread
> will be supported. So it'll be a lot more useful/flexible in general.

oha that's nice, I'm pretty excited :)

I'm just wondering if all op are supported when the SQPOLL flag is
set? the accept op seems to fail with -EINVAL, when I enable SQPOLL

to reproduce it:
https://gist.github.com/1Jo1/accb91b737abb55d07487799739ad70a
(just want to test a non blocking accept op in SQPOLL mode)

---
Josef




On Sun, 6 Sep 2020 at 18:25, Jens Axboe <axboe@kernel.dk> wrote:
>
> On 9/6/20 10:24 AM, Josef wrote:
> >> You're using the 'fd' as the file descriptor, for registered files
> >> you want to use the index instead. Since it's the only fd you
> >> registered, the index would be 0 and that's what you should use.
> >
> > oh..yeah it works, thanks :)
>
> Great!
>
> >> It's worth mentioning that for 5.10 and on, SQPOLL will no longer
> >> require registered files.
> >
> > that's awesome, it would be really handy as I just implemented a kind
> > of workaround in netty :)
>
> On top of that, capabilities will also be reduced from root to
> CAP_SYS_NICE instead, and sharing across rings for the SQPOLL thread
> will be supported. So it'll be a lot more useful/flexible in general.
>
> --
> Jens Axboe
>

Re: SQPOLL question

[Jens Axboe]

On 9/7/20 4:23 AM, Josef wrote:
>> On top of that, capabilities will also be reduced from root to
>> CAP_SYS_NICE instead, and sharing across rings for the SQPOLL thread
>> will be supported. So it'll be a lot more useful/flexible in general.
> 
> oha that's nice, I'm pretty excited :)
> 
> I'm just wondering if all op are supported when the SQPOLL flag is
> set? the accept op seems to fail with -EINVAL, when I enable SQPOLL
> 
> to reproduce it:
> https://gist.github.com/1Jo1/accb91b737abb55d07487799739ad70a
> (just want to test a non blocking accept op in SQPOLL mode)

Yes, that is known, you cannot open/close descriptors with the
SQPOLL that requires fixed files, as that requires modifying the
file descriptor. 5.10 should not have any limitations.

-- 
Jens Axboe

Re: SQPOLL question

[Josef]

> Yes, that is known, you cannot open/close descriptors with the
> SQPOLL that requires fixed files, as that requires modifying the
> file descriptor. 5.10 should not have any limitations.

ok I got it, let me know when the implementation/testing is finished
for SQPOLL then I could test my netty implementation

---
Josef

Re: SQPOLL question

[Jens Axboe]

On 9/7/20 8:58 AM, Josef wrote:
>> Yes, that is known, you cannot open/close descriptors with the
>> SQPOLL that requires fixed files, as that requires modifying the
>> file descriptor. 5.10 should not have any limitations.
> 
> ok I got it, let me know when the implementation/testing is finished
> for SQPOLL then I could test my netty implementation

If you're up for it, you could just clone my for-5.10/io_uring and base
your SQPOLL testing on that. Should be finished, modulo bugs...

-- 
Jens Axboe

Re: SQPOLL question

[Josef]

> If you're up for it, you could just clone my for-5.10/io_uring and base
> your SQPOLL testing on that. Should be finished, modulo bugs...

yeah did some benchmark tests and I'm quite impressed, however accept
op seems to fail with -EBADF when the flag IOSQE_ASYNC is set, is that
known?

---
Josef

Re: SQPOLL question

[Jens Axboe]

On 9/8/20 12:47 AM, Josef wrote:
>> If you're up for it, you could just clone my for-5.10/io_uring and base
>> your SQPOLL testing on that. Should be finished, modulo bugs...
> 
> yeah did some benchmark tests and I'm quite impressed, however accept
> op seems to fail with -EBADF when the flag IOSQE_ASYNC is set, is that
> known?

Nope, ran a quick test case here on the current tree, works for me.

Are you using for-5.10 and SQEPOLL + ASYNC accept? I'll give that a
test spin.

-- 
Jens Axboe

Re: SQPOLL question

[Jens Axboe]

On 9/8/20 8:36 AM, Jens Axboe wrote:
> On 9/8/20 12:47 AM, Josef wrote:
>>> If you're up for it, you could just clone my for-5.10/io_uring and base
>>> your SQPOLL testing on that. Should be finished, modulo bugs...
>>
>> yeah did some benchmark tests and I'm quite impressed, however accept
>> op seems to fail with -EBADF when the flag IOSQE_ASYNC is set, is that
>> known?
> 
> Nope, ran a quick test case here on the current tree, works for me.
> 
> Are you using for-5.10 and SQEPOLL + ASYNC accept? I'll give that a
> test spin.

This should do it for your testing, need to confirm this is absolutely
safe. But it'll make it work for the 5.10/io_uring setup of allowing
file open/closes.

diff --git a/fs/io_uring.c b/fs/io_uring.c
index 80913973337a..e21a7a9c6a59 100644
--- a/fs/io_uring.c
+++ b/fs/io_uring.c
@@ -6757,7 +6757,7 @@ static enum sq_ret __io_sq_thread(struct io_ring_ctx *ctx,
 
 	mutex_lock(&ctx->uring_lock);
 	if (likely(!percpu_ref_is_dying(&ctx->refs)))
-		ret = io_submit_sqes(ctx, to_submit, NULL, -1);
+		ret = io_submit_sqes(ctx, to_submit, ctx->ring_file, ctx->ring_fd);
 	mutex_unlock(&ctx->uring_lock);
 
 	if (!io_sqring_full(ctx) && wq_has_sleeper(&ctx->sqo_sq_wait))
@@ -8966,6 +8966,11 @@ static int io_uring_create(unsigned entries, struct io_uring_params *p,
 		goto err;
 	}
 
+	if (p->flags & IORING_SETUP_SQPOLL) {
+		ctx->ring_fd = fd;
+		ctx->ring_file = file;
+	}
+
 	ret = io_sq_offload_create(ctx, p);
 	if (ret)
 		goto err;

-- 
Jens Axboe

Re: SQPOLL question

[Josef]

> Are you using for-5.10 and SQEPOLL + ASYNC accept? I'll give that a
> test spin.

yes exactly

> This should do it for your testing, need to confirm this is absolutely
> safe. But it'll make it work for the 5.10/io_uring setup of allowing
> file open/closes.
>
> diff --git a/fs/io_uring.c b/fs/io_uring.c
> index 80913973337a..e21a7a9c6a59 100644
> --- a/fs/io_uring.c
> +++ b/fs/io_uring.c
> @@ -6757,7 +6757,7 @@ static enum sq_ret __io_sq_thread(struct io_ring_ctx *ctx,
>
>         mutex_lock(&ctx->uring_lock);
>         if (likely(!percpu_ref_is_dying(&ctx->refs)))
> -               ret = io_submit_sqes(ctx, to_submit, NULL, -1);
> +               ret = io_submit_sqes(ctx, to_submit, ctx->ring_file, ctx->ring_fd);
>         mutex_unlock(&ctx->uring_lock);
>
>         if (!io_sqring_full(ctx) && wq_has_sleeper(&ctx->sqo_sq_wait))
> @@ -8966,6 +8966,11 @@ static int io_uring_create(unsigned entries, struct io_uring_params *p,
>                 goto err;
>         }
>
> +       if (p->flags & IORING_SETUP_SQPOLL) {
> +               ctx->ring_fd = fd;
> +               ctx->ring_file = file;
> +       }
> +
>         ret = io_sq_offload_create(ctx, p);
>         if (ret)
>                 goto err;
>

sorry I couldn't apply this patch, my last commit is
https://git.kernel.org/pub/scm/linux/kernel/git/axboe/linux-block.git/commit/?h=for-5.10/io_uring&id=9c2446cffaf55da88e7a9a7c0a5aeb02a9eba2c0
what's your last commit?

it's a small patch, so I'll try it manually :)

---
Josef

Re: SQPOLL question

[Jens Axboe]

On 9/8/20 11:42 AM, Josef wrote:
>> Are you using for-5.10 and SQEPOLL + ASYNC accept? I'll give that a
>> test spin.
> 
> yes exactly
> 
>> This should do it for your testing, need to confirm this is absolutely
>> safe. But it'll make it work for the 5.10/io_uring setup of allowing
>> file open/closes.
>>
>> diff --git a/fs/io_uring.c b/fs/io_uring.c
>> index 80913973337a..e21a7a9c6a59 100644
>> --- a/fs/io_uring.c
>> +++ b/fs/io_uring.c
>> @@ -6757,7 +6757,7 @@ static enum sq_ret __io_sq_thread(struct io_ring_ctx *ctx,
>>
>>         mutex_lock(&ctx->uring_lock);
>>         if (likely(!percpu_ref_is_dying(&ctx->refs)))
>> -               ret = io_submit_sqes(ctx, to_submit, NULL, -1);
>> +               ret = io_submit_sqes(ctx, to_submit, ctx->ring_file, ctx->ring_fd);
>>         mutex_unlock(&ctx->uring_lock);
>>
>>         if (!io_sqring_full(ctx) && wq_has_sleeper(&ctx->sqo_sq_wait))
>> @@ -8966,6 +8966,11 @@ static int io_uring_create(unsigned entries, struct io_uring_params *p,
>>                 goto err;
>>         }
>>
>> +       if (p->flags & IORING_SETUP_SQPOLL) {
>> +               ctx->ring_fd = fd;
>> +               ctx->ring_file = file;
>> +       }
>> +
>>         ret = io_sq_offload_create(ctx, p);
>>         if (ret)
>>                 goto err;
>>
> 
> sorry I couldn't apply this patch, my last commit is
> https://git.kernel.org/pub/scm/linux/kernel/git/axboe/linux-block.git/commit/?h=for-5.10/io_uring&id=9c2446cffaf55da88e7a9a7c0a5aeb02a9eba2c0
> what's your last commit?
> 
> it's a small patch, so I'll try it manually :)

Oops sorry, pushed out the queue. Should apply cleanly on top of that.

-- 
Jens Axboe