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

[Ian Stokes]

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

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

Ian