[ovs-dev] [ovs-dev, v2] netdev-dpdk: Memset rte_flow_item on a need basis.

[Ilya Maximets]

On 05.02.2019 16:09, Asaf Penso wrote:
> 
> 
> 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.

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

> 
>>
>> 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!