[ovs-dev] [PATCH] odp-util: Convert flow serialization parameters to a struct.
Jesse Gross
jesse at nicira.com
Thu Jun 18 23:48:17 UTC 2015
Thanks, applied to master.
On Thu, Jun 18, 2015 at 4:26 PM, Andy Zhou <azhou at nicira.com> wrote:
> Signed-off-by: Andy Zhou <azhou at nicira.com>
>
> On Wed, Jun 17, 2015 at 10:41 AM, Jesse Gross <jesse at nicira.com> 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 <jesse at nicira.com>
>> ---
>> 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
>> dev at openvswitch.org
>> http://openvswitch.org/mailman/listinfo/dev
More information about the dev
mailing list