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

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

On Wed, Feb 01, 2023 at 01:51:25AM +0000, Parav Pandit wrote:
> 
> 
> > From: Heng Qi <hengqi@linux.alibaba.com>
> > Sent: Tuesday, January 31, 2023 12:57 AM
> > 
> > On Wed, Jan 18, 2023 at 11:45:39PM +0000, Parav Pandit wrote:
> > >
> > >
> > > > From: virtio-dev@lists.oasis-open.org
> > > > <virtio-dev@lists.oasis-open.org>
> > > > Sent: Wednesday, January 4, 2023 2:14 AM
> > >
> > > > 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.
> > >
> > > > +\item[VIRTIO_NET_F_HASH_TUNNEL(52)] Device supports inner
> > > > +    header hash for GRE, VXLAN and GENEVE tunnel-encapsulated packets.
> > > > +
> > > A device may not support all 3 at the same time.
> > > Please remove mentioning tunneling protocols description from here.
> > > Just say device support inner header hash ...
> > 
> > Sorry for the late reply due to vacation.
> > 
> > Good idea, Michael suggested doing the same. But we also discussed this issue:
> > Early, we used a feature bit to force devices to support GRE and VXLAN 
> ..
> There is no need to force the device.
> LM will work as it will use both he values - the feature bit and the supported/negotiated hash type in the config space.
> 
> > > An additional bit map somewhere else should say supported hash over
> > different tunneling types.
> > >
> > 
> > Yes, we use \field{supported_hash_types} to declare supported hash types.
> Nice. So yes, just remove the description from the feature bits.
> [..]
> > >
> > > With the inclusion of tunnel outer header, it doesn't need to redefine the
> > hashing for inner packets which is exactly same without the tunnel.
> > > hash tunnel capability only indicates that hashing is done on the inner packets
> > as_before.
> > >
> > 
> > This seems like a trade-off, and I can get rid of this if the simple statement
> > "computing the hash for the inner header of the tunnel packet is the same as
> > without tunnel"
> > is also clear to the reader.
> > What do you think? Cc Jason and Michael.
> Not sure I follow your comment.
> Inclusion of outer header has zero change in inner header hash.
> 
> The data structure etc is just fine in the patch you proposed.
> So, there is no need to redefine all over it again.
> Only description need to be simplified as below.
> Please just change the text wording to indicate that when tunnel feature is negotiated, and if the hashing is performed by the device on the inner header, hash_report_tunnel will contain the valid outer tunnel type information.
> 
> Also, this patch is adding two functionalities.
> 1. Inner header hash calculation of existing already defined hash types 
> 2. outer header hash for new type for GRE,VXLAN,GENEVE.
> #1 should be in 1st patch.
> #2 should be in 2nd patch.
> This is better to review.

Parav, you come to this discussion pretty late. Asking to split up
the patch when it's v1/v2 is ok. Asking after others have
already reviewed v6 is not you are making review easier
for yourself but re-review harder for others who already
have a mind map of the patch.

In this case unless we really want to enable these separately
(and frankly I don't see a good reason to) then
splitting it up makes review more confusing.

> Overall useful features. Thanks for adding.