From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: virtio-comment-return-983-cohuck=redhat.com@lists.oasis-open.org Sender: List-Post: List-Help: List-Unsubscribe: List-Subscribe: Received: from lists.oasis-open.org (oasis-open.org [10.110.1.242]) by lists.oasis-open.org (Postfix) with ESMTP id 19DC298603B for ; Wed, 4 Dec 2019 05:20:08 +0000 (UTC) Date: Wed, 4 Dec 2019 00:20:01 -0500 (EST) From: Yuri Benditovich Message-ID: <471210655.12237442.1575436801606.JavaMail.zimbra@redhat.com> In-Reply-To: <20191203152148-mutt-send-email-mst@kernel.org> References: <20191203183212.1721642-1-yuri.benditovich@daynix.com> <20191203152148-mutt-send-email-mst@kernel.org> MIME-Version: 1.0 Subject: Re: [virtio-comment] Re: [PATCH] virtio-net: define feature of per-packet RSS hash delivery Content-Type: multipart/alternative; boundary="----=_Part_12237441_1660184818.1575436801605" To: "Michael S. Tsirkin" Cc: Yuri Benditovich , virtio-comment@lists.oasis-open.org List-ID: ------=_Part_12237441_1660184818.1575436801605 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: quoted-printable ----- Original Message ----- > From: "Michael S. Tsirkin" > To: "Yuri Benditovich" > 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 > 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.=20 > > --- > > 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 rec= eive > > 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 Ty= pes > > / 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 va= lue > > 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:Devi= ce > > Types / Network Device / Devi > > \item Else skip IPv6 extension headers and calculate the hash as define= d > > above for IPv6 packet without extension headers > > \end{itemize} > > > > +If VIRTIO_NET_F_HASH_REPORT was negotiated, the device reports calcula= ted > > 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".=20 > So we'd have a chapter for hash calculation, defining hash types > etc. A chapter for hash calculation is in previous patch=20 https://lists.oasis-open.org/archives/virtio-comment/201911/msg00014.html= =20 > 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.=20 Packet hash is calculated according to allowed hash types.=20 If hash delivery is negotiated, the device reports the calculated hash type= which is, naturally, one of valid ones.=20 > > -- > > 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-l= ists > Committee: https://www.oasis-open.org/committees/virtio/ > Join OASIS: https://www.oasis-open.org/join/ ------=_Part_12237441_1660184818.1575436801605 Content-Type: text/html; charset=UTF-8 Content-Transfer-Encoding: quoted-printable


To: "Y= uri Benditovich" <yuri.benditovich@daynix.com>
Cc: virtio-c= omment@lists.oasis-open.org
Sent: Tuesday, December 3, 2019 10:25= :30 PM
Subject: [virtio-comment] Re: [PATCH] virtio-net: define f= eature of per-packet RSS hash delivery

On Tue, Dec 03, 20= 19 at 08:32:12PM +0200, Yuri Benditovich wrote:
> Fixes: https://gith= ub.com/oasis-tcs/virtio-spec/issues/66
> Conditional extending of vir= tio header structure to deliver
> packet's hash and hash type used fo= r calculation.
>
> Signed-off-by: Yuri Benditovich <yuri.be= nditovich@daynix.com>

Functionally this looks mostly g= ood. 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{Featur= e bits}\label{sec:Device Types / Network Device / Feature bits
> &nbs= p;\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_N= ET_F_RSS(60)] Device supports RSS (receive-side scaling)
>   &nb= sp;  with Toeplitz hash calculation and configurable hash parameters f= or receive steering
>  
> @@ -2844,6 +2847,8 @@ \subsubsec= tion{Feature bit requirements}\label{sec:Device Types / Network Device
&= gt;  \item[VIRTIO_NET_F_CTRL_MAC_ADDR] Requires VIRTIO_NET_F_CTRL_VQ.<= br>>  \item[VIRTIO_NET_F_RSC_EXT] Requires VIRTIO_NET_F_HOST_TSO4 o= r VIRTIO_NET_F_HOST_TSO6.
>  \item[VIRTIO_NET_F_RSS] Requires VI= RTIO_NET_F_CTRL_VQ.
> +\item[VIRTIO_NET_F_HASH_REPORT] Requires VIRTI= O_NET_F_RSS.
> +
>  \end{description}
>  
&g= t;  \subsubsection{Legacy Interface: Feature bits}\label{sec:Device Ty= pes / Network Device / Feature bits / Legacy Interface: Feature bits}
&g= t; @@ -3071,6 +3076,8 @@ \subsection{Device Operation}\label{sec:Device Typ= es / Network Device / Device O
>          le= 16 csum_start;
>          le16 csum_offset;<= br>>          le16 num_buffers;
> +  = ;      le32 hash_value; (Only if VIRTIO_NET_F_HASH_REPORT ne= gotiated)
> +        le16 hash_type; (Only if VIR= TIO_NET_F_HASH_REPORT negotiated)
>  };
>  \end{lstli= sting}
>  
> @@ -3330,6 +3337,15 @@ \subsubsection{Process= ing of Incoming Packets}\label{sec:Device Types / Network
>   &n= bsp;set: if so, device has validated the packet checksum.
>   &n= bsp;In case of multiple encapsulated protocols, one level of checksums
&= gt;    has been validated.
> +\item If VIRTIO_NET_F_HASH_RE= PORT 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.
> +
> + &nbs= p;If the hash was not calculated, \field{hash_type} contains zero.
> = +
> +  For defined hash types and their meaning, see \ref{sec:De= vice 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-si= de scaling (RSS) / RSS hash types}.
>  \end{enumerate}
> &= nbsp;
>  Additionally, VIRTIO_NET_F_GUEST_CSUM, TSO4, TSO6, UDP = and ECN
> @@ -3909,6 +3925,14 @@ \subsubsection{Control Virtqueue}\la= bel{sec:Device Types / Network Device / Devi
>  \item Else skip = IPv6 extension headers and calculate the hash as defined above for IPv6 pac= ket without extension headers
>  \end{itemize}
>  > +If VIRTIO_NET_F_HASH_REPORT was negotiated, the device reports calcu= lated 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 t= o any reason, the device did not calculate the hash, it sets \field{hash_ty= pe} to zero.
> +
>  \drivernormative{\subparagraph}{Settin= g RSS parameters}{Device Types / Network Device / Device Operation / Contro= l Virtqueue / Receive-side scaling (RSS) }
>  
>  A d= river MUST NOT send the VIRTIO_NET_CTRL_MQ_RSS_CONFIG command if the featur= e VIRTIO_NET_F_RSS has not been negotiated.


This is u= nderstandably a minimal change, but imho messes things up:
hash calculat= ion 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 ha= sh types
etc.
A chapter for hash calculation is in prev= ious patch
https://lists.oasis-open.org/archives/virtio-comme= nt/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 t= ype which is, naturally, one of valid ones.


> --
> 2.= 17.2


This publicly archived list offers a means to pr= ovide 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.p= df
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/


------=_Part_12237441_1660184818.1575436801605--