Message-ID: <20120905053425.GA7517@redhat.com>
Date:	Wed, 5 Sep 2012 08:34:25 +0300
From:	"Michael S. Tsirkin" <mst@...hat.com>
To:	Jason Wang <jasowang@...hat.com>
Cc:	rusty@...tcorp.com.au, virtualization@...ts.linux-foundation.org,
	netdev@...r.kernel.org, kvm@...r.kernel.org
Subject: Re: [PATCHv2] virtio-spec: virtio network device multiqueue support

On Wed, Sep 05, 2012 at 11:34:15AM +0800, Jason Wang wrote:
> On 09/03/2012 07:55 PM, Michael S. Tsirkin wrote:
> >At Jason's request, I am trying to help finalize the spec for
> >the new multiqueue feature.
> >
> >Changes from Jason's rfc:
> >- reserved vq 3: this makes all rx vqs even and tx vqs odd, which
> >   looks nicer to me.
> >- documented packet steering, added a generalized steering programming
> >   command. Current modes are single queue and host driven multiqueue,
> >   but I envision support for guest driven multiqueue in the future.
> 
> For host driven, more thought in the long term. Maybe we could add
> more policy to choose the rxq such as hashing, round-robin and
> cpuid.

As we discussed off-list, different guests may need wildly
different strategies. For example different queues for
different qos priorities might make a lot of sense.
So for now I'll remove the host-driven option and
add _GUEST (or maybe better name is _RX_FOLLOWS_TX)
rule which records the queue number on packet transmit and
uses that on receive.

> >- make default vqs unused when in mq mode - this wastes some memory
> >   but makes it more efficient to switch between modes as
> >   we can avoid this causing packet reordering.
> 
> Not sure whether or not this can really helps. Depending on the host
> scheduler, we may always see a disorder when we do the switching.

Since guest handles one queue at a time during switch,
won't this mean host reorders packets even with a single queue?

