| Message ID | 20230221135721.3230763-1-leitao@debian.org (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | [1/2] io_uring: Move from hlist to io_wq_work_node | expand |
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); }
On 21/02/2023 17:45, Pavel Begunkov wrote: > 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. The extra brackets are required if we are assignments in if, otherwise the compiler raises a warning (bugprone-assignment-in-if-condition) > Personally, I don't have anything > against assignments in if, but it's probably better to avoid them Sure. I will remove the assignents in "if" part and maybe replicate what we have in io_alloc_cache_get(). Something as: if (cache->list.next) { node = cache->list.next; Thanks for the review!
On 2/21/23 18:38, Breno Leitao wrote: > On 21/02/2023 17:45, Pavel Begunkov wrote: >> 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. > > The extra brackets are required if we are assignments in if, otherwise > the compiler raises a warning (bugprone-assignment-in-if-condition) I mean braces / curly brackets. >> Personally, I don't have anything >> against assignments in if, but it's probably better to avoid them > > Sure. I will remove the assignents in "if" part and maybe replicate what > we have > in io_alloc_cache_get(). Something as: > if (cache->list.next) { > node = cache->list.next; > > Thanks for the review!
On 2/21/23 11:38?AM, Breno Leitao wrote: > Sure. I will remove the assignents in "if" part and maybe replicate what > we have > in io_alloc_cache_get(). Something as: > if (cache->list.next) { > node = cache->list.next; > > Thanks for the review! Pavel is right in that we generally discourage assignments in if statements etc. For the above, the usual approach would be: node = cache->list.next; if (node) { ... }
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 { }; struct io_alloc_cache { - struct hlist_head list; + struct io_wq_work_node list; unsigned int nr_cached; }; diff --git a/io_uring/alloc_cache.h b/io_uring/alloc_cache.h index 729793ae9712..0d9ff9402a37 100644 --- a/io_uring/alloc_cache.h +++ b/io_uring/alloc_cache.h @@ -7,7 +7,7 @@ #define IO_ALLOC_CACHE_MAX 512 struct io_cache_entry { - struct hlist_node node; + struct io_wq_work_node node; }; static inline bool io_alloc_cache_put(struct io_alloc_cache *cache, @@ -15,7 +15,7 @@ static inline bool io_alloc_cache_put(struct io_alloc_cache *cache, { if (cache->nr_cached < IO_ALLOC_CACHE_MAX) { cache->nr_cached++; - hlist_add_head(&entry->node, &cache->list); + wq_stack_add_head(&entry->node, &cache->list); return true; } return false; @@ -23,11 +23,14 @@ static inline bool io_alloc_cache_put(struct io_alloc_cache *cache, static inline struct io_cache_entry *io_alloc_cache_get(struct io_alloc_cache *cache) { - 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); + 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); } + cache->nr_cached = 0; } #endif
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. 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(-)