On Wed, Sep 25, 2019 at 3:59 PM Justin Pettit <[email protected]> wrote: > > > > On Aug 28, 2019, at 3:14 PM, Yi-Hung Wei <[email protected]> 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 _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
