Re: [virtio-comment] [PATCH v2] virtio-net: Define per-packet hash reporting feature

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

On Wed, Feb 05, 2020 at 04:32:42AM -0500, Yuri Benditovich wrote:
> ----- Original Message ----- 
> 
> > From: "Michael S. Tsirkin" <mst@redhat.com>
> > To: "Yuri Benditovich" <ybendito@redhat.com>
> > Cc: "Yuri Benditovich" <yuri.benditovich@daynix.com>,
> > virtio-comment@lists.oasis-open.org
> > Sent: Monday, February 3, 2020 3:20:56 PM
> > Subject: Re: [virtio-comment] [PATCH v2] virtio-net: Define per-packet hash
> > reporting feature
> 
> > On Mon, Feb 03, 2020 at 08:10:57AM -0500, Yuri Benditovich wrote:
> > > > > +\item The device uses \field{hash_key_data} of the
> > > > > virtio_net_rss_config
> > > > > structure.
> > > > > +\end{itemize}
> > > > > +See the definition of virtio_net_rss_config structure in
> > > > > \ref{sec:Device
> > > > > Types / Network Device / Device Operation / Control Virtqueue /
> > > > > Receive-side scaling (RSS) / Setting RSS parameters}
> > > > > +
> > > > > +If the feature VIRTIO_NET_F_RSS was not negotiated, the device uses
> > > > > internal configuration
> > > > > +of enabled hash types and a key used for hash calculation.
> > >
> > > > I don't understand this last paragraph. Could you clarify?
> > >
> > > In order to calculate a hash the device shall have 2 things:
> > > a key for hash calculation
> > > a bitmask which hash types are enabled
> > >
> > > In case the device supportes RSS command (even with single queue), it can
> > > take them from current RSS configuration.
> > > If only VIRTIO_NET_F_HASH_REPORT is negotiated, the device uses internal
> > > preset of both parameters.
> > >
> > > Do you suggest something different?
> 
> > Hmm control over key/mask would be nice to have.
> > I guess we could add a command along the lines of:
> 
> > struct virtio_net_hash_config {
> > le32 hash_types;
> > le16 reserved[4];
> > u8 hash_key_length;
> > u8 hash_key_data[hash_key_length];
> > };
> 
> > This needs some thought: we don't want to request that devices have
> > double the machinery so the command should not be there if RSS is
> > enabled ...
> 
> > Maybe the command is only valid if HASH is set but RSS is clear?
> 
> No problem.
> To avoid further ping-pongs:
> What is this reserved[4] needed for?

It makes the layout match the RSS config command exactly, so
we don't need to duplicate code.
In fact we can instead just say that this command
uses the same structure, with rss fields are zeroed out.

> Please confirm:
> - this is an additional command under VIRTIO_NET_CTRL_MQ
> - should be supported when HASH_REPORT is negotiated, with or without MQ, not needed if RSS supported
> - Initially all hashes are disabled

This is what I would do, yes.

> > --
> > MST
> 
> > This publicly archived list offers a means to provide input to the
> > OASIS Virtual I/O Device (VIRTIO) TC.
> 
> > In order to verify user consent to the Feedback License terms and
> > to minimize spam in the list archive, subscription is required
> > before posting.
> 
> > Subscribe: virtio-comment-subscribe@lists.oasis-open.org
> > Unsubscribe: virtio-comment-unsubscribe@lists.oasis-open.org
> > List help: virtio-comment-help@lists.oasis-open.org
> > List archive: https://lists.oasis-open.org/archives/virtio-comment/
> > Feedback License: https://www.oasis-open.org/who/ipr/feedback_license.pdf
> > List Guidelines: https://www.oasis-open.org/policies-guidelines/mailing-lists
> > Committee: https://www.oasis-open.org/committees/virtio/
> > Join OASIS: https://www.oasis-open.org/join/