[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