[virtio-dev] [PATCH v2 0/2] Introducing RSS virtio-net

[virtio-dev] [PATCH v2 0/2] Introducing RSS virtio-net

[Sameeh Jubran]

From: Sameeh Jubran <sjubran@redhat.com>

Hi all,

This patch series introduces an infrastructure into virtio-net for
adding steering modes while introducing RSS as one.

Please take a look and share your thoughts.

Difference from v1:

* generic steering mode introduced and RSS is defined as part of the steering
  feature by Jason request

v1:
https://lists.oasis-open.org/archives/virtio-dev/201801/msg00177.html

Sameeh Jubran (2):
  content: net: Add VIRTIO_NET_F_CTRL_STEERING_MODE feature
  content: net: steering mode: Add RSS

 content.tex | 173 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 173 insertions(+)

-- 
2.13.6


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

[virtio-dev] [PATCH v2 1/2] content: net: Add VIRTIO_NET_F_CTRL_STEERING_MODE feature

[Sameeh Jubran]

From: Sameeh Jubran <sameeh.j@gmail.com>

This commit introduces steering mode into network device. Steering
mode is a general infrastructure for various steering modes that can
be implemented on top of it such as Automatic and RSS.

Signed-off-by: Sameeh Jubran <sjubran@redhat.com>
---
 content.tex | 59 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 59 insertions(+)

diff --git a/content.tex b/content.tex
index c840588..3d538e8 100644
--- a/content.tex
+++ b/content.tex
@@ -3115,6 +3115,9 @@ features.
 
 \item[VIRTIO_NET_F_CTRL_MAC_ADDR(23)] Set MAC address through control
     channel.
+
+\item[VIRTIO_NET_F_CTRL_STEERING_MODE(27)] Device supports configurable steering
+    mode.
 \end{description}
 
 \subsubsection{Feature bit requirements}\label{sec:Device Types / Network Device / Feature bits / Feature bit requirements}
@@ -3317,6 +3320,8 @@ struct virtio_net_hdr {
         le16 csum_start;
         le16 csum_offset;
         le16 num_buffers;
+// Only if VIRTIO_NET_F_CTRL_STEERING_MODE has been negotiated
+        le64 hash;
 };
 \end{lstlisting}
 
@@ -4007,6 +4012,60 @@ VIRTIO_NET_CTRL_MQ_VQ_PAIRS_SET command.
 The device MUST NOT queue packets on receive queues greater than
 \field{virtqueue_pairs} once it has placed the VIRTIO_NET_CTRL_MQ_VQ_PAIRS_SET command in the used ring.
 
+\paragraph{Steering mode}\label{sec:Device Types / Network Device / Device Operation / Control Virtqueue / Steering mode}
+
+\begin{lstlisting}
+// steering mode flags
+#define STEERING_MODE_AUTO          1
+
+struct virtio_net_steering_modes {
+le32 steering_modes;
+};
+
+struct virtio_net_steering_mode {
+le32 steering_mode;
+le32 command;
+
+    union {
+    }
+};
+
+#define VIRTIO_NET_F_CTRL_STEERING_MODE     27
+
+#define VIRTIO_NET_CTRL_STEERING_MODE                7
+#define VIRTIO_NET_CTRL_SM_GET_SUPPORTED_MODES        0
+#define VIRTIO_NET_CTRL_SM_CONTROL                    1
+\end{lstlisting}
+
+If the VIRTIO_NET_F_CTRL_STEERING_MODE is negotiated the driver can send control commands for the
+configuring the steering mode. There are multiple steering modes and they can be configured using
+the VIRTIO_NET_CTRL_SM_CONTROL command. Each mode has it's own set of commands.
+
+The auto steering mode is the default mode if nothing else has been configured by the driver
+and the VIRTIO_NET_F_CTRL_STEERING_MODE feature is acked by the driver.
+
+The class VIRTIO_NET_CTRL_STEERING_MODE has two commands:
+
+\begin{enumerate}
+
+\item VIRTIO_NET_CTRL_SM_GET_SUPPORTED_MODES is used for getting the supported steering modes by the device.
+	The command should be executed by the driver before attempting to configure the steering mode. Once the device
+	receives this command it should fill the virtio_net_steering_modes structure with the supported steering mode
+	flags.
+
+\item The command VIRTIO_NET_CTRL_SM_CONTROL is used for controlling the different steering modes. Each steering mode
+	has its own set of commands. When executing this command the structure virtio_net_steering_mode should be used as follows:
+	the \field{steering_mode} should be filled with one of the steering mode flags along with an appropriate command from the chosen
+	steering mode. The union of virtio_net_steering_mode should be used and filled as the mode's command suggests.
+\end{enumerate}
+
+If this feature has been negotiated, the virtio header has an additional
+\field{hash} field attached to it.
+
+\subparagraph{Automatic Receive Steering}{Device Types / Network Device / Device Operation / Control Virtqueue / Steering mode / Automatic Receive Steering}
+
+This is the default steering mode, please refer to the "Automatic receive steering in multiqueue" section.
+
 \subparagraph{Legacy Interface: Automatic receive steering in multiqueue mode}\label{sec:Device Types / Network Device / Device Operation / Control Virtqueue / Automatic receive steering in multiqueue mode / Legacy Interface: Automatic receive steering in multiqueue mode}
 When using the legacy interface, transitional devices and drivers
 MUST format \field{virtqueue_pairs}
-- 
2.13.6


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

[virtio-dev] [PATCH v2 2/2] content: net: steering mode: Add RSS

[Sameeh Jubran]

From: Sameeh Jubran <sjubran@redhat.com>

This commit introduces the RSS feature into virtio-net. It is introduced
as a sub mode for a general command which configures the steering mode.

Most modern high end network devices today support configurable hash functions,
this commit introduces RSS - Receive Side Scaling - [1] to virtio net device.

The RSS is a technology from Microsoft that boosts network device performance
by efficiently distributing the traffic among the CPUs in a multiprocessor
system.

This feature is supported in most of the modern network cards as well as most
modern OSes including Linux and Windows. It is worth mentioning that both DPDK
and Hyper-v support RSS too.

[1] https://docs.microsoft.com/en-us/windows-hardware/drivers/network/ndis-receive-side-scaling2

Signed-off-by: Sameeh Jubran <sjubran@redhat.com>
---
 content.tex | 114 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 114 insertions(+)

diff --git a/content.tex b/content.tex
index 3d538e8..8076147 100644
--- a/content.tex
+++ b/content.tex
@@ -4017,6 +4017,7 @@ The device MUST NOT queue packets on receive queues greater than
 \begin{lstlisting}
 // steering mode flags
 #define STEERING_MODE_AUTO          1
+#define STEERING_MODE_RSS           2
 
 struct virtio_net_steering_modes {
 le32 steering_modes;
@@ -4027,6 +4028,7 @@ le32 steering_mode;
 le32 command;
 
     union {
+    struct virtio_net_rss rss_conf;
     }
 };
 
@@ -4066,6 +4068,118 @@ If this feature has been negotiated, the virtio header has an additional
 
 This is the default steering mode, please refer to the "Automatic receive steering in multiqueue" section.
 
+\subparagraph{Receive Side Scaling}{Device Types / Network Device / Device Operation / Control Virtqueue / Steering mode / Receive Side Scaling}
+
+\begin{lstlisting}
+#define RSS_HASH_FUNCTION_NONE      1
+#define RSS_HASH_FUNCTION_TOEPLITZ  2
+#define RSS_HASH_FUNCTION_SYMMETRIC 3
+
+// Hash function fields
+#define RSS_HASH_FIELDS_IPV4          0x00000100
+#define RSS_HASH_FIELDS_TCP_IPV4      0x00000200
+#define RSS_HASH_FIELDS_IPV6          0x00000400
+#define RSS_HASH_FIELDS_IPV6_EX       0x00000800
+#define RSS_HASH_FIELDS_TCP_IPV6      0x00001000
+#define RSS_HASH_FIELDS_TCP_IPV6_EX   0x00002000
+
+struct virtio_net_rss_supported_hash{
+le32 hash_function;
+}
+
+struct virtio_net_rss {
+le32 hash_function;
+le32 hash_function_flags;
+le32 hash_key_length;
+le32 indirection_table_length;
+	struct {
+		le32 hash_key[hash_key_length];
+		le32 indirection_table[indirection_table_length];
+	}
+};
+
+#define VIRTIO_NET_SM_CTRL_RSS_GET_SUPPORTED_FUNCTIONS   0
+#define VIRTIO_NET_SM_CTRL_RSS_SET                       1
+\end{lstlisting}
+
+If the VIRTIO_NET_F_CTRL_STEERING_MODE is negotiated the driver can send control
+commands for the RSS configuration. For configuring RSS the virtio_net_steering_mode
+should be filled. The \field{steering_mode} field should be filled with the STEERING_MODE_RSS
+flag along with one of the VIRTIO_NET_SM_CTRL_RSS commands in the \field{command} field. The
+\field{rss_conf} field should be used.
+
+The class VIRTIO_NET_CTRL_RSS has two commands:
+
+\begin{enumerate}
+\item VIRTIO_NET_SM_CTRL_RSS_GET_SUPPORTED_FUNCTIONS returns the hash functions
+	supported by the device to the driver.
+\item VIRTIO_NET_SM_CTRL_RSS_SET applies the new RSS configuration. The command is
+	used by the driver for setting RSS hash function, hash key and
+	indirection table in the device.
+\end{enumerate}
+
+\devicenormative{\subparagraph}{Receive Side Scaling}{Device Types / Network Device / Device Operation / Control Virtqueue / Steering mode / Receive Side Scaling}
+
+The device MUST fill the virtio_net_rss_supported_hash structure with the hash
+functions it supports and return the structure to the driver. Zero or more
+flags of the RSS_HASH_FUNCTION flags MUST be used to fill the \field{hash_function}
+field.
+
+The device MUST drop all previous RSS configuration upon receiving
+VIRTIO_NET_SM_CTRL_RSS_SET command.
+
+The device MUST set the RSS configuration according to the settings provided as
+follows, once the configuration process is completed the device SHOULD apply
+the hash function to each of the incoming packets and distribute the packets
+through the virqueues using the calculated hash and the indirection table
+that were earlier provided by the driver.
+
+Setting RSS configuration
+\begin{enumerate}
+\item The driver fills all of the fields and passes them through the control
+	queue to the device.
+
+\item The device sets the RSS configuration as provided by the driver.
+
+\item If the device successfully applied the configuration, on each packet
+	received the device MUST calculate the hashing for the packet and
+	store it in the virtio-net header in the \field{hash} field and the
+	hash fields used in the calculation in rss_hash_type.
+\end{enumerate}
+
+In case of any unexpected values/ unsupported hash function the driver
+MUST return VIRTIO_NET_ERR in the \field{ack} field.
+
+\drivernormative{\subparagraph}{Receive Side Scaling}{Device Types / Network Device / Device Operation / Control Virtqueue / Steering mode / Receive Side Scaling}
+
+If the driver wants to set RSS hash it should fill the RSS structure fields
+as follows:
+
+\begin{itemize}
+\item The driver SHOULD choose the hash function that SHOULD be used and fill
+	it in the \field{hash_function} field along with the appropriate flags
+	in the \field{hash_function_flags} field. These flags indicate to the
+	device which packet fields MUST be used in the calculation process of
+	the hash.
+\item Once the hash function has been chosen a suitable hash key should be set
+	in the \field{hash_key} field, the length of the key should be stored
+	in the \field{hash_key_length} field.
+\item Lastly the driver should fill the indirection table array in the
+	\field{indirection_table} field while setting the array length in
+	\field{indirection_table_length}. This structure is used by the device
+	for determining in which RX virt queue the packet should be placed.
+\end{itemize}
+Once the configuration phase is over successfully, the packets SHOULD have the
+\field{hash} field with the hash value that was calculated by the device.
+
+Whenever the driver wants to discard the current RSS settings, it can send an
+VIRTIO_NET_SM_CTRL_RSS_SET command along with rss structure that has
+RSS_HASH_FUNCTION_NONE the \field{hash_function} field.
+
+The driver MUST check the \field{ack} field value provided by the device, in
+case the value is not VIRTIO_NET_OK then the driver MUST handle failure and
+retry another hash function or else give up.
+
 \subparagraph{Legacy Interface: Automatic receive steering in multiqueue mode}\label{sec:Device Types / Network Device / Device Operation / Control Virtqueue / Automatic receive steering in multiqueue mode / Legacy Interface: Automatic receive steering in multiqueue mode}
 When using the legacy interface, transitional devices and drivers
 MUST format \field{virtqueue_pairs}
-- 
2.13.6


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

Re: [virtio-dev] [PATCH v2 1/2] content: net: Add VIRTIO_NET_F_CTRL_STEERING_MODE feature

[Michael S. Tsirkin]

