Hi Thomas,
I'm looking at the rhashtable usage.
But as I read the nft_hash_destroy() its not clear to me how
rht_for_each_entry_safe() and nft_hash_elem_destroy() keep everything
in sync.
Here is the code in question,
static void nft_hash_destroy(const struct nft_set *set)
{
struct rhashtable *priv = nft_set_priv(set);
const struct bucket_table *tbl;
struct nft_hash_elem *he;
struct rhash_head *pos, *next;
unsigned int i;
/* Stop an eventual async resizing */
priv->being_destroyed = true;
mutex_lock(&priv->mutex); <-- get the lock so we have single updater
tbl = rht_dereference(priv->tbl, priv);
for (i = 0; i < tbl->size; i++) {
rht_for_each_entry_safe(he, pos, next, tbl, i, node)
nft_hash_elem_destroy(set, he); <-- does a kfree on he?
}
mutex_unlock(&priv->mutex); <-- release the lock
rhashtable_destroy(priv);
}
Is it really safe to kfree 'he' without waiting a grace
period for any rcu readers to drop the reference?
I'm considering what happens if nft_hash_destroy runs in
parallel with nft_hash_lookup?
Thanks,
John
--
John Fastabend Intel Corporation
On 01/12/15 at 02:30pm, John Fastabend wrote:
quoted
/* Stop an eventual async resizing */
priv->being_destroyed = true;
This aborts and eventual resize in the background.
quoted
mutex_lock(&priv->mutex); <-- get the lock so we have single updater
this ensures that the resize finished.
quoted
tbl = rht_dereference(priv->tbl, priv);
for (i = 0; i < tbl->size; i++) {
rht_for_each_entry_safe(he, pos, next, tbl, i, node)
nft_hash_elem_destroy(set, he); <-- does a kfree on he?
}
mutex_unlock(&priv->mutex); <-- release the lock
rhashtable_destroy(priv);
Since no insert or removal can occur we can be assured that no new
entry was added in the meantime so we can destroy the rhashtable
without any further protection.
quoted
}
Is it really safe to kfree 'he' without waiting a grace
period for any rcu readers to drop the reference?
I'm considering what happens if nft_hash_destroy runs in
parallel with nft_hash_lookup?
The nft_set API ensures that a destroy can't occur in parallel to
an insertion or removal. All we have to ensure is that any resizing
in the background is aborted and completed.
If you look at the code before the rhashtable there was no locking
at all:
static void nft_hash_destroy(const struct nft_set *set)
{
const struct nft_hash *priv = nft_set_priv(set);
const struct nft_hash_table *tbl = nft_dereference(priv->tbl);
struct nft_hash_elem *he, *next;
unsigned int i;
for (i = 0; i < tbl->size; i++) {
for (he = nft_dereference(tbl->buckets[i]); he != NULL;
he = next) {
next = nft_dereference(he->next);
nft_hash_elem_destroy(set, he);
}
}
kfree(tbl);
}
On 01/12/2015 02:42 PM, tgraf wrote:
On 01/12/15 at 02:30pm, John Fastabend wrote:
quoted
quoted
/* Stop an eventual async resizing */
priv->being_destroyed = true;
This aborts and eventual resize in the background.
quoted
quoted
mutex_lock(&priv->mutex); <-- get the lock so we have single updater
this ensures that the resize finished.
quoted
quoted
tbl = rht_dereference(priv->tbl, priv);
for (i = 0; i < tbl->size; i++) {
rht_for_each_entry_safe(he, pos, next, tbl, i, node)
nft_hash_elem_destroy(set, he); <-- does a kfree on he?
}
mutex_unlock(&priv->mutex); <-- release the lock
rhashtable_destroy(priv);
Since no insert or removal can occur we can be assured that no new
entry was added in the meantime so we can destroy the rhashtable
without any further protection.
quoted
quoted
}
Is it really safe to kfree 'he' without waiting a grace
period for any rcu readers to drop the reference?
I'm considering what happens if nft_hash_destroy runs in
parallel with nft_hash_lookup?
The nft_set API ensures that a destroy can't occur in parallel to
an insertion or removal. All we have to ensure is that any resizing
in the background is aborted and completed.
It must also somehow ensure there are no readers with a reference
as well. Thanks for the explanation.
If you look at the code before the rhashtable there was no locking
at all:
static void nft_hash_destroy(const struct nft_set *set)
{
const struct nft_hash *priv = nft_set_priv(set);
const struct nft_hash_table *tbl = nft_dereference(priv->tbl);
struct nft_hash_elem *he, *next;
unsigned int i;
for (i = 0; i < tbl->size; i++) {
for (he = nft_dereference(tbl->buckets[i]); he != NULL;
he = next) {
next = nft_dereference(he->next);
nft_hash_elem_destroy(set, he);
}
}
kfree(tbl);
}
--
John Fastabend Intel Corporation