[PATCH 1/1] io_uring: don't scm-account for non af_unix sockets

[PATCH 1/1] io_uring: don't scm-account for non af_unix sockets

[Pavel Begunkov]

io_uring deals with file reference loops by registering all fixed files
in the SCM/GC infrastrucure. However, only a small subset of all file
types can keep long-term references to other files and those that don't
are not interesting for the garbage collector as they can't be in a
reference loop. They neither can be directly recycled by GC nor affect
loop searching.

Let's skip io_uring SCM accounting for loop-less files, i.e. all but
af_unix sockets, quite imroving fixed file updates performance and
greatly helpnig with memory footprint.

Signed-off-by: Pavel Begunkov <asml.silence@gmail.com>
---
 fs/io_uring.c | 51 ++++++++++++++++++++++++++++++++++++++-------------
 1 file changed, 38 insertions(+), 13 deletions(-)

diff --git a/fs/io_uring.c b/fs/io_uring.c
index b4c85d85f88d..be178694e8db 100644
--- a/fs/io_uring.c
+++ b/fs/io_uring.c
@@ -1211,6 +1211,18 @@ struct sock *io_uring_get_socket(struct file *file)
 }
 EXPORT_SYMBOL(io_uring_get_socket);
 
+#if defined(CONFIG_UNIX)
+static inline bool io_file_need_scm(struct file *filp)
+{
+	return !!unix_get_socket(filp);
+}
+#else
+static inline bool io_file_need_scm(struct file *filp)
+{
+	return 0;
+}
+#endif
+
 static inline void io_tw_lock(struct io_ring_ctx *ctx, bool *locked)
 {
 	if (!*locked) {
@@ -8424,6 +8436,17 @@ static void io_free_file_tables(struct io_file_table *table)
 
 static void __io_sqe_files_unregister(struct io_ring_ctx *ctx)
 {
+	int i;
+
+	for (i = 0; i < ctx->nr_user_files; i++) {
+		struct file *file = io_file_from_index(ctx, i);
+
+		if (!file || io_file_need_scm(file))
+			continue;
+		io_fixed_file_slot(&ctx->file_table, i)->file_ptr = 0;
+		fput(file);
+	}
+
 #if defined(CONFIG_UNIX)
 	if (ctx->ring_sock) {
 		struct sock *sock = ctx->ring_sock->sk;
@@ -8432,16 +8455,6 @@ static void __io_sqe_files_unregister(struct io_ring_ctx *ctx)
 		while ((skb = skb_dequeue(&sock->sk_receive_queue)) != NULL)
 			kfree_skb(skb);
 	}
-#else
-	int i;
-
-	for (i = 0; i < ctx->nr_user_files; i++) {
-		struct file *file;
-
-		file = io_file_from_index(ctx, i);
-		if (file)
-			fput(file);
-	}
 #endif
 	io_free_file_tables(&ctx->file_table);
 	io_rsrc_data_free(ctx->file_data);
@@ -8590,7 +8603,9 @@ static struct io_sq_data *io_get_sq_data(struct io_uring_params *p,
 /*
  * Ensure the UNIX gc is aware of our file set, so we are certain that
  * the io_uring can be safely unregistered on process exit, even if we have
- * loops in the file referencing.
+ * loops in the file referencing. We account only files that can hold other
+ * files because otherwise they can't form a loop and so are not interesting
+ * for GC.
  */
 static int __io_sqe_files_scm(struct io_ring_ctx *ctx, int nr, int offset)
 {
@@ -8616,8 +8631,9 @@ static int __io_sqe_files_scm(struct io_ring_ctx *ctx, int nr, int offset)
 	for (i = 0; i < nr; i++) {
 		struct file *file = io_file_from_index(ctx, i + offset);
 
-		if (!file)
+		if (!file || !io_file_need_scm(file))
 			continue;
+
 		fpl->fp[nr_files] = get_file(file);
 		unix_inflight(fpl->user, fpl->fp[nr_files]);
 		nr_files++;
@@ -8634,7 +8650,7 @@ static int __io_sqe_files_scm(struct io_ring_ctx *ctx, int nr, int offset)
 		for (i = 0; i < nr; i++) {
 			struct file *file = io_file_from_index(ctx, i + offset);
 
-			if (file)
+			if (file && io_file_need_scm(file))
 				fput(file);
 		}
 	} else {
@@ -8676,6 +8692,7 @@ static int io_sqe_files_scm(struct io_ring_ctx *ctx)
 
 		if (file)
 			fput(file);
+		io_fixed_file_slot(&ctx->file_table, total)->file_ptr = 0;
 		total++;
 	}
 
@@ -8697,6 +8714,11 @@ static void io_rsrc_file_put(struct io_ring_ctx *ctx, struct io_rsrc_put *prsrc)
 	struct sk_buff *skb;
 	int i;
 
+	if (!io_file_need_scm(file)) {
+		fput(file);
+		return;
+	}
+
 	__skb_queue_head_init(&list);
 
 	/*
@@ -8889,6 +8911,9 @@ static int io_sqe_file_register(struct io_ring_ctx *ctx, struct file *file,
 	struct sk_buff_head *head = &sock->sk_receive_queue;
 	struct sk_buff *skb;
 
+	if (!io_file_need_scm(file))
+		return 0;
+
 	/*
 	 * See if we can merge this file into an existing skb SCM_RIGHTS
 	 * file set. If there's no room, fall back to allocating a new skb
-- 
2.35.1

Re: [PATCH 1/1] io_uring: don't scm-account for non af_unix sockets

[Jens Axboe]

On Wed, 6 Apr 2022 21:33:56 +0100, Pavel Begunkov wrote:
> io_uring deals with file reference loops by registering all fixed files
> in the SCM/GC infrastrucure. However, only a small subset of all file
> types can keep long-term references to other files and those that don't
> are not interesting for the garbage collector as they can't be in a
> reference loop. They neither can be directly recycled by GC nor affect
> loop searching.
> 
> [...]

Applied, thanks!

[1/1] io_uring: don't scm-account for non af_unix sockets
      commit: 181440ea22d834719489d2a33289f2834a333687

Best regards,
-- 
Jens Axboe

Re: [PATCH 1/1] io_uring: don't scm-account for non af_unix sockets

[Pavel Begunkov]

On 4/6/22 21:33, Pavel Begunkov wrote:
> io_uring deals with file reference loops by registering all fixed files
> in the SCM/GC infrastrucure. However, only a small subset of all file
> types can keep long-term references to other files and those that don't
> are not interesting for the garbage collector as they can't be in a
> reference loop. They neither can be directly recycled by GC nor affect
> loop searching.
> 
> Let's skip io_uring SCM accounting for loop-less files, i.e. all but
> af_unix sockets, quite imroving fixed file updates performance and
> greatly helpnig with memory footprint.

Just to throw some numbers, simple loop with fixed file updates
in batches of 32 showed 2-3x performance improvement
(~400K-500K updates/s -> ~1.2M).


> Signed-off-by: Pavel Begunkov <asml.silence@gmail.com>
> ---
>   fs/io_uring.c | 51 ++++++++++++++++++++++++++++++++++++++-------------
>   1 file changed, 38 insertions(+), 13 deletions(-)
> 
> diff --git a/fs/io_uring.c b/fs/io_uring.c
> index b4c85d85f88d..be178694e8db 100644
> --- a/fs/io_uring.c
> +++ b/fs/io_uring.c
> @@ -1211,6 +1211,18 @@ struct sock *io_uring_get_socket(struct file *file)
>   }
>   EXPORT_SYMBOL(io_uring_get_socket);
>   
> +#if defined(CONFIG_UNIX)
> +static inline bool io_file_need_scm(struct file *filp)
> +{
> +	return !!unix_get_socket(filp);
> +}
> +#else
> +static inline bool io_file_need_scm(struct file *filp)
> +{
> +	return 0;
> +}
> +#endif
> +
>   static inline void io_tw_lock(struct io_ring_ctx *ctx, bool *locked)
>   {
>   	if (!*locked) {
> @@ -8424,6 +8436,17 @@ static void io_free_file_tables(struct io_file_table *table)
>   
>   static void __io_sqe_files_unregister(struct io_ring_ctx *ctx)
>   {
> +	int i;
> +
> +	for (i = 0; i < ctx->nr_user_files; i++) {
> +		struct file *file = io_file_from_index(ctx, i);
> +
> +		if (!file || io_file_need_scm(file))
> +			continue;
> +		io_fixed_file_slot(&ctx->file_table, i)->file_ptr = 0;
> +		fput(file);
> +	}
> +
>   #if defined(CONFIG_UNIX)
>   	if (ctx->ring_sock) {
>   		struct sock *sock = ctx->ring_sock->sk;
> @@ -8432,16 +8455,6 @@ static void __io_sqe_files_unregister(struct io_ring_ctx *ctx)
>   		while ((skb = skb_dequeue(&sock->sk_receive_queue)) != NULL)
>   			kfree_skb(skb);
>   	}
> -#else
> -	int i;
> -
> -	for (i = 0; i < ctx->nr_user_files; i++) {
> -		struct file *file;
> -
> -		file = io_file_from_index(ctx, i);
> -		if (file)
> -			fput(file);
> -	}
>   #endif
>   	io_free_file_tables(&ctx->file_table);
>   	io_rsrc_data_free(ctx->file_data);
> @@ -8590,7 +8603,9 @@ static struct io_sq_data *io_get_sq_data(struct io_uring_params *p,
>   /*
>    * Ensure the UNIX gc is aware of our file set, so we are certain that
>    * the io_uring can be safely unregistered on process exit, even if we have
> - * loops in the file referencing.
> + * loops in the file referencing. We account only files that can hold other
> + * files because otherwise they can't form a loop and so are not interesting
> + * for GC.
>    */
>   static int __io_sqe_files_scm(struct io_ring_ctx *ctx, int nr, int offset)
>   {
> @@ -8616,8 +8631,9 @@ static int __io_sqe_files_scm(struct io_ring_ctx *ctx, int nr, int offset)
>   	for (i = 0; i < nr; i++) {
>   		struct file *file = io_file_from_index(ctx, i + offset);
>   
> -		if (!file)
> +		if (!file || !io_file_need_scm(file))
>   			continue;
> +
>   		fpl->fp[nr_files] = get_file(file);
>   		unix_inflight(fpl->user, fpl->fp[nr_files]);
>   		nr_files++;
> @@ -8634,7 +8650,7 @@ static int __io_sqe_files_scm(struct io_ring_ctx *ctx, int nr, int offset)
>   		for (i = 0; i < nr; i++) {
>   			struct file *file = io_file_from_index(ctx, i + offset);
>   
> -			if (file)
> +			if (file && io_file_need_scm(file))
>   				fput(file);
>   		}
>   	} else {
> @@ -8676,6 +8692,7 @@ static int io_sqe_files_scm(struct io_ring_ctx *ctx)
>   
>   		if (file)
>   			fput(file);
> +		io_fixed_file_slot(&ctx->file_table, total)->file_ptr = 0;
>   		total++;
>   	}
>   
> @@ -8697,6 +8714,11 @@ static void io_rsrc_file_put(struct io_ring_ctx *ctx, struct io_rsrc_put *prsrc)
>   	struct sk_buff *skb;
>   	int i;
>   
> +	if (!io_file_need_scm(file)) {
> +		fput(file);
> +		return;
> +	}
> +
>   	__skb_queue_head_init(&list);
>   
>   	/*
> @@ -8889,6 +8911,9 @@ static int io_sqe_file_register(struct io_ring_ctx *ctx, struct file *file,
>   	struct sk_buff_head *head = &sock->sk_receive_queue;
>   	struct sk_buff *skb;
>   
> +	if (!io_file_need_scm(file))
> +		return 0;
> +
>   	/*
>   	 * See if we can merge this file into an existing skb SCM_RIGHTS
>   	 * file set. If there's no room, fall back to allocating a new skb

-- 
Pavel Begunkov