[Date Prev] | [Thread Prev] | [Thread Next] | [Date Next] -- [Date Index] | [Thread Index] | [List Home]
Subject: Re: [PATCH v8 08/16] packed virtqueues: more efficient virtqueue layout
On Sat, Feb 24, 2018 at 01:17:08PM +0800, Tiwei Bie wrote:
> On Fri, Feb 16, 2018 at 09:24:12AM +0200, Michael S. Tsirkin wrote:
> [...]
> > +\subsection{Scatter-Gather Support}
> > +\label{sec:Packed Virtqueues / Scatter-Gather Support}
> > +
> > +Some drivers need an ability to supply a list of multiple buffer
> > +elements (also known as a scatter/gather list) with a request.
> > +Two optional features support this: descriptor
> > +chaining and indirect descriptors.
> > +
> > +If neither feature has been negotiated, each buffer is
> > +physically-contigious, either read-only or write-only and is
> > +described completely by a single descriptor.
>
> Based on the updates in "Descriptor Chaining" section
> and the below note in cover letter:
>
> - dropped _F_DESC_LIST, 1.0 includes this unconditionally, we
> can do same
>
> Descriptor chaining is always available.
That's right - I should fix this.
> > +
> [...]
> > +\subsection{Event Suppression Structure Format}\label{sec:Basic
> > +Facilities of a Virtio Device / Packed Virtqueues / Event Suppression Structure
> > +Format}
> > +
> > +The following structure is used to reduce the number of
> > +notifications sent between driver and device.
> > +
> > +\begin{lstlisting}
> > +__le16 desc_event_off : 15; /* Descriptor Event Offset */
> > +int desc_event_wrap : 1; /* Descriptor Event Wrap Counter */
>
> Is this `int` a typo?
It's a single bit so I think it does not matter.
What type would you like me to use instead?
> > +__le16 desc_event_flags : 2; /* Descriptor Event Flags */
> > +\end{lstlisting}
> [...]
> > +\subsubsection{Implementation Example}\label{sec:Basic Facilities of a Virtio Device / Packed Virtqueues / Supplying Buffers to The Device / Implementation Example}
> > +
> > +Below is an example driver code. It does not attempt to reduce
> > +the number of device interrupts, neither does it support
> > +the VIRTIO_F_RING_EVENT_IDX feature.
> > +
> > +\begin{lstlisting}
> > +
> > +first = vq->next_avail;
> > +id = alloc_id(vq);
> > +
> > +for (each buffer element b) {
> > + vq->desc[vq->next_avail].address = get_addr(b);
> > + vq->desc[vq->next_avail].len = get_len(b);
> > + init_desc(vq->next_avail, b);
> > + avail = vq->avail_wrap_count;
> > + used = !vq->avail_wrap_count;
> > + f = get_flags(b) | (avail << VIRTQ_DESC_F_AVAIL) | (used << VIRTQ_DESC_F_USED);
>
> In this version, above two flags are defined as:
>
> #define VIRTQ_DESC_F_AVAIL (1 << 7)
> #define VIRTQ_DESC_F_USED (1 << 15)
>
> So we couldn't use them to shift the bit like this.
You are right. I guess we should change all flags to be bit numbers.
> > + /* Don't mark the 1st descriptor available until all of them are ready. */
> > + if (vq->next_avail == first) {
> > + flags = f;
> > + } else {
> > + vq->desc[vq->next_avail].flags = f;
> > + }
> > +
> > + vq->next_avail++;
> > +
> > + if (vq->next_avail >= vq->size) {
> > + vq->next_avail = 0;
> > + vq->avail_wrap_count \^= 1;
> > + }
>
> Maybe it's better to not mix using tab and space in
> this example code.
I'm not sure it matters for latex but I agree we should be
consistent.
> > +
> > +
> > +}
> > +/* ID included in the last descriptor in the list */
> > +vq->desc[vq->next_avail].id = id;
>
> Maybe it should be something like this:
>
> vq->desc[prev].id = id;
>
> Otherwise, the other fields (e.g. flags) of this desc (the
> vq->desc[vq->next_avail]) also need to be updated.
>
> > +write_memory_barrier();
> > +vq->desc[first].flags = flags;
> > +
> > +memory_barrier();
> > +
> > +if (vq->device_event.flags != 0x2) {
>
> Maybe it should be:
>
> if (vq->device_event.flags != 0x1) {
>
> As the flags definitions in this version are:
>
> 00b enable events
> 01b disable events
> 10b enable events for a specific descriptor
> (as specified by Descriptor Ring Change Event Offset/Wrap Counter).
> Only valid if VIRTIO_F_RING_EVENT_IDX has been negotiated.
> 11b reserved
I'll recheck it and reply to above two separately.
> > + notify_device(vq);
> > +}
> > +
> > +\end{lstlisting}
> > +
> > +
> > +\drivernormative{\paragraph}{Notifying The Device}{Basic Facilities of a Virtio Device / Packed Virtqueues / Supplying Buffers to The Device / Notifying The Device}
> > +The driver MUST perform a suitable memory barrier before reading
> > +the Driver Event Suppression structure, to avoid missing a notification.
> > +
> > +\subsection{Receiving Used Buffers From The Device}\label{sec:Basic Facilities of a Virtio Device / Packed Virtqueues / Receiving Used Buffers From The Device}
> > +
> > +Once the device has used buffers referred to by a descriptor (read from or written to them, or
> > +parts of both, depending on the nature of the virtqueue and the
> > +device), it interrupts the driver
> > +as detailed in section \ref{sec:Basic
> > +Facilities of a Virtio Device / Packed Virtqueues / Event
> > +Suppression Structure Format}.
> > +
> > +\begin{note}
> > +For optimal performance, a driver MAY disable interrupts while processing
> > +the used buffers, but beware the problem of missing interrupts between
> > +emptying the ring and reenabling interrupts. This is usually handled by
> > +re-checking for more used buffers after interrups are re-enabled:
> > +\end{note}
> > +
> > +\begin{lstlisting}
> > +vq->driver_event.flags = 0x2;
>
> If my understanding is correct, this is to disable interrupt.
> So maybe it should be:
>
> vq->driver_event.flags = 0x1;
I'll check.
> > +
> > +for (;;) {
> > + struct virtq_desc *d = vq->desc[vq->next_used];
> > +
> > + flags = d->flags;
> > + bool avail = flags & (1 << VIRTQ_DESC_F_AVAIL);
> > + bool used = flags & (1 << VIRTQ_DESC_F_USED);
>
> We couldn't use them to shift the bit like this in this version.
>
> > +
> > + if (avail != used) {
> > + vq->driver_event.flags = 0x1;
>
> If my understanding is correct, this is to enable interrupt.
> So maybe it should be:
>
> vq->driver_event.flags = 0x0;
>
> > + memory_barrier();
> > +
> > + flags = d->flags;
> > + bool avail = flags & (1 << VIRTQ_DESC_F_AVAIL);
> > + bool used = flags & (1 << VIRTQ_DESC_F_USED);
>
> We couldn't use them to shift the bit like this in this version.
>
> > + if (avail != used) {
> > + break;
> > + }
> > +
> > + vq->driver_event.flags = 0x2;
>
> If my understanding is correct, this is to disable interrupt.
> So maybe it should be:
>
> vq->driver_event.flags = 0x1;
Thanks for all the comments, will address.
> > + }
> > +
> > + read_memory_barrier();
> > + process_buffer(d);
> > + vq->next_used++;
> > + if (vq->next_used >= vq->size) {
> > + vq->next_used = 0;
> > + }
> > +}
> > +\end{lstlisting}
> > --
> > MST
> >
[Date Prev] | [Thread Prev] | [Thread Next] | [Date Next] -- [Date Index] | [Thread Index] | [List Home]