Messages in this thread | | | Date | Thu, 25 Jul 2019 10:33:25 -0400 | From | "Michael S. Tsirkin" <> | Subject | Re: [External Email] Re: [PATCH 1/2] virtio-mmio: Process vrings more proactively |
| |
On Mon, Jul 22, 2019 at 10:28:30AM +0800, Fam Zheng wrote: > > On 7/19/19 11:17 PM, Michael S. Tsirkin wrote: > > On Fri, Jul 19, 2019 at 09:31:34PM +0800, Fei Li wrote: > > > From: Fam Zheng <zhengfeiran@bytedance.com> > > > > > > This allows the backend to _not_ trap mmio read of the status register > > > after injecting IRQ in the data path, which can improve the performance > > > significantly by avoiding a vmexit for each interrupt. > > > > > > More importantly it also makes it possible for Firecracker to hook up > > > virtio-mmio with vhost-net, in which case there isn't a way to implement > > > proper status register handling. > > > > > > For a complete backend that does set either INT_CONFIG bit or INT_VRING > > > bit upon generating irq, what happens hasn't changed. > > > > > > Signed-off-by: Fam Zheng <zhengfeiran@bytedance.com> > > This has a side effect of skipping vring callbacks > > if they trigger at the same time with a config > > interrupt. > > I don't see why this is safe. > > Good point! I think the block can be moved out from the else block and run > unconditionally then. > > Fam
Won't same callback run from multiple irq handlers then? Running multiple vring callbacks at the same time isn't a good idea either.
> > > > > > > > --- > > > drivers/virtio/virtio_mmio.c | 4 +--- > > > 1 file changed, 1 insertion(+), 3 deletions(-) > > > > > > diff --git a/drivers/virtio/virtio_mmio.c b/drivers/virtio/virtio_mmio.c > > > index e09edb5c5e06..9b42502b2204 100644 > > > --- a/drivers/virtio/virtio_mmio.c > > > +++ b/drivers/virtio/virtio_mmio.c > > > @@ -295,9 +295,7 @@ static irqreturn_t vm_interrupt(int irq, void *opaque) > > > if (unlikely(status & VIRTIO_MMIO_INT_CONFIG)) { > > > virtio_config_changed(&vm_dev->vdev); > > > ret = IRQ_HANDLED; > > > - } > > > - > > > - if (likely(status & VIRTIO_MMIO_INT_VRING)) { > > > + } else { > > > spin_lock_irqsave(&vm_dev->lock, flags); > > > list_for_each_entry(info, &vm_dev->virtqueues, node) > > > ret |= vring_interrupt(irq, info->vq); > > > -- > > > 2.11.0
| |