OASIS Mailing List ArchivesView the OASIS mailing list archive below
or browse/search using MarkMail.

 


Help: OASIS Mailing Lists Help | MarkMail Help

virtio-dev message

[Date Prev] | [Thread Prev] | [Thread Next] | [Date Next] -- [Date Index] | [Thread Index] | [List Home]


Subject: Re: [virtio-dev] [RFC virtio-net 1/1] content: net: Add VIRTIO_NET_F_CTRL_RSS feature




On Tue, Mar 20, 2018 at 12:29 AM, Vijayabhaskar Balakrishna <vijay.balakrishna@oracle.com> wrote:
On 3/19/2018 6:33 AM, Sameeh Jubran wrote:


On Thu, Mar 15, 2018 at 4:19 AM, Jason Wang <jasowang@redhat.com> wrote:


On 2018年01月26日 00:27, Sameeh Jubran wrote:
From: Sameeh Jubran <sameeh.j@gmail.com>

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 | 133 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
  1 file changed, 133 insertions(+)

diff --git a/content.tex b/content.tex
index c840588..5969b28 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_RSS(24)] Device supports configurable RSS hash
+    functions.
  \end{description}
    \subsubsection{Feature bit requirements}\label{sec:Device Types / Network Device / Feature bits / Feature bit requirements}
@@ -3138,6 +3141,8 @@ Some networking feature bits require other networking feature bits
  \item[VIRTIO_NET_F_GUEST_ANNOUNCE] Requires VIRTIO_NET_F_CTRL_VQ.
  \item[VIRTIO_NET_F_MQ] Requires VIRTIO_NET_F_CTRL_VQ.
  \item[VIRTIO_NET_F_CTRL_MAC_ADDR] Requires VIRTIO_NET_F_CTRL_VQ.
+
+\item[VIRTIO_NET_F_CTRL_RSS] Requires VIRTIO_NET_F_MQ.
  \end{description}

Is this a requirement of RSSv2? Looks like at least RSS can work without multiqueue in V1.
You are right this is not a requirement and should be dropped. More on RSS with single hardware queue:
Do we still expect efficiency (distribution of traffic among CPUs) in case of single queue by having RSS (in host software)?
 This depends on the configuration settings for the hash function and indirection table.



    \subsubsection{Legacy Interface: Feature bits}\label{sec:Device Types / Network Device / Feature bits / Legacy Interface: Feature bits}
@@ -3308,6 +3313,7 @@ struct virtio_net_hdr {
          u8 flags;
  #define VIRTIO_NET_HDR_GSO_NONE        0
  #define VIRTIO_NET_HDR_GSO_TCPV4       1
+#define VIRTIO_NET_HDR_GSO_RSS         2

What's the reason of introducing a new GSO type here? RSS should be independent for a specific offloading type I think.
This was the straight forward option as there is no need to extend the virtio header but it can be moved to a specific offload.


  #define VIRTIO_NET_HDR_GSO_UDP         3
  #define VIRTIO_NET_HDR_GSO_TCPV6       4
  #define VIRTIO_NET_HDR_GSO_ECN      0x80
@@ -3317,6 +3323,8 @@ struct virtio_net_hdr {
          le16 csum_start;
          le16 csum_offset;
          le16 num_buffers;
+// Only if RSS hash offload has been negotiated
+        le64 rss_hash_value;
I asked Sameeh privately and asking here again.   Is compatibility maintained if we introduce a new field to virtio_net_hdr?
I believe so, only if the RSS feature has been acked then this header is used which should preserve compatibility 

Not a native English speaker, but "rss_hash" should be sufficient.
will do 


  };
  \end{lstlisting}
  @@ -4007,6 +4015,131 @@ 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{RSS hash offload}\label{sec:Device Types / Network Device / Device Operation / Control Virtqueue / RSS hash offload}
+
+\begin{lstlisting}
+#define RSS_HASH_FUNCTION_NONE      1
+#define RSS_HASH_FUNCTION_TOEPLITZ  2
+#define RSS_HASH_FUNCTION_SYMMETRIC 3
Should above be bit field?  I mean first two are fine, should ..SYMMETRIC should be 4 (0x4)    Otherwise description "Zero or more flags of the RSS_HASH_FUNCTION flags MUST be used to fill the field hash_function" below wouldn't be correct.
Each function is a distinct function, there is no sense for setting two or more functions in the field. Indeed the description is not accurate and I'll be addressing this in v2, thanks for catching this :)


+
+// 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_ctrl_rss_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];
+       }
+};
I'm new here, hence my question: Is array size typically described like above in virtio spec?  And with #define constant in implementation? 
Yes, take a look here for example:
https://github.com/oasis-tcs/virtio-spec/blob/master/content.tex#L4778 
https://github.com/oasis-tcs/virtio-spec/blob/master/content.tex#L2716

