On Tue, Oct 22, 2019 at 7:35 PM Tonghao Zhang <xiangxia.m....@gmail.com> wrote: > > On Tue, Oct 22, 2019 at 2:58 PM Pravin Shelar <pshe...@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()? _______________________________________________ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev