On Tue, Apr 17, 2018 at 5:14 PM, Michael S. Tsirkin <[email protected]> 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 <[email protected]> > wrote: > > > > On Mon, Apr 16, 2018 at 04:05:26PM +0300, Sameeh Jubran wrote: > > > From: Sameeh Jubran <[email protected]> > > > > > > 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 <[email protected]> > > > --- > > > 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: [email protected] > 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>.*
