On 19.02.2019 13:45, Asaf Penso wrote: > Looks good, I would consider adding a default case for the switch with an > appropriate message.
There is an 'if' statement a few lines above the 'switch' that checks for unsupported protocols. And we should not print any warnings in case of unsupported protocol, if matching on it is not requested (i.e. all the fields wildcarded). > > Regards, > Asaf Penso > >> -----Original Message----- >> From: Ilya Maximets <[email protected]> >> Sent: Wednesday, February 6, 2019 5:41 PM >> To: [email protected]; Ian Stokes <[email protected]> >> Cc: Asaf Penso <[email protected]>; Roni Bar Yanai >> <[email protected]>; Ophir Munk <[email protected]>; Ilya >> Maximets <[email protected]> >> Subject: [PATCH v2] 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' dropped as we already know the address >> and able to use 'mask.ipv4.hdr.next_proto_id' directly. >> - 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 <[email protected]> >> --- >> >> Version 2: >> * Dropped 'ipv4_next_proto_mask' pointer as we able to use >> 'mask.ipv4.hdr.next_proto_id' directly. >> >> lib/netdev-dpdk.c | 189 +++++++++++++++++++--------------------------- >> 1 file changed, 78 insertions(+), 111 deletions(-) >> >> diff --git a/lib/netdev-dpdk.c b/lib/netdev-dpdk.c index >> 26022a59c..d18dd1b6c 100644 >> --- a/lib/netdev-dpdk.c >> +++ b/lib/netdev-dpdk.c >> @@ -4564,28 +4564,36 @@ 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; >> + >> + memset(&spec, 0, sizeof spec); >> + memset(&mask, 0, sizeof mask); >> >> /* 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 +4606,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 >> +4655,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; >> - } >> + 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; >> - } >> + mask.ipv4.hdr.next_proto_id = 0; >> + 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; >> - } >> + mask.ipv4.hdr.next_proto_id = 0; >> + 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; >> + mask.ipv4.hdr.next_proto_id = 0; >> + 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 [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
