[Date Prev] | [Thread Prev] | [Thread Next] | [Date Next] -- [Date Index] | [Thread Index] | [List Home]
Subject: Re: [virtio-dev] [PATCH v2 2/2] content: net: steering mode: Add RSS
Exactly.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?
>
>
>
> > +\end{lstlisting}
> > +
> > +If the VIRTIO_NET_F_CTRL_STEERING_MODE is negotiated the driver can send Then maybe this sentence should not imply that VIRTIO_NET_F_CTRL_STEERING_
> 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.
MODE
allows RSS.
We need to somehow describe it in the spec :)
> 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?
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}. As host must maintain this in memory, you can have host specify the
>
> 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.
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
> Software Engineer @ Daynix.
[Date Prev] | [Thread Prev] | [Thread Next] | [Date Next] -- [Date Index] | [Thread Index] | [List Home]