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

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

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?

-- 
MST