On Sat, Nov 18, 2017 at 02:23:22AM -0200, Flavio Leitner wrote:
> On Thu, 26 Oct 2017 15:30:42 -0400
> Eric Garver <e...@erig.me> wrote:
>
> > This supports using the ct_clear action in the kernel datapath. To
> > preserve compatibility with current ct_clear behavior on old kernels, we
> > only pass this action down to the datapath if a probe reveals the
> > datapath actually supports it.
> >
> > Signed-off-by: Eric Garver <e...@erig.me>
> > Acked-by: William Tu <u9012...@gmail.com>
> > ---
[...]
> > diff --git a/datapath/linux/compat/include/linux/openvswitch.h
> > b/datapath/linux/compat/include/linux/openvswitch.h
> > index bc6c94b8d52d..28f20103af81 100644
> > --- a/datapath/linux/compat/include/linux/openvswitch.h
> > +++ b/datapath/linux/compat/include/linux/openvswitch.h
> > @@ -924,6 +924,7 @@ enum ovs_action_attr {
> > OVS_ACTION_ATTR_TRUNC, /* u32 struct ovs_action_trunc. */
> > OVS_ACTION_ATTR_PUSH_ETH, /* struct ovs_action_push_eth. */
> > OVS_ACTION_ATTR_POP_ETH, /* No argument. */
> > + OVS_ACTION_ATTR_CT_CLEAR, /* No argument. */
>
> Missing to update the comment above the definition.
Right. Thanks.
[...]
> > --- a/lib/odp-util.c
> > +++ b/lib/odp-util.c
> > @@ -126,6 +126,7 @@ odp_action_len(uint16_t type)
> > case OVS_ACTION_ATTR_SET_MASKED: return ATTR_LEN_VARIABLE;
> > case OVS_ACTION_ATTR_SAMPLE: return ATTR_LEN_VARIABLE;
> > case OVS_ACTION_ATTR_CT: return ATTR_LEN_VARIABLE;
> > + case OVS_ACTION_ATTR_CT_CLEAR: return 0;
> > case OVS_ACTION_ATTR_PUSH_ETH: return sizeof(struct
> > ovs_action_push_eth);
> > case OVS_ACTION_ATTR_POP_ETH: return 0;
> > case OVS_ACTION_ATTR_CLONE: return ATTR_LEN_VARIABLE;
> > @@ -1054,6 +1055,9 @@ format_odp_action(struct ds *ds, const struct nlattr
> > *a,
> > case OVS_ACTION_ATTR_CT:
> > format_odp_conntrack_action(ds, a);
> > break;
> > + case OVS_ACTION_ATTR_CT_CLEAR:
> > + ds_put_cstr(ds, "ct_clear");
> > + break;
> > case OVS_ACTION_ATTR_CLONE:
> > format_odp_clone_action(ds, a, portno_names);
> > break;
>
> Aren't we missing parse_odp_action()?
Indeed. Thanks for pointing that out. I also updated the "OVS datapath
actions parsing and formatting - valid forms" test case, which makes
this blatantly obvious. :)
[...]
> > diff --git a/ofproto/ofproto-dpif.c b/ofproto/ofproto-dpif.c
> > index 43d670a15c3f..88fd9d5c8c8f 100644
> > --- a/ofproto/ofproto-dpif.c
> > +++ b/ofproto/ofproto-dpif.c
> > @@ -1322,6 +1322,37 @@ check_ct_eventmask(struct dpif_backer *backer)
> > return !error;
> > }
> >
> > +static bool
> > +check_ct_clear(struct dpif_backer *backer)
> > +{
> > + struct odputil_keybuf keybuf;
> > + uint8_t actbuf[NL_A_FLAG_SIZE];
> > + struct ofpbuf actions;
> > + struct ofpbuf key;
> > + struct flow flow;
> > + bool supported;
> > +
> > + struct odp_flow_key_parms odp_parms = {
> > + .flow = &flow,
> > + .probe = true,
> > + };
> > +
> > + memset(&flow, 0, sizeof flow);
> > + ofpbuf_use_stack(&key, &keybuf, sizeof keybuf);
> > + odp_flow_key_from_flow(&odp_parms, &key);
> > +
> > + ofpbuf_use_stack(&actions, &actbuf, sizeof actbuf);
> > + nl_msg_put_flag(&actions, OVS_ACTION_ATTR_CT_CLEAR);
> > +
> > + supported = dpif_probe_feature(backer->dpif, "ct_clear", &key,
> > + &actions, NULL);
> > +
> > + VLOG_INFO("%s: Datapath %s ct_clear action",
> > + dpif_name(backer->dpif), (supported) ? "supports"
> > + : "does not support");
> > + return supported;
> > +}
> > +
> > #define CHECK_FEATURE__(NAME, SUPPORT, FIELD, VALUE, ETHTYPE)
> > \
> > static bool
> > \
> > check_##NAME(struct dpif_backer *backer)
> > \
> > @@ -1386,6 +1417,7 @@ check_support(struct dpif_backer *backer)
> > backer->rt_support.clone = check_clone(backer);
> > backer->rt_support.sample_nesting = check_max_sample_nesting(backer);
> > backer->rt_support.ct_eventmask = check_ct_eventmask(backer);
> > + backer->rt_support.ct_clear = check_ct_clear(backer);
> >
> > /* Flow fields. */
> > backer->rt_support.odp.ct_state = check_ct_state(backer);
> > diff --git a/ofproto/ofproto-dpif.h b/ofproto/ofproto-dpif.h
> > index 0857c070c8ac..8752061d6439 100644
> > --- a/ofproto/ofproto-dpif.h
> > +++ b/ofproto/ofproto-dpif.h
> > @@ -178,7 +178,10 @@ struct group_dpif *group_dpif_lookup(struct
> > ofproto_dpif *,
> > DPIF_SUPPORT_FIELD(size_t, sample_nesting, "Sample nesting")
> > \
> >
> > \
> > /* OVS_CT_ATTR_EVENTMASK supported by OVS_ACTION_ATTR_CT action. */
> > \
> > - DPIF_SUPPORT_FIELD(bool, ct_eventmask, "Conntrack eventmask")
> > + DPIF_SUPPORT_FIELD(bool, ct_eventmask, "Conntrack eventmask")
> > \
> > +
> > \
> > + /* True if the datapath supports OVS_ACTION_ATTR_CT_CLEAR action. */
> > \
> > + DPIF_SUPPORT_FIELD(bool, ct_clear, "Conntrack clear")
> >
> > /* Stores the various features which the corresponding backer supports. */
> > struct dpif_backer_support {
>
> Perhaps mention this change in NEWS just in case?
Good idea. I'll add something.
Thanks for reviewing Flavio!
_______________________________________________
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