> >Rusty, could you please take a look and comment?
> >If this looks OK to everyone, we can proceed with finalizing the
> >implementation.  This patch is against
> >eb9fc84d0d3c46438aaab190e2401a9e5409a052 in virtio-spec git tree.
> >
> >-->
> >
> >virtio-spec: virtio network device multiqueue support
> >
> >Add multiqueue support to virtio network device.
> >Add a new feature flag VIRTIO_NET_F_MULTIQUEUE for this feature, a new
> >configuration field max_virtqueue_pairs to detect supported number of
> >virtqueues as well as a new command VIRTIO_NET_CTRL_STEERING to program
> >packet steering.
> >
> >Signed-off-by: Michael S. Tsirkin<mst@...hat.com>
> >
> >--
> >
> >diff --git a/virtio-spec.lyx b/virtio-spec.lyx
> >index 7a073f4..583debc 100644
> >--- a/virtio-spec.lyx
> >+++ b/virtio-spec.lyx
> >@@ -58,6 +58,7 @@
> >  \html_be_strict false
> >  \author -608949062 "Rusty Russell,,,"
> >  \author 1531152142 "Paolo Bonzini,,,"
> >+\author 1986246365 "Michael S. Tsirkin"
> >  \end_header
> >
> >  \begin_body
> >@@ -3896,6 +3897,37 @@ Only if VIRTIO_NET_F_CTRL_VQ set
> >  \end_inset
> >
> >
> >+\change_inserted 1986246365 1346663522
> >+ 3: reserved
> >+\end_layout
> >+
> >+\begin_layout Description
> >+
> >+\change_inserted 1986246365 1346663550
> >+4: receiveq1.
> >+ 5: transmitq1.
> >+ 6: receiveq2.
> >+ 7.
> >+ transmitq2.
> >+ ...
> >+ 2N+2:receivqN, 2N+3:transmitqN
> >+\begin_inset Foot
> >+status open
> >+
> >+\begin_layout Plain Layout
> >+
> >+\change_inserted 1986246365 1346663558
> >+Only if VIRTIO_NET_F_CTRL_VQ set.
> >+ N is indicated by max_virtqueue_pairs field.
> >+\change_unchanged
> >+
> >+\end_layout
> >+
> >+\end_inset
> >+
> >+
> >+\change_unchanged
> >+
> >  \end_layout
> >
> >  \begin_layout Description
> >@@ -4056,6 +4088,17 @@ VIRTIO_NET_F_CTRL_VLAN
> >
> >  \begin_layout Description
> >  VIRTIO_NET_F_GUEST_ANNOUNCE(21) Guest can send gratuitous packets.
> >+\change_inserted 1986246365 1346617842
> >+
> >+\end_layout
> >+
> >+\begin_layout Description
> >+
> >+\change_inserted 1986246365 1346618103
> >+VIRTIO_NET_F_MULTIQUEUE(22) Device has multiple receive and transmission
> >+ queues.
> >+\change_unchanged
> >+
> >  \end_layout
> >
> >  \end_deeper
> >@@ -4068,11 +4111,45 @@ configuration
> >  \begin_inset space ~
> >  \end_inset
> >
> >-layout Two configuration fields are currently defined.
> >+layout
> >+\change_deleted 1986246365 1346671560
> >+Two
> >+\change_inserted 1986246365 1346671647
> >+Six
> >+\change_unchanged
> >+ configuration fields are currently defined.
> >   The mac address field always exists (though is only valid if VIRTIO_NET_F_MAC
> >   is set), and the status field only exists if VIRTIO_NET_F_STATUS is set.
> >   Two read-only bits are currently defined for the status field: VIRTIO_NET_S_LIN
> >  K_UP and VIRTIO_NET_S_ANNOUNCE.
> >+
> >+\change_inserted 1986246365 1346672138
> >+ The following four read-only fields only exists if VIRTIO_NET_F_MULTIQUEUE
> >+ is set.
> >+ The max_virtqueue_pairs field specifies the maximum number of each of transmit
> >+ and receive virtqueues that can used for multiqueue operation.
> 
> s/can/can be/
> >+ The following read-only fields:
> >+\emph on
> >+current_steering_rule
> >+\emph default
> >+,
> >+\emph on
> >+reserved
> >+\emph default
> >+ and
> >+\emph on
> >+current_steering_param
> >+\emph default
> >+ store the last successful VIRTIO_NET_CTRL_STEERING
> >+\begin_inset CommandInset ref
> >+LatexCommand ref
> >+reference "sub:Transmit-Packet-Steering"
> >+
> >+\end_inset
> >+
> >+ command executed by driver, for debugging.
> >+
> >+\change_unchanged
> >
> >  \begin_inset listings
> >  inline false
> >@@ -4105,6 +4182,40 @@ struct virtio_net_config {
> >  \begin_layout Plain Layout
> >
> >      u16 status;
> >+\change_inserted 1986246365 1346671221
> >+
> >+\end_layout
> >+
> >+\begin_layout Plain Layout
> >+
> >+\change_inserted 1986246365 1346671532
> >+
> >+    u16 max_virtqueue_pairs;
> >+\end_layout
> >+
> >+\begin_layout Plain Layout
> >+
> >+\change_inserted 1986246365 1346671531
> >+
> >+    u8 current_steering_rule;
> >+\change_unchanged
> >+
> >+\end_layout
> >+
> >+\begin_layout Plain Layout
> >+
> >+\change_inserted 1986246365 1346671499
> >+
> >+    u8 reserved;
> >+\end_layout
> >+
> >+\begin_layout Plain Layout
> >+
> >+\change_inserted 1986246365 1346671530
> >+
> >+    u16 current_steering_param;
> >+\change_unchanged
> >+
> >  \end_layout
> >
> >  \begin_layout Plain Layout
> >@@ -4151,6 +4262,18 @@ physical
> >  \begin_layout Enumerate
> >  If the VIRTIO_NET_F_CTRL_VQ feature bit is negotiated, identify the control
> >   virtqueue.
> >+\change_inserted 1986246365 1346618052
> >+
> >+\end_layout
> >+
> >+\begin_layout Enumerate
> >+
> >+\change_inserted 1986246365 1346618175
> >+If VIRTIO_NET_F_MULTIQUEUE feature bit is negotiated, identify the receive
> >+ and transmission queues that are going to be used in multiqueue mode.
> >+ Only queues that are going to be used need to be initialized.
> >+\change_unchanged
> >+
> >  \end_layout
> >
> >  \begin_layout Enumerate
> >@@ -4168,7 +4291,11 @@ status
> >  \end_layout
> >
> >  \begin_layout Enumerate
> >-The receive virtqueue should be filled with receive buffers.
> >+The receive virtqueue
> >+\change_inserted 1986246365 1346618180
> >+(s)
> >+\change_unchanged
> >+ should be filled with receive buffers.
> >   This is described in detail below in
> >  \begin_inset Quotes eld
> >  \end_inset
> >@@ -4516,6 +4643,201 @@ Note that the header will be two bytes longer for the VIRTIO_NET_F_MRG_RXBUF
> >  \end_layout
> >
> >  \begin_layout Subsection*
> >+
> >+\change_inserted 1986246365 1346670975
> >+\begin_inset CommandInset label
> >+LatexCommand label
> >+name "sub:Transmit-Packet-Steering"
> >+
> >+\end_inset
> >+
> >+Transmit Packet Steering
> >+\end_layout
> >+
> 
> Since this needs control vq, move this after the section of control vq?

I don't mind but this is also transmit related.

> >+\begin_layout Standard
> >+
> >+\change_inserted 1986246365 1346670592
> >+When VIRTIO_NET_F_MULTIQUEUE feature bit is negotiated, guest can use any
> >+ of multiple configured transmit queues to transmit a given packet.
> >+ To avoid packet reordering by device (which gnerally leads to performance
> >+ degradation) driver should attempt to utilize the same transmit virtqueue
> >+ for all packets of a given transmit flow.
> >+ For bi-directional protocols (in practice, TCP), a given network connection
> >+ can utilize both transmit and receive queues.
> >+ For best performance, packets from a single connection should utilize the
> >+ paired transmit and receive queues from the same virtqueue pair; for example
> >+ both transmitqN and receiveqN.
> >+ This rule makes it possible to optimize processing on the device side,
> >+ but this is not a hard requirement: devices should function correctly even
> >+ when this rule is not followed.
> >+\end_layout
> >+
> >+\begin_layout Standard
> >+
> >+\change_inserted 1986246365 1346670727
> >+Driver selects an active steering rule using VIRTIO_NET_CTRL_STEERING command
> >+ (this controls both which virtqueue is selected for a given packet for
> >+ receive and notifies the device which virtqueues are about to be used for
> >+ transmit).
> >+\end_layout
> >+
> >+\begin_layout Standard
> >+
> 
> Not sure it's suitable to define the policy in the spec ( and even a
> not hard requirement ). An alternative is to just define some tools
> such as the queue number negotiation mechanism like modern card and
> just let the driver to choose the policy, this can give both
> flexibility and space for the future evolution of the device. E.g.
> we may have a guest controlled flow director in the future, we can
> have a seperate command to enable/disable it and a separate feature
> bits.

