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

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

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

Respectfully,

Sameeh Jubran

Linkedin

Software Engineer @ Daynix.

virtio-dev message