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.bal...@ericsson.com]
> Sent: Thursday, July 6, 2017 6:12 PM
> To: Yang, Yi Y <yi.y.y...@intel.com>; Eric Garver <e...@erig.me>
> Cc: 'd...@openvswitch.org' <d...@openvswitch.org>; Jan Scheurich
> <jan.scheur...@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.y...@intel.com]
> > Sent: Thursday, July 06, 2017 11:10 AM
> > To: Eric Garver <e...@erig.me>; Zoltán Balogh
> > <zoltan.bal...@ericsson.com>
> > Cc: 'd...@openvswitch.org' <d...@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...@erig.me]
> > Sent: Wednesday, July 5, 2017 11:14 PM
> > To: Zoltán Balogh <zoltan.bal...@ericsson.com>
> > Cc: Yang, Yi Y <yi.y.y...@intel.com>; 'd...@openvswitch.org'
> > <d...@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.bal...@ericsson.com>
> > > Reported-by: Eric Garver <e...@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
> > > d...@openvswitch.org
> > > https://mail.openvswitch.org/mailman/listinfo/ovs-dev
> _______________________________________________
> dev mailing list
> d...@openvswitch.org
> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
_______________________________________________
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
