[ovs-dev] [ovs-dev, v2] netdev-dpdk: Memset rte_flow_item on a need basis.
Asaf Penso
asafp at mellanox.com
Tue Feb 5 13:09:08 UTC 2019
Regards,
Asaf Penso
> -----Original Message-----
> From: Ilya Maximets <i.maximets at samsung.com>
> Sent: Tuesday, February 5, 2019 1:46 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>; Finn Christensen <fc at napatech.com>
> Subject: Re: [ovs-dev,v2] netdev-dpdk: Memset rte_flow_item on a need
> basis.
>
> On 04.02.2019 19:14, 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.
> >
> > Signed-off-by: Asaf Penso <asafp at mellanox.com>
> > ---
>
> I thought about this part of code a bit. IMHO, we could create a union from
> the L4 items and clear them all at once. Like this:
>
> uint8_t proto = 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;
>
> memset(&spec, 0, sizeof spec);
> memset(&mask, 0, sizeof mask);
>
>
> Ethernet is a common case, userspace datapath always has exact match on
> vlan,
> IPv4 is also the common case, I think. So current code in most cases we'll not
> call memset only for few of L4 protocols which are in the union here and
> does not eat extra memory.
> Anyway we're not on a very hot path.
>
> With that you'll be able to easily convert L4 proto checking 'if's to a single
> 'switch (proto)' statement and also clear the *ipv4_next_proto_mask
> unconditionally.
>
> What do you think ?
I fully agree and if you can have a separate patch it would be great.
>
> You could incorporate these changes to this patch or I could prepare the
> separate patch on top of it.
>
> Anyway, for the current version:
>
> Acked-by: Ilya Maximets <i.maximets at samsung.com>
>
Thanks!
> > v2 update coding-style compliant for sizeof operator
> > ---
> > lib/netdev-dpdk.c | 36 ++++++++++++++++++------------------
> > 1 file changed, 18 insertions(+), 18 deletions(-)
> >
> > diff --git a/lib/netdev-dpdk.c b/lib/netdev-dpdk.c index
> > 4bf0ca9..f07b10c 100644
> > --- a/lib/netdev-dpdk.c
> > +++ b/lib/netdev-dpdk.c
> > @@ -4570,18 +4570,18 @@ 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)) {
> > - rte_memcpy(ð_spec.dst, &match->flow.dl_dst,
> sizeof(eth_spec.dst));
> > - rte_memcpy(ð_spec.src, &match->flow.dl_src,
> sizeof(eth_spec.src));
> > + 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));
> > + sizeof eth_mask.dst);
> > rte_memcpy(ð_mask.src, &match->wc.masks.dl_src,
> > - sizeof(eth_mask.src));
> > + sizeof eth_mask.src);
> > eth_mask.type = match->wc.masks.dl_type;
> >
> > add_flow_pattern(&patterns, RTE_FLOW_ITEM_TYPE_ETH, @@
> > -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