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

[Heng Qi <hengqi@linux.alibaba.com>]

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.
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