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

[Parav Pandit <parav@nvidia.com>]

On 3/6/2023 10:48 AM, Heng Qi wrote:

>   
> +\item[VIRTIO_NET_F_HASH_TUNNEL(52)] Device supports inner header hash
May be to say inner packet header hash..
This make its little more clear about "which header" that you explained 
in the commit log.

> +    for 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.
> @@ -139,6 +142,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.
I think this should also say that HASH_TUNNEL requires either of the 
F_RSS or F_HASH_REPORT.
Because without it HASH_TUNNEL is not useful.
Right?
if no, than my below comments are meaningless.


>   \end{description}
>   
>   \subsubsection{Legacy Interface: Feature bits}\label{sec:Device Types / Network Device / Feature bits / Legacy Interface: Feature bits}
> @@ -198,20 +202,27 @@ \subsection{Device configuration layout}\label{sec:Device Types / Network Device
>           u8 rss_max_key_size;
>           le16 rss_max_indirection_table_length;
>           le32 supported_hash_types;
> +        le32 supported_tunnel_hash_types;
>   };
>   \end{lstlisting}
> -The following field, \field{rss_max_key_size} only exists if VIRTIO_NET_F_RSS or VIRTIO_NET_F_HASH_REPORT is set.
> +The following field, \field{rss_max_key_size} only exists if VIRTIO_NET_F_RSS, VIRTIO_NET_F_HASH_REPORT or VIRTIO_NET_F_HASH_TUNNEL is set.
>   It specifies the maximum supported length of RSS key in bytes.
>  
I think rss_max_key_size field dependency should be only of the existing 
feature bits F_RSS and F_HASH_REPORT.
This is because those are the bits really deciding to consider 
rss_max_key_size.

>   The following field, \field{rss_max_indirection_table_length} only exists if VIRTIO_NET_F_RSS is set.
>   It specifies the maximum number of 16-bit entries in RSS indirection table.
>   
>   The next field, \field{supported_hash_types} only exists if the device supports hash calculation,
> -i.e. if VIRTIO_NET_F_RSS or VIRTIO_NET_F_HASH_REPORT is set.
> +i.e. if VIRTIO_NET_F_RSS, VIRTIO_NET_F_HASH_REPORT or VIRTIO_NET_F_HASH_TUNNEL is set.
>  
Same as above.

>   Field \field{supported_hash_types} contains the bitmask of supported hash types.
>   See \ref{sec:Device Types / Network Device / Device Operation / Processing of Incoming Packets / Hash calculation for incoming packets / Supported/enabled hash types} for details of supported hash types.
>   
> +The next field, \field{supported_tunnel_hash_types} only exists if the device
> +supports inner hash calculation, i.e. if VIRTIO_NET_F_HASH_TUNNEL is set.
> +
Above line "the next field .." can be just same as "Device supports 
inner packet header hash calculation, i.e..."
This is because here, the term "header" is missed, which is present in 
the definition of feature bit 52.

>   
>   The device MUST set \field{rss_max_key_size} to at least 40, if it offers
> -VIRTIO_NET_F_RSS or VIRTIO_NET_F_HASH_REPORT.
> +VIRTIO_NET_F_RSS, VIRTIO_NET_F_HASH_REPORT or VIRTIO_NET_F_HASH_TUNNEL.
>   
This needs change if the above first comment about rss_max_key_size is 
right.

