[PATCH] kvm tools: Process virito blk requests in separate thread

[PATCH] kvm tools: Process virito blk requests in separate thread

[Asias He]

Currently, all blk requests are processed in notify_vq() which is in
the context of ioeventfd thread: ioeventfd__thread(). The processing
in notify_vq() may take a long time to complete.

We should make notify_vq() return as soon as possible, since all devices
are sharing the single ioeventfd thread. Otherwise, it will block other
device's notify_vq() being called and starve other devices.

In virtio net's notify_vq(), we simply signal the tx/rx handle thread
and return.

This patch makes virtio blk's notify_vq() just notify the blk thread
instead of doing the real hard read/write work. Tests show that the
overhead of the notification operations introduced by this patch is
small.

Signed-off-by: Asias He <asias.hejun@gmail.com>
---
 tools/kvm/virtio/blk.c |   26 +++++++++++++++++++++++++-
 1 files changed, 25 insertions(+), 1 deletions(-)

diff --git a/tools/kvm/virtio/blk.c b/tools/kvm/virtio/blk.c
index d1a0197..9d2f37e 100644
--- a/tools/kvm/virtio/blk.c
+++ b/tools/kvm/virtio/blk.c
@@ -51,6 +51,11 @@ struct blk_dev {
 
 	struct virt_queue		vqs[NUM_VIRT_QUEUES];
 	struct blk_dev_req		reqs[VIRTIO_BLK_QUEUE_SIZE];
+
+	pthread_t			io_thread;
+	int				io_efd;
+
+	struct kvm			*kvm;
 };
 
 static LIST_HEAD(bdevs);
@@ -176,11 +181,26 @@ static int init_vq(struct kvm *kvm, void *dev, u32 vq, u32 pfn)
 	return 0;
 }
 
+static void *virtio_blk_thread(void *dev)
+{
+	struct blk_dev *bdev = dev;
+	u64 data;
+
+	while (1) {
+		read(bdev->io_efd, &data, sizeof(u64));
+		virtio_blk_do_io(bdev->kvm, &bdev->vqs[0], bdev);
+	}
+
+	pthread_exit(NULL);
+	return NULL;
+}
+
 static int notify_vq(struct kvm *kvm, void *dev, u32 vq)
 {
 	struct blk_dev *bdev = dev;
+	u64 data = 1;
 
-	virtio_blk_do_io(kvm, &bdev->vqs[vq], bdev);
+	write(bdev->io_efd, &data, sizeof(data));
 
 	return 0;
 }
@@ -228,6 +248,8 @@ void virtio_blk__init(struct kvm *kvm, struct disk_image *disk)
 			.capacity	= disk->size / SECTOR_SIZE,
 			.seg_max	= DISK_SEG_MAX,
 		},
+		.io_efd			= eventfd(0, 0),
+		.kvm			= kvm,
 	};
 
 	virtio_trans_init(&bdev->vtrans, VIRTIO_PCI);
@@ -244,6 +266,8 @@ void virtio_blk__init(struct kvm *kvm, struct disk_image *disk)
 
 	disk_image__set_callback(bdev->disk, virtio_blk_complete);
 
