Message ID | AM2PR07MB10422C717DFD78196A0A012D8AD70@AM2PR07MB1042.eurprd07.prod.outlook.com |
---|---|
State | Rejected |
Headers | show |
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@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 > dev@openvswitch.org > https://mail.openvswitch.org/mailman/listinfo/ovs-dev
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); + } + } + } 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); + } + } + } 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);
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@ericsson.com> Reported-by: Eric Garver <e@erig.me> --- lib/dpif-netlink.c | 120 +++++++++++++++++++++++++++++++++++++++-------------- 1 file changed, 90 insertions(+), 30 deletions(-)