| Message ID | 1571135440-24313-9-git-send-email-xiangxia.m.yue@gmail.com |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | optimize openvswitch flow looking up | expand |
On Wed, Oct 16, 2019 at 5:50 AM <xiangxia.m.yue@gmail.com> wrote: > > From: Tonghao Zhang <xiangxia.m.yue@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.yue@gmail.com> > Tested-by: Greg Rose <gvrose8192@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? In that case better fix could be calling ovs_flow_tbl_remove() equivalent from table_instance_destroy when it is iterating flow table.
On Fri, Oct 18, 2019 at 6:38 AM Pravin Shelar <pshelar@ovn.org> wrote: > > On Wed, Oct 16, 2019 at 5:50 AM <xiangxia.m.yue@gmail.com> wrote: > > > > From: Tonghao Zhang <xiangxia.m.yue@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.yue@gmail.com> > > Tested-by: Greg Rose <gvrose8192@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). but one thing, when we flush the flow, we don't flush the mask flow.( If necessary, one patch should be sent) > In that case better fix could be calling ovs_flow_tbl_remove() > equivalent from table_instance_destroy when it is iterating flow > table. I think operation of the flow mask and flow table should use different API, for example: for flow mask, we use the: -tbl_mask_array_add_mask -tbl_mask_array_del_mask -tbl_mask_array_alloc -tbl_mask_array_realloc -tbl_mask_array_destroy(this patch introduce.) table instance: -table_instance_alloc -table_instance_destroy ....
On Thu, Oct 17, 2019 at 8:16 PM Tonghao Zhang <xiangxia.m.yue@gmail.com> wrote: > > On Fri, Oct 18, 2019 at 6:38 AM Pravin Shelar <pshelar@ovn.org> wrote: > > > > On Wed, Oct 16, 2019 at 5:50 AM <xiangxia.m.yue@gmail.com> wrote: > > > > > > From: Tonghao Zhang <xiangxia.m.yue@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.yue@gmail.com> > > > Tested-by: Greg Rose <gvrose8192@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. > but one thing, when we flush the flow, we don't flush the mask flow.( > If necessary, one patch should be sent) > > > In that case better fix could be calling ovs_flow_tbl_remove() > > equivalent from table_instance_destroy when it is iterating flow > > table. > I think operation of the flow mask and flow table should use > different API, for example: > for flow mask, we use the: > -tbl_mask_array_add_mask > -tbl_mask_array_del_mask > -tbl_mask_array_alloc > -tbl_mask_array_realloc > -tbl_mask_array_destroy(this patch introduce.) > > table instance: > -table_instance_alloc > -table_instance_destroy > ....
On Sat, Oct 19, 2019 at 2:12 AM Pravin Shelar <pshelar@ovn.org> wrote: > > On Thu, Oct 17, 2019 at 8:16 PM Tonghao Zhang <xiangxia.m.yue@gmail.com> wrote: > > > > On Fri, Oct 18, 2019 at 6:38 AM Pravin Shelar <pshelar@ovn.org> wrote: > > > > > > On Wed, Oct 16, 2019 at 5:50 AM <xiangxia.m.yue@gmail.com> wrote: > > > > > > > > From: Tonghao Zhang <xiangxia.m.yue@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.yue@gmail.com> > > > > Tested-by: Greg Rose <gvrose8192@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. > > > > but one thing, when we flush the flow, we don't flush the mask flow.( > > If necessary, one patch should be sent) > > > > > In that case better fix could be calling ovs_flow_tbl_remove() > > > equivalent from table_instance_destroy when it is iterating flow > > > table. > > I think operation of the flow mask and flow table should use > > different API, for example: > > for flow mask, we use the: > > -tbl_mask_array_add_mask > > -tbl_mask_array_del_mask > > -tbl_mask_array_alloc > > -tbl_mask_array_realloc > > -tbl_mask_array_destroy(this patch introduce.) > > > > table instance: > > -table_instance_alloc > > -table_instance_destroy > > ....
On Sun, Oct 20, 2019 at 10:02 PM Tonghao Zhang <xiangxia.m.yue@gmail.com> wrote: > > On Sat, Oct 19, 2019 at 2:12 AM Pravin Shelar <pshelar@ovn.org> wrote: > > > > On Thu, Oct 17, 2019 at 8:16 PM Tonghao Zhang <xiangxia.m.yue@gmail.com> wrote: > > > > > > On Fri, Oct 18, 2019 at 6:38 AM Pravin Shelar <pshelar@ovn.org> wrote: > > > > > > > > On Wed, Oct 16, 2019 at 5:50 AM <xiangxia.m.yue@gmail.com> wrote: > > > > > > > > > > From: Tonghao Zhang <xiangxia.m.yue@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.yue@gmail.com> > > > > > Tested-by: Greg Rose <gvrose8192@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.
On Tue, Oct 22, 2019 at 2:58 PM Pravin Shelar <pshelar@ovn.org> wrote: > > On Sun, Oct 20, 2019 at 10:02 PM Tonghao Zhang <xiangxia.m.yue@gmail.com> wrote: > > > > On Sat, Oct 19, 2019 at 2:12 AM Pravin Shelar <pshelar@ovn.org> wrote: > > > > > > On Thu, Oct 17, 2019 at 8:16 PM Tonghao Zhang <xiangxia.m.yue@gmail.com> wrote: > > > > > > > > On Fri, Oct 18, 2019 at 6:38 AM Pravin Shelar <pshelar@ovn.org> wrote: > > > > > > > > > > On Wed, Oct 16, 2019 at 5:50 AM <xiangxia.m.yue@gmail.com> wrote: > > > > > > > > > > > > From: Tonghao Zhang <xiangxia.m.yue@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.yue@gmail.com> > > > > > > Tested-by: Greg Rose <gvrose8192@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)
On Tue, Oct 22, 2019 at 7:35 PM Tonghao Zhang <xiangxia.m.yue@gmail.com> wrote: > > On Tue, Oct 22, 2019 at 2:58 PM Pravin Shelar <pshelar@ovn.org> wrote: > > ... > > > > > > 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); > This would destroy running table causing unnecessary flow miss. Lets keep current scheme of setting up new table before destroying current one. > - 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); Can you just rename table_instance_remove() to ovs_flow_tbl_remove()?
On Thu, Oct 24, 2019 at 3:14 PM Pravin Shelar <pshelar@ovn.org> wrote: > > On Tue, Oct 22, 2019 at 7:35 PM Tonghao Zhang <xiangxia.m.yue@gmail.com> wrote: > > > > On Tue, Oct 22, 2019 at 2:58 PM Pravin Shelar <pshelar@ovn.org> wrote: > > > > ... > > > > > > > > > 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); > > > This would destroy running table causing unnecessary flow miss. Lets > keep current scheme of setting up new table before destroying current > one. > > > - 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); > Can you just rename table_instance_remove() to ovs_flow_tbl_remove()? diff --git a/net/openvswitch/flow_table.c b/net/openvswitch/flow_table.c index 5df5182..4871ab8 100644 --- a/net/openvswitch/flow_table.c +++ b/net/openvswitch/flow_table.c @@ -210,6 +210,74 @@ static int tbl_mask_array_realloc(struct flow_table *tbl, int size) return 0; } +static int tbl_mask_array_add_mask(struct flow_table *tbl, + struct sw_flow_mask *new) +{ + struct mask_array *ma = ovsl_dereference(tbl->mask_array); + int err, ma_count = READ_ONCE(ma->count); + + if (ma_count >= ma->max) { + err = tbl_mask_array_realloc(tbl, ma->max + + MASK_ARRAY_SIZE_MIN); + if (err) + return err; + + ma = ovsl_dereference(tbl->mask_array); + } + + BUG_ON(ovsl_dereference(ma->masks[ma_count])); + + rcu_assign_pointer(ma->masks[ma_count], new); + WRITE_ONCE(ma->count, ma_count +1); + + return 0; +} + +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); + } +} + int ovs_flow_tbl_init(struct flow_table *table) { struct table_instance *ti, *ufid_ti; @@ -257,7 +325,28 @@ static void flow_tbl_destroy_rcu_cb(struct rcu_head *rcu) __table_instance_destroy(ti); } -static void table_instance_destroy(struct table_instance *ti, +static void table_instance_remove(struct flow_table *table, + struct table_instance *ti, + struct table_instance *ufid_ti, + struct sw_flow *flow, + bool count) +{ + hlist_del_rcu(&flow->flow_table.node[ti->node_ver]); + if (count) + table->count--; + + if (ovs_identifier_is_ufid(&flow->id)) { + hlist_del_rcu(&flow->ufid_table.node[ufid_ti->node_ver]); + + if (count) + table->ufid_count--; + } + + flow_mask_remove(table, flow->mask); +} + +static void table_instance_destroy(struct flow_table *table, + struct table_instance *ti, struct table_instance *ufid_ti, bool deferred) { @@ -274,13 +363,11 @@ 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, ti, ufid_ti, flow, false); ovs_flow_free(flow, deferred); } } @@ -305,7 +392,7 @@ void ovs_flow_tbl_destroy(struct flow_table *table) 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, ti, ufid_ti, false); } struct sw_flow *ovs_flow_tbl_dump_next(struct table_instance *ti, @@ -421,7 +508,7 @@ int ovs_flow_tbl_flush(struct flow_table *flow_table) flow_table->count = 0; flow_table->ufid_count = 0; - table_instance_destroy(old_ti, old_ufid_ti, true); + table_instance_destroy(flow_table, old_ti, old_ufid_ti, true); return 0; err_free_ti: @@ -700,51 +787,6 @@ 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) { @@ -752,17 +794,7 @@ void ovs_flow_tbl_remove(struct flow_table *table, struct sw_flow *flow) 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, ti, ufid_ti, flow, true); } static struct sw_flow_mask *mask_alloc(void) @@ -805,29 +837,6 @@ static struct sw_flow_mask *flow_mask_find(const struct flow_table *tbl, return NULL; } -static int tbl_mask_array_add_mask(struct flow_table *tbl, - struct sw_flow_mask *new) -{ - struct mask_array *ma = ovsl_dereference(tbl->mask_array); - int err, ma_count = READ_ONCE(ma->count); - - if (ma_count >= ma->max) { - err = tbl_mask_array_realloc(tbl, ma->max + - MASK_ARRAY_SIZE_MIN); - if (err) - return err; - - ma = ovsl_dereference(tbl->mask_array); - } - - BUG_ON(ovsl_dereference(ma->masks[ma_count])); - - rcu_assign_pointer(ma->masks[ma_count], new); - WRITE_ONCE(ma->count, ma_count +1); - - return 0; -} - /* Add 'mask' into the mask list, if it is not already there. */ static int flow_mask_insert(struct flow_table *tbl, struct sw_flow *flow, const struct sw_flow_mask *new)
On Sun, Oct 27, 2019 at 11:49 PM Tonghao Zhang <xiangxia.m.yue@gmail.com> wrote: > > On Thu, Oct 24, 2019 at 3:14 PM Pravin Shelar <pshelar@ovn.org> wrote: > > > > On Tue, Oct 22, 2019 at 7:35 PM Tonghao Zhang <xiangxia.m.yue@gmail.com> wrote: > > > > > > On Tue, Oct 22, 2019 at 2:58 PM Pravin Shelar <pshelar@ovn.org> wrote: > > > > > > ... > > ... > > > 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); > > > > > This would destroy running table causing unnecessary flow miss. Lets > > keep current scheme of setting up new table before destroying current > > one. > > > > > - rcu_assign_pointer(flow_table->ti, new_ti); .... ... > /* Must be called with OVS mutex held. */ > void ovs_flow_tbl_remove(struct flow_table *table, struct sw_flow *flow) > { > @@ -752,17 +794,7 @@ void ovs_flow_tbl_remove(struct flow_table > *table, struct sw_flow *flow) > 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, ti, ufid_ti, flow, true); > } Lets rename table_instance_remove() to imply it is freeing a flow. Otherwise looks good.
On Tue, Oct 29, 2019 at 3:38 PM Pravin Shelar <pshelar@ovn.org> wrote: > > On Sun, Oct 27, 2019 at 11:49 PM Tonghao Zhang <xiangxia.m.yue@gmail.com> wrote: > > > > On Thu, Oct 24, 2019 at 3:14 PM Pravin Shelar <pshelar@ovn.org> wrote: > > > > > > On Tue, Oct 22, 2019 at 7:35 PM Tonghao Zhang <xiangxia.m.yue@gmail.com> wrote: > > > > > > > > On Tue, Oct 22, 2019 at 2:58 PM Pravin Shelar <pshelar@ovn.org> wrote: > > > > > > > > ... > > > > ... > > > > 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); > > > > > > > This would destroy running table causing unnecessary flow miss. Lets > > > keep current scheme of setting up new table before destroying current > > > one. > > > > > > > - rcu_assign_pointer(flow_table->ti, new_ti); > .... > ... > > /* Must be called with OVS mutex held. */ > > void ovs_flow_tbl_remove(struct flow_table *table, struct sw_flow *flow) > > { > > @@ -752,17 +794,7 @@ void ovs_flow_tbl_remove(struct flow_table > > *table, struct sw_flow *flow) > > 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, ti, ufid_ti, flow, true); > > } > Lets rename table_instance_remove() to imply it is freeing a flow. hi Pravin, the function ovs_flow_free will free the flow actually. In -ovs_flow_cmd_del ovs_flow_tbl_remove ... ovs_flow_free In -table_instance_destroy table_instance_remove ovs_flow_free But if rename the table_instance_remove, table_instance_flow_free ? > Otherwise looks good.
On Tue, Oct 29, 2019 at 4:31 AM Tonghao Zhang <xiangxia.m.yue@gmail.com> wrote: > > On Tue, Oct 29, 2019 at 3:38 PM Pravin Shelar <pshelar@ovn.org> wrote: > > > > On Sun, Oct 27, 2019 at 11:49 PM Tonghao Zhang <xiangxia.m.yue@gmail.com> wrote: > > > > > > On Thu, Oct 24, 2019 at 3:14 PM Pravin Shelar <pshelar@ovn.org> wrote: > > > > > > > > On Tue, Oct 22, 2019 at 7:35 PM Tonghao Zhang <xiangxia.m.yue@gmail.com> wrote: > > > > > > > > > > On Tue, Oct 22, 2019 at 2:58 PM Pravin Shelar <pshelar@ovn.org> wrote: > > > > > > > > > > ... > > > > > > ... > > > > > 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); > > > > > > > > > This would destroy running table causing unnecessary flow miss. Lets > > > > keep current scheme of setting up new table before destroying current > > > > one. > > > > > > > > > - rcu_assign_pointer(flow_table->ti, new_ti); > > .... > > ... > > > /* Must be called with OVS mutex held. */ > > > void ovs_flow_tbl_remove(struct flow_table *table, struct sw_flow *flow) > > > { > > > @@ -752,17 +794,7 @@ void ovs_flow_tbl_remove(struct flow_table > > > *table, struct sw_flow *flow) > > > 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, ti, ufid_ti, flow, true); > > > } > > Lets rename table_instance_remove() to imply it is freeing a flow. > hi Pravin, the function ovs_flow_free will free the flow actually. In > -ovs_flow_cmd_del > ovs_flow_tbl_remove > ... > ovs_flow_free > > In -table_instance_destroy > table_instance_remove > ovs_flow_free > > But if rename the table_instance_remove, table_instance_flow_free ? table_instance_flow_free() looks good.
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); }