+	pthread_create(&bdev->io_thread, NULL, virtio_blk_thread, bdev);
+
 	if (compat_id != -1)
 		compat_id = compat__add_message("virtio-blk device was not detected",
 						"While you have requested a virtio-blk device, "
-- 
1.7.7.3

Re: [PATCH] kvm tools: Process virito blk requests in separate thread

[Sasha Levin]

On Tue, 2011-11-29 at 22:28 +0800, Asias He wrote:
> Currently, all blk requests are processed in notify_vq() which is in
> the context of ioeventfd thread: ioeventfd__thread(). The processing
> in notify_vq() may take a long time to complete.
> 
> We should make notify_vq() return as soon as possible, since all devices
> are sharing the single ioeventfd thread. Otherwise, it will block other
> device's notify_vq() being called and starve other devices.
> 
> In virtio net's notify_vq(), we simply signal the tx/rx handle thread
> and return.

Why not use the threadpool?

> 
> This patch makes virtio blk's notify_vq() just notify the blk thread
> instead of doing the real hard read/write work. Tests show that the
> overhead of the notification operations introduced by this patch is
> small.
> 
> Signed-off-by: Asias He <asias.hejun@gmail.com>
> ---

-- 

Sasha.

Re: [PATCH] kvm tools: Process virito blk requests in separate thread

[Asias He]

On 11/29/2011 10:36 PM, Sasha Levin wrote:
> On Tue, 2011-11-29 at 22:28 +0800, Asias He wrote:
>> Currently, all blk requests are processed in notify_vq() which is in
>> the context of ioeventfd thread: ioeventfd__thread(). The processing
>> in notify_vq() may take a long time to complete.
>>
>> We should make notify_vq() return as soon as possible, since all devices
>> are sharing the single ioeventfd thread. Otherwise, it will block other
>> device's notify_vq() being called and starve other devices.
>>
>> In virtio net's notify_vq(), we simply signal the tx/rx handle thread
>> and return.
> 
> Why not use the threadpool?

No.

1) In thread pool model, each job handling operation:
thread_pool__do_job() takes about 6 or 7 mutex_{lock,unlock} ops. Most
of the mutex are global (job_mutex) which are contented by the threads
in the pool. It's fine for the non performance critical virtio devices,
such as console, rng, etc. But it's not optimal for net and blk devices.

2) Using dedicated threads to handle blk requests opens the door for
user to set different IO priority for the blk threads.

3) It also reduces the contentions between net and blk devices if they
do not share the thead pool.

--->> the thread pool lock flow <<---

thread_pool__do_job() {

        mutex_lock(&jobinfo->mutex);
        if (jobinfo->signalcount++ == 0)
                thread_pool__job_push_locked(job) {
			        mutex_lock(&job_mutex);
			        thread_pool__job_push(job);
		        	mutex_unlock(&job_mutex);

		}
        mutex_unlock(&jobinfo->mutex);

        mutex_lock(&job_muex);
        pthread_cond_signal(&job_cond);
        mutex_unlock(&job_mutex);

}

thread_pool__threadfunc()
{

        for (;;) {
                mutex_lock(&job_mutex);
                pthread_cond_wait(&job_cond, &job_mutex);
                mutex_unlock(&job_mutex);

                if (curjob)
                        thread_pool__handle_job(curjob);
        }
}

thread_pool__handle_job()
{

        while (job) {
                job->callback(job->kvm, job->data);

                mutex_lock(&job->mutex);
                        thread_pool__job_push_locked(job);
                mutex_unlock(&job->mutex);

                job = thread_pool__job_pop_locked();
        }

}


-- 
Asias He

Re: [PATCH] kvm tools: Process virito blk requests in separate thread

[Pekka Enberg]

On Wed, 30 Nov 2011, Asias He wrote:
>>> In virtio net's notify_vq(), we simply signal the tx/rx handle thread
>>> and return.
>>
>> Why not use the threadpool?
>
> No.

Sasha?

Re: [PATCH] kvm tools: Process virito blk requests in separate thread

[Sasha Levin]

On Thu, 2011-12-01 at 10:48 +0200, Pekka Enberg wrote:
> On Wed, 30 Nov 2011, Asias He wrote:
> >>> In virtio net's notify_vq(), we simply signal the tx/rx handle thread
> >>> and return.
> >>
> >> Why not use the threadpool?
> >
> > No.
> 
> Sasha?

I was looking into the concept of adding 'dedicated' threads to the
threadpool, since when the threadpool was added originally one of the
purposes was to have all worker threads in a single place.

This way we could still have threads in one place, which would make it
easier to control all of them but still won't hurt performance of those
threads.

We can merge this patch, and I'll do the dedicated thread patch on top
of it.

-- 

Sasha.

