[ovs-dev] [PATCH v5 9/9] ofproto-dpif-xlate: Translate timeout policy in ct action

Yi-Hung Wei yihung.wei at gmail.com
Wed Sep 25 23:34:40 UTC 2019


On Wed, Sep 25, 2019 at 3:59 PM Justin Pettit <jpettit at ovn.org> wrote:
>
>
> > On Aug 28, 2019, at 3:14 PM, Yi-Hung Wei <yihung.wei at gmail.com> wrote:
> >
> > diff --git a/lib/dpif-provider.h b/lib/dpif-provider.h
> > index e988626ea05b..4a599c8eb866 100644
> > --- a/lib/dpif-provider.h
> > +++ b/lib/dpif-provider.h
> > @@ -542,6 +542,16 @@ struct dpif_class {
> >                                        struct ct_dpif_timeout_policy *tp);
> >     int (*ct_timeout_policy_dump_done)(struct dpif *, void *state);
> >
> > +    /* Gets timeout policy based on 'tp_id', 'dl_type' and 'nw_proto'.
> > +     * On success, returns 0, stores the timeout policy name in 'tp_name',
> > +     * and sets 'is_generic'. 'is_generic' is false if the returned timeout
> > +     * policy in the 'dpif' is specific to 'dl_type' and 'nw_proto' in the
> > +     * datapath, i.e. in kernel datapath.  Sets 'is_generic' to true, if
> > +     * the timeout policy supports all OVS supported L3/L4 protocols. */
> > +    int (*ct_get_timeout_policy_name)(struct dpif *, uint32_t tp_id,
> > +                                      uint16_t dl_type, uint8_t nw_proto,
> > +                                      struct ds *tp_name, bool *is_generic);
>
> I don't have a great justification, but we've generally used "char **" instead of "struct ds" in these interfaces.  Let me know what you think of the attached incremental.  It ended up being a more invasive change than I expected, but I think it will be more consistent in our API.
>
> > diff --git a/ofproto/ofproto-dpif-xlate.c b/ofproto/ofproto-dpif-xlate.c
> > index 17800f3c8a3f..c8e508bca2c8 100644
> > --- a/ofproto/ofproto-dpif-xlate.c
> > +++ b/ofproto/ofproto-dpif-xlate.c
> > @@ -5983,6 +5983,25 @@ put_ct_helper(struct xlate_ctx *ctx,
> > }
> >
> > static void
> > +put_ct_timeout(struct ofpbuf *odp_actions, const struct dpif_backer *backer,
> > +               const struct flow *flow, struct flow_wildcards *wc,
> > +               uint16_t zone_id)
> > +{
> > +    struct ds tp_name = DS_EMPTY_INITIALIZER;
> > +    bool unwildcard;
> > +
> > +    if (ofproto_dpif_ct_zone_timeout_policy_get_name(backer, zone_id,
> > +            ntohs(flow->dl_type), flow->nw_proto, &tp_name, &unwildcard)) {
> > +        nl_msg_put_string(odp_actions, OVS_CT_ATTR_TIMEOUT, ds_cstr(&tp_name));
> > +
> > +        if (unwildcard) {
> > +                memset(&wc->masks.nw_proto, 0xff, sizeof wc->masks.nw_proto);
>
> I think it's worth mentioning again why we're unwildcarding "nw_proto" and why it wasn't necessary to also unwildcard "dl_type".  How about something like the following:
>
>             /* The underlying datapath requires separate timeout
>              * policies for different Ethertypes and IP protocols.  We
>              * don't need to unwildcard 'wc->masks.dl_type' since that
>              * field is always unwildcarded in megaflows. */
>             memset(&wc->masks.nw_proto, 0xff, sizeof wc->masks.nw_proto);
>
> Sorry for the long delay in finishing this review.  Assuming you're happy with the changes, I'll merge the series into master.
>
> Thanks,
>
> --Justin

Thanks Justin for the review.  The proposed diff looks good to me.

Thanks,

-Yi-Hung


More information about the dev mailing list