Message-ID: <782b4b43-790c-6e89-ea74-aac1fd4ff1e2@gmail.com>
Date:   Tue, 21 Feb 2023 17:45:02 +0000
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:102.0) Gecko/20100101
 Thunderbird/102.7.2
Subject: Re: [PATCH 1/2] io_uring: Move from hlist to io_wq_work_node
Content-Language: en-US
To:     Breno Leitao <leitao@debian.org>, axboe@kernel.dk,
        io-uring@vger.kernel.org
Cc:     linux-kernel@vger.kernel.org, gustavold@meta.com, leit@meta.com
References: <20230221135721.3230763-1-leitao@debian.org>
From:   Pavel Begunkov <asml.silence@gmail.com>
In-Reply-To: <20230221135721.3230763-1-leitao@debian.org>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 7bit
Precedence: bulk

On 2/21/23 13:57, Breno Leitao wrote:
> Having cache entries linked using the hlist format brings no benefit, and
> also requires an unnecessary extra pointer address per cache entry.
> 
> Use the internal io_wq_work_node single-linked list for the internal
> alloc caches (async_msghdr and async_poll)
> 
> This is required to be able to use KASAN on cache entries, since we do
> not need to touch unused (and poisoned) cache entries when adding more
> entries to the list.

Looks good, a few nits

> 
> Suggested-by: Pavel Begunkov <asml.silence@gmail.com>
> Signed-off-by: Breno Leitao <leitao@debian.org>
> ---
>   include/linux/io_uring_types.h |  2 +-
>   io_uring/alloc_cache.h         | 27 +++++++++++++++------------
>   2 files changed, 16 insertions(+), 13 deletions(-)
> 
> diff --git a/include/linux/io_uring_types.h b/include/linux/io_uring_types.h
> index 0efe4d784358..efa66b6c32c9 100644
> --- a/include/linux/io_uring_types.h
> +++ b/include/linux/io_uring_types.h
> @@ -188,7 +188,7 @@ struct io_ev_fd {
>   };
>   
[...]
> -	if (!hlist_empty(&cache->list)) {
> -		struct hlist_node *node = cache->list.first;
> -
> -		hlist_del(node);
> -		return container_of(node, struct io_cache_entry, node);
> +	struct io_wq_work_node *node;
> +	struct io_cache_entry *entry;
> +
> +	if (cache->list.next) {
> +		node = cache->list.next;
> +		entry = container_of(node, struct io_cache_entry, node);

I'd prefer to get rid of the node var, it'd be a bit cleaner
than keeping two pointers to the same chunk.

entry = container_of(node, struct io_cache_entry,
                      cache->list.next);

> +		cache->list.next = node->next;
> +		return entry;
>   	}
>   
>   	return NULL;
> @@ -35,19 +38,19 @@ static inline struct io_cache_entry *io_alloc_cache_get(struct io_alloc_cache *c
>   
>   static inline void io_alloc_cache_init(struct io_alloc_cache *cache)
>   {
> -	INIT_HLIST_HEAD(&cache->list);
> +	cache->list.next = NULL;
>   	cache->nr_cached = 0;
>   }
>   
>   static inline void io_alloc_cache_free(struct io_alloc_cache *cache,
>   					void (*free)(struct io_cache_entry *))
>   {
> -	while (!hlist_empty(&cache->list)) {
> -		struct hlist_node *node = cache->list.first;
> +	struct io_cache_entry *entry;
>   
> -		hlist_del(node);
> -		free(container_of(node, struct io_cache_entry, node));
> +	while ((entry = io_alloc_cache_get(cache))) {
> +		free(entry);

We don't need brackets here. Personally, I don't have anything
against assignments in if, but it's probably better to avoid them,
or there will be a patch in a couple of months based on a random
code analysis report as happened many times before.

while (1) {
	struct io_cache_entry *entry = get();

	if (!entry)
		break;
	free(entry);
}	

-- 
Pavel Begunkov