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