Re: [PATCH] kvm tools: Process virito blk requests in separate thread

[Pekka Enberg]

On Thu, Dec 1, 2011 at 10:58 AM, Sasha Levin <levinsasha928@gmail.com> wrote:
> I was looking into the concept of adding 'dedicated' threads to the
> threadpool, since when the threadpool was added originally one of the
> purposes was to have all worker threads in a single place.
>
> This way we could still have threads in one place, which would make it
> easier to control all of them but still won't hurt performance of those
> threads.
>
> We can merge this patch, and I'll do the dedicated thread patch on top
> of it.

Nah. Lets add the new API and change this patch to use it instead.

Re: [PATCH] kvm tools: Process virito blk requests in separate thread

[Asias He]

On Thu, Jun 7, 2012 at 4:47 PM, Paolo Bonzini <pbonzini@redhat.com> wrote:
> Il 07/06/2012 03:38, Asias He ha scritto:
>>> > I never looked at the kvmtool code, but I think Asias has a point.  If
>>> > the guest submits I/O to lots of devices, they would contend on the
>>> > single thread.  There was a similar proof of concept patch for QEMU that
>>> > provided substantial benefit.
>>> >
>>> > However, it sounds a bit wasteful to have the dedicated thread run with
>>> > a second eventfd.  Would it be hard to just use the virtio-blk ioeventfd
>>> > directly?
>> Without this patch, we are already using the virtio-blk ioeventfd
>> which is being monitored in the shared ioeventfd__thread() thead.
>
> Yes, I understood that. :)

;-)

> I'm wondering why it is still necessary to
> monitor that eventfd in the shared thread.  Perhaps you could optionally
> skip the epoll registration/deregistration in ioeventfd__{add,del}_event.

Yes, we can  monitor the eventfd  only in block  thread.  But that
requires changing our virtio core code and doing ioeventfd setup in
block specific code. This still does not save a thread for us. And the
overhead of using shared eventfd + notify to block thead should be
small. So I'd prefer the current model.

-- 
Asias He

Re: [PATCH] kvm tools: Process virito blk requests in separate thread

[Paolo Bonzini]

Il 07/06/2012 03:38, Asias He ha scritto:
>> > I never looked at the kvmtool code, but I think Asias has a point.  If
>> > the guest submits I/O to lots of devices, they would contend on the
>> > single thread.  There was a similar proof of concept patch for QEMU that
>> > provided substantial benefit.
>> >
>> > However, it sounds a bit wasteful to have the dedicated thread run with
>> > a second eventfd.  Would it be hard to just use the virtio-blk ioeventfd
>> > directly?
> Without this patch, we are already using the virtio-blk ioeventfd
> which is being monitored in the shared ioeventfd__thread() thead.

Yes, I understood that. :)  I'm wondering why it is still necessary to
monitor that eventfd in the shared thread.  Perhaps you could optionally
skip the epoll registration/deregistration in ioeventfd__{add,del}_event.

Paolo

Re: [PATCH] kvm tools: Process virito blk requests in separate thread

[Asias He]

On Wed, Jun 6, 2012 at 8:35 PM, Paolo Bonzini <pbonzini@redhat.com> wrote:
> Il 04/06/2012 18:07, Sasha Levin ha scritto:
>>> > All blk requests are processed in notify_vq() which is in the context of
>>> > ioeventfd thread: ioeventfd__thread(). The processing in notify_vq() may
>>> > take a long time to complete and all devices share the single ioeventfd
>>> > thead, so this might block other device's notify_vq() being called and
>>> > starve other devices.
>> We're using native vectored AIO for for processing blk requests, so I'm
>> not certain if theres any point in giving the blk device it's own thread
>> for handling that.
>
> I never looked at the kvmtool code, but I think Asias has a point.  If
> the guest submits I/O to lots of devices, they would contend on the
> single thread.  There was a similar proof of concept patch for QEMU that
> provided substantial benefit.
>
> However, it sounds a bit wasteful to have the dedicated thread run with
> a second eventfd.  Would it be hard to just use the virtio-blk ioeventfd
> directly?