Problem with such approaches is, if we make them very generic they
will be slow (interpreter) or hard to implement (jit compiler).
I think rx follows tx above solves this neatly without
a lot of complexity.


> >+\change_inserted 1986246365 1346670594
> >+This command accepts a single out argument in the following format:
> >+\end_layout
> >+
> >+\begin_layout Standard
> >+
> >+\change_inserted 1986246365 1346670594
> >+\begin_inset listings
> >+inline false
> >+status open
> >+
> >+\begin_layout Plain Layout
> >+
> >+\change_inserted 1986246365 1346670594
> >+
> >+#define VIRTIO_NET_CTRL_STEERING       4
> >+\end_layout
> >+
> >+\begin_layout Plain Layout
> >+
> >+\change_inserted 1986246365 1346670594
> >+
> >+\end_layout
> >+
> >+\begin_layout Plain Layout
> >+
> >+\change_inserted 1986246365 1346670594
> >+
> >+struct virtio_net_ctrl_steering {
> >+\end_layout
> >+
> >+\begin_layout Plain Layout
> >+
> >+\change_inserted 1986246365 1346671425
> >+
> >+	u8 current_steering_rule;
> >+\end_layout
> >+
> >+\begin_layout Plain Layout
> >+
> >+\change_inserted 1986246365 1346670594
> >+
> >+    u8 reserved;
> >+\end_layout
> >+
> >+\begin_layout Plain Layout
> >+
> >+\change_inserted 1986246365 1346671485
> >+
> >+	u16 current_steering_param;
> >+\end_layout
> >+
> >+\begin_layout Plain Layout
> >+
> >+\change_inserted 1986246365 1346670594
> >+
> >+};
> >+\end_layout
> >+
> >+\begin_layout Plain Layout
> >+
> >+\change_inserted 1986246365 1346670594
> >+
> >+#define VIRTIO_NET_CTRL_STEERING_SINGLE       0
> >+\end_layout
> >+
> >+\begin_layout Plain Layout
> >+
> >+\change_inserted 1986246365 1346670594
> >+
> >+#define VIRTIO_NET_CTRL_STEERING_HOST  1
> >+\end_layout
> >+
> >+\end_inset
> >+
> >+
> >+\end_layout
> >+
> >+\begin_layout Standard
> >+
> >+\change_inserted 1986246365 1346671803
> >+The field
> >+\emph on
> >+rule
> >+\emph default
> >+ specifies the function used to select transmit virtqueue for a given packet;
> >+ the field
> >+\emph on
> >+param
> >+\emph default
> >+ makes it possible to pass an extra parameter if appropriate.
> >+ When
> >+\emph on
> >+rule
> >+\emph default
> >+ is set to VIRTIO_NET_CTRL_STEERING_SINGLE all packets are steered to the
> >+ default virtqueue transmitq (1); param is unused; this is the default.
> >+ When
> >+\emph on
> >+rule
> >+\emph default
> >+ is set to VIRTIO_NET_CTRL_STEERING_HOST packets are steered by driver to
> >+ the first (
> >+\emph on
> >+param
> >+\emph default
> >++1) multiqueue virtqueues transmitq1...transmitqN; the default transmitq is
> >+ unused.
> 
> It's driver that select the transmitq, does the device need to know
> about the transmit steering used in driver? defining the those
> transmission steering commands for the deivce looks useless. Maybe
> we'd better rename the subsection to "Packet Steering" and focus on
> the receive part?

