[ovs-dev] [PATCH v1 2/6] dpif-netdev: add tunnel_valid flag to skip ip/ipv6 address comparison

[Ilya Maximets]

On 7/6/20 1:27 PM, Yanqin Wei wrote:
> Hi Ilya,
> 
>>> ---
>>
>> Hi.
>> First of all, thanks for working on performance improvements!
> Thanks, I saw some slides where OVS was used to compare flow scalability with other projects. It inspired me to optimize this code.
> 
>>
>> However, this doesn't look as a clean patch.
> There are some trade-off for legacy code.

What trade-offs?

>>
>> Why we need both pkt_metadata_datapath_init() and pkt_metadata_init() ?
>> Why we can't just not initialize ip_dst and use tunnel_valid flag everywhere?
> 
> This patch wants to reduce the scope of modification( only for fastpath), because performance is not critical for slow path. So tunnel dst@ is set before leaving fast path(upcall).
> Another reason is 'flow_tnl' member is defined in both ' pkt_metadata' and 'flow'.  If tunnel_valid flag is introduced into 'flow', the layout and  legacy flow API also need to be modified.

I understand that you didn't want to touch anything beside the
performance critical parts.  However, dp_packet_/pkt_ API is
already heavily overloaded and having a few very similar
functions that can or can not be used in some contexts makes
things even more complicated.  It's hard to read and maintain.
And it's prone to errors in case someone will try t modify
datapath code.
I'd prefer not t have different initialization functions and
only have one variant.  This will also solve the issue that
every other part of code uses tunneling metadata without checking
'tunnel_valid' flag.  This is actually a logical mistake.
And yes, 'tunnel_valid' flag really needs a comment inside the
structure definition.

> 
>>
>> Current version complicates code making it less readable and prone to errors.
> Do you prefer to use tunnel_valid in both fast path and slow path? I could send v2 for this modification.
>  
>> Best regards, Ilya Maximets.