On Mon 26-09-22 10:58:00, Michal Hocko wrote:
[...]
> A better option to me seems to be reworking the rhashtable_insert_rehash
> to not rely on an atomic allocation. I am not familiar with that code
> but it seems to me that the only reason this allocation mode is used is
> due to rcu locking around rhashtable_try_insert. Is there any reason we
> cannot drop the rcu lock, allocate with the full GFP_KERNEL allocation
> power and retry with the pre allocated object? rhashtable_insert_slow is
> already doing that to implement its never fail semantic.
So a very blunt and likely not 100% correct take on this side of things.
But it should give an idea at least.
---
diff --git a/lib/rhashtable.c b/lib/rhashtable.c
index e12bbfb240b8..fc547c43b05d 100644
--- a/lib/rhashtable.c
+++ b/lib/rhashtable.c
@@ -437,31 +437,11 @@ static void rht_deferred_worker(struct work_struct *work)
}
static int rhashtable_insert_rehash(struct rhashtable *ht,
- struct bucket_table *tbl)
+ struct bucket_table *tbl,
+ struct bucket_table *new_tbl)
{
- struct bucket_table *old_tbl;
- struct bucket_table *new_tbl;
- unsigned int size;
int err;
- old_tbl = rht_dereference_rcu(ht->tbl, ht);
-
- size = tbl->size;
-
- err = -EBUSY;
-
- if (rht_grow_above_75(ht, tbl))
- size *= 2;
- /* Do not schedule more than one rehash */
- else if (old_tbl != tbl)
- goto fail;
-
- err = -ENOMEM;
-
- new_tbl = bucket_table_alloc(ht, size, GFP_ATOMIC | __GFP_NOWARN);
- if (new_tbl == NULL)
- goto fail;
-
err = rhashtable_rehash_attach(ht, tbl, new_tbl);
if (err) {
bucket_table_free(new_tbl);
@@ -471,17 +451,6 @@ static int rhashtable_insert_rehash(struct rhashtable *ht,
schedule_work(&ht->run_work);
return err;
-
-fail:
- /* Do not fail the insert if someone else did a rehash. */
- if (likely(rcu_access_pointer(tbl->future_tbl)))
- return 0;
-
- /* Schedule async rehash to retry allocation in process context. */
- if (err == -ENOMEM)
- schedule_work(&ht->run_work);
-
- return err;
}
static void *rhashtable_lookup_one(struct rhashtable *ht,
@@ -619,9 +588,33 @@ static void *rhashtable_try_insert(struct rhashtable *ht, const void *key,
}
} while (!IS_ERR_OR_NULL(new_tbl));
- if (PTR_ERR(data) == -EAGAIN)
- data = ERR_PTR(rhashtable_insert_rehash(ht, tbl) ?:
+ if (PTR_ERR(data) == -EAGAIN) {
+ struct bucket_table *old_tbl;
+ unsigned int size;
+
+ old_tbl = rht_dereference_rcu(ht->tbl, ht);
+ size = tbl->size;
+
+ data = ERR_PTR(-EBUSY);
+
+ if (rht_grow_above_75(ht, tbl))
+ size *= 2;
+ /* Do not schedule more than one rehash */
+ else if (old_tbl != tbl)
+ return data;
+
+ data = ERR_PTR(-ENOMEM);
+
+ rcu_read_unlock();
+ new_tbl = bucket_table_alloc(ht, size, GFP_KERNEL);
+ rcu_read_lock();
+
+ if (!new_tbl)
+ return data;
+
+ data = ERR_PTR(rhashtable_insert_rehash(ht, tbl, new_tbl) ?:
-EAGAIN);
+ }
return data;
}
--
Michal Hocko
SUSE Labs
