Thank you for reviewing! I'm going to fix it. /Zoltan
> -----Original Message----- > From: Eric Garver [mailto:e...@erig.me] > Sent: Monday, July 03, 2017 8:52 PM > To: Zoltán Balogh <zoltan.bal...@ericsson.com> > Cc: 'd...@openvswitch.org' <d...@openvswitch.org> > Subject: Re: [ovs-dev] [PATCH] dpif-netilnk: convert packet_type netlink > attribute > > On Mon, Jul 03, 2017 at 02:27:18PM +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 | 34 ++++++++++++++++++++++++++++------ > > 1 file changed, 28 insertions(+), 6 deletions(-) > > > > diff --git a/lib/dpif-netlink.c b/lib/dpif-netlink.c > > index 562f6134c..d8051a09e 100644 > > --- a/lib/dpif-netlink.c > > +++ b/lib/dpif-netlink.c > > @@ -3434,11 +3434,11 @@ dpif_netlink_flow_from_ofpbuf(struct > > dpif_netlink_flow *flow, > > > > > > /* > > - * 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, > > +put_convert_packet_type(struct ofpbuf *buf, uint16_t type, > > const struct nlattr *data, uint16_t data_len) > > { > > const struct nlattr *packet_type; > > @@ -3446,8 +3446,9 @@ put_exclude_packet_type(struct ofpbuf *buf, uint16_t > > type, > > packet_type = nl_attr_find__(data, data_len, OVS_KEY_ATTR_PACKET_TYPE); > > > > if (packet_type) { > > - /* exclude PACKET_TYPE Netlink attribute. */ > > + /* Convert PACKET_TYPE Netlink attribute. */ > > ovs_assert(NLA_ALIGN(packet_type->nla_len) == NL_A_U32_SIZE); > > + ovs_be32 value = nl_attr_get_be32(packet_type); > > 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 > > @@ -3455,10 +3456,31 @@ put_exclude_packet_type(struct ofpbuf *buf, > > uint16_t type, > > 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); > > + > > + /* Put OVS_KEY_ATTR_ETHERTYPE if needed. */ > > + if (ntohl(value) == PT_ETH) { > > I don't think this works for the mask case. I think value will be > OVS_BE32_MAX. > > > + ovs_assert(ethernet_present); > > + } else { > > + const struct nlattr *eth_type; > > + ovs_be16 ns_type = pt_ns_type_be(value); > > + > > + 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 will have to put OVS_BE16_MAX for mask case. > > > + } > > + } > > + > > } else { > > nl_msg_put_unspec(buf, type, data, data_len); > > } > > @@ -3489,11 +3511,11 @@ dpif_netlink_flow_to_ofpbuf(const struct > > dpif_netlink_flow *flow, > > } > > if (!flow->ufid_terse || !flow->ufid_present) { > > if (flow->key_len) { > > - put_exclude_packet_type(buf, OVS_FLOW_ATTR_KEY, flow->key, > > + put_convert_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, > > + put_convert_packet_type(buf, OVS_FLOW_ATTR_MASK, flow->mask, > > flow->mask_len); > > } > > if (flow->actions || flow->actions_len) { > > -- > > 2.11.0 > > > > _______________________________________________ > > 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