Thx very much, applied to master, branch-2.3 with suggested changes.
On Tue, Jul 1, 2014 at 12:22 PM, Pravin Shelar <pshe...@nicira.com> wrote:
> On Tue, Jul 1, 2014 at 10:46 AM, Alex Wang <al...@nicira.com> wrote:
> > Due to the race condition in userspace, there is chance that two
> > overlapping megaflows could be installed in datapath. And this
> > causes userspace unable to delete the less inclusive megaflow flow
> > even after it timeout, since the flow_del logic will stop at the
> > first match of masked flow.
> >
> > This commit fixes the bug by making the kernel flow_del and flow_get
> > logic check all masks in that case.
> >
> > Signed-off-by: Alex Wang <al...@nicira.com>
> > Acked-by: Andy Zhou <az...@nicira.com>
> >
> > ---
> > PATCH -> V2:
> > - use exact lookup in flow_new and flow_set.
> > - remove the initialization to unused variable.
> > - change mask array iteration range to (0, count].
> > - use ovsl_dereference().
> > ---
> > datapath/datapath.c | 24 ++++++++++++------------
> > datapath/flow_table.c | 26 ++++++++++++++++++++++++--
> > datapath/flow_table.h | 2 ++
> > 3 files changed, 38 insertions(+), 14 deletions(-)
> >
> > diff --git a/datapath/datapath.c b/datapath/datapath.c
> > index a4d6473..ab63947 100644
> > --- a/datapath/datapath.c
> > +++ b/datapath/datapath.c
> > @@ -923,8 +923,12 @@ static int ovs_flow_cmd_new(struct sk_buff *skb,
> struct genl_info *info)
> > }
> > /* The unmasked key has to be the same for flow updates.
> */
> > if (unlikely(!ovs_flow_cmp_unmasked_key(flow, &match))) {
> > - error = -EEXIST;
> > - goto err_unlock_ovs;
> > + /* Look for any overlapping flow. */
> > + flow = ovs_flow_tbl_lookup_exact(&dp->table,
> &match);
> > + if (!flow) {
> > + error = -ENOENT;
> > + goto err_unlock_ovs;
> > + }
> > }
> > /* Update actions. */
> > old_acts = ovsl_dereference(flow->sf_acts);
> > @@ -1015,16 +1019,12 @@ static int ovs_flow_cmd_set(struct sk_buff *skb,
> struct genl_info *info)
> > goto err_unlock_ovs;
> > }
> > /* Check that the flow exists. */
> > - flow = ovs_flow_tbl_lookup(&dp->table, &key);
> > + flow = ovs_flow_tbl_lookup_exact(&dp->table, &match);
> > if (unlikely(!flow)) {
> > error = -ENOENT;
> > goto err_unlock_ovs;
> > }
> > - /* The unmasked key has to be the same for flow updates. */
> > - if (unlikely(!ovs_flow_cmp_unmasked_key(flow, &match))) {
> > - error = -EEXIST;
> > - goto err_unlock_ovs;
> > - }
> > +
> > /* Update actions, if present. */
> > if (likely(acts)) {
> > old_acts = ovsl_dereference(flow->sf_acts);
> > @@ -1096,8 +1096,8 @@ static int ovs_flow_cmd_get(struct sk_buff *skb,
> struct genl_info *info)
> > goto unlock;
> > }
> >
> > - flow = ovs_flow_tbl_lookup(&dp->table, &key);
> > - if (!flow || !ovs_flow_cmp_unmasked_key(flow, &match)) {
> > + flow = ovs_flow_tbl_lookup_exact(&dp->table, &match);
> > + if (!flow) {
> > err = -ENOENT;
> > goto unlock;
> > }
> > @@ -1144,8 +1144,8 @@ static int ovs_flow_cmd_del(struct sk_buff *skb,
> struct genl_info *info)
> > err = ovs_flow_tbl_flush(&dp->table);
> > goto unlock;
> > }
> > - flow = ovs_flow_tbl_lookup(&dp->table, &key);
> > - if (unlikely(!flow || !ovs_flow_cmp_unmasked_key(flow, &match)))
> {
> > + flow = ovs_flow_tbl_lookup_exact(&dp->table, &match);
> > + if (unlikely(!flow)) {
> > err = -ENOENT;
> > goto unlock;
> > }
> > diff --git a/datapath/flow_table.c b/datapath/flow_table.c
> > index 58a25c7..f54cb8e 100644
> > --- a/datapath/flow_table.c
> > +++ b/datapath/flow_table.c
> > @@ -513,7 +513,6 @@ static struct sw_flow *masked_flow_lookup(struct
> table_instance *ti,
> > return NULL;
> > }
> >
> > -
> > static struct sw_flow *flow_lookup(struct flow_table *tbl,
> > struct table_instance *ti,
> > struct mask_array *ma,
> > @@ -614,10 +613,33 @@ struct sw_flow *ovs_flow_tbl_lookup(struct
> flow_table *tbl,
> > u32 __always_unused n_mask_hit;
> > u32 __always_unused index;
> >
> > - n_mask_hit = 0;
> > return flow_lookup(tbl, ti, ma, key, &n_mask_hit, &index);
> > }
> >
> > +struct sw_flow *ovs_flow_tbl_lookup_exact(struct flow_table *tbl,
> > + struct sw_flow_match *match)
> > +{
> > + struct table_instance *ti = rcu_dereference_ovsl(tbl->ti);
> > + struct mask_array *ma = rcu_dereference_ovsl(tbl->mask_array);
> > + struct sw_flow *flow;
> > + u32 __always_unused n_mask_hit;
> > + int i;
> > +
> > + /* Always called under ovs-mutex. */
> > + for (i = 0; i <= ma->max; i++) {
> You can use count rather than max.
>
> > + struct sw_flow_mask *mask;
> > +
> > + mask = ovsl_dereference(ma->masks[i]);
> > + if (mask) {
> > + flow = masked_flow_lookup(ti, match->key, mask,
> &n_mask_hit);
> > + if (flow && ovs_flow_cmp_unmasked_key(flow,
> match)) { /* Found */
> > + return flow;
> > + }
> > + }
> > + }
> > + return NULL;
> > +}
> > +
> > int ovs_flow_tbl_num_masks(const struct flow_table *table)
> > {
> > struct mask_array *ma;
> > diff --git a/datapath/flow_table.h b/datapath/flow_table.h
> > index ee86953..a05d36a 100644
> > --- a/datapath/flow_table.h
> > +++ b/datapath/flow_table.h
> > @@ -89,6 +89,8 @@ struct sw_flow *ovs_flow_tbl_lookup_stats(struct
> flow_table *,
> > u32 *n_mask_hit);
> > struct sw_flow *ovs_flow_tbl_lookup(struct flow_table *,
> > const struct sw_flow_key *);
> > +struct sw_flow *ovs_flow_tbl_lookup_exact(struct flow_table *,
> > + struct sw_flow_match *match);
> >
> > bool ovs_flow_cmp_unmasked_key(const struct sw_flow *flow,
> > struct sw_flow_match *match);
> Acked-by: Pravin B Shelar <pshe...@nicira.com>
>
> > --
> > 1.7.9.5
> >
>
_______________________________________________
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
