On Wed, Feb 01, 2023 at 01:51:25AM +0000, Parav Pandit wrote:
>
>
> > From: Heng Qi <hen...@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.
---------------------------------------------------------------------
To unsubscribe, e-mail: virtio-dev-unsubscr...@lists.oasis-open.org
For additional commands, e-mail: virtio-dev-h...@lists.oasis-open.org
