> > I see your point, but a reader will see the "global notifications
> > coalescing parameter" concept, and won't know what it is until next
> > paragraph.
> > Maybe we can have a new list with the notification coalescing concepts
> > (like the notification coalescing parameters)? Just throwing
> >> -If the VIRTIO_NET_F_NOTF_COAL feature is negotiated, the driver can
> >> -send control commands for dynamically changing the coalescing parameters.
> >> +If the VIRTIO_NET_F_NOTF_COAL feature is negotiated, the driver can send
> >> the VIRTIO_NET_CTRL_NOTF_COAL_TX_SET
> >> +and
n Zhuo
> ---
> This patch is on top of Alvaro's latest v7 patch:
> https://lists.oasis-open.org/archives/virtio-dev/202302/msg00431.html .
>
> v8->v9:
>1. Declare the commands that can be sent for each feature. @Alvaro
> Karsz
>2. Add information about "
I'd like to propose this patch for a TC vote.
Thanks :)
-
To unsubscribe, e-mail: virtio-dev-unsubscr...@lists.oasis-open.org
For additional commands, e-mail: virtio-dev-h...@lists.oasis-open.org
> A general note,
>
> Correct my if I'm wrong, but the fact that a driver can't send
> VIRTIO_NET_CTRL_NOTF_COAL_[T/R]X_SET commands if
> VIRTIO_NET_F_NOTF_COAL is not negotiated isn't explicitly mentioned
> anywhere.
> A reader may think that the commands cant be sent if
I meant "can", not
close to 2". @Michael S . Tsirkin, @David Edmondson
>
> v6->v7:
>1. Clarify the relationship of VIRTIO_NET_CTRL_NOTF_COAL_TX/RX_SET and
> VIRTIO_NET_CTRL_NOTF_COAL_VQ_SET. @Alvaro Karsz, @Michael S. Tsirkin
>2. Remove the formula for vqn range. @Parav Pandit
&
t;.
Fixes: https://github.com/oasis-tcs/virtio-spec/issues/162
Signed-off-by: Alvaro Karsz
---
v2:
- Rephrase commit log, no changes to patch body.
device-types/net/description.tex | 8
1 file changed, 8 insertions(+)
diff --git a/device-types/net/description.tex b/device
alescing parameters for vq
> reset. @Michael S. Tsirkin
>4. Fix some syntax errors. @Michael S. Tsirkin, @David Edmondson
>
> v3->v4:
>1. Include virtio_net_ctrl_coal in the virtio_net_ctrl_coal_vq
> structure. @Alvaro Karsz
>2. Add consideration of
> Minor nit (for the next time, was trivial to fix up): Putting your s-o-b
> beneath the "---" instructs git am to cut it off; the changelog should
> go below the s-o-b.
You're right, sorry, I missed it.
-
To unsubscribe,
Hi,
> I agree that we need to stick with SHOULD here.
>
> Are VIRTIO_NET_F_CTRL_RX_EXTRA and VIRTIO_NET_F_CTRL_RX independent of
> each other, i.e. can you negotiate VIRTIO_NET_F_CTRL_RX_EXTRA but not
> VIRTIO_NET_F_CTRL_RX? Looks like it to me, but would like to confirm.
> (We would at most be
I didn't add the control vq feature as a feature bit requirement for
the same reason as [1].
If you think that we can add a feature bit requirement instead of
"SHOULD NOT offer" and "SHOULD NOT negotiate", I can add it in v2.
[1] https://github.com/oasis-tcs/virtio-spec/issues/158
t;.
Fixes: https://github.com/oasis-tcs/virtio-spec/issues/162
Signed-off-by: Alvaro Karsz
---
device-types/net/description.tex | 8
1 file changed, 8 insertions(+)
diff --git a/device-types/net/description.tex b/device-types/net/description.tex
index 8487ccd..cf37da5 100644
--- a/de
I'd like to propose this patch for a TC vote.
Thanks :)
-
To unsubscribe, e-mail: virtio-dev-unsubscr...@lists.oasis-open.org
For additional commands, e-mail: virtio-dev-h...@lists.oasis-open.org
coalescing operation.
Fixes: https://github.com/oasis-tcs/virtio-spec/issues/159
Signed-off-by: Alvaro Karsz
Reviewed-by: Parav Pandit
Acked-by: Michael S. Tsirkin
---
v2:
- Add the last 2 points to the patch.
- Rephrase the "commands are best-effort" note.
Heng,
Thanks for your work too!
Michael,
Thanks, I"ll fix the grammar in v7.
-
To unsubscribe, e-mail: virtio-dev-unsubscr...@lists.oasis-open.org
For additional commands, e-mail: virtio-dev-h...@lists.oasis-open.org
coalescing operation.
Signed-off-by: Alvaro Karsz
Reviewed-by: Parav Pandit
---
v2:
- Add the last 2 points to the patch.
- Rephrase the "commands are best-effort" note.
- Replace "notification" with "used buffer notification" to be
more
> Not good. I feel we must come up with spec that is backwards compatible.
> Hmm, maybe this is why Parav kept talking about modes.
> I did not realize at the time, sorry Parav.
>
> I still feel modes are not the best way to describe things so I propose this:
> - in addition to per vq parameters,
> > Yes, max_packets and max_usecs SHOULD be reset to 0.
> > When the virtqueue is re-enabled, it means that notification conditions are
> > met after each packet is sent/received.
> >
> > As described by Alvaro in "[PATCH v5] virtio-net: Fix and update
> > VIRTIO_NET_F_NOTF_COAL feature":
> >
> sounds good. maybe "counting packets and microseconds"? to make clear
> timer resets too.
>
> > I don't really mind, If you think that the device implementation is
> > not essential here and we should add your suggestion, I'm ok with
> > that.
>
> I don't mind, your proposal seems good too.
Ok,
> Looks good, thanks! Yet something to improve, see below:
> > ---
> > v2:
> > - Add the last 2 points to the patch.
> > - Rephrase the "commands are best-effort" note.
> > - Replace "notification" with "used buffer notification" to be
> > more consistent.
> > v3:
> >
> > Maybe we can use struct virtio_net_ctrl_coal inside struct
> > virtio_net_ctrl_coal_vq instead of repeating max_usecs and
> > max_packets?
> > I'm not sure if it would be confusing, what do you think?
> >
>
> Hi Alvaro.
>
> I guess you mean one of the following two forms:
>
> #1
> struct
Hi Heng,
> +\item[VIRTIO_NET_F_VQ_NOTF_COAL(52)] Device supports the virtqueue
> +notifications coalescing.
> +
> \item[VIRTIO_NET_F_NOTF_COAL(53)] Device supports notifications coalescing.
>
> \item[VIRTIO_NET_F_GUEST_USO4 (54)] Driver can receive USOv4 packets.
> @@ -3140,6 +3143,7 @@
coalescing operation.
Signed-off-by: Alvaro Karsz
---
v2:
- Add the last 2 points to the patch.
- Rephrase the "commands are best-effort" note.
- Replace "notification" with "used buffer notification" to be
more consistent.
v3:
- Add
> > Actually, no.
> > \field{usecs} refers to the field in the struct.
> > usecs refers to the units of measurement.
> >
> > So if for example \field{usecs} = 40, then it evaluates to 40 usecs ->
> > "are met when 40 usecs elapses".
>
> But this is not how it reads, people are not computers, and
Hi Heng,
> > +When the device has non-zero \field{usecs} and non-zero
> > \field{max_packets}, it starts counting usecs and packets upon
> > receiving/sending a packet.
> > +The device counts packets and usecs for each receive virtqueue and
> > transmit virtqueue separately.
> > +In this case,
coalescing operation.
Signed-off-by: Alvaro Karsz
---
v2:
- Add the last 2 points to the patch.
- Rephrase the "commands are best-effort" note.
- Replace "notification" with "used buffer notification" to be
more consistent.
v3:
- Add
> > +\begin{note}
> > +In general, these commands are best-effort: spurious notifications could
> > still arrive.
> > +\end{note}
>
> This is quite vague. How about:
>
> The behaviour of the device in response to these commands is
> best-effort: the device may generate notifications more
> > +\begin{note}
> > +In general, these commands are best-effort: spurious notifications could
> > still arrive.
> > +\end{note}
>
> This is quite vague. How about:
>
> The behaviour of the device in response to these commands is
> best-effort: the device may generate notifications more
Thanks.
I'll send v4 tomorrow.
-
To unsubscribe, e-mail: virtio-dev-unsubscr...@lists.oasis-open.org
For additional commands, e-mail: virtio-dev-h...@lists.oasis-open.org
coalescing operation.
Signed-off-by: Alvaro Karsz
---
v2:
- Add the last 2 points to the patch.
- Rephrase the "commands are best-effort" note.
- Replace "notification" with "used buffer notification" to be
more consistent.
v3:
- Add
> ok so the event we are looking for is "coalescing parameters are met".
> It is ok but I would like something that works even if
> there is no coalescing. How about "notification conditions are met" then?
> The coalescing parameters specify the conditions then
>
Sounds good.
I'll add an intro explaining how the entire coalescing thing works,
without relying on the examples.
> > How about adding the following line in the main paragraph:
> >
> > Coalescing parameters are met when the number of sent/received packets
> > reaches \field{tx_max_packets} since the last used
> > +Every transmit and receive virtqueue needs to count its own packets and
> > should not be affected by notifications sent by other virtqueues.
>
> I find this slightly confusing in that it describes counting
> which we did not previously mention. and it is not virtqueue iself
> counting of
Splitting this patch as suggested in v1 will be messy,
We have lines affected by multiple changes, for example:
+\item VIRTIO_NET_CTRL_NOTF_COAL_RX_SET: set the \field{usecs} and
\field{max_packets} parameters for all the receive virtqueues.
Is affected by the consolidation of the structs and
the behavior of coalescing with regards to delivering
notifications when a change occur.
- Stating that the commands should apply to all the receive/transmit
virtqueues.
- Stating that every receive/transmit virtqueue should count it's own
packets.
Signed-off-by: Alvaro Karsz
---
v2
> I'm not sure the specific property is even a feature and not a bug.
> One can argue e.g. that ethtool should fail with EBUSY rather
> than being silently overwritten.
> Thats why I feel this kind of policy is best set by driver.
When you write "set by driver", you mean that [X] the spec needs
I think that I wasn't clear enough.
I'm not saying that we should not define in the spec how to handle a
situation when a device receives both RX_SET and VQ_SET (or a driver
sends both).
I'm saying that I don't think that the driver should handle the
situation the way you described it:
> When
You write "both are configured", "try to configure both", "disable one
and switch to other" "mixed mode" etc..
But no modes/levels are mentioned in the patch, not once.
I totally agree that *if* we define 2 modes, we should have strict
rules on how to switch between modes and how to operate with
> >> Please add short description something like,
> >>
> >> When the driver prefers to use per virtqueue notifications coalescing, and
> >> if queue group (transmit or receive) level notification coalescing is
> >> enabled, driver SHOULD first disable device level notification coalescing.
> >>
> > Please add short description something like,
> >
> > When the driver prefers to use per virtqueue notifications coalescing, and
> > if queue group (transmit or receive) level notification coalescing is
> > enabled, driver SHOULD first disable device level notification coalescing.
> > Or it
> I missed your previous response.
> Sorry for the late response.
No problem at all :)
> Why cannot device say as MUST requirement?
>
> Let say there is a device out that exposes a F_HASH_REPORT without F_CTRL_VQ.
> So driver tried to send a command and failed to issue cmd.
> End result, hash
> Please add short description something like,
>
> When the driver prefers to use per virtqueue notifications coalescing, and if
> queue group (transmit or receive) level notification coalescing is enabled,
> driver SHOULD first disable device level notification coalescing.
> Or it should be,
>
Should we have a vote?
Fixes: https://github.com/oasis-tcs/virtio-spec/issues/158
-
To unsubscribe, e-mail: virtio-dev-unsubscr...@lists.oasis-open.org
For additional commands, e-mail: virtio-dev-h...@lists.oasis-open.org
> >> So, should we remove VIRTIO_NET_F_CTRL_VQ here, or fix
> >> VIRTIO_NET_F_HOST_ECN?
> > Ah good point.
> > But I think VIRTIO_NET_F_VQ_NOTF_COAL should not depend on
> > VIRTIO_NET_F_NOTF_COAL.
> > This way devices can drop the all-rx/all-tx commands if they want to.
>
> We need to confirm
} instead of the qid. @Michael S. Tsirkin
> > 3. Unify tx and rx control structres into one structure
> > virtio_net_ctrl_coal_vq. @Michael S. Tsirkin
> > 4. Add a new control command VIRTIO_NET_CTRL_NOTF_COAL_VQ. @Michael S.
> > Tsirkin, @Parav Pandit, @Alvaro Karsz
> >
_NOTF_COAL.
> @Michael S. Tsirkin
> 2. Use the \field{vqn} instead of the qid. @Michael S. Tsirkin
> 3. Unify tx and rx control structres into one structure
> virtio_net_ctrl_coal_vq. @Michael S. Tsirkin
> 4. Add a new control command VIRTIO_NET_CTRL_NOTF_COAL_VQ. @Mi
Maybe we should do something more future friendly.
We may want to add more coalescing related features in the future.
* Maybe max/min sets?
* Maybe to offload the adaptive algo to the DPU?
It will require more feature bits..
We could rename this new feature from VIRTIO_NET_F_PERQUEUE_NOTF_COAL
> > From: Alvaro Karsz
> > Sent: Wednesday, February 8, 2023 4:56 PM
>
> > > Alvaro,
> > > Do you know if any software used it? Can you get some real data?
> >
> > I implemented this feature in our DPU, so at least 1 vendor is using this
> >
> > From: Michael S. Tsirkin
> > Sent: Wednesday, February 8, 2023 3:52 PM
> >
> > On Wed, Feb 08, 2023 at 07:53:09PM +0200, Alvaro Karsz wrote:
> > > > > From: Michael S. Tsirkin
> > > > > Sent: Wednesday, February 8, 2023 9:48 AM
> &
> On Wed, Feb 08, 2023 at 07:53:09PM +0200, Alvaro Karsz wrote:
> > > > From: Michael S. Tsirkin
> > > > Sent: Wednesday, February 8, 2023 9:48 AM
> > > >
> > > > On Wed, Feb 08, 2023 at 02:44:37PM +, Parav Pandit wrote:
> > >
> > From: Michael S. Tsirkin
> > Sent: Wednesday, February 8, 2023 12:39 PM
> >
> > On Wed, Feb 08, 2023 at 05:30:28PM +, Parav Pandit wrote:
> > > > From: Michael S. Tsirkin
> > > > Sent: Tuesday, February 7, 2023 9:18 AM
> > >
> > > > > Why migration generate too many spurious interrupts?
> > From: Michael S. Tsirkin
> > Sent: Wednesday, February 8, 2023 9:48 AM
> >
> > On Wed, Feb 08, 2023 at 02:44:37PM +, Parav Pandit wrote:
> > >
> > > > From: Michael S. Tsirkin
> > > > Sent: Wednesday, February 8, 2023 9:43 AM
> > > >
> > > > On Wed, Feb 08, 2023 at 02:37:55PM +,
Do you want a new version?
This one seems fine to me.
-
To unsubscribe, e-mail: virtio-dev-unsubscr...@lists.oasis-open.org
For additional commands, e-mail: virtio-dev-h...@lists.oasis-open.org
Seems that we still have some disagreements here.
Should I remove the "best effort" note? I actually think that this is
a good note, but I'll remove it if you think that I should.
Should I replace TX and RX with receive virtqueue and transmit
virtqueue? This may be related to Heng's patch, so
> An example of a question is this: we have two RX queues say 1 and 2 each
> with a distinct interrupt vector. coalescing is set to 10 packets. Now
> 9 packets arrive on queue 1 and 1 on queue 2. Do you expect an
> interrupt? If yes which one - 1 or 2 or both?
Sorry, I wasn't clear enough.
In
> Arguably it's true.
> It will all be up to the committee vote of course ...
> To keep things a bit safer how about we just allow commands
> without qid instead of a special qid value?
> Also if we are going to change things how about adding a query command too?
>
> Also Alvaro what is your take
Hi Heng,
> Currently, the coalescing profile is directly applied to all queues.
> This patch supports configuring the parameters for a specified queue.
>
> When the traffic between queues is unbalanced, for example, one queue
> is busy and another queue is idle, then it will be very useful to
>
Hi Parav, thanks for the comments.
> > +Note: In general, these commands are best-effort: A device could send a
> > notification even if it is not supposed to.
> >
> Please remove this note from this patch.
> Instead of Note, we need to describe this device requirements description.
> We better
Hi Parav,
> > +A driver SHOULD NOT negotiate VIRTIO_NET_F_HASH_REPORT if it does not
> > +negotiate VIRTIO_NET_F_CTRL_VQ.
> > +
> Same for the driver too.
> Like below.
> The driver should not negotiate a feature that requires to use a control VQ
> when VIRTIO_NET_F_CTRL_VQ is not negotiated.
the behavior of coalescing with regards to delivering
notifications when a change occur.
Signed-off-by: Alvaro Karsz
---
device-types/net/description.tex | 40 ++--
1 file changed, 22 insertions(+), 18 deletions(-)
diff --git a/device-types/net/description.tex b
Hi,
I'm going to drop this patch and focus on a new patch to fix the
existing notification coalescing feature, including:
- Unifying the virtio_net_ctrl_coal structs.
- Clarifying that the coalescing commands are best-effort.
- Specifying coalescing in respect to delivering interrupts when config
: Explain the dependency in a less confusing way.
Signed-off-by: Alvaro Karsz
device-types/net/description.tex | 6 ++
1 file changed, 6 insertions(+)
diff --git a/device-types/net/description.tex b/device-types/net/description.tex
index 700a1cb..1741c79 100644
--- a/device-types/net
-off-by: Alvaro Karsz
device-types/net/description.tex | 5 +
1 file changed, 5 insertions(+)
diff --git a/device-types/net/description.tex b/device-types/net/description.tex
index 700a1cb..4270481 100644
--- a/device-types/net/description.tex
+++ b/device-types/net/description.tex
@@ -256,6
If the VIRTIO_NET_F_HASH_REPORT feature is negotiated, the driver may
send VIRTIO_NET_CTRL_MQ_HASH_CONFIG commands, thus, the control VQ
feature must be negotiated.
Signed-off-by: Alvaro Karsz
---
device-types/net/description.tex | 1 +
1 file changed, 1 insertion(+)
diff --git a/device-types
> By comparison this patch is an attempt to offload ethtool's
> --coalesce parameters to the card. IMO what it misses is
> completeness, e.g. sample-interval is not specified.
AFAIK sample-interval refers to the adaptive coalescing
(adaptive-rx/adaptive-tx) and not to high/low.
Hi,
> > This patch adds a new feature, VIRTIO_NET_F_NOTF_COAL_LOW_HIGH, while
> > clarifying/fixing existing coalescing concepts.
> >
> While its visible in the patch itself, what is being done,
> its important to mention the problem statement/motivation for the feature at
> start of the commit
> > #define VIRTIO_NET_CTRL_NOTF_COAL 6
> > #define VIRTIO_NET_CTRL_NOTF_COAL_TX_SET 0
> > #define VIRTIO_NET_CTRL_NOTF_COAL_RX_SET 1
> > + #define VIRTIO_NET_CTRL_NOTF_COAL_LOW_TX_SET 2 //Only if
> > VIRTIO_NET_F_NOTF_COAL_LOW_HIGH negotiated
> > + #define
ping
-
To unsubscribe, e-mail: virtio-dev-unsubscr...@lists.oasis-open.org
For additional commands, e-mail: virtio-dev-h...@lists.oasis-open.org
oal structs, since the one for tx
> and the one for rx are identical.
> - Clarifies that the coalescing commands are best-effort.
> - Specifies coalescing in respect to delivering interrupts when config
> changes.
>
> Signed-off-by: Alvaro Karsz
-off-by: Alvaro Karsz
---
v2:
- Remove the "set of coalescing parameters" concept, use
"coalescing parameters" instead.
- Unify struct virtio_net_ctrl_coal_rx and strcut
virtio_net_ctrl_coal_tx
to struct virtio_net_ctrl_coal.
- Separat
> > is it really true we always want the same rate for tx and rx?
> > why not separate commands for each? this is the way we went
> > for general coalescing at least.
>
> I actually had ethtool in mind while writing the patch.
> AFAIK ethtool has a single rate for both rx and tx low/high, so using
Thanks for your comments.
> On Wed, Dec 21, 2022 at 12:17:29PM +0200, Alvaro Karsz wrote:
> > This patch adds a new feature VIRTIO_NET_F_NOTF_COAL_LOW_HIGH,
> > and adds 2 new commands to VIRTIO_NET_CTRL_NOTF_COAL class:
> > * VIRTIO_NET_CTRL_NOTF_COAL_LOW_SET -
Ping
-
To unsubscribe, e-mail: virtio-dev-unsubscr...@lists.oasis-open.org
For additional commands, e-mail: virtio-dev-h...@lists.oasis-open.org
Hi,
> I want to know which one is better than NetDim(Coalesce Adaptive) in driver.
>
> I know Heng Qi's work in this.
>
> Thanks
Why choose? we can have both.
ethtool can handle both pkt_rate_low/pkt_rate_high and
use_adaptive_rx_coalesce/use_adaptive_tx_coalesce.
The adaptive algorithm can
may have up to 3 sets, and should switch between them based on
the packet rate.
Signed-off-by: Alvaro Karsz
---
content.tex | 61 -
1 file changed, 60 insertions(+), 1 deletion(-)
diff --git a/content.tex b/content.tex
index 96f4723..969f945
75 matches
Mail list logo
