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. _______________________________________________ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev