[virtio-dev] Re: [PATCH v7] virtio-net: support inner header hash

2023-01-11 Thread Michael S. Tsirkin
On Wed, Jan 04, 2023 at 03:14:01PM +0800, Heng Qi wrote:
> 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.
> 
> We add a feature bit VIRTIO_NET_F_HASH_TUNNEL and related bitmasks
> in \field{hash_types}, which instructs the device to calculate the
> hash using the inner headers of tunnel-encapsulated packets. Besides,
> values in \field{hash_report_tunnel} are added to report tunnel types.
> 
> Fixes: https://github.com/oasis-tcs/virtio-spec/issues/151
> 
> Reviewed-by: Jason Wang 
> Signed-off-by: Heng Qi 
> Signed-off-by: Xuan Zhuo 
> ---
> v6:
>   1. Modify the wording of some sentences for clarity. @Michael S. Tsirkin
>   2. Fix some syntax issues. @Michael S. Tsirkin
> 
> v5:
>   1. Fix some syntax and capitalization issues. @Michael S. Tsirkin
>   2. Use encapsulated/encaptulation uniformly. @Michael S. Tsirkin
>   3. Move the links to introduction section. @Michael S. Tsirkin
>   4. Clarify some sentences. @Michael S. Tsirkin
> 
> v4:
>   1. Clarify some paragraphs. @Cornelia Huck
>   2. Fix the u8 type. @Cornelia Huck
> 
> v3:
>   1. Rename VIRTIO_NET_F_HASH_GRE_VXLAN_GENEVE_INNER to 
> VIRTIO_NET_F_HASH_TUNNEL. @Jason Wang
>   2. Make things clearer. @Jason Wang @Michael S. Tsirkin
>   3. Keep the possibility to use inner hash for automatic receive 
> steering. @Jason Wang
>   4. Add the "Tunnel packet" paragraph to avoid repeating the GRE etc. 
> many times. @Michael S. Tsirkin
> 
> v2:
>   1. Add a feature bit for GRE/VXLAN/GENEVE inner hash. @Jason Wang
>   2. Chang \field{hash_tunnel} to \field{hash_report_tunnel}. @Jason 
> Wang, @Michael S. Tsirkin
> 
> v1:
>   1. Remove the patch for the bitmask fix. @Michael S. Tsirkin
>   2. Clarify some paragraphs. @Jason Wang
>   3. Add \field{hash_tunnel} and VIRTIO_NET_HASH_REPORT_GRE. @Yuri 
> Benditovich
> 
>  content.tex  | 191 +--
>  introduction.tex |  19 +
>  2 files changed, 203 insertions(+), 7 deletions(-)
> 
> diff --git a/content.tex b/content.tex
> index e863709..7845f6c 100644
> --- a/content.tex
> +++ b/content.tex
> @@ -3084,6 +3084,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_TUNNEL(52)] Device supports inner
> +header hash for GRE, VXLAN and GENEVE tunnel-encapsulated packets.
> +
>  \item[VIRTIO_NET_F_NOTF_COAL(53)] Device supports notifications coalescing.
>  
>  \item[VIRTIO_NET_F_GUEST_USO4 (54)] Driver can receive USOv4 packets.
> @@ -3095,7 +3098,8 @@ \subsection{Feature bits}\label{sec:Device Types / 
> Network Device / Feature bits
>   to several segments when each of these smaller packets has UDP header.
>  
>  \item[VIRTIO_NET_F_HASH_REPORT(57)] Device can report per-packet hash
> -value and a type of calculated hash.
> +value, a type of calculated hash, and, if VIRTIO_NET_F_HASH_TUNNEL
> +is negotiated, an encapsulation packet type.
>  
>  \item[VIRTIO_NET_F_GUEST_HDRLEN(59)] Driver can provide the exact 
> \field{hdr_len}
>  value. Device benefits from knowing the exact header length.
> @@ -3140,6 +3144,7 @@ \subsubsection{Feature bit 
> requirements}\label{sec:Device Types / Network Device
>  \item[VIRTIO_NET_F_NOTF_COAL] 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_TUNNEL] Requires VIRTIO_NET_F_CTRL_VQ.
>  \end{description}
>  
>  \subsubsection{Legacy Interface: Feature bits}\label{sec:Device Types / 
> Network Device / Feature bits / Legacy Interface: Feature bits}
> @@ -3386,7 +3391,8 @@ \subsection{Device Operation}\label{sec:Device Types / 
> Network Device / Device O
>  le16 num_buffers;
>  le32 hash_value;(Only if VIRTIO_NET_F_HASH_REPORT negotiated)
>  le16 hash_report;   (Only if VIRTIO_NET_F_HASH_REPORT negotiated)
> -le16 padding_reserved;  (Only if VIRTIO_NET_F_HASH_REPORT negotiated)
> +u8 hash_report_tunnel;  (Only if VIRTIO_NET_F_HASH_REPORT 
> negotiated, only valid of VIRTIO_NET_F_HASH_TUNNEL negotiated)

of -> if?

also let's add: otherwise reserved?


> +u8 padding_reserved;(Only if VIRTIO_NET_F_HASH_REPORT negotiated)
>  };
>  \end{lstlisting}
>  
> @@ -3837,7 +3843,8 @@ \subsubsection{Processing of Incoming 
> Packets}\label{sec:Device Types / Network
>  A device attempts to calculate a per-packet hash in the following cases:
>  \begin{itemize}
>  \item The feature VIRTIO_NET_F_RSS was negotiated. The device uses the hash 
> to determine the receive virtqueue to place incoming packets.
> -\item The feature 

[virtio-dev] Re: [PATCH v7] virtio-net: support inner header hash

2023-01-09 Thread Michael S. Tsirkin
On Wed, Jan 04, 2023 at 03:14:01PM +0800, Heng Qi wrote:
> +are defined below:
> +
> +\begin{lstlisting}
> +#define VIRTIO_NET_HASH_REPORT_TUNNEL_NONE 0
> +#define VIRTIO_NET_HASH_REPORT_GRE 1
> +#define VIRTIO_NET_HASH_REPORT_VXLAN   2
> +#define VIRTIO_NET_HASH_REPORT_GENEVE  3
> +\end{lstlisting}

Btw this "are defined below" all over the place is just contributing
to making the spec unnecesarily verbose. Simple "are:" will do.

-- 
MST


-
To unsubscribe, e-mail: virtio-dev-unsubscr...@lists.oasis-open.org
For additional commands, e-mail: virtio-dev-h...@lists.oasis-open.org



[virtio-dev] Re: [PATCH v7] virtio-net: support inner header hash

2023-01-09 Thread Michael S. Tsirkin
On Wed, Jan 04, 2023 at 03:14:01PM +0800, Heng Qi wrote:
> 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.
> 
> We add a feature bit VIRTIO_NET_F_HASH_TUNNEL and related bitmasks
> in \field{hash_types}, which instructs the device to calculate the
> hash using the inner headers of tunnel-encapsulated packets. Besides,
> values in \field{hash_report_tunnel} are added to report tunnel types.
> 
> Fixes: https://github.com/oasis-tcs/virtio-spec/issues/151
> 
> Reviewed-by: Jason Wang 
> Signed-off-by: Heng Qi 
> Signed-off-by: Xuan Zhuo 
> ---
> v6:
>   1. Modify the wording of some sentences for clarity. @Michael S. Tsirkin
>   2. Fix some syntax issues. @Michael S. Tsirkin
> 
> v5:
>   1. Fix some syntax and capitalization issues. @Michael S. Tsirkin
>   2. Use encapsulated/encaptulation uniformly. @Michael S. Tsirkin
>   3. Move the links to introduction section. @Michael S. Tsirkin
>   4. Clarify some sentences. @Michael S. Tsirkin
> 
> v4:
>   1. Clarify some paragraphs. @Cornelia Huck
>   2. Fix the u8 type. @Cornelia Huck
> 
> v3:
>   1. Rename VIRTIO_NET_F_HASH_GRE_VXLAN_GENEVE_INNER to 
> VIRTIO_NET_F_HASH_TUNNEL. @Jason Wang
>   2. Make things clearer. @Jason Wang @Michael S. Tsirkin
>   3. Keep the possibility to use inner hash for automatic receive 
> steering. @Jason Wang
>   4. Add the "Tunnel packet" paragraph to avoid repeating the GRE etc. 
> many times. @Michael S. Tsirkin
> 
> v2:
>   1. Add a feature bit for GRE/VXLAN/GENEVE inner hash. @Jason Wang
>   2. Chang \field{hash_tunnel} to \field{hash_report_tunnel}. @Jason 
> Wang, @Michael S. Tsirkin
> 
> v1:
>   1. Remove the patch for the bitmask fix. @Michael S. Tsirkin
>   2. Clarify some paragraphs. @Jason Wang
>   3. Add \field{hash_tunnel} and VIRTIO_NET_HASH_REPORT_GRE. @Yuri 
> Benditovich
> 
>  content.tex  | 191 +--
>  introduction.tex |  19 +
>  2 files changed, 203 insertions(+), 7 deletions(-)
> 
> diff --git a/content.tex b/content.tex
> index e863709..7845f6c 100644
> --- a/content.tex
> +++ b/content.tex
> @@ -3084,6 +3084,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_TUNNEL(52)] Device supports inner
> +header hash for GRE, VXLAN and GENEVE tunnel-encapsulated packets.
> +
>  \item[VIRTIO_NET_F_NOTF_COAL(53)] Device supports notifications coalescing.
>  
>  \item[VIRTIO_NET_F_GUEST_USO4 (54)] Driver can receive USOv4 packets.
> @@ -3095,7 +3098,8 @@ \subsection{Feature bits}\label{sec:Device Types / 
> Network Device / Feature bits
>   to several segments when each of these smaller packets has UDP header.
>  
>  \item[VIRTIO_NET_F_HASH_REPORT(57)] Device can report per-packet hash
> -value and a type of calculated hash.
> +value, a type of calculated hash, and, if VIRTIO_NET_F_HASH_TUNNEL
> +is negotiated, an encapsulation packet type.
>  
>  \item[VIRTIO_NET_F_GUEST_HDRLEN(59)] Driver can provide the exact 
> \field{hdr_len}
>  value. Device benefits from knowing the exact header length.
> @@ -3140,6 +3144,7 @@ \subsubsection{Feature bit 
> requirements}\label{sec:Device Types / Network Device
>  \item[VIRTIO_NET_F_NOTF_COAL] 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_TUNNEL] Requires VIRTIO_NET_F_CTRL_VQ.
>  \end{description}
>  
>  \subsubsection{Legacy Interface: Feature bits}\label{sec:Device Types / 
> Network Device / Feature bits / Legacy Interface: Feature bits}
> @@ -3386,7 +3391,8 @@ \subsection{Device Operation}\label{sec:Device Types / 
> Network Device / Device O
>  le16 num_buffers;
>  le32 hash_value;(Only if VIRTIO_NET_F_HASH_REPORT negotiated)
>  le16 hash_report;   (Only if VIRTIO_NET_F_HASH_REPORT negotiated)
> -le16 padding_reserved;  (Only if VIRTIO_NET_F_HASH_REPORT negotiated)
> +u8 hash_report_tunnel;  (Only if VIRTIO_NET_F_HASH_REPORT 
> negotiated, only valid of VIRTIO_NET_F_HASH_TUNNEL negotiated)
> +u8 padding_reserved;(Only if VIRTIO_NET_F_HASH_REPORT negotiated)
>  };
>  \end{lstlisting}
>  
> @@ -3837,7 +3843,8 @@ \subsubsection{Processing of Incoming 
> Packets}\label{sec:Device Types / Network
>  A device attempts to calculate a per-packet hash in the following cases:
>  \begin{itemize}
>  \item The feature VIRTIO_NET_F_RSS was negotiated. The device uses the hash 
> to determine the receive virtqueue to place incoming packets.
> -\item The feature VIRTIO_NET_F_HASH_REPORT was negotiated. The device 
> 

[virtio-dev] Re: [PATCH v7] virtio-net: support inner header hash

2023-01-05 Thread Michael S. Tsirkin
On Wed, Jan 04, 2023 at 03:14:01PM +0800, Heng Qi wrote:
> 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.
> 
> We add a feature bit VIRTIO_NET_F_HASH_TUNNEL and related bitmasks
> in \field{hash_types}, which instructs the device to calculate the
> hash using the inner headers of tunnel-encapsulated packets. Besides,
> values in \field{hash_report_tunnel} are added to report tunnel types.
> 
> Fixes: https://github.com/oasis-tcs/virtio-spec/issues/151
> 
> Reviewed-by: Jason Wang 
> Signed-off-by: Heng Qi 
> Signed-off-by: Xuan Zhuo 


ok close to being ready. a couple of minor comments.

> ---
> v6:
>   1. Modify the wording of some sentences for clarity. @Michael S. Tsirkin
>   2. Fix some syntax issues. @Michael S. Tsirkin
> 
> v5:
>   1. Fix some syntax and capitalization issues. @Michael S. Tsirkin
>   2. Use encapsulated/encaptulation uniformly. @Michael S. Tsirkin
>   3. Move the links to introduction section. @Michael S. Tsirkin
>   4. Clarify some sentences. @Michael S. Tsirkin
> 
> v4:
>   1. Clarify some paragraphs. @Cornelia Huck
>   2. Fix the u8 type. @Cornelia Huck
> 
> v3:
>   1. Rename VIRTIO_NET_F_HASH_GRE_VXLAN_GENEVE_INNER to 
> VIRTIO_NET_F_HASH_TUNNEL. @Jason Wang
>   2. Make things clearer. @Jason Wang @Michael S. Tsirkin
>   3. Keep the possibility to use inner hash for automatic receive 
> steering. @Jason Wang
>   4. Add the "Tunnel packet" paragraph to avoid repeating the GRE etc. 
> many times. @Michael S. Tsirkin
> 
> v2:
>   1. Add a feature bit for GRE/VXLAN/GENEVE inner hash. @Jason Wang
>   2. Chang \field{hash_tunnel} to \field{hash_report_tunnel}. @Jason 
> Wang, @Michael S. Tsirkin
> 
> v1:
>   1. Remove the patch for the bitmask fix. @Michael S. Tsirkin
>   2. Clarify some paragraphs. @Jason Wang
>   3. Add \field{hash_tunnel} and VIRTIO_NET_HASH_REPORT_GRE. @Yuri 
> Benditovich
> 
>  content.tex  | 191 +--
>  introduction.tex |  19 +
>  2 files changed, 203 insertions(+), 7 deletions(-)
> 
> diff --git a/content.tex b/content.tex
> index e863709..7845f6c 100644
> --- a/content.tex
> +++ b/content.tex
> @@ -3084,6 +3084,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_TUNNEL(52)] Device supports inner
> +header hash for GRE, VXLAN and GENEVE tunnel-encapsulated packets.

I would probably drop the list of tunnel types here.

> +
>  \item[VIRTIO_NET_F_NOTF_COAL(53)] Device supports notifications coalescing.
>  
>  \item[VIRTIO_NET_F_GUEST_USO4 (54)] Driver can receive USOv4 packets.
> @@ -3095,7 +3098,8 @@ \subsection{Feature bits}\label{sec:Device Types / 
> Network Device / Feature bits
>   to several segments when each of these smaller packets has UDP header.
>  
>  \item[VIRTIO_NET_F_HASH_REPORT(57)] Device can report per-packet hash
> -value and a type of calculated hash.
> +value, a type of calculated hash, and, if VIRTIO_NET_F_HASH_TUNNEL
> +is negotiated, an encapsulation packet type.
>  
>  \item[VIRTIO_NET_F_GUEST_HDRLEN(59)] Driver can provide the exact 
> \field{hdr_len}
>  value. Device benefits from knowing the exact header length.
> @@ -3140,6 +3144,7 @@ \subsubsection{Feature bit 
> requirements}\label{sec:Device Types / Network Device
>  \item[VIRTIO_NET_F_NOTF_COAL] 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_TUNNEL] Requires VIRTIO_NET_F_CTRL_VQ.
>  \end{description}
>  
>  \subsubsection{Legacy Interface: Feature bits}\label{sec:Device Types / 
> Network Device / Feature bits / Legacy Interface: Feature bits}
> @@ -3386,7 +3391,8 @@ \subsection{Device Operation}\label{sec:Device Types / 
> Network Device / Device O
>  le16 num_buffers;
>  le32 hash_value;(Only if VIRTIO_NET_F_HASH_REPORT negotiated)
>  le16 hash_report;   (Only if VIRTIO_NET_F_HASH_REPORT negotiated)
> -le16 padding_reserved;  (Only if VIRTIO_NET_F_HASH_REPORT negotiated)
> +u8 hash_report_tunnel;  (Only if VIRTIO_NET_F_HASH_REPORT 
> negotiated, only valid of VIRTIO_NET_F_HASH_TUNNEL negotiated)
> +u8 padding_reserved;(Only if VIRTIO_NET_F_HASH_REPORT negotiated)
>  };
>  \end{lstlisting}
>  
> @@ -3837,7 +3843,8 @@ \subsubsection{Processing of Incoming 
> Packets}\label{sec:Device Types / Network
>  A device attempts to calculate a per-packet hash in the following cases:
>  \begin{itemize}
>  \item The feature VIRTIO_NET_F_RSS was negotiated. The device uses the hash 
> to determine the receive