* [PATCH qemu] virtio-net: add feature bit for any header s/g
From: Michael S. Tsirkin @ 2012-09-28 9:31 UTC (permalink / raw)
To: Thomas Lendacky
Cc: Anthony Liguori, kvm, qemu-devel, virtualization, avi,
Sasha Levin
Old qemu versions required that 1st s/g entry is the header.
My recent patchset titled "virtio-net: iovec handling cleanup"
removed this limitation but a feature
bit is needed so guests know it's safe to lay out
header differently.
This patch applies on top and adds such a feature bit.
virtio net header inline with the data is beneficial
for latency and small packet bandwidth.
Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
---
hw/virtio-net.h | 4 +++-
1 file changed, 3 insertions(+), 1 deletion(-)
diff --git a/hw/virtio-net.h b/hw/virtio-net.h
index 36aa463..e7187e4 100644
--- a/hw/virtio-net.h
+++ b/hw/virtio-net.h
@@ -44,6 +44,7 @@
#define VIRTIO_NET_F_CTRL_RX 18 /* Control channel RX mode support */
#define VIRTIO_NET_F_CTRL_VLAN 19 /* Control channel VLAN filtering */
#define VIRTIO_NET_F_CTRL_RX_EXTRA 20 /* Extra RX mode control support */
+#define VIRTIO_NET_F_ANY_HEADER_SG 22 /* Host can handle any header s/g */
#define VIRTIO_NET_S_LINK_UP 1 /* Link is up */
@@ -186,5 +187,6 @@ struct virtio_net_ctrl_mac {
DEFINE_PROP_BIT("ctrl_vq", _state, _field, VIRTIO_NET_F_CTRL_VQ, true), \
DEFINE_PROP_BIT("ctrl_rx", _state, _field, VIRTIO_NET_F_CTRL_RX, true), \
DEFINE_PROP_BIT("ctrl_vlan", _state, _field, VIRTIO_NET_F_CTRL_VLAN, true), \
- DEFINE_PROP_BIT("ctrl_rx_extra", _state, _field, VIRTIO_NET_F_CTRL_RX_EXTRA, true)
+ DEFINE_PROP_BIT("ctrl_rx_extra", _state, _field, VIRTIO_NET_F_CTRL_RX_EXTRA, true), \
+ DEFINE_PROP_BIT("any_header_sg", _state, _field, VIRTIO_NET_F_ANY_HEADER_SG, true)
#endif
--
MST
^ permalink raw reply related
* [PATCH 3/3] virtio-net: put virtio net header inline with data
From: Michael S. Tsirkin @ 2012-09-28 9:26 UTC (permalink / raw)
To: Thomas Lendacky
Cc: kvm, netdev, linux-kernel, virtualization, avi, Sasha Levin
In-Reply-To: <cover.1348824232.git.mst@redhat.com>
For small packets we can simplify xmit processing
by linearizing buffers with the header:
most packets seem to have enough head room
we can use for this purpose.
Since existing hypervisors require that header
is the first s/g element, we need a feature bit
for this.
Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
---
drivers/net/virtio_net.c | 44 +++++++++++++++++++++++++++++++++++---------
include/linux/virtio_net.h | 5 ++++-
2 files changed, 39 insertions(+), 10 deletions(-)
diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
index 316f1be..6e6e53e 100644
--- a/drivers/net/virtio_net.c
+++ b/drivers/net/virtio_net.c
@@ -67,6 +67,9 @@ struct virtnet_info {
/* Host will merge rx buffers for big packets (shake it! shake it!) */
bool mergeable_rx_bufs;
+ /* Host can handle any s/g split between our header and packet data */
+ bool any_header_sg;
+
/* enable config space updates */
bool config_enable;
@@ -576,11 +579,28 @@ static void free_old_xmit_skbs(struct virtnet_info *vi)
static int xmit_skb(struct virtnet_info *vi, struct sk_buff *skb)
{
- struct skb_vnet_hdr *hdr = skb_vnet_hdr(skb);
+ struct skb_vnet_hdr *hdr;
const unsigned char *dest = ((struct ethhdr *)skb->data)->h_dest;
unsigned num_sg;
+ unsigned hdr_len;
+ bool can_push;
+
pr_debug("%s: xmit %p %pM\n", vi->dev->name, skb, dest);
+ if (vi->mergeable_rx_bufs)
+ hdr_len = sizeof hdr->mhdr;
+ else
+ hdr_len = sizeof hdr->hdr;
+
+ can_push = vi->any_header_sg &&
+ !((unsigned long)skb->data & (__alignof__(*hdr) - 1)) &&
+ !skb_header_cloned(skb) && skb_headroom(skb) >= hdr_len;
+ /* Even if we can, don't push here yet as this would skew
+ * csum_start offset below. */
+ if (can_push)
+ hdr = (struct skb_vnet_hdr *)(skb->data - hdr_len);
+ else
+ hdr = skb_vnet_hdr(skb);
if (skb->ip_summed == CHECKSUM_PARTIAL) {
hdr->hdr.flags = VIRTIO_NET_HDR_F_NEEDS_CSUM;
@@ -609,15 +629,18 @@ static int xmit_skb(struct virtnet_info *vi, struct sk_buff *skb)
hdr->hdr.gso_size = hdr->hdr.hdr_len = 0;
}
- hdr->mhdr.num_buffers = 0;
-
- /* Encode metadata header at front. */
if (vi->mergeable_rx_bufs)
- sg_set_buf(vi->tx_sg, &hdr->mhdr, sizeof hdr->mhdr);
- else
- sg_set_buf(vi->tx_sg, &hdr->hdr, sizeof hdr->hdr);
+ hdr->mhdr.num_buffers = 0;
- num_sg = skb_to_sgvec(skb, vi->tx_sg + 1, 0, skb->len) + 1;
+ if (can_push) {
+ __skb_push(skb, hdr_len);
+ num_sg = skb_to_sgvec(skb, vi->tx_sg, 0, skb->len);
+ /* Pull header back to avoid skew in tx bytes calculations. */
+ __skb_pull(skb, hdr_len);
+ } else {
+ sg_set_buf(vi->tx_sg, hdr, hdr_len);
+ num_sg = skb_to_sgvec(skb, vi->tx_sg + 1, 0, skb->len) + 1;
+ }
return virtqueue_add_buf(vi->svq, vi->tx_sg, num_sg,
0, skb, GFP_ATOMIC);
}
@@ -1128,6 +1151,9 @@ static int virtnet_probe(struct virtio_device *vdev)
if (virtio_has_feature(vdev, VIRTIO_NET_F_MRG_RXBUF))
vi->mergeable_rx_bufs = true;
+ if (virtio_has_feature(vdev, VIRTIO_NET_F_ANY_HEADER_SG))
+ vi->any_header_sg = true;
+
err = init_vqs(vi);
if (err)
goto free_stats;
@@ -1286,7 +1312,7 @@ static unsigned int features[] = {
VIRTIO_NET_F_GUEST_ECN, VIRTIO_NET_F_GUEST_UFO,
VIRTIO_NET_F_MRG_RXBUF, VIRTIO_NET_F_STATUS, VIRTIO_NET_F_CTRL_VQ,
VIRTIO_NET_F_CTRL_RX, VIRTIO_NET_F_CTRL_VLAN,
- VIRTIO_NET_F_GUEST_ANNOUNCE,
+ VIRTIO_NET_F_GUEST_ANNOUNCE, VIRTIO_NET_F_ANY_HEADER_SG
};
static struct virtio_driver virtio_net_driver = {
diff --git a/include/linux/virtio_net.h b/include/linux/virtio_net.h
index 2470f54..16a577b 100644
--- a/include/linux/virtio_net.h
+++ b/include/linux/virtio_net.h
@@ -51,6 +51,7 @@
#define VIRTIO_NET_F_CTRL_RX_EXTRA 20 /* Extra RX mode control support */
#define VIRTIO_NET_F_GUEST_ANNOUNCE 21 /* Guest can announce device on the
* network */
+#define VIRTIO_NET_F_ANY_HEADER_SG 22 /* Host can handle any header s/g */
#define VIRTIO_NET_S_LINK_UP 1 /* Link is up */
#define VIRTIO_NET_S_ANNOUNCE 2 /* Announcement is needed */
@@ -62,7 +63,9 @@ struct virtio_net_config {
__u16 status;
} __attribute__((packed));
-/* This is the first element of the scatter-gather list. If you don't
+/* This header comes first in the scatter-gather list.
+ * If VIRTIO_NET_F_ANY_HEADER_SG is not negotiated, it must
+ * be the first element of the scatter-gather list. If you don't
* specify GSO or CSUM features, you can simply ignore the header. */
struct virtio_net_hdr {
#define VIRTIO_NET_HDR_F_NEEDS_CSUM 1 // Use csum_start, csum_offset
--
MST
^ permalink raw reply related
* [PATCH 2/3] virtio-net: correct capacity math on ring full
From: Michael S. Tsirkin @ 2012-09-28 9:26 UTC (permalink / raw)
To: Thomas Lendacky
Cc: kvm, netdev, linux-kernel, virtualization, avi, Sasha Levin
In-Reply-To: <cover.1348824232.git.mst@redhat.com>
Capacity math on ring full is wrong: we are
looking at num_sg but that might be optimistic
because of indirect buffer use.
The implementation also penalizes fast path
with extra memory accesses for the benefit of
ring full condition handling which is slow path.
It's easy to query ring capacity so let's do just that.
This change also makes it easier to move vnet header
for tx around as follow-up patch does.
Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
---
drivers/net/virtio_net.c | 15 +++++++--------
1 file changed, 7 insertions(+), 8 deletions(-)
diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
index 83d2b0c..316f1be 100644
--- a/drivers/net/virtio_net.c
+++ b/drivers/net/virtio_net.c
@@ -95,7 +95,6 @@ struct skb_vnet_hdr {
struct virtio_net_hdr hdr;
struct virtio_net_hdr_mrg_rxbuf mhdr;
};
- unsigned int num_sg;
};
struct padded_vnet_hdr {
@@ -557,10 +556,10 @@ again:
return received;
}
-static unsigned int free_old_xmit_skbs(struct virtnet_info *vi)
+static void free_old_xmit_skbs(struct virtnet_info *vi)
{
struct sk_buff *skb;
- unsigned int len, tot_sgs = 0;
+ unsigned int len;
struct virtnet_stats *stats = this_cpu_ptr(vi->stats);
while ((skb = virtqueue_get_buf(vi->svq, &len)) != NULL) {
@@ -571,16 +570,15 @@ static unsigned int free_old_xmit_skbs(struct virtnet_info *vi)
stats->tx_packets++;
u64_stats_update_end(&stats->tx_syncp);
- tot_sgs += skb_vnet_hdr(skb)->num_sg;
dev_kfree_skb_any(skb);
}
- return tot_sgs;
}
static int xmit_skb(struct virtnet_info *vi, struct sk_buff *skb)
{
struct skb_vnet_hdr *hdr = skb_vnet_hdr(skb);
const unsigned char *dest = ((struct ethhdr *)skb->data)->h_dest;
+ unsigned num_sg;
pr_debug("%s: xmit %p %pM\n", vi->dev->name, skb, dest);
@@ -619,8 +617,8 @@ static int xmit_skb(struct virtnet_info *vi, struct sk_buff *skb)
else
sg_set_buf(vi->tx_sg, &hdr->hdr, sizeof hdr->hdr);
- hdr->num_sg = skb_to_sgvec(skb, vi->tx_sg + 1, 0, skb->len) + 1;
- return virtqueue_add_buf(vi->svq, vi->tx_sg, hdr->num_sg,
+ num_sg = skb_to_sgvec(skb, vi->tx_sg + 1, 0, skb->len) + 1;
+ return virtqueue_add_buf(vi->svq, vi->tx_sg, num_sg,
0, skb, GFP_ATOMIC);
}
@@ -664,7 +662,8 @@ static netdev_tx_t start_xmit(struct sk_buff *skb, struct net_device *dev)
netif_stop_queue(dev);
if (unlikely(!virtqueue_enable_cb_delayed(vi->svq))) {
/* More just got used, free them then recheck. */
- capacity += free_old_xmit_skbs(vi);
+ free_old_xmit_skbs(vi);
+ capacity = virtqueue_get_capacity(vi->svq);
if (capacity >= 2+MAX_SKB_FRAGS) {
netif_start_queue(dev);
virtqueue_disable_cb(vi->svq);
--
MST
^ permalink raw reply related
* [PATCH 1/3] virtio: add API to query ring capacity
From: Michael S. Tsirkin @ 2012-09-28 9:26 UTC (permalink / raw)
To: Thomas Lendacky
Cc: kvm, netdev, linux-kernel, virtualization, avi, Sasha Levin
In-Reply-To: <cover.1348824232.git.mst@redhat.com>
It's sometimes necessary to query ring capacity after dequeueing a
buffer. Add an API for this.
Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
---
drivers/virtio/virtio_ring.c | 19 +++++++++++++++++++
include/linux/virtio.h | 2 ++
2 files changed, 21 insertions(+)
diff --git a/drivers/virtio/virtio_ring.c b/drivers/virtio/virtio_ring.c
index 5aa43c3..ee3d80b 100644
--- a/drivers/virtio/virtio_ring.c
+++ b/drivers/virtio/virtio_ring.c
@@ -715,4 +715,23 @@ unsigned int virtqueue_get_vring_size(struct virtqueue *_vq)
}
EXPORT_SYMBOL_GPL(virtqueue_get_vring_size);
+/**
+ * virtqueue_get_capacity - query available ring capacity
+ * @vq: the struct virtqueue we're talking about.
+ *
+ * Caller must ensure we don't call this with other virtqueue operations
+ * at the same time (except where noted), otherwise result is unreliable.
+ *
+ * Returns remaining capacity of queue.
+ * Note that it only really makes sense to treat all
+ * return values as "available": indirect buffers mean that
+ * we can put an entire sg[] array inside a single queue entry.
+ */
+unsigned int virtqueue_get_capacity(struct virtqueue *_vq)
+{
+ struct vring_virtqueue *vq = to_vvq(_vq);
+ return vq->num_free;
+}
+EXPORT_SYMBOL_GPL(virtqueue_get_capacity);
+
MODULE_LICENSE("GPL");
diff --git a/include/linux/virtio.h b/include/linux/virtio.h
index a1ba8bb..fab61e8 100644
--- a/include/linux/virtio.h
+++ b/include/linux/virtio.h
@@ -50,6 +50,8 @@ void *virtqueue_detach_unused_buf(struct virtqueue *vq);
unsigned int virtqueue_get_vring_size(struct virtqueue *vq);
+unsigned int virtqueue_get_capacity(struct virtqueue *vq);
+
/**
* virtio_device - representation of a device using virtio
* @index: unique position on the virtio bus
--
MST
^ permalink raw reply related
* [PATCH 0/3] virtio-net: inline header support
From: Michael S. Tsirkin @ 2012-09-28 9:26 UTC (permalink / raw)
To: Thomas Lendacky
Cc: kvm, netdev, linux-kernel, virtualization, avi, Sasha Levin
Thinking about Sasha's patches, we can reduce ring usage
for virtio net small packets dramatically if we put
virtio net header inline with the data.
This can be done for free in case guest net stack allocated
extra head room for the packet, and I don't see
why would this have any downsides.
Even though with my recent patches qemu
no longer requires header to be the first s/g element,
we need a new feature bit to detect this.
A trivial qemu patch will be sent separately.
We could get rid of an extra s/g for big packets too,
but since in practice everyone enables mergeable buffers,
I don't see much of a point.
Rusty, if you decide to pick this up I'll send a
(rather trivial) spec patch shortly afterwards, but holidays
are beginning here. Considering how simple
the guest patch is, I hope it can make it in 3.7?
Also note that patch 1 and 2 are IMO a good
idea without patch 3. If you decide to defer patch 3
pls consider 1/2 separately.
Before:
[root@virtlab203 qemu]# ssh robin ./netperf/bin/netperf -t TCP_RR -H
11.0.0.4
TCP REQUEST/RESPONSE TEST from 0.0.0.0 (0.0.0.0) port 0 AF_INET to
11.0.0.4 (11.0.0.4) port 0 AF_INET : demo
Local /Remote
Socket Size Request Resp. Elapsed Trans.
Send Recv Size Size Time Rate
bytes Bytes bytes bytes secs. per sec
16384 87380 1 1 10.00 2992.88
16384 87380
After:
[root@virtlab203 qemu]# ssh robin ./netperf/bin/netperf -t TCP_RR -H
11.0.0.4
TCP REQUEST/RESPONSE TEST from 0.0.0.0 (0.0.0.0) port 0 AF_INET to
11.0.0.4 (11.0.0.4) port 0 AF_INET : demo
Local /Remote
Socket Size Request Resp. Elapsed Trans.
Send Recv Size Size Time Rate
bytes Bytes bytes bytes secs. per sec
16384 87380 1 1 10.00 3195.57
16384 87380
Michael S. Tsirkin (3):
virtio: add API to query ring capacity
virtio-net: correct capacity math on ring full
virtio-net: put virtio net header inline with data
drivers/net/virtio_net.c | 57 +++++++++++++++++++++++++++++++-------------
drivers/virtio/virtio_ring.c | 19 +++++++++++++++
include/linux/virtio.h | 2 ++
include/linux/virtio_net.h | 5 +++-
4 files changed, 66 insertions(+), 17 deletions(-)
--
MST
^ permalink raw reply
* Re: [PATCH] virtio-blk: Disable callback in virtblk_done()
From: Michael S. Tsirkin @ 2012-09-28 8:32 UTC (permalink / raw)
To: Rusty Russell; +Cc: kvm, virtualization
In-Reply-To: <87ehlo497o.fsf@rustcorp.com.au>
On Thu, Sep 27, 2012 at 09:40:03AM +0930, Rusty Russell wrote:
> I forgot about the cool hack which MST put in to defer event updates
> using disable_cb/enable_cb.
I considered sticking some invalid value
in event index on disable but in my testing it did not seem to
give any gain, and knowing actual index of the other side
is better for debugging.
--
MST
^ permalink raw reply
* Re: [PATCH] virtio-blk: Disable callback in virtblk_done()
From: Asias He @ 2012-09-28 7:03 UTC (permalink / raw)
To: Rusty Russell; +Cc: virtualization, kvm, Michael S. Tsirkin
In-Reply-To: <87fw623chz.fsf@rustcorp.com.au>
On 09/28/2012 02:08 PM, Rusty Russell wrote:
> Asias He <asias@redhat.com> writes:
>>> I forgot about the cool hack which MST put in to defer event updates
>>> using disable_cb/enable_cb.
>>
>> Hmm, are you talking about virtqueue_enable_cb_delayed()?
>
> Just the fact that virtqueue_disable_cb() prevents updates of
> used_index, and then we do the update in virtqueue_enable_cb().
Okay.
--
Asias
^ permalink raw reply
* Re: [PATCH] virtio-blk: Disable callback in virtblk_done()
From: Rusty Russell @ 2012-09-28 6:08 UTC (permalink / raw)
To: Asias He; +Cc: virtualization, kvm, Michael S. Tsirkin
In-Reply-To: <5063F6D2.7030908@redhat.com>
Asias He <asias@redhat.com> writes:
>> I forgot about the cool hack which MST put in to defer event updates
>> using disable_cb/enable_cb.
>
> Hmm, are you talking about virtqueue_enable_cb_delayed()?
Just the fact that virtqueue_disable_cb() prevents updates of
used_index, and then we do the update in virtqueue_enable_cb().
Cheers,
Rusty.
^ permalink raw reply
* Re: [PATCH 0/2] virtio-mmio updates for 3.7
From: Pawel Moll @ 2012-09-27 16:33 UTC (permalink / raw)
To: Rusty Russell; +Cc: virtualization@lists.linux-foundation.org, Brian Foley
In-Reply-To: <87mx0d3apj.fsf@rustcorp.com.au>
On Wed, 2012-09-26 at 01:10 +0100, Rusty Russell wrote:
> > I was thinking about it, but as the problem manifests itself mainly on
> > architectures with large page size, the impact is limited.
> >
> > But if you think it's worth it, I will repost with Cc: stable.
>
> I agree with you. But if it's not CC' stable, it's not for 3.7 either,
> since at this late stage the rules are basically synonymous.
Ok, let's postpone it till 3.8 for now, than. If we find out that this
is a real problem I'll re-post it with Cc: stable around 3.7-rc2, 3.
Cheers!
Pawel
^ permalink raw reply
* Re: [PATCH] virtio-blk: Disable callback in virtblk_done()
From: Paolo Bonzini @ 2012-09-27 10:01 UTC (permalink / raw)
To: Rusty Russell; +Cc: Michael S. Tsirkin, kvm, virtualization
In-Reply-To: <87ehlo497o.fsf@rustcorp.com.au>
Il 27/09/2012 02:10, Rusty Russell ha scritto:
>>> >> + do {
>>> >> + virtqueue_disable_cb(vq);
>>> >> + while ((vbr = virtqueue_get_buf(vblk->vq, &len)) != NULL) {
>>> >> + if (vbr->bio) {
>>> >> + virtblk_bio_done(vbr);
>>> >> + bio_done = true;
>>> >> + } else {
>>> >> + virtblk_request_done(vbr);
>>> >> + req_done = true;
>>> >> + }
>>> >> }
>>> >> - }
>>> >> + } while (!virtqueue_enable_cb(vq));
>>> >> /* In case queue is stopped waiting for more buffers. */
>>> >> if (req_done)
>>> >> blk_start_queue(vblk->disk->queue);
> Fascinating. Please just confirm that VIRTIO_RING_F_EVENT_IDX is
> enabled?
Yeah, it's a nice and cheap trick. Stefan, I see that you had this in
virtio-scsi since even before I picked it up. Do you remember how you
came up with it?
Paolo
^ permalink raw reply
* Re: [PATCH] virtio-blk: Disable callback in virtblk_done()
From: Asias He @ 2012-09-27 6:48 UTC (permalink / raw)
To: Rusty Russell; +Cc: virtualization, kvm, Michael S. Tsirkin
In-Reply-To: <87ehlo497o.fsf@rustcorp.com.au>
On 09/27/2012 08:10 AM, Rusty Russell wrote:
> Asias He <asias@redhat.com> writes:
>
>> On 09/25/2012 10:36 AM, Asias He wrote:
>>> This reduces unnecessary interrupts that host could send to guest while
>>> guest is in the progress of irq handling.
>>>
>>> If one vcpu is handling the irq, while another interrupt comes, in
>>> handle_edge_irq(), the guest will mask the interrupt via mask_msi_irq()
>>> which is a very heavy operation that goes all the way down to host.
>>>
>>> Signed-off-by: Asias He <asias@redhat.com>
>>> ---
>>
>> Here are some performance numbers on qemu:
>
> I assume this is with qemu using kvm, not qemu in soft emulation? :)
Of course.
>
>> Before:
>> -------------------------------------
>> seq-read : io=0 B, bw=269730KB/s, iops=67432 , runt= 62200msec
>> seq-write : io=0 B, bw=339716KB/s, iops=84929 , runt= 49386msec
>> rand-read : io=0 B, bw=270435KB/s, iops=67608 , runt= 62038msec
>> rand-write: io=0 B, bw=354436KB/s, iops=88608 , runt= 47335msec
>> clat (usec): min=101 , max=138052 , avg=14822.09, stdev=11771.01
>> clat (usec): min=96 , max=81543 , avg=11798.94, stdev=7735.60
>> clat (usec): min=128 , max=140043 , avg=14835.85, stdev=11765.33
>> clat (usec): min=109 , max=147207 , avg=11337.09, stdev=5990.35
>> cpu : usr=15.93%, sys=60.37%, ctx=7764972, majf=0, minf=54
>> cpu : usr=32.73%, sys=120.49%, ctx=7372945, majf=0, minf=1
>> cpu : usr=18.84%, sys=58.18%, ctx=7775420, majf=0, minf=1
>> cpu : usr=24.20%, sys=59.85%, ctx=8307886, majf=0, minf=0
>> vdb: ios=8389107/8368136, merge=0/0, ticks=19457874/14616506,
>> in_queue=34206098, util=99.68%
>> 43: interrupt in total: 887320
>> fio --exec_prerun="echo 3 > /proc/sys/vm/drop_caches" --group_reporting
>> --ioscheduler=noop --thread --bs=4k --size=512MB --direct=1 --numjobs=16
>> --ioengine=libaio --iodepth=64 --loops=3 --ramp_time=0
>> --filename=/dev/vdb --name=seq-read --stonewall --rw=read
>> --name=seq-write --stonewall --rw=write --name=rnd-read --stonewall
>> --rw=randread --name=rnd-write --stonewall --rw=randwrite
>>
>> After:
>> -------------------------------------
>> seq-read : io=0 B, bw=309503KB/s, iops=77375 , runt= 54207msec
>> seq-write : io=0 B, bw=448205KB/s, iops=112051 , runt= 37432msec
>> rand-read : io=0 B, bw=311254KB/s, iops=77813 , runt= 53902msec
>> rand-write: io=0 B, bw=377152KB/s, iops=94287 , runt= 44484msec
>> clat (usec): min=81 , max=90588 , avg=12946.06, stdev=9085.94
>> clat (usec): min=57 , max=72264 , avg=8967.97, stdev=5951.04
>> clat (usec): min=29 , max=101046 , avg=12889.95, stdev=9067.91
>> clat (usec): min=52 , max=106152 , avg=10660.56, stdev=4778.19
>> cpu : usr=15.05%, sys=57.92%, ctx=7710941, majf=0, minf=54
>> cpu : usr=26.78%, sys=101.40%, ctx=7387891, majf=0, minf=2
>> cpu : usr=19.03%, sys=58.17%, ctx=7681976, majf=0, minf=8
>> cpu : usr=24.65%, sys=58.34%, ctx=8442632, majf=0, minf=4
>> vdb: ios=8389086/8361888, merge=0/0, ticks=17243780/12742010,
>> in_queue=30078377, util=99.59%
>> 43: interrupt in total: 1259639
>> fio --exec_prerun="echo 3 > /proc/sys/vm/drop_caches" --group_reporting
>> --ioscheduler=noop --thread --bs=4k --size=512MB --direct=1 --numjobs=16
>> --ioengine=libaio --iodepth=64 --loops=3 --ramp_time=0
>> --filename=/dev/vdb --name=seq-read --stonewall --rw=read
>> --name=seq-write --stonewall --rw=write --name=rnd-read --stonewall
>> --rw=randread --name=rnd-write --stonewall --rw=randwrite
>>
>>> drivers/block/virtio_blk.c | 19 +++++++++++--------
>>> 1 file changed, 11 insertions(+), 8 deletions(-)
>>>
>>> diff --git a/drivers/block/virtio_blk.c b/drivers/block/virtio_blk.c
>>> index 53b81d5..0bdde8f 100644
>>> --- a/drivers/block/virtio_blk.c
>>> +++ b/drivers/block/virtio_blk.c
>>> @@ -274,15 +274,18 @@ static void virtblk_done(struct virtqueue *vq)
>>> unsigned int len;
>>>
>>> spin_lock_irqsave(vblk->disk->queue->queue_lock, flags);
>>> - while ((vbr = virtqueue_get_buf(vblk->vq, &len)) != NULL) {
>>> - if (vbr->bio) {
>>> - virtblk_bio_done(vbr);
>>> - bio_done = true;
>>> - } else {
>>> - virtblk_request_done(vbr);
>>> - req_done = true;
>>> + do {
>>> + virtqueue_disable_cb(vq);
>>> + while ((vbr = virtqueue_get_buf(vblk->vq, &len)) != NULL) {
>>> + if (vbr->bio) {
>>> + virtblk_bio_done(vbr);
>>> + bio_done = true;
>>> + } else {
>>> + virtblk_request_done(vbr);
>>> + req_done = true;
>>> + }
>>> }
>>> - }
>>> + } while (!virtqueue_enable_cb(vq));
>>> /* In case queue is stopped waiting for more buffers. */
>>> if (req_done)
>>> blk_start_queue(vblk->disk->queue);
>
> Fascinating. Please just confirm that VIRTIO_RING_F_EVENT_IDX is
> enabled?
Sure. It is enabled ;-)
>
> I forgot about the cool hack which MST put in to defer event updates
> using disable_cb/enable_cb.
Hmm, are you talking about virtqueue_enable_cb_delayed()?
>
> Applied!
> Rusty.
>
--
Asias
^ permalink raw reply
* Proposal for virtio standardization.
From: Rusty Russell @ 2012-09-27 0:29 UTC (permalink / raw)
To: Anthony Liguori, Adam Litke, Amit Shah, Avi Kivity,
Avishay Traeger, Jason Wang, Michael S. Tsirkin, Ohad Ben-Cohen,
Paolo Bonzini, Pawel Moll, Sasha Levin, Cornelia Huck
Cc: qemu-devel, LKML, kvm, virtualization
Hi all,
I've had several requests for a more formal approach to the
virtio draft spec, and (after some soul-searching) I'd like to try that.
The proposal is to use OASIS as the standards body, as it's
fairly light-weight as these things go. For me this means paperwork and
setting up a Working Group and getting the right people involved as
Voting members starting with the current contributors; for most of you
it just means a new mailing list, though I'll be cross-posting any
drafts and major changes here anyway.
I believe that a documented standard (aka virtio 1.0) will
increase visibility and adoption in areas outside our normal linux/kvm
universe. There's been some of that already, but this is the clearest
path to accelerate it. Not the easiest path, but I believe that a solid
I/O standard is a Good Thing for everyone.
Yet I also want to decouple new and experimental development
from the standards effort; running code comes first. New feature bits
and new device numbers should be reservable without requiring a full
spec change.
So the essence of my proposal is:
1) I start a Working Group within OASIS where we can aim for virtio spec
1.0.
2) The current spec is textually reordered so the core is clearly
bus-independent, with PCI, mmio, etc appendices.
3) Various clarifications, formalizations and cleanups to the spec text,
and possibly elimination of old deprecated features.
4) The only significant change to the spec is that we use PCI
capabilities, so we can have infinite feature bits.
(see
http://lists.linuxfoundation.org/pipermail/virtualization/2011-December/019198.html)
5) Changes to the ring layout and other such things are deferred to a
future virtio version; whether this is done within OASIS or
externally depends on how well this works for the 1.0 release.
Thoughts?
Rusty.
^ permalink raw reply
* Re: [PATCH] virtio-blk: Disable callback in virtblk_done()
From: Rusty Russell @ 2012-09-27 0:10 UTC (permalink / raw)
To: Asias He; +Cc: virtualization, kvm, Michael S. Tsirkin
In-Reply-To: <5061BAED.9020205@redhat.com>
Asias He <asias@redhat.com> writes:
> On 09/25/2012 10:36 AM, Asias He wrote:
>> This reduces unnecessary interrupts that host could send to guest while
>> guest is in the progress of irq handling.
>>
>> If one vcpu is handling the irq, while another interrupt comes, in
>> handle_edge_irq(), the guest will mask the interrupt via mask_msi_irq()
>> which is a very heavy operation that goes all the way down to host.
>>
>> Signed-off-by: Asias He <asias@redhat.com>
>> ---
>
> Here are some performance numbers on qemu:
I assume this is with qemu using kvm, not qemu in soft emulation? :)
> Before:
> -------------------------------------
> seq-read : io=0 B, bw=269730KB/s, iops=67432 , runt= 62200msec
> seq-write : io=0 B, bw=339716KB/s, iops=84929 , runt= 49386msec
> rand-read : io=0 B, bw=270435KB/s, iops=67608 , runt= 62038msec
> rand-write: io=0 B, bw=354436KB/s, iops=88608 , runt= 47335msec
> clat (usec): min=101 , max=138052 , avg=14822.09, stdev=11771.01
> clat (usec): min=96 , max=81543 , avg=11798.94, stdev=7735.60
> clat (usec): min=128 , max=140043 , avg=14835.85, stdev=11765.33
> clat (usec): min=109 , max=147207 , avg=11337.09, stdev=5990.35
> cpu : usr=15.93%, sys=60.37%, ctx=7764972, majf=0, minf=54
> cpu : usr=32.73%, sys=120.49%, ctx=7372945, majf=0, minf=1
> cpu : usr=18.84%, sys=58.18%, ctx=7775420, majf=0, minf=1
> cpu : usr=24.20%, sys=59.85%, ctx=8307886, majf=0, minf=0
> vdb: ios=8389107/8368136, merge=0/0, ticks=19457874/14616506,
> in_queue=34206098, util=99.68%
> 43: interrupt in total: 887320
> fio --exec_prerun="echo 3 > /proc/sys/vm/drop_caches" --group_reporting
> --ioscheduler=noop --thread --bs=4k --size=512MB --direct=1 --numjobs=16
> --ioengine=libaio --iodepth=64 --loops=3 --ramp_time=0
> --filename=/dev/vdb --name=seq-read --stonewall --rw=read
> --name=seq-write --stonewall --rw=write --name=rnd-read --stonewall
> --rw=randread --name=rnd-write --stonewall --rw=randwrite
>
> After:
> -------------------------------------
> seq-read : io=0 B, bw=309503KB/s, iops=77375 , runt= 54207msec
> seq-write : io=0 B, bw=448205KB/s, iops=112051 , runt= 37432msec
> rand-read : io=0 B, bw=311254KB/s, iops=77813 , runt= 53902msec
> rand-write: io=0 B, bw=377152KB/s, iops=94287 , runt= 44484msec
> clat (usec): min=81 , max=90588 , avg=12946.06, stdev=9085.94
> clat (usec): min=57 , max=72264 , avg=8967.97, stdev=5951.04
> clat (usec): min=29 , max=101046 , avg=12889.95, stdev=9067.91
> clat (usec): min=52 , max=106152 , avg=10660.56, stdev=4778.19
> cpu : usr=15.05%, sys=57.92%, ctx=7710941, majf=0, minf=54
> cpu : usr=26.78%, sys=101.40%, ctx=7387891, majf=0, minf=2
> cpu : usr=19.03%, sys=58.17%, ctx=7681976, majf=0, minf=8
> cpu : usr=24.65%, sys=58.34%, ctx=8442632, majf=0, minf=4
> vdb: ios=8389086/8361888, merge=0/0, ticks=17243780/12742010,
> in_queue=30078377, util=99.59%
> 43: interrupt in total: 1259639
> fio --exec_prerun="echo 3 > /proc/sys/vm/drop_caches" --group_reporting
> --ioscheduler=noop --thread --bs=4k --size=512MB --direct=1 --numjobs=16
> --ioengine=libaio --iodepth=64 --loops=3 --ramp_time=0
> --filename=/dev/vdb --name=seq-read --stonewall --rw=read
> --name=seq-write --stonewall --rw=write --name=rnd-read --stonewall
> --rw=randread --name=rnd-write --stonewall --rw=randwrite
>
>> drivers/block/virtio_blk.c | 19 +++++++++++--------
>> 1 file changed, 11 insertions(+), 8 deletions(-)
>>
>> diff --git a/drivers/block/virtio_blk.c b/drivers/block/virtio_blk.c
>> index 53b81d5..0bdde8f 100644
>> --- a/drivers/block/virtio_blk.c
>> +++ b/drivers/block/virtio_blk.c
>> @@ -274,15 +274,18 @@ static void virtblk_done(struct virtqueue *vq)
>> unsigned int len;
>>
>> spin_lock_irqsave(vblk->disk->queue->queue_lock, flags);
>> - while ((vbr = virtqueue_get_buf(vblk->vq, &len)) != NULL) {
>> - if (vbr->bio) {
>> - virtblk_bio_done(vbr);
>> - bio_done = true;
>> - } else {
>> - virtblk_request_done(vbr);
>> - req_done = true;
>> + do {
>> + virtqueue_disable_cb(vq);
>> + while ((vbr = virtqueue_get_buf(vblk->vq, &len)) != NULL) {
>> + if (vbr->bio) {
>> + virtblk_bio_done(vbr);
>> + bio_done = true;
>> + } else {
>> + virtblk_request_done(vbr);
>> + req_done = true;
>> + }
>> }
>> - }
>> + } while (!virtqueue_enable_cb(vq));
>> /* In case queue is stopped waiting for more buffers. */
>> if (req_done)
>> blk_start_queue(vblk->disk->queue);
Fascinating. Please just confirm that VIRTIO_RING_F_EVENT_IDX is
enabled?
I forgot about the cool hack which MST put in to defer event updates
using disable_cb/enable_cb.
Applied!
Rusty.
^ permalink raw reply
* Re: [PATCHv5 2/3] virtio_console: Add support for remoteproc serial
From: Rusty Russell @ 2012-09-26 23:52 UTC (permalink / raw)
To: Amit Shah
Cc: Arnd Bergmann, Michael S. Tsirkin, sjurbren, linux-kernel,
virtualization, Linus Walleij, Sjur Brændeland
In-Reply-To: <1348580837-10919-3-git-send-email-sjur.brandeland@stericsson.com>
sjur.brandeland@stericsson.com writes:
> From: Sjur Brændeland <sjur.brandeland@stericsson.com>
>
> Add a simple serial connection driver called
> VIRTIO_ID_RPROC_SERIAL (11) for communicating with a
> remote processor in an asymmetric multi-processing
> configuration.
>
...
> static struct port_buffer *alloc_buf(struct virtqueue *vq, size_t buf_size,
> int nrbufs)
> {
> struct port_buffer *buf;
> size_t alloc_size;
>
> + if (is_rproc_serial(vq->vdev) && !irqs_disabled())
> + reclaim_dma_bufs();
Hmm, you need a gfp_t arg into alloc_buf; your last patch simply changed
them all to GFP_ATOMIC, which makes the console far less memory
friendly.
You check !irqs_disabled() in a couple of places; I think the caller
needs to indicate (possibly by checking for gfp == GFP_KERNEL) whether
it's safe to call reclaim_dma_bufs().
> @@ -838,6 +927,10 @@ static ssize_t port_fops_splice_write(struct pipe_inode_info *pipe,
> .u.data = &sgl,
> };
>
> + /* rproc_serial does not support splice */
> + if (is_rproc_serial(port->out_vq->vdev))
> + return -EINVAL;
Why not? ;)
Thanks,
Rusty.
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
^ permalink raw reply
* RE: [PATCH 1/3] virtio_console:Merge struct buffer_token into struct port_buffer
From: Rusty Russell @ 2012-09-26 23:42 UTC (permalink / raw)
To: Sjur BRENDELAND, Masami Hiramatsu
Cc: sjurbren@gmail.com, Michael S. Tsirkin, Linus Walleij,
linux-kernel@vger.kernel.org,
virtualization@lists.linux-foundation.org,
yrl.pp-manager.tt@hitachi.com, Amit Shah
In-Reply-To: <81C3A93C17462B4BBD7E272753C1057923BD8365F7@EXDCVYMBSTM005.EQ1STM.local>
Sjur BRENDELAND <sjur.brandeland@stericsson.com> writes:
>> This allocates one redundant sg entry when nrbuf > 0,
>> but I think it is OK. (just a comment)
>
> I did this on purpose for the sake of simplicity, but I can
> change this to something like:
> alloc_size = sizeof(*buf) + sizeof(buf->sg) * max(nrbufs - 1, 1);
That's why we use [0] in the definition (a GCC extension).
Cheers,
Rusty.
^ permalink raw reply
* Re: [PATCH 1/3] virtio_console:Merge struct buffer_token into struct port_buffer
From: Masami Hiramatsu @ 2012-09-26 9:40 UTC (permalink / raw)
To: Sjur BRENDELAND
Cc: sjurbren@gmail.com, Michael S. Tsirkin, Linus Walleij,
linux-kernel@vger.kernel.org,
virtualization@lists.linux-foundation.org,
yrl.pp-manager.tt@hitachi.com, Amit Shah
In-Reply-To: <81C3A93C17462B4BBD7E272753C1057923BD8365F7@EXDCVYMBSTM005.EQ1STM.local>
(2012/09/26 16:48), Sjur BRENDELAND wrote:
>>> -static struct port_buffer *alloc_buf(size_t buf_size)
>>> +static struct port_buffer *alloc_buf(struct virtqueue *vq, size_t buf_size,
>>> + int nrbufs)
>>> {
>>> struct port_buffer *buf;
>>> + size_t alloc_size;
>>>
>>> - buf = kmalloc(sizeof(*buf), GFP_KERNEL);
>>> + /* Allocate buffer and the scatter list */
>>> + alloc_size = sizeof(*buf) + sizeof(struct scatterlist) * nrbufs;
>>
>> This allocates one redundant sg entry when nrbuf > 0,
>> but I think it is OK. (just a comment)
>
> I did this on purpose for the sake of simplicity, but I can
> change this to something like:
> alloc_size = sizeof(*buf) + sizeof(buf->sg) * max(nrbufs - 1, 1);
You wouldn't need to change that. I think current code is enough simple
and reasonable. :)
Thanks!
--
Masami HIRAMATSU
Software Platform Research Dept. Linux Technology Center
Hitachi, Ltd., Yokohama Research Laboratory
E-mail: masami.hiramatsu.pt@hitachi.com
^ permalink raw reply
* RE: [PATCH 1/3] virtio_console:Merge struct buffer_token into struct port_buffer
From: Sjur BRENDELAND @ 2012-09-26 7:48 UTC (permalink / raw)
To: Masami Hiramatsu
Cc: sjurbren@gmail.com, Michael S. Tsirkin, Linus Walleij,
linux-kernel@vger.kernel.org,
virtualization@lists.linux-foundation.org,
yrl.pp-manager.tt@hitachi.com, Amit Shah
In-Reply-To: <50626C11.9040708@hitachi.com>
> > This merge reduces code size by unifying the approach for
> > sending scatter-lists and regular buffers. Any type of
> > write operation (splice, write, put_chars) will now allocate
> > a port_buffer and send_buf() and free_buf() can always be used.
>
> Thanks!
> This looks much nicer and simpler. I just have some comments below.
OK, good to hear that you agree to this kind of change. I'll do a respin
of this patch fixing the issues you have pointed out.
> > static void free_buf(struct port_buffer *buf)
> > {
> > + int i;
> > +
> > kfree(buf->buf);
>
> this should be done only when !buf->sgpages, or (see below)
Agree, I'll put this statement in an else branch to the if-statement below.
>
> > +
> > + if (buf->sgpages)
> > + for (i = 0; i < buf->sgpages; i++) {
> > + struct page *page = sg_page(&buf->sg[i]);
> > + if (!page)
> > + break;
> > + put_page(page);
> > + }
> > +
> > kfree(buf);
> > }
> >
> > -static struct port_buffer *alloc_buf(size_t buf_size)
> > +static struct port_buffer *alloc_buf(struct virtqueue *vq, size_t buf_size,
> > + int nrbufs)
> > {
> > struct port_buffer *buf;
> > + size_t alloc_size;
> >
> > - buf = kmalloc(sizeof(*buf), GFP_KERNEL);
> > + /* Allocate buffer and the scatter list */
> > + alloc_size = sizeof(*buf) + sizeof(struct scatterlist) * nrbufs;
>
> This allocates one redundant sg entry when nrbuf > 0,
> but I think it is OK. (just a comment)
I did this on purpose for the sake of simplicity, but I can
change this to something like:
alloc_size = sizeof(*buf) + sizeof(buf->sg) * max(nrbufs - 1, 1);
> > + buf = kmalloc(alloc_size, GFP_ATOMIC);
>
> This should be kzalloc(), or buf->buf and others are not initialized,
> which will cause unexpected kfree bug at kfree(buf->buf) in free_buf.
Agree, kzalloc() is better in this case.
> > if (!buf)
> > goto fail;
> > - buf->buf = kzalloc(buf_size, GFP_KERNEL);
> > +
> > + buf->sgpages = nrbufs;
> > + if (nrbufs > 0)
> > + return buf;
> > +
> > + buf->buf = kmalloc(buf_size, GFP_ATOMIC);
>
> You can also use kzalloc here as previous code does.
> But if the reason why using kzalloc comes from the security,
> I think kmalloc is enough here, since the host can access
> all the guest pages anyway.
With this new patch alloc_buf() is used both for both RX and TX.
The out_vq did previously use malloc(). But I have preserved
the legacy behavior for the in_vq by calling memset() in function
fill_queue().
Thanks,
Sjur
^ permalink raw reply
* Re: [PATCH 1/3] virtio_console:Merge struct buffer_token into struct port_buffer
From: Masami Hiramatsu @ 2012-09-26 2:44 UTC (permalink / raw)
To: sjur.brandeland
Cc: Michael S. Tsirkin, sjurbren, linux-kernel, virtualization,
yrl.pp-manager.tt, Amit Shah, Linus Walleij
In-Reply-To: <1348580837-10919-2-git-send-email-sjur.brandeland@stericsson.com>
(2012/09/25 22:47), sjur.brandeland@stericsson.com wrote:
> From: Sjur Brændeland <sjur.brandeland@stericsson.com>
>
> This merge reduces code size by unifying the approach for
> sending scatter-lists and regular buffers. Any type of
> write operation (splice, write, put_chars) will now allocate
> a port_buffer and send_buf() and free_buf() can always be used.
Thanks!
This looks much nicer and simpler. I just have some comments below.
> Signed-off-by: Sjur Brændeland <sjur.brandeland@stericsson.com>
> cc: Rusty Russell <rusty@rustcorp.com.au>
> cc: Michael S. Tsirkin <mst@redhat.com>
> cc: Amit Shah <amit.shah@redhat.com>
> cc: Linus Walleij <linus.walleij@linaro.org>
> cc: Masami Hiramatsu <masami.hiramatsu.pt@hitachi.com>
> ---
> drivers/char/virtio_console.c | 141 ++++++++++++++++++-----------------------
> 1 files changed, 62 insertions(+), 79 deletions(-)
>
> diff --git a/drivers/char/virtio_console.c b/drivers/char/virtio_console.c
> index 8ab9c3d..f4f7b04 100644
> --- a/drivers/char/virtio_console.c
> +++ b/drivers/char/virtio_console.c
> @@ -111,6 +111,11 @@ struct port_buffer {
> size_t len;
> /* offset in the buf from which to consume data */
> size_t offset;
> +
> + /* If sgpages == 0 then buf is used, else sg is used */
> + unsigned int sgpages;
> +
> + struct scatterlist sg[1];
> };
>
> /*
> @@ -338,23 +343,46 @@ static inline bool use_multiport(struct ports_device *portdev)
>
> static void free_buf(struct port_buffer *buf)
> {
> + int i;
> +
> kfree(buf->buf);
this should be done only when !buf->sgpages, or (see below)
> +
> + if (buf->sgpages)
> + for (i = 0; i < buf->sgpages; i++) {
> + struct page *page = sg_page(&buf->sg[i]);
> + if (!page)
> + break;
> + put_page(page);
> + }
> +
> kfree(buf);
> }
>
> -static struct port_buffer *alloc_buf(size_t buf_size)
> +static struct port_buffer *alloc_buf(struct virtqueue *vq, size_t buf_size,
> + int nrbufs)
> {
> struct port_buffer *buf;
> + size_t alloc_size;
>
> - buf = kmalloc(sizeof(*buf), GFP_KERNEL);
> + /* Allocate buffer and the scatter list */
> + alloc_size = sizeof(*buf) + sizeof(struct scatterlist) * nrbufs;
This allocates one redundant sg entry when nrbuf > 0,
but I think it is OK. (just a comment)
> + buf = kmalloc(alloc_size, GFP_ATOMIC);
This should be kzalloc(), or buf->buf and others are not initialized,
which will cause unexpected kfree bug at kfree(buf->buf) in free_buf.
> if (!buf)
> goto fail;
> - buf->buf = kzalloc(buf_size, GFP_KERNEL);
> +
> + buf->sgpages = nrbufs;
> + if (nrbufs > 0)
> + return buf;
> +
> + buf->buf = kmalloc(buf_size, GFP_ATOMIC);
You can also use kzalloc here as previous code does.
But if the reason why using kzalloc comes from the security,
I think kmalloc is enough here, since the host can access
all the guest pages anyway.
> if (!buf->buf)
> goto free_buf;
> buf->len = 0;
> buf->offset = 0;
> buf->size = buf_size;
> +
> + /* Prepare scatter buffer for sending */
> + sg_init_one(buf->sg, buf->buf, buf_size);
> return buf;
>
> free_buf:
Thank you,
--
Masami HIRAMATSU
Software Platform Research Dept. Linux Technology Center
Hitachi, Ltd., Yokohama Research Laboratory
E-mail: masami.hiramatsu.pt@hitachi.com
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
^ permalink raw reply
* Re: [PATCH 0/2] virtio-mmio updates for 3.7
From: Rusty Russell @ 2012-09-26 0:10 UTC (permalink / raw)
To: Pawel Moll; +Cc: virtualization@lists.linux-foundation.org, Brian Foley
In-Reply-To: <1348571527.27071.37.camel@hornet>
Pawel Moll <pawel.moll@arm.com> writes:
> On Tue, 2012-09-25 at 01:11 +0100, Rusty Russell wrote:
>> > Would you be so kind and consider getting those two small fixes into
>> > 3.7 merge? All credits go to Brian, all shame on me :-)
>>
>> Should these also be cc'd to stable@kernel.org?
>
> I was thinking about it, but as the problem manifests itself mainly on
> architectures with large page size, the impact is limited.
>
> But if you think it's worth it, I will repost with Cc: stable.
I agree with you. But if it's not CC' stable, it's not for 3.7 either,
since at this late stage the rules are basically synonymous.
Cheers,
Rusty.
^ permalink raw reply
* Re: [PATCH v10 3/5] virtio_balloon: introduce migration primitives to balloon pages
From: Rafael Aquini @ 2012-09-25 18:07 UTC (permalink / raw)
To: Michael S. Tsirkin
Cc: Rik van Riel, Konrad Rzeszutek Wilk, linux-kernel, virtualization,
linux-mm, Peter Zijlstra, Andi Kleen, Minchan Kim, Andrew Morton,
Paul E. McKenney
In-Reply-To: <20120925004024.GA22665@redhat.com>
On Tue, Sep 25, 2012 at 02:40:24AM +0200, Michael S. Tsirkin wrote:
> > @@ -139,9 +158,15 @@ static void fill_balloon(struct virtio_balloon *vb, size_t num)
> > break;
> > }
> > set_page_pfns(vb->pfns + vb->num_pfns, page);
> > - vb->num_pages += VIRTIO_BALLOON_PAGES_PER_PAGE;
> > totalram_pages--;
> > +
> > + BUG_ON(!trylock_page(page));
>
> So here page lock is nested within balloon_lock.
>
This page is coming from Buddy free-lists and as such, fill_balloon does not
race against page migration (which takes pages from an already isolated page list).
We need to grab page lock here, to prevent page isolation from happening while
this page is yet under "balloon insertion" step (mapping assign). Also, failing
to grab the page lock here (remember this page is coming from Buddy) means a
BUG.
I'll make sure to comment that on the code. Thanks for the nit.
> > +int virtballoon_migratepage(struct address_space *mapping,
> > + struct page *newpage, struct page *page, enum migrate_mode mode)
> > +{
> > + struct virtio_balloon *vb = __page_balloon_device(page);
> > +
> > + BUG_ON(!vb);
> > +
> > + mutex_lock(&vb->balloon_lock);
>
>
> While here balloon_lock is taken and according to documentation
> this is called under page lock.
>
> So nesting is reversed which is normally a problem.
> Unfortunately lockep does not seem to work for page lock
> otherwise it would detect this.
> If this reversed nesting is not a problem, please add
> comments in code documenting that this is intentional
> and how it works.
>
The scheme works because we do not invert the page locking, actually. If we
stumble accross a locked page while holding mutex(balloon_lock) we just give up
that page for that round. The comment @ __leak_balloon explains that already:
----
+ /*
+ * Grab the page lock to avoid racing against threads isolating
+ * pages from, or migrating pages back to vb->pages list.
+ * (both tasks are done under page lock protection)
+ *
+ * Failing to grab the page lock here means this page is being
+ * isolated already, or its migration has not finished yet.
+ *
+ * We simply cannot afford to keep waiting on page lock here,
+ * otherwise we might cause a lock inversion and remain dead-
+ * locked with threads isolating/migrating pages.
+ * So, we give up this round if we fail to grab the page lock.
+ */
+ if (!trylock_page(page))
+ break;
----
The otherwise is true as well. If the thread trying to isolate pages stumbles
across a balloon page already locked at leak_balloon, it gives up the isolation
step and mutex(&balloon_lock) is never attempted down the code path. Check this
isolate_balloon_page() snippet out:
----
+ /*
+ * As balloon pages are not isolated from LRU lists, concurrent
+ * compaction threads can race against page migration functions
+ * move_to_new_page() & __unmap_and_move().
+ * In order to avoid having an already isolated balloon page
+ * being (wrongly) re-isolated while it is under migration,
+ * lets be sure we have the page lock before proceeding with
+ * the balloon page isolation steps.
+ */
+ if (likely(trylock_page(page))) {
----
I'll rework the comment to indicate the leak_balloon() case as well.
^ permalink raw reply
* Re: [PATCH] virtio-blk: Disable callback in virtblk_done()
From: Asias He @ 2012-09-25 14:08 UTC (permalink / raw)
To: Rusty Russell; +Cc: virtualization, kvm, Michael S. Tsirkin
In-Reply-To: <1348540577-7474-1-git-send-email-asias@redhat.com>
On 09/25/2012 10:36 AM, Asias He wrote:
> This reduces unnecessary interrupts that host could send to guest while
> guest is in the progress of irq handling.
>
> If one vcpu is handling the irq, while another interrupt comes, in
> handle_edge_irq(), the guest will mask the interrupt via mask_msi_irq()
> which is a very heavy operation that goes all the way down to host.
>
> Signed-off-by: Asias He <asias@redhat.com>
> ---
Here are some performance numbers on qemu:
Before:
-------------------------------------
seq-read : io=0 B, bw=269730KB/s, iops=67432 , runt= 62200msec
seq-write : io=0 B, bw=339716KB/s, iops=84929 , runt= 49386msec
rand-read : io=0 B, bw=270435KB/s, iops=67608 , runt= 62038msec
rand-write: io=0 B, bw=354436KB/s, iops=88608 , runt= 47335msec
clat (usec): min=101 , max=138052 , avg=14822.09, stdev=11771.01
clat (usec): min=96 , max=81543 , avg=11798.94, stdev=7735.60
clat (usec): min=128 , max=140043 , avg=14835.85, stdev=11765.33
clat (usec): min=109 , max=147207 , avg=11337.09, stdev=5990.35
cpu : usr=15.93%, sys=60.37%, ctx=7764972, majf=0, minf=54
cpu : usr=32.73%, sys=120.49%, ctx=7372945, majf=0, minf=1
cpu : usr=18.84%, sys=58.18%, ctx=7775420, majf=0, minf=1
cpu : usr=24.20%, sys=59.85%, ctx=8307886, majf=0, minf=0
vdb: ios=8389107/8368136, merge=0/0, ticks=19457874/14616506,
in_queue=34206098, util=99.68%
43: interrupt in total: 887320
fio --exec_prerun="echo 3 > /proc/sys/vm/drop_caches" --group_reporting
--ioscheduler=noop --thread --bs=4k --size=512MB --direct=1 --numjobs=16
--ioengine=libaio --iodepth=64 --loops=3 --ramp_time=0
--filename=/dev/vdb --name=seq-read --stonewall --rw=read
--name=seq-write --stonewall --rw=write --name=rnd-read --stonewall
--rw=randread --name=rnd-write --stonewall --rw=randwrite
After:
-------------------------------------
seq-read : io=0 B, bw=309503KB/s, iops=77375 , runt= 54207msec
seq-write : io=0 B, bw=448205KB/s, iops=112051 , runt= 37432msec
rand-read : io=0 B, bw=311254KB/s, iops=77813 , runt= 53902msec
rand-write: io=0 B, bw=377152KB/s, iops=94287 , runt= 44484msec
clat (usec): min=81 , max=90588 , avg=12946.06, stdev=9085.94
clat (usec): min=57 , max=72264 , avg=8967.97, stdev=5951.04
clat (usec): min=29 , max=101046 , avg=12889.95, stdev=9067.91
clat (usec): min=52 , max=106152 , avg=10660.56, stdev=4778.19
cpu : usr=15.05%, sys=57.92%, ctx=7710941, majf=0, minf=54
cpu : usr=26.78%, sys=101.40%, ctx=7387891, majf=0, minf=2
cpu : usr=19.03%, sys=58.17%, ctx=7681976, majf=0, minf=8
cpu : usr=24.65%, sys=58.34%, ctx=8442632, majf=0, minf=4
vdb: ios=8389086/8361888, merge=0/0, ticks=17243780/12742010,
in_queue=30078377, util=99.59%
43: interrupt in total: 1259639
fio --exec_prerun="echo 3 > /proc/sys/vm/drop_caches" --group_reporting
--ioscheduler=noop --thread --bs=4k --size=512MB --direct=1 --numjobs=16
--ioengine=libaio --iodepth=64 --loops=3 --ramp_time=0
--filename=/dev/vdb --name=seq-read --stonewall --rw=read
--name=seq-write --stonewall --rw=write --name=rnd-read --stonewall
--rw=randread --name=rnd-write --stonewall --rw=randwrite
> drivers/block/virtio_blk.c | 19 +++++++++++--------
> 1 file changed, 11 insertions(+), 8 deletions(-)
>
> diff --git a/drivers/block/virtio_blk.c b/drivers/block/virtio_blk.c
> index 53b81d5..0bdde8f 100644
> --- a/drivers/block/virtio_blk.c
> +++ b/drivers/block/virtio_blk.c
> @@ -274,15 +274,18 @@ static void virtblk_done(struct virtqueue *vq)
> unsigned int len;
>
> spin_lock_irqsave(vblk->disk->queue->queue_lock, flags);
> - while ((vbr = virtqueue_get_buf(vblk->vq, &len)) != NULL) {
> - if (vbr->bio) {
> - virtblk_bio_done(vbr);
> - bio_done = true;
> - } else {
> - virtblk_request_done(vbr);
> - req_done = true;
> + do {
> + virtqueue_disable_cb(vq);
> + while ((vbr = virtqueue_get_buf(vblk->vq, &len)) != NULL) {
> + if (vbr->bio) {
> + virtblk_bio_done(vbr);
> + bio_done = true;
> + } else {
> + virtblk_request_done(vbr);
> + req_done = true;
> + }
> }
> - }
> + } while (!virtqueue_enable_cb(vq));
> /* In case queue is stopped waiting for more buffers. */
> if (req_done)
> blk_start_queue(vblk->disk->queue);
>
--
Asias
^ permalink raw reply
* Re: [PATCH v10 1/5] mm: introduce a common interface for balloon pages mobility
From: Rafael Aquini @ 2012-09-25 14:00 UTC (permalink / raw)
To: Michael S. Tsirkin
Cc: Rik van Riel, Konrad Rzeszutek Wilk, linux-kernel, virtualization,
linux-mm, Peter Zijlstra, Andi Kleen, Minchan Kim, Andrew Morton,
Paul E. McKenney
In-Reply-To: <20120925010549.GA22893@redhat.com>
On Tue, Sep 25, 2012 at 03:05:49AM +0200, Michael S. Tsirkin wrote:
> If these are all under page lock these barriers just confuse things,
> because they are almost never enough by themselves.
> So in that case it would be better to drop them and document
> usage as you are going to.
>
Would the following make more sense (with the proprer comments, as well) ?
---8<---
+static inline void balloon_page_set(struct page *page,
+ struct address_space *mapping,
+ struct list_head *head)
+{
+ list_add(&page->lru, head);
+ smp_wmb();
+ page->mapping = mapping;
+}
+
+static inline void balloon_page_del(struct page *page)
+{
+ page->mapping = NULL;
+ smp_wmb();
+ list_del(&page->lru);
+}
+
+static inline bool __is_movable_balloon_page(struct page *page)
+{
+ struct address_space *mapping = ACCESS_ONCE(page->mapping);
+ smp_read_barrier_depends();
+ return mapping_balloon(mapping);
+}
+
---8<---
There's still a case where we have to test page->mapping->flags and we cannot
afford to wait for, or grab, the page lock @ isolate_migratepages_range().
The barriers won't avoid leak_ballon() racing against isolate_migratepages_range(),
but they surely will make tests for page->mapping more consistent. And for those
cases where leak_balloon() races against
isolate_migratepages_range->isolate_balloon_page(), we solve the conflict of
interest through page refcounting and page lock. I'm preparing a more extensive
doc to include at Documentation/ to explain the interfaces and how we cope with
these mentioned races, as well.
^ permalink raw reply
* [PATCH 3/3] virtio_console: Don't initialize buffers to zero
From: sjur.brandeland @ 2012-09-25 13:47 UTC (permalink / raw)
To: Amit Shah; +Cc: sjurbren, Sjur Brændeland, linux-kernel, virtualization
In-Reply-To: <1348580837-10919-1-git-send-email-sjur.brandeland@stericsson.com>
From: Sjur Brændeland <sjur.brandeland@stericsson.com>
Skip initializing the receive buffers.
Signed-off-by: Sjur Brændeland <sjur.brandeland@stericsson.com>
---
drivers/char/virtio_console.c | 1 -
1 files changed, 0 insertions(+), 1 deletions(-)
diff --git a/drivers/char/virtio_console.c b/drivers/char/virtio_console.c
index faedd2c..e7d8787 100644
--- a/drivers/char/virtio_console.c
+++ b/drivers/char/virtio_console.c
@@ -1344,7 +1344,6 @@ static unsigned int fill_queue(struct virtqueue *vq, spinlock_t *lock)
if (!buf)
break;
- memset(buf->buf, 0, PAGE_SIZE);
spin_lock_irq(lock);
ret = add_inbuf(vq, buf);
if (ret < 0) {
--
1.7.5.4
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
^ permalink raw reply related
* [PATCHv5 2/3] virtio_console: Add support for remoteproc serial
From: sjur.brandeland @ 2012-09-25 13:47 UTC (permalink / raw)
To: Amit Shah
Cc: Arnd Bergmann, Michael S. Tsirkin, sjurbren, linux-kernel,
virtualization, Linus Walleij, Sjur Brændeland
In-Reply-To: <1348580837-10919-1-git-send-email-sjur.brandeland@stericsson.com>
From: Sjur Brændeland <sjur.brandeland@stericsson.com>
Add a simple serial connection driver called
VIRTIO_ID_RPROC_SERIAL (11) for communicating with a
remote processor in an asymmetric multi-processing
configuration.
This implementation reuses the existing virtio_console
implementation, and adds support for DMA allocation
of data buffers and disables use of tty console and
the virtio control queue.
Signed-off-by: Sjur Brændeland <sjur.brandeland@stericsson.com>
cc: Rusty Russell <rusty@rustcorp.com.au>
cc: Michael S. Tsirkin <mst@redhat.com>
cc: Amit Shah <amit.shah@redhat.com>
cc: Ohad Ben-Cohen <ohad@wizery.com>
cc: Linus Walleij <linus.walleij@linaro.org>
cc: Arnd Bergmann <arnd@arndb.de>
---
Changes since v4:
- New baseline
- Use name is_rproc_enabled
- Renamed list and spin-lock used for pending deletion of dma buffers
- Minor style fixes: indentation, removed brace
drivers/char/virtio_console.c | 184 +++++++++++++++++++++++++++++++++++++----
include/linux/virtio_ids.h | 1 +
2 files changed, 170 insertions(+), 15 deletions(-)
diff --git a/drivers/char/virtio_console.c b/drivers/char/virtio_console.c
index f4f7b04..faedd2c 100644
--- a/drivers/char/virtio_console.c
+++ b/drivers/char/virtio_console.c
@@ -37,8 +37,12 @@
#include <linux/wait.h>
#include <linux/workqueue.h>
#include <linux/module.h>
+#include <linux/dma-mapping.h>
+#include <linux/kconfig.h>
#include "../tty/hvc/hvc_console.h"
+#define is_rproc_enabled IS_ENABLED(CONFIG_REMOTEPROC)
+
/*
* This is a global struct for storing common data for all the devices
* this driver handles.
@@ -112,6 +116,15 @@ struct port_buffer {
/* offset in the buf from which to consume data */
size_t offset;
+ /* DMA address of buffer */
+ dma_addr_t dma;
+
+ /* Device we got DMA memory from */
+ struct device *dev;
+
+ /* List of pending dma buffers to free */
+ struct list_head list;
+
/* If sgpages == 0 then buf is used, else sg is used */
unsigned int sgpages;
@@ -330,6 +343,11 @@ static bool is_console_port(struct port *port)
return false;
}
+static bool is_rproc_serial(const struct virtio_device *vdev)
+{
+ return is_rproc_enabled && vdev->id.device == VIRTIO_ID_RPROC_SERIAL;
+}
+
static inline bool use_multiport(struct ports_device *portdev)
{
/*
@@ -341,32 +359,84 @@ static inline bool use_multiport(struct ports_device *portdev)
return portdev->vdev->features[0] & (1 << VIRTIO_CONSOLE_F_MULTIPORT);
}
+static DEFINE_SPINLOCK(dma_bufs_lock);
+static LIST_HEAD(pending_free_dma_bufs);
+
static void free_buf(struct port_buffer *buf)
{
int i;
+ unsigned long flags;
- kfree(buf->buf);
+ if (!buf->dev)
+ kfree(buf->buf);
- if (buf->sgpages)
+ if (buf->sgpages) {
for (i = 0; i < buf->sgpages; i++) {
struct page *page = sg_page(&buf->sg[i]);
if (!page)
break;
put_page(page);
}
+ return;
+ }
+
+ if (buf->dev && is_rproc_enabled) {
+
+ /* dma_free_coherent requires interrupts to be enabled. */
+ if (irqs_disabled()) {
+ /* queue up dma-buffers to be freed later */
+ spin_lock_irqsave(&dma_bufs_lock, flags);
+ list_add_tail(&buf->list, &pending_free_dma_bufs);
+ spin_unlock_irqrestore(&dma_bufs_lock, flags);
+ return;
+ }
+ dma_free_coherent(buf->dev, buf->size, buf->buf, buf->dma);
+
+ /* Release device refcnt and allow it to be freed */
+ might_sleep();
+ put_device(buf->dev);
+ }
kfree(buf);
}
+static void reclaim_dma_bufs(void)
+{
+ unsigned long flags;
+ struct port_buffer *buf, *tmp;
+ LIST_HEAD(tmp_list);
+
+ WARN_ON(irqs_disabled());
+ if (list_empty(&pending_free_dma_bufs))
+ return;
+
+ BUG_ON(!is_rproc_enabled);
+
+ /* Create a copy of the pending_free_dma_bufs while holding the lock*/
+ spin_lock_irqsave(&dma_bufs_lock, flags);
+ list_cut_position(&tmp_list, &pending_free_dma_bufs,
+ pending_free_dma_bufs.prev);
+ spin_unlock_irqrestore(&dma_bufs_lock, flags);
+
+ /* Release the dma buffers, without irqs enabled */
+ list_for_each_entry_safe(buf, tmp, &tmp_list, list) {
+ list_del(&buf->list);
+ free_buf(buf);
+ }
+}
+
static struct port_buffer *alloc_buf(struct virtqueue *vq, size_t buf_size,
int nrbufs)
{
struct port_buffer *buf;
size_t alloc_size;
+ if (is_rproc_serial(vq->vdev) && !irqs_disabled())
+ reclaim_dma_bufs();
+
/* Allocate buffer and the scatter list */
alloc_size = sizeof(*buf) + sizeof(struct scatterlist) * nrbufs;
- buf = kmalloc(alloc_size, GFP_ATOMIC);
+ buf = kzalloc(alloc_size, GFP_ATOMIC);
if (!buf)
goto fail;
@@ -374,11 +444,30 @@ static struct port_buffer *alloc_buf(struct virtqueue *vq, size_t buf_size,
if (nrbufs > 0)
return buf;
- buf->buf = kmalloc(buf_size, GFP_ATOMIC);
+ if (is_rproc_serial(vq->vdev)) {
+ /*
+ * Allocate DMA memory from ancestor. When a virtio
+ * device is created by remoteproc, the DMA memory is
+ * associated with the grandparent device:
+ * vdev => rproc => platform-dev.
+ * The code here would have been less quirky if
+ * DMA_MEMORY_INCLUDES_CHILDREN had been supported
+ * in dma-coherent.c
+ */
+ if (!vq->vdev->dev.parent || !vq->vdev->dev.parent->parent)
+ goto free_buf;
+ buf->dev = vq->vdev->dev.parent->parent;
+
+ /* Increase device refcnt to avoid freeing it*/
+ get_device(buf->dev);
+ buf->buf = dma_alloc_coherent(buf->dev, buf_size, &buf->dma,
+ GFP_ATOMIC);
+ } else {
+ buf->buf = kmalloc(buf_size, GFP_ATOMIC);
+ }
+
if (!buf->buf)
goto free_buf;
- buf->len = 0;
- buf->offset = 0;
buf->size = buf_size;
/* Prepare scatter buffer for sending */
@@ -838,6 +927,10 @@ static ssize_t port_fops_splice_write(struct pipe_inode_info *pipe,
.u.data = &sgl,
};
+ /* rproc_serial does not support splice */
+ if (is_rproc_serial(port->out_vq->vdev))
+ return -EINVAL;
+
ret = wait_port_writable(port, filp->f_flags & O_NONBLOCK);
if (ret < 0)
return ret;
@@ -902,6 +995,8 @@ static int port_fops_release(struct inode *inode, struct file *filp)
reclaim_consumed_buffers(port);
spin_unlock_irq(&port->outvq_lock);
+ if (is_rproc_serial(port->portdev->vdev) && !irqs_disabled())
+ reclaim_dma_bufs();
/*
* Locks aren't necessary here as a port can't be opened after
* unplug, and if a port isn't unplugged, a kref would already
@@ -1058,7 +1153,10 @@ static void resize_console(struct port *port)
return;
vdev = port->portdev->vdev;
- if (virtio_has_feature(vdev, VIRTIO_CONSOLE_F_SIZE))
+
+ /* Don't test F_SIZE at all if we're rproc: not a valid feature! */
+ if (!is_rproc_serial(vdev) &&
+ virtio_has_feature(vdev, VIRTIO_CONSOLE_F_SIZE))
hvc_resize(port->cons.hvc, port->cons.ws);
}
@@ -1339,10 +1437,18 @@ static int add_port(struct ports_device *portdev, u32 id)
goto free_device;
}
- /*
- * If we're not using multiport support, this has to be a console port
- */
- if (!use_multiport(port->portdev)) {
+ if (is_rproc_serial(port->portdev->vdev))
+ /*
+ * For rproc_serial assume remote processor is connected.
+ * rproc_serial does not want the console port, only
+ * the generic port implementation.
+ */
+ port->host_connected = true;
+ else if (!use_multiport(port->portdev)) {
+ /*
+ * If we're not using multiport support,
+ * this has to be a console port.
+ */
err = init_port_console(port);
if (err)
goto free_inbufs;
@@ -1418,6 +1524,15 @@ static void remove_port_data(struct port *port)
/* Remove buffers we queued up for the Host to send us data in. */
while ((buf = virtqueue_detach_unused_buf(port->in_vq)))
free_buf(buf);
+
+ /*
+ * Remove buffers from out queue for rproc-serial. We cannot afford
+ * to leak any DMA mem, so reclaim this memory even if this might be
+ * racy for the remote processor.
+ */
+ if (is_rproc_serial(port->portdev->vdev))
+ while ((buf = virtqueue_detach_unused_buf(port->out_vq)))
+ free_buf(buf);
}
/*
@@ -1865,11 +1980,15 @@ static int __devinit virtcons_probe(struct virtio_device *vdev)
multiport = false;
portdev->config.max_nr_ports = 1;
- if (virtio_config_val(vdev, VIRTIO_CONSOLE_F_MULTIPORT,
- offsetof(struct virtio_console_config,
- max_nr_ports),
- &portdev->config.max_nr_ports) == 0)
+
+ /* Don't test MULTIPORT at all if we're rproc: not a valid feature! */
+ if (!is_rproc_serial(vdev) &&
+ virtio_config_val(vdev, VIRTIO_CONSOLE_F_MULTIPORT,
+ offsetof(struct virtio_console_config,
+ max_nr_ports),
+ &portdev->config.max_nr_ports) == 0) {
multiport = true;
+ }
err = init_vqs(portdev);
if (err < 0) {
@@ -1979,6 +2098,16 @@ static unsigned int features[] = {
VIRTIO_CONSOLE_F_MULTIPORT,
};
+static struct virtio_device_id rproc_serial_id_table[] = {
+#if IS_ENABLED(CONFIG_REMOTEPROC)
+ { VIRTIO_ID_RPROC_SERIAL, VIRTIO_DEV_ANY_ID },
+#endif
+ { 0 },
+};
+
+static unsigned int rproc_serial_features[] = {
+};
+
#ifdef CONFIG_PM
static int virtcons_freeze(struct virtio_device *vdev)
{
@@ -2063,6 +2192,20 @@ static struct virtio_driver virtio_console = {
#endif
};
+/*
+ * virtio_rproc_serial refers to __devinit function which causes
+ * section mismatch warnings. So use __refdata to silence warnings.
+ */
+static struct virtio_driver __refdata virtio_rproc_serial = {
+ .feature_table = rproc_serial_features,
+ .feature_table_size = ARRAY_SIZE(rproc_serial_features),
+ .driver.name = "virtio_rproc_serial",
+ .driver.owner = THIS_MODULE,
+ .id_table = rproc_serial_id_table,
+ .probe = virtcons_probe,
+ .remove = virtcons_remove,
+};
+
static int __init init(void)
{
int err;
@@ -2087,7 +2230,15 @@ static int __init init(void)
pr_err("Error %d registering virtio driver\n", err);
goto free;
}
+ err = register_virtio_driver(&virtio_rproc_serial);
+ if (err < 0) {
+ pr_err("Error %d registering virtio rproc serial driver\n",
+ err);
+ goto unregister;
+ }
return 0;
+unregister:
+ unregister_virtio_driver(&virtio_console);
free:
if (pdrvdata.debugfs_dir)
debugfs_remove_recursive(pdrvdata.debugfs_dir);
@@ -2097,7 +2248,10 @@ free:
static void __exit fini(void)
{
+ reclaim_dma_bufs();
+
unregister_virtio_driver(&virtio_console);
+ unregister_virtio_driver(&virtio_rproc_serial);
class_destroy(pdrvdata.class);
if (pdrvdata.debugfs_dir)
diff --git a/include/linux/virtio_ids.h b/include/linux/virtio_ids.h
index 270fb22..cb28b52 100644
--- a/include/linux/virtio_ids.h
+++ b/include/linux/virtio_ids.h
@@ -37,5 +37,6 @@
#define VIRTIO_ID_RPMSG 7 /* virtio remote processor messaging */
#define VIRTIO_ID_SCSI 8 /* virtio scsi */
#define VIRTIO_ID_9P 9 /* 9p virtio console */
+#define VIRTIO_ID_RPROC_SERIAL 11 /* virtio remoteproc serial link */
#endif /* _LINUX_VIRTIO_IDS_H */
--
1.7.5.4
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
^ permalink raw reply related
* [PATCH 1/3] virtio_console:Merge struct buffer_token into struct port_buffer
From: sjur.brandeland @ 2012-09-25 13:47 UTC (permalink / raw)
To: Amit Shah
Cc: Michael S. Tsirkin, sjurbren, linux-kernel, virtualization,
Masami Hiramatsu, Linus Walleij, Sjur Brændeland
In-Reply-To: <1348580837-10919-1-git-send-email-sjur.brandeland@stericsson.com>
From: Sjur Brændeland <sjur.brandeland@stericsson.com>
This merge reduces code size by unifying the approach for
sending scatter-lists and regular buffers. Any type of
write operation (splice, write, put_chars) will now allocate
a port_buffer and send_buf() and free_buf() can always be used.
Signed-off-by: Sjur Brændeland <sjur.brandeland@stericsson.com>
cc: Rusty Russell <rusty@rustcorp.com.au>
cc: Michael S. Tsirkin <mst@redhat.com>
cc: Amit Shah <amit.shah@redhat.com>
cc: Linus Walleij <linus.walleij@linaro.org>
cc: Masami Hiramatsu <masami.hiramatsu.pt@hitachi.com>
---
drivers/char/virtio_console.c | 141 ++++++++++++++++++-----------------------
1 files changed, 62 insertions(+), 79 deletions(-)
diff --git a/drivers/char/virtio_console.c b/drivers/char/virtio_console.c
index 8ab9c3d..f4f7b04 100644
--- a/drivers/char/virtio_console.c
+++ b/drivers/char/virtio_console.c
@@ -111,6 +111,11 @@ struct port_buffer {
size_t len;
/* offset in the buf from which to consume data */
size_t offset;
+
+ /* If sgpages == 0 then buf is used, else sg is used */
+ unsigned int sgpages;
+
+ struct scatterlist sg[1];
};
/*
@@ -338,23 +343,46 @@ static inline bool use_multiport(struct ports_device *portdev)
static void free_buf(struct port_buffer *buf)
{
+ int i;
+
kfree(buf->buf);
+
+ if (buf->sgpages)
+ for (i = 0; i < buf->sgpages; i++) {
+ struct page *page = sg_page(&buf->sg[i]);
+ if (!page)
+ break;
+ put_page(page);
+ }
+
kfree(buf);
}
-static struct port_buffer *alloc_buf(size_t buf_size)
+static struct port_buffer *alloc_buf(struct virtqueue *vq, size_t buf_size,
+ int nrbufs)
{
struct port_buffer *buf;
+ size_t alloc_size;
- buf = kmalloc(sizeof(*buf), GFP_KERNEL);
+ /* Allocate buffer and the scatter list */
+ alloc_size = sizeof(*buf) + sizeof(struct scatterlist) * nrbufs;
+ buf = kmalloc(alloc_size, GFP_ATOMIC);
if (!buf)
goto fail;
- buf->buf = kzalloc(buf_size, GFP_KERNEL);
+
+ buf->sgpages = nrbufs;
+ if (nrbufs > 0)
+ return buf;
+
+ buf->buf = kmalloc(buf_size, GFP_ATOMIC);
if (!buf->buf)
goto free_buf;
buf->len = 0;
buf->offset = 0;
buf->size = buf_size;
+
+ /* Prepare scatter buffer for sending */
+ sg_init_one(buf->sg, buf->buf, buf_size);
return buf;
free_buf:
@@ -476,52 +504,25 @@ static ssize_t send_control_msg(struct port *port, unsigned int event,
return 0;
}
-struct buffer_token {
- union {
- void *buf;
- struct scatterlist *sg;
- } u;
- /* If sgpages == 0 then buf is used, else sg is used */
- unsigned int sgpages;
-};
-
-static void reclaim_sg_pages(struct scatterlist *sg, unsigned int nrpages)
-{
- int i;
- struct page *page;
-
- for (i = 0; i < nrpages; i++) {
- page = sg_page(&sg[i]);
- if (!page)
- break;
- put_page(page);
- }
- kfree(sg);
-}
/* Callers must take the port->outvq_lock */
static void reclaim_consumed_buffers(struct port *port)
{
- struct buffer_token *tok;
+ struct port_buffer *buf;
unsigned int len;
if (!port->portdev) {
/* Device has been unplugged. vqs are already gone. */
return;
}
- while ((tok = virtqueue_get_buf(port->out_vq, &len))) {
- if (tok->sgpages)
- reclaim_sg_pages(tok->u.sg, tok->sgpages);
- else
- kfree(tok->u.buf);
- kfree(tok);
+ while ((buf = virtqueue_get_buf(port->out_vq, &len))) {
+ free_buf(buf);
port->outvq_full = false;
}
}
-static ssize_t __send_to_port(struct port *port, struct scatterlist *sg,
- int nents, size_t in_count,
- struct buffer_token *tok, bool nonblock)
+static ssize_t send_buf(struct port *port, struct port_buffer *buf, int nents,
+ size_t in_count, bool nonblock)
{
struct virtqueue *out_vq;
ssize_t ret;
@@ -534,7 +535,7 @@ static ssize_t __send_to_port(struct port *port, struct scatterlist *sg,
reclaim_consumed_buffers(port);
- ret = virtqueue_add_buf(out_vq, sg, nents, 0, tok, GFP_ATOMIC);
+ ret = virtqueue_add_buf(out_vq, buf->sg, nents, 0, buf, GFP_ATOMIC);
/* Tell Host to go! */
virtqueue_kick(out_vq);
@@ -559,8 +560,11 @@ static ssize_t __send_to_port(struct port *port, struct scatterlist *sg,
* we need to kmalloc a GFP_ATOMIC buffer each time the
* console driver writes something out.
*/
- while (!virtqueue_get_buf(out_vq, &len))
+ for (buf = virtqueue_get_buf(out_vq, &len); !buf;
+ buf = virtqueue_get_buf(out_vq, &len))
cpu_relax();
+
+ free_buf(buf);
done:
spin_unlock_irqrestore(&port->outvq_lock, flags);
@@ -572,36 +576,6 @@ done:
return in_count;
}
-static ssize_t send_buf(struct port *port, void *in_buf, size_t in_count,
- bool nonblock)
-{
- struct scatterlist sg[1];
- struct buffer_token *tok;
-
- tok = kmalloc(sizeof(*tok), GFP_ATOMIC);
- if (!tok)
- return -ENOMEM;
- tok->sgpages = 0;
- tok->u.buf = in_buf;
-
- sg_init_one(sg, in_buf, in_count);
-
- return __send_to_port(port, sg, 1, in_count, tok, nonblock);
-}
-
-static ssize_t send_pages(struct port *port, struct scatterlist *sg, int nents,
- size_t in_count, bool nonblock)
-{
- struct buffer_token *tok;
-
- tok = kmalloc(sizeof(*tok), GFP_ATOMIC);
- if (!tok)
- return -ENOMEM;
- tok->sgpages = nents;
- tok->u.sg = sg;
-
- return __send_to_port(port, sg, nents, in_count, tok, nonblock);
-}
/*
* Give out the data that's requested from the buffer that we have
@@ -748,7 +722,7 @@ static ssize_t port_fops_write(struct file *filp, const char __user *ubuf,
size_t count, loff_t *offp)
{
struct port *port;
- char *buf;
+ struct port_buffer *buf;
ssize_t ret;
bool nonblock;
@@ -766,11 +740,11 @@ static ssize_t port_fops_write(struct file *filp, const char __user *ubuf,
count = min((size_t)(32 * 1024), count);
- buf = kmalloc(count, GFP_KERNEL);
+ buf = alloc_buf(port->out_vq, count, 0);
if (!buf)
return -ENOMEM;
- ret = copy_from_user(buf, ubuf, count);
+ ret = copy_from_user(buf->buf, ubuf, count);
if (ret) {
ret = -EFAULT;
goto free_buf;
@@ -784,13 +758,13 @@ static ssize_t port_fops_write(struct file *filp, const char __user *ubuf,
* through to the host.
*/
nonblock = true;
- ret = send_buf(port, buf, count, nonblock);
+ ret = send_buf(port, buf, 1, count, nonblock);
if (nonblock && ret > 0)
goto out;
free_buf:
- kfree(buf);
+ free_buf(buf);
out:
return ret;
}
@@ -856,6 +830,7 @@ static ssize_t port_fops_splice_write(struct pipe_inode_info *pipe,
struct port *port = filp->private_data;
struct sg_list sgl;
ssize_t ret;
+ struct port_buffer *buf;
struct splice_desc sd = {
.total_len = len,
.flags = flags,
@@ -867,17 +842,17 @@ static ssize_t port_fops_splice_write(struct pipe_inode_info *pipe,
if (ret < 0)
return ret;
+ buf = alloc_buf(port->out_vq, 0, pipe->nrbufs);
sgl.n = 0;
sgl.len = 0;
sgl.size = pipe->nrbufs;
- sgl.sg = kmalloc(sizeof(struct scatterlist) * sgl.size, GFP_KERNEL);
- if (unlikely(!sgl.sg))
- return -ENOMEM;
-
+ sgl.sg = buf->sg;
sg_init_table(sgl.sg, sgl.size);
ret = __splice_from_pipe(pipe, &sd, pipe_to_sg);
if (likely(ret > 0))
- ret = send_pages(port, sgl.sg, sgl.n, sgl.len, true);
+ ret = send_buf(port, buf, sgl.n, sgl.len, true);
+ else
+ free_buf(buf);
return ret;
}
@@ -1031,6 +1006,7 @@ static const struct file_operations port_fops = {
static int put_chars(u32 vtermno, const char *buf, int count)
{
struct port *port;
+ struct port_buffer *port_buf;
if (unlikely(early_put_chars))
return early_put_chars(vtermno, buf, count);
@@ -1039,7 +1015,13 @@ static int put_chars(u32 vtermno, const char *buf, int count)
if (!port)
return -EPIPE;
- return send_buf(port, (void *)buf, count, false);
+ port_buf = alloc_buf(port->out_vq, count, 0);
+ if (port_buf == NULL)
+ return -ENOMEM;
+
+ memcpy(port_buf->buf, buf, count);
+
+ return send_buf(port, port_buf, 1, count, false);
}
/*
@@ -1260,10 +1242,11 @@ static unsigned int fill_queue(struct virtqueue *vq, spinlock_t *lock)
nr_added_bufs = 0;
do {
- buf = alloc_buf(PAGE_SIZE);
+ buf = alloc_buf(vq, PAGE_SIZE, 0);
if (!buf)
break;
+ memset(buf->buf, 0, PAGE_SIZE);
spin_lock_irq(lock);
ret = add_inbuf(vq, buf);
if (ret < 0) {
--
1.7.5.4
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
^ permalink raw reply related
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox