RE: [virtio-comment] [RFC PATCH] VIRTIO_F_USED_EVENT_AUTO_DISABLE: add new used buffer notification suppression mechanism

[Lege Wang <lege.wang@jaguarmicro.com>]

hi,

I'd like to have some more discussions about used buffer notification mechanism, thanks.
> > +\item [next_wrap] Wrap Counter.
> > +      With VIRTIO_F_RING_PACKED this is the wrap counter
> > +      referring to the next used descriptor.
> > +      Without VIRTIO_F_RING_PACKED this is the most significant bit
> > +      (bit 15) of the used index.
> > +\end{description}
> > +
> This forces the driver to process _all_ used buffer notifications before enabling
> them again.
> There is a race condition that used ring updates are occurring from the device
> (without notification) and driver updates the value.
> For example,
> 1. driver posted descriptors from index 0 to 9.
> 2. Used buffer notification arrived for 0 to 3.
> 3. Driver processes used buffer ring from 0 to 3 and enables used buffer
> notification for index 4.
> 4. While #3 is happening, the device already updated the used buffer entries.
> 5. So device view of used ring entry is messed up as it already passed that point.
> 
> Hence a better driver used buffer notification should be,
> 
> 1. index upto which it has processed
I would change it to " index upto which it will process next", the reason is that
virtio core driver codes have already a well-maintained "struct vring_virtqueue->last_used_idx",
we can use it for notification, and using this value, the device will also be able to learn
the driver consumption rate.

> 2. explicit enable/disable bit to enable the notification
I wonder if there's a case that the driver just notifies the used index info, but not enable
the device notification, the driver will always need one enable operation finally. And the notification
data would be similar to:
le32 { 
  union { 
    vq_index: 16;           /* Used if VIRTIO_F_NOTIF_CONFIG_DATA not negotiated */ 
    vq_notif_config_data: 16; /* Used if VIRTIO_F_NOTIF_CONFIG_DATA negotiated */ 
  }; 
  next_off : 15; 
  next_wrap : 1; 
};
next_off and next_wrap will denote last_used_idx info, and vq_index or vq_notif_config_data is
also necessary, so for one 32-bit register, there's no room for this enable/disable bit.


Regards,
lege.wang
> 
> These two fields clearly communicate to the device, upto how much used buffer
> is consumed and understand the driver consumption rate.
> This way, there is no ambiguity for device on where to write next used ring
> entry.
> (where to write next used entry is not driven by the notification enablement).
> 
> And the explicit enable bit indicates notifications are enabled back.
> 
> Technically, only enable bit is needed, but doing #1 is helpful for the device to
> understand driver consumption pace.
> And for that may be current notification can be made 64-bits or as suggested in
> your patch to have new location.
>