[Date Prev] | [Thread Prev] | [Thread Next] | [Date Next] -- [Date Index] | [Thread Index] | [List Home]
Subject: Re: [virtio] RE: Re: [virtio-dev] Device-to-driver notification management for hardware implementations
On Wed, Jan 23, 2019 at 07:21:05AM +0000, Chandra Thyamagondlu wrote:
> Hi David,
>
> It looks like a good idea, we are trying to implement packed ring in hardware
> and we are finding the driver event suppression to be very hard to implement.
> The big issue for hardware implementation is having to read the event
> suppression structure over PCIe to decide whether we can generate the interrupt
> or not. As you point out the current mechanism has the race condition of
> generating the interrupt as we donât have any feedback event if the ISR has
> finished processing the packets in other words if driver is enabling the
> interrupt.
Just to clarify: if driver re-checks the ring
after enabling interrupts, I don't think there's
a race condition.
> I think your proposal of write to p_int_en_doorbell will work for us. The write
> event to this new address in BAR itself implicitly tell the device that
> interrupt is enabled and also it conveys the last used entry that it processed.
> Then device can provide nice interrupt moderation by making sure at least one
> new used entry was written and some time has elapsed from previous interrupt to
> driver.
>
> It would also be helpful if you can also outline the new structure and your
> proposal for offset in the BAR.
>
>
>
> Chandra
So this boils down to adding ability to notify devices about enabling
interrupts. Question would be, this slows down the driver. Is this
still worth doing? Right now we are trying to offload as
much as we can to the device.
>
>
>
>
> Subject: Re: [virtio-dev] Device-to-driver notification management for hardware
> implementations
>
> Date: Thu, 8 Nov 2018 12:16:28 +0000
>
> From: David Riddoch <driddoch@solarflare.com>
>
> To: Stefan Hajnoczi <stefanha@redhat.com>
>
> CC: Virtio-Dev <virtio-dev@lists.oasis-open.org>, Steve Pope
> <spope@solarflare.com>
>
>
>
>
> On 05/11/18 08:53, Stefan Hajnoczi wrote:
>
> > On Wed, Sep 12, 2018 at 05:41:07PM +0100, David Riddoch wrote:
>
> >> The VIRTIO_F_NOTIFICATION_DATA feature helps hardware implementations by
>
> >> telling them directly how many descriptors are available in the ring, saving
>
> >> speculative reads. There is a related issue with interrupts: As things
>
> >> stand, after writing 'used' descriptors into the ring (packed or split) the
>
> >> device has to read the event idx to determine whether a device-to-driver
>
> >> notification (interrupt) is needed.
>
> >>
>
> >> It would be very helpful to a hardware implementation if the event idx could
>
> >> be written directly to the device. Has anyone proposed an extension to
>
> >> address this already? (I have some thoughts on how to do this if we don't
>
> >> have a proposal yet).
>
> > Hi David,
>
> > Has there been any further discussion on this topic? It seems like a
>
> > useful feature to have in VIRTIO.
>
> Hi Stefan,
>
>
>
> Thanks for the nudge, no further discussion. So here are my thoughts:
>
>
>
> To provide context here is an outline of how drivers typically process
>
> completions from a device. This applies to both para-virtualised and
>
> real hardware. (I think we only need to worry about packed-ring).
>
>
>
> disable_cb();
>
> again:
>
> while( budget_left && desc_is_used() )
>
> handle_used();
>
> if( budget_left &&enable_cb() )
>
> goto again;
>
>
>
> The budget stuff relates to napi polling, and isn't always there. But if
>
> it is, and budget is exhausted, then an outer loop will invoke this code
>
> again and so we don't want an interrupt.
>
>
>
> In our current packed-ring design enable_cb() does roughly this:
>
>
>
> enable_cb_packed() {
>
> guest_mem->event_suppress = last_used_idx;
>
> virtio_mb();
>
> return desc_is_used();
>
> }
>
>
>
> This tells the device to raise an interrupt when moving used_idx over
>
> the given idx. (And you can set that idx to a position further in the
>
> ring if you're not interested in getting woken until a bunch of
>
> descriptors have been consumed).
>
>
>
> This design is unpleasant for hardware devices, because it forces them
>
> to issue a read over the PCIe bus after writing used descriptors, in
>
> order to determine whether an interrupt is needed. Such a read adds
>
> complexity, adds overhead to the PCIe bus and memory system, and adds
>
> latency.
>
>
>
> In hardware devices interrupts are typically enabled like this:
>
>
>
> enable_cb_hw() {
>
> writel(last_used_idx, &dev_bar->int_en_doorbell);
>
> return 0;
>
> }
>
>
>
> This tells the device to raise an interrupt once the descriptor at the
>
> given index has been used. If it has already been used, then an
>
> interrupt is raised immediately, else the interrupt is enabled and
>
> raised after the used descriptor has been written. The race condition
>
> is handled without having to check the ring again. Note that interrupts
>
> are fire-once: For each write to int_en_doorbell you get a single
>
> interrupt, and disable_cb() is therefore a no-op.
>
>
>
> When you emulate a device using this model, you get a vmexit every time
>
> int_en_doorbell is written, which is expensive. Therefore this approach
>
> is not desirable when emulated.
>
>
>
> So perhaps we should introduce an option to use this hardware-oriented
>
> model. The problem is that with vDPA the implementation of a device can
>
> shift under the feet of a driver between hardware and emulation. We'd
>
> like to get the best performance we can in both cases.
>
>
>
> My proposal is that we add an option to use both models at the same
>
> time. enable_cb() would become:
>
>
>
> 30 enable_cb_new() {
>
> 31 if( int_en_doorbell_enabled )
>
> 32 writel(last_used_idx, p_int_en_doorbell);
>
> 33 guest_mem->event_suppress = last_used_idx;
>
> 34 virtio_mb();
>
> 35 return desc_is_used();
>
> 36 }
>
>
>
> p_int_en_doorbell it notionally a pointer into a memory mapping onto a
>
> device BAR, and when backed by hardware that is exactly what it is (and
>
> the information in the event suppression structure is ignored). But
>
> when backed by an emulated device the p_int_en_doorbell mapping should
>
> just point at normal host memory (which is otherwise unused). This
>
> means that we avoid vmexits in the emulated case.
>
>
>
> Comments?
>
>
>
> David
>
>
>
> --
>
> David Riddoch <driddoch@solarflare.com> -- Chief Architect, Solarflare
>
>
>
> The information contained in this message is confidential and is intended for
> the addressee(s) only. If you have received this message in error, please
> notify the sender immediately and delete the message. Unless you are an
> addressee (or authorized to receive for an addressee), you may not use, copy or
> disclose to anyone this message or any information contained in this message.
> The unauthorized use, disclosure, copying or alteration of this message is
> strictly prohibited.
>
[Date Prev] | [Thread Prev] | [Thread Next] | [Date Next] -- [Date Index] | [Thread Index] | [List Home]