Without this patch, we are already using the virtio-blk ioeventfd
which is being monitored in the shared ioeventfd__thread() thead.
The eventfd in this patch is being used as a way to decouple the
processing work from the ioeventfd__thread() thead. It's a bit like
pthread_cond_signal()/pthread_cond_wait().

-- 
Asias He

Re: [PATCH] kvm tools: Process virito blk requests in separate thread

[Paolo Bonzini]

Il 04/06/2012 18:07, Sasha Levin ha scritto:
>> > All blk requests are processed in notify_vq() which is in the context of
>> > ioeventfd thread: ioeventfd__thread(). The processing in notify_vq() may
>> > take a long time to complete and all devices share the single ioeventfd
>> > thead, so this might block other device's notify_vq() being called and
>> > starve other devices.
> We're using native vectored AIO for for processing blk requests, so I'm
> not certain if theres any point in giving the blk device it's own thread
> for handling that.

I never looked at the kvmtool code, but I think Asias has a point.  If
the guest submits I/O to lots of devices, they would contend on the
single thread.  There was a similar proof of concept patch for QEMU that
provided substantial benefit.

However, it sounds a bit wasteful to have the dedicated thread run with
a second eventfd.  Would it be hard to just use the virtio-blk ioeventfd
directly?

Paolo

Re: [PATCH] kvm tools: Process virito blk requests in separate thread

[Pekka Enberg]

On Mon, Jun 4, 2012 at 6:40 PM, Asias He <asias.hejun@gmail.com> wrote:
> All blk requests are processed in notify_vq() which is in the context of
> ioeventfd thread: ioeventfd__thread(). The processing in notify_vq() may
> take a long time to complete and all devices share the single ioeventfd
> thead, so this might block other device's notify_vq() being called and
> starve other devices.
>
> This patch makes virtio blk's notify_vq() just notify the blk thread
> instead of doing the real hard read/write work. Tests show that the
> overhead of the notification operations is small.
>
> The reasons for using dedicated thead instead of using thead pool
> follow:
>
> 1) In thread pool model, each job handling operation:
> thread_pool__do_job() takes about 6 or 7 mutex_{lock,unlock} ops. Most
> of the mutex are global (job_mutex) which are contented by the threads
> in the pool. It's fine for the non performance critical virtio devices,
> such as console, rng, etc. But it's not optimal for net and blk devices.
>
> 2) Using dedicated threads to handle blk requests opens the door for
> user to set different IO priority for the blk threads.
>
> 3) It also reduces the contentions between net and blk devices if they
> do not share the thead pool.
>
> Signed-off-by: Asias He <asias.hejun@gmail.com>

You wouldn't happen to have any performance numbers for this? ;-)

Re: [PATCH] kvm tools: Process virito blk requests in separate thread

[Pekka Enberg]

On Tue, Jun 5, 2012 at 4:03 AM, Asias He <asias.hejun@gmail.com> wrote:
> On Tue, Jun 5, 2012 at 12:07 AM, Sasha Levin <levinsasha928@gmail.com> wrote:
>> On Mon, 2012-06-04 at 23:40 +0800, Asias He wrote:
>>> All blk requests are processed in notify_vq() which is in the context of
>>> ioeventfd thread: ioeventfd__thread(). The processing in notify_vq() may
>>> take a long time to complete and all devices share the single ioeventfd
>>> thead, so this might block other device's notify_vq() being called and
>>> starve other devices.
>>
>> We're using native vectored AIO for for processing blk requests, so I'm
>> not certain if theres any point in giving the blk device it's own thread
>> for handling that.
>
> We discussed this last year. Search the same subject. Pekka suggested
> improving the thead pool API to support dedicated thead support. I'd
> prefer to merge this patch for now since that support is till not
> there.
>
> Recently, I added some debug code to see how many loops
> virtio_blk_do_io() will do until it finishes the processing.
> I am seeing something like this:
>
>   Info: virtio_blk_do_io max_nr_loop=8427
>
> The processing takes a very long time.

