Skip to contentSkip to conversation

nft_hash rhashtable question

DISCUSSION John Fastabend 2 participants 3 messages Jan 12, 2015

Subsystems (2)
Networking Network drivers
lore
Share & tools
lore

John Fastabend

author
#0 lore raw

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,

Quoted 13 lines

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);
Quoted 8 lines
        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

Signature · 1 line
John Fastabend         Intel Corporation
#1 lore raw
Quoted 2 lines · John Fastabend
Quoted 2 lines
       /* Stop an eventual async resizing */
       priv->being_destroyed = true;

This aborts and eventual resize in the background.

Quoted 1 line
Quoted 1 line
       mutex_lock(&priv->mutex); <-- get the lock so we have single updater

this ensures that the resize finished.

Quoted 8 lines
Quoted 8 lines
       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 8 lines
Quoted 1 line

}

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);
}
author
#2 lore raw
Quoted 29 lines · tgraf
Quoted 2 lines · John Fastabend
Quoted 2 lines
        /* Stop an eventual async resizing */
        priv->being_destroyed = true;

This aborts and eventual resize in the background.

Quoted 1 line
Quoted 1 line
        mutex_lock(&priv->mutex); <-- get the lock so we have single updater

this ensures that the resize finished.

Quoted 8 lines
Quoted 8 lines
        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 6 lines
Quoted 1 line

}

Is it really safe to kfree 'he' without waiting a grace period for any rcu readers to drop the reference?

Quoted 7 lines
Quoted 2 lines

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.

Quoted 20 lines

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);
}
Signature · 1 line
John Fastabend         Intel Corporation