[ovs-dev,v2] classifier/dpif-netdev: Double postponing to free subtables.

From: Zhantao Fu <fuzhantao@huawei.com>

Subtable destruction should be double postponed because readers could still obtain old values while iterating over pvector implementation before its new version published.

Changes since v1:
- delete the unused parameter in code, modify the subject  and commit-message.

Signed-off-by: Zhantao Fu <fuzhantao@huawei.com>
---
 lib/classifier.c  | 31 +++++++++++++++++++------------  lib/dpif-netdev.c | 10 ++++++++--
 2 files changed, 27 insertions(+), 14 deletions(-)

     cmap_remove(&cls->subtables_map, &subtable->cmap_node,
                 subtable->mask.hash);
-    cmap_destroy(&subtable->rules);
-    ovsrcu_postpone(free, subtable);
+    ovsrcu_postpone(dpcls_subtable_destroy_cb, subtable);
 }
 
 /* Destroys 'cls'.  Rules within 'cls', if any, are not freed; this is the
--
1.9.5.msysgit.1

Bleep bloop.  Greetings fuzhantao, I am a robot and I have tried out your patch.
Thanks for your contribution.

I encountered some error that I wasn't expecting.  See the details below.


git-am:
fatal: corrupt patch at line 63
Repository lacks necessary blobs to fall back on 3-way merge.
Cannot fall back to three-way merge.
Patch failed at 0001 classifier/dpif-netdev: Double postponing to free subtables.
The copy of the patch that failed is found in:
   /var/lib/jenkins/jobs/upstream_build_from_pw/workspace/.git/rebase-apply/patch
When you have resolved this problem, run "git am --resolved".
If you prefer to skip this patch, run "git am --skip" instead.
To restore the original branch and stop patching, run "git am --abort".


Please check this out.  If you feel there has been an error, please email aconole@bytheb.org

Thanks,
0-day Robot

Hi.
Thanks for v2.
Patch is a bit malformed and can not be applied. I guess it's an
issue with your mail client or mail server. You may try sending
patch with 'git send-email' or re-check settings of your mail client.
Please increase the version in case of re-sending.

If above will not work for you, you could create a pull-request on
github. You'll need to send a link to the mail-list in this case.

On 23.04.2019 5:06, fuzhantao wrote:
> From: Zhantao Fu <fuzhantao@huawei.com>
> 
> Subtable destruction should be double postponed because readers could still obtain old values while iterating over pvector implementation before its new version published.
> 
> Changes since v1:
> - delete the unused parameter in code, modify the subject  and commit-message.

Changes between versions should go after the '---' because they should
not be part of the commit message. It's just a reference for reviewers.

> 
> Signed-off-by: Zhantao Fu <fuzhantao@huawei.com>
> ---
>  lib/classifier.c  | 31 +++++++++++++++++++------------  lib/dpif-netdev.c | 10 ++++++++--
>  2 files changed, 27 insertions(+), 14 deletions(-)
> 
> diff --git a/lib/classifier.c b/lib/classifier.c index edb40c8..0fad953 100644
> --- a/lib/classifier.c
> +++ b/lib/classifier.c
> @@ -1468,6 +1468,24 @@ miniflow_get_map_in_range(const struct miniflow *miniflow, uint8_t start,
>      return map;
>  }
>  
> +static void
> +subtable_destroy_cb(struct cls_subtable *subtable) {
> +    int i;
> +
> +    ovs_assert(ovsrcu_get_protected(struct trie_node *, &subtable->ports_trie)
> +               == NULL);
> +    ovs_assert(cmap_is_empty(&subtable->rules));
> +    ovs_assert(rculist_is_empty(&subtable->rules_list));
> +
> +    for (i = 0; i < subtable->n_indices; i++) {
> +        ccmap_destroy(&subtable->indices[i]);
> +    }
> +    cmap_destroy(&subtable->rules);
> +
> +    ovsrcu_postpone(free, subtable);
> +}
> +
>  /* The new subtable will be visible to the readers only after this. */  static struct cls_subtable *  insert_subtable(struct classifier *cls, const struct minimask *mask) @@ -1534,22 +1552,11 @@ insert_subtable(struct classifier *cls, const struct minimask *mask)  static void  destroy_subtable(struct classifier *cls, struct cls_subtable *subtable)  {
> -    int i;
> -
>      pvector_remove(&cls->subtables, subtable);
>      cmap_remove(&cls->subtables_map, &subtable->cmap_node,
>                  minimask_hash(&subtable->mask, 0));
>  
> -    ovs_assert(ovsrcu_get_protected(struct trie_node *, &subtable->ports_trie)
> -               == NULL);
> -    ovs_assert(cmap_is_empty(&subtable->rules));
> -    ovs_assert(rculist_is_empty(&subtable->rules_list));
> -
> -    for (i = 0; i < subtable->n_indices; i++) {
> -        ccmap_destroy(&subtable->indices[i]);
> -    }
> -    cmap_destroy(&subtable->rules);
> -    ovsrcu_postpone(free, subtable);
> +    ovsrcu_postpone(subtable_destroy_cb, subtable);
>  }
>  
>  static unsigned int be_get_bit_at(const ovs_be32 value[], unsigned int ofs); diff --git a/lib/dpif-netdev.c b/lib/dpif-netdev.c index 4d6d0c3..8657a61 100644
> --- a/lib/dpif-netdev.c
> +++ b/lib/dpif-netdev.c
> @@ -7592,6 +7592,13 @@ struct dpcls_subtable {
>      /* 'mask' must be the last field, additional space is allocated here. */  };
>  
> +static void
> +dpcls_subtable_destroy_cb(struct dpcls_subtable *subtable) {
> +    cmap_destroy(&subtable->rules);
> +    ovsrcu_postpone(free, subtable);
> +}
> +
>  /* Initializes 'cls' as a classifier that initially contains no classification
>   * rules. */
>  static void
> @@ -7608,8 +7615,7 @@ dpcls_destroy_subtable(struct dpcls *cls, struct dpcls_subtable *subtable)
>      pvector_remove(&cls->subtables, subtable);
>      cmap_remove(&cls->subtables_map, &subtable->cmap_node,
>                  subtable->mask.hash);
> -    cmap_destroy(&subtable->rules);
> -    ovsrcu_postpone(free, subtable);
> +    ovsrcu_postpone(dpcls_subtable_destroy_cb, subtable);
>  }
>  
>  /* Destroys 'cls'.  Rules within 'cls', if any, are not freed; this is the
> --
> 1.9.5.msysgit.1
> 
> 
> 
>