* [PATCH 1/1] io_uring: clean up io_uring_register_get_file()
@ 2025-01-16 2:53 Pavel Begunkov
2025-01-16 13:09 ` lizetao
2025-01-16 14:13 ` Jens Axboe
0 siblings, 2 replies; 5+ messages in thread
From: Pavel Begunkov @ 2025-01-16 2:53 UTC (permalink / raw)
To: io-uring; +Cc: asml.silence
Make it always reference the returned file. It's safer, especially with
unregistrations happening under it. And it makes the api cleaner with no
conditional clean ups by the caller.
Signed-off-by: Pavel Begunkov <asml.silence@gmail.com>
---
io_uring/register.c | 6 ++++--
io_uring/rsrc.c | 4 ++--
2 files changed, 6 insertions(+), 4 deletions(-)
diff --git a/io_uring/register.c b/io_uring/register.c
index 5e48413706ac..a93c979c2f38 100644
--- a/io_uring/register.c
+++ b/io_uring/register.c
@@ -841,6 +841,8 @@ struct file *io_uring_register_get_file(unsigned int fd, bool registered)
return ERR_PTR(-EINVAL);
fd = array_index_nospec(fd, IO_RINGFD_REG_MAX);
file = tctx->registered_rings[fd];
+ if (file)
+ get_file(file);
} else {
file = fget(fd);
}
@@ -907,7 +909,7 @@ SYSCALL_DEFINE4(io_uring_register, unsigned int, fd, unsigned int, opcode,
trace_io_uring_register(ctx, opcode, ctx->file_table.data.nr,
ctx->buf_table.nr, ret);
mutex_unlock(&ctx->uring_lock);
- if (!use_registered_ring)
- fput(file);
+
+ fput(file);
return ret;
}
diff --git a/io_uring/rsrc.c b/io_uring/rsrc.c
index 964a47c8d85e..792c22b6f2d4 100644
--- a/io_uring/rsrc.c
+++ b/io_uring/rsrc.c
@@ -1073,7 +1073,7 @@ int io_register_clone_buffers(struct io_ring_ctx *ctx, void __user *arg)
if (IS_ERR(file))
return PTR_ERR(file);
ret = io_clone_buffers(ctx, file->private_data, &buf);
- if (!registered_src)
- fput(file);
+
+ fput(file);
return ret;
}
--
2.47.1
^ permalink raw reply related [flat|nested] 5+ messages in thread* RE: [PATCH 1/1] io_uring: clean up io_uring_register_get_file()
2025-01-16 2:53 [PATCH 1/1] io_uring: clean up io_uring_register_get_file() Pavel Begunkov
@ 2025-01-16 13:09 ` lizetao
2025-01-16 14:10 ` Jens Axboe
2025-01-16 14:13 ` Jens Axboe
1 sibling, 1 reply; 5+ messages in thread
From: lizetao @ 2025-01-16 13:09 UTC (permalink / raw)
To: Pavel Begunkov; +Cc: io-uring@vger.kernel.org, Jens Axboe
Hi,
> -----Original Message-----
> From: Pavel Begunkov <asml.silence@gmail.com>
> Sent: Thursday, January 16, 2025 10:53 AM
> To: io-uring@vger.kernel.org
> Cc: asml.silence@gmail.com
> Subject: [PATCH 1/1] io_uring: clean up io_uring_register_get_file()
>
> Make it always reference the returned file. It's safer, especially with
> unregistrations happening under it. And it makes the api cleaner with no
> conditional clean ups by the caller.
>
> Signed-off-by: Pavel Begunkov <asml.silence@gmail.com>
> ---
> io_uring/register.c | 6 ++++--
> io_uring/rsrc.c | 4 ++--
> 2 files changed, 6 insertions(+), 4 deletions(-)
>
> diff --git a/io_uring/register.c b/io_uring/register.c index
> 5e48413706ac..a93c979c2f38 100644
> --- a/io_uring/register.c
> +++ b/io_uring/register.c
> @@ -841,6 +841,8 @@ struct file *io_uring_register_get_file(unsigned int fd,
> bool registered)
> return ERR_PTR(-EINVAL);
> fd = array_index_nospec(fd, IO_RINGFD_REG_MAX);
> file = tctx->registered_rings[fd];
> + if (file)
> + get_file(file);
Should performance be a priority here?
> } else {
> file = fget(fd);
> }
> @@ -907,7 +909,7 @@ SYSCALL_DEFINE4(io_uring_register, unsigned int, fd,
> unsigned int, opcode,
> trace_io_uring_register(ctx, opcode, ctx->file_table.data.nr,
> ctx->buf_table.nr, ret);
> mutex_unlock(&ctx->uring_lock);
> - if (!use_registered_ring)
> - fput(file);
> +
> + fput(file);
> return ret;
> }
> diff --git a/io_uring/rsrc.c b/io_uring/rsrc.c index
> 964a47c8d85e..792c22b6f2d4 100644
> --- a/io_uring/rsrc.c
> +++ b/io_uring/rsrc.c
> @@ -1073,7 +1073,7 @@ int io_register_clone_buffers(struct io_ring_ctx *ctx,
> void __user *arg)
> if (IS_ERR(file))
> return PTR_ERR(file);
> ret = io_clone_buffers(ctx, file->private_data, &buf);
> - if (!registered_src)
> - fput(file);
> +
> + fput(file);
> return ret;
> }
> --
> 2.47.1
>
---
Li Zetao
^ permalink raw reply [flat|nested] 5+ messages in thread* Re: [PATCH 1/1] io_uring: clean up io_uring_register_get_file()
2025-01-16 13:09 ` lizetao
@ 2025-01-16 14:10 ` Jens Axboe
2025-01-17 1:40 ` lizetao
0 siblings, 1 reply; 5+ messages in thread
From: Jens Axboe @ 2025-01-16 14:10 UTC (permalink / raw)
To: lizetao, Pavel Begunkov; +Cc: io-uring@vger.kernel.org
On 1/16/25 6:09 AM, lizetao wrote:
> Hi,
>
>> -----Original Message-----
>> From: Pavel Begunkov <asml.silence@gmail.com>
>> Sent: Thursday, January 16, 2025 10:53 AM
>> To: io-uring@vger.kernel.org
>> Cc: asml.silence@gmail.com
>> Subject: [PATCH 1/1] io_uring: clean up io_uring_register_get_file()
>>
>> Make it always reference the returned file. It's safer, especially with
>> unregistrations happening under it. And it makes the api cleaner with no
>> conditional clean ups by the caller.
>>
>> Signed-off-by: Pavel Begunkov <asml.silence@gmail.com>
>> ---
>> io_uring/register.c | 6 ++++--
>> io_uring/rsrc.c | 4 ++--
>> 2 files changed, 6 insertions(+), 4 deletions(-)
>>
>> diff --git a/io_uring/register.c b/io_uring/register.c index
>> 5e48413706ac..a93c979c2f38 100644
>> --- a/io_uring/register.c
>> +++ b/io_uring/register.c
>> @@ -841,6 +841,8 @@ struct file *io_uring_register_get_file(unsigned int fd,
>> bool registered)
>> return ERR_PTR(-EINVAL);
>> fd = array_index_nospec(fd, IO_RINGFD_REG_MAX);
>> file = tctx->registered_rings[fd];
>> + if (file)
>> + get_file(file);
>
> Should performance be a priority here?
Performance only really matters for high frequency invocations, of which
the register part is not. So no, should not matter at all.
--
Jens Axboe
^ permalink raw reply [flat|nested] 5+ messages in thread
* RE: [PATCH 1/1] io_uring: clean up io_uring_register_get_file()
2025-01-16 14:10 ` Jens Axboe
@ 2025-01-17 1:40 ` lizetao
0 siblings, 0 replies; 5+ messages in thread
From: lizetao @ 2025-01-17 1:40 UTC (permalink / raw)
To: Jens Axboe, Pavel Begunkov; +Cc: io-uring@vger.kernel.org
Hi,
> -----Original Message-----
> From: Jens Axboe <axboe@kernel.dk>
> Sent: Thursday, January 16, 2025 10:10 PM
> To: lizetao <lizetao1@huawei.com>; Pavel Begunkov <asml.silence@gmail.com>
> Cc: io-uring@vger.kernel.org
> Subject: Re: [PATCH 1/1] io_uring: clean up io_uring_register_get_file()
>
> On 1/16/25 6:09 AM, lizetao wrote:
> > Hi,
> >
> >> -----Original Message-----
> >> From: Pavel Begunkov <asml.silence@gmail.com>
> >> Sent: Thursday, January 16, 2025 10:53 AM
> >> To: io-uring@vger.kernel.org
> >> Cc: asml.silence@gmail.com
> >> Subject: [PATCH 1/1] io_uring: clean up io_uring_register_get_file()
> >>
> >> Make it always reference the returned file. It's safer, especially
> >> with unregistrations happening under it. And it makes the api cleaner
> >> with no conditional clean ups by the caller.
> >>
> >> Signed-off-by: Pavel Begunkov <asml.silence@gmail.com>
> >> ---
> >> io_uring/register.c | 6 ++++--
> >> io_uring/rsrc.c | 4 ++--
> >> 2 files changed, 6 insertions(+), 4 deletions(-)
> >>
> >> diff --git a/io_uring/register.c b/io_uring/register.c index
> >> 5e48413706ac..a93c979c2f38 100644
> >> --- a/io_uring/register.c
> >> +++ b/io_uring/register.c
> >> @@ -841,6 +841,8 @@ struct file *io_uring_register_get_file(unsigned
> >> int fd, bool registered)
> >> return ERR_PTR(-EINVAL);
> >> fd = array_index_nospec(fd, IO_RINGFD_REG_MAX);
> >> file = tctx->registered_rings[fd];
> >> + if (file)
> >> + get_file(file);
> >
> > Should performance be a priority here?
>
> Performance only really matters for high frequency invocations, of which the
> register part is not. So no, should not matter at all.
Ok. I got it.
Tested-by: Li Zetao <lizetao1@huawei.com>
Reviewed-by: Li Zetao <lizetao1@huawei.com>
>
> --
> Jens Axboe
---
Li Zetao
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH 1/1] io_uring: clean up io_uring_register_get_file()
2025-01-16 2:53 [PATCH 1/1] io_uring: clean up io_uring_register_get_file() Pavel Begunkov
2025-01-16 13:09 ` lizetao
@ 2025-01-16 14:13 ` Jens Axboe
1 sibling, 0 replies; 5+ messages in thread
From: Jens Axboe @ 2025-01-16 14:13 UTC (permalink / raw)
To: io-uring, Pavel Begunkov
On Thu, 16 Jan 2025 02:53:26 +0000, Pavel Begunkov wrote:
> Make it always reference the returned file. It's safer, especially with
> unregistrations happening under it. And it makes the api cleaner with no
> conditional clean ups by the caller.
>
>
Applied, thanks!
[1/1] io_uring: clean up io_uring_register_get_file()
commit: 4415be009035b67b9101cf96b02cfee95621b3a6
Best regards,
--
Jens Axboe
^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2025-01-17 1:40 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2025-01-16 2:53 [PATCH 1/1] io_uring: clean up io_uring_register_get_file() Pavel Begunkov
2025-01-16 13:09 ` lizetao
2025-01-16 14:10 ` Jens Axboe
2025-01-17 1:40 ` lizetao
2025-01-16 14:13 ` Jens Axboe
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox