On 04.02.2019 18:50, Asaf Penso wrote: > > > Regards, > Asaf Penso > >> -----Original Message----- >> From: Ilya Maximets <[email protected]> >> Sent: Monday, February 4, 2019 5:41 PM >> To: Asaf Penso <[email protected]>; [email protected] >> Cc: Roni Bar Yanai <[email protected]>; Stokes, Ian >> <[email protected]> >> Subject: Re: [ovs-dev] netdev-dpdk: Memset rte_flow_item on a need basis. >> >> On 04.02.2019 18:08, Asaf Penso wrote: >>> >>> >>> Regards, >>> Asaf Penso >>> >>>> -----Original Message----- >>>> From: Ilya Maximets <[email protected]> >>>> Sent: Monday, February 4, 2019 4:14 PM >>>> To: Asaf Penso <[email protected]>; [email protected] >>>> Cc: Roni Bar Yanai <[email protected]>; Stokes, Ian >>>> <[email protected]> >>>> Subject: Re: [ovs-dev] netdev-dpdk: Memset rte_flow_item on a need >> basis. >>>> >>>> On 04.02.2019 15:50, Asaf Penso wrote: >>>>> In netdev_dpdk_add_rte_flow_offload function different >> rte_flow_item >>>>> are created as part of the pattern matching. >>>>> >>>>> For most of them, there is a check whether the wildcards are not zero. >>>>> In case of zero, nothing is being done with the rte_flow_item. >>>>> >>>>> Befor the wildcard check, and regardless of the result, the >>>>> rte_flow_item is being memset. >>>>> >>>>> The patch moves the memset function only if the condition of the >>>>> wildcard is true, thus saving cycles of memset if not needed. >>>> >>>> Structures are actually used only inside their 'if' blocks. >>>> IMHO, it's better to move the definitions inside too. >>>> >>> >>> Inside each 'if' block there is a call to add_flow_pattern that updates the >> pattern's pointers to these structs. >>> Having them defined inside the block will cause their value to be corrupted >> once we exit the block. >>> They are needed all the way until rte_flow_create call. >> >> Oh. Sorry. You're right. I forget about that. >> >> BTW, as you touching this code, maybe you could make it a bit more coding- >> style compliant ? It's about sizeof operator. Coding-style asks to not >> parenthesize the operand. Like this: >> >> memset(ð_spec, 0, sizeof eth_spec); >> >> It'll be nice if you could fix that. >> You may also fix that for the 'rte_memcpy' in the fist 'if'. > > Sure, no issue! I'll fix it for all the places in my patch, both in memset > and in rte_memcpy.
Thanks. > Anything else or can I upload v2? No. Go ahead. Looks good at the first glance. > >> >>> >>> >>>>> >>>>> Signed-off-by: Asaf Penso <[email protected]> >>>>> --- >>>>> lib/netdev-dpdk.c | 28 ++++++++++++++-------------- >>>>> 1 file changed, 14 insertions(+), 14 deletions(-) >>>>> >>>>> diff --git a/lib/netdev-dpdk.c b/lib/netdev-dpdk.c index >>>>> 4bf0ca9..5216b74 100644 >>>>> --- a/lib/netdev-dpdk.c >>>>> +++ b/lib/netdev-dpdk.c >>>>> @@ -4570,10 +4570,10 @@ netdev_dpdk_add_rte_flow_offload(struct >>>> netdev *netdev, >>>>> /* Eth */ >>>>> struct rte_flow_item_eth eth_spec; >>>>> struct rte_flow_item_eth eth_mask; >>>>> - memset(ð_spec, 0, sizeof(eth_spec)); >>>>> - memset(ð_mask, 0, sizeof(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; @@ -4600,9 +4600,9 @@ >>>>> netdev_dpdk_add_rte_flow_offload(struct netdev *netdev, >>>>> /* VLAN */ >>>>> struct rte_flow_item_vlan vlan_spec; >>>>> struct rte_flow_item_vlan vlan_mask; >>>>> - memset(&vlan_spec, 0, sizeof(vlan_spec)); >>>>> - memset(&vlan_mask, 0, sizeof(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); >>>>> >>>>> @@ -4617,9 +4617,9 @@ netdev_dpdk_add_rte_flow_offload(struct >>>> netdev *netdev, >>>>> uint8_t proto = 0; >>>>> struct rte_flow_item_ipv4 ipv4_spec; >>>>> struct rte_flow_item_ipv4 ipv4_mask; >>>>> - memset(&ipv4_spec, 0, sizeof(ipv4_spec)); >>>>> - memset(&ipv4_mask, 0, sizeof(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; >>>>> @@ -4662,9 +4662,9 @@ netdev_dpdk_add_rte_flow_offload(struct >>>> netdev >>>>> *netdev, >>>>> >>>>> struct rte_flow_item_tcp tcp_spec; >>>>> struct rte_flow_item_tcp tcp_mask; >>>>> - memset(&tcp_spec, 0, sizeof(tcp_spec)); >>>>> - memset(&tcp_mask, 0, sizeof(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; >>>>> @@ -4687,9 +4687,9 @@ netdev_dpdk_add_rte_flow_offload(struct >>>> netdev >>>>> *netdev, >>>>> >>>>> struct rte_flow_item_udp udp_spec; >>>>> struct rte_flow_item_udp udp_mask; >>>>> - memset(&udp_spec, 0, sizeof(udp_spec)); >>>>> - memset(&udp_mask, 0, sizeof(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; >>>>> >>>>> @@ -4708,9 +4708,9 @@ netdev_dpdk_add_rte_flow_offload(struct >>>> netdev >>>>> *netdev, >>>>> >>>>> struct rte_flow_item_sctp sctp_spec; >>>>> struct rte_flow_item_sctp sctp_mask; >>>>> - memset(&sctp_spec, 0, sizeof(sctp_spec)); >>>>> - memset(&sctp_mask, 0, sizeof(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; >>>>> >>>>> @@ -4729,9 +4729,9 @@ netdev_dpdk_add_rte_flow_offload(struct >>>> netdev >>>>> *netdev, >>>>> >>>>> struct rte_flow_item_icmp icmp_spec; >>>>> struct rte_flow_item_icmp icmp_mask; >>>>> - memset(&icmp_spec, 0, sizeof(icmp_spec)); >>>>> - memset(&icmp_mask, 0, sizeof(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); >>>>> >>>>> _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
