From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: virtio-dev-return-3848-cohuck=redhat.com@lists.oasis-open.org Sender: List-Post: List-Help: List-Unsubscribe: List-Subscribe: Received: from lists.oasis-open.org (oasis-open.org [66.179.20.138]) by lists.oasis-open.org (Postfix) with ESMTP id C6DB95818F86 for ; Tue, 17 Apr 2018 03:59:34 -0700 (PDT) MIME-Version: 1.0 In-Reply-To: References: <20180416130526.6233-1-sameeh@daynix.com> <20180416130526.6233-2-sameeh@daynix.com> <20180416234515-mutt-send-email-mst@kernel.org> From: Sameeh Jubran Date: Tue, 17 Apr 2018 13:59:18 +0300 Message-ID: Content-Type: multipart/alternative; boundary="94eb2c0e4b5eeb0812056a093a48" Subject: Re: [virtio-dev] [PATCH v2 1/2] content: net: Add VIRTIO_NET_F_CTRL_STEERING_MODE feature To: "Michael S. Tsirkin" Cc: virtio-dev , Jason Wang , Vijayabhaskar Balakrishna , Yan Vugenfirer , Vlad Yasevich List-ID: --94eb2c0e4b5eeb0812056a093a48 Content-Type: text/plain; charset="UTF-8" On Tue, Apr 17, 2018 at 1:43 PM, Sameeh Jubran wrote: > > > On Tue, Apr 17, 2018 at 2:05 AM, Michael S. Tsirkin > wrote: > >> On Mon, Apr 16, 2018 at 04:05:25PM +0300, Sameeh Jubran wrote: >> > From: Sameeh Jubran >> > >> > 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 >> > --- >> > 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 .* --94eb2c0e4b5eeb0812056a093a48 Content-Type: text/html; charset="UTF-8" Content-Transfer-Encoding: quoted-printable


On Tue, Apr 17, 2018 at 1:43 PM, Sameeh Jubran <sameeh@daynix.com&= gt; wrote:


On Tue, Apr 17, 2018 at 2:05 AM, Michael S. Tsirkin &= lt;mst@redhat.com&g= t; wrote:
On Mon, Apr 16, 2018 at 04:05:25PM +0300, Same= eh 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>
> ---
>=C2=A0 content.tex | 59 ++++++++++++++++++++++++++++++++++++++++++= +++++++++++++++++
>=C2=A0 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.
>=C2=A0
>=C2=A0 \item[VIRTIO_NET_F_CTRL_MAC_ADDR(23)] Set MAC address throu= gh control
>=C2=A0 =C2=A0 =C2=A0 channel.
> +
> +\item[VIRTIO_NET_F_CTRL_STEERING_MODE(27)] Device supports confi= gurable steering
> +=C2=A0 =C2=A0 mode.
>=C2=A0 \end{description}
>=C2=A0
>=C2=A0 \subsubsection{Feature bit requirements}\label{sec:Device Types = / Network Device / Feature bits / Feature bit requirements}
> @@ -3317,6 +3320,8 @@ struct virtio_net_hdr {
>=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 le16 csum_start;
>=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 le16 csum_offset;
>=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 le16 num_buffers;
> +// Only if VIRTIO_NET_F_CTRL_STEERING_MODE has been negotiated > +=C2=A0 =C2=A0 =C2=A0 =C2=A0 le64 hash;
>=C2=A0 };
>=C2=A0 \end{lstlisting}
>=C2=A0
> @@ -4007,6 +4012,60 @@ VIRTIO_NET_CTRL_MQ_VQ_PAIRS_SET command. >=C2=A0 The device MUST NOT queue packets on receive queues greater than=
>=C2=A0 \field{virtqueue_pairs} once it has placed the VIRTIO_NET_CTRL_M= Q_VQ_PAIRS_SET command in the used ring.
>=C2=A0
> +\paragraph{Steering mode}\label{sec:Device Types / Network Device / D= evice Operation / Control Virtqueue / Steering mode}
> +
> +\begin{lstlisting}
> +// steering mode flags
> +#define STEERING_MODE_AUTO=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 1
> +
> +struct virtio_net_steering_modes {
> +le32 steering_modes;
> +};
> +
> +struct virtio_net_steering_mode {
> +le32 steering_mode;
> +le32 command;
> +
> +=C2=A0 =C2=A0 union {
> +=C2=A0 =C2=A0 }
> +};
> +
> +#define VIRTIO_NET_F_CTRL_STEERING_MODE=C2=A0 =C2=A0 =C2=A027
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.=C2=A0

Pls coordinate with Vlad (cc'd) who is adding a new feature too.

> +
> +#define VIRTIO_NET_CTRL_STEERING_MODE=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2= =A0 =C2=A0 =C2=A0 =C2=A0 7
> +#define VIRTIO_NET_CTRL_SM_GET_SUPPORTED_MODES=C2=A0 =C2=A0 =C2= =A0 =C2=A0 0
> +#define VIRTIO_NET_CTRL_SM_CONTROL=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 = =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 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 se= t of commands.
> +
> +The auto steering mode is the default mode if nothing else has been c= onfigured 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.
> +=C2=A0 =C2=A0 =C2=A0The command should be executed by the driver befo= re attempting to configure the steering mode. Once the device
> +=C2=A0 =C2=A0 =C2=A0receives this command it should fill the virtio_n= et_steering_modes structure with the supported steering mode
> +=C2=A0 =C2=A0 =C2=A0flags.
> +
> +\item The command VIRTIO_NET_CTRL_SM_CONTROL is used for controlling = the different steering modes. Each steering mode
> +=C2=A0 =C2=A0 =C2=A0has its own set of commands. When executing this = command the structure virtio_net_steering_mode should be used as follows: > +=C2=A0 =C2=A0 =C2=A0the \field{steering_mode} should be filled with o= ne of the steering mode flags along with an appropriate command from the ch= osen
> +=C2=A0 =C2=A0 =C2=A0steering mode. The union of virtio_net_steering_m= ode 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 Devi= ce / Device Operation / Control Virtqueue / Steering mode / Automatic Recei= ve Steering}
> +
> +This is the default steering mode, please refer to the "Automati= c receive steering in multiqueue" section.
> +
>=C2=A0 \subparagraph{Legacy Interface: Automatic receive steering in mu= ltiqueue 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}
>=C2=A0 When using the legacy interface, transitional devices and driver= s
>=C2=A0 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.
<= /div>
What are you referring to specifically? Can you please elab= orate?=C2=A0

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,
<= /div>
Just to clarify, Windows does provide us= with hash for the packets so we can forward them to the device using the h= ash 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 calcula= tion?
We are already extending the virtio-header for RSC feature = which didn't make it into spec somehow.=C2=A0


> --
> 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 @ Da= ynix.



--
Respectfully,
<= /font>
Sameeh Jubran
Soft= ware Engineer @ Daynix<= /a>.
--94eb2c0e4b5eeb0812056a093a48--