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

Yanqin Wei Yanqin.Wei at arm.com
Fri Jul 10 08:30:13 UTC 2020


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?
In some function, the parameter is struct flow_tnl instead of 'pkt_metadata' or 'flow'. In these function, tunnel_valid cannot be used for valid check. So some function signatures need be modified.
> 
> >>
> >> 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.
OK, it makes sense. I'll check all the places where flow_tnl is used and update v2 for tunnel_valid checking. 

> And yes, 'tunnel_valid' flag really needs a comment inside the structure
> definition.
OK, will add comment in V2.
> 
> >
> >>
> >> 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.


More information about the dev mailing list