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

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

On Wed, Dec 04, 2019 at 12:20:01AM -0500, Yuri Benditovich wrote:
> 
> 
> ━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━
> 
>     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".

I might be misunderstanding things. It looks like with this
one can enable hash even with a single RX queue.
No?

> 
> 
>     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
> 

Can't see it there. Do you mean
+\subparagraph{RSS hash types}\label{sec:Device Types / Network Device / Device Operation / Control Virtqueue / Receive-side scaling (RSS) / RSS hash types}
?
Sure, what I am saying it needs to be moved from out of RSS then.

> 
>     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.

So we need to say this in the spec I think.

> 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/
> 
> 
> 


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/