On 2/5/2019 1:18 PM, Ilya Maximets wrote:
On 05.02.2019 16:09, Asaf Penso wrote:


Regards,
Asaf Penso

-----Original Message-----
From: Ilya Maximets <[email protected]>
Sent: Tuesday, February 5, 2019 1:46 PM
To: Asaf Penso <[email protected]>; [email protected]
Cc: Roni Bar Yanai <[email protected]>; Stokes, Ian
<[email protected]>; Finn Christensen <[email protected]>
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 <[email protected]>
---

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.

OK. Good.
I'll prepare the patch as soon as this one will be merged.


Thanks for the patch Asaf, I've pushed this to master, I'll also add your name to the AUTHORS doc.

Ian
_______________________________________________
dev mailing list
[email protected]
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Reply via email to