Re: [ovs-dev] [RFC net-next 1/6] openvswitch: exclude kernel flow key from upcalls

[Adrian Moreno]

On 11/25/22 16:51, Ilya Maximets wrote:
> On 11/25/22 16:29, Adrian Moreno wrote:
> > On 11/23/22 22:22, Ilya Maximets wrote:
> > > On 11/22/22 15:03, Aaron Conole wrote:
> > > > When processing upcall commands, two groups of data are available to
> > > > userspace for processing: the actual packet data and the kernel
> > > > sw flow key data.  The inclusion of the flow key allows the userspace
> > > > avoid running through the dissection again.
> > > >
> > > > However, the userspace can choose to ignore the flow key data, as is
> > > > the case in some ovs-vswitchd upcall processing.  For these messages,
> > > > having the flow key data merely adds additional data to the upcall
> > > > pipeline without any actual gain.  Userspace simply throws the data
> > > > away anyway.
> > >
> > > Hi, Aaron.  While it's true that OVS in userpsace is re-parsing the
> > > packet from scratch and using the newly parsed key for the OpenFlow
> > > translation, the kernel-porvided key is still used in a few important
> > > places.  Mainly for the compatibility checking.  The use is described
> > > here in more details:
> > >     https://docs.kernel.org/networking/openvswitch.html#flow-key-compatibility
> > >
> > > We need to compare the key generated in userspace with the key
> > > generated by the kernel to know if it's safe to install the new flow
> > > to the kernel, i.e. if the kernel and OVS userpsace are parsing the
> > > packet in the same way.
> >
> > Hi Ilya,
> >
> > Do we need to do that for every packet?
> > Could we send a bitmask of supported fields to userspace at feature
> > negotiation and let OVS slowpath flows that it knows the kernel won't
> > be able to handle properly?
>
> It's not that simple, because supported fields in a packet depend
> on previous fields in that same packet.  For example, parsing TCP
> header is generally supported, but it won't be parsed for IPv6
> fragments (even the first one), number of vlan headers will affect
> the parsing as we do not parse deeper than 2 vlan headers, etc.
> So, I'm afraid we have to have a per-packet information, unless we
> can somehow probe all the possible valid combinations of packet
> headers.

Surely. I understand that we'd need more than just a bit per field. Things like 
L4 on IPv6 frags would need another bit and the number of VLAN headers would 
need some more. But, are these a handful of exceptions or do we really need all 
the possible combinations of headers? If it's a matter of naming a handful of 
corner cases I think we could consider expressing them at initialization time 
and safe some buffer space plus computation time both in kernel and userspace.

--
Adrián Moreno

> > > On the other hand, OVS today doesn't check the data, it only checks
> > > which fields are present.  So, if we can generate and pass the bitmap
> > > of fields present in the key or something similar without sending the
> > > full key, that might still save some CPU cycles and memory in the
> > > socket buffer while preserving the ability to check for forward and
> > > backward compatibility.  What do you think?
> > >
> > >
> > > The rest of the patch set seems useful even without patch #1 though.
> > >
> > > Nit: This patch #1 should probably be merged with the patch #6 and be
> > > at the end of a patch set, so the selftest and the main code are updated
> > > at the same time.
> > >
> > > Best regards, Ilya Maximets.
> > > _______________________________________________
> > > dev mailing list
> > > dev@xxxxxxxxxxxxxxx
> > > https://mail.openvswitch.org/mailman/listinfo/ovs-dev
> >
> > Thanks