Re: [net-next RFC V5 4/5] virtio_net: multiqueue support
On 07/29/2012 11:44 AM, Michael S. Tsirkin wrote: On Sat, Jul 21, 2012 at 02:02:58PM +0200, Sasha Levin wrote: On 07/20/2012 03:40 PM, Michael S. Tsirkin wrote: - err = init_vqs(vi); + if (virtio_has_feature(vdev, VIRTIO_NET_F_CTRL_VQ)) + vi-has_cvq = true; + How about we disable multiqueue if there's no cvq? Will make logic a bit simpler, won't it? multiqueues don't really depend on cvq. Does this added complexity really justifies adding an artificial limit? Well !cvq support is a legacy feature: the reason we support it in driver is to avoid breaking on old hosts. Adding more code to that path just doesn't make much sense since old hosts won't have mq. Is it really a legacy feature? The spec suggests that its an optional queue which is not necessary for the operation of the device. Which is why we never implemented it in lkvm - we weren't interested in any of the features it provided at that time and we could provide high performance with vhost support even without it. ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [net-next RFC V5 4/5] virtio_net: multiqueue support
On Sat, Jul 21, 2012 at 02:02:58PM +0200, Sasha Levin wrote: On 07/20/2012 03:40 PM, Michael S. Tsirkin wrote: - err = init_vqs(vi); +if (virtio_has_feature(vdev, VIRTIO_NET_F_CTRL_VQ)) +vi-has_cvq = true; + How about we disable multiqueue if there's no cvq? Will make logic a bit simpler, won't it? multiqueues don't really depend on cvq. Does this added complexity really justifies adding an artificial limit? Well !cvq support is a legacy feature: the reason we support it in driver is to avoid breaking on old hosts. Adding more code to that path just doesn't make much sense since old hosts won't have mq. -- MST ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [net-next RFC V5 4/5] virtio_net: multiqueue support
On Mon, Jul 23, 2012 at 01:48:35PM +0800, Jason Wang wrote:
+ }
- if (virtio_has_feature(vi-vdev, VIRTIO_NET_F_CTRL_VQ)) {
+ ret = vi-vdev-config-find_vqs(vi-vdev, total_vqs, vqs, callbacks,
+(const char **)names);
+ if (ret)
+ goto err;
+
+ if (vi-has_cvq)
vi-cvq = vqs[2];
- if (virtio_has_feature(vi-vdev, VIRTIO_NET_F_CTRL_VLAN))
- vi-dev-features |= NETIF_F_HW_VLAN_FILTER;
+ for (i = 0; i vi-num_queue_pairs * 2; i += 2) {
+ int j = i == 0 ? i : i + vi-has_cvq;
+ vi-rq[i / 2]-vq = vqs[j];
+ vi-sq[i / 2]-vq = vqs[j + 1];
Same here.
Consider the code is really simple, seem no need to use helpers.
Well it was not simple to at least one reader :)
The problem is not this logic is complex,
it is that it is spread all over the code.
If we had e.g. vnet_tx_vqn_to_queuenum vnet_tx_queuenum_to_vqn
and same for rx, then the logic would all be
in one place, and have a tidy comment on top explaining
the VQ numbering scheme.
--
MST
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [net-next RFC V5 4/5] virtio_net: multiqueue support
On 07/29/2012 05:44 PM, Michael S. Tsirkin wrote: On Sat, Jul 21, 2012 at 02:02:58PM +0200, Sasha Levin wrote: On 07/20/2012 03:40 PM, Michael S. Tsirkin wrote: - err = init_vqs(vi); + if (virtio_has_feature(vdev, VIRTIO_NET_F_CTRL_VQ)) + vi-has_cvq = true; + How about we disable multiqueue if there's no cvq? Will make logic a bit simpler, won't it? multiqueues don't really depend on cvq. Does this added complexity really justifies adding an artificial limit? Well !cvq support is a legacy feature: the reason we support it in driver is to avoid breaking on old hosts. Adding more code to that path just doesn't make much sense since old hosts won't have mq. After some thought about this, maybe there's no need to the cvq for the negotiation if we want support only two modes ( 1 tx/rx queue pair and N tx/rx queue pairs). We can do this just through the feature bit negotiation. ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [net-next RFC V5 4/5] virtio_net: multiqueue support
On 07/23/2012 05:28 PM, Sasha Levin wrote: On 07/23/2012 07:54 AM, Jason Wang wrote: On 07/21/2012 08:02 PM, Sasha Levin wrote: On 07/20/2012 03:40 PM, Michael S. Tsirkin wrote: -err = init_vqs(vi); +if (virtio_has_feature(vdev, VIRTIO_NET_F_CTRL_VQ)) +vi-has_cvq = true; + How about we disable multiqueue if there's no cvq? Will make logic a bit simpler, won't it? multiqueues don't really depend on cvq. Does this added complexity really justifies adding an artificial limit? Yes, it does not depends on cvq. Cvq were just used to negotiate the number of queues a guest wishes to use which is really useful (at least for now). Since multiqueue can not out-perform for single queue in every kinds of workloads or benchmark, so we want to let guest driver use single queue by default even when multiqueue were enabled by management software and let use to enalbe it through ethtool. So user could not feel regression when it switch to use a multiqueue capable driver and backend. Why would you limit it to a single vq if the user has specified a different number of vqs (1) in the virtio-net device config? -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/ The only reason is to prevent the user from seeing the regression. The performance of small packet sending is wrose than single queue, it tends to send more but small packets when multiqueue is enabled. If we make multiqueue bahave as good as single queue, we can remove this limit. ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [net-next RFC V5 4/5] virtio_net: multiqueue support
On 07/23/2012 07:54 AM, Jason Wang wrote: On 07/21/2012 08:02 PM, Sasha Levin wrote: On 07/20/2012 03:40 PM, Michael S. Tsirkin wrote: -err = init_vqs(vi); +if (virtio_has_feature(vdev, VIRTIO_NET_F_CTRL_VQ)) +vi-has_cvq = true; + How about we disable multiqueue if there's no cvq? Will make logic a bit simpler, won't it? multiqueues don't really depend on cvq. Does this added complexity really justifies adding an artificial limit? Yes, it does not depends on cvq. Cvq were just used to negotiate the number of queues a guest wishes to use which is really useful (at least for now). Since multiqueue can not out-perform for single queue in every kinds of workloads or benchmark, so we want to let guest driver use single queue by default even when multiqueue were enabled by management software and let use to enalbe it through ethtool. So user could not feel regression when it switch to use a multiqueue capable driver and backend. Why would you limit it to a single vq if the user has specified a different number of vqs (1) in the virtio-net device config? ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [net-next RFC V5 4/5] virtio_net: multiqueue support
On 07/20/2012 09:40 PM, Michael S. Tsirkin wrote:
On Thu, Jul 05, 2012 at 06:29:53PM +0800, Jason Wang wrote:
This patch converts virtio_net to a multi queue device. After negotiated
VIRTIO_NET_F_MULTIQUEUE feature, the virtio device has many tx/rx queue pairs,
and driver could read the number from config space.
The driver expects the number of rx/tx queue paris is equal to the number of
vcpus. To maximize the performance under this per-cpu rx/tx queue pairs, some
optimization were introduced:
- Txq selection is based on the processor id in order to avoid contending a lock
whose owner may exits to host.
- Since the txq/txq were per-cpu, affinity hint were set to the cpu that owns
the queue pairs.
Signed-off-by: Krishna Kumarkrkum...@in.ibm.com
Signed-off-by: Jason Wangjasow...@redhat.com
Overall fine. I think it is best to smash the following patch
into this one, so that default behavior does not
jump to mq then back. some comments below: mostly nits, and a minor bug.
Sure, thanks for the reviewing.
If you are worried the patch is too big, it can be split
differently
- rework to use send_queue/receive_queue structures, no
functional changes.
- add multiqueue
but this is not a must.
---
drivers/net/virtio_net.c | 645 ++-
include/linux/virtio_net.h |2 +
2 files changed, 452 insertions(+), 195 deletions(-)
diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
index 1db445b..7410187 100644
--- a/drivers/net/virtio_net.c
+++ b/drivers/net/virtio_net.c
@@ -26,6 +26,7 @@
#includelinux/scatterlist.h
#includelinux/if_vlan.h
#includelinux/slab.h
+#includelinux/interrupt.h
static int napi_weight = 128;
module_param(napi_weight, int, 0444);
@@ -41,6 +42,8 @@ module_param(gso, bool, 0444);
#define VIRTNET_SEND_COMMAND_SG_MAX2
#define VIRTNET_DRIVER_VERSION 1.0.0
+#define MAX_QUEUES 256
+
struct virtnet_stats {
struct u64_stats_sync tx_syncp;
struct u64_stats_sync rx_syncp;
Would be a bit better not to have artificial limits like that.
Maybe allocate arrays at probe time, then we can
take whatever the device gives us?
Sure.
@@ -51,43 +54,69 @@ struct virtnet_stats {
u64 rx_packets;
};
-struct virtnet_info {
- struct virtio_device *vdev;
- struct virtqueue *rvq, *svq, *cvq;
- struct net_device *dev;
+/* Internal representation of a send virtqueue */
+struct send_queue {
+ /* Virtqueue associated with this send _queue */
+ struct virtqueue *vq;
+
+ /* TX: fragments + linear part + virtio header */
+ struct scatterlist sg[MAX_SKB_FRAGS + 2];
+};
+
+/* Internal representation of a receive virtqueue */
+struct receive_queue {
+ /* Virtqueue associated with this receive_queue */
+ struct virtqueue *vq;
+
+ /* Back pointer to the virtnet_info */
+ struct virtnet_info *vi;
+
struct napi_struct napi;
- unsigned int status;
/* Number of input buffers, and max we've ever had. */
unsigned int num, max;
+ /* Work struct for refilling if we run low on memory. */
+ struct delayed_work refill;
+
+ /* Chain pages by the private ptr. */
+ struct page *pages;
+
+ /* RX: fragments + linear part + virtio header */
+ struct scatterlist sg[MAX_SKB_FRAGS + 2];
+};
+
+struct virtnet_info {
+ u16 num_queue_pairs;/* # of RX/TX vq pairs */
+
+ struct send_queue *sq[MAX_QUEUES] cacheline_aligned_in_smp;
+ struct receive_queue *rq[MAX_QUEUES] cacheline_aligned_in_smp;
The assumption is a tx/rx pair is handled on the same cpu, yes?
If yes maybe make it a single array to improve cache locality
a bit?
struct queue_pair {
struct send_queue sq;
struct receive_queue rq;
};
Ok.
+ struct virtqueue *cvq;
+
+ struct virtio_device *vdev;
+ struct net_device *dev;
+ unsigned int status;
+
/* I like... big packets and I cannot lie! */
bool big_packets;
/* Host will merge rx buffers for big packets (shake it! shake it!) */
bool mergeable_rx_bufs;
+ /* Has control virtqueue */
+ bool has_cvq;
+
won't checking (cvq != NULL) be enough?
Enough, so has_cvq is dupliated with vi-cvq. I will remove it in next
version.
/* enable config space updates */
bool config_enable;
/* Active statistics */
struct virtnet_stats __percpu *stats;
- /* Work struct for refilling if we run low on memory. */
- struct delayed_work refill;
-
/* Work struct for config space updates */
struct work_struct config_work;
/* Lock for config space updates */
struct mutex config_lock;
-
- /* Chain pages by the private ptr. */
- struct page *pages;
-
- /* fragments + linear part + virtio header */
- struct scatterlist rx_sg[MAX_SKB_FRAGS + 2];
Re: [net-next RFC V5 4/5] virtio_net: multiqueue support
On 07/21/2012 08:02 PM, Sasha Levin wrote: On 07/20/2012 03:40 PM, Michael S. Tsirkin wrote: - err = init_vqs(vi); + if (virtio_has_feature(vdev, VIRTIO_NET_F_CTRL_VQ)) + vi-has_cvq = true; + How about we disable multiqueue if there's no cvq? Will make logic a bit simpler, won't it? multiqueues don't really depend on cvq. Does this added complexity really justifies adding an artificial limit? Yes, it does not depends on cvq. Cvq were just used to negotiate the number of queues a guest wishes to use which is really useful (at least for now). Since multiqueue can not out-perform for single queue in every kinds of workloads or benchmark, so we want to let guest driver use single queue by default even when multiqueue were enabled by management software and let use to enalbe it through ethtool. So user could not feel regression when it switch to use a multiqueue capable driver and backend. So the only difference is the user experiences. -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/ ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [net-next RFC V5 4/5] virtio_net: multiqueue support
On 07/20/2012 03:40 PM, Michael S. Tsirkin wrote: -err = init_vqs(vi); + if (virtio_has_feature(vdev, VIRTIO_NET_F_CTRL_VQ)) + vi-has_cvq = true; + How about we disable multiqueue if there's no cvq? Will make logic a bit simpler, won't it? multiqueues don't really depend on cvq. Does this added complexity really justifies adding an artificial limit? ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [net-next RFC V5 4/5] virtio_net: multiqueue support
On Thu, Jul 05, 2012 at 06:29:53PM +0800, Jason Wang wrote:
This patch converts virtio_net to a multi queue device. After negotiated
VIRTIO_NET_F_MULTIQUEUE feature, the virtio device has many tx/rx queue pairs,
and driver could read the number from config space.
The driver expects the number of rx/tx queue paris is equal to the number of
vcpus. To maximize the performance under this per-cpu rx/tx queue pairs, some
optimization were introduced:
- Txq selection is based on the processor id in order to avoid contending a
lock
whose owner may exits to host.
- Since the txq/txq were per-cpu, affinity hint were set to the cpu that owns
the queue pairs.
Signed-off-by: Krishna Kumar krkum...@in.ibm.com
Signed-off-by: Jason Wang jasow...@redhat.com
Overall fine. I think it is best to smash the following patch
into this one, so that default behavior does not
jump to mq then back. some comments below: mostly nits, and a minor bug.
If you are worried the patch is too big, it can be split
differently
- rework to use send_queue/receive_queue structures, no
functional changes.
- add multiqueue
but this is not a must.
---
drivers/net/virtio_net.c | 645 ++-
include/linux/virtio_net.h |2 +
2 files changed, 452 insertions(+), 195 deletions(-)
diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
index 1db445b..7410187 100644
--- a/drivers/net/virtio_net.c
+++ b/drivers/net/virtio_net.c
@@ -26,6 +26,7 @@
#include linux/scatterlist.h
#include linux/if_vlan.h
#include linux/slab.h
+#include linux/interrupt.h
static int napi_weight = 128;
module_param(napi_weight, int, 0444);
@@ -41,6 +42,8 @@ module_param(gso, bool, 0444);
#define VIRTNET_SEND_COMMAND_SG_MAX2
#define VIRTNET_DRIVER_VERSION 1.0.0
+#define MAX_QUEUES 256
+
struct virtnet_stats {
struct u64_stats_sync tx_syncp;
struct u64_stats_sync rx_syncp;
Would be a bit better not to have artificial limits like that.
Maybe allocate arrays at probe time, then we can
take whatever the device gives us?
@@ -51,43 +54,69 @@ struct virtnet_stats {
u64 rx_packets;
};
-struct virtnet_info {
- struct virtio_device *vdev;
- struct virtqueue *rvq, *svq, *cvq;
- struct net_device *dev;
+/* Internal representation of a send virtqueue */
+struct send_queue {
+ /* Virtqueue associated with this send _queue */
+ struct virtqueue *vq;
+
+ /* TX: fragments + linear part + virtio header */
+ struct scatterlist sg[MAX_SKB_FRAGS + 2];
+};
+
+/* Internal representation of a receive virtqueue */
+struct receive_queue {
+ /* Virtqueue associated with this receive_queue */
+ struct virtqueue *vq;
+
+ /* Back pointer to the virtnet_info */
+ struct virtnet_info *vi;
+
struct napi_struct napi;
- unsigned int status;
/* Number of input buffers, and max we've ever had. */
unsigned int num, max;
+ /* Work struct for refilling if we run low on memory. */
+ struct delayed_work refill;
+
+ /* Chain pages by the private ptr. */
+ struct page *pages;
+
+ /* RX: fragments + linear part + virtio header */
+ struct scatterlist sg[MAX_SKB_FRAGS + 2];
+};
+
+struct virtnet_info {
+ u16 num_queue_pairs;/* # of RX/TX vq pairs */
+
+ struct send_queue *sq[MAX_QUEUES] cacheline_aligned_in_smp;
+ struct receive_queue *rq[MAX_QUEUES] cacheline_aligned_in_smp;
The assumption is a tx/rx pair is handled on the same cpu, yes?
If yes maybe make it a single array to improve cache locality
a bit?
struct queue_pair {
struct send_queue sq;
struct receive_queue rq;
};
+ struct virtqueue *cvq;
+
+ struct virtio_device *vdev;
+ struct net_device *dev;
+ unsigned int status;
+
/* I like... big packets and I cannot lie! */
bool big_packets;
/* Host will merge rx buffers for big packets (shake it! shake it!) */
bool mergeable_rx_bufs;
+ /* Has control virtqueue */
+ bool has_cvq;
+
won't checking (cvq != NULL) be enough?
/* enable config space updates */
bool config_enable;
/* Active statistics */
struct virtnet_stats __percpu *stats;
- /* Work struct for refilling if we run low on memory. */
- struct delayed_work refill;
-
/* Work struct for config space updates */
struct work_struct config_work;
/* Lock for config space updates */
struct mutex config_lock;
-
- /* Chain pages by the private ptr. */
- struct page *pages;
-
- /* fragments + linear part + virtio header */
- struct scatterlist rx_sg[MAX_SKB_FRAGS + 2];
- struct scatterlist tx_sg[MAX_SKB_FRAGS + 2];
};
struct skb_vnet_hdr {
@@ -108,6 +137,22 @@ struct padded_vnet_hdr {
char padding[6];
};
Re: [net-next RFC V5 4/5] virtio_net: multiqueue support
On 07/06/2012 04:02 AM, Amos Kong wrote:
On 07/05/2012 06:29 PM, Jason Wang wrote:
This patch converts virtio_net to a multi queue device. After negotiated
VIRTIO_NET_F_MULTIQUEUE feature, the virtio device has many tx/rx queue pairs,
and driver could read the number from config space.
The driver expects the number of rx/tx queue paris is equal to the number of
vcpus. To maximize the performance under this per-cpu rx/tx queue pairs, some
optimization were introduced:
- Txq selection is based on the processor id in order to avoid contending a lock
whose owner may exits to host.
- Since the txq/txq were per-cpu, affinity hint were set to the cpu that owns
the queue pairs.
Signed-off-by: Krishna Kumarkrkum...@in.ibm.com
Signed-off-by: Jason Wangjasow...@redhat.com
---
...
static int virtnet_probe(struct virtio_device *vdev)
{
- int err;
+ int i, err;
struct net_device *dev;
struct virtnet_info *vi;
+ u16 num_queues, num_queue_pairs;
+
+ /* Find if host supports multiqueue virtio_net device */
+ err = virtio_config_val(vdev, VIRTIO_NET_F_MULTIQUEUE,
+ offsetof(struct virtio_net_config,
+ num_queues),num_queues);
+
+ /* We need atleast 2 queue's */
s/atleast/at least/
+ if (err || num_queues 2)
+ num_queues = 2;
+ if (num_queues MAX_QUEUES * 2)
+ num_queues = MAX_QUEUES;
num_queues = MAX_QUEUES * 2;
MAX_QUEUES is the limitation of RX or TX.
Right, it's a typo, thanks.
+
+ num_queue_pairs = num_queues / 2;
...
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [net-next RFC V5 4/5] virtio_net: multiqueue support
On 07/05/2012 06:29 PM, Jason Wang wrote:
This patch converts virtio_net to a multi queue device. After negotiated
VIRTIO_NET_F_MULTIQUEUE feature, the virtio device has many tx/rx queue pairs,
and driver could read the number from config space.
The driver expects the number of rx/tx queue paris is equal to the number of
vcpus. To maximize the performance under this per-cpu rx/tx queue pairs, some
optimization were introduced:
- Txq selection is based on the processor id in order to avoid contending a
lock
whose owner may exits to host.
- Since the txq/txq were per-cpu, affinity hint were set to the cpu that owns
the queue pairs.
Signed-off-by: Krishna Kumar krkum...@in.ibm.com
Signed-off-by: Jason Wang jasow...@redhat.com
---
...
static int virtnet_probe(struct virtio_device *vdev)
{
- int err;
+ int i, err;
struct net_device *dev;
struct virtnet_info *vi;
+ u16 num_queues, num_queue_pairs;
+
+ /* Find if host supports multiqueue virtio_net device */
+ err = virtio_config_val(vdev, VIRTIO_NET_F_MULTIQUEUE,
+ offsetof(struct virtio_net_config,
+ num_queues), num_queues);
+
+ /* We need atleast 2 queue's */
s/atleast/at least/
+ if (err || num_queues 2)
+ num_queues = 2;
+ if (num_queues MAX_QUEUES * 2)
+ num_queues = MAX_QUEUES;
num_queues = MAX_QUEUES * 2;
MAX_QUEUES is the limitation of RX or TX.
+
+ num_queue_pairs = num_queues / 2;
...
--
Amos.
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