On Mon, Apr 16, 2018 at 04:05:25PM +0300, Sameeh Jubran wrote:
> From: Sameeh Jubran <sameeh.j@gmail.com>
> 
> This commit introduces steering mode into network device. Steering
> mode is a general infrastructure for various steering modes that can
> be implemented on top of it such as Automatic and RSS.
> 
> Signed-off-by: Sameeh Jubran <sjubran@redhat.com>
> ---
>  content.tex | 59 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 59 insertions(+)
> 
> diff --git a/content.tex b/content.tex
> index c840588..3d538e8 100644
> --- a/content.tex
> +++ b/content.tex
> @@ -3115,6 +3115,9 @@ features.
>  
>  \item[VIRTIO_NET_F_CTRL_MAC_ADDR(23)] Set MAC address through control
>      channel.
> +
> +\item[VIRTIO_NET_F_CTRL_STEERING_MODE(27)] Device supports configurable steering
> +    mode.
>  \end{description}
>  
>  \subsubsection{Feature bit requirements}\label{sec:Device Types / Network Device / Feature bits / Feature bit requirements}
> @@ -3317,6 +3320,8 @@ struct virtio_net_hdr {
>          le16 csum_start;
>          le16 csum_offset;
>          le16 num_buffers;
> +// Only if VIRTIO_NET_F_CTRL_STEERING_MODE has been negotiated
> +        le64 hash;
>  };
>  \end{lstlisting}
>  
> @@ -4007,6 +4012,60 @@ VIRTIO_NET_CTRL_MQ_VQ_PAIRS_SET command.
>  The device MUST NOT queue packets on receive queues greater than
>  \field{virtqueue_pairs} once it has placed the VIRTIO_NET_CTRL_MQ_VQ_PAIRS_SET command in the used ring.
>  
> +\paragraph{Steering mode}\label{sec:Device Types / Network Device / Device Operation / Control Virtqueue / Steering mode}
> +
> +\begin{lstlisting}
> +// steering mode flags
> +#define STEERING_MODE_AUTO          1
> +
> +struct virtio_net_steering_modes {
> +le32 steering_modes;
> +};
> +
> +struct virtio_net_steering_mode {
> +le32 steering_mode;
> +le32 command;
> +
> +    union {
> +    }
> +};
> +
> +#define VIRTIO_NET_F_CTRL_STEERING_MODE     27

Please use a high number (63 and down). We are running out of
low numbers it is best to keep them for things we must backport
to old virtio - such as mtu - which are required for the device
working properly.

