Re: [virtio-dev] Re: [virtio-comment] Re: [PATCH v7] virtio-net: support inner header hash

["Michael S. Tsirkin" <mst@redhat.com>]

On Tue, Jan 10, 2023 at 10:06:53AM +0800, Jason Wang wrote:
> On Mon, Jan 9, 2023 at 7:34 PM Michael S. Tsirkin <mst@redhat.com> wrote:
> >
> > On Mon, Jan 09, 2023 at 04:59:26PM +0800, Jason Wang wrote:
> > > On Mon, Jan 9, 2023 at 10:43 AM Heng Qi <hengqi@linux.alibaba.com> wrote:
> > > >
> > > > On Fri, Jan 06, 2023 at 01:59:38AM -0500, Michael S. Tsirkin wrote:
> > > > > On Fri, Jan 06, 2023 at 02:42:21PM +0800, Heng Qi wrote:
> > > > > > On Fri, Jan 06, 2023 at 12:27:04AM -0500, Michael S. Tsirkin wrote:
> > > > > > > On Wed, Jan 04, 2023 at 03:14:01PM +0800, Heng Qi wrote:
> > > > > > > > If the tunnel is used to encapsulate the packets, the hash calculated
> > > > > > > > using the outer header of the receive packets is always fixed for the
> > > > > > > > same flow packets, i.e. they will be steered to the same receive queue.
> > > > > > > >
> > > > > > > > We add a feature bit VIRTIO_NET_F_HASH_TUNNEL and related bitmasks
> > > > > > > > in \field{hash_types}, which instructs the device to calculate the
> > > > > > > > hash using the inner headers of tunnel-encapsulated packets. Besides,
> > > > > > > > values in \field{hash_report_tunnel} are added to report tunnel types.
> > > > > > > >
> > > > > > > > Fixes: https://github.com/oasis-tcs/virtio-spec/issues/151
> > > > > > > >
> > > > > > > > Reviewed-by: Jason Wang <jasowang@redhat.com>
> > > > > > > > Signed-off-by: Heng Qi <hengqi@linux.alibaba.com>
> > > > > > > > Signed-off-by: Xuan Zhuo <xuanzhuo@linux.alibaba.com>
> > > > > > >
> > > > > > >
> > > > > > > ok close to being ready. a couple of minor comments.
> > > > > > >
> > > > > > > > ---
> > > > > > > > v6:
> > > > > > > >         1. Modify the wording of some sentences for clarity. @Michael S. Tsirkin
> > > > > > > >         2. Fix some syntax issues. @Michael S. Tsirkin
> > > > > > > >
> > > > > > > > v5:
> > > > > > > >         1. Fix some syntax and capitalization issues. @Michael S. Tsirkin
> > > > > > > >         2. Use encapsulated/encaptulation uniformly. @Michael S. Tsirkin
> > > > > > > >         3. Move the links to introduction section. @Michael S. Tsirkin
> > > > > > > >         4. Clarify some sentences. @Michael S. Tsirkin
> > > > > > > >
> > > > > > > > v4:
> > > > > > > >         1. Clarify some paragraphs. @Cornelia Huck
> > > > > > > >         2. Fix the u8 type. @Cornelia Huck
> > > > > > > >
> > > > > > > > v3:
> > > > > > > >         1. Rename VIRTIO_NET_F_HASH_GRE_VXLAN_GENEVE_INNER to VIRTIO_NET_F_HASH_TUNNEL. @Jason Wang
> > > > > > > >         2. Make things clearer. @Jason Wang @Michael S. Tsirkin
> > > > > > > >         3. Keep the possibility to use inner hash for automatic receive steering. @Jason Wang
> > > > > > > >         4. Add the "Tunnel packet" paragraph to avoid repeating the GRE etc. many times. @Michael S. Tsirkin
> > > > > > > >
> > > > > > > > v2:
> > > > > > > >         1. Add a feature bit for GRE/VXLAN/GENEVE inner hash. @Jason Wang
> > > > > > > >         2. Chang \field{hash_tunnel} to \field{hash_report_tunnel}. @Jason Wang, @Michael S. Tsirkin
> > > > > > > >
> > > > > > > > v1:
> > > > > > > >         1. Remove the patch for the bitmask fix. @Michael S. Tsirkin
> > > > > > > >         2. Clarify some paragraphs. @Jason Wang
> > > > > > > >         3. Add \field{hash_tunnel} and VIRTIO_NET_HASH_REPORT_GRE. @Yuri Benditovich
> > > > > > > >
> > > > > > > >  content.tex      | 191 +++++++++++++++++++++++++++++++++++++++++++++--
> > > > > > > >  introduction.tex |  19 +++++
> > > > > > > >  2 files changed, 203 insertions(+), 7 deletions(-)
> > > > > > > >
> > > > > > > > diff --git a/content.tex b/content.tex
> > > > > > > > index e863709..7845f6c 100644
> > > > > > > > --- a/content.tex
> > > > > > > > +++ b/content.tex
> > > > > > > > @@ -3084,6 +3084,9 @@ \subsection{Feature bits}\label{sec:Device Types / Network Device / Feature bits
> > > > > > > >  \item[VIRTIO_NET_F_CTRL_MAC_ADDR(23)] Set MAC address through control
> > > > > > > >      channel.
> > > > > > > >
> > > > > > > > +\item[VIRTIO_NET_F_HASH_TUNNEL(52)] Device supports inner
> > > > > > > > +    header hash for GRE, VXLAN and GENEVE tunnel-encapsulated packets.
> > > > > > >
> > > > > > > I would probably drop the list of tunnel types here.
> > > > > >
> > > > > > Do you mean to use "Device supports inner header hash for
> > > > > > tunnel-encapsulated packets." instead? Why? We do want to use this
> > > > > > feature bit to indicate that the device supports inner hashing of
> > > > > > GRE, VXLAN and GENEVE encapsulated packets. As in the v3 discussion
> > > > > > https://lists.oasis-open.org/archives/virtio-dev/202212/msg00024.html ,
> > > > > > we discussed using VIRTIO_NET_F_HASH_TUNNEL to replace
> > > > > > VIRTIO_NET_F_HASH_GRE_VXLAN_GENEVE_INNER and plan to use
> > > > > > VIRTIO_NET_F_HASH_TUNNEL_XYZ for future extensions.
> > > > >
> > > > > So imagine we add a new tunnel type. Let's say there's VXLAN v2.
> > > > > why would we need a new feature bit? I think a new hash type
> > > > > will be sufficient. No?
> > > >
> > > > If the description for VIRTIO_NET_F_HASH_TUNNEL is as follows:
> > > > "[VIRTIO_NET_F_HASH_TUNNEL(52)] Device supports inner header hash for tunnel-encapsulated packets.".
> > > > Then the following may happen
> > > > 1. For VXLANv2, if both device src and device dst have negotiated this feature, it is assumed that
> > > >    device src supports VXLAN and VXLANv2, but device dst may only support VXLAN, not VXLANv2.
> > > > 2. For other encapsulation protocols such as ip in ip, after device src and device dst have
> > > >    negotiated this feature, it is assumed that device src supports GRE, VXLAN, GENEVE and ip in ip,
> > > >    but it is not clear that device dst also supports ip in ip. Especially when migrating, this can
> > > >    lead to inconsistencies in live migrations.
> >
> > it does not matter. e.g. if device supports GRE but not VXLAN then
> > feature bit it set but hash types are different.
> 
> But the hash will be used for RSS steering. Consider the steering on
> ipip work on src but not on dst it would break application logic.

Exactly. And for this reason just making sure feature bits match
is not sufficient for migration. It follows that hash types
must also match, and therefore there is no need to invent
new feature bits for migration if all we are doing is adding new hash types.


> > and this is not different from any other offload we have.
> 
> But we have different feature bits for TCPv4/v6.


For segmentation but not for hash. Basically because we
don't have segmentation types analogous to hash types.

> >
> > Generally the only reason we even use a new feature bit for this
> > tunneling is because the header structure is a bit different
> > so this kind of makes sense.
> >
> > >
> > > Yes, this looks like the only way that I can think of to keep
> > > migration compatibility in an easy way.
> > >
> > > Thanks
> >
> > Jason migration compatibility must also check hash types.
> > There's no chance with the current hash offload scheme to
> > only rely on feature bits for migration compatibility.
> 
> This will give extra pressure on the management stack, e.g it requires
> the device to have an out of spec way for introspection.
> 
> Thanks

As I tried to explain this is already the case. Feature bits do not
describe device capabilities fully, some of them are in config space.

> >
> >
> > > > So, I think it's better to keep the original description:
> > > > "[VIRTIO_NET_F_HASH_TUNNEL(52)] Device supports inner header hash for GRE, VXLAN and GENEVE tunnel-encapsulated packets."
> > > >
> > > > Thanks.
> > > >
> > > > >
> > > > > --
> > > > > MST
> > > > >
> > > > >
> > > > > ---------------------------------------------------------------------
> > > > > To unsubscribe, e-mail: virtio-dev-unsubscribe@lists.oasis-open.org
> > > > > For additional commands, e-mail: virtio-dev-help@lists.oasis-open.org
> > > >
> >