[ovs-dev] [PATCH] netdev-dpdk: Use single struct/union for flow offload items.
Ophir Munk
ophirmu at mellanox.com
Wed Feb 6 15:16:41 UTC 2019
Hi,
Please find comments inline.
> -----Original Message-----
> From: ovs-dev-bounces at openvswitch.org <ovs-dev-
> bounces at openvswitch.org> On Behalf Of Ilya Maximets
> Sent: Wednesday, February 6, 2019 3:19 PM
> To: ovs-dev at openvswitch.org; Ian Stokes <ian.stokes at intel.com>
> Cc: Ilya Maximets <i.maximets at samsung.com>; Roni Bar Yanai
> <roniba at mellanox.com>
> Subject: [ovs-dev] [PATCH] netdev-dpdk: Use single struct/union for flow
> offload items.
>
> Having a single structure allows to simplify the code path and clear all the
> items at once (probably faster). This does not increase stack memory usage
> because all the L4 related items grouped in a union.
>
> Changes:
> - Memsets combined.
> - 'ipv4_next_proto_mask' calculated at the top as we already know
> the address. We also do not need to check it before clearing.
> - Group of 'if' statements for L4 protocols turned to a 'switch'.
> We can do that, because we don't have semi-local variables anymore.
> - Eliminated 'end_proto_check' label. Not needed with 'switch'.
>
> Additionally 'rte_memcpy' replaced with simple 'memcpy' as it makes no
> sense to use 'rte_memcpy' for 6 bytes.
>
> Signed-off-by: Ilya Maximets <i.maximets at samsung.com>
> ---
> lib/netdev-dpdk.c | 190 +++++++++++++++++++---------------------------
> 1 file changed, 79 insertions(+), 111 deletions(-)
>
> diff --git a/lib/netdev-dpdk.c b/lib/netdev-dpdk.c index
> 26022a59c..79219f6ef 100644
> --- a/lib/netdev-dpdk.c
> +++ b/lib/netdev-dpdk.c
> @@ -4564,28 +4564,37 @@ netdev_dpdk_add_rte_flow_offload(struct
> netdev *netdev,
> struct flow_actions actions = { .actions = NULL, .cnt = 0 };
> struct rte_flow *flow;
> struct rte_flow_error error;
> - uint8_t *ipv4_next_proto_mask = NULL;
> + uint8_t proto = 0;
> int ret = 0;
> + struct flow_items {
> + struct rte_flow_item_eth eth;
> + struct rte_flow_item_vlan vlan;
> + struct rte_flow_item_ipv4 ipv4;
> + union {
> + struct rte_flow_item_tcp tcp;
> + struct rte_flow_item_udp udp;
> + struct rte_flow_item_sctp sctp;
> + struct rte_flow_item_icmp icmp;
> + };
> + } spec, mask;
> + uint8_t *ipv4_next_proto_mask = &mask.ipv4.hdr.next_proto_id;
> +
Please consider removing ipv4_next_porto_mask pointer assignment and later use directly mask.ipv4.hdr.next_proto_id.
> + memset(&spec, 0, sizeof spec);
> + memset(&mask, 0, sizeof mask);
For your consideration: we can save some memset() calls if we moved them inside the Eth case.
I like their current position for better readability.
>
> /* Eth */
> - struct rte_flow_item_eth eth_spec;
> - struct rte_flow_item_eth eth_mask;
> if (!eth_addr_is_zero(match->wc.masks.dl_src) ||
> !eth_addr_is_zero(match->wc.masks.dl_dst)) {
> - memset(ð_spec, 0, sizeof eth_spec);
> - memset(ð_mask, 0, sizeof eth_mask);
> - rte_memcpy(ð_spec.dst, &match->flow.dl_dst, sizeof
> eth_spec.dst);
> - rte_memcpy(ð_spec.src, &match->flow.dl_src, sizeof eth_spec.src);
> - eth_spec.type = match->flow.dl_type;
> -
> - rte_memcpy(ð_mask.dst, &match->wc.masks.dl_dst,
> - sizeof eth_mask.dst);
> - rte_memcpy(ð_mask.src, &match->wc.masks.dl_src,
> - sizeof eth_mask.src);
> - eth_mask.type = match->wc.masks.dl_type;
> + memcpy(&spec.eth.dst, &match->flow.dl_dst, sizeof spec.eth.dst);
> + memcpy(&spec.eth.src, &match->flow.dl_src, sizeof spec.eth.src);
> + spec.eth.type = match->flow.dl_type;
> +
> + memcpy(&mask.eth.dst, &match->wc.masks.dl_dst, sizeof
> mask.eth.dst);
> + memcpy(&mask.eth.src, &match->wc.masks.dl_src, sizeof
> mask.eth.src);
> + mask.eth.type = match->wc.masks.dl_type;
>
> add_flow_pattern(&patterns, RTE_FLOW_ITEM_TYPE_ETH,
> - ð_spec, ð_mask);
> + &spec.eth, &mask.eth);
> } else {
> /*
> * If user specifies a flow (like UDP flow) without L2 patterns, @@ -
> 4598,50 +4607,37 @@ netdev_dpdk_add_rte_flow_offload(struct netdev
> *netdev,
> }
>
> /* VLAN */
> - struct rte_flow_item_vlan vlan_spec;
> - struct rte_flow_item_vlan vlan_mask;
> if (match->wc.masks.vlans[0].tci && match->flow.vlans[0].tci) {
> - memset(&vlan_spec, 0, sizeof vlan_spec);
> - memset(&vlan_mask, 0, sizeof vlan_mask);
> - vlan_spec.tci = match->flow.vlans[0].tci & ~htons(VLAN_CFI);
> - vlan_mask.tci = match->wc.masks.vlans[0].tci & ~htons(VLAN_CFI);
> + spec.vlan.tci = match->flow.vlans[0].tci & ~htons(VLAN_CFI);
> + mask.vlan.tci = match->wc.masks.vlans[0].tci &
> + ~htons(VLAN_CFI);
>
> /* match any protocols */
> - vlan_mask.inner_type = 0;
> + mask.vlan.inner_type = 0;
>
> add_flow_pattern(&patterns, RTE_FLOW_ITEM_TYPE_VLAN,
> - &vlan_spec, &vlan_mask);
> + &spec.vlan, &mask.vlan);
> }
>
> /* IP v4 */
> - uint8_t proto = 0;
> - struct rte_flow_item_ipv4 ipv4_spec;
> - struct rte_flow_item_ipv4 ipv4_mask;
> if (match->flow.dl_type == htons(ETH_TYPE_IP)) {
> - memset(&ipv4_spec, 0, sizeof ipv4_spec);
> - memset(&ipv4_mask, 0, sizeof ipv4_mask);
> -
> - ipv4_spec.hdr.type_of_service = match->flow.nw_tos;
> - ipv4_spec.hdr.time_to_live = match->flow.nw_ttl;
> - ipv4_spec.hdr.next_proto_id = match->flow.nw_proto;
> - ipv4_spec.hdr.src_addr = match->flow.nw_src;
> - ipv4_spec.hdr.dst_addr = match->flow.nw_dst;
> -
> - ipv4_mask.hdr.type_of_service = match->wc.masks.nw_tos;
> - ipv4_mask.hdr.time_to_live = match->wc.masks.nw_ttl;
> - ipv4_mask.hdr.next_proto_id = match->wc.masks.nw_proto;
> - ipv4_mask.hdr.src_addr = match->wc.masks.nw_src;
> - ipv4_mask.hdr.dst_addr = match->wc.masks.nw_dst;
> + spec.ipv4.hdr.type_of_service = match->flow.nw_tos;
> + spec.ipv4.hdr.time_to_live = match->flow.nw_ttl;
> + spec.ipv4.hdr.next_proto_id = match->flow.nw_proto;
> + spec.ipv4.hdr.src_addr = match->flow.nw_src;
> + spec.ipv4.hdr.dst_addr = match->flow.nw_dst;
> +
> + mask.ipv4.hdr.type_of_service = match->wc.masks.nw_tos;
> + mask.ipv4.hdr.time_to_live = match->wc.masks.nw_ttl;
> + mask.ipv4.hdr.next_proto_id = match->wc.masks.nw_proto;
> + mask.ipv4.hdr.src_addr = match->wc.masks.nw_src;
> + mask.ipv4.hdr.dst_addr = match->wc.masks.nw_dst;
>
> add_flow_pattern(&patterns, RTE_FLOW_ITEM_TYPE_IPV4,
> - &ipv4_spec, &ipv4_mask);
> + &spec.ipv4, &mask.ipv4);
>
> /* Save proto for L4 protocol setup */
> - proto = ipv4_spec.hdr.next_proto_id &
> - ipv4_mask.hdr.next_proto_id;
> -
> - /* Remember proto mask address for later modification */
> - ipv4_next_proto_mask = &ipv4_mask.hdr.next_proto_id;
> + proto = spec.ipv4.hdr.next_proto_id &
> + mask.ipv4.hdr.next_proto_id;
> }
>
> if (proto != IPPROTO_ICMP && proto != IPPROTO_UDP && @@ -4660,96
> +4656,68 @@ netdev_dpdk_add_rte_flow_offload(struct netdev *netdev,
> goto out;
> }
>
> - struct rte_flow_item_tcp tcp_spec;
> - struct rte_flow_item_tcp tcp_mask;
> - if (proto == IPPROTO_TCP) {
> - memset(&tcp_spec, 0, sizeof tcp_spec);
> - memset(&tcp_mask, 0, sizeof tcp_mask);
> - tcp_spec.hdr.src_port = match->flow.tp_src;
> - tcp_spec.hdr.dst_port = match->flow.tp_dst;
> - tcp_spec.hdr.data_off = ntohs(match->flow.tcp_flags) >> 8;
> - tcp_spec.hdr.tcp_flags = ntohs(match->flow.tcp_flags) & 0xff;
> + switch (proto) {
> + case IPPROTO_TCP:
> + spec.tcp.hdr.src_port = match->flow.tp_src;
> + spec.tcp.hdr.dst_port = match->flow.tp_dst;
> + spec.tcp.hdr.data_off = ntohs(match->flow.tcp_flags) >> 8;
> + spec.tcp.hdr.tcp_flags = ntohs(match->flow.tcp_flags) & 0xff;
>
> - tcp_mask.hdr.src_port = match->wc.masks.tp_src;
> - tcp_mask.hdr.dst_port = match->wc.masks.tp_dst;
> - tcp_mask.hdr.data_off = ntohs(match->wc.masks.tcp_flags) >> 8;
> - tcp_mask.hdr.tcp_flags = ntohs(match->wc.masks.tcp_flags) & 0xff;
> + mask.tcp.hdr.src_port = match->wc.masks.tp_src;
> + mask.tcp.hdr.dst_port = match->wc.masks.tp_dst;
> + mask.tcp.hdr.data_off = ntohs(match->wc.masks.tcp_flags) >> 8;
> + mask.tcp.hdr.tcp_flags = ntohs(match->wc.masks.tcp_flags) &
> + 0xff;
>
> add_flow_pattern(&patterns, RTE_FLOW_ITEM_TYPE_TCP,
> - &tcp_spec, &tcp_mask);
> + &spec.tcp, &mask.tcp);
>
> /* proto == TCP and ITEM_TYPE_TCP, thus no need for proto match */
> - if (ipv4_next_proto_mask) {
> - *ipv4_next_proto_mask = 0;
> - }
> - goto end_proto_check;
> - }
> + *ipv4_next_proto_mask = 0;
Please consider:
mask.ipv4.hdr.next_proto_id = 0;
> + break;
>
> - struct rte_flow_item_udp udp_spec;
> - struct rte_flow_item_udp udp_mask;
> - if (proto == IPPROTO_UDP) {
> - memset(&udp_spec, 0, sizeof udp_spec);
> - memset(&udp_mask, 0, sizeof udp_mask);
> - udp_spec.hdr.src_port = match->flow.tp_src;
> - udp_spec.hdr.dst_port = match->flow.tp_dst;
> + case IPPROTO_UDP:
> + spec.udp.hdr.src_port = match->flow.tp_src;
> + spec.udp.hdr.dst_port = match->flow.tp_dst;
>
> - udp_mask.hdr.src_port = match->wc.masks.tp_src;
> - udp_mask.hdr.dst_port = match->wc.masks.tp_dst;
> + mask.udp.hdr.src_port = match->wc.masks.tp_src;
> + mask.udp.hdr.dst_port = match->wc.masks.tp_dst;
>
> add_flow_pattern(&patterns, RTE_FLOW_ITEM_TYPE_UDP,
> - &udp_spec, &udp_mask);
> + &spec.udp, &mask.udp);
>
> /* proto == UDP and ITEM_TYPE_UDP, thus no need for proto match */
> - if (ipv4_next_proto_mask) {
> - *ipv4_next_proto_mask = 0;
> - }
> - goto end_proto_check;
> - }
> + *ipv4_next_proto_mask = 0;
Dito
> + break;
>
> - struct rte_flow_item_sctp sctp_spec;
> - struct rte_flow_item_sctp sctp_mask;
> - if (proto == IPPROTO_SCTP) {
> - memset(&sctp_spec, 0, sizeof sctp_spec);
> - memset(&sctp_mask, 0, sizeof sctp_mask);
> - sctp_spec.hdr.src_port = match->flow.tp_src;
> - sctp_spec.hdr.dst_port = match->flow.tp_dst;
> + case IPPROTO_SCTP:
> + spec.sctp.hdr.src_port = match->flow.tp_src;
> + spec.sctp.hdr.dst_port = match->flow.tp_dst;
>
> - sctp_mask.hdr.src_port = match->wc.masks.tp_src;
> - sctp_mask.hdr.dst_port = match->wc.masks.tp_dst;
> + mask.sctp.hdr.src_port = match->wc.masks.tp_src;
> + mask.sctp.hdr.dst_port = match->wc.masks.tp_dst;
>
> add_flow_pattern(&patterns, RTE_FLOW_ITEM_TYPE_SCTP,
> - &sctp_spec, &sctp_mask);
> + &spec.sctp, &mask.sctp);
>
> /* proto == SCTP and ITEM_TYPE_SCTP, thus no need for proto match */
> - if (ipv4_next_proto_mask) {
> - *ipv4_next_proto_mask = 0;
> - }
> - goto end_proto_check;
> - }
> + *ipv4_next_proto_mask = 0;
Dito
> + break;
>
> - struct rte_flow_item_icmp icmp_spec;
> - struct rte_flow_item_icmp icmp_mask;
> - if (proto == IPPROTO_ICMP) {
> - memset(&icmp_spec, 0, sizeof icmp_spec);
> - memset(&icmp_mask, 0, sizeof icmp_mask);
> - icmp_spec.hdr.icmp_type = (uint8_t)ntohs(match->flow.tp_src);
> - icmp_spec.hdr.icmp_code = (uint8_t)ntohs(match->flow.tp_dst);
> + case IPPROTO_ICMP:
> + spec.icmp.hdr.icmp_type = (uint8_t) ntohs(match->flow.tp_src);
> + spec.icmp.hdr.icmp_code = (uint8_t) ntohs(match->flow.tp_dst);
>
> - icmp_mask.hdr.icmp_type = (uint8_t)ntohs(match->wc.masks.tp_src);
> - icmp_mask.hdr.icmp_code = (uint8_t)ntohs(match->wc.masks.tp_dst);
> + mask.icmp.hdr.icmp_type = (uint8_t) ntohs(match->wc.masks.tp_src);
> + mask.icmp.hdr.icmp_code = (uint8_t)
> + ntohs(match->wc.masks.tp_dst);
>
> add_flow_pattern(&patterns, RTE_FLOW_ITEM_TYPE_ICMP,
> - &icmp_spec, &icmp_mask);
> + &spec.icmp, &mask.icmp);
>
> /* proto == ICMP and ITEM_TYPE_ICMP, thus no need for proto match
> */
> - if (ipv4_next_proto_mask) {
> - *ipv4_next_proto_mask = 0;
> - }
> - goto end_proto_check;
> + *ipv4_next_proto_mask = 0;
Dito
> + break;
> }
>
> -end_proto_check:
> -
> add_flow_pattern(&patterns, RTE_FLOW_ITEM_TYPE_END, NULL, NULL);
>
> struct rte_flow_action_mark mark;
> --
> 2.17.1
>
> _______________________________________________
> dev mailing list
> dev at openvswitch.org
> https://emea01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fma
> il.openvswitch.org%2Fmailman%2Flistinfo%2Fovs-
> dev&data=02%7C01%7Cophirmu%40mellanox.com%7C6c046deb6e3c4c
> 2de10f08d68c35a8f5%7Ca652971c7d2e4d9ba6a4d149256f461b%7C0%7C0%7C
> 636850559431205667&sdata=opRCXKToPo9cAAgjf2P5Rld9ep1s849LTeml
> ReJQpmU%3D&reserved=0
More information about the dev
mailing list