Pls coordinate with Vlad (cc'd) who is adding a new feature too.

> +
> +#define VIRTIO_NET_CTRL_STEERING_MODE                7
> +#define VIRTIO_NET_CTRL_SM_GET_SUPPORTED_MODES        0
> +#define VIRTIO_NET_CTRL_SM_CONTROL                    1
> +\end{lstlisting}
> +
> +If the VIRTIO_NET_F_CTRL_STEERING_MODE is negotiated the driver can send control commands for the
> +configuring the steering mode. There are multiple steering modes and they can be configured using
> +the VIRTIO_NET_CTRL_SM_CONTROL command. Each mode has it's own set of commands.
> +
> +The auto steering mode is the default mode if nothing else has been configured by the driver
> +and the VIRTIO_NET_F_CTRL_STEERING_MODE feature is acked by the driver.
> +
> +The class VIRTIO_NET_CTRL_STEERING_MODE has two commands:
> +
> +\begin{enumerate}
> +
> +\item VIRTIO_NET_CTRL_SM_GET_SUPPORTED_MODES is used for getting the supported steering modes by the device.
> +	The command should be executed by the driver before attempting to configure the steering mode. Once the device
> +	receives this command it should fill the virtio_net_steering_modes structure with the supported steering mode
> +	flags.
> +
> +\item The command VIRTIO_NET_CTRL_SM_CONTROL is used for controlling the different steering modes. Each steering mode
> +	has its own set of commands. When executing this command the structure virtio_net_steering_mode should be used as follows:
> +	the \field{steering_mode} should be filled with one of the steering mode flags along with an appropriate command from the chosen
> +	steering mode. The union of virtio_net_steering_mode should be used and filled as the mode's command suggests.
> +\end{enumerate}
> +
> +If this feature has been negotiated, the virtio header
> has an additional
> +\field{hash} field attached to it.
> +
> +\subparagraph{Automatic Receive Steering}{Device Types / Network Device / Device Operation / Control Virtqueue / Steering mode / Automatic Receive Steering}
> +
> +This is the default steering mode, please refer to the "Automatic receive steering in multiqueue" section.
> +
>  \subparagraph{Legacy Interface: Automatic receive steering in multiqueue mode}\label{sec:Device Types / Network Device / Device Operation / Control Virtqueue / Automatic receive steering in multiqueue mode / Legacy Interface: Automatic receive steering in multiqueue mode}
>  When using the legacy interface, transitional devices and drivers
>  MUST format \field{virtqueue_pairs}

Please add each part where it belongs, e.g. a feature bit with other
feature bits, header field with other header fields, etc.

I think Vlad tested extending the header and found it adds
non-negligeable performance overhead. Is it worth splitting
out hash calculation? Is hash used for RX path only?


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

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

[virtio-dev] Re: [PATCH v2 2/2] content: net: steering mode: Add RSS

[Vijayabhaskar Balakrishna]

On 4/16/2018 6:05 AM, Sameeh Jubran wrote:
> From: Sameeh Jubran <sjubran@redhat.com>
>
> This commit introduces the RSS feature into virtio-net. It is introduced
> as a sub mode for a general command which configures the steering mode.
>
> Most modern high end network devices today support configurable hash functions,
> this commit introduces RSS - Receive Side Scaling - [1] to virtio net device.
>
> The RSS is a technology from Microsoft that boosts network device performance
> by efficiently distributing the traffic among the CPUs in a multiprocessor
> system.
>
> This feature is supported in most of the modern network cards as well as most
> modern OSes including Linux and Windows. It is worth mentioning that both DPDK
> and Hyper-v support RSS too.
>
> [1] https://docs.microsoft.com/en-us/windows-hardware/drivers/network/ndis-receive-side-scaling2
>
> Signed-off-by: Sameeh Jubran <sjubran@redhat.com>
> ---
>   content.tex | 114 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
>   1 file changed, 114 insertions(+)
>
> diff --git a/content.tex b/content.tex
> index 3d538e8..8076147 100644
> --- a/content.tex
> +++ b/content.tex
> @@ -4017,6 +4017,7 @@ The device MUST NOT queue packets on receive queues greater than
>   \begin{lstlisting}
>   // steering mode flags
>   #define STEERING_MODE_AUTO          1
> +#define STEERING_MODE_RSS           2
>   
>   struct virtio_net_steering_modes {
>   le32 steering_modes;
> @@ -4027,6 +4028,7 @@ le32 steering_mode;
>   le32 command;
>   
>       union {
> +    struct virtio_net_rss rss_conf;
>       }
>   };
>   
> @@ -4066,6 +4068,118 @@ If this feature has been negotiated, the virtio header has an additional
>   
>   This is the default steering mode, please refer to the "Automatic receive steering in multiqueue" section.
>   
> +\subparagraph{Receive Side Scaling}{Device Types / Network Device / Device Operation / Control Virtqueue / Steering mode / Receive Side Scaling}
> +
> +\begin{lstlisting}
> +#define RSS_HASH_FUNCTION_NONE      1
> +#define RSS_HASH_FUNCTION_TOEPLITZ  2
> +#define RSS_HASH_FUNCTION_SYMMETRIC 3
Above are bit fields?  Should it be 0x1, 0x2, 0x4 etc.,
> +
> +// Hash function fields
> +#define RSS_HASH_FIELDS_IPV4          0x00000100
> +#define RSS_HASH_FIELDS_TCP_IPV4      0x00000200
> +#define RSS_HASH_FIELDS_IPV6          0x00000400
> +#define RSS_HASH_FIELDS_IPV6_EX       0x00000800
> +#define RSS_HASH_FIELDS_TCP_IPV6      0x00001000
> +#define RSS_HASH_FIELDS_TCP_IPV6_EX   0x00002000
> +
> +struct virtio_net_rss_supported_hash{
> +le32 hash_function;
> +}
> +
> +struct virtio_net_rss {
> +le32 hash_function;
> +le32 hash_function_flags;
> +le32 hash_key_length;
> +le32 indirection_table_length;
> +	struct {
> +		le32 hash_key[hash_key_length];
> +		le32 indirection_table[indirection_table_length];
> +	}
> +};
> +
> +#define VIRTIO_NET_SM_CTRL_RSS_GET_SUPPORTED_FUNCTIONS   0
> +#define VIRTIO_NET_SM_CTRL_RSS_SET                       1
> +\end{lstlisting}
> +
> +If the VIRTIO_NET_F_CTRL_STEERING_MODE is negotiated the driver can send control
> +commands for the RSS configuration. For configuring RSS the virtio_net_steering_mode
> +should be filled. The \field{steering_mode} field should be filled with the STEERING_MODE_RSS
> +flag along with one of the VIRTIO_NET_SM_CTRL_RSS commands in the \field{command} field. The
> +\field{rss_conf} field should be used.
> +
> +The class VIRTIO_NET_CTRL_RSS has two commands:
> +
> +\begin{enumerate}
> +\item VIRTIO_NET_SM_CTRL_RSS_GET_SUPPORTED_FUNCTIONS returns the hash functions
> +	supported by the device to the driver.
> +\item VIRTIO_NET_SM_CTRL_RSS_SET applies the new RSS configuration. The command is
> +	used by the driver for setting RSS hash function, hash key and
> +	indirection table in the device.
> +\end{enumerate}
> +
> +\devicenormative{\subparagraph}{Receive Side Scaling}{Device Types / Network Device / Device Operation / Control Virtqueue / Steering mode / Receive Side Scaling}
> +
> +The device MUST fill the virtio_net_rss_supported_hash structure with the hash
> +functions it supports and return the structure to the driver. Zero or more
> +flags of the RSS_HASH_FUNCTION flags MUST be used to fill the \field{hash_function}
> +field.
> +
> +The device MUST drop all previous RSS configuration upon receiving
> +VIRTIO_NET_SM_CTRL_RSS_SET command.
> +
> +The device MUST set the RSS configuration according to the settings provided as
> +follows, once the configuration process is completed the device SHOULD apply
> +the hash function to each of the incoming packets and distribute the packets
> +through the virqueues using the calculated hash and the indirection table
> +that were earlier provided by the driver.
> +
> +Setting RSS configuration
> +\begin{enumerate}
> +\item The driver fills all of the fields and passes them through the control
> +	queue to the device.
> +
> +\item The device sets the RSS configuration as provided by the driver.
> +
> +\item If the device successfully applied the configuration, on each packet
> +	received the device MUST calculate the hashing for the packet and
calculate the hash for the packet..
> +	store it in the virtio-net header in the \field{hash} field and the
> +	hash fields used in the calculation in rss_hash_type.
"rss_hash_type" carryover from previous (v1) patch?  May be should say:

  MUST calculate the hash for the packet using fields specified in hash_function_flags

> +\end{enumerate}
> +
> +In case of any unexpected values/ unsupported hash function the driver
> +MUST return VIRTIO_NET_ERR in the \field{ack} field.
> +
> +\drivernormative{\subparagraph}{Receive Side Scaling}{Device Types / Network Device / Device Operation / Control Virtqueue / Steering mode / Receive Side Scaling}
> +
> +If the driver wants to set RSS hash it should fill the RSS structure fields
> +as follows:
> +
> +\begin{itemize}
> +\item The driver SHOULD choose the hash function that SHOULD be used and fill
> +	it in the \field{hash_function} field along with the appropriate flags
> +	in the \field{hash_function_flags} field. These flags indicate to the
> +	device which packet fields MUST be used in the calculation process of
> +	the hash.
> +\item Once the hash function has been chosen a suitable hash key should be set
> +	in the \field{hash_key} field, the length of the key should be stored
> +	in the \field{hash_key_length} field.
> +\item Lastly the driver should fill the indirection table array in the
> +	\field{indirection_table} field while setting the array length in
> +	\field{indirection_table_length}. This structure is used by the device
> +	for determining in which RX virt queue the packet should be placed.
> +\end{itemize}
> +Once the configuration phase is over successfully, the packets SHOULD have the
> +\field{hash} field with the hash value that was calculated by the device.
> +
> +Whenever the driver wants to discard the current RSS settings, it can send an
> +VIRTIO_NET_SM_CTRL_RSS_SET command along with rss structure that has
> +RSS_HASH_FUNCTION_NONE the \field{hash_function} field.
> +
> +The driver MUST check the \field{ack} field value provided by the device, in
> +case the value is not VIRTIO_NET_OK then the driver MUST handle failure and
> +retry another hash function or else give up.
> +
>   \subparagraph{Legacy Interface: Automatic receive steering in multiqueue mode}\label{sec:Device Types / Network Device / Device Operation / Control Virtqueue / Automatic receive steering in multiqueue mode / Legacy Interface: Automatic receive steering in multiqueue mode}
>   When using the legacy interface, transitional devices and drivers
>   MUST format \field{virtqueue_pairs}


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

[virtio-dev] Re: [PATCH v2 1/2] content: net: Add VIRTIO_NET_F_CTRL_STEERING_MODE feature

[Vijayabhaskar Balakrishna]

On 4/16/2018 6:05 AM, Sameeh Jubran wrote:

> From: Sameeh Jubran <sameeh.j@gmail.com>
>
> This commit introduces steering mode into network device. Steering
> mode is a general infrastructure for various steering modes that can
> be implemented on top of it such as Automatic and RSS.
>
> Signed-off-by: Sameeh Jubran <sjubran@redhat.com>
> ---
>   content.tex | 59 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
>   1 file changed, 59 insertions(+)
>
> diff --git a/content.tex b/content.tex
> index c840588..3d538e8 100644
> --- a/content.tex
> +++ b/content.tex
> @@ -3115,6 +3115,9 @@ features.
>   
>   \item[VIRTIO_NET_F_CTRL_MAC_ADDR(23)] Set MAC address through control
>       channel.
> +
> +\item[VIRTIO_NET_F_CTRL_STEERING_MODE(27)] Device supports configurable steering
> +    mode.
>   \end{description}
>   
>   \subsubsection{Feature bit requirements}\label{sec:Device Types / Network Device / Feature bits / Feature bit requirements}
> @@ -3317,6 +3320,8 @@ struct virtio_net_hdr {
>           le16 csum_start;
>           le16 csum_offset;
>           le16 num_buffers;
> +// Only if VIRTIO_NET_F_CTRL_STEERING_MODE has been negotiated
> +        le64 hash;
>   };
>   \end{lstlisting}
>   
> @@ -4007,6 +4012,60 @@ VIRTIO_NET_CTRL_MQ_VQ_PAIRS_SET command.
>   The device MUST NOT queue packets on receive queues greater than
>   \field{virtqueue_pairs} once it has placed the VIRTIO_NET_CTRL_MQ_VQ_PAIRS_SET command in the used ring.
>   
> +\paragraph{Steering mode}\label{sec:Device Types / Network Device / Device Operation / Control Virtqueue / Steering mode}
> +
> +\begin{lstlisting}
> +// steering mode flags
> +#define STEERING_MODE_AUTO          1
As steering mode flags is a bit field, should it be 0x1?
> +
> +struct virtio_net_steering_modes {
> +le32 steering_modes;
> +};
> +
> +struct virtio_net_steering_mode {
> +le32 steering_mode;
> +le32 command;
> +
> +    union {
> +    }
> +};
> +
> +#define VIRTIO_NET_F_CTRL_STEERING_MODE     27
> +
> +#define VIRTIO_NET_CTRL_STEERING_MODE                7
> +#define VIRTIO_NET_CTRL_SM_GET_SUPPORTED_MODES        0
> +#define VIRTIO_NET_CTRL_SM_CONTROL                    1
> +\end{lstlisting}
> +
> +If the VIRTIO_NET_F_CTRL_STEERING_MODE is negotiated the driver can send control commands for the
> +configuring the steering mode. There are multiple steering modes and they can be configured using
> +the VIRTIO_NET_CTRL_SM_CONTROL command. Each mode has it's own set of commands.
> +
> +The auto steering mode is the default mode if nothing else has been configured by the driver
> +and the VIRTIO_NET_F_CTRL_STEERING_MODE feature is acked by the driver.
> +
> +The class VIRTIO_NET_CTRL_STEERING_MODE has two commands:
> +
> +\begin{enumerate}
> +
> +\item VIRTIO_NET_CTRL_SM_GET_SUPPORTED_MODES is used for getting the supported steering modes by the device.
> +	The command should be executed by the driver before attempting to configure the steering mode. Once the device
> +	receives this command it should fill the virtio_net_steering_modes structure with the supported steering mode
> +	flags.
> +
> +\item The command VIRTIO_NET_CTRL_SM_CONTROL is used for controlling the different steering modes. Each steering mode
> +	has its own set of commands. When executing this command the structure virtio_net_steering_mode should be used as follows:
> +	the \field{steering_mode} should be filled with one of the steering mode flags along with an appropriate command from the chosen
> +	steering mode. The union of virtio_net_steering_mode should be used and filled as the mode's command suggests.
> +\end{enumerate}
> +
> +If this feature has been negotiated, the virtio header has an additional
> +\field{hash} field attached to it.
may be say: ..virtio net header has additional field{hash}
> +
> +\subparagraph{Automatic Receive Steering}{Device Types / Network Device / Device Operation / Control Virtqueue / Steering mode / Automatic Receive Steering}
> +
> +This is the default steering mode, please refer to the "Automatic receive steering in multiqueue" section.
> +
>   \subparagraph{Legacy Interface: Automatic receive steering in multiqueue mode}\label{sec:Device Types / Network Device / Device Operation / Control Virtqueue / Automatic receive steering in multiqueue mode / Legacy Interface: Automatic receive steering in multiqueue mode}
>   When using the legacy interface, transitional devices and drivers
>   MUST format \field{virtqueue_pairs}


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

Re: [virtio-dev] [PATCH v2 2/2] content: net: steering mode: Add RSS

[Michael S. Tsirkin]

On Mon, Apr 16, 2018 at 04:05:26PM +0300, Sameeh Jubran wrote:
> From: Sameeh Jubran <sjubran@redhat.com>
> 
> This commit introduces the RSS feature into virtio-net. It is introduced
> as a sub mode for a general command which configures the steering mode.
> 
> Most modern high end network devices today support configurable hash functions,
> this commit introduces RSS - Receive Side Scaling - [1] to virtio net device.
> 
> The RSS is a technology from Microsoft that boosts network device performance
> by efficiently distributing the traffic among the CPUs in a multiprocessor
> system.
> 
> This feature is supported in most of the modern network cards as well as most
> modern OSes including Linux and Windows. It is worth mentioning that both DPDK
> and Hyper-v support RSS too.
> 
> [1] https://docs.microsoft.com/en-us/windows-hardware/drivers/network/ndis-receive-side-scaling2
> 
> Signed-off-by: Sameeh Jubran <sjubran@redhat.com>
> ---
>  content.tex | 114 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 114 insertions(+)
> 
> diff --git a/content.tex b/content.tex
> index 3d538e8..8076147 100644
> --- a/content.tex
> +++ b/content.tex
> @@ -4017,6 +4017,7 @@ The device MUST NOT queue packets on receive queues greater than
>  \begin{lstlisting}
>  // steering mode flags
>  #define STEERING_MODE_AUTO          1
> +#define STEERING_MODE_RSS           2
>  
>  struct virtio_net_steering_modes {
>  le32 steering_modes;
> @@ -4027,6 +4028,7 @@ le32 steering_mode;
>  le32 command;
>  
>      union {
> +    struct virtio_net_rss rss_conf;
>      }
>  };
>  
> @@ -4066,6 +4068,118 @@ If this feature has been negotiated, the virtio header has an additional
>  
>  This is the default steering mode, please refer to the "Automatic receive steering in multiqueue" section.
>  
> +\subparagraph{Receive Side Scaling}{Device Types / Network Device / Device Operation / Control Virtqueue / Steering mode / Receive Side Scaling}
> +
> +\begin{lstlisting}
> +#define RSS_HASH_FUNCTION_NONE      1
> +#define RSS_HASH_FUNCTION_TOEPLITZ  2
> +#define RSS_HASH_FUNCTION_SYMMETRIC 3
> +
> +// Hash function fields
> +#define RSS_HASH_FIELDS_IPV4          0x00000100
> +#define RSS_HASH_FIELDS_TCP_IPV4      0x00000200
> +#define RSS_HASH_FIELDS_IPV6          0x00000400
> +#define RSS_HASH_FIELDS_IPV6_EX       0x00000800
> +#define RSS_HASH_FIELDS_TCP_IPV6      0x00001000
> +#define RSS_HASH_FIELDS_TCP_IPV6_EX   0x00002000
> +
> +struct virtio_net_rss_supported_hash{
> +le32 hash_function;
> +}
> +
> +struct virtio_net_rss {
> +le32 hash_function;
> +le32 hash_function_flags;
> +le32 hash_key_length;
> +le32 indirection_table_length;
> +	struct {
> +		le32 hash_key[hash_key_length];
> +		le32 indirection_table[indirection_table_length];
> +	}
> +};
> +
> +#define VIRTIO_NET_SM_CTRL_RSS_GET_SUPPORTED_FUNCTIONS   0
> +#define VIRTIO_NET_SM_CTRL_RSS_SET                       1


Please add comments in the code as well.

> +\end{lstlisting}
> +
> +If the VIRTIO_NET_F_CTRL_STEERING_MODE is negotiated the driver can send control
> +commands for the RSS configuration.

Confused.  Does VIRTIO_NET_F_CTRL_STEERING_MODE imply steering mode
generally or RSS specifically?
How does a device with streering mode ctrl but without RSS look?


> For configuring RSS the virtio_net_steering_mode
> +should be filled. The \field{steering_mode} field should be filled with the STEERING_MODE_RSS
> +flag along with one of the VIRTIO_NET_SM_CTRL_RSS commands in the \field{command} field. The
> +\field{rss_conf} field should be used.
> +
> +The class VIRTIO_NET_CTRL_RSS has two commands:
> +
> +\begin{enumerate}
> +\item VIRTIO_NET_SM_CTRL_RSS_GET_SUPPORTED_FUNCTIONS returns the hash functions
> +	supported by the device to the driver.
> +\item VIRTIO_NET_SM_CTRL_RSS_SET applies the new RSS configuration. The command is
> +	used by the driver for setting RSS hash function, hash key and
> +	indirection table in the device.
> +\end{enumerate}
> +
> +\devicenormative{\subparagraph}{Receive Side Scaling}{Device Types / Network Device / Device Operation / Control Virtqueue / Steering mode / Receive Side Scaling}
> +
> +The device MUST fill the virtio_net_rss_supported_hash structure with the hash
> +functions it supports and return the structure to the driver. Zero or more
> +flags of the RSS_HASH_FUNCTION flags MUST be used to fill the \field{hash_function}
> +field.
> +
> +The device MUST drop all previous RSS configuration upon receiving
> +VIRTIO_NET_SM_CTRL_RSS_SET command.
> +
> +The device MUST set the RSS configuration according to the settings provided as
> +follows, once the configuration process is completed the device SHOULD apply
> +the hash function to each of the incoming packets and distribute the packets
> +through the virqueues using the calculated hash and the indirection table
> +that were earlier provided by the driver.
> +
> +Setting RSS configuration
> +\begin{enumerate}
> +\item The driver fills all of the fields and passes them through the control
> +	queue to the device.
> +
> +\item The device sets the RSS configuration as provided by the driver.
> +
> +\item If the device successfully applied the configuration, on each packet
> +	received the device MUST calculate the hashing for the packet and
> +	store it in the virtio-net header in the \field{hash} field and the
> +	hash fields used in the calculation in rss_hash_type.
> +\end{enumerate}
> +
> +In case of any unexpected values/ unsupported hash function the driver
> +MUST return VIRTIO_NET_ERR in the \field{ack} field.

driver or the device?

All MUST statements belon in correct normative sections.

Generally what is the text trying to say here?

> +
> +\drivernormative{\subparagraph}{Receive Side Scaling}{Device Types / Network Device / Device Operation / Control Virtqueue / Steering mode / Receive Side Scaling}
> +
> +If the driver wants to set RSS hash it should fill the RSS structure fields
> +as follows:
> +
> +\begin{itemize}
> +\item The driver SHOULD choose the hash function that SHOULD be used and fill
> +	it in the \field{hash_function} field along with the appropriate flags
> +	in the \field{hash_function_flags} field. These flags indicate to the
> +	device which packet fields MUST be used in the calculation process of
> +	the hash.
> +\item Once the hash function has been chosen a suitable hash key should be set
> +	in the \field{hash_key} field,

which key is suitable?

> the length of the key should be stored
> +	in the \field{hash_key_length} field.

in 4 byte units?
are there limits on the length?

> +\item Lastly the driver should fill the indirection table array

is an indirection table required? why not have a function produce
the queue number?

> in the
> +	\field{indirection_table} field while setting the array length in
> +	\field{indirection_table_length}.

any limits on the length here?

> This structure is used by the device
> +	for determining in which RX virt queue the packet should be placed.

How?

> +\end{itemize}
> +Once the configuration phase is over successfully, the packets SHOULD have the
> +\field{hash} field with the hash value that was calculated by the device.

Why not make this optional? I suspect it's not really useful e.g. for
dpdk or xdp which do not generally use hardware offloads.

> +
> +Whenever the driver wants to discard the current RSS settings, it can send an
> +VIRTIO_NET_SM_CTRL_RSS_SET command along with rss structure that has
> +RSS_HASH_FUNCTION_NONE the \field{hash_function} field.

And then what happens?

> +
> +The driver MUST check the \field{ack} field value provided by the device, in
> +case the value is not VIRTIO_NET_OK then the driver MUST handle failure and
> +retry another hash function or else give up.

Is there anything special here?

> +
>  \subparagraph{Legacy Interface: Automatic receive steering in multiqueue mode}\label{sec:Device Types / Network Device / Device Operation / Control Virtqueue / Automatic receive steering in multiqueue mode / Legacy Interface: Automatic receive steering in multiqueue mode}
>  When using the legacy interface, transitional devices and drivers
>  MUST format \field{virtqueue_pairs}

conformance statements will need to be updated.

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

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

Re: [virtio-dev] [PATCH v2 1/2] content: net: Add VIRTIO_NET_F_CTRL_STEERING_MODE feature

[Sameeh Jubran]

[-- Attachment #1: Type: text/plain, Size: 6092 bytes --]

On Tue, Apr 17, 2018 at 2:05 AM, Michael S. Tsirkin <mst@redhat.com> wrote:

> On Mon, Apr 16, 2018 at 04:05:25PM +0300, Sameeh Jubran wrote:
> > From: Sameeh Jubran <sameeh.j@gmail.com>
> >
> > This commit introduces steering mode into network device. Steering
> > mode is a general infrastructure for various steering modes that can
> > be implemented on top of it such as Automatic and RSS.
> >
> > Signed-off-by: Sameeh Jubran <sjubran@redhat.com>
> > ---
> >  content.tex | 59 ++++++++++++++++++++++++++++++
> +++++++++++++++++++++++++++++
> >  1 file changed, 59 insertions(+)
> >
> > diff --git a/content.tex b/content.tex
> > index c840588..3d538e8 100644
> > --- a/content.tex
> > +++ b/content.tex
> > @@ -3115,6 +3115,9 @@ features.
> >
> >  \item[VIRTIO_NET_F_CTRL_MAC_ADDR(23)] Set MAC address through control
> >      channel.
> > +
> > +\item[VIRTIO_NET_F_CTRL_STEERING_MODE(27)] Device supports
> configurable steering
> > +    mode.
> >  \end{description}
> >
> >  \subsubsection{Feature bit requirements}\label{sec:Device Types /
> Network Device / Feature bits / Feature bit requirements}
> > @@ -3317,6 +3320,8 @@ struct virtio_net_hdr {
> >          le16 csum_start;
> >          le16 csum_offset;
> >          le16 num_buffers;
> > +// Only if VIRTIO_NET_F_CTRL_STEERING_MODE has been negotiated
> > +        le64 hash;
> >  };
> >  \end{lstlisting}
> >
> > @@ -4007,6 +4012,60 @@ VIRTIO_NET_CTRL_MQ_VQ_PAIRS_SET command.
> >  The device MUST NOT queue packets on receive queues greater than
> >  \field{virtqueue_pairs} once it has placed the
> VIRTIO_NET_CTRL_MQ_VQ_PAIRS_SET command in the used ring.
> >
> > +\paragraph{Steering mode}\label{sec:Device Types / Network Device /
> Device Operation / Control Virtqueue / Steering mode}
> > +
> > +\begin{lstlisting}
> > +// steering mode flags
> > +#define STEERING_MODE_AUTO          1
> > +
> > +struct virtio_net_steering_modes {
> > +le32 steering_modes;
> > +};
> > +
> > +struct virtio_net_steering_mode {
> > +le32 steering_mode;
> > +le32 command;
> > +
> > +    union {
> > +    }
> > +};
> > +
> > +#define VIRTIO_NET_F_CTRL_STEERING_MODE     27
>
> Please use a high number (63 and down). We are running out of
> low numbers it is best to keep them for things we must backport
> to old virtio - such as mtu - which are required for the device
> working properly.
>
Will do.

>
> Pls coordinate with Vlad (cc'd) who is adding a new feature too.
>
> > +
> > +#define VIRTIO_NET_CTRL_STEERING_MODE                7
> > +#define VIRTIO_NET_CTRL_SM_GET_SUPPORTED_MODES        0
> > +#define VIRTIO_NET_CTRL_SM_CONTROL                    1
> > +\end{lstlisting}
> > +
> > +If the VIRTIO_NET_F_CTRL_STEERING_MODE is negotiated the driver can
> send control commands for the
> > +configuring the steering mode. There are multiple steering modes and
> they can be configured using
> > +the VIRTIO_NET_CTRL_SM_CONTROL command. Each mode has it's own set of
> commands.
> > +
> > +The auto steering mode is the default mode if nothing else has been
> configured by the driver
> > +and the VIRTIO_NET_F_CTRL_STEERING_MODE feature is acked by the driver.
> > +
> > +The class VIRTIO_NET_CTRL_STEERING_MODE has two commands:
> > +
> > +\begin{enumerate}
> > +
> > +\item VIRTIO_NET_CTRL_SM_GET_SUPPORTED_MODES is used for getting the
> supported steering modes by the device.
> > +     The command should be executed by the driver before attempting to
> configure the steering mode. Once the device
> > +     receives this command it should fill the virtio_net_steering_modes
> structure with the supported steering mode
> > +     flags.
> > +
> > +\item The command VIRTIO_NET_CTRL_SM_CONTROL is used for controlling
> the different steering modes. Each steering mode
> > +     has its own set of commands. When executing this command the
> structure virtio_net_steering_mode should be used as follows:
> > +     the \field{steering_mode} should be filled with one of the
> steering mode flags along with an appropriate command from the chosen
> > +     steering mode. The union of virtio_net_steering_mode should be
> used and filled as the mode's command suggests.
> > +\end{enumerate}
> > +
> > +If this feature has been negotiated, the virtio header
> > has an additional
> > +\field{hash} field attached to it.
> > +
> > +\subparagraph{Automatic Receive Steering}{Device Types / Network Device
> / Device Operation / Control Virtqueue / Steering mode / Automatic Receive
> Steering}
> > +
> > +This is the default steering mode, please refer to the "Automatic
> receive steering in multiqueue" section.
> > +
> >  \subparagraph{Legacy Interface: Automatic receive steering in
> multiqueue mode}\label{sec:Device Types / Network Device / Device Operation
> / Control Virtqueue / Automatic receive steering in multiqueue mode /
> Legacy Interface: Automatic receive steering in multiqueue mode}
> >  When using the legacy interface, transitional devices and drivers
> >  MUST format \field{virtqueue_pairs}
>
> Please add each part where it belongs, e.g. a feature bit with other
> feature bits, header field with other header fields, etc.
>
What are you referring to specifically? Can you please elaborate?

>
> I think Vlad tested extending the header and found it adds
> non-negligeable performance overhead. Is it worth splitting
> out hash calculation? Is hash used for RX path only?
>
Hash is used for RX only and I can't think this is not avoidable. We need
to inform the driver what is the hash value for the packet,
it's a Windows requirement. What do you mean by splitting out the hash
calculation?
We are already extending the virtio-header for RSC feature which didn't
make it into spec somehow.

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



-- 
Respectfully,
*Sameeh Jubran*
*Linkedin <https://il.linkedin.com/pub/sameeh-jubran/87/747/a8a>*
*Software Engineer @ Daynix <http://www.daynix.com>.*

[-- Attachment #2: Type: text/html, Size: 9357 bytes --]

Re: [virtio-dev] [PATCH v2 1/2] content: net: Add VIRTIO_NET_F_CTRL_STEERING_MODE feature

[Sameeh Jubran]

[-- Attachment #1: Type: text/plain, Size: 6730 bytes --]

On Tue, Apr 17, 2018 at 1:43 PM, Sameeh Jubran <sameeh@daynix.com> wrote:

>
>
> On Tue, Apr 17, 2018 at 2:05 AM, Michael S. Tsirkin <mst@redhat.com>
> wrote:
>
>> On Mon, Apr 16, 2018 at 04:05:25PM +0300, Sameeh Jubran wrote:
>> > From: Sameeh Jubran <sameeh.j@gmail.com>
>> >
>> > This commit introduces steering mode into network device. Steering
>> > mode is a general infrastructure for various steering modes that can
>> > be implemented on top of it such as Automatic and RSS.
>> >
>> > Signed-off-by: Sameeh Jubran <sjubran@redhat.com>
>> > ---
>> >  content.tex | 59 ++++++++++++++++++++++++++++++
>> +++++++++++++++++++++++++++++
>> >  1 file changed, 59 insertions(+)
>> >
>> > diff --git a/content.tex b/content.tex
>> > index c840588..3d538e8 100644
>> > --- a/content.tex
>> > +++ b/content.tex
>> > @@ -3115,6 +3115,9 @@ features.
>> >
>> >  \item[VIRTIO_NET_F_CTRL_MAC_ADDR(23)] Set MAC address through control
>> >      channel.
>> > +
>> > +\item[VIRTIO_NET_F_CTRL_STEERING_MODE(27)] Device supports
>> configurable steering
>> > +    mode.
>> >  \end{description}
>> >
>> >  \subsubsection{Feature bit requirements}\label{sec:Device Types /
>> Network Device / Feature bits / Feature bit requirements}
>> > @@ -3317,6 +3320,8 @@ struct virtio_net_hdr {
>> >          le16 csum_start;
>> >          le16 csum_offset;
>> >          le16 num_buffers;
>> > +// Only if VIRTIO_NET_F_CTRL_STEERING_MODE has been negotiated
>> > +        le64 hash;
>> >  };
>> >  \end{lstlisting}
>> >
>> > @@ -4007,6 +4012,60 @@ VIRTIO_NET_CTRL_MQ_VQ_PAIRS_SET command.
>> >  The device MUST NOT queue packets on receive queues greater than
>> >  \field{virtqueue_pairs} once it has placed the
>> VIRTIO_NET_CTRL_MQ_VQ_PAIRS_SET command in the used ring.
>> >
>> > +\paragraph{Steering mode}\label{sec:Device Types / Network Device /
>> Device Operation / Control Virtqueue / Steering mode}
>> > +
>> > +\begin{lstlisting}
>> > +// steering mode flags
>> > +#define STEERING_MODE_AUTO          1
>> > +
>> > +struct virtio_net_steering_modes {
>> > +le32 steering_modes;
>> > +};
>> > +
>> > +struct virtio_net_steering_mode {
>> > +le32 steering_mode;
>> > +le32 command;
>> > +
>> > +    union {
>> > +    }
>> > +};
>> > +
>> > +#define VIRTIO_NET_F_CTRL_STEERING_MODE     27
>>
>> Please use a high number (63 and down). We are running out of
>> low numbers it is best to keep them for things we must backport
>> to old virtio - such as mtu - which are required for the device
>> working properly.
>>
> Will do.
>
>>
>> Pls coordinate with Vlad (cc'd) who is adding a new feature too.
>>
>> > +
>> > +#define VIRTIO_NET_CTRL_STEERING_MODE                7
>> > +#define VIRTIO_NET_CTRL_SM_GET_SUPPORTED_MODES        0
>> > +#define VIRTIO_NET_CTRL_SM_CONTROL                    1
>> > +\end{lstlisting}
>> > +
>> > +If the VIRTIO_NET_F_CTRL_STEERING_MODE is negotiated the driver can
>> send control commands for the
>> > +configuring the steering mode. There are multiple steering modes and
>> they can be configured using
>> > +the VIRTIO_NET_CTRL_SM_CONTROL command. Each mode has it's own set of
>> commands.
>> > +
>> > +The auto steering mode is the default mode if nothing else has been
>> configured by the driver
>> > +and the VIRTIO_NET_F_CTRL_STEERING_MODE feature is acked by the
>> driver.
>> > +
>> > +The class VIRTIO_NET_CTRL_STEERING_MODE has two commands:
>> > +
>> > +\begin{enumerate}
>> > +
>> > +\item VIRTIO_NET_CTRL_SM_GET_SUPPORTED_MODES is used for getting the
>> supported steering modes by the device.
>> > +     The command should be executed by the driver before attempting to
>> configure the steering mode. Once the device
>> > +     receives this command it should fill the
>> virtio_net_steering_modes structure with the supported steering mode
>> > +     flags.
>> > +
>> > +\item The command VIRTIO_NET_CTRL_SM_CONTROL is used for controlling
>> the different steering modes. Each steering mode
>> > +     has its own set of commands. When executing this command the
>> structure virtio_net_steering_mode should be used as follows:
>> > +     the \field{steering_mode} should be filled with one of the
>> steering mode flags along with an appropriate command from the chosen
>> > +     steering mode. The union of virtio_net_steering_mode should be
>> used and filled as the mode's command suggests.
>> > +\end{enumerate}
>> > +
>> > +If this feature has been negotiated, the virtio header
>> > has an additional
>> > +\field{hash} field attached to it.
>> > +
>> > +\subparagraph{Automatic Receive Steering}{Device Types / Network
>> Device / Device Operation / Control Virtqueue / Steering mode / Automatic
>> Receive Steering}
>> > +
>> > +This is the default steering mode, please refer to the "Automatic
>> receive steering in multiqueue" section.
>> > +
>> >  \subparagraph{Legacy Interface: Automatic receive steering in
>> multiqueue mode}\label{sec:Device Types / Network Device / Device Operation
>> / Control Virtqueue / Automatic receive steering in multiqueue mode /
>> Legacy Interface: Automatic receive steering in multiqueue mode}
>> >  When using the legacy interface, transitional devices and drivers
>> >  MUST format \field{virtqueue_pairs}
>>
>> Please add each part where it belongs, e.g. a feature bit with other
>> feature bits, header field with other header fields, etc.
>>
> What are you referring to specifically? Can you please elaborate?
>
>>
>> I think Vlad tested extending the header and found it adds
>> non-negligeable performance overhead. Is it worth splitting
>> out hash calculation? Is hash used for RX path only?
>>
> Hash is used for RX only and I can't think this is not avoidable. We need
> to inform the driver what is the hash value for the packet,
>
Just to clarify, Windows does provide us with hash for the packets so we
can forward them to the device using the hash field in the virtio header.
This value can be used to perform optimizations by the backend.

> it's a Windows requirement. What do you mean by splitting out the hash
> calculation?
> We are already extending the virtio-header for RSC feature which didn't
> make it into spec somehow.
>
>>
>>
>> > --
>> > 2.13.6
>> >
>> >
>> > ---------------------------------------------------------------------
>> > To unsubscribe, e-mail: virtio-dev-unsubscribe@lists.oasis-open.org
>> > For additional commands, e-mail: virtio-dev-help@lists.oasis-open.org
>>
>
>
>
> --
> Respectfully,
> *Sameeh Jubran*
> *Linkedin <https://il.linkedin.com/pub/sameeh-jubran/87/747/a8a>*
> *Software Engineer @ Daynix <http://www.daynix.com>.*
>



-- 
Respectfully,
*Sameeh Jubran*
*Linkedin <https://il.linkedin.com/pub/sameeh-jubran/87/747/a8a>*
*Software Engineer @ Daynix <http://www.daynix.com>.*

[-- Attachment #2: Type: text/html, Size: 11797 bytes --]

[virtio-dev] Re: [PATCH v2 1/2] content: net: Add VIRTIO_NET_F_CTRL_STEERING_MODE feature

[Sameeh Jubran]

[-- Attachment #1: Type: text/plain, Size: 4988 bytes --]

On Tue, Apr 17, 2018 at 4:30 AM, Vijayabhaskar Balakrishna <
vijay.balakrishna@oracle.com> wrote:

> On 4/16/2018 6:05 AM, Sameeh Jubran wrote:
>
> From: Sameeh Jubran <sameeh.j@gmail.com>
>>
>> This commit introduces steering mode into network device. Steering
>> mode is a general infrastructure for various steering modes that can
>> be implemented on top of it such as Automatic and RSS.
>>
>> Signed-off-by: Sameeh Jubran <sjubran@redhat.com>
>> ---
>>   content.tex | 59 ++++++++++++++++++++++++++++++
>> +++++++++++++++++++++++++++++
>>   1 file changed, 59 insertions(+)
>>
>> diff --git a/content.tex b/content.tex
>> index c840588..3d538e8 100644
>> --- a/content.tex
>> +++ b/content.tex
>> @@ -3115,6 +3115,9 @@ features.
>>     \item[VIRTIO_NET_F_CTRL_MAC_ADDR(23)] Set MAC address through control
>>       channel.
>> +
>> +\item[VIRTIO_NET_F_CTRL_STEERING_MODE(27)] Device supports configurable
>> steering
>> +    mode.
>>   \end{description}
>>     \subsubsection{Feature bit requirements}\label{sec:Device Types /
>> Network Device / Feature bits / Feature bit requirements}
>> @@ -3317,6 +3320,8 @@ struct virtio_net_hdr {
>>           le16 csum_start;
>>           le16 csum_offset;
>>           le16 num_buffers;
>> +// Only if VIRTIO_NET_F_CTRL_STEERING_MODE has been negotiated
>> +        le64 hash;
>>   };
>>   \end{lstlisting}
>>   @@ -4007,6 +4012,60 @@ VIRTIO_NET_CTRL_MQ_VQ_PAIRS_SET command.
>>   The device MUST NOT queue packets on receive queues greater than
>>   \field{virtqueue_pairs} once it has placed the
>> VIRTIO_NET_CTRL_MQ_VQ_PAIRS_SET command in the used ring.
>>   +\paragraph{Steering mode}\label{sec:Device Types / Network Device /
>> Device Operation / Control Virtqueue / Steering mode}
>> +
>> +\begin{lstlisting}
>> +// steering mode flags
>> +#define STEERING_MODE_AUTO          1
>>
> As steering mode flags is a bit field, should it be 0x1?

They are indeed bit fields, will dd 0x in order to clarify that.

>
> +
>> +struct virtio_net_steering_modes {
>> +le32 steering_modes;
>> +};
>> +
>> +struct virtio_net_steering_mode {
>> +le32 steering_mode;
>> +le32 command;
>> +
>> +    union {
>> +    }
>> +};
>> +
>> +#define VIRTIO_NET_F_CTRL_STEERING_MODE     27
>> +
>> +#define VIRTIO_NET_CTRL_STEERING_MODE                7
>> +#define VIRTIO_NET_CTRL_SM_GET_SUPPORTED_MODES        0
>> +#define VIRTIO_NET_CTRL_SM_CONTROL                    1
>> +\end{lstlisting}
>> +
>> +If the VIRTIO_NET_F_CTRL_STEERING_MODE is negotiated the driver can
>> send control commands for the
>> +configuring the steering mode. There are multiple steering modes and
>> they can be configured using
>> +the VIRTIO_NET_CTRL_SM_CONTROL command. Each mode has it's own set of
>> commands.
>> +
>> +The auto steering mode is the default mode if nothing else has been
>> configured by the driver
>> +and the VIRTIO_NET_F_CTRL_STEERING_MODE feature is acked by the driver.
>> +
>> +The class VIRTIO_NET_CTRL_STEERING_MODE has two commands:
>> +
>> +\begin{enumerate}
>> +
>> +\item VIRTIO_NET_CTRL_SM_GET_SUPPORTED_MODES is used for getting the
>> supported steering modes by the device.
>> +       The command should be executed by the driver before attempting to
>> configure the steering mode. Once the device
>> +       receives this command it should fill the
>> virtio_net_steering_modes structure with the supported steering mode
>> +       flags.
>> +
>> +\item The command VIRTIO_NET_CTRL_SM_CONTROL is used for controlling the
>> different steering modes. Each steering mode
>> +       has its own set of commands. When executing this command the
>> structure virtio_net_steering_mode should be used as follows:
>> +       the \field{steering_mode} should be filled with one of the
>> steering mode flags along with an appropriate command from the chosen
>> +       steering mode. The union of virtio_net_steering_mode should be
>> used and filled as the mode's command suggests.
>> +\end{enumerate}
>> +
>> +If this feature has been negotiated, the virtio header has an additional
>> +\field{hash} field attached to it.
>>
> may be say: ..virtio net header has additional field{hash}


> +
>> +\subparagraph{Automatic Receive Steering}{Device Types / Network Device
>> / Device Operation / Control Virtqueue / Steering mode / Automatic Receive
>> Steering}
>> +
>> +This is the default steering mode, please refer to the "Automatic
>> receive steering in multiqueue" section.
>> +
>>   \subparagraph{Legacy Interface: Automatic receive steering in
>> multiqueue mode}\label{sec:Device Types / Network Device / Device Operation
>> / Control Virtqueue / Automatic receive steering in multiqueue mode /
>> Legacy Interface: Automatic receive steering in multiqueue mode}
>>   When using the legacy interface, transitional devices and drivers
>>   MUST format \field{virtqueue_pairs}
>>
>
>


-- 
Respectfully,
*Sameeh Jubran*
*Linkedin <https://il.linkedin.com/pub/sameeh-jubran/87/747/a8a>*
*Software Engineer @ Daynix <http://www.daynix.com>.*

[-- Attachment #2: Type: text/html, Size: 7624 bytes --]

[virtio-dev] Re: [PATCH v2 2/2] content: net: steering mode: Add RSS

[Sameeh Jubran]

[-- Attachment #1: Type: text/plain, Size: 8435 bytes --]

On Tue, Apr 17, 2018 at 4:30 AM, Vijayabhaskar Balakrishna <
vijay.balakrishna@oracle.com> wrote:

>
>
> On 4/16/2018 6:05 AM, Sameeh Jubran wrote:
>
>> From: Sameeh Jubran <sjubran@redhat.com>
>>
>> This commit introduces the RSS feature into virtio-net. It is introduced
>> as a sub mode for a general command which configures the steering mode.
>>
>> Most modern high end network devices today support configurable hash
>> functions,
>> this commit introduces RSS - Receive Side Scaling - [1] to virtio net
>> device.
>>
>> The RSS is a technology from Microsoft that boosts network device
>> performance
>> by efficiently distributing the traffic among the CPUs in a multiprocessor
>> system.
>>
>> This feature is supported in most of the modern network cards as well as
>> most
>> modern OSes including Linux and Windows. It is worth mentioning that both
>> DPDK
>> and Hyper-v support RSS too.
>>
>> [1] https://docs.microsoft.com/en-us/windows-hardware/drivers/ne
>> twork/ndis-receive-side-scaling2
>>
>> Signed-off-by: Sameeh Jubran <sjubran@redhat.com>
>> ---
>>   content.tex | 114 ++++++++++++++++++++++++++++++
>> ++++++++++++++++++++++++++++++
>>   1 file changed, 114 insertions(+)
>>
>> diff --git a/content.tex b/content.tex
>> index 3d538e8..8076147 100644
>> --- a/content.tex
>> +++ b/content.tex
>> @@ -4017,6 +4017,7 @@ The device MUST NOT queue packets on receive queues
>> greater than
>>   \begin{lstlisting}
>>   // steering mode flags
>>   #define STEERING_MODE_AUTO          1
>> +#define STEERING_MODE_RSS           2
>>     struct virtio_net_steering_modes {
>>   le32 steering_modes;
>> @@ -4027,6 +4028,7 @@ le32 steering_mode;
>>   le32 command;
>>         union {
>> +    struct virtio_net_rss rss_conf;
>>       }
>>   };
>>   @@ -4066,6 +4068,118 @@ If this feature has been negotiated, the virtio
>> header has an additional
>>     This is the default steering mode, please refer to the "Automatic
>> receive steering in multiqueue" section.
>>   +\subparagraph{Receive Side Scaling}{Device Types / Network Device /
>> Device Operation / Control Virtqueue / Steering mode / Receive Side Scaling}
>> +
>> +\begin{lstlisting}
>> +#define RSS_HASH_FUNCTION_NONE      1
>> +#define RSS_HASH_FUNCTION_TOEPLITZ  2
>> +#define RSS_HASH_FUNCTION_SYMMETRIC 3
>>
> Above are bit fields?  Should it be 0x1, 0x2, 0x4 etc.,

Will do.

>
> +
>> +// Hash function fields
>> +#define RSS_HASH_FIELDS_IPV4          0x00000100
>> +#define RSS_HASH_FIELDS_TCP_IPV4      0x00000200
>> +#define RSS_HASH_FIELDS_IPV6          0x00000400
>> +#define RSS_HASH_FIELDS_IPV6_EX       0x00000800
>> +#define RSS_HASH_FIELDS_TCP_IPV6      0x00001000
>> +#define RSS_HASH_FIELDS_TCP_IPV6_EX   0x00002000
>> +
>> +struct virtio_net_rss_supported_hash{
>> +le32 hash_function;
>> +}
>> +
>> +struct virtio_net_rss {
>> +le32 hash_function;
>> +le32 hash_function_flags;
>> +le32 hash_key_length;
>> +le32 indirection_table_length;
>> +       struct {
>> +               le32 hash_key[hash_key_length];
>> +               le32 indirection_table[indirection_table_length];
>> +       }
>> +};
>> +
>> +#define VIRTIO_NET_SM_CTRL_RSS_GET_SUPPORTED_FUNCTIONS   0
>> +#define VIRTIO_NET_SM_CTRL_RSS_SET                       1
>> +\end{lstlisting}
>> +
>> +If the VIRTIO_NET_F_CTRL_STEERING_MODE is negotiated the driver can
>> send control
>> +commands for the RSS configuration. For configuring RSS the
>> virtio_net_steering_mode
>> +should be filled. The \field{steering_mode} field should be filled with
>> the STEERING_MODE_RSS
>> +flag along with one of the VIRTIO_NET_SM_CTRL_RSS commands in the
>> \field{command} field. The
>> +\field{rss_conf} field should be used.
>> +
>> +The class VIRTIO_NET_CTRL_RSS has two commands:
>> +
>> +\begin{enumerate}
>> +\item VIRTIO_NET_SM_CTRL_RSS_GET_SUPPORTED_FUNCTIONS returns the hash
>> functions
>> +       supported by the device to the driver.
>> +\item VIRTIO_NET_SM_CTRL_RSS_SET applies the new RSS configuration. The
>> command is
>> +       used by the driver for setting RSS hash function, hash key and
>> +       indirection table in the device.
>> +\end{enumerate}
>> +
>> +\devicenormative{\subparagraph}{Receive Side Scaling}{Device Types /
>> Network Device / Device Operation / Control Virtqueue / Steering mode /
>> Receive Side Scaling}
>> +
>> +The device MUST fill the virtio_net_rss_supported_hash structure with
>> the hash
>> +functions it supports and return the structure to the driver. Zero or
>> more
>> +flags of the RSS_HASH_FUNCTION flags MUST be used to fill the
>> \field{hash_function}
>> +field.
>> +
>> +The device MUST drop all previous RSS configuration upon receiving
>> +VIRTIO_NET_SM_CTRL_RSS_SET command.
>> +
>> +The device MUST set the RSS configuration according to the settings
>> provided as
>> +follows, once the configuration process is completed the device SHOULD
>> apply
>> +the hash function to each of the incoming packets and distribute the
>> packets
>> +through the virqueues using the calculated hash and the indirection table
>> +that were earlier provided by the driver.
>> +
>> +Setting RSS configuration
>> +\begin{enumerate}
>> +\item The driver fills all of the fields and passes them through the
>> control
>> +       queue to the device.
>> +
>> +\item The device sets the RSS configuration as provided by the driver.
>> +
>> +\item If the device successfully applied the configuration, on each
>> packet
>> +       received the device MUST calculate the hashing for the packet and
>>
> calculate the hash for the packet..
>
>> +       store it in the virtio-net header in the \field{hash} field and
>> the
>> +       hash fields used in the calculation in rss_hash_type.
>>
> "rss_hash_type" carryover from previous (v1) patch?  May be should say:
>
>  MUST calculate the hash for the packet using fields specified in
> hash_function_flags
>
Will do thanks :)

>
>

> +\end{enumerate}
>> +
>> +In case of any unexpected values/ unsupported hash function the driver
>> +MUST return VIRTIO_NET_ERR in the \field{ack} field.
>> +
>> +\drivernormative{\subparagraph}{Receive Side Scaling}{Device Types /
>> Network Device / Device Operation / Control Virtqueue / Steering mode /
>> Receive Side Scaling}
>> +
>> +If the driver wants to set RSS hash it should fill the RSS structure
>> fields
>> +as follows:
>> +
>> +\begin{itemize}
>> +\item The driver SHOULD choose the hash function that SHOULD be used and
>> fill
>> +       it in the \field{hash_function} field along with the appropriate
>> flags
>> +       in the \field{hash_function_flags} field. These flags indicate to
>> the
>> +       device which packet fields MUST be used in the calculation
>> process of
>> +       the hash.
>> +\item Once the hash function has been chosen a suitable hash key should
>> be set
>> +       in the \field{hash_key} field, the length of the key should be
>> stored
>> +       in the \field{hash_key_length} field.
>> +\item Lastly the driver should fill the indirection table array in the
>> +       \field{indirection_table} field while setting the array length in
>> +       \field{indirection_table_length}. This structure is used by the
>> device
>> +       for determining in which RX virt queue the packet should be
>> placed.
>> +\end{itemize}
>> +Once the configuration phase is over successfully, the packets SHOULD
>> have the
>> +\field{hash} field with the hash value that was calculated by the device.
>> +
>> +Whenever the driver wants to discard the current RSS settings, it can
>> send an
>> +VIRTIO_NET_SM_CTRL_RSS_SET command along with rss structure that has
>> +RSS_HASH_FUNCTION_NONE the \field{hash_function} field.
>> +
>> +The driver MUST check the \field{ack} field value provided by the
>> device, in
>> +case the value is not VIRTIO_NET_OK then the driver MUST handle failure
>> and
>> +retry another hash function or else give up.
>> +
>>   \subparagraph{Legacy Interface: Automatic receive steering in
>> multiqueue mode}\label{sec:Device Types / Network Device / Device Operation
>> / Control Virtqueue / Automatic receive steering in multiqueue mode /
>> Legacy Interface: Automatic receive steering in multiqueue mode}
>>   When using the legacy interface, transitional devices and drivers
>>   MUST format \field{virtqueue_pairs}
>>
>
>


-- 
Respectfully,
*Sameeh Jubran*
*Linkedin <https://il.linkedin.com/pub/sameeh-jubran/87/747/a8a>*
*Software Engineer @ Daynix <http://www.daynix.com>.*

[-- Attachment #2: Type: text/html, Size: 11796 bytes --]

Re: [virtio-dev] [PATCH v2 2/2] content: net: steering mode: Add RSS

[Sameeh Jubran]

[-- Attachment #1: Type: text/plain, Size: 11356 bytes --]

On Tue, Apr 17, 2018 at 4:50 AM, Michael S. Tsirkin <mst@redhat.com> wrote:

> On Mon, Apr 16, 2018 at 04:05:26PM +0300, Sameeh Jubran wrote:
> > From: Sameeh Jubran <sjubran@redhat.com>
> >
> > This commit introduces the RSS feature into virtio-net. It is introduced
> > as a sub mode for a general command which configures the steering mode.
> >
> > Most modern high end network devices today support configurable hash
> functions,
> > this commit introduces RSS - Receive Side Scaling - [1] to virtio net
> device.
> >
> > The RSS is a technology from Microsoft that boosts network device
> performance
> > by efficiently distributing the traffic among the CPUs in a
> multiprocessor
> > system.
> >
> > This feature is supported in most of the modern network cards as well as
> most
> > modern OSes including Linux and Windows. It is worth mentioning that
> both DPDK
> > and Hyper-v support RSS too.
> >
> > [1] https://docs.microsoft.com/en-us/windows-hardware/drivers/
> network/ndis-receive-side-scaling2
> >
> > Signed-off-by: Sameeh Jubran <sjubran@redhat.com>
> > ---
> >  content.tex | 114 ++++++++++++++++++++++++++++++
> ++++++++++++++++++++++++++++++
> >  1 file changed, 114 insertions(+)
> >
> > diff --git a/content.tex b/content.tex
> > index 3d538e8..8076147 100644
> > --- a/content.tex
> > +++ b/content.tex
> > @@ -4017,6 +4017,7 @@ The device MUST NOT queue packets on receive
> queues greater than
> >  \begin{lstlisting}
> >  // steering mode flags
> >  #define STEERING_MODE_AUTO          1
> > +#define STEERING_MODE_RSS           2
> >
> >  struct virtio_net_steering_modes {
> >  le32 steering_modes;
> > @@ -4027,6 +4028,7 @@ le32 steering_mode;
> >  le32 command;
> >
> >      union {
> > +    struct virtio_net_rss rss_conf;
> >      }
> >  };
> >
> > @@ -4066,6 +4068,118 @@ If this feature has been negotiated, the virtio
> header has an additional
> >
> >  This is the default steering mode, please refer to the "Automatic
> receive steering in multiqueue" section.
> >
> > +\subparagraph{Receive Side Scaling}{Device Types / Network Device /
> Device Operation / Control Virtqueue / Steering mode / Receive Side Scaling}
> > +
> > +\begin{lstlisting}
> > +#define RSS_HASH_FUNCTION_NONE      1
> > +#define RSS_HASH_FUNCTION_TOEPLITZ  2
> > +#define RSS_HASH_FUNCTION_SYMMETRIC 3
> > +
> > +// Hash function fields
> > +#define RSS_HASH_FIELDS_IPV4          0x00000100
> > +#define RSS_HASH_FIELDS_TCP_IPV4      0x00000200
> > +#define RSS_HASH_FIELDS_IPV6          0x00000400
> > +#define RSS_HASH_FIELDS_IPV6_EX       0x00000800
> > +#define RSS_HASH_FIELDS_TCP_IPV6      0x00001000
> > +#define RSS_HASH_FIELDS_TCP_IPV6_EX   0x00002000
> > +
> > +struct virtio_net_rss_supported_hash{
> > +le32 hash_function;
> > +}
> > +
> > +struct virtio_net_rss {
> > +le32 hash_function;
> > +le32 hash_function_flags;
> > +le32 hash_key_length;
> > +le32 indirection_table_length;
> > +     struct {
> > +             le32 hash_key[hash_key_length];
> > +             le32 indirection_table[indirection_table_length];
> > +     }
> > +};
> > +
> > +#define VIRTIO_NET_SM_CTRL_RSS_GET_SUPPORTED_FUNCTIONS   0
> > +#define VIRTIO_NET_SM_CTRL_RSS_SET                       1
>
>
> Please add comments in the code as well.

Do you mean that I should add more comments to the code above which
explains the structures and defines?

>


> > +\end{lstlisting}
> > +
> > +If the VIRTIO_NET_F_CTRL_STEERING_MODE is negotiated the driver can
> send control
> > +commands for the RSS configuration.
>
> Confused.  Does VIRTIO_NET_F_CTRL_STEERING_MODE imply steering mode
> generally or RSS specifically?
> How does a device with streering mode ctrl but without RSS look?

The steering mode provides an infrastructure to multiple modes and it is
not specific to RSS. The default mode of the steering mode is the Automatic
mode which is the default mode when MQ is enabled. I though this would be
the best option to provide backward compatibility and makes sense as we
already have "Automatic steering mode" in the spec.
VIRTIO_NET_F_CTRL_STEERING_MODE  gives each mode it's own subset of
commands as is the case in RSS.

>
>
> > For configuring RSS the virtio_net_steering_mode
> > +should be filled. The \field{steering_mode} field should be filled with
> the STEERING_MODE_RSS
> > +flag along with one of the VIRTIO_NET_SM_CTRL_RSS commands in the
> \field{command} field. The
> > +\field{rss_conf} field should be used.
> > +
> > +The class VIRTIO_NET_CTRL_RSS has two commands:
> > +
> > +\begin{enumerate}
> > +\item VIRTIO_NET_SM_CTRL_RSS_GET_SUPPORTED_FUNCTIONS returns the hash
> functions
> > +     supported by the device to the driver.
> > +\item VIRTIO_NET_SM_CTRL_RSS_SET applies the new RSS configuration. The
> command is
> > +     used by the driver for setting RSS hash function, hash key and
> > +     indirection table in the device.
> > +\end{enumerate}
> > +
> > +\devicenormative{\subparagraph}{Receive Side Scaling}{Device Types /
> Network Device / Device Operation / Control Virtqueue / Steering mode /
> Receive Side Scaling}
> > +
> > +The device MUST fill the virtio_net_rss_supported_hash structure with
> the hash
> > +functions it supports and return the structure to the driver. Zero or
> more
> > +flags of the RSS_HASH_FUNCTION flags MUST be used to fill the
> \field{hash_function}
> > +field.
> > +
> > +The device MUST drop all previous RSS configuration upon receiving
> > +VIRTIO_NET_SM_CTRL_RSS_SET command.
> > +
> > +The device MUST set the RSS configuration according to the settings
> provided as
> > +follows, once the configuration process is completed the device SHOULD
> apply
> > +the hash function to each of the incoming packets and distribute the
> packets
> > +through the virqueues using the calculated hash and the indirection
> table
> > +that were earlier provided by the driver.
> > +
> > +Setting RSS configuration
> > +\begin{enumerate}
> > +\item The driver fills all of the fields and passes them through the
> control
> > +     queue to the device.
> > +
> > +\item The device sets the RSS configuration as provided by the driver.
> > +
> > +\item If the device successfully applied the configuration, on each
> packet
> > +     received the device MUST calculate the hashing for the packet and
> > +     store it in the virtio-net header in the \field{hash} field and the
> > +     hash fields used in the calculation in rss_hash_type.
> > +\end{enumerate}
> > +
> > +In case of any unexpected values/ unsupported hash function the driver
> > +MUST return VIRTIO_NET_ERR in the \field{ack} field.
>
> driver or the device?


> All MUST statements belon in correct normative sections.
>
True, this is obviously a mistake, it should be the device instead of the
driver.

>
> Generally what is the text trying to say here?
>
That the device should handle errors and inform the drivers if there are
any errors during the execution of the commands.

>
> > +
> > +\drivernormative{\subparagraph}{Receive Side Scaling}{Device Types /
> Network Device / Device Operation / Control Virtqueue / Steering mode /
> Receive Side Scaling}
> > +
> > +If the driver wants to set RSS hash it should fill the RSS structure
> fields
> > +as follows:
> > +
> > +\begin{itemize}
> > +\item The driver SHOULD choose the hash function that SHOULD be used
> and fill
> > +     it in the \field{hash_function} field along with the appropriate
> flags
> > +     in the \field{hash_function_flags} field. These flags indicate to
> the
> > +     device which packet fields MUST be used in the calculation process
> of
> > +     the hash.
> > +\item Once the hash function has been chosen a suitable hash key should
> be set
> > +     in the \field{hash_key} field,
>
> which key is suitable?
>
depends on the configuration, in Windows case the OS provide us with the
key, however, in general it could be anything.

>
> > the length of the key should be stored
> > +     in the \field{hash_key_length} field.
>
> in 4 byte units?
> are there limits on the length?
>
I think one byte can be used as well as now it is 40 bytes in Windows. What
is the best size that should be chosen in order to keep it future proof?

>
> > +\item Lastly the driver should fill the indirection table array
>
> is an indirection table required? why not have a function produce
> the queue number?
>
Yes it is required as in Windows it is given by the OS. the indirection
table is basically a map between the hash and cpu ids which correspond to
queue ids.

>
> > in the
> > +     \field{indirection_table} field while setting the array length in
> > +     \field{indirection_table_length}.
>
> any limits on the length here?
>
currently the indirection table is 128 bytes in Windows. We can change the
sizes to fit that case, but what about future compatibility? What do you
think should be the optimal size of the fields in order to keep this future
proof while keeping the overhead minimal.

>
> > This structure is used by the device
> > +     for determining in which RX virt queue the packet should be placed.
>
> How?
>
By applying the hash function on the packet while using the provided key in
order to determine the hash for the packet and then use the indirection
table to place the packet inside the suitable queue. This is hash function
dependent implementation, but more elaboration can be added indeed.

>
> > +\end{itemize}
> > +Once the configuration phase is over successfully, the packets SHOULD
> have the
> > +\field{hash} field with the hash value that was calculated by the
> device.
>
> Why not make this optional? I suspect it's not really useful e.g. for
> dpdk or xdp which do not generally use hardware offloads.
>
Good point, will make it optional.

>
> > +
> > +Whenever the driver wants to discard the current RSS settings, it can
> send an
> > +VIRTIO_NET_SM_CTRL_RSS_SET command along with rss structure that has
> > +RSS_HASH_FUNCTION_NONE the \field{hash_function} field.
>
> And then what happens?
>
and then the device is configured with RSS settings and MUST start
distributing the packets into the queues accordingly while attaching the
calculated hash to the header.

>
> > +
> > +The driver MUST check the \field{ack} field value provided by the
> device, in
> > +case the value is not VIRTIO_NET_OK then the driver MUST handle failure
> and
> > +retry another hash function or else give up.
>
> Is there anything special here?
>
Not really just describing the error handling in detail.

>
> > +
> >  \subparagraph{Legacy Interface: Automatic receive steering in
> multiqueue mode}\label{sec:Device Types / Network Device / Device Operation
> / Control Virtqueue / Automatic receive steering in multiqueue mode /
> Legacy Interface: Automatic receive steering in multiqueue mode}
> >  When using the legacy interface, transitional devices and drivers
> >  MUST format \field{virtqueue_pairs}
>
> conformance statements will need to be updated.
>
which statements for example?

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



-- 
Respectfully,
*Sameeh Jubran*
*Linkedin <https://il.linkedin.com/pub/sameeh-jubran/87/747/a8a>*
*Software Engineer @ Daynix <http://www.daynix.com>.*

[-- Attachment #2: Type: text/html, Size: 18329 bytes --]

Re: [virtio-dev] [PATCH v2 2/2] content: net: steering mode: Add RSS

[Michael S. Tsirkin]

On Tue, Apr 17, 2018 at 02:50:15PM +0300, Sameeh Jubran wrote:
> 
> 
> On Tue, Apr 17, 2018 at 4:50 AM, Michael S. Tsirkin <mst@redhat.com> wrote:
> 
>     On Mon, Apr 16, 2018 at 04:05:26PM +0300, Sameeh Jubran wrote:
>     > From: Sameeh Jubran <sjubran@redhat.com>
>     >
>     > This commit introduces the RSS feature into virtio-net. It is introduced
>     > as a sub mode for a general command which configures the steering mode.
>     >
>     > Most modern high end network devices today support configurable hash
>     functions,
>     > this commit introduces RSS - Receive Side Scaling - [1] to virtio net
>     device.
>     >
>     > The RSS is a technology from Microsoft that boosts network device
>     performance
>     > by efficiently distributing the traffic among the CPUs in a
>     multiprocessor
>     > system.
>     >
>     > This feature is supported in most of the modern network cards as well as
>     most
>     > modern OSes including Linux and Windows. It is worth mentioning that both
>     DPDK
>     > and Hyper-v support RSS too.
>     >
>     > [1] https://docs.microsoft.com/en-us/windows-hardware/drivers/network/
>     ndis-receive-side-scaling2
>     >
>     > Signed-off-by: Sameeh Jubran <sjubran@redhat.com>
>     > ---
>     >  content.tex | 114 ++++++++++++++++++++++++++++++
>     ++++++++++++++++++++++++++++++
>     >  1 file changed, 114 insertions(+)
>     >
>     > diff --git a/content.tex b/content.tex
>     > index 3d538e8..8076147 100644
>     > --- a/content.tex
>     > +++ b/content.tex
>     > @@ -4017,6 +4017,7 @@ The device MUST NOT queue packets on receive queues
>     greater than
>     >  \begin{lstlisting}
>     >  // steering mode flags
>     >  #define STEERING_MODE_AUTO          1
>     > +#define STEERING_MODE_RSS           2
>     > 
>     >  struct virtio_net_steering_modes {
>     >  le32 steering_modes;
>     > @@ -4027,6 +4028,7 @@ le32 steering_mode;
>     >  le32 command;
>     > 
>     >      union {
>     > +    struct virtio_net_rss rss_conf;
>     >      }
>     >  };
>     > 
>     > @@ -4066,6 +4068,118 @@ If this feature has been negotiated, the virtio
>     header has an additional
>     > 
>     >  This is the default steering mode, please refer to the "Automatic
>     receive steering in multiqueue" section.
>     > 
>     > +\subparagraph{Receive Side Scaling}{Device Types / Network Device /
>     Device Operation / Control Virtqueue / Steering mode / Receive Side
>     Scaling}
>     > +
>     > +\begin{lstlisting}
>     > +#define RSS_HASH_FUNCTION_NONE      1
>     > +#define RSS_HASH_FUNCTION_TOEPLITZ  2
>     > +#define RSS_HASH_FUNCTION_SYMMETRIC 3
>     > +
>     > +// Hash function fields
>     > +#define RSS_HASH_FIELDS_IPV4          0x00000100
>     > +#define RSS_HASH_FIELDS_TCP_IPV4      0x00000200
>     > +#define RSS_HASH_FIELDS_IPV6          0x00000400
>     > +#define RSS_HASH_FIELDS_IPV6_EX       0x00000800
>     > +#define RSS_HASH_FIELDS_TCP_IPV6      0x00001000
>     > +#define RSS_HASH_FIELDS_TCP_IPV6_EX   0x00002000
>     > +
>     > +struct virtio_net_rss_supported_hash{
>     > +le32 hash_function;
>     > +}
>     > +
>     > +struct virtio_net_rss {
>     > +le32 hash_function;
>     > +le32 hash_function_flags;
>     > +le32 hash_key_length;
>     > +le32 indirection_table_length;
>     > +     struct {
>     > +             le32 hash_key[hash_key_length];
>     > +             le32 indirection_table[indirection_table_length];
>     > +     }
>     > +};
>     > +
>     > +#define VIRTIO_NET_SM_CTRL_RSS_GET_SUPPORTED_FUNCTIONS   0
>     > +#define VIRTIO_NET_SM_CTRL_RSS_SET                       1
> 
> 
>     Please add comments in the code as well.
> 
> Do you mean that I should add more comments to the code above which explains
> the structures and defines?

Exactly.

>      
> 
>    
>     > +\end{lstlisting}
>     > +
>     > +If the VIRTIO_NET_F_CTRL_STEERING_MODE is negotiated the driver can send
>     control
>     > +commands for the RSS configuration.
> 
>     Confused.  Does VIRTIO_NET_F_CTRL_STEERING_MODE imply steering mode
>     generally or RSS specifically?
>     How does a device with streering mode ctrl but without RSS look?
> 
> The steering mode provides an infrastructure to multiple modes and it is not
> specific to RSS.


Then maybe this sentence should not imply that VIRTIO_NET_F_CTRL_STEERING_MODE
allows RSS.

> The default mode of the steering mode is the Automatic mode
> which is the default mode when MQ is enabled. I though this would be the best
> option to provide backward compatibility and makes sense as we already have
> "Automatic steering mode" in the spec.
> VIRTIO_NET_F_CTRL_STEERING_MODE  gives each mode it's own subset of commands as
> is the case in RSS.
> 
> 
> 
>     > For configuring RSS the virtio_net_steering_mode
>     > +should be filled. The \field{steering_mode} field should be filled with
>     the STEERING_MODE_RSS
>     > +flag along with one of the VIRTIO_NET_SM_CTRL_RSS commands in the \field
>     {command} field. The
>     > +\field{rss_conf} field should be used.
>     > +
>     > +The class VIRTIO_NET_CTRL_RSS has two commands:
>     > +
>     > +\begin{enumerate}
>     > +\item VIRTIO_NET_SM_CTRL_RSS_GET_SUPPORTED_FUNCTIONS returns the hash
>     functions
>     > +     supported by the device to the driver.
>     > +\item VIRTIO_NET_SM_CTRL_RSS_SET applies the new RSS configuration. The
>     command is
>     > +     used by the driver for setting RSS hash function, hash key and
>     > +     indirection table in the device.
>     > +\end{enumerate}
>     > +
>     > +\devicenormative{\subparagraph}{Receive Side Scaling}{Device Types /
>     Network Device / Device Operation / Control Virtqueue / Steering mode /
>     Receive Side Scaling}
>     > +
>     > +The device MUST fill the virtio_net_rss_supported_hash structure with
>     the hash
>     > +functions it supports and return the structure to the driver. Zero or
>     more
>     > +flags of the RSS_HASH_FUNCTION flags MUST be used to fill the \field
>     {hash_function}
>     > +field.
>     > +
>     > +The device MUST drop all previous RSS configuration upon receiving
>     > +VIRTIO_NET_SM_CTRL_RSS_SET command.
>     > +
>     > +The device MUST set the RSS configuration according to the settings
>     provided as
>     > +follows, once the configuration process is completed the device SHOULD
>     apply
>     > +the hash function to each of the incoming packets and distribute the
>     packets
>     > +through the virqueues using the calculated hash and the indirection
>     table
>     > +that were earlier provided by the driver.
>     > +
>     > +Setting RSS configuration
>     > +\begin{enumerate}
>     > +\item The driver fills all of the fields and passes them through the
>     control
>     > +     queue to the device.
>     > +
>     > +\item The device sets the RSS configuration as provided by the driver.
>     > +
>     > +\item If the device successfully applied the configuration, on each
>     packet
>     > +     received the device MUST calculate the hashing for the packet and
>     > +     store it in the virtio-net header in the \field{hash} field and the
>     > +     hash fields used in the calculation in rss_hash_type.
>     > +\end{enumerate}
>     > +
>     > +In case of any unexpected values/ unsupported hash function the driver
>     > +MUST return VIRTIO_NET_ERR in the \field{ack} field.
> 
>     driver or the device? 
> 
> 
>     All MUST statements belon in correct normative sections.
> 
> True, this is obviously a mistake, it should be the device instead of the
> driver. 
> 
> 
>     Generally what is the text trying to say here?
> 
> That the device should handle errors and inform the drivers if there are any
> errors during the execution of the commands. 
> 
>    
>     > +
>     > +\drivernormative{\subparagraph}{Receive Side Scaling}{Device Types /
>     Network Device / Device Operation / Control Virtqueue / Steering mode /
>     Receive Side Scaling}
>     > +
>     > +If the driver wants to set RSS hash it should fill the RSS structure
>     fields
>     > +as follows:
>     > +
>     > +\begin{itemize}
>     > +\item The driver SHOULD choose the hash function that SHOULD be used and
>     fill
>     > +     it in the \field{hash_function} field along with the appropriate
>     flags
>     > +     in the \field{hash_function_flags} field. These flags indicate to
>     the
>     > +     device which packet fields MUST be used in the calculation process
>     of
>     > +     the hash.
>     > +\item Once the hash function has been chosen a suitable hash key should
>     be set
>     > +     in the \field{hash_key} field,
> 
>     which key is suitable?
> 
> depends on the configuration, in Windows case the OS provide us with the key,
> however, in general it could be anything. 
> 

We need to somehow describe it in the spec :)

>     > the length of the key should be stored
>     > +     in the \field{hash_key_length} field.
> 
>     in 4 byte units?
>     are there limits on the length?
> 
> I think one byte can be used as well as now it is 40 bytes in Windows. What is
> the best size that should be chosen in order to keep it future proof?

As a 1st step, could you add spec text explaining what it is?


>     > +\item Lastly the driver should fill the indirection table array
> 
>     is an indirection table required? why not have a function produce
>     the queue number?
> 
> Yes it is required as in Windows it is given by the OS. the indirection table
> is basically a map between the hash and cpu ids which correspond to queue ids. 
> 
>    
>     > in the
>     > +     \field{indirection_table} field while setting the array length in
>     > +     \field{indirection_table_length}.
> 
>     any limits on the length here?
> 
> currently the indirection table is 128 bytes in Windows. We can change the
> sizes to fit that case, but what about future compatibility? What do you think
> should be the optimal size of the fields in order to keep this future proof
> while keeping the overhead minimal.

As host must maintain this in memory, you can have host specify the
maximum. guest will bail out if host does not support what it needs.


>    
>     > This structure is used by the device
>     > +     for determining in which RX virt queue the packet should be placed.
> 
>     How?
> 
> By applying the hash function on the packet while using the provided key in
> order to determine the hash for the packet and then use the indirection table
> to place the packet inside the suitable queue. This is hash function dependent
> implementation, but more elaboration can be added indeed. 

I guess indirection table is independent?

>    
>     > +\end{itemize}
>     > +Once the configuration phase is over successfully, the packets SHOULD
>     have the
>     > +\field{hash} field with the hash value that was calculated by the
>     device.
> 
>     Why not make this optional? I suspect it's not really useful e.g. for
>     dpdk or xdp which do not generally use hardware offloads.
> 
> Good point, will make it optional. 
> 
>    
>     > +
>     > +Whenever the driver wants to discard the current RSS settings, it can
>     send an
>     > +VIRTIO_NET_SM_CTRL_RSS_SET command along with rss structure that has
>     > +RSS_HASH_FUNCTION_NONE the \field{hash_function} field.
> 
>     And then what happens?
> 
> and then the device is configured with RSS settings


so NONE will enable RSS?

> and MUST start distributing
> the packets into the queues accordingly while attaching the calculated hash to
> the header.
> 
>    
>     > +
>     > +The driver MUST check the \field{ack} field value provided by the
>     device, in
>     > +case the value is not VIRTIO_NET_OK then the driver MUST handle failure
>     and
>     > +retry another hash function or else give up.
> 
>     Is there anything special here?
> 
> Not really just describing the error handling in detail. 

If it applies to all commands maybe add it to generic code.

>    
>     > +
>     >  \subparagraph{Legacy Interface: Automatic receive steering in multiqueue
>     mode}\label{sec:Device Types / Network Device / Device Operation / Control
>     Virtqueue / Automatic receive steering in multiqueue mode / Legacy
>     Interface: Automatic receive steering in multiqueue mode}
>     >  When using the legacy interface, transitional devices and drivers
>     >  MUST format \field{virtqueue_pairs}
> 
>     conformance statements will need to be updated.
> 
> which statements for example?

I mean links are needed in conformance.tex

>    
>     > --
>     > 2.13.6
>     >
>     >
>     > ---------------------------------------------------------------------
>     > To unsubscribe, e-mail: virtio-dev-unsubscribe@lists.oasis-open.org
>     > For additional commands, e-mail: virtio-dev-help@lists.oasis-open.org
> 
> 
> 
> 
> --
> Respectfully,
> Sameeh Jubran
> Linkedin
> Software Engineer @ Daynix.

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

Re: [virtio-dev] [PATCH v2 2/2] content: net: steering mode: Add RSS

[Sameeh Jubran]

[-- Attachment #1: Type: text/plain, Size: 14079 bytes --]

On Tue, Apr 17, 2018 at 5:14 PM, Michael S. Tsirkin <mst@redhat.com> wrote:

> On Tue, Apr 17, 2018 at 02:50:15PM +0300, Sameeh Jubran wrote:
> >
> >
> > On Tue, Apr 17, 2018 at 4:50 AM, Michael S. Tsirkin <mst@redhat.com>
> wrote:
> >
> >     On Mon, Apr 16, 2018 at 04:05:26PM +0300, Sameeh Jubran wrote:
> >     > From: Sameeh Jubran <sjubran@redhat.com>
> >     >
> >     > This commit introduces the RSS feature into virtio-net. It is
> introduced
> >     > as a sub mode for a general command which configures the steering
> mode.
> >     >
> >     > Most modern high end network devices today support configurable
> hash
> >     functions,
> >     > this commit introduces RSS - Receive Side Scaling - [1] to virtio
> net
> >     device.
> >     >
> >     > The RSS is a technology from Microsoft that boosts network device
> >     performance
> >     > by efficiently distributing the traffic among the CPUs in a
> >     multiprocessor
> >     > system.
> >     >
> >     > This feature is supported in most of the modern network cards as
> well as
> >     most
> >     > modern OSes including Linux and Windows. It is worth mentioning
> that both
> >     DPDK
> >     > and Hyper-v support RSS too.
> >     >
> >     > [1] https://docs.microsoft.com/en-us/windows-hardware/drivers/
> network/
> >     ndis-receive-side-scaling2
> >     >
> >     > Signed-off-by: Sameeh Jubran <sjubran@redhat.com>
> >     > ---
> >     >  content.tex | 114 ++++++++++++++++++++++++++++++
> >     ++++++++++++++++++++++++++++++
> >     >  1 file changed, 114 insertions(+)
> >     >
> >     > diff --git a/content.tex b/content.tex
> >     > index 3d538e8..8076147 100644
> >     > --- a/content.tex
> >     > +++ b/content.tex
> >     > @@ -4017,6 +4017,7 @@ The device MUST NOT queue packets on receive
> queues
> >     greater than
> >     >  \begin{lstlisting}
> >     >  // steering mode flags
> >     >  #define STEERING_MODE_AUTO          1
> >     > +#define STEERING_MODE_RSS           2
> >     >
> >     >  struct virtio_net_steering_modes {
> >     >  le32 steering_modes;
> >     > @@ -4027,6 +4028,7 @@ le32 steering_mode;
> >     >  le32 command;
> >     >
> >     >      union {
> >     > +    struct virtio_net_rss rss_conf;
> >     >      }
> >     >  };
> >     >
> >     > @@ -4066,6 +4068,118 @@ If this feature has been negotiated, the
> virtio
> >     header has an additional
> >     >
> >     >  This is the default steering mode, please refer to the "Automatic
> >     receive steering in multiqueue" section.
> >     >
> >     > +\subparagraph{Receive Side Scaling}{Device Types / Network Device
> /
> >     Device Operation / Control Virtqueue / Steering mode / Receive Side
> >     Scaling}
> >     > +
> >     > +\begin{lstlisting}
> >     > +#define RSS_HASH_FUNCTION_NONE      1
> >     > +#define RSS_HASH_FUNCTION_TOEPLITZ  2
> >     > +#define RSS_HASH_FUNCTION_SYMMETRIC 3
> >     > +
> >     > +// Hash function fields
> >     > +#define RSS_HASH_FIELDS_IPV4          0x00000100
> >     > +#define RSS_HASH_FIELDS_TCP_IPV4      0x00000200
> >     > +#define RSS_HASH_FIELDS_IPV6          0x00000400
> >     > +#define RSS_HASH_FIELDS_IPV6_EX       0x00000800
> >     > +#define RSS_HASH_FIELDS_TCP_IPV6      0x00001000
> >     > +#define RSS_HASH_FIELDS_TCP_IPV6_EX   0x00002000
> >     > +
> >     > +struct virtio_net_rss_supported_hash{
> >     > +le32 hash_function;
> >     > +}
> >     > +
> >     > +struct virtio_net_rss {
> >     > +le32 hash_function;
> >     > +le32 hash_function_flags;
> >     > +le32 hash_key_length;
> >     > +le32 indirection_table_length;
> >     > +     struct {
> >     > +             le32 hash_key[hash_key_length];
> >     > +             le32 indirection_table[indirection_table_length];
> >     > +     }
> >     > +};
> >     > +
> >     > +#define VIRTIO_NET_SM_CTRL_RSS_GET_SUPPORTED_FUNCTIONS   0
> >     > +#define VIRTIO_NET_SM_CTRL_RSS_SET                       1
> >
> >
> >     Please add comments in the code as well.
> >
> > Do you mean that I should add more comments to the code above which
> explains
> > the structures and defines?
>
> Exactly.
>
> >
> >
> >
> >     > +\end{lstlisting}
> >     > +
> >     > +If the VIRTIO_NET_F_CTRL_STEERING_MODE is negotiated the driver
> can send
> >     control
> >     > +commands for the RSS configuration.
> >
> >     Confused.  Does VIRTIO_NET_F_CTRL_STEERING_MODE imply steering mode
> >     generally or RSS specifically?
> >     How does a device with streering mode ctrl but without RSS look?
> >
> > The steering mode provides an infrastructure to multiple modes and it is
> not
> > specific to RSS.
>
>
> Then maybe this sentence should not imply that VIRTIO_NET_F_CTRL_STEERING_
> MODE
> allows RSS.
>
> > The default mode of the steering mode is the Automatic mode
> > which is the default mode when MQ is enabled. I though this would be the
> best
> > option to provide backward compatibility and makes sense as we already
> have
> > "Automatic steering mode" in the spec.
> > VIRTIO_NET_F_CTRL_STEERING_MODE  gives each mode it's own subset of
> commands as
> > is the case in RSS.
> >
> >
> >
> >     > For configuring RSS the virtio_net_steering_mode
> >     > +should be filled. The \field{steering_mode} field should be
> filled with
> >     the STEERING_MODE_RSS
> >     > +flag along with one of the VIRTIO_NET_SM_CTRL_RSS commands in the
> \field
> >     {command} field. The
> >     > +\field{rss_conf} field should be used.
> >     > +
> >     > +The class VIRTIO_NET_CTRL_RSS has two commands:
> >     > +
> >     > +\begin{enumerate}
> >     > +\item VIRTIO_NET_SM_CTRL_RSS_GET_SUPPORTED_FUNCTIONS returns the
> hash
> >     functions
> >     > +     supported by the device to the driver.
> >     > +\item VIRTIO_NET_SM_CTRL_RSS_SET applies the new RSS
> configuration. The
> >     command is
> >     > +     used by the driver for setting RSS hash function, hash key
> and
> >     > +     indirection table in the device.
> >     > +\end{enumerate}
> >     > +
> >     > +\devicenormative{\subparagraph}{Receive Side Scaling}{Device
> Types /
> >     Network Device / Device Operation / Control Virtqueue / Steering
> mode /
> >     Receive Side Scaling}
> >     > +
> >     > +The device MUST fill the virtio_net_rss_supported_hash structure
> with
> >     the hash
> >     > +functions it supports and return the structure to the driver.
> Zero or
> >     more
> >     > +flags of the RSS_HASH_FUNCTION flags MUST be used to fill the
> \field
> >     {hash_function}
> >     > +field.
> >     > +
> >     > +The device MUST drop all previous RSS configuration upon receiving
> >     > +VIRTIO_NET_SM_CTRL_RSS_SET command.
> >     > +
> >     > +The device MUST set the RSS configuration according to the
> settings
> >     provided as
> >     > +follows, once the configuration process is completed the device
> SHOULD
> >     apply
> >     > +the hash function to each of the incoming packets and distribute
> the
> >     packets
> >     > +through the virqueues using the calculated hash and the
> indirection
> >     table
> >     > +that were earlier provided by the driver.
> >     > +
> >     > +Setting RSS configuration
> >     > +\begin{enumerate}
> >     > +\item The driver fills all of the fields and passes them through
> the
> >     control
> >     > +     queue to the device.
> >     > +
> >     > +\item The device sets the RSS configuration as provided by the
> driver.
> >     > +
> >     > +\item If the device successfully applied the configuration, on
> each
> >     packet
> >     > +     received the device MUST calculate the hashing for the
> packet and
> >     > +     store it in the virtio-net header in the \field{hash} field
> and the
> >     > +     hash fields used in the calculation in rss_hash_type.
> >     > +\end{enumerate}
> >     > +
> >     > +In case of any unexpected values/ unsupported hash function the
> driver
> >     > +MUST return VIRTIO_NET_ERR in the \field{ack} field.
> >
> >     driver or the device?
> >
> >
> >     All MUST statements belon in correct normative sections.
> >
> > True, this is obviously a mistake, it should be the device instead of the
> > driver.
> >
> >
> >     Generally what is the text trying to say here?
> >
> > That the device should handle errors and inform the drivers if there are
> any
> > errors during the execution of the commands.
> >
> >
> >     > +
> >     > +\drivernormative{\subparagraph}{Receive Side Scaling}{Device
> Types /
> >     Network Device / Device Operation / Control Virtqueue / Steering
> mode /
> >     Receive Side Scaling}
> >     > +
> >     > +If the driver wants to set RSS hash it should fill the RSS
> structure
> >     fields
> >     > +as follows:
> >     > +
> >     > +\begin{itemize}
> >     > +\item The driver SHOULD choose the hash function that SHOULD be
> used and
> >     fill
> >     > +     it in the \field{hash_function} field along with the
> appropriate
> >     flags
> >     > +     in the \field{hash_function_flags} field. These flags
> indicate to
> >     the
> >     > +     device which packet fields MUST be used in the calculation
> process
> >     of
> >     > +     the hash.
> >     > +\item Once the hash function has been chosen a suitable hash key
> should
> >     be set
> >     > +     in the \field{hash_key} field,
> >
> >     which key is suitable?
> >
> > depends on the configuration, in Windows case the OS provide us with the
> key,
> > however, in general it could be anything.
> >
>
> We need to somehow describe it in the spec :)
>
> >     > the length of the key should be stored
> >     > +     in the \field{hash_key_length} field.
> >
> >     in 4 byte units?
> >     are there limits on the length?
> >
> > I think one byte can be used as well as now it is 40 bytes in Windows.
> What is
> > the best size that should be chosen in order to keep it future proof?
>
> As a 1st step, could you add spec text explaining what it is?
>
>
> >     > +\item Lastly the driver should fill the indirection table array
> >
> >     is an indirection table required? why not have a function produce
> >     the queue number?
> >
> > Yes it is required as in Windows it is given by the OS. the indirection
> table
> > is basically a map between the hash and cpu ids which correspond to
> queue ids.
> >
> >
> >     > in the
> >     > +     \field{indirection_table} field while setting the array
> length in
> >     > +     \field{indirection_table_length}.
> >
> >     any limits on the length here?
> >
> > currently the indirection table is 128 bytes in Windows. We can change
> the
> > sizes to fit that case, but what about future compatibility? What do you
> think
> > should be the optimal size of the fields in order to keep this future
> proof
> > while keeping the overhead minimal.
>
> As host must maintain this in memory, you can have host specify the
> maximum. guest will bail out if host does not support what it needs.
>
>
> >
> >     > This structure is used by the device
> >     > +     for determining in which RX virt queue the packet should be
> placed.
> >
> >     How?
> >
> > By applying the hash function on the packet while using the provided key
> in
> > order to determine the hash for the packet and then use the indirection
> table
> > to place the packet inside the suitable queue. This is hash function
> dependent
> > implementation, but more elaboration can be added indeed.
>
> I guess indirection table is independent?
>
> >
> >     > +\end{itemize}
> >     > +Once the configuration phase is over successfully, the packets
> SHOULD
> >     have the
> >     > +\field{hash} field with the hash value that was calculated by the
> >     device.
> >
> >     Why not make this optional? I suspect it's not really useful e.g. for
> >     dpdk or xdp which do not generally use hardware offloads.
> >
> > Good point, will make it optional.
> >
> >
> >     > +
> >     > +Whenever the driver wants to discard the current RSS settings, it
> can
> >     send an
> >     > +VIRTIO_NET_SM_CTRL_RSS_SET command along with rss structure that
> has
> >     > +RSS_HASH_FUNCTION_NONE the \field{hash_function} field.
> >
> >     And then what happens?
> >
> > and then the device is configured with RSS settings
>
>
> so NONE will enable RSS?
>
> > and MUST start distributing
> > the packets into the queues accordingly while attaching the calculated
> hash to
> > the header.
> >
> >
> >     > +
> >     > +The driver MUST check the \field{ack} field value provided by the
> >     device, in
> >     > +case the value is not VIRTIO_NET_OK then the driver MUST handle
> failure
> >     and
> >     > +retry another hash function or else give up.
> >
> >     Is there anything special here?
> >
> > Not really just describing the error handling in detail.
>
> If it applies to all commands maybe add it to generic code.
>
> >
> >     > +
> >     >  \subparagraph{Legacy Interface: Automatic receive steering in
> multiqueue
> >     mode}\label{sec:Device Types / Network Device / Device Operation /
> Control
> >     Virtqueue / Automatic receive steering in multiqueue mode / Legacy
> >     Interface: Automatic receive steering in multiqueue mode}
> >     >  When using the legacy interface, transitional devices and drivers
> >     >  MUST format \field{virtqueue_pairs}
> >
> >     conformance statements will need to be updated.
> >
> > which statements for example?
>
> I mean links are needed in conformance.tex
>
Do you think that the Steering Mode should be in conformance?

>
> >
> >     > --
> >     > 2.13.6
> >     >
> >     >
> >     > ------------------------------------------------------------
> ---------
> >     > To unsubscribe, e-mail: virtio-dev-unsubscribe@lists.
> oasis-open.org
> >     > For additional commands, e-mail: virtio-dev-help@lists.oasis-
> open.org
> >
> >
> >
> >
> > --
> > Respectfully,
> > Sameeh Jubran
> > Linkedin
> > Software Engineer @ Daynix.
>



-- 
Respectfully,
*Sameeh Jubran*
*Linkedin <https://il.linkedin.com/pub/sameeh-jubran/87/747/a8a>*
*Software Engineer @ Daynix <http://www.daynix.com>.*

[-- Attachment #2: Type: text/html, Size: 19944 bytes --]

Re: [virtio-dev] [PATCH v2 2/2] content: net: steering mode: Add RSS

[Michael S. Tsirkin]

On Wed, Apr 18, 2018 at 01:34:48PM +0300, Sameeh Jubran wrote:
> Do you think that the Steering Mode should be in conformance?  

Any conformance clauses must be linked to from the conformance
chapter.

-- 
MST

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