Fow rx follows tx device needs to know how many tx queues
will be used.
Also I think it's good to be explicit and write up what
we expect driver to do rather than leave it to
drivers to interpret the spec any way they like.

> >+ Driver must have configured all these (
> >+\emph on
> >+param
> >+\emph default
> >++1) virtqueues beforehand.
> >+ For best performance, driver should detects flow to virtqueue pair mapping
> >+ on receive and selects the transmit virtqueue from the same virtqueue pair.
> >+\end_layout
> >+
> >+\begin_layout Standard
> >+
> >+\change_inserted 1986246365 1346670762
> >+Supported steering rules can be added and removed in the future.
> >+ Driver should probe for supported rules by checking ack values of the command.
> >+\end_layout
> >+
> 
> What's the advantages of using this over feature bits?

Feature bits are negotiated at startup. Experience shows
there might be no ideal steering rule for all workloads
so switching them at runtime is needed.


> >+\begin_layout Standard
> >+
> >+\change_inserted 1986246365 1346671315
> >+When steering rule is modified, some packets can still be outstanding in
> >+ one or more of the virtqueues.
> >+ For transmit, device is recommended to complete processing of the transmit
> >+ queue(s) utilized by the original steering before processing any packets
> >+ delivered by the modified steering rule.
> >+\end_layout
> >+
> >+\begin_layout Standard
> >+
> >+\change_inserted 1986246365 1346671412
> >+For debugging, current steering rule can also be read from the configuration
> >+ space.
> >+\end_layout
> >+
> >+\begin_layout Standard
> >+
> >+\change_inserted 1986246365 1346670762
> >+See also receive steering description
> >+\begin_inset CommandInset ref
> >+LatexCommand ref
> >+reference "sub:Receive-Packet-Steering"
> >+
> >+\end_inset
> >+
> >+.
> >+\end_layout
> >+
> >+\begin_layout Subsection*
> >  Packet Transmission Interrupt
> >  \end_layout
> >
> >@@ -4988,8 +5310,24 @@ status open
> >  The Guest needs to check VIRTIO_NET_S_ANNOUNCE bit in status field when
> >   it notices the changes of device configuration.
> >   The command VIRTIO_NET_CTRL_ANNOUNCE_ACK is used to indicate that driver
> >- has recevied the notification and device would clear the VIRTIO_NET_S_ANNOUNCE
> >- bit in the status filed after it received this command.
> >+ has rece
> >+\change_inserted 1986246365 1346663932
> >+i
> >+\change_unchanged
> >+v
> >+\change_deleted 1986246365 1346663934
> >+i
> >+\change_unchanged
> >+ed the notification and device would clear the VIRTIO_NET_S_ANNOUNCE bit
> >+ in the status fi
> >+\change_inserted 1986246365 1346663942
> >+e
> >+\change_unchanged
> >+l
> >+\change_deleted 1986246365 1346663943
> >+e
> >+\change_unchanged
> >+d after it received this command.
> >  \end_layout
> >
> >  \begin_layout Standard
> >@@ -5004,10 +5342,101 @@ Sending the gratuitous packets or marking there are pending gratuitous packets
> >  \begin_layout Enumerate
> >  Sending VIRTIO_NET_CTRL_ANNOUNCE_ACK command through control vq.
> >
> >+\change_deleted 1986246365 1346662247
> >+
> >  \end_layout
> >
> >-\begin_layout Enumerate
> >+\begin_layout Subsection*
> >+
> >+\change_inserted 1986246365 1346670357
> >+\begin_inset CommandInset label
> >+LatexCommand label
> >+name "sub:Receive-Packet-Steering"
> >+
> >+\end_inset
> >+
> >+Receive Packet Steering
> >+\end_layout
> >+
> >+\begin_layout Standard
> >+
> >+\change_inserted 1986246365 1346671046
> >+When VIRTIO_NET_F_MULTIQUEUE feature bit is negotiated, device can use any
> >+ of multiple configured receive queues to pass a given packet to driver.
> >+ Driver controls which virtqueue is selected in practice by configuring
> >+ packet steering rule using VIRTIO_NET_CTRL_STEERING command, as described
> >+ above
> >+\begin_inset CommandInset ref
> >+LatexCommand ref
> >+reference "sub:Transmit-Packet-Steering"
> >+
> >+\end_inset
> >+
> >  .
> >+\end_layout
> >+
> >+\begin_layout Standard
> >+
> >+\change_inserted 1986246365 1346671818
> >+The field
> >+\emph on
> >+rule
> >+\emph default
> >+ specifies the function used to select receive virtqueue for a given packet;
> >+ the field
> >+\emph on
> >+param
> >+\emph default
> >+ makes it possible to pass an extra parameter if appropriate.
> >+ When
> >+\emph on
> >+rule
> >+\emph default
> >+ is set to VIRTIO_NET_CTRL_STEERING_SINGLE all packets are steered to the
> >+ default virtqueue receveq (0); param is unused; this is the default.
> 
> A question here is whether or not we need to do the seamless
> switching between mq modes and sq modes. If we expect it happens
> rare, we could afforad some disordering by dropping this and just do
> the switching by resetting the device and negotiating the MULTIQUEUE
> bit or not on demand.

