[ovs-dev] [PATCH v2] dpif-netlink: convert packet_type netlink attribute

Yang, Yi Y yi.y.yang at intel.com
Thu Jul 6 09:10:24 UTC 2017


Eric, maybe the below patch can fix your issue, please give a try.

diff --git a/lib/odp-util.c b/lib/odp-util.c
index 20373ec..c5f714d 100644
--- a/lib/odp-util.c
+++ b/lib/odp-util.c
@@ -4863,6 +4863,9 @@ odp_flow_key_from_flow__(const struct odp_flow_key_parms *parms,
                 goto unencap;
             }
         }
+    } else {
+        if (export_mask) {
+            nl_msg_put_be16(buf, OVS_KEY_ATTR_ETHERTYPE, OVS_BE16_MAX);
     }

     if (ntohs(flow->dl_type) < ETH_TYPE_MIN) {

-----Original Message-----
From: Eric Garver [mailto:e at erig.me] 
Sent: Wednesday, July 5, 2017 11:14 PM
To: Zoltán Balogh <zoltan.balogh at ericsson.com>
Cc: Yang, Yi Y <yi.y.yang at intel.com>; 'dev at openvswitch.org' <dev at openvswitch.org>
Subject: Re: [ovs-dev] [PATCH v2] dpif-netlink: convert packet_type netlink attribute

Hi Zoltan,

On Tue, Jul 04, 2017 at 09:23:17PM +0000, Zoltán Balogh wrote:
> By introducing packet type-aware pipeline, match on ethertype was 
> removed when packet type is not Ethernet. As pointed out by Eric 
> Garver, this could have a side effect on the kernel datapath:
> https://patchwork.ozlabs.org/patch/782991/
> 
> This patch does approach the problem from a different perspective.
> Instead of re-introducing the unconditional match on Ethernet type in 
> all megaflow entries, as suggested by Eric, mapping of packet_type != 
> PT_ETH to dl_type could be handled in the put_exclude_packet_type() in dpif-netlink.c.
> 
> Put_exclude_packet_type() filters the packet_type netlink attribute 
> out, before all attributes are sent from userspace to kernel. This 
> commit modifies the put_exclude_packet_type() function to do a proper 
> conversation and add the missing OVS_KEY_ATTR_ETHERTYPE attribute if it's needed.
> 
> Signed-off-by: Zoltán Balogh <zoltan.balogh at ericsson.com>
> Reported-by: Eric Garver <e at erig.me>
> ---
>  lib/dpif-netlink.c | 120 
> +++++++++++++++++++++++++++++++++++++++--------------
>  1 file changed, 90 insertions(+), 30 deletions(-)
> 
> diff --git a/lib/dpif-netlink.c b/lib/dpif-netlink.c index 
> 562f613..cf4e98f 100644
> --- a/lib/dpif-netlink.c
> +++ b/lib/dpif-netlink.c
> @@ -3433,34 +3433,101 @@ dpif_netlink_flow_from_ofpbuf(struct 
> dpif_netlink_flow *flow,  }
>  
>  
> +static bool
> +put_exclude_packet_type(struct ofpbuf *buf, uint16_t type,
> +                        const struct nlattr *data, uint16_t data_len,
> +                        const struct nlattr *packet_type) {
> +    ovs_assert(NLA_ALIGN(packet_type->nla_len) == NL_A_U32_SIZE);
> +    size_t packet_type_len = NL_A_U32_SIZE;
> +    size_t first_chunk_size = (uint8_t *)packet_type - (uint8_t *)data;
> +    size_t second_chunk_size = data_len - first_chunk_size
> +                               - packet_type_len;
> +    uint8_t *first_attr = NULL;
> +    struct nlattr *next_attr = nl_attr_next(packet_type);
> +
> +    bool ethernet_present = nl_attr_find__(data, data_len,
> +                                           OVS_KEY_ATTR_ETHERNET);
> +
> +    first_attr = nl_msg_put_unspec_uninit(buf, type,
> +                                          data_len - packet_type_len);
> +    memcpy(first_attr, data, first_chunk_size);
> +    memcpy(first_attr + first_chunk_size, next_attr, 
> + second_chunk_size);
> +
> +    return ethernet_present;
> +}
> +
>  /*
> - * If PACKET_TYPE attribute is present in 'data', it filters 
> PACKET_TYPE out,
> - * then puts 'data' to 'buf'.
> + * If PACKET_TYPE attribute is present in 'data', converts it to 
> + ETHERNET and
> + * ETHERTYPE attributes, then puts 'data' to 'buf'.
>   */
>  static void
> -put_exclude_packet_type(struct ofpbuf *buf, uint16_t type,
> -                        const struct nlattr *data, uint16_t data_len)
> +put_convert_packet_type(struct ofpbuf *buf,
> +                        const struct dpif_netlink_flow *flow)
>  {
>      const struct nlattr *packet_type;
> +    const struct nlattr *data = flow->key;
> +    uint16_t data_len = flow->key_len;
> +    const struct nlattr *mask_data = flow->mask;
> +    uint16_t mask_len = flow->mask_len;
> +    ovs_be32 value = htonl(PT_ETH);
> +    bool ethernet_present = false;
> +
> +    /* Verify KEY attributes.  */
> +    if (data_len) {
> +        packet_type = nl_attr_find__(data, data_len, OVS_KEY_ATTR_PACKET_TYPE);
> +        if (packet_type) {
> +            /* Convert PACKET_TYPE Netlink attribute. */
> +            value = nl_attr_get_be32(packet_type);
> +            ethernet_present = put_exclude_packet_type(buf, OVS_FLOW_ATTR_KEY,
> +                                                       data, data_len,
> +                                                       packet_type);
> +            /* Put OVS_KEY_ATTR_ETHERTYPE if needed. */
> +            if (ntohl(value) == PT_ETH) {
> +                ovs_assert(ethernet_present);
> +            } else {
> +                const struct nlattr *eth_type;
> +                ovs_be16 ns_type = pt_ns_type_be(value);
>  
> -    packet_type = nl_attr_find__(data, data_len, OVS_KEY_ATTR_PACKET_TYPE);
> -
> -    if (packet_type) {
> -        /* exclude PACKET_TYPE Netlink attribute. */
> -        ovs_assert(NLA_ALIGN(packet_type->nla_len) == NL_A_U32_SIZE);
> -        size_t packet_type_len = NL_A_U32_SIZE;
> -        size_t first_chunk_size = (uint8_t *)packet_type - (uint8_t *)data;
> -        size_t second_chunk_size = data_len - first_chunk_size
> -                                   - packet_type_len;
> -        uint8_t *first_attr = NULL;
> -        struct nlattr *next_attr = nl_attr_next(packet_type);
> -
> -        first_attr = nl_msg_put_unspec_uninit(buf, type,
> -                                              data_len - packet_type_len);
> -        memcpy(first_attr, data, first_chunk_size);
> -        memcpy(first_attr + first_chunk_size, next_attr, second_chunk_size);
> -    } else {
> -        nl_msg_put_unspec(buf, type, data, data_len);
> +                ovs_assert(ethernet_present == false);
> +
> +                eth_type = nl_attr_find__(data, data_len,
> +                                          OVS_KEY_ATTR_ETHERTYPE);
> +                if (eth_type) {
> +                    ovs_assert(nl_attr_get_be16(eth_type) == ns_type);
> +                } else {
> +                    nl_msg_put_be16(buf, OVS_KEY_ATTR_ETHERTYPE, 
> + ns_type);

This inserts OVS_KEY_ATTR_ETHERTYPE at the top level of the message. It needs to be inserted _inside_ the nested OVS_FLOW_ATTR_KEY.

> +                }
> +            }
> +        } else {
> +            nl_msg_put_unspec(buf, OVS_FLOW_ATTR_KEY, data, data_len);
> +        }
> +    }
> +
> +    /* Verify MASK attributes. */
> +    if (mask_len) {
> +        packet_type = nl_attr_find__(mask_data, mask_len,
> +                                     OVS_KEY_ATTR_PACKET_TYPE);
> +        if (packet_type) {
> +            /* Convert PACKET_TYPE Netlink attribute. */
> +            ethernet_present = put_exclude_packet_type(buf, OVS_FLOW_ATTR_MASK,
> +                                                       mask_data, mask_len,
> +                                                       packet_type);
> +            /* Put OVS_KEY_ATTR_ETHERTYPE if needed. */
> +            if (ntohl(value) != PT_ETH) {
> +                const struct nlattr *eth_type;
> +
> +                ovs_assert(ethernet_present == false);
> +
> +                eth_type = nl_attr_find__(mask_data, mask_len,
> +                                          OVS_KEY_ATTR_ETHERTYPE);
> +                if (!eth_type) {
> +                    nl_msg_put_be16(buf, OVS_KEY_ATTR_ETHERTYPE, 
> + OVS_BE16_MAX);

Same as above.

> +                }
> +            }
> +        } else {
> +            nl_msg_put_unspec(buf, OVS_FLOW_ATTR_MASK, mask_data, mask_len);
> +        }
>      }
>  }
>  
> @@ -3488,14 +3555,7 @@ dpif_netlink_flow_to_ofpbuf(const struct dpif_netlink_flow *flow,
>                         | OVS_UFID_F_OMIT_ACTIONS);
>      }
>      if (!flow->ufid_terse || !flow->ufid_present) {
> -        if (flow->key_len) {
> -            put_exclude_packet_type(buf, OVS_FLOW_ATTR_KEY, flow->key,
> -                                           flow->key_len);
> -        }
> -        if (flow->mask_len) {
> -            put_exclude_packet_type(buf, OVS_FLOW_ATTR_MASK, flow->mask,
> -                                           flow->mask_len);
> -        }
> +        put_convert_packet_type(buf, flow);
>          if (flow->actions || flow->actions_len) {
>              nl_msg_put_unspec(buf, OVS_FLOW_ATTR_ACTIONS,
>                                flow->actions, flow->actions_len);
> --
> 1.9.1
> 
> _______________________________________________
> dev mailing list
> dev at openvswitch.org
> https://mail.openvswitch.org/mailman/listinfo/ovs-dev


More information about the dev mailing list