Ingo, any comments on the thread-per-blk-device model?

Re: [PATCH] kvm tools: Process virito blk requests in separate thread

[Cyrill Gorcunov]

On Tue, Jun 05, 2012 at 08:47:17AM +0800, Asias He wrote:
> > I must admit I don't understand this code ;) The data get read into
> > stack variable forever?
> 
> The data we read itself is not interesting at all. virtio_blk_thread()
> sleeps on the eventfd bdev->io_efd until notify_vq() writes something
> to wake it up.

Ah, thanks for explanation, Asias.

	Cyrill

Re: [PATCH] kvm tools: Process virito blk requests in separate thread

[Asias He]

On Tue, Jun 5, 2012 at 12:07 AM, Sasha Levin <levinsasha928@gmail.com> wrote:
> On Mon, 2012-06-04 at 23:40 +0800, Asias He wrote:
>> All blk requests are processed in notify_vq() which is in the context of
>> ioeventfd thread: ioeventfd__thread(). The processing in notify_vq() may
>> take a long time to complete and all devices share the single ioeventfd
>> thead, so this might block other device's notify_vq() being called and
>> starve other devices.
>
> We're using native vectored AIO for for processing blk requests, so I'm
> not certain if theres any point in giving the blk device it's own thread
> for handling that.

We discussed this last year. Search the same subject. Pekka suggested
improving the thead pool API to support dedicated thead support. I'd
prefer to merge this patch for now since that support is till not
there.

Recently, I added some debug code to see how many loops
virtio_blk_do_io() will do until it finishes the processing.
I am seeing something like this:

   Info: virtio_blk_do_io max_nr_loop=8427

The processing takes a very long time.

-- 
Asias He

Re: [PATCH] kvm tools: Process virito blk requests in separate thread

[Asias He]

On Mon, Jun 4, 2012 at 11:48 PM, Cyrill Gorcunov <gorcunov@openvz.org> wrote:
> On Mon, Jun 04, 2012 at 11:40:53PM +0800, Asias He wrote:
>>
>> +static void *virtio_blk_thread(void *dev)
>> +{
>> +     struct blk_dev *bdev = dev;
>> +     u64 data;
>> +
>> +     while (1) {
>> +             read(bdev->io_efd, &data, sizeof(u64));
>> +             virtio_blk_do_io(bdev->kvm, &bdev->vqs[0], bdev);
>> +     }
>> +
>> +     pthread_exit(NULL);
>> +     return NULL;
>> +}
>
> I must admit I don't understand this code ;) The data get read into
> stack variable forever?

The data we read itself is not interesting at all. virtio_blk_thread()
sleeps on the eventfd bdev->io_efd until notify_vq() writes something
to wake it up.

-- 
Asias He

Re: [PATCH] kvm tools: Process virito blk requests in separate thread

[Sasha Levin]

On Mon, 2012-06-04 at 23:40 +0800, Asias He wrote:
> All blk requests are processed in notify_vq() which is in the context of
> ioeventfd thread: ioeventfd__thread(). The processing in notify_vq() may
> take a long time to complete and all devices share the single ioeventfd
> thead, so this might block other device's notify_vq() being called and
> starve other devices.

We're using native vectored AIO for for processing blk requests, so I'm
not certain if theres any point in giving the blk device it's own thread
for handling that.

Re: [PATCH] kvm tools: Process virito blk requests in separate thread

[Cyrill Gorcunov]

On Mon, Jun 04, 2012 at 11:40:53PM +0800, Asias He wrote:
>  
> +static void *virtio_blk_thread(void *dev)
> +{
> +	struct blk_dev *bdev = dev;
> +	u64 data;
> +
> +	while (1) {
> +		read(bdev->io_efd, &data, sizeof(u64));
> +		virtio_blk_do_io(bdev->kvm, &bdev->vqs[0], bdev);
> +	}
> +
> +	pthread_exit(NULL);
> +	return NULL;
> +}

