[virtio-comment] Re: [PATCH v12 03/10] content: Rename confusing queue_notify_data and vqn names

["Michael S. Tsirkin" <mst@redhat.com>]

On Wed, Apr 05, 2023 at 01:20:04PM +0000, Parav Pandit wrote:
> 
> 
> > From: Michael S. Tsirkin <mst@redhat.com>
> > Sent: Wednesday, April 5, 2023 1:22 AM
> > 
> > This is not much of an improvement.
> > 
> > On Wed, Apr 05, 2023 at 04:06:50AM +0300, Parav Pandit wrote:
> > > Currently queue_notify_data register indicates the device internal
> > > queue notification identifier. This register is meaningful only when
> > > feature bit VIRTIO_F_NOTIF_CONFIG_DATA is negotiated.
> > >
> > > However, above register name often get confusing association with very
> > > similar feature bit VIRTIO_F_NOTIFICATION_DATA.
> > >
> > > When VIRTIO_F_NOTIFICATION_DATA feature bit is negotiated,
> > > notification really involves sending additional queue progress related
> > > information (not queue identifier).
> > >
> > > Hence
> > > 1. to avoid any misunderstanding and association of queue_notify_data
> > > with similar name VIRTIO_F_NOTIFICATION_DATA,
> > >
> > > and
> > > 2. to reflect that queue_notify_data is the actual device internal vq
> > > identifier, rename it to queue_notify_id.
> > >
> > > Reflect vq identifier in the driver notification structure by renaming
> > > ambiguous vqn to vq_notify_id.
> > >
> > > The driver notification section assumes that queue notification
> > > contains vq index always. CONFIG_DATA feature bit introduction missed
> > > to update the driver notification section. Hence, correct it.
> > >
> > > Signed-off-by: Parav Pandit <parav@nvidia.com>
> > >
> > > ---
> > > Some side notes:
> > > renaming vqn to vqnd is even more confusing because data is really the
> > > queue identifier.
> > 
> > Clear to whom?  Why do you think so? Marvell who pushed this feature said
> > they stick some kind of constant value there which matches what their
> > hardware expects. Sounds like a valid way to use this.
> Spec currently has described it as "identifier".

Nope, it gives an identifier as one example of a legal value.


> Snippet:
> "for example an internal virtqueue identifier,
> or an internal offset related to the virtqueue number"
> 
> > So no, not an identifier, and in any case "vq identifier" is a really general and
> > useful term, I would rather not burn it up on a baroque feature that almost no
> > one sets - for almost everyone else this is simply vq index.
> > 
> > If you really insist on renaming this away from "vqn", maybe
> > vq_index_config_data will do, and we can add a comment /* Either vq index or
> > vq config data, previously named vqn */
> > 
> Vq_index_config_data also aligns with the crazy feature bit name.
> So I will use that.

:)

It's just standard thing you get if you overload fields. And this
is data path we could not just add fields easily.

-- 
MST


This publicly archived list offers a means to provide input to the
OASIS Virtual I/O Device (VIRTIO) TC.

In order to verify user consent to the Feedback License terms and
to minimize spam in the list archive, subscription is required
before posting.

Subscribe: virtio-comment-subscribe@lists.oasis-open.org
Unsubscribe: virtio-comment-unsubscribe@lists.oasis-open.org
List help: virtio-comment-help@lists.oasis-open.org
List archive: https://lists.oasis-open.org/archives/virtio-comment/
Feedback License: https://www.oasis-open.org/who/ipr/feedback_license.pdf
List Guidelines: https://www.oasis-open.org/policies-guidelines/mailing-lists
Committee: https://www.oasis-open.org/committees/virtio/
Join OASIS: https://www.oasis-open.org/join/