Messages in this thread | | | Date | Tue, 21 Feb 2023 17:45:02 +0000 | Subject | Re: [PATCH 1/2] io_uring: Move from hlist to io_wq_work_node | From | Pavel Begunkov <> |
| |
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
| |