On Tue, Oct 22, 2019 at 2:58 PM Pravin Shelar <pshe...@ovn.org> wrote:
>
> On Sun, Oct 20, 2019 at 10:02 PM Tonghao Zhang <xiangxia.m....@gmail.com>
> wrote:
> >
> > On Sat, Oct 19, 2019 at 2:12 AM Pravin Shelar <pshe...@ovn.org> wrote:
> > >
> > > On Thu, Oct 17, 2019 at 8:16 PM Tonghao Zhang <xiangxia.m....@gmail.com>
> > > wrote:
> > > >
> > > > On Fri, Oct 18, 2019 at 6:38 AM Pravin Shelar <pshe...@ovn.org> wrote:
> > > > >
> > > > > On Wed, Oct 16, 2019 at 5:50 AM <xiangxia.m....@gmail.com> wrote:
> > > > > >
> > > > > > From: Tonghao Zhang <xiangxia.m....@gmail.com>
> > > > > >
> > > > > > When we destroy the flow tables which may contain the flow_mask,
> > > > > > so release the flow mask struct.
> > > > > >
> > > > > > Signed-off-by: Tonghao Zhang <xiangxia.m....@gmail.com>
> > > > > > Tested-by: Greg Rose <gvrose8...@gmail.com>
> > > > > > ---
> > > > > > net/openvswitch/flow_table.c | 14 +++++++++++++-
> > > > > > 1 file changed, 13 insertions(+), 1 deletion(-)
> > > > > >
> > > > > > diff --git a/net/openvswitch/flow_table.c
> > > > > > b/net/openvswitch/flow_table.c
> > > > > > index 5df5182..d5d768e 100644
> > > > > > --- a/net/openvswitch/flow_table.c
> > > > > > +++ b/net/openvswitch/flow_table.c
> > > > > > @@ -295,6 +295,18 @@ static void table_instance_destroy(struct
> > > > > > table_instance *ti,
> > > > > > }
> > > > > > }
> > > > > >
> > > > > > +static void tbl_mask_array_destroy(struct flow_table *tbl)
> > > > > > +{
> > > > > > + struct mask_array *ma = ovsl_dereference(tbl->mask_array);
> > > > > > + int i;
> > > > > > +
> > > > > > + /* Free the flow-mask and kfree_rcu the NULL is allowed. */
> > > > > > + for (i = 0; i < ma->max; i++)
> > > > > > + kfree_rcu(rcu_dereference_raw(ma->masks[i]), rcu);
> > > > > > +
> > > > > > + kfree_rcu(rcu_dereference_raw(tbl->mask_array), rcu);
> > > > > > +}
> > > > > > +
> > > > > > /* No need for locking this function is called from RCU callback or
> > > > > > * error path.
> > > > > > */
> > > > > > @@ -304,7 +316,7 @@ void ovs_flow_tbl_destroy(struct flow_table
> > > > > > *table)
> > > > > > struct table_instance *ufid_ti =
> > > > > > rcu_dereference_raw(table->ufid_ti);
> > > > > >
> > > > > > free_percpu(table->mask_cache);
> > > > > > - kfree_rcu(rcu_dereference_raw(table->mask_array), rcu);
> > > > > > + tbl_mask_array_destroy(table);
> > > > > > table_instance_destroy(ti, ufid_ti, false);
> > > > > > }
> > > > >
> > > > > This should not be required. mask is linked to a flow and gets
> > > > > released when flow is removed.
> > > > > Does the memory leak occur when OVS module is abruptly unloaded and
> > > > > userspace does not cleanup flow table?
> > > > When we destroy the ovs datapath or net namespace is destroyed , the
> > > > mask memory will be happened. The call tree:
> > > > ovs_exit_net/ovs_dp_cmd_del
> > > > -->__dp_destroy
> > > > -->destroy_dp_rcu
> > > > -->ovs_flow_tbl_destroy
> > > > -->table_instance_destroy (which don't release the mask memory because
> > > > don't call the ovs_flow_tbl_remove /flow_mask_remove directly or
> > > > indirectly).
> > > >
> > > Thats what I suggested earlier, we need to call function similar to
> > > ovs_flow_tbl_remove(), we could refactor code to use the code.
> > > This is better since by introducing tbl_mask_array_destroy() is
> > > creating a dangling pointer to mask in sw-flow object. OVS is anyway
> > > iterating entire flow table to release sw-flow in
> > > table_instance_destroy(), it is natural to release mask at that point
> > > after releasing corresponding sw-flow.
> > I got it, thanks. I rewrite the codes, can you help me to review it.
> > If fine, I will sent it next version.
> > >
> > >
> Sure, I can review it, Can you send the patch inlined in mail?
>
> Thanks.
diff --git a/net/openvswitch/flow_table.c b/net/openvswitch/flow_table.c
index 5df5182..5b20793 100644
--- a/net/openvswitch/flow_table.c
+++ b/net/openvswitch/flow_table.c
@@ -257,10 +257,75 @@ static void flow_tbl_destroy_rcu_cb(struct rcu_head *rcu)
__table_instance_destroy(ti);
}
-static void table_instance_destroy(struct table_instance *ti,
- struct table_instance *ufid_ti,
+static void tbl_mask_array_del_mask(struct flow_table *tbl,
+ struct sw_flow_mask *mask)
+{
+ struct mask_array *ma = ovsl_dereference(tbl->mask_array);
+ int i, ma_count = READ_ONCE(ma->count);
+
+ /* Remove the deleted mask pointers from the array */
+ for (i = 0; i < ma_count; i++) {
+ if (mask == ovsl_dereference(ma->masks[i]))
+ goto found;
+ }
+
+ BUG();
+ return;
+
+found:
+ WRITE_ONCE(ma->count, ma_count -1);
+
+ rcu_assign_pointer(ma->masks[i], ma->masks[ma_count -1]);
+ RCU_INIT_POINTER(ma->masks[ma_count -1], NULL);
+
+ kfree_rcu(mask, rcu);
+
+ /* Shrink the mask array if necessary. */
+ if (ma->max >= (MASK_ARRAY_SIZE_MIN * 2) &&
+ ma_count <= (ma->max / 3))
+ tbl_mask_array_realloc(tbl, ma->max / 2);
+}
+
+/* Remove 'mask' from the mask list, if it is not needed any more. */
+static void flow_mask_remove(struct flow_table *tbl, struct sw_flow_mask *mask)
+{
+ if (mask) {
+ /* ovs-lock is required to protect mask-refcount and
+ * mask list.
+ */
+ ASSERT_OVSL();
+ BUG_ON(!mask->ref_count);
+ mask->ref_count--;
+
+ if (!mask->ref_count)
+ tbl_mask_array_del_mask(tbl, mask);
+ }
+}
+
+static void table_instance_remove(struct flow_table *table, struct
sw_flow *flow)
+{
+ struct table_instance *ti = ovsl_dereference(table->ti);
+ struct table_instance *ufid_ti = ovsl_dereference(table->ufid_ti);
+
+ BUG_ON(table->count == 0);
+ hlist_del_rcu(&flow->flow_table.node[ti->node_ver]);
+ table->count--;
+ if (ovs_identifier_is_ufid(&flow->id)) {
+ hlist_del_rcu(&flow->ufid_table.node[ufid_ti->node_ver]);
+ table->ufid_count--;
+ }
+
+ /* RCU delete the mask. 'flow->mask' is not NULLed, as it should be
+ * accessible as long as the RCU read lock is held.
+ */
+ flow_mask_remove(table, flow->mask);
+}
+
+static void table_instance_destroy(struct flow_table *table,
bool deferred)
{
+ struct table_instance *ti = ovsl_dereference(table->ti);
+ struct table_instance *ufid_ti = ovsl_dereference(table->ufid_ti);
int i;
if (!ti)
@@ -274,13 +339,9 @@ static void table_instance_destroy(struct
table_instance *ti,
struct sw_flow *flow;
struct hlist_head *head = &ti->buckets[i];
struct hlist_node *n;
- int ver = ti->node_ver;
- int ufid_ver = ufid_ti->node_ver;
- hlist_for_each_entry_safe(flow, n, head, flow_table.node[ver]) {
- hlist_del_rcu(&flow->flow_table.node[ver]);
- if (ovs_identifier_is_ufid(&flow->id))
- hlist_del_rcu(&flow->ufid_table.node[ufid_ver]);
+ hlist_for_each_entry_safe(flow, n, head,
flow_table.node[ti->node_ver]) {
+ table_instance_remove(table, flow);
ovs_flow_free(flow, deferred);
}
}
@@ -300,12 +361,9 @@ static void table_instance_destroy(struct
table_instance *ti,
*/
void ovs_flow_tbl_destroy(struct flow_table *table)
{
- struct table_instance *ti = rcu_dereference_raw(table->ti);
- struct table_instance *ufid_ti = rcu_dereference_raw(table->ufid_ti);
-
free_percpu(table->mask_cache);
kfree_rcu(rcu_dereference_raw(table->mask_array), rcu);
- table_instance_destroy(ti, ufid_ti, false);
+ table_instance_destroy(table, false);
}
struct sw_flow *ovs_flow_tbl_dump_next(struct table_instance *ti,
@@ -400,10 +458,9 @@ static struct table_instance
*table_instance_rehash(struct table_instance *ti,
return new_ti;
}
-int ovs_flow_tbl_flush(struct flow_table *flow_table)
+int ovs_flow_tbl_flush(struct flow_table *table)
{
- struct table_instance *old_ti, *new_ti;
- struct table_instance *old_ufid_ti, *new_ufid_ti;
+ struct table_instance *new_ti, *new_ufid_ti;
new_ti = table_instance_alloc(TBL_MIN_BUCKETS);
if (!new_ti)
@@ -412,16 +469,12 @@ int ovs_flow_tbl_flush(struct flow_table *flow_table)
if (!new_ufid_ti)
goto err_free_ti;
- old_ti = ovsl_dereference(flow_table->ti);
- old_ufid_ti = ovsl_dereference(flow_table->ufid_ti);
+ table_instance_destroy(table, true);
- rcu_assign_pointer(flow_table->ti, new_ti);
- rcu_assign_pointer(flow_table->ufid_ti, new_ufid_ti);
- flow_table->last_rehash = jiffies;
- flow_table->count = 0;
- flow_table->ufid_count = 0;
+ rcu_assign_pointer(table->ti, new_ti);
+ rcu_assign_pointer(table->ufid_ti, new_ufid_ti);
+ table->last_rehash = jiffies;
- table_instance_destroy(old_ti, old_ufid_ti, true);
return 0;
err_free_ti:
@@ -700,69 +753,10 @@ static struct table_instance
*table_instance_expand(struct table_instance *ti,
return table_instance_rehash(ti, ti->n_buckets * 2, ufid);
}
-static void tbl_mask_array_del_mask(struct flow_table *tbl,
- struct sw_flow_mask *mask)
-{
- struct mask_array *ma = ovsl_dereference(tbl->mask_array);
- int i, ma_count = READ_ONCE(ma->count);
-
- /* Remove the deleted mask pointers from the array */
- for (i = 0; i < ma_count; i++) {
- if (mask == ovsl_dereference(ma->masks[i]))
- goto found;
- }
-
- BUG();
- return;
-
-found:
- WRITE_ONCE(ma->count, ma_count -1);
-
- rcu_assign_pointer(ma->masks[i], ma->masks[ma_count -1]);
- RCU_INIT_POINTER(ma->masks[ma_count -1], NULL);
-
- kfree_rcu(mask, rcu);
-
- /* Shrink the mask array if necessary. */
- if (ma->max >= (MASK_ARRAY_SIZE_MIN * 2) &&
- ma_count <= (ma->max / 3))
- tbl_mask_array_realloc(tbl, ma->max / 2);
-}
-
-/* Remove 'mask' from the mask list, if it is not needed any more. */
-static void flow_mask_remove(struct flow_table *tbl, struct sw_flow_mask *mask)
-{
- if (mask) {
- /* ovs-lock is required to protect mask-refcount and
- * mask list.
- */
- ASSERT_OVSL();
- BUG_ON(!mask->ref_count);
- mask->ref_count--;
-
- if (!mask->ref_count)
- tbl_mask_array_del_mask(tbl, mask);
- }
-}
-
/* Must be called with OVS mutex held. */
void ovs_flow_tbl_remove(struct flow_table *table, struct sw_flow *flow)
{
- struct table_instance *ti = ovsl_dereference(table->ti);
- struct table_instance *ufid_ti = ovsl_dereference(table->ufid_ti);
-
- BUG_ON(table->count == 0);
- hlist_del_rcu(&flow->flow_table.node[ti->node_ver]);
- table->count--;
- if (ovs_identifier_is_ufid(&flow->id)) {
- hlist_del_rcu(&flow->ufid_table.node[ufid_ti->node_ver]);
- table->ufid_count--;
- }
-
- /* RCU delete the mask. 'flow->mask' is not NULLed, as it should be
- * accessible as long as the RCU read lock is held.
- */
- flow_mask_remove(table, flow->mask);
+ table_instance_remove(table, flow);
}
static struct sw_flow_mask *mask_alloc(void)
