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

Zoltán Balogh zoltan.balogh at ericsson.com
Thu Jul 6 13:14:31 UTC 2017


Hi Eric,

Thank you for clarifying. 

    ...
    [OVS_FLOW_ATTR_KEY]
        ...
        [OVS_KEY_ATTR_IPV4] = ...
        ...
    [OVS_KEY_ATTR_ETHERTYPE] = ...  <--- the one inserted

I have not found any API that would extend a nested attribute. 
Maybe I'm wrong, but I thought 'buf' holds a single nested attribute. If this is true, isn't possible to add the ethertype to it and increase its size?

  nl_msg_put_be16(buf, OVS_KEY_ATTR_ETHERTYPE, ns_type);
  buf->size += NL_A_U16_SIZE;

Or should I create a new nested attribute instead and copy the content of the original one plus the ethernet attribute? What would you propose?

diff --git a/lib/dpif-netlink.c b/lib/dpif-netlink.c
index cf4e98f29..98a97b3a2 100644
--- a/lib/dpif-netlink.c
+++ b/lib/dpif-netlink.c
@@ -3497,6 +3497,7 @@ put_convert_packet_type(struct ofpbuf *buf,
                     ovs_assert(nl_attr_get_be16(eth_type) == ns_type);
                 } else {
                     nl_msg_put_be16(buf, OVS_KEY_ATTR_ETHERTYPE, ns_type);
+                    buf->size += NL_A_U16_SIZE;
                 }
             }
         } else {
@@ -3514,15 +3515,14 @@ put_convert_packet_type(struct ofpbuf *buf,
                                                         mask_data, mask_len,
                                                         packet_type);
             /* Put OVS_KEY_ATTR_ETHERTYPE if needed. */
-            if (ntohl(value) != PT_ETH) {
+            if (!ethernet_present) {
                 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);
+                    buf->size += NL_A_U16_SIZE;
                 }
             }
         } else {


---

Best regards,
Zoltan

> -----Original Message-----
> From: Eric Garver [mailto:e at erig.me]
> Sent: Thursday, July 06, 2017 2:54 PM
> To: Yang, Yi Y <yi.y.yang at intel.com>
> Cc: Zoltán Balogh <zoltan.balogh at ericsson.com>; 'dev at openvswitch.org' <dev at openvswitch.org>
> Subject: Re: [ovs-dev] [PATCH v2] dpif-netlink: convert packet_type netlink attribute
> 
> On Thu, Jul 06, 2017 at 10:14:06AM +0000, Yang, Yi Y wrote:
> > I misunderstood Eric, he means we shouldn't have ETHERTYPE if packet_type is L3 type. I tried this one just now,
> it works.
> 
> No. That's not what I meant. We _need_ ETHERTYPE for L3-type (NS 1). The
> kernel expects this since it does not understand packet_type.
> 
> See my additional comments below.
> 
> >
> > diff --git a/lib/odp-util.c b/lib/odp-util.c
> > index 20373ec..2bf30bb 100644
> > --- a/lib/odp-util.c
> > +++ b/lib/odp-util.c
> > @@ -4883,7 +4883,9 @@ odp_flow_key_from_flow__(const struct odp_flow_key_parms *parms,
> >          goto unencap;
> >      }
> >
> > -    nl_msg_put_be16(buf, OVS_KEY_ATTR_ETHERTYPE, data->dl_type);
> > +    if (flow->packet_type == htonl(PT_ETH)) {
> > +        nl_msg_put_be16(buf, OVS_KEY_ATTR_ETHERTYPE, data->dl_type);
> > +    }
> >
> >      if (eth_type_vlan(flow->dl_type)) {
> >          goto unencap;
> >
> > -----Original Message-----
> > From: Zoltán Balogh [mailto:zoltan.balogh at ericsson.com]
> > Sent: Thursday, July 6, 2017 6:12 PM
> > To: Yang, Yi Y <yi.y.yang at intel.com>; Eric Garver <e at erig.me>
> > Cc: 'dev at openvswitch.org' <dev at openvswitch.org>; Jan Scheurich <jan.scheurich at ericsson.com>
> > Subject: RE: [ovs-dev] [PATCH v2] dpif-netlink: convert packet_type netlink attribute
> >
> > Hello Yi,
> >
> > I ran make check after applying your change and two tunnel tests did fail.
> >
> > ## ------------------------------ ##
> > ## openvswitch 2.7.90 test suite. ##
> > ## ------------------------------ ##
> >
> > 790: tunnel_push_pop_ipv6 - action                   FAILED (tunnel-push-pop-ipv6.at:170)
> > 787: tunnel_push_pop - action                        FAILED (tunnel-push-pop.at:221)
> >
> > The error message from testsuite.log:
> > +2017-07-06T10:06:32.119Z|00001|odp_util(revalidator37)|ERR|attribute
> > +eth has length 2 but should have length 12
> >
> > Best regards,
> > Zoltan
> >
> > > -----Original Message-----
> > > From: Yang, Yi Y [mailto:yi.y.yang at intel.com]
> > > Sent: Thursday, July 06, 2017 11:10 AM
> > > To: Eric Garver <e at erig.me>; Zoltán Balogh
> > > <zoltan.balogh at ericsson.com>
> > > Cc: 'dev at openvswitch.org' <dev at openvswitch.org>
> > > Subject: RE: [ovs-dev] [PATCH v2] dpif-netlink: convert packet_type
> > > netlink attribute
> > >
> > > 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.
> 
> To clarify, with this patch what you end up is essentially a message
> that looks like this:
> 
>     ...
>     [OVS_FLOW_ATTR_KEY]
>         ...
>         [OVS_KEY_ATTR_IPV4] = ...
>         ...
>     [OVS_KEY_ATTR_ETHERTYPE] = ...  <--- the one inserted
> 
> The added OVS_KEY_ATTR_ETHERTYPE is added at the top-level of the
> message. Instead it needs to be inside the
> OVS_FLOW_ATTR_KEY/OVS_FLOW_ATTR_MASK like below:
> 
>     ...
>     [OVS_FLOW_ATTR_KEY]
>         ...
>         [OVS_KEY_ATTR_IPV4] = ...
>         ...
>         [OVS_KEY_ATTR_ETHERTYPE] = ...  <--- the one inserted
> 
> 
> > >
> > > > +                }
> > > > +            }
> > > > +        } 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
> > _______________________________________________
> > dev mailing list
> > dev at openvswitch.org
> > https://mail.openvswitch.org/mailman/listinfo/ovs-dev


More information about the dev mailing list