Thanks,
Vijay

+
+#define VIRTIO_NET_F_CTRL_RSS     24
+
+#define VIRTIO_NET_CTRL_RSS                           6
+#define VIRTIO_NET_CTRL_RSS_GET_SUPPORTED_FUNCTIONS   0
+#define VIRTIO_NET_CTRL_RSS_DISABLE                   1
+#define VIRTIO_NET_CTRL_RSS_SET                       2
+\end{lstlisting}
+
+If the VIRTIO_NET_F_CTRL_RSS is negotiated the driver can send control
+commands for the RSS configuration.
+
+The class VIRTIO_NET_CTRL_RSS has three commands:
+
+\begin{enumerate}
+\item VIRTIO_NET_CTRL_RSS_GET_SUPPLIED_FUNCTIONS returns the hash functions
+       supported by the device to the driver.

Can we just reuse virtio_net_rss in this case, it contains all the informations (e.g hash key or whatever others)?
Sure this can work 


+\item VIRTIO_NET_CTRL_RSS_DISABLE Instructs the device that RSS hashing is no
+       longer required.

So this implies that if MQ is supported, we will go to use automatic steering mode? I was thinking maybe it's better to introduce a generic command to switch between steering mode.
 So you are suggesting that we should have an additional generic command that changes between steering modes and RSS is one of those modes?


+\item VIRTIO_NET_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}
+
+If this feaure has been negotiated, the virtio header has an additional
+field{rss_hash_value} field attached to it.
+
+\devicenormative{\subparagraph}{RSS hash offload}{Device Types / Network Device / Device Operation / Control Virtqueue / RSS hash offload}
+
+The device MUST fill the virtio_net_ctrl_rss_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.
+
+Upon recieving VIRTIO_NET_CTRL_DISABLE the device SHOULD stop calculating and
+attaching hashes

For simplicity, what if just compute the hash in this case? Consider a driver may want to use this hash (e.g Linux driver).
I get the use case but maybe introduce a new command for computing hashes only? I think the disable should be preserved for disabling only.


for the packets as well as stop setting the VIRTIO_NET_HDR_GSO_RSS
+in the \field{gso_type} field, however the \field{hash_function} field is kept
+as a part of the header.
+
+The device MUST drop all previous RSS configuration upon receiving
+VIRTIO_NET_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.

We support changing #queues dynamically, so need to clarify the behavior when e.g the destination queues were disabled.
I will go into these details in the next version. 


+
+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
+       recieved the device MUST calculate the hashing for the packet and
+       store it in the virtio-net header in rss_hash_value 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}{RSS hash offload}{Device Types / Network Device / Device Operation / Control Virtqueue / RSS hash offload}
+
+If the driver negotiates the feature VIRTIO_NET_F_CTRL_RSS the driver SHOULD
+send VIRTIO_NET_CTRL_RSS_SET command to the device along with the RSS structure
+filled. The RSS structure fields should be filled 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 approperiate flags
+       in the \field{hash_function_flags} field. These flags inidicate 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.

Is the indirection table expected to be changed frequently? If yes, a single entry modification will cause a update of the whole table.
No, it should not be changing frequently


+\end{itemize}
+Once the configuration phase is over successfully, the packets SHOULD have the
+\field{rss_hash_value} 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_CTRL_RSS_SET command along with rss structure that has
+RSS_HASH_FUNCTION_NONE the \field{hash_function} field.

So this function looks equivalent to VIRTIO_NET_CTRL_RSS_DISABLE?
Yes, do you think we should discard one of the two?

Thanks


+
+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 hanlde faliure 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
Software Engineer @ Daynix.




--
Respectfully,
Sameeh Jubran
Software Engineer @ Daynix.


[Date Prev] | [Thread Prev] | [Thread Next] | [Date Next] -- [Date Index] | [Thread Index] | [List Home]