[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH v1 2/2] virtio-scsi: dataplane: notify guest as
From: |
Ming Lei |
Subject: |
Re: [Qemu-devel] [PATCH v1 2/2] virtio-scsi: dataplane: notify guest as batch |
Date: |
Wed, 12 Nov 2014 10:33:08 +0800 |
On Wed, Nov 12, 2014 at 1:28 AM, Stefan Hajnoczi <address@hidden> wrote:
> On Tue, Nov 11, 2014 at 09:17:10AM +0800, Ming Lei wrote:
>> +static void notify_guest_bh(void *opaque)
>> +{
>> + VirtIOSCSI *s = opaque;
>> + unsigned int qid;
>> + uint64_t pending = s->pending_guest_notify;
>> +
>> + s->pending_guest_notify = 0;
>> +
>> + while ((qid = ffsl(pending))) {
>> + qid--;
>> + event_notifier_set(&s->cmd_vrings[qid]->guest_notifier);
>> + pending &= ~(1 << qid);
>> + }
>> +}
>
> Looks like we're not honoring virtio's usual interrupt mitigation
> mechanism (e.g. EVENT_IDX) for virtio-scsi. Why is
> vring_should_notify() not used?
Yes, that should be used, but in virtio-blk, BH still can suppress
about ~13K/sec write(), and for virtio-scsi, it should be more since
there may be more queues.
>
>> +
>> static void virtio_scsi_bad_req(void)
>> {
>> error_report("wrong size for virtio-scsi headers");
>> @@ -824,7 +839,12 @@ void virtio_scsi_common_realize(DeviceState *dev, Error
>> **errp,
>> }
>>
>> if (s->conf.iothread) {
>> - virtio_scsi_set_iothread(VIRTIO_SCSI(s), s->conf.iothread);
>> + VirtIOSCSI *vis = VIRTIO_SCSI(s);
>> +
>> + QEMU_BUILD_BUG_ON(VIRTIO_PCI_QUEUE_MAX > 64);
>> + virtio_scsi_set_iothread(vis, s->conf.iothread);
>> + vis->pending_guest_notify = 0;
>> + vis->guest_notify_bh = aio_bh_new(vis->ctx, notify_guest_bh, vis);
>
> Have you checked state transitions like PCI or virtio reset? They need
> to cancel the BH and clear ->pending_guest_notify.
>
> There is also a transition from dataplane to non-dataplane mode for live
> migration.
>
> Please make sure no interrupts are dropped or stale data is kept across
> these transitions.
Good catch, and I will make sure that in next version, and virtio-blk
dataplane need to consider that too.
Thanks,