[ovs-dev] netdev-dpdk: Memset rte_flow_item on a need basis.
Asaf Penso
asafp at mellanox.com
Mon Feb 4 15:50:43 UTC 2019
Regards,
Asaf Penso
> -----Original Message-----
> From: Ilya Maximets <i.maximets at samsung.com>
> Sent: Monday, February 4, 2019 5:41 PM
> To: Asaf Penso <asafp at mellanox.com>; ovs-dev at openvswitch.org
> Cc: Roni Bar Yanai <roniba at mellanox.com>; Stokes, Ian
> <ian.stokes at intel.com>
> 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 <i.maximets at samsung.com>
> >> Sent: Monday, February 4, 2019 4:14 PM
> >> To: Asaf Penso <asafp at mellanox.com>; ovs-dev at openvswitch.org
> >> Cc: Roni Bar Yanai <roniba at mellanox.com>; Stokes, Ian
> >> <ian.stokes at intel.com>
> >> 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.
Anything else or can I upload v2?
>
> >
> >
> >>>
> >>> Signed-off-by: Asaf Penso <asafp at mellanox.com>
> >>> ---
> >>> 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);
> >>>
> >>>
More information about the dev
mailing list