Thanks, applied to master.
On Thu, Jun 18, 2015 at 4:26 PM, Andy Zhou <[email protected]> wrote: > 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
