| Message ID | 20161116004612.79315-2-diproiettod@vmware.com |
|---|---|
| State | Superseded |
| Headers | show |
On 16.11.2016 03:45, Daniele Di Proietto wrote: > We keep all the per-port classifiers around, since they can be reused, > but when a pmd thread is destroyed we should free them. > > Found using valgrind. > > Fixes: 3453b4d62a98("dpif-netdev: dpcls per in_port with sorted > subtables") > > Signed-off-by: Daniele Di Proietto <diproiettod@vmware.com> > --- > lib/dpif-netdev.c | 1 + > 1 file changed, 1 insertion(+) > > diff --git a/lib/dpif-netdev.c b/lib/dpif-netdev.c > index c477248..c19d3e8 100644 > --- a/lib/dpif-netdev.c > +++ b/lib/dpif-netdev.c > @@ -3330,6 +3330,7 @@ dp_netdev_destroy_pmd(struct dp_netdev_pmd_thread *pmd) > /* All flows (including their dpcls_rules) have been deleted already */ > CMAP_FOR_EACH (cls, node, &pmd->classifiers) { > dpcls_destroy(cls); > + free(cls); free should be postponed using rcu. This change exists in my incremental patch: https://mail.openvswitch.org/pipermail/ovs-dev/2016-November/325404.html > } > cmap_destroy(&pmd->classifiers); > cmap_destroy(&pmd->flow_table); >
On 25/11/2016 06:22, "Ilya Maximets" <i.maximets@samsung.com> wrote: >On 16.11.2016 03:45, Daniele Di Proietto wrote: >> We keep all the per-port classifiers around, since they can be reused, >> but when a pmd thread is destroyed we should free them. >> >> Found using valgrind. >> >> Fixes: 3453b4d62a98("dpif-netdev: dpcls per in_port with sorted >> subtables") >> >> Signed-off-by: Daniele Di Proietto <diproiettod@vmware.com> >> --- >> lib/dpif-netdev.c | 1 + >> 1 file changed, 1 insertion(+) >> >> diff --git a/lib/dpif-netdev.c b/lib/dpif-netdev.c >> index c477248..c19d3e8 100644 >> --- a/lib/dpif-netdev.c >> +++ b/lib/dpif-netdev.c >> @@ -3330,6 +3330,7 @@ dp_netdev_destroy_pmd(struct dp_netdev_pmd_thread *pmd) >> /* All flows (including their dpcls_rules) have been deleted already */ >> CMAP_FOR_EACH (cls, node, &pmd->classifiers) { >> dpcls_destroy(cls); >> + free(cls); > >free should be postponed using rcu. >This change exists in my incremental patch: >https://mail.openvswitch.org/pipermail/ovs-dev/2016-November/325404.html I think that it should be safe to call free() immediately here, because nobody else is accessing the cmap: * CMAP_FOR_EACH is "safe" in the sense of HMAP_FOR_EACH_SAFE. That is, it is * safe to free the current node before going on to the next iteration. Most * of the time, though, this doesn't matter for a cmap because node * deallocation has to be postponed until the next grace period. This means * that this guarantee is useful only in deallocation code already executing at * postponed time, when it is known that the RCU grace period has already * expired. That said, it's probably better to postpone the free() than to remember this special case, especially if we end up moving that code somewhere else, so I took your advice. Thanks, Daniele > >> } >> cmap_destroy(&pmd->classifiers); >> cmap_destroy(&pmd->flow_table); >>
diff --git a/lib/dpif-netdev.c b/lib/dpif-netdev.c index c477248..c19d3e8 100644 --- a/lib/dpif-netdev.c +++ b/lib/dpif-netdev.c @@ -3330,6 +3330,7 @@ dp_netdev_destroy_pmd(struct dp_netdev_pmd_thread *pmd) /* All flows (including their dpcls_rules) have been deleted already */ CMAP_FOR_EACH (cls, node, &pmd->classifiers) { dpcls_destroy(cls); + free(cls); } cmap_destroy(&pmd->classifiers); cmap_destroy(&pmd->flow_table);
We keep all the per-port classifiers around, since they can be reused, but when a pmd thread is destroyed we should free them. Found using valgrind. Fixes: 3453b4d62a98("dpif-netdev: dpcls per in_port with sorted subtables") Signed-off-by: Daniele Di Proietto <diproiettod@vmware.com> --- lib/dpif-netdev.c | 1 + 1 file changed, 1 insertion(+)