fix(virtq): only enable_notification when about to stop consumption

[ihciah]

Problem

Current usage for virtqueue is like:

while let Some(desc_chain) = q.pop_descriptor_chain(mem) {
    // handle desc_chain
    if !queue
        .enable_notification(mem)
        .map_err(Error::QueueEnableNotification)?
    {
        break;
    }
}

However, it will bring not only more guest memory write and fence cost, but also more guest kick count.

According to virtio-queue's doc of enable_notification, it advises:

  // With the current implementation, a common way of consuming entries from the available ring
  // while also leveraging notification suppression is to use a loop, for example:
  //
  // loop {
  //     // We have to explicitly disable notifications if `VIRTIO_F_EVENT_IDX` has not been
  //     // negotiated.
  //     self.disable_notification()?;
  //
  //     for chain in self.iter()? {
  //         // Do something with each chain ...
  //         // Let's assume we process all available chains here.
  //     }
  //
  //     // If `enable_notification` returns `true`, the driver has added more entries to the
  //     // available ring.
  //     if !self.enable_notification()? {
  //         break;
  //     }
  // }

The enable_notification needs to be called if and only if we are about to stop consuming the queue.

Solution

I tried to change the code as the style suggested above, but it makes code confusing because there are many exit branches in complicated functions.

However, the caller must consume the queue until empty or it guarentees there would be another event source triggers the consumption like timer fd. So it's a good idea to add a function like pop_desc_chain_with_notification, when it returns None, it means the notification is enabled. And the caller won't have to worry about when and whether to enable_notification.

Implementation

The Queue is not defined by us. So the only way to extend it is to adding a extension trait and implement it.

I didn't find a proper crate to put it in, so I added a new crate called virtio-ext and put the trait and impl in, and replaced nearly all pop_descriptor_chain with enable_notification to pop_desc_chain_with_notification.

Another Fix(maybe?) about block.rs

During the replacing, I found the enable_notification at L395 of block.rs is redundant because it does not consume any descriptor. And when handling COMPLETION_EVENT, if the rate_limiter not pass, there would be no notification to guest(for completions that already pushed into the queue). Since the code is related and small, I fixed(maybe?) it in the same commit. I can split it into another PR if needed.

I'm not familiar with the block device, if I'm wrong please point it out.

[rbradford]

Thanks for investigating this - why not open an PR against https://github.com/rust-vmm/vm-virtio/ with your new API and then that can be consumed here.

[ihciah]

Thanks for advise. I'll try to put it in rust-vmm recently.

[elainewu1222]

During the replacing, I found the enable_notification at L395 of block.rs is redundant because it does not consume any descriptor.

hi @ihciah, the intention for enable_notification in process_queue_complete is by design. For aio path, the IO request is consumed in process_queue_submit, but it's actually async-handled by kernel. When kernel done, it notifies the backend in process_queue_complete.
In such case, if enable_notification is triggered in process_queue_submit, frontend is very less likely to benefit from the event idx. That's because frontend will see immediate update in avail_event and it will choose to notify every time.
So instead we trigger it in process_queue_complete, when IO is really done. Also inside COMPLETION_EVENT handler the backend will call process_queue_submit in the end to construct a handling loop.

And when handling COMPLETION_EVENT, if the rate_limiter not pass, there would be no notification to guest(for completions that already pushed into the queue)

This seems a real issue, thanks for pointing out.

[liuw]

During the replacing, I found the enable_notification at L395 of block.rs is redundant because it does not consume any descriptor.

hi @ihciah, the intention for enable_notification in process_queue_complete is by design. For aio path, the IO request is consumed in process_queue_submit, but it's actually async-handled by kernel. When kernel done, it notifies the backend in process_queue_complete. In such case, if enable_notification is triggered in process_queue_submit, frontend is very less likely to benefit from the event idx. That's because frontend will see immediate update in avail_event and it will choose to notify every time. So instead we trigger it in process_queue_complete, when IO is really done. Also inside COMPLETION_EVENT handler the backend will call process_queue_submit in the end to construct a handling loop.

And when handling COMPLETION_EVENT, if the rate_limiter not pass, there would be no notification to guest(for completions that already pushed into the queue)

This seems a real issue, thanks for pointing out.

Are you going to propose a fix?

[elainewu1222]

@ihciah @liuw opened a pr #6672 for fixing virtio-blk COMPLETION_EVENT handling.

[rbradford]

What's the status of this PR - it doesn't apply now. If this is still an issue please open an issue so we can track.