Re: [virtio-comment] Re: [PATCH] virtio-net: define feature of per-packe

Subject: Re: [virtio-comment] Re: [PATCH] virtio-net: define feature of per-packet RSS hash delivery

From: "Michael S. Tsirkin" <mst@redhat.com>
To: "Yuri Benditovich" <yuri.benditovich@daynix.com>
Cc: virtio-comment@lists.oasis-open.org
Sent: Tuesday, December 3, 2019 10:25:30 PM
Subject: [virtio-comment] Re: [PATCH] virtio-net: define feature of per-packet RSS hash delivery

On Tue, Dec 03, 2019 at 08:32:12PM +0200, Yuri Benditovich wrote:
> Fixes: https://github.com/oasis-tcs/virtio-spec/issues/66
> Conditional extending of virtio header structure to deliver
> packet's hash and hash type used for calculation.
>
> Signed-off-by: Yuri Benditovich <yuri.benditovich@daynix.com>

Functionally this looks mostly good. But we need to change the
way it's documented slightly.

Probably, because the RSS patch is not merged yet.

> ---
> content.tex | 24 ++++++++++++++++++++++++
> 1 file changed, 24 insertions(+)
>
> diff --git a/content.tex b/content.tex
> index 01be7df..44975e4 100644
> --- a/content.tex
> +++ b/content.tex
> @@ -2811,6 +2811,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_REPORT(58)] Device can report per-packet hash value
> + and a type of calculated hash
> +
> \item[VIRTIO_NET_F_RSS(60)] Device supports RSS (receive-side scaling)
> with Toeplitz hash calculation and configurable hash parameters for receive steering
>
> @@ -2844,6 +2847,8 @@ \subsubsection{Feature bit requirements}\label{sec:Device Types / Network Device
> \item[VIRTIO_NET_F_CTRL_MAC_ADDR] Requires VIRTIO_NET_F_CTRL_VQ.
> \item[VIRTIO_NET_F_RSC_EXT] Requires VIRTIO_NET_F_HOST_TSO4 or VIRTIO_NET_F_HOST_TSO6.
> \item[VIRTIO_NET_F_RSS] Requires VIRTIO_NET_F_CTRL_VQ.
> +\item[VIRTIO_NET_F_HASH_REPORT] Requires VIRTIO_NET_F_RSS.
> +
> \end{description}
>
> \subsubsection{Legacy Interface: Feature bits}\label{sec:Device Types / Network Device / Feature bits / Legacy Interface: Feature bits}
> @@ -3071,6 +3076,8 @@ \subsection{Device Operation}\label{sec:Device Types / Network Device / Device O
> le16 csum_start;
> le16 csum_offset;
> le16 num_buffers;
> + le32 hash_value; (Only if VIRTIO_NET_F_HASH_REPORT negotiated)
> + le16 hash_type; (Only if VIRTIO_NET_F_HASH_REPORT negotiated)
> };
> \end{lstlisting}
>
> @@ -3330,6 +3337,15 @@ \subsubsection{Processing of Incoming Packets}\label{sec:Device Types / Network
> set: if so, device has validated the packet checksum.
> In case of multiple encapsulated protocols, one level of checksums
> has been validated.
> +\item If VIRTIO_NET_F_HASH_REPORT was negotiated and a device has calculated
> + a hash for the packet, \field{hash_value} contains calculated hash value and
> + \field{hash_type} contains exact hash type.
> +
> + If the hash was not calculated, \field{hash_type} contains zero.
> +
> + For defined hash types and their meaning, see \ref{sec:Device Types / Network Device / Device configuration layout / RSS}.
> +
> + For the procedure of hash calculation, see \ref{sec:Device Types / Network Device / Device Operation / Control Virtqueue / Receive-side scaling (RSS) / RSS hash types}.
> \end{enumerate}
>
> Additionally, VIRTIO_NET_F_GUEST_CSUM, TSO4, TSO6, UDP and ECN
> @@ -3909,6 +3925,14 @@ \subsubsection{Control Virtqueue}\label{sec:Device Types / Network Device / Devi
> \item Else skip IPv6 extension headers and calculate the hash as defined above for IPv6 packet without extension headers
> \end{itemize}
>
> +If VIRTIO_NET_F_HASH_REPORT was negotiated, the device reports calculated hash information in fields of virtio_net_hdr as follows:
> +
> +Exact hash type is populated in \field{hash_type}
> +
> +Hash value is populated in \field{hash_value}
> +
> +If, due to any reason, the device did not calculate the hash, it sets \field{hash_type} to zero.
> +
> \drivernormative{\subparagraph}{Setting RSS parameters}{Device Types / Network Device / Device Operation / Control Virtqueue / Receive-side scaling (RSS) }
>
> A driver MUST NOT send the VIRTIO_NET_CTRL_MQ_RSS_CONFIG command if the feature VIRTIO_NET_F_RSS has not been negotiated.

This is understandably a minimal change, but imho messes things up:
hash calculation is no longer part of RSS so really should be moved
from out of there.

Please advise why "hash calculation is no longer part of RSS".

So we'd have a chapter for hash calculation, defining hash types
etc.

A chapter for hash calculation is in previous patch

https://lists.oasis-open.org/archives/virtio-comment/201911/msg00014.html

Then both RSS and the new chapter refer to that.

Also, I am guessing supported hash types should be valid
in config space with the new feature - isn't that right?

Yes, of course. But this is not related to hash delivery.

Packet hash is calculated according to allowed hash types.

If hash delivery is negotiated, the device reports the calculated hash type which is, naturally, one of valid ones.

> --
> 2.17.2

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/

virtio-comment message