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

[Sameeh Jubran <sameeh@daynix.com>]

[-- 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 --]