* [PATCH v3 1/2] include/uapi/linux/virtio_blk.h: introduce feature of VIRTIO_BLK_F_MQ
From: Ming Lei @ 2014-06-26 9:41 UTC (permalink / raw)
To: Jens Axboe, linux-kernel-u79uwXL29TY76Z2rM5mHXA
Cc: Rusty Russell, linux-api-u79uwXL29TY76Z2rM5mHXA,
virtualization-cunTk1MwBs9QetFLy7KEm3xJsTq8ys+cHZ5vskTnxNA,
Michael S. Tsirkin, Stefan Hajnoczi, Paolo Bonzini, Ming Lei
In-Reply-To: <1403775708-22244-1-git-send-email-ming.lei-Z7WLFzj8eWMS+FvcfC7Uqw@public.gmane.org>
Current virtio-blk spec only supports one virtual queue for transfering
data between VM and host, and inside VM all kinds of operations on
the virtual queue needs to hold one lock, so cause below problems:
- bad scalability
- bad throughput
This patch requests to introduce feature of VIRTIO_BLK_F_MQ
so that more than one virtual queues can be used to virtio-blk
device, then above problems can be solved or eased.
Signed-off-by: Ming Lei <ming.lei-Z7WLFzj8eWMS+FvcfC7Uqw@public.gmane.org>
---
include/uapi/linux/virtio_blk.h | 5 +++++
1 file changed, 5 insertions(+)
diff --git a/include/uapi/linux/virtio_blk.h b/include/uapi/linux/virtio_blk.h
index 6d8e61c..9ad67b2 100644
--- a/include/uapi/linux/virtio_blk.h
+++ b/include/uapi/linux/virtio_blk.h
@@ -40,6 +40,7 @@
#define VIRTIO_BLK_F_WCE 9 /* Writeback mode enabled after reset */
#define VIRTIO_BLK_F_TOPOLOGY 10 /* Topology information is available */
#define VIRTIO_BLK_F_CONFIG_WCE 11 /* Writeback mode available in config */
+#define VIRTIO_BLK_F_MQ 12 /* support more than one vq */
#ifndef __KERNEL__
/* Old (deprecated) name for VIRTIO_BLK_F_WCE. */
@@ -77,6 +78,10 @@ struct virtio_blk_config {
/* writeback mode (if VIRTIO_BLK_F_CONFIG_WCE) */
__u8 wce;
+ __u8 unused;
+
+ /* number of vqs, only available when VIRTIO_BLK_F_MQ is set */
+ __u16 num_queues;
} __attribute__((packed));
/*
--
1.7.9.5
^ permalink raw reply related
* [PATCH v3 0/2] block: virtio-blk: support multi vq per virtio-blk
From: Ming Lei @ 2014-06-26 9:41 UTC (permalink / raw)
To: Jens Axboe, linux-kernel-u79uwXL29TY76Z2rM5mHXA
Cc: Rusty Russell, linux-api-u79uwXL29TY76Z2rM5mHXA,
virtualization-cunTk1MwBs9QetFLy7KEm3xJsTq8ys+cHZ5vskTnxNA,
Michael S. Tsirkin, Stefan Hajnoczi, Paolo Bonzini
Hi,
These patches try to support multi virtual queues(multi-vq) in one
virtio-blk device, and maps each virtual queue(vq) to blk-mq's
hardware queue.
With this approach, both scalability and performance on virtio-blk
device can get improved.
For verifying the improvement, I implements virtio-blk multi-vq over
qemu's dataplane feature, and both handling host notification
from each vq and processing host I/O are still kept in the per-device
iothread context, the change is based on qemu v2.0.0 release, and
can be accessed from below tree:
git://kernel.ubuntu.com/ming/qemu.git #v2.0.0-virtblk-mq.1
For enabling the multi-vq feature, 'num_queues=N' need to be added into
'-device virtio-blk-pci ...' of qemu command line, and suggest to pass
'vectors=N+1' to keep one MSI irq vector per each vq, and the feature
depends on x-data-plane.
Fio(libaio, randread, iodepth=64, bs=4K, jobs=N) is run inside VM to
verify the improvement.
I just create a small quadcore VM and run fio inside the VM, and
num_queues of the virtio-blk device is set as 2, but looks the
improvement is still obvious. The host is 2 sockets, 8cores(16threads)
server.
1), about scalability
- jobs = 2, thoughput: +33%
- jobs = 4, thoughput: +100%
2), about top thoughput: +39%
So in my test, even for a quad-core VM, if the virtqueue number
is increased from 1 to 2, both scalability and performance can
get improved a lot.
In above qemu implementation of virtio-blk-mq device, only one
IOthread handles requests from all vqs, and the above throughput
data has been very close to same fio test in host side with single
job. So more improvement should be observed once more IOthreads are
used for handling requests from multi vqs.
TODO:
- adjust vq's irq smp_affinity according to blk-mq hw queue's cpumask
V3:
- fix use-after-free on vq->name reported by Michael
V2: (suggestions from Michael and Dave Chinner)
- allocate virtqueues' pointers dynamically
- make sure the per-queue spinlock isn't kept in same cache line
- make each queue's name different
V1:
- remove RFC since no one objects
- add '__u8 unused' for pending as suggested by Rusty
- use virtio_cread_feature() directly, suggested by Rusty
Thanks,
--
Ming Lei
^ permalink raw reply
* Re: [PATCH v2 2/2] block: virtio-blk: support multi virt queues per virtio-blk device
From: Ming Lei @ 2014-06-26 8:23 UTC (permalink / raw)
To: Michael S. Tsirkin
Cc: Jens Axboe, linux-api, Linux Kernel Mailing List,
Linux Virtualization, Stefan Hajnoczi, Paolo Bonzini
In-Reply-To: <20140626074551.GA21823@redhat.com>
On Thu, Jun 26, 2014 at 3:45 PM, Michael S. Tsirkin <mst@redhat.com> wrote:
> On Thu, Jun 26, 2014 at 10:08:46AM +0800, Ming Lei wrote:
>> Firstly this patch supports more than one virtual queues for virtio-blk
>> device.
>>
>> Secondly this patch maps the virtual queue to blk-mq's hardware queue.
>>
>> With this approach, both scalability and performance can be improved.
>>
>> Signed-off-by: Ming Lei <ming.lei@canonical.com>
>> ---
>> drivers/block/virtio_blk.c | 109 ++++++++++++++++++++++++++++++++++++--------
>> 1 file changed, 89 insertions(+), 20 deletions(-)
>>
>> diff --git a/drivers/block/virtio_blk.c b/drivers/block/virtio_blk.c
>> index f63d358..b0a49a0 100644
>> --- a/drivers/block/virtio_blk.c
>> +++ b/drivers/block/virtio_blk.c
>> @@ -21,11 +21,14 @@ static DEFINE_IDA(vd_index_ida);
>>
>> static struct workqueue_struct *virtblk_wq;
>>
>> +struct virtio_blk_vq {
>> + struct virtqueue *vq;
>> + spinlock_t lock;
>> +} ____cacheline_aligned_in_smp;
>> +
>
> Padding wastes a hot cacheline here.
> What about this patch I sent:
>
> virtio-blk: move spinlock to vq itself
>
> Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
>
> Rusty didn't respond, try including it as 1/3 in your patchset
> and we'll see if anyone objects?
I think your patch is fine, but I'd like to follow current virtio vq's
lock rule.
Your patch should not be part of this patchset because
you introduce one spinlock inside vq, and you need to
replace other virtio devices' per-vq lock to the builtin lock
too in your patchset.
>
>
>> struct virtio_blk
>> {
>> struct virtio_device *vdev;
>> - struct virtqueue *vq;
>> - spinlock_t vq_lock;
>>
>> /* The disk structure for the kernel. */
>> struct gendisk *disk;
>> @@ -47,6 +50,10 @@ struct virtio_blk
>>
>> /* Ida index - used to track minor number allocations. */
>> int index;
>> +
>> + /* num of vqs */
>> + int num_vqs;
>> + struct virtio_blk_vq *vqs;
>> };
>>
>> struct virtblk_req
>> @@ -133,14 +140,15 @@ static void virtblk_done(struct virtqueue *vq)
>> {
>> struct virtio_blk *vblk = vq->vdev->priv;
>> bool req_done = false;
>> + int qid = vq->index;
>> struct virtblk_req *vbr;
>> unsigned long flags;
>> unsigned int len;
>>
>> - spin_lock_irqsave(&vblk->vq_lock, flags);
>> + spin_lock_irqsave(&vblk->vqs[qid].lock, flags);
>> do {
>> virtqueue_disable_cb(vq);
>> - while ((vbr = virtqueue_get_buf(vblk->vq, &len)) != NULL) {
>> + while ((vbr = virtqueue_get_buf(vblk->vqs[qid].vq, &len)) != NULL) {
>> blk_mq_complete_request(vbr->req);
>> req_done = true;
>> }
>> @@ -151,7 +159,7 @@ static void virtblk_done(struct virtqueue *vq)
>> /* In case queue is stopped waiting for more buffers. */
>> if (req_done)
>> blk_mq_start_stopped_hw_queues(vblk->disk->queue, true);
>> - spin_unlock_irqrestore(&vblk->vq_lock, flags);
>> + spin_unlock_irqrestore(&vblk->vqs[qid].lock, flags);
>> }
>>
>> static int virtio_queue_rq(struct blk_mq_hw_ctx *hctx, struct request *req)
>> @@ -160,6 +168,7 @@ static int virtio_queue_rq(struct blk_mq_hw_ctx *hctx, struct request *req)
>> struct virtblk_req *vbr = blk_mq_rq_to_pdu(req);
>> unsigned long flags;
>> unsigned int num;
>> + int qid = hctx->queue_num;
>> const bool last = (req->cmd_flags & REQ_END) != 0;
>> int err;
>> bool notify = false;
>> @@ -202,12 +211,12 @@ static int virtio_queue_rq(struct blk_mq_hw_ctx *hctx, struct request *req)
>> vbr->out_hdr.type |= VIRTIO_BLK_T_IN;
>> }
>>
>> - spin_lock_irqsave(&vblk->vq_lock, flags);
>> - err = __virtblk_add_req(vblk->vq, vbr, vbr->sg, num);
>> + spin_lock_irqsave(&vblk->vqs[qid].lock, flags);
>> + err = __virtblk_add_req(vblk->vqs[qid].vq, vbr, vbr->sg, num);
>> if (err) {
>> - virtqueue_kick(vblk->vq);
>> + virtqueue_kick(vblk->vqs[qid].vq);
>> blk_mq_stop_hw_queue(hctx);
>> - spin_unlock_irqrestore(&vblk->vq_lock, flags);
>> + spin_unlock_irqrestore(&vblk->vqs[qid].lock, flags);
>> /* Out of mem doesn't actually happen, since we fall back
>> * to direct descriptors */
>> if (err == -ENOMEM || err == -ENOSPC)
>> @@ -215,12 +224,12 @@ static int virtio_queue_rq(struct blk_mq_hw_ctx *hctx, struct request *req)
>> return BLK_MQ_RQ_QUEUE_ERROR;
>> }
>>
>> - if (last && virtqueue_kick_prepare(vblk->vq))
>> + if (last && virtqueue_kick_prepare(vblk->vqs[qid].vq))
>> notify = true;
>> - spin_unlock_irqrestore(&vblk->vq_lock, flags);
>> + spin_unlock_irqrestore(&vblk->vqs[qid].lock, flags);
>>
>> if (notify)
>> - virtqueue_notify(vblk->vq);
>> + virtqueue_notify(vblk->vqs[qid].vq);
>> return BLK_MQ_RQ_QUEUE_OK;
>> }
>>
>> @@ -377,12 +386,71 @@ static void virtblk_config_changed(struct virtio_device *vdev)
>> static int init_vq(struct virtio_blk *vblk)
>> {
>> int err = 0;
>> + int i;
>> + vq_callback_t **callbacks;
>> + const char **names;
>> + char *name_array;
>> + struct virtqueue **vqs;
>> + unsigned short num_vqs;
>> + struct virtio_device *vdev = vblk->vdev;
>>
>> - /* We expect one virtqueue, for output. */
>> - vblk->vq = virtio_find_single_vq(vblk->vdev, virtblk_done, "requests");
>> - if (IS_ERR(vblk->vq))
>> - err = PTR_ERR(vblk->vq);
>> + err = virtio_cread_feature(vdev, VIRTIO_BLK_F_MQ,
>> + struct virtio_blk_config, num_queues,
>> + &num_vqs);
>> + if (err)
>> + num_vqs = 1;
>> +
>> + vblk->vqs = kmalloc(sizeof(*vblk->vqs) * num_vqs, GFP_KERNEL);
>> + if (!vblk->vqs) {
>> + err = -ENOMEM;
>> + goto out;
>> + }
>>
>> + name_array = kmalloc(sizeof(char) * 32 * num_vqs, GFP_KERNEL);
>
> sizeof(char) is 1, just drop it.
> We don't do if (!NULL) just in case someone redefined it, either.
>
>> + if (!name_array)
>> + goto err_name_array;
>
> You want vmalloc here, it will fail on high # of vqs, and speed
> doesn't matter for names.
I don't think there should be lots of vqs:
- each virtio-blk has only one disk, not like virtio-scsi
- with aio, the block io can easily reach its top throughput
with very few IO threads
- for each IO thread, just several vqs can make it at full load
(in my test, 2 vqs can make one iothread at full loading)
- more vqs, more notifications and irqs, which hurt performance too
so I think we needn't consider the huge vqs case until it is
proved to be necessary, we have use one vq per virtio-blk
working for long time at all.
>
>> +
>> + names = kmalloc(sizeof(*names) * num_vqs, GFP_KERNEL);
>> + if (!names)
>> + goto err_names;
>> +
>> + callbacks = kmalloc(sizeof(*callbacks) * num_vqs, GFP_KERNEL);
>> + if (!callbacks)
>> + goto err_callbacks;
>> +
>> + vqs = kmalloc(sizeof(*vqs) * num_vqs, GFP_KERNEL);
>> + if (!vqs)
>> + goto err_vqs;
>> +
>> + for (i = 0; i < num_vqs; i++) {
>> + callbacks[i] = virtblk_done;
>> + snprintf(&name_array[i * 32], 32, "req.%d", i);
>
> Eschew abbreviation. Call it requests.%d.
I like short name because it can fit in 80 character's column
when reading /proc/interrupts.
>
>> + names[i] = &name_array[i * 32];
>> + }
>
> That 32 and pointer math hurts.
> Please create a structure and use an array everywhere.
OK.
>
>
>> +
>> + /* Discover virtqueues and write information to configuration. */
>> + err = vdev->config->find_vqs(vdev, num_vqs, vqs, callbacks, names);
>> + if (err)
>> + goto err_find_vqs;
>> +
>> + for (i = 0; i < num_vqs; i++) {
>> + spin_lock_init(&vblk->vqs[i].lock);
>> + vblk->vqs[i].vq = vqs[i];
>> + }
>> + vblk->num_vqs = num_vqs;
>> +
>> + err_find_vqs:
>> + kfree(vqs);
>> + err_vqs:
>> + kfree(callbacks);
>> + err_callbacks:
>> + kfree(names);
>> + err_names:
>> + kfree(name_array);
>
> This one will cause use after free if vq names are later used, since
> vring_new_virtqueue simply does
> vq->vq.name = name;
>
> You need to keep the memory around until unplug.
That is a bug, will fix that.
>
>> + err_name_array:
>> + if (err)
>> + kfree(vblk->vqs);
>> + out:
>> return err;
>> }
>>
>> @@ -551,7 +619,6 @@ static int virtblk_probe(struct virtio_device *vdev)
>> err = init_vq(vblk);
>> if (err)
>> goto out_free_vblk;
>> - spin_lock_init(&vblk->vq_lock);
>>
>> /* FIXME: How many partitions? How long is a piece of string? */
>> vblk->disk = alloc_disk(1 << PART_BITS);
>> @@ -562,7 +629,7 @@ static int virtblk_probe(struct virtio_device *vdev)
>>
>> /* Default queue sizing is to fill the ring. */
>> if (!virtblk_queue_depth) {
>> - virtblk_queue_depth = vblk->vq->num_free;
>> + virtblk_queue_depth = vblk->vqs[0].vq->num_free;
>> /* ... but without indirect descs, we use 2 descs per req */
>> if (!virtio_has_feature(vdev, VIRTIO_RING_F_INDIRECT_DESC))
>> virtblk_queue_depth /= 2;
>> @@ -570,7 +637,6 @@ static int virtblk_probe(struct virtio_device *vdev)
>>
>> memset(&vblk->tag_set, 0, sizeof(vblk->tag_set));
>> vblk->tag_set.ops = &virtio_mq_ops;
>> - vblk->tag_set.nr_hw_queues = 1;
>> vblk->tag_set.queue_depth = virtblk_queue_depth;
>> vblk->tag_set.numa_node = NUMA_NO_NODE;
>> vblk->tag_set.flags = BLK_MQ_F_SHOULD_MERGE;
>> @@ -578,6 +644,7 @@ static int virtblk_probe(struct virtio_device *vdev)
>> sizeof(struct virtblk_req) +
>> sizeof(struct scatterlist) * sg_elems;
>> vblk->tag_set.driver_data = vblk;
>> + vblk->tag_set.nr_hw_queues = vblk->num_vqs;
>>
>> err = blk_mq_alloc_tag_set(&vblk->tag_set);
>> if (err)
>> @@ -727,6 +794,7 @@ static void virtblk_remove(struct virtio_device *vdev)
>> refc = atomic_read(&disk_to_dev(vblk->disk)->kobj.kref.refcount);
>> put_disk(vblk->disk);
>> vdev->config->del_vqs(vdev);
>> + kfree(vblk->vqs);
>> kfree(vblk);
>>
>> /* Only free device id if we don't have any users */
>> @@ -777,7 +845,8 @@ static const struct virtio_device_id id_table[] = {
>> static unsigned int features[] = {
>> VIRTIO_BLK_F_SEG_MAX, VIRTIO_BLK_F_SIZE_MAX, VIRTIO_BLK_F_GEOMETRY,
>> VIRTIO_BLK_F_RO, VIRTIO_BLK_F_BLK_SIZE, VIRTIO_BLK_F_SCSI,
>> - VIRTIO_BLK_F_WCE, VIRTIO_BLK_F_TOPOLOGY, VIRTIO_BLK_F_CONFIG_WCE
>> + VIRTIO_BLK_F_WCE, VIRTIO_BLK_F_TOPOLOGY, VIRTIO_BLK_F_CONFIG_WCE,
>> + VIRTIO_BLK_F_MQ,
>> };
>>
>> static struct virtio_driver virtio_blk = {
>> --
>> 1.7.9.5
^ permalink raw reply
* Re: [PATCH v2 0/2] block: virtio-blk: support multi vq per virtio-blk
From: Michael S. Tsirkin @ 2014-06-26 7:46 UTC (permalink / raw)
To: Jens Axboe
Cc: linux-api, Ming Lei, linux-kernel, virtualization,
Stefan Hajnoczi, Paolo Bonzini
In-Reply-To: <53ABAA34.6070006@kernel.dk>
On Wed, Jun 25, 2014 at 11:05:56PM -0600, Jens Axboe wrote:
> On 2014-06-25 20:08, Ming Lei wrote:
> >Hi,
> >
> >These patches try to support multi virtual queues(multi-vq) in one
> >virtio-blk device, and maps each virtual queue(vq) to blk-mq's
> >hardware queue.
> >
> >With this approach, both scalability and performance on virtio-blk
> >device can get improved.
> >
> >For verifying the improvement, I implements virtio-blk multi-vq over
> >qemu's dataplane feature, and both handling host notification
> >from each vq and processing host I/O are still kept in the per-device
> >iothread context, the change is based on qemu v2.0.0 release, and
> >can be accessed from below tree:
> >
> > git://kernel.ubuntu.com/ming/qemu.git #v2.0.0-virtblk-mq.1
> >
> >For enabling the multi-vq feature, 'num_queues=N' need to be added into
> >'-device virtio-blk-pci ...' of qemu command line, and suggest to pass
> >'vectors=N+1' to keep one MSI irq vector per each vq, and the feature
> >depends on x-data-plane.
> >
> >Fio(libaio, randread, iodepth=64, bs=4K, jobs=N) is run inside VM to
> >verify the improvement.
> >
> >I just create a small quadcore VM and run fio inside the VM, and
> >num_queues of the virtio-blk device is set as 2, but looks the
> >improvement is still obvious.
> >
> >1), about scalability
> >- without mutli-vq feature
> > -- jobs=2, thoughput: 145K iops
> > -- jobs=4, thoughput: 100K iops
> >- with mutli-vq feature
> > -- jobs=2, thoughput: 193K iops
> > -- jobs=4, thoughput: 202K iops
> >
> >2), about thoughput
> >- without mutli-vq feature
> > -- thoughput: 145K iops
> >- with mutli-vq feature
> > -- thoughput: 202K iops
>
> Of these numbers, I think it's important to highlight that the 2 thread case
> is 33% faster and the 2 -> 4 thread case scales linearly (100%) while the
> pre-patch case sees negative scaling going from 2 -> 4 threads (-39%).
>
> I haven't run your patches yet, but from looking at the code, it looks good.
> It's pretty straightforward. See feel free to add my reviewed-by.
>
> Rusty, do you want to ack this (and I'll slurp it up for 3.17)
Looks like I found some issues, so not yet pls.
> or take this
> yourself? Or something else?
>
>
> --
> Jens Axboe
^ permalink raw reply
* Re: [PATCH v2 2/2] block: virtio-blk: support multi virt queues per virtio-blk device
From: Michael S. Tsirkin @ 2014-06-26 7:45 UTC (permalink / raw)
To: Ming Lei
Cc: Jens Axboe, linux-api, linux-kernel, virtualization,
Stefan Hajnoczi, Paolo Bonzini
In-Reply-To: <1403748526-1923-3-git-send-email-ming.lei@canonical.com>
On Thu, Jun 26, 2014 at 10:08:46AM +0800, Ming Lei wrote:
> Firstly this patch supports more than one virtual queues for virtio-blk
> device.
>
> Secondly this patch maps the virtual queue to blk-mq's hardware queue.
>
> With this approach, both scalability and performance can be improved.
>
> Signed-off-by: Ming Lei <ming.lei@canonical.com>
> ---
> drivers/block/virtio_blk.c | 109 ++++++++++++++++++++++++++++++++++++--------
> 1 file changed, 89 insertions(+), 20 deletions(-)
>
> diff --git a/drivers/block/virtio_blk.c b/drivers/block/virtio_blk.c
> index f63d358..b0a49a0 100644
> --- a/drivers/block/virtio_blk.c
> +++ b/drivers/block/virtio_blk.c
> @@ -21,11 +21,14 @@ static DEFINE_IDA(vd_index_ida);
>
> static struct workqueue_struct *virtblk_wq;
>
> +struct virtio_blk_vq {
> + struct virtqueue *vq;
> + spinlock_t lock;
> +} ____cacheline_aligned_in_smp;
> +
Padding wastes a hot cacheline here.
What about this patch I sent:
virtio-blk: move spinlock to vq itself
Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
Rusty didn't respond, try including it as 1/3 in your patchset
and we'll see if anyone objects?
> struct virtio_blk
> {
> struct virtio_device *vdev;
> - struct virtqueue *vq;
> - spinlock_t vq_lock;
>
> /* The disk structure for the kernel. */
> struct gendisk *disk;
> @@ -47,6 +50,10 @@ struct virtio_blk
>
> /* Ida index - used to track minor number allocations. */
> int index;
> +
> + /* num of vqs */
> + int num_vqs;
> + struct virtio_blk_vq *vqs;
> };
>
> struct virtblk_req
> @@ -133,14 +140,15 @@ static void virtblk_done(struct virtqueue *vq)
> {
> struct virtio_blk *vblk = vq->vdev->priv;
> bool req_done = false;
> + int qid = vq->index;
> struct virtblk_req *vbr;
> unsigned long flags;
> unsigned int len;
>
> - spin_lock_irqsave(&vblk->vq_lock, flags);
> + spin_lock_irqsave(&vblk->vqs[qid].lock, flags);
> do {
> virtqueue_disable_cb(vq);
> - while ((vbr = virtqueue_get_buf(vblk->vq, &len)) != NULL) {
> + while ((vbr = virtqueue_get_buf(vblk->vqs[qid].vq, &len)) != NULL) {
> blk_mq_complete_request(vbr->req);
> req_done = true;
> }
> @@ -151,7 +159,7 @@ static void virtblk_done(struct virtqueue *vq)
> /* In case queue is stopped waiting for more buffers. */
> if (req_done)
> blk_mq_start_stopped_hw_queues(vblk->disk->queue, true);
> - spin_unlock_irqrestore(&vblk->vq_lock, flags);
> + spin_unlock_irqrestore(&vblk->vqs[qid].lock, flags);
> }
>
> static int virtio_queue_rq(struct blk_mq_hw_ctx *hctx, struct request *req)
> @@ -160,6 +168,7 @@ static int virtio_queue_rq(struct blk_mq_hw_ctx *hctx, struct request *req)
> struct virtblk_req *vbr = blk_mq_rq_to_pdu(req);
> unsigned long flags;
> unsigned int num;
> + int qid = hctx->queue_num;
> const bool last = (req->cmd_flags & REQ_END) != 0;
> int err;
> bool notify = false;
> @@ -202,12 +211,12 @@ static int virtio_queue_rq(struct blk_mq_hw_ctx *hctx, struct request *req)
> vbr->out_hdr.type |= VIRTIO_BLK_T_IN;
> }
>
> - spin_lock_irqsave(&vblk->vq_lock, flags);
> - err = __virtblk_add_req(vblk->vq, vbr, vbr->sg, num);
> + spin_lock_irqsave(&vblk->vqs[qid].lock, flags);
> + err = __virtblk_add_req(vblk->vqs[qid].vq, vbr, vbr->sg, num);
> if (err) {
> - virtqueue_kick(vblk->vq);
> + virtqueue_kick(vblk->vqs[qid].vq);
> blk_mq_stop_hw_queue(hctx);
> - spin_unlock_irqrestore(&vblk->vq_lock, flags);
> + spin_unlock_irqrestore(&vblk->vqs[qid].lock, flags);
> /* Out of mem doesn't actually happen, since we fall back
> * to direct descriptors */
> if (err == -ENOMEM || err == -ENOSPC)
> @@ -215,12 +224,12 @@ static int virtio_queue_rq(struct blk_mq_hw_ctx *hctx, struct request *req)
> return BLK_MQ_RQ_QUEUE_ERROR;
> }
>
> - if (last && virtqueue_kick_prepare(vblk->vq))
> + if (last && virtqueue_kick_prepare(vblk->vqs[qid].vq))
> notify = true;
> - spin_unlock_irqrestore(&vblk->vq_lock, flags);
> + spin_unlock_irqrestore(&vblk->vqs[qid].lock, flags);
>
> if (notify)
> - virtqueue_notify(vblk->vq);
> + virtqueue_notify(vblk->vqs[qid].vq);
> return BLK_MQ_RQ_QUEUE_OK;
> }
>
> @@ -377,12 +386,71 @@ static void virtblk_config_changed(struct virtio_device *vdev)
> static int init_vq(struct virtio_blk *vblk)
> {
> int err = 0;
> + int i;
> + vq_callback_t **callbacks;
> + const char **names;
> + char *name_array;
> + struct virtqueue **vqs;
> + unsigned short num_vqs;
> + struct virtio_device *vdev = vblk->vdev;
>
> - /* We expect one virtqueue, for output. */
> - vblk->vq = virtio_find_single_vq(vblk->vdev, virtblk_done, "requests");
> - if (IS_ERR(vblk->vq))
> - err = PTR_ERR(vblk->vq);
> + err = virtio_cread_feature(vdev, VIRTIO_BLK_F_MQ,
> + struct virtio_blk_config, num_queues,
> + &num_vqs);
> + if (err)
> + num_vqs = 1;
> +
> + vblk->vqs = kmalloc(sizeof(*vblk->vqs) * num_vqs, GFP_KERNEL);
> + if (!vblk->vqs) {
> + err = -ENOMEM;
> + goto out;
> + }
>
> + name_array = kmalloc(sizeof(char) * 32 * num_vqs, GFP_KERNEL);
sizeof(char) is 1, just drop it.
We don't do if (!NULL) just in case someone redefined it, either.
> + if (!name_array)
> + goto err_name_array;
You want vmalloc here, it will fail on high # of vqs, and speed
doesn't matter for names.
> +
> + names = kmalloc(sizeof(*names) * num_vqs, GFP_KERNEL);
> + if (!names)
> + goto err_names;
> +
> + callbacks = kmalloc(sizeof(*callbacks) * num_vqs, GFP_KERNEL);
> + if (!callbacks)
> + goto err_callbacks;
> +
> + vqs = kmalloc(sizeof(*vqs) * num_vqs, GFP_KERNEL);
> + if (!vqs)
> + goto err_vqs;
> +
> + for (i = 0; i < num_vqs; i++) {
> + callbacks[i] = virtblk_done;
> + snprintf(&name_array[i * 32], 32, "req.%d", i);
Eschew abbreviation. Call it requests.%d.
> + names[i] = &name_array[i * 32];
> + }
That 32 and pointer math hurts.
Please create a structure and use an array everywhere.
> +
> + /* Discover virtqueues and write information to configuration. */
> + err = vdev->config->find_vqs(vdev, num_vqs, vqs, callbacks, names);
> + if (err)
> + goto err_find_vqs;
> +
> + for (i = 0; i < num_vqs; i++) {
> + spin_lock_init(&vblk->vqs[i].lock);
> + vblk->vqs[i].vq = vqs[i];
> + }
> + vblk->num_vqs = num_vqs;
> +
> + err_find_vqs:
> + kfree(vqs);
> + err_vqs:
> + kfree(callbacks);
> + err_callbacks:
> + kfree(names);
> + err_names:
> + kfree(name_array);
This one will cause use after free if vq names are later used, since
vring_new_virtqueue simply does
vq->vq.name = name;
You need to keep the memory around until unplug.
> + err_name_array:
> + if (err)
> + kfree(vblk->vqs);
> + out:
> return err;
> }
>
> @@ -551,7 +619,6 @@ static int virtblk_probe(struct virtio_device *vdev)
> err = init_vq(vblk);
> if (err)
> goto out_free_vblk;
> - spin_lock_init(&vblk->vq_lock);
>
> /* FIXME: How many partitions? How long is a piece of string? */
> vblk->disk = alloc_disk(1 << PART_BITS);
> @@ -562,7 +629,7 @@ static int virtblk_probe(struct virtio_device *vdev)
>
> /* Default queue sizing is to fill the ring. */
> if (!virtblk_queue_depth) {
> - virtblk_queue_depth = vblk->vq->num_free;
> + virtblk_queue_depth = vblk->vqs[0].vq->num_free;
> /* ... but without indirect descs, we use 2 descs per req */
> if (!virtio_has_feature(vdev, VIRTIO_RING_F_INDIRECT_DESC))
> virtblk_queue_depth /= 2;
> @@ -570,7 +637,6 @@ static int virtblk_probe(struct virtio_device *vdev)
>
> memset(&vblk->tag_set, 0, sizeof(vblk->tag_set));
> vblk->tag_set.ops = &virtio_mq_ops;
> - vblk->tag_set.nr_hw_queues = 1;
> vblk->tag_set.queue_depth = virtblk_queue_depth;
> vblk->tag_set.numa_node = NUMA_NO_NODE;
> vblk->tag_set.flags = BLK_MQ_F_SHOULD_MERGE;
> @@ -578,6 +644,7 @@ static int virtblk_probe(struct virtio_device *vdev)
> sizeof(struct virtblk_req) +
> sizeof(struct scatterlist) * sg_elems;
> vblk->tag_set.driver_data = vblk;
> + vblk->tag_set.nr_hw_queues = vblk->num_vqs;
>
> err = blk_mq_alloc_tag_set(&vblk->tag_set);
> if (err)
> @@ -727,6 +794,7 @@ static void virtblk_remove(struct virtio_device *vdev)
> refc = atomic_read(&disk_to_dev(vblk->disk)->kobj.kref.refcount);
> put_disk(vblk->disk);
> vdev->config->del_vqs(vdev);
> + kfree(vblk->vqs);
> kfree(vblk);
>
> /* Only free device id if we don't have any users */
> @@ -777,7 +845,8 @@ static const struct virtio_device_id id_table[] = {
> static unsigned int features[] = {
> VIRTIO_BLK_F_SEG_MAX, VIRTIO_BLK_F_SIZE_MAX, VIRTIO_BLK_F_GEOMETRY,
> VIRTIO_BLK_F_RO, VIRTIO_BLK_F_BLK_SIZE, VIRTIO_BLK_F_SCSI,
> - VIRTIO_BLK_F_WCE, VIRTIO_BLK_F_TOPOLOGY, VIRTIO_BLK_F_CONFIG_WCE
> + VIRTIO_BLK_F_WCE, VIRTIO_BLK_F_TOPOLOGY, VIRTIO_BLK_F_CONFIG_WCE,
> + VIRTIO_BLK_F_MQ,
> };
>
> static struct virtio_driver virtio_blk = {
> --
> 1.7.9.5
^ permalink raw reply
* Re: [PATCH v2 0/2] block: virtio-blk: support multi vq per virtio-blk
From: Ming Lei @ 2014-06-26 5:28 UTC (permalink / raw)
To: Jens Axboe
Cc: Linux Kernel Mailing List, Rusty Russell,
linux-api-u79uwXL29TY76Z2rM5mHXA, Linux Virtualization,
Michael S. Tsirkin, Stefan Hajnoczi, Paolo Bonzini
In-Reply-To: <53ABAA34.6070006-tSWWG44O7X1aa/9Udqfwiw@public.gmane.org>
On Thu, Jun 26, 2014 at 1:05 PM, Jens Axboe <axboe-tSWWG44O7X1aa/9Udqfwiw@public.gmane.org> wrote:
> On 2014-06-25 20:08, Ming Lei wrote:
>>
>> Hi,
>>
>> These patches try to support multi virtual queues(multi-vq) in one
>> virtio-blk device, and maps each virtual queue(vq) to blk-mq's
>> hardware queue.
>>
>> With this approach, both scalability and performance on virtio-blk
>> device can get improved.
>>
>> For verifying the improvement, I implements virtio-blk multi-vq over
>> qemu's dataplane feature, and both handling host notification
>> from each vq and processing host I/O are still kept in the per-device
>> iothread context, the change is based on qemu v2.0.0 release, and
>> can be accessed from below tree:
>>
>> git://kernel.ubuntu.com/ming/qemu.git #v2.0.0-virtblk-mq.1
>>
>> For enabling the multi-vq feature, 'num_queues=N' need to be added into
>> '-device virtio-blk-pci ...' of qemu command line, and suggest to pass
>> 'vectors=N+1' to keep one MSI irq vector per each vq, and the feature
>> depends on x-data-plane.
>>
>> Fio(libaio, randread, iodepth=64, bs=4K, jobs=N) is run inside VM to
>> verify the improvement.
>>
>> I just create a small quadcore VM and run fio inside the VM, and
>> num_queues of the virtio-blk device is set as 2, but looks the
>> improvement is still obvious.
>>
>> 1), about scalability
>> - without mutli-vq feature
>> -- jobs=2, thoughput: 145K iops
>> -- jobs=4, thoughput: 100K iops
>> - with mutli-vq feature
>> -- jobs=2, thoughput: 193K iops
>> -- jobs=4, thoughput: 202K iops
>>
>> 2), about thoughput
>> - without mutli-vq feature
>> -- thoughput: 145K iops
>> - with mutli-vq feature
>> -- thoughput: 202K iops
>
>
> Of these numbers, I think it's important to highlight that the 2 thread case
> is 33% faster and the 2 -> 4 thread case scales linearly (100%) while the
> pre-patch case sees negative scaling going from 2 -> 4 threads (-39%).
This is because my qemu implementation on multi vq only uses
single iothread to handle requests from all vqs, and the only iothread
is already at full load now, that said on host side the
same fio test(single job) results is ~200K iops too.
>
> I haven't run your patches yet, but from looking at the code, it looks good.
> It's pretty straightforward. See feel free to add my reviewed-by.
Thanks a lot.
>
> Rusty, do you want to ack this (and I'll slurp it up for 3.17) or take this
> yourself? Or something else?
That is great if this can be merged to 3.17.
Thanks,
--
Ming Lei
^ permalink raw reply
* Re: [PATCH v2 0/2] block: virtio-blk: support multi vq per virtio-blk
From: Jens Axboe @ 2014-06-26 5:05 UTC (permalink / raw)
To: Ming Lei, linux-kernel-u79uwXL29TY76Z2rM5mHXA
Cc: Rusty Russell, linux-api-u79uwXL29TY76Z2rM5mHXA,
virtualization-cunTk1MwBs9QetFLy7KEm3xJsTq8ys+cHZ5vskTnxNA,
Michael S. Tsirkin, Stefan Hajnoczi, Paolo Bonzini
In-Reply-To: <1403748526-1923-1-git-send-email-ming.lei-Z7WLFzj8eWMS+FvcfC7Uqw@public.gmane.org>
On 2014-06-25 20:08, Ming Lei wrote:
> Hi,
>
> These patches try to support multi virtual queues(multi-vq) in one
> virtio-blk device, and maps each virtual queue(vq) to blk-mq's
> hardware queue.
>
> With this approach, both scalability and performance on virtio-blk
> device can get improved.
>
> For verifying the improvement, I implements virtio-blk multi-vq over
> qemu's dataplane feature, and both handling host notification
> from each vq and processing host I/O are still kept in the per-device
> iothread context, the change is based on qemu v2.0.0 release, and
> can be accessed from below tree:
>
> git://kernel.ubuntu.com/ming/qemu.git #v2.0.0-virtblk-mq.1
>
> For enabling the multi-vq feature, 'num_queues=N' need to be added into
> '-device virtio-blk-pci ...' of qemu command line, and suggest to pass
> 'vectors=N+1' to keep one MSI irq vector per each vq, and the feature
> depends on x-data-plane.
>
> Fio(libaio, randread, iodepth=64, bs=4K, jobs=N) is run inside VM to
> verify the improvement.
>
> I just create a small quadcore VM and run fio inside the VM, and
> num_queues of the virtio-blk device is set as 2, but looks the
> improvement is still obvious.
>
> 1), about scalability
> - without mutli-vq feature
> -- jobs=2, thoughput: 145K iops
> -- jobs=4, thoughput: 100K iops
> - with mutli-vq feature
> -- jobs=2, thoughput: 193K iops
> -- jobs=4, thoughput: 202K iops
>
> 2), about thoughput
> - without mutli-vq feature
> -- thoughput: 145K iops
> - with mutli-vq feature
> -- thoughput: 202K iops
Of these numbers, I think it's important to highlight that the 2 thread
case is 33% faster and the 2 -> 4 thread case scales linearly (100%)
while the pre-patch case sees negative scaling going from 2 -> 4 threads
(-39%).
I haven't run your patches yet, but from looking at the code, it looks
good. It's pretty straightforward. See feel free to add my reviewed-by.
Rusty, do you want to ack this (and I'll slurp it up for 3.17) or take
this yourself? Or something else?
--
Jens Axboe
^ permalink raw reply
* [PATCH v2 2/2] block: virtio-blk: support multi virt queues per virtio-blk device
From: Ming Lei @ 2014-06-26 2:08 UTC (permalink / raw)
To: Jens Axboe, linux-kernel
Cc: Rusty Russell, linux-api, virtualization, Michael S. Tsirkin,
Stefan Hajnoczi, Paolo Bonzini, Ming Lei
In-Reply-To: <1403748526-1923-1-git-send-email-ming.lei@canonical.com>
Firstly this patch supports more than one virtual queues for virtio-blk
device.
Secondly this patch maps the virtual queue to blk-mq's hardware queue.
With this approach, both scalability and performance can be improved.
Signed-off-by: Ming Lei <ming.lei@canonical.com>
---
drivers/block/virtio_blk.c | 109 ++++++++++++++++++++++++++++++++++++--------
1 file changed, 89 insertions(+), 20 deletions(-)
diff --git a/drivers/block/virtio_blk.c b/drivers/block/virtio_blk.c
index f63d358..b0a49a0 100644
--- a/drivers/block/virtio_blk.c
+++ b/drivers/block/virtio_blk.c
@@ -21,11 +21,14 @@ static DEFINE_IDA(vd_index_ida);
static struct workqueue_struct *virtblk_wq;
+struct virtio_blk_vq {
+ struct virtqueue *vq;
+ spinlock_t lock;
+} ____cacheline_aligned_in_smp;
+
struct virtio_blk
{
struct virtio_device *vdev;
- struct virtqueue *vq;
- spinlock_t vq_lock;
/* The disk structure for the kernel. */
struct gendisk *disk;
@@ -47,6 +50,10 @@ struct virtio_blk
/* Ida index - used to track minor number allocations. */
int index;
+
+ /* num of vqs */
+ int num_vqs;
+ struct virtio_blk_vq *vqs;
};
struct virtblk_req
@@ -133,14 +140,15 @@ static void virtblk_done(struct virtqueue *vq)
{
struct virtio_blk *vblk = vq->vdev->priv;
bool req_done = false;
+ int qid = vq->index;
struct virtblk_req *vbr;
unsigned long flags;
unsigned int len;
- spin_lock_irqsave(&vblk->vq_lock, flags);
+ spin_lock_irqsave(&vblk->vqs[qid].lock, flags);
do {
virtqueue_disable_cb(vq);
- while ((vbr = virtqueue_get_buf(vblk->vq, &len)) != NULL) {
+ while ((vbr = virtqueue_get_buf(vblk->vqs[qid].vq, &len)) != NULL) {
blk_mq_complete_request(vbr->req);
req_done = true;
}
@@ -151,7 +159,7 @@ static void virtblk_done(struct virtqueue *vq)
/* In case queue is stopped waiting for more buffers. */
if (req_done)
blk_mq_start_stopped_hw_queues(vblk->disk->queue, true);
- spin_unlock_irqrestore(&vblk->vq_lock, flags);
+ spin_unlock_irqrestore(&vblk->vqs[qid].lock, flags);
}
static int virtio_queue_rq(struct blk_mq_hw_ctx *hctx, struct request *req)
@@ -160,6 +168,7 @@ static int virtio_queue_rq(struct blk_mq_hw_ctx *hctx, struct request *req)
struct virtblk_req *vbr = blk_mq_rq_to_pdu(req);
unsigned long flags;
unsigned int num;
+ int qid = hctx->queue_num;
const bool last = (req->cmd_flags & REQ_END) != 0;
int err;
bool notify = false;
@@ -202,12 +211,12 @@ static int virtio_queue_rq(struct blk_mq_hw_ctx *hctx, struct request *req)
vbr->out_hdr.type |= VIRTIO_BLK_T_IN;
}
- spin_lock_irqsave(&vblk->vq_lock, flags);
- err = __virtblk_add_req(vblk->vq, vbr, vbr->sg, num);
+ spin_lock_irqsave(&vblk->vqs[qid].lock, flags);
+ err = __virtblk_add_req(vblk->vqs[qid].vq, vbr, vbr->sg, num);
if (err) {
- virtqueue_kick(vblk->vq);
+ virtqueue_kick(vblk->vqs[qid].vq);
blk_mq_stop_hw_queue(hctx);
- spin_unlock_irqrestore(&vblk->vq_lock, flags);
+ spin_unlock_irqrestore(&vblk->vqs[qid].lock, flags);
/* Out of mem doesn't actually happen, since we fall back
* to direct descriptors */
if (err == -ENOMEM || err == -ENOSPC)
@@ -215,12 +224,12 @@ static int virtio_queue_rq(struct blk_mq_hw_ctx *hctx, struct request *req)
return BLK_MQ_RQ_QUEUE_ERROR;
}
- if (last && virtqueue_kick_prepare(vblk->vq))
+ if (last && virtqueue_kick_prepare(vblk->vqs[qid].vq))
notify = true;
- spin_unlock_irqrestore(&vblk->vq_lock, flags);
+ spin_unlock_irqrestore(&vblk->vqs[qid].lock, flags);
if (notify)
- virtqueue_notify(vblk->vq);
+ virtqueue_notify(vblk->vqs[qid].vq);
return BLK_MQ_RQ_QUEUE_OK;
}
@@ -377,12 +386,71 @@ static void virtblk_config_changed(struct virtio_device *vdev)
static int init_vq(struct virtio_blk *vblk)
{
int err = 0;
+ int i;
+ vq_callback_t **callbacks;
+ const char **names;
+ char *name_array;
+ struct virtqueue **vqs;
+ unsigned short num_vqs;
+ struct virtio_device *vdev = vblk->vdev;
- /* We expect one virtqueue, for output. */
- vblk->vq = virtio_find_single_vq(vblk->vdev, virtblk_done, "requests");
- if (IS_ERR(vblk->vq))
- err = PTR_ERR(vblk->vq);
+ err = virtio_cread_feature(vdev, VIRTIO_BLK_F_MQ,
+ struct virtio_blk_config, num_queues,
+ &num_vqs);
+ if (err)
+ num_vqs = 1;
+
+ vblk->vqs = kmalloc(sizeof(*vblk->vqs) * num_vqs, GFP_KERNEL);
+ if (!vblk->vqs) {
+ err = -ENOMEM;
+ goto out;
+ }
+ name_array = kmalloc(sizeof(char) * 32 * num_vqs, GFP_KERNEL);
+ if (!name_array)
+ goto err_name_array;
+
+ names = kmalloc(sizeof(*names) * num_vqs, GFP_KERNEL);
+ if (!names)
+ goto err_names;
+
+ callbacks = kmalloc(sizeof(*callbacks) * num_vqs, GFP_KERNEL);
+ if (!callbacks)
+ goto err_callbacks;
+
+ vqs = kmalloc(sizeof(*vqs) * num_vqs, GFP_KERNEL);
+ if (!vqs)
+ goto err_vqs;
+
+ for (i = 0; i < num_vqs; i++) {
+ callbacks[i] = virtblk_done;
+ snprintf(&name_array[i * 32], 32, "req.%d", i);
+ names[i] = &name_array[i * 32];
+ }
+
+ /* Discover virtqueues and write information to configuration. */
+ err = vdev->config->find_vqs(vdev, num_vqs, vqs, callbacks, names);
+ if (err)
+ goto err_find_vqs;
+
+ for (i = 0; i < num_vqs; i++) {
+ spin_lock_init(&vblk->vqs[i].lock);
+ vblk->vqs[i].vq = vqs[i];
+ }
+ vblk->num_vqs = num_vqs;
+
+ err_find_vqs:
+ kfree(vqs);
+ err_vqs:
+ kfree(callbacks);
+ err_callbacks:
+ kfree(names);
+ err_names:
+ kfree(name_array);
+ err_name_array:
+ if (err)
+ kfree(vblk->vqs);
+ out:
return err;
}
@@ -551,7 +619,6 @@ static int virtblk_probe(struct virtio_device *vdev)
err = init_vq(vblk);
if (err)
goto out_free_vblk;
- spin_lock_init(&vblk->vq_lock);
/* FIXME: How many partitions? How long is a piece of string? */
vblk->disk = alloc_disk(1 << PART_BITS);
@@ -562,7 +629,7 @@ static int virtblk_probe(struct virtio_device *vdev)
/* Default queue sizing is to fill the ring. */
if (!virtblk_queue_depth) {
- virtblk_queue_depth = vblk->vq->num_free;
+ virtblk_queue_depth = vblk->vqs[0].vq->num_free;
/* ... but without indirect descs, we use 2 descs per req */
if (!virtio_has_feature(vdev, VIRTIO_RING_F_INDIRECT_DESC))
virtblk_queue_depth /= 2;
@@ -570,7 +637,6 @@ static int virtblk_probe(struct virtio_device *vdev)
memset(&vblk->tag_set, 0, sizeof(vblk->tag_set));
vblk->tag_set.ops = &virtio_mq_ops;
- vblk->tag_set.nr_hw_queues = 1;
vblk->tag_set.queue_depth = virtblk_queue_depth;
vblk->tag_set.numa_node = NUMA_NO_NODE;
vblk->tag_set.flags = BLK_MQ_F_SHOULD_MERGE;
@@ -578,6 +644,7 @@ static int virtblk_probe(struct virtio_device *vdev)
sizeof(struct virtblk_req) +
sizeof(struct scatterlist) * sg_elems;
vblk->tag_set.driver_data = vblk;
+ vblk->tag_set.nr_hw_queues = vblk->num_vqs;
err = blk_mq_alloc_tag_set(&vblk->tag_set);
if (err)
@@ -727,6 +794,7 @@ static void virtblk_remove(struct virtio_device *vdev)
refc = atomic_read(&disk_to_dev(vblk->disk)->kobj.kref.refcount);
put_disk(vblk->disk);
vdev->config->del_vqs(vdev);
+ kfree(vblk->vqs);
kfree(vblk);
/* Only free device id if we don't have any users */
@@ -777,7 +845,8 @@ static const struct virtio_device_id id_table[] = {
static unsigned int features[] = {
VIRTIO_BLK_F_SEG_MAX, VIRTIO_BLK_F_SIZE_MAX, VIRTIO_BLK_F_GEOMETRY,
VIRTIO_BLK_F_RO, VIRTIO_BLK_F_BLK_SIZE, VIRTIO_BLK_F_SCSI,
- VIRTIO_BLK_F_WCE, VIRTIO_BLK_F_TOPOLOGY, VIRTIO_BLK_F_CONFIG_WCE
+ VIRTIO_BLK_F_WCE, VIRTIO_BLK_F_TOPOLOGY, VIRTIO_BLK_F_CONFIG_WCE,
+ VIRTIO_BLK_F_MQ,
};
static struct virtio_driver virtio_blk = {
--
1.7.9.5
^ permalink raw reply related
* [PATCH v2 1/2] include/uapi/linux/virtio_blk.h: introduce feature of VIRTIO_BLK_F_MQ
From: Ming Lei @ 2014-06-26 2:08 UTC (permalink / raw)
To: Jens Axboe, linux-kernel
Cc: Rusty Russell, linux-api, virtualization, Michael S. Tsirkin,
Stefan Hajnoczi, Paolo Bonzini, Ming Lei
In-Reply-To: <1403748526-1923-1-git-send-email-ming.lei@canonical.com>
Current virtio-blk spec only supports one virtual queue for transfering
data between VM and host, and inside VM all kinds of operations on
the virtual queue needs to hold one lock, so cause below problems:
- bad scalability
- bad throughput
This patch requests to introduce feature of VIRTIO_BLK_F_MQ
so that more than one virtual queues can be used to virtio-blk
device, then above problems can be solved or eased.
Signed-off-by: Ming Lei <ming.lei@canonical.com>
---
include/uapi/linux/virtio_blk.h | 5 +++++
1 file changed, 5 insertions(+)
diff --git a/include/uapi/linux/virtio_blk.h b/include/uapi/linux/virtio_blk.h
index 6d8e61c..9ad67b2 100644
--- a/include/uapi/linux/virtio_blk.h
+++ b/include/uapi/linux/virtio_blk.h
@@ -40,6 +40,7 @@
#define VIRTIO_BLK_F_WCE 9 /* Writeback mode enabled after reset */
#define VIRTIO_BLK_F_TOPOLOGY 10 /* Topology information is available */
#define VIRTIO_BLK_F_CONFIG_WCE 11 /* Writeback mode available in config */
+#define VIRTIO_BLK_F_MQ 12 /* support more than one vq */
#ifndef __KERNEL__
/* Old (deprecated) name for VIRTIO_BLK_F_WCE. */
@@ -77,6 +78,10 @@ struct virtio_blk_config {
/* writeback mode (if VIRTIO_BLK_F_CONFIG_WCE) */
__u8 wce;
+ __u8 unused;
+
+ /* number of vqs, only available when VIRTIO_BLK_F_MQ is set */
+ __u16 num_queues;
} __attribute__((packed));
/*
--
1.7.9.5
^ permalink raw reply related
* [PATCH v2 0/2] block: virtio-blk: support multi vq per virtio-blk
From: Ming Lei @ 2014-06-26 2:08 UTC (permalink / raw)
To: Jens Axboe, linux-kernel-u79uwXL29TY76Z2rM5mHXA
Cc: Rusty Russell, linux-api-u79uwXL29TY76Z2rM5mHXA,
virtualization-cunTk1MwBs9QetFLy7KEm3xJsTq8ys+cHZ5vskTnxNA,
Michael S. Tsirkin, Stefan Hajnoczi, Paolo Bonzini
Hi,
These patches try to support multi virtual queues(multi-vq) in one
virtio-blk device, and maps each virtual queue(vq) to blk-mq's
hardware queue.
With this approach, both scalability and performance on virtio-blk
device can get improved.
For verifying the improvement, I implements virtio-blk multi-vq over
qemu's dataplane feature, and both handling host notification
from each vq and processing host I/O are still kept in the per-device
iothread context, the change is based on qemu v2.0.0 release, and
can be accessed from below tree:
git://kernel.ubuntu.com/ming/qemu.git #v2.0.0-virtblk-mq.1
For enabling the multi-vq feature, 'num_queues=N' need to be added into
'-device virtio-blk-pci ...' of qemu command line, and suggest to pass
'vectors=N+1' to keep one MSI irq vector per each vq, and the feature
depends on x-data-plane.
Fio(libaio, randread, iodepth=64, bs=4K, jobs=N) is run inside VM to
verify the improvement.
I just create a small quadcore VM and run fio inside the VM, and
num_queues of the virtio-blk device is set as 2, but looks the
improvement is still obvious.
1), about scalability
- without mutli-vq feature
-- jobs=2, thoughput: 145K iops
-- jobs=4, thoughput: 100K iops
- with mutli-vq feature
-- jobs=2, thoughput: 193K iops
-- jobs=4, thoughput: 202K iops
2), about thoughput
- without mutli-vq feature
-- thoughput: 145K iops
- with mutli-vq feature
-- thoughput: 202K iops
So in my test, even for a quad-core VM, if the virtqueue number
is increased from 1 to 2, both scalability and performance can
get improved a lot.
TODO:
- adjust vq's irq smp_affinity according to blk-mq hw queue's cpumask
V2: (suggestions from Michael and Dave Chinner)
- allocate virtqueues' pointers dynamically
- make sure the per-queue spinlock isn't kept in same cache line
- make each queue's name different
V1:
- remove RFC since no one objects
- add '__u8 unused' for pending as suggested by Rusty
- use virtio_cread_feature() directly, suggested by Rusty
Thanks,
--
Ming Lei
^ permalink raw reply
* Re: [PATCH] mm: update the description for madvise_remove
From: David Rientjes @ 2014-06-25 22:30 UTC (permalink / raw)
To: Wang Sheng-Hui
Cc: Michael Kerrisk, Andrew Morton, Naoya Horiguchi, Johannes Weiner,
Andi Kleen, Vladimir Cernov, linux-mm, linux-api, linux-man
In-Reply-To: <53AA2CD5.6060202@gmail.com>
On Wed, 25 Jun 2014, Wang Sheng-Hui wrote:
> Patch to man-page.
>
> [PATCH] madvise.2: update the description for MADV_REMOVE
>
> Currently we have more filesystems supporting fallcate, e.g ext4/btrfs,
> which can response to MADV_REMOVE gracefully.
>
> And if filesystems don't support fallocate, the return error would be
> EOPNOTSUPP, instead of ENOSYS.
>
> Signed-off-by: Wang Sheng-Hui <shhuiw@gmail.com>
Great, thanks! This looks like it can be applied immediately and doesn't
require us to wait for any kernel change.
Good catch with the return value.
> ---
> man2/madvise.2 | 10 +++-------
> 1 file changed, 3 insertions(+), 7 deletions(-)
>
> diff --git a/man2/madvise.2 b/man2/madvise.2
> index 032ead7..4ce869c 100644
> --- a/man2/madvise.2
> +++ b/man2/madvise.2
> @@ -99,13 +99,9 @@ or zero-fill-on-demand pages for mappings
> without an underlying file.
> .TP
> .BR MADV_REMOVE " (since Linux 2.6.16)"
> -Free up a given range of pages
> -and its associated backing store.
> -Currently,
> -.\" 2.6.18-rc5
> -only shmfs/tmpfs supports this; other filesystems return with the
> -error
> -.BR ENOSYS .
> +Free up a given range of pages and its associated backing store.
> +Filesystems that don't support fallocate will return error
> +.BR EOPNOTSUPP.
> .\" Databases want to use this feature to drop a section of their
> .\" bufferpool (shared memory segments) - without writing back to
> .\" disk/swap space. This feature is also useful for supporting
--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org. For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>
^ permalink raw reply
* Re: [PATCH v8 9/9] seccomp: implement SECCOMP_FILTER_FLAG_TSYNC
From: Kees Cook @ 2014-06-25 18:31 UTC (permalink / raw)
To: Oleg Nesterov
Cc: linux-arch, linux-mips-6z/3iImG2C8G8FEW9MqTrA, Will Drewry,
linux-security-module, Linux API,
x86-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org, LKML,
Andy Lutomirski, Daniel Borkmann, Julien Tinnes,
Michael Kerrisk (man-pages), Andrew Morton, David Drysdale,
linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r@public.gmane.org,
Alexei Starovoitov
In-Reply-To: <20140625182012.GA19437-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
On Wed, Jun 25, 2014 at 11:20 AM, Oleg Nesterov <oleg-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org> wrote:
> On 06/25, Kees Cook wrote:
>>
>> On Wed, Jun 25, 2014 at 10:24 AM, Oleg Nesterov <oleg-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org> wrote:
>> >
>> > However, do_execve() takes cred_guard_mutex at the start in prepare_bprm_creds()
>> > and drops it in install_exec_creds(), so it should solve the problem?
>>
>> I can't tell yet. I'm still trying to understand the order of
>> operations here. It looks like de_thread() takes the sighand lock.
>> do_execve_common does:
>>
>> prepare_bprm_creds (takes cred_guard_mutex)
>> check_unsafe_exec (checks nnp to set LSM_UNSAFE_NO_NEW_PRIVS)
>> prepare_binprm (handles suid escalation, checks nnp separately)
>> security_bprm_set_creds (checks LSM_UNSAFE_NO_NEW_PRIVS)
>> exec_binprm
>> load_elf_binary
>> flush_old_exec
>> de_thread (takes and releases sighand->lock)
>> install_exec_creds (releases cred_guard_mutex)
>
> Yes, and note that when cred_guard_mutex is dropped all other threads
> are already killed,
>
>> I don't see a way to use cred_guard_mutex during tsync (which holds
>> sighand->lock) without dead-locking. What were you considering here?
>
> Just take/drop current->signal->cred_guard_mutex along with ->siglock
> in seccomp_set_mode_filter() ? Unconditionally on depending on
> SECCOMP_FILTER_FLAG_TSYNC.
Yeah, this looks good. *whew* Testing it now, so far so good.
Thanks!
-Kees
--
Kees Cook
Chrome OS Security
^ permalink raw reply
* Re: [PATCH v8 3/9] seccomp: introduce writer locking
From: Oleg Nesterov @ 2014-06-25 18:29 UTC (permalink / raw)
To: Kees Cook
Cc: linux-kernel, Andy Lutomirski, Michael Kerrisk (man-pages),
Alexei Starovoitov, Andrew Morton, Daniel Borkmann, Will Drewry,
Julien Tinnes, David Drysdale, linux-api, x86, linux-arm-kernel,
linux-mips, linux-arch, linux-security-module
In-Reply-To: <20140625180705.GB18185@redhat.com>
On 06/25, Oleg Nesterov wrote:
>
> On 06/24, Kees Cook wrote:
> >
> > +static void copy_seccomp(struct task_struct *p)
> > +{
> > +#ifdef CONFIG_SECCOMP
> > + /*
> > + * Must be called with sighand->lock held, which is common to
> > + * all threads in the group. Regardless, nothing special is
> > + * needed for the child since it is not yet in the tasklist.
> > + */
> > + BUG_ON(!spin_is_locked(¤t->sighand->siglock));
> > +
> > + get_seccomp_filter(current);
> > + p->seccomp = current->seccomp;
> > +
> > + if (p->seccomp.mode != SECCOMP_MODE_DISABLED)
> > + set_tsk_thread_flag(p, TIF_SECCOMP);
> > +#endif
> > +}
>
> Wait. But what about no_new_privs? We should copy it as well...
>
> Perhaps this helper should be updated a bit and moved into seccomp.c so
> that seccomp_sync_threads() could use it too.
This way we can also unexport get_seccomp_filter().
Oleg.
^ permalink raw reply
* Re: [PATCH v8 9/9] seccomp: implement SECCOMP_FILTER_FLAG_TSYNC
From: Kees Cook @ 2014-06-25 18:25 UTC (permalink / raw)
To: Andy Lutomirski
Cc: Oleg Nesterov, LKML, Michael Kerrisk (man-pages),
Alexei Starovoitov, Andrew Morton, Daniel Borkmann, Will Drewry,
Julien Tinnes, David Drysdale, Linux API, x86@kernel.org,
linux-arm-kernel@lists.infradead.org, linux-mips, linux-arch,
linux-security-module
In-Reply-To: <CALCETrXr+g6tM+9gQJKKnGj_XoyXi+7ZQpAWjykhOAErpE8MBw@mail.gmail.com>
On Wed, Jun 25, 2014 at 11:09 AM, Andy Lutomirski <luto@amacapital.net> wrote:
> On Wed, Jun 25, 2014 at 10:57 AM, Kees Cook <keescook@chromium.org> wrote:
>> On Wed, Jun 25, 2014 at 10:24 AM, Oleg Nesterov <oleg@redhat.com> wrote:
>>> On 06/25, Kees Cook wrote:
>>>>
>>>> On Wed, Jun 25, 2014 at 9:52 AM, Oleg Nesterov <oleg@redhat.com> wrote:
>>>> >
>>>> > Yes, at least this should close the race with suid-exec. And there are no
>>>> > other users. Except apparmor, and I hope you will check it because I simply
>>>> > do not know what it does ;)
>>>> >
>>>> >> I wonder if changes to nnp need to "flushed" during syscall entry
>>>> >> instead of getting updated externally/asynchronously? That way it
>>>> >> won't be out of sync with the seccomp mode/filters.
>>>> >>
>>>> >> Perhaps secure computing needs to check some (maybe seccomp-only)
>>>> >> atomic flags and flip on the "real" nnp if found?
>>>> >
>>>> > Not sure I understand you, could you clarify?
>>>>
>>>> Instead of having TSYNC change the nnp bit, it can set a new flag, say:
>>>>
>>>> task->seccomp.flags |= SECCOMP_NEEDS_NNP;
>>>>
>>>> This would be set along with seccomp.mode, seccomp.filter, and
>>>> TIF_SECCOMP. Then, during the next secure_computing() call that thread
>>>> makes, it would check the flag:
>>>>
>>>> if (task->seccomp.flags & SECCOMP_NEEDS_NNP)
>>>> task->nnp = 1;
>>>>
>>>> This means that nnp couldn't change in the middle of a running syscall.
>>>
>>> Aha, so you were worried about the same thing. Not sure we need this,
>>> but at least I understand you and...
>>>
>>>> Hmmm. Perhaps this doesn't solve anything, though? Perhaps my proposal
>>>> above would actually make things worse, since now we'd have a thread
>>>> with seccomp set up, and no nnp. If it was in the middle of exec,
>>>> we're still causing a problem.
>>>
>>> Yes ;)
>>>
>>>> I think we'd also need a way to either delay the seccomp changes, or
>>>> to notice this condition during exec. Bleh.
>>>
>>> Hmm. confused again,
>>
>> I mean to suggest that the tsync changes would be stored in each
>> thread, but somewhere other than the true seccomp struct, but with
>> TIF_SECCOMP set. When entering secure_computing(), current would check
>> for the "changes to sync", and apply them, then start the syscall. In
>> this way, we can never race a syscall (like exec).
>
> I'm not sure that helps. If you set a pending filter part-way through
> exec, and exec copies that pending filter but doesn't notice NNP, then
> there's an exploitable race.
>
>>
>>>> What actually happens with a multi-threaded process calls exec? I
>>>> assume all the other threads are destroyed?
>>>
>>> Yes. But this is the point-of-no-return, de_thread() is called after the execing
>>> thared has already passed (say) check_unsafe_exec().
>>>
>>> However, do_execve() takes cred_guard_mutex at the start in prepare_bprm_creds()
>>> and drops it in install_exec_creds(), so it should solve the problem?
>>
>> I can't tell yet. I'm still trying to understand the order of
>> operations here. It looks like de_thread() takes the sighand lock.
>> do_execve_common does:
>>
>> prepare_bprm_creds (takes cred_guard_mutex)
>> check_unsafe_exec (checks nnp to set LSM_UNSAFE_NO_NEW_PRIVS)
>> prepare_binprm (handles suid escalation, checks nnp separately)
>> security_bprm_set_creds (checks LSM_UNSAFE_NO_NEW_PRIVS)
>> exec_binprm
>> load_elf_binary
>> flush_old_exec
>> de_thread (takes and releases sighand->lock)
>> install_exec_creds (releases cred_guard_mutex)
>>
>> I don't see a way to use cred_guard_mutex during tsync (which holds
>> sighand->lock) without dead-locking. What were you considering here?
>
> Grab cred_guard_mutex and then sighand->lock, perhaps?
Ah, yes, task->signal is like sighand: shared across all threads.
--
Kees Cook
Chrome OS Security
^ permalink raw reply
* Re: [PATCH v8 9/9] seccomp: implement SECCOMP_FILTER_FLAG_TSYNC
From: Oleg Nesterov @ 2014-06-25 18:20 UTC (permalink / raw)
To: Kees Cook
Cc: LKML, Andy Lutomirski, Michael Kerrisk (man-pages),
Alexei Starovoitov, Andrew Morton, Daniel Borkmann, Will Drewry,
Julien Tinnes, David Drysdale, Linux API, x86@kernel.org,
linux-arm-kernel@lists.infradead.org, linux-mips, linux-arch,
linux-security-module
In-Reply-To: <CAGXu5jKkLS3++_dtWHnjWudVvaSR9DRwjNG3q00SmSy6XoCMaw@mail.gmail.com>
On 06/25, Kees Cook wrote:
>
> On Wed, Jun 25, 2014 at 10:24 AM, Oleg Nesterov <oleg@redhat.com> wrote:
> >
> > However, do_execve() takes cred_guard_mutex at the start in prepare_bprm_creds()
> > and drops it in install_exec_creds(), so it should solve the problem?
>
> I can't tell yet. I'm still trying to understand the order of
> operations here. It looks like de_thread() takes the sighand lock.
> do_execve_common does:
>
> prepare_bprm_creds (takes cred_guard_mutex)
> check_unsafe_exec (checks nnp to set LSM_UNSAFE_NO_NEW_PRIVS)
> prepare_binprm (handles suid escalation, checks nnp separately)
> security_bprm_set_creds (checks LSM_UNSAFE_NO_NEW_PRIVS)
> exec_binprm
> load_elf_binary
> flush_old_exec
> de_thread (takes and releases sighand->lock)
> install_exec_creds (releases cred_guard_mutex)
Yes, and note that when cred_guard_mutex is dropped all other threads
are already killed,
> I don't see a way to use cred_guard_mutex during tsync (which holds
> sighand->lock) without dead-locking. What were you considering here?
Just take/drop current->signal->cred_guard_mutex along with ->siglock
in seccomp_set_mode_filter() ? Unconditionally on depending on
SECCOMP_FILTER_FLAG_TSYNC.
Oleg.
^ permalink raw reply
* Re: [PATCH v8 9/9] seccomp: implement SECCOMP_FILTER_FLAG_TSYNC
From: Andy Lutomirski @ 2014-06-25 18:09 UTC (permalink / raw)
To: Kees Cook
Cc: Oleg Nesterov, LKML, Michael Kerrisk (man-pages),
Alexei Starovoitov, Andrew Morton, Daniel Borkmann, Will Drewry,
Julien Tinnes, David Drysdale, Linux API, x86@kernel.org,
linux-arm-kernel@lists.infradead.org, linux-mips, linux-arch,
linux-security-module
In-Reply-To: <CAGXu5jKkLS3++_dtWHnjWudVvaSR9DRwjNG3q00SmSy6XoCMaw@mail.gmail.com>
On Wed, Jun 25, 2014 at 10:57 AM, Kees Cook <keescook@chromium.org> wrote:
> On Wed, Jun 25, 2014 at 10:24 AM, Oleg Nesterov <oleg@redhat.com> wrote:
>> On 06/25, Kees Cook wrote:
>>>
>>> On Wed, Jun 25, 2014 at 9:52 AM, Oleg Nesterov <oleg@redhat.com> wrote:
>>> >
>>> > Yes, at least this should close the race with suid-exec. And there are no
>>> > other users. Except apparmor, and I hope you will check it because I simply
>>> > do not know what it does ;)
>>> >
>>> >> I wonder if changes to nnp need to "flushed" during syscall entry
>>> >> instead of getting updated externally/asynchronously? That way it
>>> >> won't be out of sync with the seccomp mode/filters.
>>> >>
>>> >> Perhaps secure computing needs to check some (maybe seccomp-only)
>>> >> atomic flags and flip on the "real" nnp if found?
>>> >
>>> > Not sure I understand you, could you clarify?
>>>
>>> Instead of having TSYNC change the nnp bit, it can set a new flag, say:
>>>
>>> task->seccomp.flags |= SECCOMP_NEEDS_NNP;
>>>
>>> This would be set along with seccomp.mode, seccomp.filter, and
>>> TIF_SECCOMP. Then, during the next secure_computing() call that thread
>>> makes, it would check the flag:
>>>
>>> if (task->seccomp.flags & SECCOMP_NEEDS_NNP)
>>> task->nnp = 1;
>>>
>>> This means that nnp couldn't change in the middle of a running syscall.
>>
>> Aha, so you were worried about the same thing. Not sure we need this,
>> but at least I understand you and...
>>
>>> Hmmm. Perhaps this doesn't solve anything, though? Perhaps my proposal
>>> above would actually make things worse, since now we'd have a thread
>>> with seccomp set up, and no nnp. If it was in the middle of exec,
>>> we're still causing a problem.
>>
>> Yes ;)
>>
>>> I think we'd also need a way to either delay the seccomp changes, or
>>> to notice this condition during exec. Bleh.
>>
>> Hmm. confused again,
>
> I mean to suggest that the tsync changes would be stored in each
> thread, but somewhere other than the true seccomp struct, but with
> TIF_SECCOMP set. When entering secure_computing(), current would check
> for the "changes to sync", and apply them, then start the syscall. In
> this way, we can never race a syscall (like exec).
I'm not sure that helps. If you set a pending filter part-way through
exec, and exec copies that pending filter but doesn't notice NNP, then
there's an exploitable race.
>
>>> What actually happens with a multi-threaded process calls exec? I
>>> assume all the other threads are destroyed?
>>
>> Yes. But this is the point-of-no-return, de_thread() is called after the execing
>> thared has already passed (say) check_unsafe_exec().
>>
>> However, do_execve() takes cred_guard_mutex at the start in prepare_bprm_creds()
>> and drops it in install_exec_creds(), so it should solve the problem?
>
> I can't tell yet. I'm still trying to understand the order of
> operations here. It looks like de_thread() takes the sighand lock.
> do_execve_common does:
>
> prepare_bprm_creds (takes cred_guard_mutex)
> check_unsafe_exec (checks nnp to set LSM_UNSAFE_NO_NEW_PRIVS)
> prepare_binprm (handles suid escalation, checks nnp separately)
> security_bprm_set_creds (checks LSM_UNSAFE_NO_NEW_PRIVS)
> exec_binprm
> load_elf_binary
> flush_old_exec
> de_thread (takes and releases sighand->lock)
> install_exec_creds (releases cred_guard_mutex)
>
> I don't see a way to use cred_guard_mutex during tsync (which holds
> sighand->lock) without dead-locking. What were you considering here?
>
Grab cred_guard_mutex and then sighand->lock, perhaps?
> -Kees
>
> --
> Kees Cook
> Chrome OS Security
--
Andy Lutomirski
AMA Capital Management, LLC
^ permalink raw reply
* Re: [PATCH v8 5/9] seccomp: split mode set routines
From: Andy Lutomirski @ 2014-06-25 18:07 UTC (permalink / raw)
To: Kees Cook
Cc: Oleg Nesterov, LKML, Michael Kerrisk (man-pages),
Alexei Starovoitov, Andrew Morton, Daniel Borkmann, Will Drewry,
Julien Tinnes, David Drysdale, Linux API,
x86-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org,
linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r@public.gmane.org,
linux-mips-6z/3iImG2C8G8FEW9MqTrA, linux-arch,
linux-security-module
In-Reply-To: <CAGXu5jL17k6=GXju6x+eLU20FMwBHhnuRiHoQD1Bzj_EmpiKjg-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org>
On Wed, Jun 25, 2014 at 11:00 AM, Kees Cook <keescook-F7+t8E8rja9g9hUCZPvPmw@public.gmane.org> wrote:
> On Wed, Jun 25, 2014 at 10:51 AM, Oleg Nesterov <oleg-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org> wrote:
>> On 06/25, Andy Lutomirski wrote:
>>>
>>> On Wed, Jun 25, 2014 at 10:32 AM, Oleg Nesterov <oleg-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org> wrote:
>>> > On 06/25, Andy Lutomirski wrote:
>>> >>
>>> >> Write the filter, then smp_mb (or maybe a weaker barrier is okay),
>>> >> then set the bit.
>>> >
>>> > Yes, exactly, this is what I meant. Plas rmb() in __secure_computing().
>>> >
>>> > But I still can't understand the rest of your discussion about the
>>> > ordering we need ;)
>>>
>>> Let me try again from scratch.
>>>
>>> Currently there are three relevant variables: TIF_SECCOMP,
>>> seccomp.mode, and seccomp.filter. __secure_computing needs
>>> seccomp.mode and seccomp.filter to be in sync, and it wants (but
>>> doesn't really need) TIF_SECCOMP to be in sync as well.
>>>
>>> My suggestion is to rearrange it a bit. Move mode into seccomp.filter
>>> (so that filter == NULL implies no seccomp) and don't check
>
> This would require that we reimplement mode 1 seccomp via mode 2
> filters. Which isn't too hard, but may add complexity.
>
>>> TIF_SECCOMP in secure_computing. Then turning on seccomp is entirely
>>> atomic except for the fact that the seccomp hooks won't be called if
>>> filter != NULL but !TIF_SECCOMP. This removes all ordering
>>> requirements.
>>
>> Ah, got it, thanks. Perhaps I missed somehing, but to me this looks like
>> unnecessary complication at first glance.
>>
>> We alredy have TIF_SECCOMP, we need it anyway, and we should only care
>> about the case when this bit is actually set, so that we can race with
>> the 1st call of __secure_computing().
>>
>> Otherwise we are fine: we can miss the new filter anyway, ->mode can't
>> be changed it is already nonzero.
>>
>>> Alternatively, __secure_computing could still BUG_ON(!seccomp.filter).
>>> In that case, filter needs to be set before TIF_SECCOMP is set, but
>>> that's straightforward.
>>
>> Yep. And this is how seccomp_assign_mode() already works? It is called
>> after we change ->filter chain, it changes ->mode before set(TIF_SECCOMP)
>> just it lacks a barrier.
>
> Right, I think the best solution is to add the barrier. I was
> concerned that adding the read barrier in secure_computing would have
> a performance impact, though.
>
I can't speak for ARM, but I think that all of the read barriers are
essentially free on x86. (smp_mb is a very different story, but that
shouldn't be needed here.)
--Andy
^ permalink raw reply
* Re: [PATCH v8 3/9] seccomp: introduce writer locking
From: Oleg Nesterov @ 2014-06-25 18:07 UTC (permalink / raw)
To: Kees Cook
Cc: linux-kernel, Andy Lutomirski, Michael Kerrisk (man-pages),
Alexei Starovoitov, Andrew Morton, Daniel Borkmann, Will Drewry,
Julien Tinnes, David Drysdale, linux-api, x86, linux-arm-kernel,
linux-mips, linux-arch, linux-security-module
In-Reply-To: <1403642893-23107-4-git-send-email-keescook@chromium.org>
On 06/24, Kees Cook wrote:
>
> +static void copy_seccomp(struct task_struct *p)
> +{
> +#ifdef CONFIG_SECCOMP
> + /*
> + * Must be called with sighand->lock held, which is common to
> + * all threads in the group. Regardless, nothing special is
> + * needed for the child since it is not yet in the tasklist.
> + */
> + BUG_ON(!spin_is_locked(¤t->sighand->siglock));
> +
> + get_seccomp_filter(current);
> + p->seccomp = current->seccomp;
> +
> + if (p->seccomp.mode != SECCOMP_MODE_DISABLED)
> + set_tsk_thread_flag(p, TIF_SECCOMP);
> +#endif
> +}
Wait. But what about no_new_privs? We should copy it as well...
Perhaps this helper should be updated a bit and moved into seccomp.c so
that seccomp_sync_threads() could use it too.
Oleg.
^ permalink raw reply
* Re: [PATCH v8 5/9] seccomp: split mode set routines
From: Kees Cook @ 2014-06-25 18:00 UTC (permalink / raw)
To: Oleg Nesterov
Cc: Andy Lutomirski, LKML, Michael Kerrisk (man-pages),
Alexei Starovoitov, Andrew Morton, Daniel Borkmann, Will Drewry,
Julien Tinnes, David Drysdale, Linux API, x86@kernel.org,
linux-arm-kernel@lists.infradead.org, linux-mips, linux-arch,
linux-security-module
In-Reply-To: <20140625175136.GA18185@redhat.com>
On Wed, Jun 25, 2014 at 10:51 AM, Oleg Nesterov <oleg@redhat.com> wrote:
> On 06/25, Andy Lutomirski wrote:
>>
>> On Wed, Jun 25, 2014 at 10:32 AM, Oleg Nesterov <oleg@redhat.com> wrote:
>> > On 06/25, Andy Lutomirski wrote:
>> >>
>> >> Write the filter, then smp_mb (or maybe a weaker barrier is okay),
>> >> then set the bit.
>> >
>> > Yes, exactly, this is what I meant. Plas rmb() in __secure_computing().
>> >
>> > But I still can't understand the rest of your discussion about the
>> > ordering we need ;)
>>
>> Let me try again from scratch.
>>
>> Currently there are three relevant variables: TIF_SECCOMP,
>> seccomp.mode, and seccomp.filter. __secure_computing needs
>> seccomp.mode and seccomp.filter to be in sync, and it wants (but
>> doesn't really need) TIF_SECCOMP to be in sync as well.
>>
>> My suggestion is to rearrange it a bit. Move mode into seccomp.filter
>> (so that filter == NULL implies no seccomp) and don't check
This would require that we reimplement mode 1 seccomp via mode 2
filters. Which isn't too hard, but may add complexity.
>> TIF_SECCOMP in secure_computing. Then turning on seccomp is entirely
>> atomic except for the fact that the seccomp hooks won't be called if
>> filter != NULL but !TIF_SECCOMP. This removes all ordering
>> requirements.
>
> Ah, got it, thanks. Perhaps I missed somehing, but to me this looks like
> unnecessary complication at first glance.
>
> We alredy have TIF_SECCOMP, we need it anyway, and we should only care
> about the case when this bit is actually set, so that we can race with
> the 1st call of __secure_computing().
>
> Otherwise we are fine: we can miss the new filter anyway, ->mode can't
> be changed it is already nonzero.
>
>> Alternatively, __secure_computing could still BUG_ON(!seccomp.filter).
>> In that case, filter needs to be set before TIF_SECCOMP is set, but
>> that's straightforward.
>
> Yep. And this is how seccomp_assign_mode() already works? It is called
> after we change ->filter chain, it changes ->mode before set(TIF_SECCOMP)
> just it lacks a barrier.
Right, I think the best solution is to add the barrier. I was
concerned that adding the read barrier in secure_computing would have
a performance impact, though.
--
Kees Cook
Chrome OS Security
^ permalink raw reply
* Re: [PATCH v8 9/9] seccomp: implement SECCOMP_FILTER_FLAG_TSYNC
From: Kees Cook @ 2014-06-25 17:57 UTC (permalink / raw)
To: Oleg Nesterov
Cc: LKML, Andy Lutomirski, Michael Kerrisk (man-pages),
Alexei Starovoitov, Andrew Morton, Daniel Borkmann, Will Drewry,
Julien Tinnes, David Drysdale, Linux API, x86@kernel.org,
linux-arm-kernel@lists.infradead.org, linux-mips, linux-arch,
linux-security-module
In-Reply-To: <20140625172410.GA17133@redhat.com>
On Wed, Jun 25, 2014 at 10:24 AM, Oleg Nesterov <oleg@redhat.com> wrote:
> On 06/25, Kees Cook wrote:
>>
>> On Wed, Jun 25, 2014 at 9:52 AM, Oleg Nesterov <oleg@redhat.com> wrote:
>> >
>> > Yes, at least this should close the race with suid-exec. And there are no
>> > other users. Except apparmor, and I hope you will check it because I simply
>> > do not know what it does ;)
>> >
>> >> I wonder if changes to nnp need to "flushed" during syscall entry
>> >> instead of getting updated externally/asynchronously? That way it
>> >> won't be out of sync with the seccomp mode/filters.
>> >>
>> >> Perhaps secure computing needs to check some (maybe seccomp-only)
>> >> atomic flags and flip on the "real" nnp if found?
>> >
>> > Not sure I understand you, could you clarify?
>>
>> Instead of having TSYNC change the nnp bit, it can set a new flag, say:
>>
>> task->seccomp.flags |= SECCOMP_NEEDS_NNP;
>>
>> This would be set along with seccomp.mode, seccomp.filter, and
>> TIF_SECCOMP. Then, during the next secure_computing() call that thread
>> makes, it would check the flag:
>>
>> if (task->seccomp.flags & SECCOMP_NEEDS_NNP)
>> task->nnp = 1;
>>
>> This means that nnp couldn't change in the middle of a running syscall.
>
> Aha, so you were worried about the same thing. Not sure we need this,
> but at least I understand you and...
>
>> Hmmm. Perhaps this doesn't solve anything, though? Perhaps my proposal
>> above would actually make things worse, since now we'd have a thread
>> with seccomp set up, and no nnp. If it was in the middle of exec,
>> we're still causing a problem.
>
> Yes ;)
>
>> I think we'd also need a way to either delay the seccomp changes, or
>> to notice this condition during exec. Bleh.
>
> Hmm. confused again,
I mean to suggest that the tsync changes would be stored in each
thread, but somewhere other than the true seccomp struct, but with
TIF_SECCOMP set. When entering secure_computing(), current would check
for the "changes to sync", and apply them, then start the syscall. In
this way, we can never race a syscall (like exec).
>> What actually happens with a multi-threaded process calls exec? I
>> assume all the other threads are destroyed?
>
> Yes. But this is the point-of-no-return, de_thread() is called after the execing
> thared has already passed (say) check_unsafe_exec().
>
> However, do_execve() takes cred_guard_mutex at the start in prepare_bprm_creds()
> and drops it in install_exec_creds(), so it should solve the problem?
I can't tell yet. I'm still trying to understand the order of
operations here. It looks like de_thread() takes the sighand lock.
do_execve_common does:
prepare_bprm_creds (takes cred_guard_mutex)
check_unsafe_exec (checks nnp to set LSM_UNSAFE_NO_NEW_PRIVS)
prepare_binprm (handles suid escalation, checks nnp separately)
security_bprm_set_creds (checks LSM_UNSAFE_NO_NEW_PRIVS)
exec_binprm
load_elf_binary
flush_old_exec
de_thread (takes and releases sighand->lock)
install_exec_creds (releases cred_guard_mutex)
I don't see a way to use cred_guard_mutex during tsync (which holds
sighand->lock) without dead-locking. What were you considering here?
-Kees
--
Kees Cook
Chrome OS Security
^ permalink raw reply
* Re: [PATCH v8 1/1] man-pages: seccomp.2: document syscall
From: Michael Kerrisk (man-pages) @ 2014-06-25 17:54 UTC (permalink / raw)
To: Kees Cook
Cc: One Thousand Gnomes, Oleg Nesterov, Andy Lutomirski,
Alexei Starovoitov, Andrew Morton, Daniel Borkmann, Will Drewry,
Julien Tinnes, David Drysdale, Linux API, LKML, x86@kernel.org,
linux-arm-kernel@lists.infradead.org, linux-mips, linux-arch,
linux-security-module
In-Reply-To: <CAGXu5j+99NOtJq2-TWYm8mwNw1ki0y3rRH21wX66MVM8=jz1bQ@mail.gmail.com>
On Wed, Jun 25, 2014 at 5:10 PM, Kees Cook <keescook@chromium.org> wrote:
> On Wed, Jun 25, 2014 at 6:04 AM, One Thousand Gnomes
> <gnomes@lxorguk.ukuu.org.uk> wrote:
>> On Tue, 24 Jun 2014 13:56:15 -0700
>> Kees Cook <keescook@chromium.org> wrote:
>>
>>> Combines documentation from prctl, in-kernel seccomp_filter.txt and
>>> dropper.c, along with details specific to the new syscall.
>>
>> What is the license on the example ? Probably you want to propogate the
>> minimal form of the text in seccomp/dropper into the document example to
>> avoid confusion ?
>
> What is the license of the other code examples in man-pages?
Typically, just the same as the rest if the page text. Perhaps that
should be rethought for future examples. I haven't thought about it at
length, but, at first glance, I'm not against having separate licenses
for the page text and the code sample.
Cheers,
Michael
--
Michael Kerrisk
Linux man-pages maintainer; http://www.kernel.org/doc/man-pages/
Linux/UNIX System Programming Training: http://man7.org/training/
^ permalink raw reply
* Re: [PATCH v8 5/9] seccomp: split mode set routines
From: Oleg Nesterov @ 2014-06-25 17:51 UTC (permalink / raw)
To: Andy Lutomirski
Cc: Kees Cook, LKML, Michael Kerrisk (man-pages), Alexei Starovoitov,
Andrew Morton, Daniel Borkmann, Will Drewry, Julien Tinnes,
David Drysdale, Linux API, x86@kernel.org,
linux-arm-kernel@lists.infradead.org, linux-mips, linux-arch,
linux-security-module
In-Reply-To: <CALCETrUc65H+fn6dtMdYnB_xR39wcmgDdTbdR3fFRjyrndJhgA@mail.gmail.com>
On 06/25, Andy Lutomirski wrote:
>
> On Wed, Jun 25, 2014 at 10:32 AM, Oleg Nesterov <oleg@redhat.com> wrote:
> > On 06/25, Andy Lutomirski wrote:
> >>
> >> Write the filter, then smp_mb (or maybe a weaker barrier is okay),
> >> then set the bit.
> >
> > Yes, exactly, this is what I meant. Plas rmb() in __secure_computing().
> >
> > But I still can't understand the rest of your discussion about the
> > ordering we need ;)
>
> Let me try again from scratch.
>
> Currently there are three relevant variables: TIF_SECCOMP,
> seccomp.mode, and seccomp.filter. __secure_computing needs
> seccomp.mode and seccomp.filter to be in sync, and it wants (but
> doesn't really need) TIF_SECCOMP to be in sync as well.
>
> My suggestion is to rearrange it a bit. Move mode into seccomp.filter
> (so that filter == NULL implies no seccomp) and don't check
> TIF_SECCOMP in secure_computing. Then turning on seccomp is entirely
> atomic except for the fact that the seccomp hooks won't be called if
> filter != NULL but !TIF_SECCOMP. This removes all ordering
> requirements.
Ah, got it, thanks. Perhaps I missed somehing, but to me this looks like
unnecessary complication at first glance.
We alredy have TIF_SECCOMP, we need it anyway, and we should only care
about the case when this bit is actually set, so that we can race with
the 1st call of __secure_computing().
Otherwise we are fine: we can miss the new filter anyway, ->mode can't
be changed it is already nonzero.
> Alternatively, __secure_computing could still BUG_ON(!seccomp.filter).
> In that case, filter needs to be set before TIF_SECCOMP is set, but
> that's straightforward.
Yep. And this is how seccomp_assign_mode() already works? It is called
after we change ->filter chain, it changes ->mode before set(TIF_SECCOMP)
just it lacks a barrier.
Oleg.
^ permalink raw reply
* Re: [PATCH v8 9/9] seccomp: implement SECCOMP_FILTER_FLAG_TSYNC
From: Andy Lutomirski @ 2014-06-25 17:40 UTC (permalink / raw)
To: Oleg Nesterov
Cc: Kees Cook, LKML, Michael Kerrisk (man-pages), Alexei Starovoitov,
Andrew Morton, Daniel Borkmann, Will Drewry, Julien Tinnes,
David Drysdale, Linux API, x86@kernel.org,
linux-arm-kernel@lists.infradead.org, linux-mips, linux-arch,
linux-security-module
In-Reply-To: <20140625172410.GA17133@redhat.com>
On Wed, Jun 25, 2014 at 10:24 AM, Oleg Nesterov <oleg@redhat.com> wrote:
> On 06/25, Kees Cook wrote:
>>
>> On Wed, Jun 25, 2014 at 9:52 AM, Oleg Nesterov <oleg@redhat.com> wrote:
>> >
>> > Yes, at least this should close the race with suid-exec. And there are no
>> > other users. Except apparmor, and I hope you will check it because I simply
>> > do not know what it does ;)
>> >
>> >> I wonder if changes to nnp need to "flushed" during syscall entry
>> >> instead of getting updated externally/asynchronously? That way it
>> >> won't be out of sync with the seccomp mode/filters.
>> >>
>> >> Perhaps secure computing needs to check some (maybe seccomp-only)
>> >> atomic flags and flip on the "real" nnp if found?
>> >
>> > Not sure I understand you, could you clarify?
>>
>> Instead of having TSYNC change the nnp bit, it can set a new flag, say:
>>
>> task->seccomp.flags |= SECCOMP_NEEDS_NNP;
>>
>> This would be set along with seccomp.mode, seccomp.filter, and
>> TIF_SECCOMP. Then, during the next secure_computing() call that thread
>> makes, it would check the flag:
>>
>> if (task->seccomp.flags & SECCOMP_NEEDS_NNP)
>> task->nnp = 1;
>>
>> This means that nnp couldn't change in the middle of a running syscall.
>
> Aha, so you were worried about the same thing. Not sure we need this,
> but at least I understand you and...
>
>> Hmmm. Perhaps this doesn't solve anything, though? Perhaps my proposal
>> above would actually make things worse, since now we'd have a thread
>> with seccomp set up, and no nnp. If it was in the middle of exec,
>> we're still causing a problem.
>
> Yes ;)
>
>> I think we'd also need a way to either delay the seccomp changes, or
>> to notice this condition during exec. Bleh.
>
> Hmm. confused again,
>
>> What actually happens with a multi-threaded process calls exec? I
>> assume all the other threads are destroyed?
>
> Yes. But this is the point-of-no-return, de_thread() is called after the execing
> thared has already passed (say) check_unsafe_exec().
>
> However, do_execve() takes cred_guard_mutex at the start in prepare_bprm_creds()
> and drops it in install_exec_creds(), so it should solve the problem?
If you rely on this, then please fix this comment in fs/exec.c:
/*
* determine how safe it is to execute the proposed program
* - the caller must hold ->cred_guard_mutex to protect against
* PTRACE_ATTACH
*/
static void check_unsafe_exec(struct linux_binprm *bprm)
It sounds like cred_guard_mutex is there for exactly this reason :)
--Andy
^ permalink raw reply
* Re: [PATCH v8 5/9] seccomp: split mode set routines
From: Andy Lutomirski @ 2014-06-25 17:38 UTC (permalink / raw)
To: Oleg Nesterov
Cc: Kees Cook, LKML, Michael Kerrisk (man-pages), Alexei Starovoitov,
Andrew Morton, Daniel Borkmann, Will Drewry, Julien Tinnes,
David Drysdale, Linux API,
x86-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org,
linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r@public.gmane.org,
linux-mips-6z/3iImG2C8G8FEW9MqTrA, linux-arch,
linux-security-module
In-Reply-To: <20140625173245.GA17695-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
On Wed, Jun 25, 2014 at 10:32 AM, Oleg Nesterov <oleg-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org> wrote:
> On 06/25, Andy Lutomirski wrote:
>>
>> Write the filter, then smp_mb (or maybe a weaker barrier is okay),
>> then set the bit.
>
> Yes, exactly, this is what I meant. Plas rmb() in __secure_computing().
>
> But I still can't understand the rest of your discussion about the
> ordering we need ;)
Let me try again from scratch.
Currently there are three relevant variables: TIF_SECCOMP,
seccomp.mode, and seccomp.filter. __secure_computing needs
seccomp.mode and seccomp.filter to be in sync, and it wants (but
doesn't really need) TIF_SECCOMP to be in sync as well.
My suggestion is to rearrange it a bit. Move mode into seccomp.filter
(so that filter == NULL implies no seccomp) and don't check
TIF_SECCOMP in secure_computing. Then turning on seccomp is entirely
atomic except for the fact that the seccomp hooks won't be called if
filter != NULL but !TIF_SECCOMP. This removes all ordering
requirements.
Alternatively, __secure_computing could still BUG_ON(!seccomp.filter).
In that case, filter needs to be set before TIF_SECCOMP is set, but
that's straightforward.
--Andy
^ permalink raw reply
* Re: [PATCH v8 5/9] seccomp: split mode set routines
From: Oleg Nesterov @ 2014-06-25 17:32 UTC (permalink / raw)
To: Andy Lutomirski
Cc: Kees Cook, LKML, Michael Kerrisk (man-pages), Alexei Starovoitov,
Andrew Morton, Daniel Borkmann, Will Drewry, Julien Tinnes,
David Drysdale, Linux API, x86@kernel.org,
linux-arm-kernel@lists.infradead.org, linux-mips, linux-arch,
linux-security-module
In-Reply-To: <CALCETrVrs8sb19+UUqyFEpAFzTih5dkAwn-WpQjfgPcPJMpP5g@mail.gmail.com>
On 06/25, Andy Lutomirski wrote:
>
> Write the filter, then smp_mb (or maybe a weaker barrier is okay),
> then set the bit.
Yes, exactly, this is what I meant. Plas rmb() in __secure_computing().
But I still can't understand the rest of your discussion about the
ordering we need ;)
Oleg.
^ permalink raw reply
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