I misunderstood Eric, he means we shouldn't have ETHERTYPE if packet_type is L3 type. I tried this one just now, it works.
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. > > > + } > > + } > > + } 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