Signed-off-by: Andy Zhou <[email protected]>
On Wed, Jun 17, 2015 at 10:41 AM, Jesse Gross <[email protected]> wrote: > Serializing between userspace flows and netlink attributes currently > requires several additional parameters besides the flows themselves. > This will continue to grow in the future as well. This converts > the function arguments to a parameters struct, which makes the code > easier to read and allowing irrelevant arguments to be omitted. > > Signed-off-by: Jesse Gross <[email protected]> > --- > lib/dpif-netdev.c | 24 ++++++++++++++------ > lib/odp-util.c | 53 > ++++++++++++++++--------------------------- > lib/odp-util.h | 31 ++++++++++++++++++++----- > lib/tnl-ports.c | 16 +++++++++---- > ofproto/ofproto-dpif-upcall.c | 17 +++++++++----- > ofproto/ofproto-dpif.c | 16 ++++++++++--- > tests/test-odp.c | 9 ++++++-- > 7 files changed, 103 insertions(+), 63 deletions(-) > > diff --git a/lib/dpif-netdev.c b/lib/dpif-netdev.c > index f13169c..e8ffcd7 100644 > --- a/lib/dpif-netdev.c > +++ b/lib/dpif-netdev.c > @@ -1815,22 +1815,27 @@ dp_netdev_flow_to_dpif_flow(const struct > dp_netdev_flow *netdev_flow, > struct flow_wildcards wc; > struct dp_netdev_actions *actions; > size_t offset; > + struct odp_flow_key_parms odp_parms = { > + .flow = &netdev_flow->flow, > + .mask = &wc.masks, > + .recirc = true, > + .max_mpls_depth = SIZE_MAX, > + }; > > miniflow_expand(&netdev_flow->cr.mask->mf, &wc.masks); > > /* Key */ > offset = key_buf->size; > flow->key = ofpbuf_tail(key_buf); > - odp_flow_key_from_flow(key_buf, &netdev_flow->flow, &wc.masks, > - netdev_flow->flow.in_port.odp_port, true); > + odp_parms.odp_in_port = netdev_flow->flow.in_port.odp_port; > + odp_flow_key_from_flow(&odp_parms, key_buf); > flow->key_len = key_buf->size - offset; > > /* Mask */ > offset = mask_buf->size; > flow->mask = ofpbuf_tail(mask_buf); > - odp_flow_key_from_mask(mask_buf, &wc.masks, &netdev_flow->flow, > - odp_to_u32(wc.masks.in_port.odp_port), > - SIZE_MAX, true); > + odp_parms.odp_in_port = wc.masks.in_port.odp_port; > + odp_flow_key_from_mask(&odp_parms, mask_buf); > flow->mask_len = mask_buf->size - offset; > > /* Actions */ > @@ -3008,10 +3013,15 @@ dp_netdev_upcall(struct dp_netdev_pmd_thread *pmd, > struct dp_packet *packet_, > struct ds ds = DS_EMPTY_INITIALIZER; > char *packet_str; > struct ofpbuf key; > + struct odp_flow_key_parms odp_parms = { > + .flow = flow, > + .mask = &wc->masks, > + .odp_in_port = flow->in_port.odp_port, > + .recirc = true, > + }; > > ofpbuf_init(&key, 0); > - odp_flow_key_from_flow(&key, flow, &wc->masks, > flow->in_port.odp_port, > - true); > + odp_flow_key_from_flow(&odp_parms, &key); > packet_str = ofp_packet_to_string(dp_packet_data(packet_), > dp_packet_size(packet_)); > > diff --git a/lib/odp-util.c b/lib/odp-util.c > index 76dc44b..56979ac 100644 > --- a/lib/odp-util.c > +++ b/lib/odp-util.c > @@ -3417,13 +3417,13 @@ static void get_tp_key(const struct flow *, union > ovs_key_tp *); > static void put_tp_key(const union ovs_key_tp *, struct flow *); > > static void > -odp_flow_key_from_flow__(struct ofpbuf *buf, const struct flow *flow, > - const struct flow *mask, odp_port_t odp_in_port, > - size_t max_mpls_depth, bool recirc, bool > export_mask) > +odp_flow_key_from_flow__(const struct odp_flow_key_parms *parms, > + bool export_mask, struct ofpbuf *buf) > { > struct ovs_key_ethernet *eth_key; > size_t encap; > - const struct flow *data = export_mask ? mask : flow; > + const struct flow *flow = parms->flow; > + const struct flow *data = export_mask ? parms->mask : parms->flow; > > nl_msg_put_u32(buf, OVS_KEY_ATTR_PRIORITY, data->skb_priority); > > @@ -3433,15 +3433,15 @@ odp_flow_key_from_flow__(struct ofpbuf *buf, const > struct flow *flow, > > nl_msg_put_u32(buf, OVS_KEY_ATTR_SKB_MARK, data->pkt_mark); > > - if (recirc) { > + if (parms->recirc) { > nl_msg_put_u32(buf, OVS_KEY_ATTR_RECIRC_ID, data->recirc_id); > nl_msg_put_u32(buf, OVS_KEY_ATTR_DP_HASH, data->dp_hash); > } > > /* Add an ingress port attribute if this is a mask or 'odp_in_port' > * is not the magical value "ODPP_NONE". */ > - if (export_mask || odp_in_port != ODPP_NONE) { > - nl_msg_put_odp_port(buf, OVS_KEY_ATTR_IN_PORT, odp_in_port); > + if (export_mask || parms->odp_in_port != ODPP_NONE) { > + nl_msg_put_odp_port(buf, OVS_KEY_ATTR_IN_PORT, parms->odp_in_port); > } > > eth_key = nl_msg_put_unspec_uninit(buf, OVS_KEY_ATTR_ETHERNET, > @@ -3507,7 +3507,9 @@ odp_flow_key_from_flow__(struct ofpbuf *buf, const > struct flow *flow, > int i, n; > > n = flow_count_mpls_labels(flow, NULL); > - n = MIN(n, max_mpls_depth); > + if (export_mask) { > + n = MIN(n, parms->max_mpls_depth); > + } > mpls_key = nl_msg_put_unspec_uninit(buf, OVS_KEY_ATTR_MPLS, > n * sizeof *mpls_key); > for (i = 0; i < n; i++) { > @@ -3579,43 +3581,26 @@ unencap: > } > > /* Appends a representation of 'flow' as OVS_KEY_ATTR_* attributes to 'buf'. > - * 'flow->in_port' is ignored (since it is likely to be an OpenFlow port > - * number rather than a datapath port number). Instead, if 'odp_in_port' > - * is anything other than ODPP_NONE, it is included in 'buf' as the input > - * port. > * > * 'buf' must have at least ODPUTIL_FLOW_KEY_BYTES bytes of space, or be > - * capable of being expanded to allow for that much space. > - * > - * 'recirc' indicates support for recirculation fields. If this is true, then > - * these fields will always be serialised. */ > + * capable of being expanded to allow for that much space. */ > void > -odp_flow_key_from_flow(struct ofpbuf *buf, const struct flow *flow, > - const struct flow *mask, odp_port_t odp_in_port, > - bool recirc) > +odp_flow_key_from_flow(const struct odp_flow_key_parms *parms, > + struct ofpbuf *buf) > { > - odp_flow_key_from_flow__(buf, flow, mask, odp_in_port, SIZE_MAX, recirc, > - false); > + odp_flow_key_from_flow__(parms, false, buf); > } > > /* Appends a representation of 'mask' as OVS_KEY_ATTR_* attributes to > - * 'buf'. 'flow' is used as a template to determine how to interpret > - * 'mask'. For example, the 'dl_type' of 'mask' describes the mask, but > - * it doesn't indicate whether the other fields should be interpreted as > - * ARP, IPv4, IPv6, etc. > + * 'buf'. > * > * 'buf' must have at least ODPUTIL_FLOW_KEY_BYTES bytes of space, or be > - * capable of being expanded to allow for that much space. > - * > - * 'recirc' indicates support for recirculation fields. If this is true, then > - * these fields will always be serialised. */ > + * capable of being expanded to allow for that much space. */ > void > -odp_flow_key_from_mask(struct ofpbuf *buf, const struct flow *mask, > - const struct flow *flow, uint32_t odp_in_port_mask, > - size_t max_mpls_depth, bool recirc) > +odp_flow_key_from_mask(const struct odp_flow_key_parms *parms, > + struct ofpbuf *buf) > { > - odp_flow_key_from_flow__(buf, flow, mask, u32_to_odp(odp_in_port_mask), > - max_mpls_depth, recirc, true); > + odp_flow_key_from_flow__(parms, true, buf); > } > > /* Generate ODP flow key from the given packet metadata */ > diff --git a/lib/odp-util.h b/lib/odp-util.h > index 4f0e794..59d29f3 100644 > --- a/lib/odp-util.h > +++ b/lib/odp-util.h > @@ -158,12 +158,31 @@ int odp_flow_from_string(const char *s, > const struct simap *port_names, > struct ofpbuf *, struct ofpbuf *); > > -void odp_flow_key_from_flow(struct ofpbuf *, const struct flow * flow, > - const struct flow *mask, odp_port_t odp_in_port, > - bool recirc); > -void odp_flow_key_from_mask(struct ofpbuf *, const struct flow *mask, > - const struct flow *flow, uint32_t odp_in_port, > - size_t max_mpls_depth, bool recirc); > +struct odp_flow_key_parms { > + /* The flow and mask to be serialized. In the case of masks, 'flow' > + * is used as a template to determine how to interpret 'mask'. For > + * example, the 'dl_type' of 'mask' describes the mask, but it doesn't > + * indicate whether the other fields should be interpreted as ARP, IPv4, > + * IPv6, etc. */ > + const struct flow *flow; > + const struct flow *mask; > + > + /* 'flow->in_port' is ignored (since it is likely to be an OpenFlow port > + * number rather than a datapath port number). Instead, if 'odp_in_port' > + * is anything other than ODPP_NONE, it is included in 'buf' as the input > + * port. */ > + odp_port_t odp_in_port; > + > + /* Indicates support for recirculation fields. If this is true, then > + * these fields will always be serialised. */ > + bool recirc; > + > + /* Only used for mask translation: */ > + size_t max_mpls_depth; > +}; > + > +void odp_flow_key_from_flow(const struct odp_flow_key_parms *, struct ofpbuf > *); > +void odp_flow_key_from_mask(const struct odp_flow_key_parms *, struct ofpbuf > *); > > uint32_t odp_flow_key_hash(const struct nlattr *, size_t); > > diff --git a/lib/tnl-ports.c b/lib/tnl-ports.c > index 2602db5..a0a73c8 100644 > --- a/lib/tnl-ports.c > +++ b/lib/tnl-ports.c > @@ -161,22 +161,28 @@ tnl_port_show(struct unixctl_conn *conn, int argc > OVS_UNUSED, > size_t key_len, mask_len; > struct flow_wildcards wc; > struct ofpbuf buf; > + struct odp_flow_key_parms odp_parms = { > + .flow = &flow, > + .mask = &wc.masks, > + }; > > ds_put_format(&ds, "%s (%"PRIu32") : ", p->dev_name, p->portno); > minimask_expand(&p->cr.match.mask, &wc); > miniflow_expand(&p->cr.match.flow, &flow); > > /* Key. */ > + odp_parms.odp_in_port = flow.in_port.odp_port; > + odp_parms.recirc = true; > ofpbuf_use_stack(&buf, &keybuf, sizeof keybuf); > - odp_flow_key_from_flow(&buf, &flow, &wc.masks, > - flow.in_port.odp_port, true); > + odp_flow_key_from_flow(&odp_parms, &buf); > key = buf.data; > key_len = buf.size; > + > /* mask*/ > + odp_parms.odp_in_port = wc.masks.in_port.odp_port; > + odp_parms.recirc = false; > ofpbuf_use_stack(&buf, &maskbuf, sizeof maskbuf); > - odp_flow_key_from_mask(&buf, &wc.masks, &flow, > - odp_to_u32(wc.masks.in_port.odp_port), > - SIZE_MAX, false); > + odp_flow_key_from_mask(&odp_parms, &buf); > mask = buf.data; > mask_len = buf.size; > > diff --git a/ofproto/ofproto-dpif-upcall.c b/ofproto/ofproto-dpif-upcall.c > index c39b571..f75bc69 100644 > --- a/ofproto/ofproto-dpif-upcall.c > +++ b/ofproto/ofproto-dpif-upcall.c > @@ -1363,6 +1363,10 @@ ukey_create_from_upcall(struct upcall *upcall) > struct odputil_keybuf keystub, maskstub; > struct ofpbuf keybuf, maskbuf; > bool recirc, megaflow; > + struct odp_flow_key_parms odp_parms = { > + .flow = upcall->flow, > + .mask = &upcall->xout.wc.masks, > + }; > > if (upcall->key_len) { > ofpbuf_use_const(&keybuf, upcall->key, upcall->key_len); > @@ -1370,19 +1374,20 @@ ukey_create_from_upcall(struct upcall *upcall) > /* dpif-netdev doesn't provide a netlink-formatted flow key in the > * upcall, so convert the upcall's flow here. */ > ofpbuf_use_stack(&keybuf, &keystub, sizeof keystub); > - odp_flow_key_from_flow(&keybuf, upcall->flow, &upcall->xout.wc.masks, > - upcall->flow->in_port.odp_port, true); > + odp_parms.odp_in_port = upcall->flow->in_port.odp_port; > + odp_parms.recirc = true; > + odp_flow_key_from_flow(&odp_parms, &keybuf); > } > > atomic_read_relaxed(&enable_megaflows, &megaflow); > recirc = ofproto_dpif_get_enable_recirc(upcall->ofproto); > ofpbuf_use_stack(&maskbuf, &maskstub, sizeof maskstub); > if (megaflow) { > - size_t max_mpls; > + odp_parms.odp_in_port = ODPP_NONE; > + odp_parms.max_mpls_depth = > ofproto_dpif_get_max_mpls_depth(upcall->ofproto); > + odp_parms.recirc = recirc; > > - max_mpls = ofproto_dpif_get_max_mpls_depth(upcall->ofproto); > - odp_flow_key_from_mask(&maskbuf, &upcall->xout.wc.masks, > upcall->flow, > - UINT32_MAX, max_mpls, recirc); > + odp_flow_key_from_mask(&odp_parms, &maskbuf); > } > > return ukey_create__(keybuf.data, keybuf.size, maskbuf.data, > maskbuf.size, > diff --git a/ofproto/ofproto-dpif.c b/ofproto/ofproto-dpif.c > index 369e0b9..0de8686 100644 > --- a/ofproto/ofproto-dpif.c > +++ b/ofproto/ofproto-dpif.c > @@ -1003,13 +1003,17 @@ check_recirc(struct dpif_backer *backer) > struct odputil_keybuf keybuf; > struct ofpbuf key; > bool enable_recirc; > + struct odp_flow_key_parms odp_parms = { > + .flow = &flow, > + .recirc = true, > + }; > > memset(&flow, 0, sizeof flow); > flow.recirc_id = 1; > flow.dp_hash = 1; > > ofpbuf_use_stack(&key, &keybuf, sizeof keybuf); > - odp_flow_key_from_flow(&key, &flow, NULL, 0, true); > + odp_flow_key_from_flow(&odp_parms, &key); > enable_recirc = dpif_probe_feature(backer->dpif, "recirculation", &key, > NULL); > > @@ -1037,12 +1041,15 @@ check_ufid(struct dpif_backer *backer) > struct ofpbuf key; > ovs_u128 ufid; > bool enable_ufid; > + struct odp_flow_key_parms odp_parms = { > + .flow = &flow, > + }; > > memset(&flow, 0, sizeof flow); > flow.dl_type = htons(0x1234); > > ofpbuf_use_stack(&key, &keybuf, sizeof keybuf); > - odp_flow_key_from_flow(&key, &flow, NULL, 0, true); > + odp_flow_key_from_flow(&odp_parms, &key); > dpif_flow_hash(backer->dpif, key.data, key.size, &ufid); > > enable_ufid = dpif_probe_feature(backer->dpif, "UFID", &key, &ufid); > @@ -1144,13 +1151,16 @@ check_max_mpls_depth(struct dpif_backer *backer) > for (n = 0; n < FLOW_MAX_MPLS_LABELS; n++) { > struct odputil_keybuf keybuf; > struct ofpbuf key; > + struct odp_flow_key_parms odp_parms = { > + .flow = &flow, > + }; > > memset(&flow, 0, sizeof flow); > flow.dl_type = htons(ETH_TYPE_MPLS); > flow_set_mpls_bos(&flow, n, 1); > > ofpbuf_use_stack(&key, &keybuf, sizeof keybuf); > - odp_flow_key_from_flow(&key, &flow, NULL, 0, false); > + odp_flow_key_from_flow(&odp_parms, &key); > if (!dpif_probe_feature(backer->dpif, "MPLS", &key, NULL)) { > break; > } > diff --git a/tests/test-odp.c b/tests/test-odp.c > index 4daf472..faa60d4 100644 > --- a/tests/test-odp.c > +++ b/tests/test-odp.c > @@ -54,6 +54,11 @@ parse_keys(bool wc_keys) > } > > if (!wc_keys) { > + struct odp_flow_key_parms odp_parms = { > + .flow = &flow, > + .recirc = true, > + }; > + > /* Convert odp_key to flow. */ > fitness = odp_flow_key_to_flow(odp_key.data, odp_key.size, > &flow); > switch (fitness) { > @@ -75,8 +80,8 @@ parse_keys(bool wc_keys) > /* Convert cls_rule back to odp_key. */ > ofpbuf_uninit(&odp_key); > ofpbuf_init(&odp_key, 0); > - odp_flow_key_from_flow(&odp_key, &flow, NULL, > - flow.in_port.odp_port, true); > + odp_parms.odp_in_port = flow.in_port.odp_port; > + odp_flow_key_from_flow(&odp_parms, &odp_key); > > if (odp_key.size > ODPUTIL_FLOW_KEY_BYTES) { > printf ("too long: %"PRIu32" > %d\n", > -- > 2.1.0 > > _______________________________________________ > dev mailing list > [email protected] > http://openvswitch.org/mailman/listinfo/dev _______________________________________________ dev mailing list [email protected] http://openvswitch.org/mailman/listinfo/dev
