From 5262eea1b88b99b71decfe944aea85ce01166a09 Mon Sep 17 00:00:00 2001 From: Jesse Gross Date: Tue, 16 Jun 2015 11:15:28 -0700 Subject: [PATCH] odp-util: Convert flow serialization parameters to a struct. 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 Signed-off-by: Andy Zhou --- 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 f13169c14..e8ffcd710 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 76dc44bae..56979ac34 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 4f0e79453..59d29f3ee 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 2602db543..a0a73c85d 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 c39b5712e..f75bc69fb 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 369e0b975..0de868693 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 4daf47287..faa60d472 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",