Re: [virtio-dev] Re: [virtio-comment] RE: [virtio-dev] RE: [PATCH v12 03/10] content: Rename confusing queue_notify_data and vqn names
On Mon, 17 Apr 2023 03:04:34 -0400 "Michael S. Tsirkin" wrote: > On Mon, Apr 17, 2023 at 05:18:44AM +0200, Halil Pasic wrote: > > On Tue, 11 Apr 2023 13:35:09 + > > Parav Pandit wrote: > > > > > > From: Cornelia Huck > > > > Sent: Tuesday, April 11, 2023 4:56 AM > > > > > > > > > > > Yes, please leave it as F_CONFIG_DATA, as we're just putting some "data" > > > > there in the end (and F_CONFIG_COOKIE might indeed be confusing for the > > > > ccw case.) > > > > > > Since Halil didn't respond for 5+ days + Michel and you propose to > > > continue use CONFIG_DATA and this is rare used field, I will rename > > > > > > > Sorry, this one has fallen through the cracks. > > Well this whole patchset is just a cleanup so it's not holding up other > work at least. But I have to say it's difficult to make progress when > someone comes back from outer space after more than a week of silence > while others finished a discussion and reopens it with some new > feedback. Sorry, this was after 6 days. I didn't know that qualifies as 'outer space'. As pointed out below, I was monitoring the preceding discussion, and since the way things went was and is acceptable for me, I didn't want to muddy the waters any further. The issue I ended up addressing got introduced in very last email, which pre-announced the next version. My first intention was to explain myself, and apologize, after being called out. But then, also looking by looking at v13 I realized that there might have been a slip up because F_NOTIF_CONFIG_DATA got shortened to F_CONFIG_DATA in the discussion, which is no big deal for the discussion itself, but may have leaked in the v13 proposal. Parav has sent out the announced next version after about 8 hours. And if it weren't for my hypothesis why we ended up with the proposed name vq_config_data, the right place to discuss further would have been v13. In hindsight, I see, replying to the v12 thread wasn't a good move. [..] > > I also feel high latency is one of the reasons people are beginning to > ask to split into subcommitees where they won't have to deal with this > kind of thing. > I tend to agree. > Let's try to keep the latency low, please. Believe me, it is not like I'm actively trying to introduce extra latency. Regards, Halil > > For > > the preceding ones: I do not have a strong opinion. I do > > share Michael's and Connie's assessment regarding a possible > > clash with CCW. - To unsubscribe, e-mail: virtio-dev-unsubscr...@lists.oasis-open.org For additional commands, e-mail: virtio-dev-h...@lists.oasis-open.org
[virtio-dev] Re: Participation (was: Re: [virtio-comment] RE: [virtio-dev] RE: [PATCH v12 03/10] content: Rename confusing queue_notify_data and vqn names)
On Mon, Apr 17, 2023 at 10:47:47AM +0200, Cornelia Huck wrote: > I agree that patch reposting is happening much too fast in some > cases. Not sure how to formalize that, either. Can we please just be > more mindful of that? Reviewing time is not free. If I'm trying to do a > timely review of something and constantly see new versions while I'm not > finished yet, I do not feel like my feedback is actually valued. Imagine a group of contributors working on spec 100% of time. What do you want them to do? they need to exchange patches, slowing them down because part time reviewers are overwhelmed would be a waste. A separate mailing list would be one solution. Some tag in the subject would be another. RFC? Though RFC is used for other things as well... -- MST - To unsubscribe, e-mail: virtio-dev-unsubscr...@lists.oasis-open.org For additional commands, e-mail: virtio-dev-h...@lists.oasis-open.org
[virtio-dev] Participation (was: Re: [virtio-comment] RE: [virtio-dev] RE: [PATCH v12 03/10] content: Rename confusing queue_notify_data and vqn names)
On Mon, Apr 17 2023, "Michael S. Tsirkin" wrote: > I am not saying don't give feedback but I'm saying please help us > all be more organized, feedback time really should be within a > day or two, in rare cases up to a week. Given that there are things like weekends, public holidays, etc. one day does not look reasonable; while it certainly makes sense to continue if no feedback is forthcoming for a few days, not accounting for the fact that this is not the exclusive job for most/any of us is just a fast track to either burnout or people dropping out of virtio standardization altogether. > And I'd like to remind everyone if you are going away you are supposed > to report a leave of absence. Well... "Each request shall be made by sending an email to the TC mailing list at least 7 days before the Leave is to start." is probably not going to work for many cases. (Also, in any other community I'm participating in it is expected that you just might not be there or have time to work on it every week -- I've always seen that leave of absence thingy as something for a really long vacation or for something like parental leave, for which the max 45 days is really too short...) Not to mention that it applies to voting, not to participation on the lists. > TC's that have meetings just take away voting rights from someone who > does not attend two meetings in a row. We do it by ballot so this does > not apply, but I think we should set some limits in group's bylaws, > anyway. Ideas on formalizing this? If not we can just have informal > guidelines. There's of course a flip side to this. Some patches > seemingly go through two versions a day. Keeping up becomes a full time > job. We'd need a guideline for that, too. What do we actually expect from TC members? "Reply to emails" is not part of any formal requirement AFAICS (and not all TC members do participate on the lists on a regular basis anyway). The requirement is only to vote on the ballots, and there you're completely free to vote "abstain", so you can always squeeze in voting even if you're not able to participate otherwise. I think that's fine. "If I don't get a reply after $NUMBER_OF_WORKING_DAYS, I'll assume I can proceed as I think fit" is a reasonable assumption to make, e.g. to request a vote. Not sure if/how to formalize that. Also, how is this supposed to work if the original author doesn't reply to comments? Should the proposal be considered abandoned? I agree that patch reposting is happening much too fast in some cases. Not sure how to formalize that, either. Can we please just be more mindful of that? Reviewing time is not free. If I'm trying to do a timely review of something and constantly see new versions while I'm not finished yet, I do not feel like my feedback is actually valued. > I also feel high latency is one of the reasons people are beginning to > ask to split into subcommitees where they won't have to deal with this > kind of thing. Let's try to keep the latency low, please. I think there's multiple things to unpack here. - The very common strain of limited reviewer time. This seems to be an issue nearly everywhere. Encouraging more review helps; but if review and ensuing discussing turns into a time sink, it just cannot be handled at a reasonable activity level anymore. - Latency due to missing feedback. Can be solved by just requesting a vote if no feedback is forthcoming in a reasonable time frame. - Latency due to missing reaction to feedback. This means the proposal just doesn't make any progress. The onus is on the submitter here. - Conflicting approaches favoured by different people. This cannot be resolved in a formal way; either people need to be convinced that a certain approach will work, a middle ground found, or a way worked out that the different approaches can co-exist. In any case, this usually means long discussions which can be very frustrating, but unless we want to bulldoze over some people this is something we'll have to live with. [Personally, I think this is the worst contributor to frustration, and not something that can be solved by subcommittees.] - [I'm also not happy with the tone of some emails I've been seeing. I won't point to them in order not to stir up things that have already calmed down again.] - To unsubscribe, e-mail: virtio-dev-unsubscr...@lists.oasis-open.org For additional commands, e-mail: virtio-dev-h...@lists.oasis-open.org
[virtio-dev] Re: [virtio-comment] RE: [virtio-dev] RE: [PATCH v12 03/10] content: Rename confusing queue_notify_data and vqn names
On Mon, Apr 17, 2023 at 05:18:44AM +0200, Halil Pasic wrote: > On Tue, 11 Apr 2023 13:35:09 + > Parav Pandit wrote: > > > > From: Cornelia Huck > > > Sent: Tuesday, April 11, 2023 4:56 AM > > > > > > > > Yes, please leave it as F_CONFIG_DATA, as we're just putting some "data" > > > there in the end (and F_CONFIG_COOKIE might indeed be confusing for the > > > ccw case.) > > > > Since Halil didn't respond for 5+ days + Michel and you propose to continue > > use CONFIG_DATA and this is rare used field, I will rename > > > > Sorry, this one has fallen through the cracks. Well this whole patchset is just a cleanup so it's not holding up other work at least. But I have to say it's difficult to make progress when someone comes back from outer space after more than a week of silence while others finished a discussion and reopens it with some new feedback. I am not saying don't give feedback but I'm saying please help us all be more organized, feedback time really should be within a day or two, in rare cases up to a week. And I'd like to remind everyone if you are going away you are supposed to report a leave of absence. TC's that have meetings just take away voting rights from someone who does not attend two meetings in a row. We do it by ballot so this does not apply, but I think we should set some limits in group's bylaws, anyway. Ideas on formalizing this? If not we can just have informal guidelines. There's of course a flip side to this. Some patches seemingly go through two versions a day. Keeping up becomes a full time job. We'd need a guideline for that, too. I also feel high latency is one of the reasons people are beginning to ask to split into subcommitees where they won't have to deal with this kind of thing. Let's try to keep the latency low, please. > For > the preceding ones: I do not have a strong opinion. I do > share Michael's and Connie's assessment regarding a possible > clash with CCW. > > Let me just note that the feature ain't called, "F_CONFIG_DATA" (i.e. > with full name VIRTIO_F_CONFIG_DATA) but rather "F_NOTIF_CONFIG_DATA" > (i.e. with full name VIRTIO_F_NOTIF_CONFIG_DATA). > > > vqn to union of > > > > vq_index > > vq_config_data > > In that sense vq_config_data is not good in my opinion, because > it misses "notif" which is present in both "F_NOTIF_CONFIG_DATA" > and "queue_notify_data". > > > > Thanks. Will roll v13. > > > > I'm about tho have a look how this panned out in v13. I propose > let us continue the discussion there. > > Regards, > Halil > > > 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-subscr...@lists.oasis-open.org > > Unsubscribe: virtio-comment-unsubscr...@lists.oasis-open.org > > List help: virtio-comment-h...@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/ > > - To unsubscribe, e-mail: virtio-dev-unsubscr...@lists.oasis-open.org For additional commands, e-mail: virtio-dev-h...@lists.oasis-open.org
[virtio-dev] Re: [virtio-comment] RE: [virtio-dev] RE: [PATCH v12 03/10] content: Rename confusing queue_notify_data and vqn names
On Tue, 11 Apr 2023 13:35:09 + Parav Pandit wrote: > > From: Cornelia Huck > > Sent: Tuesday, April 11, 2023 4:56 AM > > > > > Yes, please leave it as F_CONFIG_DATA, as we're just putting some "data" > > there in the end (and F_CONFIG_COOKIE might indeed be confusing for the > > ccw case.) > > Since Halil didn't respond for 5+ days + Michel and you propose to continue > use CONFIG_DATA and this is rare used field, I will rename > Sorry, this one has fallen through the cracks. For the preceding ones: I do not have a strong opinion. I do share Michael's and Connie's assessment regarding a possible clash with CCW. Let me just note that the feature ain't called, "F_CONFIG_DATA" (i.e. with full name VIRTIO_F_CONFIG_DATA) but rather "F_NOTIF_CONFIG_DATA" (i.e. with full name VIRTIO_F_NOTIF_CONFIG_DATA). > vqn to union of > > vq_index > vq_config_data In that sense vq_config_data is not good in my opinion, because it misses "notif" which is present in both "F_NOTIF_CONFIG_DATA" and "queue_notify_data". > > Thanks. Will roll v13. > I'm about tho have a look how this panned out in v13. I propose let us continue the discussion there. Regards, Halil > 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-subscr...@lists.oasis-open.org > Unsubscribe: virtio-comment-unsubscr...@lists.oasis-open.org > List help: virtio-comment-h...@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/ > - To unsubscribe, e-mail: virtio-dev-unsubscr...@lists.oasis-open.org For additional commands, e-mail: virtio-dev-h...@lists.oasis-open.org
[virtio-dev] RE: [virtio-comment] RE: [virtio-dev] RE: [PATCH v12 03/10] content: Rename confusing queue_notify_data and vqn names
> From: Cornelia Huck > Sent: Tuesday, April 11, 2023 4:56 AM > > Yes, please leave it as F_CONFIG_DATA, as we're just putting some "data" > there in the end (and F_CONFIG_COOKIE might indeed be confusing for the > ccw case.) Since Halil didn't respond for 5+ days + Michel and you propose to continue use CONFIG_DATA and this is rare used field, I will rename vqn to union of vq_index vq_config_data Thanks. Will roll v13. - To unsubscribe, e-mail: virtio-dev-unsubscr...@lists.oasis-open.org For additional commands, e-mail: virtio-dev-h...@lists.oasis-open.org
[virtio-dev] Re: [virtio-comment] RE: [virtio-dev] RE: [PATCH v12 03/10] content: Rename confusing queue_notify_data and vqn names
On Fri, Apr 07 2023, "Michael S. Tsirkin" wrote: > On Wed, Apr 05, 2023 at 03:58:55PM +, Parav Pandit wrote: >> For sure "cookie" is better than "config_data" and I don't have objection to >> "cookie". >> >> But I disagree to the claim that "identifier" is less good than "cookie". >> >> It is pointless debate of "identifier" vs "cookie". >> >> The union format is very useful to describe this crisply, I will use it. > > I guess I'm fine with "cookie" in that in CS it is by now widely > understood to mean "some opaque data". > identifier comes from "identical", so implies a 1:1 mapping IMO. Agreed, a "cookie" is not the same as an "identifier", although it may contain one. > > > The logic behind using a cookie is that it's a bit similar > to host cookie from ccw. > However, with ccw host cookie is used unconditionally, as > opposed to depending on the flag. > > > >> Do you prefer to rename F_CONFIG_DATA To F_CONFIG_COOKIE? > > It should all be consistent, but I worry about ccw which uses cookies > unconditionally. I am fine with leaving it as F_CONFIG_DATA for now > unless we see a good way to avoid confusion for ccw. Yes, please leave it as F_CONFIG_DATA, as we're just putting some "data" there in the end (and F_CONFIG_COOKIE might indeed be confusing for the ccw case.) - To unsubscribe, e-mail: virtio-dev-unsubscr...@lists.oasis-open.org For additional commands, e-mail: virtio-dev-h...@lists.oasis-open.org
[virtio-dev] Re: [PATCH v12 03/10] content: Rename confusing queue_notify_data and vqn names
On Wed, Apr 05, 2023 at 01:20:04PM +, Parav Pandit wrote: > > > > From: Michael S. Tsirkin > > 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 > > > > > > --- > > > 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 - To unsubscribe, e-mail: virtio-dev-unsubscr...@lists.oasis-open.org For additional commands, e-mail: virtio-dev-h...@lists.oasis-open.org
[virtio-dev] Re: [virtio-comment] RE: [virtio-dev] RE: [PATCH v12 03/10] content: Rename confusing queue_notify_data and vqn names
On Wed, Apr 05, 2023 at 03:58:55PM +, Parav Pandit wrote:
> > From: Halil Pasic
> > Sent: Wednesday, April 5, 2023 11:28 AM
> >
> > On Wed, 5 Apr 2023 13:21:40 +
> > Parav Pandit wrote:
> >
> > > > VIRTIO_F_NOTIF_CONFIG_DATA is such a narrow usecase, I don't like
> > > > burning "vq identifier" on this. How about we just say something along
> > > > the
> > lines of:
> > > >
> > > Ok.
> > > >
> > > > When VIRTIO_F_NOTIFICATION_DATA has not been negotiated, this
> > > > notification involves sending either the virtqueue index or the
> > > > virtqueue config data to the device (method depending on the transport).
> > > >
> > > > And then "the data sent is a device supplied virtqueue config data".
> > > >
> > > Sounds fine. I will reword it.
> >
> > FYI in an other thread I proposed calling this a "cookie". Sorry for being
> > late to
> > the party. Yet again.
>
> If we spend (waste) more time, we will find many examples where "identifier"
> and "cookie" both are used in things associated with computer science.
>
> That too when same set of people has accepted text " internal virtqueue
> identifier" for same feature of CONFIG_DATA even though somehow it was not
> "id"!
because that's just an example:
In a trivial case the device can set \field{queue_notify_data}=vqn.
Some devices
may benefit from providing another value, for example an internal
virtqueue
identifier, or an internal offset related to the virtqueue number.
so the cookie can either be an identifier or something else.
> And when this spec refers to an RFC of UUID, session id (not "session
> cookie", even though session id is opaque and not meaningful to the recipient
> as per Wikipedia usage desc that you listed).
>
> For sure "cookie" is better than "config_data" and I don't have objection to
> "cookie".
>
> But I disagree to the claim that "identifier" is less good than "cookie".
>
> It is pointless debate of "identifier" vs "cookie".
>
> The union format is very useful to describe this crisply, I will use it.
I guess I'm fine with "cookie" in that in CS it is by now widely
understood to mean "some opaque data".
identifier comes from "identical", so implies a 1:1 mapping IMO.
The logic behind using a cookie is that it's a bit similar
to host cookie from ccw.
However, with ccw host cookie is used unconditionally, as
opposed to depending on the flag.
> Do you prefer to rename F_CONFIG_DATA To F_CONFIG_COOKIE?
It should all be consistent, but I worry about ccw which uses cookies
unconditionally. I am fine with leaving it as F_CONFIG_DATA for now
unless we see a good way to avoid confusion for ccw.
--
MST
-
To unsubscribe, e-mail: virtio-dev-unsubscr...@lists.oasis-open.org
For additional commands, e-mail: virtio-dev-h...@lists.oasis-open.org
RE: [virtio-dev] RE: [PATCH v12 03/10] content: Rename confusing queue_notify_data and vqn names
> From: Halil Pasic > Sent: Wednesday, April 5, 2023 11:28 AM > > On Wed, 5 Apr 2023 13:21:40 + > Parav Pandit wrote: > > > > VIRTIO_F_NOTIF_CONFIG_DATA is such a narrow usecase, I don't like > > > burning "vq identifier" on this. How about we just say something along the > lines of: > > > > > Ok. > > > > > > When VIRTIO_F_NOTIFICATION_DATA has not been negotiated, this > > > notification involves sending either the virtqueue index or the > > > virtqueue config data to the device (method depending on the transport). > > > > > > And then "the data sent is a device supplied virtqueue config data". > > > > > Sounds fine. I will reword it. > > FYI in an other thread I proposed calling this a "cookie". Sorry for being > late to > the party. Yet again. If we spend (waste) more time, we will find many examples where "identifier" and "cookie" both are used in things associated with computer science. That too when same set of people has accepted text " internal virtqueue identifier" for same feature of CONFIG_DATA even though somehow it was not "id"! And when this spec refers to an RFC of UUID, session id (not "session cookie", even though session id is opaque and not meaningful to the recipient as per Wikipedia usage desc that you listed). For sure "cookie" is better than "config_data" and I don't have objection to "cookie". But I disagree to the claim that "identifier" is less good than "cookie". It is pointless debate of "identifier" vs "cookie". The union format is very useful to describe this crisply, I will use it. Do you prefer to rename F_CONFIG_DATA To F_CONFIG_COOKIE? - To unsubscribe, e-mail: virtio-dev-unsubscr...@lists.oasis-open.org For additional commands, e-mail: virtio-dev-h...@lists.oasis-open.org
Re: [virtio-dev] RE: [PATCH v12 03/10] content: Rename confusing queue_notify_data and vqn names
On Wed, 5 Apr 2023 13:21:40 + Parav Pandit wrote: > > VIRTIO_F_NOTIF_CONFIG_DATA is such a narrow usecase, I don't like burning > > "vq identifier" on this. How about we just say something along the lines of: > > > Ok. > > > > When VIRTIO_F_NOTIFICATION_DATA has not been negotiated, this > > notification involves sending either the virtqueue index or the virtqueue > > config > > data to the device (method depending on the transport). > > > > And then "the data sent is a device supplied virtqueue config data". > > > Sounds fine. I will reword it. FYI in an other thread I proposed calling this a "cookie". Sorry for being late to the party. Yet again. Regards, Halil - To unsubscribe, e-mail: virtio-dev-unsubscr...@lists.oasis-open.org For additional commands, e-mail: virtio-dev-h...@lists.oasis-open.org
[virtio-dev] RE: [PATCH v12 03/10] content: Rename confusing queue_notify_data and vqn names
> From: Michael S. Tsirkin > Sent: Wednesday, April 5, 2023 1:30 AM > VIRTIO_F_NOTIF_CONFIG_DATA is such a narrow usecase, I don't like burning > "vq identifier" on this. How about we just say something along the lines of: > Ok. > > When VIRTIO_F_NOTIFICATION_DATA has not been negotiated, this > notification involves sending either the virtqueue index or the virtqueue > config > data to the device (method depending on the transport). > > And then "the data sent is a device supplied virtqueue config data". > Sounds fine. I will reword it. - To unsubscribe, e-mail: virtio-dev-unsubscr...@lists.oasis-open.org For additional commands, e-mail: virtio-dev-h...@lists.oasis-open.org
[virtio-dev] RE: [PATCH v12 03/10] content: Rename confusing queue_notify_data and vqn names
> From: Michael S. Tsirkin > 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 > > > > --- > > 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". 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. - To unsubscribe, e-mail: virtio-dev-unsubscr...@lists.oasis-open.org For additional commands, e-mail: virtio-dev-h...@lists.oasis-open.org
[virtio-dev] Re: [PATCH v12 03/10] content: Rename confusing queue_notify_data and vqn names
On Wed, Apr 05 2023, "Michael S. Tsirkin" wrote:
> On Wed, Apr 05, 2023 at 04:06:50AM +0300, Parav Pandit wrote:
>> diff --git a/content.tex b/content.tex
>> index cd93db2..d5f8026 100644
>> --- a/content.tex
>> +++ b/content.tex
>> @@ -405,8 +405,18 @@ \section{Driver Notifications} \label{sec:Basic
>> Facilities of a Virtio Device /
>> notification to the device.
>>
>> When VIRTIO_F_NOTIFICATION_DATA has not been negotiated,
>> -this notification involves sending the
>> -virtqueue index to the device (method depending on the transport).
>> +this notification involves sending only the 16-bit virtqueue notification
>> +identifier (notification method depends on the transport).
>> +
>> +\begin{itemize}
>> +\item When VIRTIO_F_NOTIF_CONFIG_DATA is not negotiated, virtqueue
>> +notification identifier is a 16-bit vq index.
>> +
>> +\item When VIRTIO_F_NOTIF_CONFIG_DATA is negotiated, virtqueue
>> +notification identifier is a device supplied virtqueue identifier. A method
>> +to supply such virtqueue notification identifier is transport
>> +specific.
>> +\end{itemize}
>>
>> However, some devices benefit from the ability to find out the
>> amount of available data in the queue without accessing the virtqueue in
>> memory:
>
>
> VIRTIO_F_NOTIF_CONFIG_DATA is such a narrow usecase, I don't like
> burning "vq identifier" on this. How about we just say something
> along the lines of:
>
>
> When VIRTIO_F_NOTIFICATION_DATA has not been negotiated, this
> notification involves sending either the virtqueue index or the
> virtqueue config data to the device (method depending on the
> transport).
>
> And then "the data sent is a device supplied virtqueue config data".
Agreed, referring to "virtqueue configuration data" or somesuch is
better than using up the "vq identifier" name for it.
-
To unsubscribe, e-mail: virtio-dev-unsubscr...@lists.oasis-open.org
For additional commands, e-mail: virtio-dev-h...@lists.oasis-open.org
[virtio-dev] Re: [PATCH v12 03/10] content: Rename confusing queue_notify_data and vqn names
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
>
> ---
> Some side notes:
> renaming vqn to vqnd is even more confusing because data is really the
> queue identifier.
>
> And NOTIFICATION_DATA really contains queue progress info (data).
>
> (vqn - n is number or notification?, notification word in the
> notification structure does not make sense).
>
> Hence above renaming.
>
> ---
> changelog:
> v11->v12:
> - new patch
> ---
> content.tex| 17 ++---
> notifications-be.c | 2 +-
> notifications-le.c | 2 +-
> transport-pci.tex | 24 ++--
> 4 files changed, 30 insertions(+), 15 deletions(-)
>
> diff --git a/content.tex b/content.tex
> index cd93db2..d5f8026 100644
> --- a/content.tex
> +++ b/content.tex
> @@ -405,8 +405,18 @@ \section{Driver Notifications} \label{sec:Basic
> Facilities of a Virtio Device /
> notification to the device.
>
> When VIRTIO_F_NOTIFICATION_DATA has not been negotiated,
> -this notification involves sending the
> -virtqueue index to the device (method depending on the transport).
> +this notification involves sending only the 16-bit virtqueue notification
> +identifier (notification method depends on the transport).
> +
> +\begin{itemize}
> +\item When VIRTIO_F_NOTIF_CONFIG_DATA is not negotiated, virtqueue
> +notification identifier is a 16-bit vq index.
> +
> +\item When VIRTIO_F_NOTIF_CONFIG_DATA is negotiated, virtqueue
> +notification identifier is a device supplied virtqueue identifier. A method
> +to supply such virtqueue notification identifier is transport
> +specific.
> +\end{itemize}
>
> However, some devices benefit from the ability to find out the
> amount of available data in the queue without accessing the virtqueue in
> memory:
VIRTIO_F_NOTIF_CONFIG_DATA is such a narrow usecase, I don't like
burning "vq identifier" on this. How about we just say something
along the lines of:
When VIRTIO_F_NOTIFICATION_DATA has not been negotiated, this
notification involves sending either the virtqueue index or the
virtqueue config data to the device (method depending on the
transport).
And then "the data sent is a device supplied virtqueue config data".
> @@ -417,7 +427,8 @@ \section{Driver Notifications} \label{sec:Basic
> Facilities of a Virtio Device /
> the following information:
>
> \begin{description}
> -\item [vqn] VQ number to be notified.
> +\item [vq_notify_id] VQ notification identifier of a
> + corresponding virtqueue.
> \item [next_off] Offset
>within the ring where the next available ring entry
>will be written.
> diff --git a/notifications-be.c b/notifications-be.c
> index 5be947e..0a7cbf1 100644
> --- a/notifications-be.c
> +++ b/notifications-be.c
> @@ -1,5 +1,5 @@
> be32 {
> - vqn : 16;
> + vq_notify_id : 16; /* previously known as vqn */
> next_off : 15;
> next_wrap : 1;
> };
> diff --git a/notifications-le.c b/notifications-le.c
> index fe51267..6cca8fb 100644
> --- a/notifications-le.c
> +++ b/notifications-le.c
> @@ -1,5 +1,5 @@
> le32 {
> - vqn : 16;
> + vq_notify_id : 16; /* previously known as vqn */
> next_off : 15;
> next_wrap : 1;
> };
> diff --git a/transport-pci.tex b/transport-pci.tex
> index 5d98467..6bbf61c 100644
> --- a/transport-pci.tex
> +++ b/transport-pci.tex
> @@ -319,7 +319,7 @@ \subsubsection{Common configuration structure
> layout}\label{sec:Virtio Transport
> le64 queue_desc;/* read-write */
> le64 queue_driver; /* read-write */
> le64 queue_device; /* read-write */
> -le16 queue_notify_data; /* read-only for driver */
> +le16 queue_notify_id; /* read-only for driver */
> le16 queue_reset; /* read-write */
> };
> \end{lstlisting}
> @@ -388,17 +388,21 @@ \subsubsection{Common
[virtio-dev] Re: [PATCH v12 03/10] content: Rename confusing queue_notify_data and vqn names
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 > > --- > 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. 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 */ > And NOTIFICATION_DATA really contains queue progress info (data). > > (vqn - n is number or notification?, notification word in the > notification structure does not make sense). > > Hence above renaming. - To unsubscribe, e-mail: virtio-dev-unsubscr...@lists.oasis-open.org For additional commands, e-mail: virtio-dev-h...@lists.oasis-open.org