So far your own performance results show there is no
ideal mode. And to me, this makes a lot of sense.
If you reset on switch this is very slow, so this basically means you
will forever rely on guest admin to know what the right thing to do is.
I agree it's a sensible thing to do in v1 but long term
an adaptive strategy requires that mode switch is lightweight.

> This can simplify the implemention.

It only seems like that. That's much more work for a driver to do, and
lots of races to avoid.  Just using vqs negotiated at startup is exactly
what all drivers currently do - it is much easier to do correctly.

> >+ When
> >+\emph on
> >+rule
> >+\emph default
> >+ is set to VIRTIO_NET_CTRL_STEERING_HOST packets are steered by host using
> >+ a device-specific steering function to the first (
> >+\emph on
> >+param
> >+\emph default
> >++1) multiqueue virtqueues receiveq1...receiveqN; the default receiveq is unused.
> >+ Driver must have configured all these (
> >+\emph on
> >+param
> >+\emph default
> >++1) virtqueues beforehand.
> >+ For best performance, driver is expected to detect flow to virtqueue pair
> >+ mapping on receive and select the transmit virtqueue from the same virtqueue
> >+ pair.
> >+\end_layout
> >+
> >+\begin_layout Standard
> >+
> >+\change_inserted 1986246365 1346669564
> >+Supported steering rules can be added and removed in the future.
> >+ Driver should probe for supported rules by checking ack values of the command.
> >+\end_layout
> >+
> >+\begin_layout Standard
> >+
> >+\change_inserted 1986246365 1346671151
> >+When steering rule is modified, some packets can still be outstanding in
> >+ one or more of the virtqueues.
> >+ For receive, driver is recommended to complete processing of the receive
> >+ queue(s) utilized by the original steering before processing any packets
> >+ delivered by the modified steering rule.
> >+\end_layout
> >+
> >+\begin_layout Standard
> >+
> >+\change_deleted 1986246365 1346664095
> >+.
> >+
> >+\change_unchanged
> >
> >  \end_layout
> >
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Powered by blists - more mailing lists