I must admit I don't understand this code ;) The data get read into
stack variable forever?

	Cyrill

[PATCH] kvm tools: Process virito blk requests in separate thread

[Asias He]

All blk requests are processed in notify_vq() which is in the context of
ioeventfd thread: ioeventfd__thread(). The processing in notify_vq() may
take a long time to complete and all devices share the single ioeventfd
thead, so this might block other device's notify_vq() being called and
starve other devices.

This patch makes virtio blk's notify_vq() just notify the blk thread
instead of doing the real hard read/write work. Tests show that the
overhead of the notification operations is small.

The reasons for using dedicated thead instead of using thead pool
follow:

1) In thread pool model, each job handling operation:
thread_pool__do_job() takes about 6 or 7 mutex_{lock,unlock} ops. Most
of the mutex are global (job_mutex) which are contented by the threads
in the pool. It's fine for the non performance critical virtio devices,
such as console, rng, etc. But it's not optimal for net and blk devices.

2) Using dedicated threads to handle blk requests opens the door for
user to set different IO priority for the blk threads.

3) It also reduces the contentions between net and blk devices if they
do not share the thead pool.

Signed-off-by: Asias He <asias.hejun@gmail.com>
---
 tools/kvm/virtio/blk.c |   26 +++++++++++++++++++++++++-
 1 file changed, 25 insertions(+), 1 deletion(-)

diff --git a/tools/kvm/virtio/blk.c b/tools/kvm/virtio/blk.c
index da92094..e0dc37d 100644
--- a/tools/kvm/virtio/blk.c
+++ b/tools/kvm/virtio/blk.c
@@ -49,6 +49,11 @@ struct blk_dev {
 
 	struct virt_queue		vqs[NUM_VIRT_QUEUES];
 	struct blk_dev_req		reqs[VIRTIO_BLK_QUEUE_SIZE];
+
+	pthread_t			io_thread;
+	int				io_efd;
+
+	struct kvm			*kvm;
 };
 
 static LIST_HEAD(bdevs);
@@ -174,11 +179,26 @@ static int init_vq(struct kvm *kvm, void *dev, u32 vq, u32 pfn)
 	return 0;
 }
 
+static void *virtio_blk_thread(void *dev)
+{
+	struct blk_dev *bdev = dev;
+	u64 data;
+
+	while (1) {
+		read(bdev->io_efd, &data, sizeof(u64));
+		virtio_blk_do_io(bdev->kvm, &bdev->vqs[0], bdev);
+	}
+
+	pthread_exit(NULL);
+	return NULL;
+}
+
 static int notify_vq(struct kvm *kvm, void *dev, u32 vq)
 {
 	struct blk_dev *bdev = dev;
+	u64 data = 1;
 
-	virtio_blk_do_io(kvm, &bdev->vqs[vq], bdev);
+	write(bdev->io_efd, &data, sizeof(data));
 
 	return 0;
 }
@@ -233,6 +253,8 @@ static int virtio_blk__init_one(struct kvm *kvm, struct disk_image *disk)
 			.capacity	= disk->size / SECTOR_SIZE,
 			.seg_max	= DISK_SEG_MAX,
 		},
+		.io_efd			= eventfd(0, 0),
+		.kvm			= kvm,
 	};
 
 	virtio_init(kvm, bdev, &bdev->vdev, &blk_dev_virtio_ops,
@@ -247,6 +269,8 @@ static int virtio_blk__init_one(struct kvm *kvm, struct disk_image *disk)
 
 	disk_image__set_callback(bdev->disk, virtio_blk_complete);
 
+	pthread_create(&bdev->io_thread, NULL, virtio_blk_thread, bdev);
+
 	if (compat_id != -1)
 		compat_id = compat__add_message("virtio-blk device was not detected",
 						"While you have requested a virtio-blk device, "
-- 
1.7.10.2