>   The device MUST set \field{rss_max_indirection_table_length} to at least 128, if it offers
>   VIRTIO_NET_F_RSS.
> @@ -843,11 +854,13 @@ \subsubsection{Processing of Incoming Packets}\label{sec:Device Types / Network
>   \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 reports the hash value and the hash type with the packet.
> +\item The feature VIRTIO_NET_F_HASH_TUNNEL was negotiated. The device supports inner hash calculation.
>   \end{itemize}
>  
inner packet header hash ..

> +If the feature VIRTIO_NET_F_HASH_TUNNEL is negotiated, the encapsulation
> +hash type below indicates that the hash is calculated over the inner header of
> +the encapsulated packet:
> +Hash type applicable for inner payload of the gre-encapsulated packet
> +\begin{lstlisting}
> +#define VIRTIO_NET_HASH_TUNNEL_TYPE_GRE         (1 << 1)
> +\end{lstlisting}
> +Hash type applicable for inner payload of the vxlan-encapsulated packet
> +\begin{lstlisting}
> +#define VIRTIO_NET_HASH_TUNNEL_TYPE_VXLAN       (1 << 2)
> +\end{lstlisting}
> +Hash type applicable for inner payload of the geneve-encapsulated packet
> +\begin{lstlisting}
> +#define VIRTIO_NET_HASH_TUNNEL_TYPE_GENEVE      (1 << 3)
> +\end{lstlisting}
> +Hash type applicable for inner payload of the ip-encapsulated packet
> +\begin{lstlisting}
> +#define VIRTIO_NET_HASH_TUNNEL_TYPE_IPIP        (1 << 4)
> +\end{lstlisting}
> +Hash type applicable for inner payload of the nvgre-encapsulated packet
> +\begin{lstlisting}
> +#define VIRTIO_NET_HASH_TUNNEL_TYPE_NVGRE       (1 << 5)
> +\end{lstlisting}
> +
Any encapsulation technology that includes UDP/L4 header likely do not 
prefer based on the inner header. This is because the outer header src 
port entropy is added based on the inner header.

I was not able to follow the discussion in v9 that you had with Michael.
Did you conclude if this is needed for vxlan too?

If not, for now it may be better to skip vxlan and nvegre as they 
inherently have unique outer header UDP src port based on the inner header.

>   \subparagraph{IPv4 packets}
>   \label{sec:Device Types / Network Device / Device Operation / Processing of Incoming Packets / Hash calculation for incoming packets / IPv4 packets}
>   The device calculates the hash on IPv4 packets according to 'Enabled hash types' bitmask as follows:
> @@ -980,6 +1045,44 @@ \subsubsection{Processing of Incoming Packets}\label{sec:Device Types / Network
>   (see \ref{sec:Device Types / Network Device / Device Operation / Processing of Incoming Packets / Hash calculation for incoming packets / IPv6 packets without extension header}).
>   \end{itemize}
>   
> +\subparagraph{Inner hash calculation of an encapsulated packet}
> +If the driver negotiates the VIRTIO_NET_F_HASH_TUNNEL feature, it can configure the
> +hash parameters (including \field{hash_tunnel_types}) for inner hash calculation by
> +sending the VIRTIO_NET_CTRL_MQ_HASH_CONFIG command. Additionally, if the VIRTIO_NET_F_RSS
> +feature is also negotiated, the driver can use the VIRTIO_NET_CTRL_RSS_CONFIG command to
> +configure the hash parameters. If multiple commands are sent, the device configuration
> +will be defined by the last command received.
> +
> +If the feature VIRTIO_NET_F_HASH_TUNNEL is negotiated and the corresponding
> +encapsulation hash type is set in \field{hash_tunnel_types}, the device calculates the
> +hash on the inner header of an encapsulated packet (See \ref{sec:Device Types
> +/ Network Device / Device Operation / Processing of Incoming Packets /
> +Hash calculation for incoming packets / Tunnel/Encapsulated packet}). If the encapsulation
> +type of an encapsulated packet is not included in \field{hash_tunnel_types} or the value
> +of \field{hash_tunnel_types} is VIRTIO_NET_HASH_TUNNEL_TYPE_NONE, the device calculates
> +the hash on the outer header.
> +
> +\field{hash_tunnel_types} is set to VIRTIO_NET_HASH_TUNNEL_TYPE_NONE by the device for the
> +unencapsulated packets.
> +
> +\subparagraph{Tunnel QoS limitation}
> +Note that the limitation mentioned below is not only introduced by inner hash calculation,
> +and the limitation of the tunnel itself, and even the driver may have only one receive queue.
> +
When there is a single receive queue, there is no tunnel QoS issue. 
Because the same queue is used one or more tunnels.
So please remove mentioning single queue text here.

Also it is better to first describe the limitation and after the reader 
understand, it make sense to say that the limitation already exists 
with/without inner header hash calculation.

> +When a specific receive queue is shared by multiple tunnels to receive encapsulating packets,
> +there is no quality of service (QoS) for these packets of multiple tunnels. 
"of multiple tunnels at the tail of sentence is not needed because you 
already have it at "shared by multiple tunnels".

> For example, when the
> +flooded packets of a certain tunnel are hashed to the queue, it may cause the traffic of this
> +queue to be unbalanced, resulting in potential packet loss and data delay.
> +
Your example is only talking about single queue..

You probably wanted to say,

When the packets of certain tunnels results in spreading these packets 
to multiple receive queues, these receive queues may have unbalanced 
amount of packets. This can result in a specific receive queue being 
full resulting in packet loss.

Or something similar..

> +Possible mitigations:
> +\begin{itemize}
> +\item Use a tool with good forwarding performance such as DPDK to keep the queue from filling up.
I dont think it is necessary to talk about specific tools like DPDK in 
specification.
Even if you omit "such as DPDK", the line reads fine.
so I suggest to drop "such as DPDK".

> +\item If the quality of service is unavailable, the driver can set \field{hash_tunnel_types} to
> +      VIRTIO_NET_HASH_TUNNEL_TYPE_NONE to disable inner hash calculation for encapsulated packets.
> +\item Choose a hash key that can avoid queue collisions.
> +\item Outside the device, prevent abnormal traffic from entering or switch the traffic to attack clusters.
> +\end{itemize}
It can still enter the switch but you want to avoid entering the virtio 
device.
Cluster is very broad term and not defined in the spec, better to avoid it.

you can rewrite it something like, Perform appropriate QoS before 
packets consume the receive buffers..

This is generic solution that can be done in device or egress of switch 
etc, which keep the spec scope upto the device.

> +
>   \paragraph{Hash reporting for incoming packets}
>   \label{sec:Device Types / Network Device / Device Operation / Processing of Incoming Packets / Hash reporting for incoming packets}
>   
> @@ -1392,12 +1495,17 @@ \subsubsection{Control Virtqueue}\label{sec:Device Types / Network Device / Devi
>       le16 reserved[4];
>       u8 hash_key_length;
>       u8 hash_key_data[hash_key_length];
> +    le32 hash_tunnel_types;
>   };
>   \end{lstlisting}
>   Field \field{hash_types} contains a bitmask of allowed hash types as
>   defined in
>   \ref{sec:Device Types / Network Device / Device Operation / Processing of Incoming Packets / Hash calculation for incoming packets / Supported/enabled hash types}.
> -Initially the device has all hash types disabled and reports only VIRTIO_NET_HASH_REPORT_NONE.
> +
> +Field \field{hash_tunnel_types} contains a bitmask of allowed hash tunnel types as
> +defined in \ref{sec:Device Types / Network Device / Device Operation / Processing of Incoming Packets / Hash calculation for incoming packets / Supported/enabled hash tunnel types}.
> +
> +Initially the device has all hash types and hash tunnel types disabled and reports only VIRTIO_NET_HASH_REPORT_NONE.
>   
>   Field \field{reserved} MUST contain zeroes. It is defined to make the structure to match the layout of virtio_net_rss_config structure,
>   defined in \ref{sec:Device Types / Network Device / Device Operation / Control Virtqueue / Receive-side scaling (RSS)}.
> @@ -1421,6 +1529,7 @@ \subsubsection{Control Virtqueue}\label{sec:Device Types / Network Device / Devi
>       le16 max_tx_vq;
>       u8 hash_key_length;
>       u8 hash_key_data[hash_key_length];
> +    le32 hash_tunnel_types;
>   };
>   \end{lstlisting}
>   Field \field{hash_types} contains a bitmask of allowed hash types as
> @@ -1441,6 +1550,9 @@ \subsubsection{Control Virtqueue}\label{sec:Device Types / Network Device / Devi
>   
>   Fields \field{hash_key_length} and \field{hash_key_data} define the key to be used in hash calculation.
>   
> +Field \field{hash_tunnel_types} contains a bitmask of allowed hash tunnel types as
> +defined in \ref{sec:Device Types / Network Device / Device Operation / Processing of Incoming Packets / Hash calculation for incoming packets / Supported/enabled hash tunnel types}.
> +
>   \drivernormative{\subparagraph}{Setting RSS parameters}{Device Types / Network Device / Device Operation / Control Virtqueue / Receive-side scaling (RSS) }
>   
You likely need to add device and driver normative statements derived 
from above text and add their references into
device-types/net/*-conformance.tex files.

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