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@xxxxxxxxx>
Signed-off-by: Breno Leitao <leitao@xxxxxxxxxx>
---
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!
--
Pavel Begunkov
