Re: [virtio-dev] RE: [virtio-comment] [PATCH] virtio-net: support per-queue coalescing moderation

2023-02-08 Thread Heng Qi




在 2023/2/8 下午11:04, Parav Pandit 写道:



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 +, Parav Pandit wrote:

From: Michael S. Tsirkin 
Sent: Wednesday, February 8, 2023 9:18 AM

On Wed, Feb 08, 2023 at 07:30:34PM +0800, Heng Qi wrote:

I see two options.
1. Just have per VQ params. Software has the full knowledge
of in which it is

operating, and state remains at software level.

This effectively achieves both the mode.

2. Have a mode cmd,
Mode = (a) per device or (b) per VQ (c) disable After the
mode is set, driver can set per device or per VQ.

I find this more clear.

Thanks.


Rereading this I think I misunderstood the proposal.
Now we are burning memory on maintaining mode, and this
information is duplicated.


It is not maintained in the pci resident memory, so it doesn't hurt.


I'd say let's just add a new command COAL_QUEUE_SET with vqn as

parameter.

Existing commands are simply defined as a shortcut to running
COAL_QUEUE_SET on all tx/rx queues respectively.

Latest command dictates the parameters. To disable just set
everything to 0 (btw we should make this explicit in the spec,
but it can be

guessed from:

Upon reset, a device MUST initialize all coalescing parameters to 0.
)


Switching between the modes (per q vs per device) implicitly is
ambiguous

and it only means device may need to iterate.

hmm i feel it's only ambiguous because i failed to explain in well.


This state is either better maintained in sw by always having per
vq or have

clearly defined mode of what device should do.

Per Q is very common even for several years old devices.
Last time I counted, there were at least 15 such devices supporting it.

So actual usage wise, I practically see that most implementations
will end up

with per vq mode.

I like to hear from Heng or Alvaro if they see any use of per device.


Right so given this, most devices will be in per queue mode all the
time. why do you want a mode then? just keep per queue.
existing commands are kept around for compat but internally just
translate to per-queue.

Since the space is not released, do we need to keep the compat?

It's been accepted for half a year so we can't say for sure no one built this.

That is likely but we should have the ability to have the Errata/ECN to correct 
it, specially for unrelease spec.


The way I propose is just a bit of firmware on device that scans all queues and
copies same parameters everywhere.

This scanning loop in sw appears cheaper to me than some embedded fw.
But is not a lot of concern.


Seems easier than worrying about this,
and we get disabling coalescing for free which you wanted. With an extra mode
its extra logic in the device fast path. Maybe it's cheap on hardware side but 
in
software it's an extra branch, not free.

Most performant data path wouldn't implement and read the extra mode.
It is always fw that is going to program same value, or per queue valued or 
disable value in each Q regardless whichever way we craft the CVQ cmd.

The sequence that bothers me is below.
1. driver set global params
2. few minutes later, now driver set param for Q=1

On this command, a device need to decide:
Should Q = 2 to N
(a) either work with previous globals, or
(b) because per Q was set for one queue, they rest of the queues implicitly 
disable it.

If it is (b),
When a command on Q object =1 is issued, it affects other Q objects. <- This I 
want to avoid.
A cmd that modifies the object, should only modify that object.

If it is (a), it is mixed mode operation, which is ambiguous definition.


I think it should be a. I think we should blur the concept of mode. 
There seems to be no mode here.
From the perspective of the device, it only needs to distinguish 
commands and do what it should do.


Thanks.



A better semantic is to define such change at device level and no extra cost in 
the data path.

-
To unsubscribe, e-mail: virtio-dev-unsubscr...@lists.oasis-open.org
For additional commands, e-mail: virtio-dev-h...@lists.oasis-open.org



-
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: [virtio-comment] [PATCH] virtio-net: support per-queue coalescing moderation

2023-02-08 Thread Heng Qi




在 2023/2/9 上午6:35, Alvaro Karsz 写道:

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 
feature

But which software (virtio net driver) in which OS is using this?

Sorry, I'm not sure I understand your question.

The feature is implemented in the linux kernel
https://github.com/torvalds/linux/commit/699b045a8e43bd1063db4795be685bfd659649dc
So we'll always have kernel versions accepting this feature, if offered.


(I will add support for the per vq command of course).
I really don't know about other vendors..

You are suggesting to reserve the command and feature bit for safety, so, if we
reserve them, why not just use them? What do we lose here?


If it is used by some unknown software, only that sw breaks by using non 
release spec.
If we use it by changing the definition, it may break that unknown sw.
If we know there is no known sw, we are better of with redefinition (by adding 
vqn, and by removing tx,rx from it).


Not having this feature/command even complicates things now that we are
talking about removing the RX and TX from the per vq command, how do you
change parameters to all TX queues? to all RX queues? we'll need 2 special
indexes, so we now need le32 to hold the queue index.

No need for special index.
How does a driver disable all queues or reset all queues? -> One by one.
So if user want to change for all TXQ, sw can do it one by one by iterating TXQ 
vqns.

Yes, but resetting the queues doesn't require a control command.
If a server has 64K queues, and a user wants to set all coalescing
parameters to X (maybe with ethtool), it will generate 64K control
commands..


At least our hardware design doesn't expect that.


-
To unsubscribe, e-mail: virtio-dev-unsubscr...@lists.oasis-open.org
For additional commands, e-mail: virtio-dev-h...@lists.oasis-open.org



-
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: [virtio-comment] [PATCH] virtio-net: support per-queue coalescing moderation

2023-02-08 Thread Heng Qi




在 2023/2/9 上午1:53, Alvaro Karsz 写道:

  > > 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 +, Parav Pandit wrote:

From: Michael S. Tsirkin 
Sent: Wednesday, February 8, 2023 9:18 AM

On Wed, Feb 08, 2023 at 07:30:34PM +0800, Heng Qi wrote:

I see two options.
1. Just have per VQ params. Software has the full knowledge
of in which it is

operating, and state remains at software level.

This effectively achieves both the mode.

2. Have a mode cmd,
Mode = (a) per device or (b) per VQ (c) disable After the
mode is set, driver can set per device or per VQ.

I find this more clear.

Thanks.


Rereading this I think I misunderstood the proposal.
Now we are burning memory on maintaining mode, and this
information is duplicated.


It is not maintained in the pci resident memory, so it doesn't hurt.


I'd say let's just add a new command COAL_QUEUE_SET with vqn as

parameter.

Existing commands are simply defined as a shortcut to running
COAL_QUEUE_SET on all tx/rx queues respectively.

Latest command dictates the parameters. To disable just set
everything to 0 (btw we should make this explicit in the spec,
but it can be

guessed from:

Upon reset, a device MUST initialize all coalescing parameters to 0.
)


Switching between the modes (per q vs per device) implicitly is
ambiguous

and it only means device may need to iterate.

hmm i feel it's only ambiguous because i failed to explain in well.


This state is either better maintained in sw by always having per
vq or have

clearly defined mode of what device should do.

Per Q is very common even for several years old devices.
Last time I counted, there were at least 15 such devices supporting it.

So actual usage wise, I practically see that most implementations
will end up

with per vq mode.

I like to hear from Heng or Alvaro if they see any use of per device.


Right so given this, most devices will be in per queue mode all the
time. why do you want a mode then? just keep per queue.
existing commands are kept around for compat but internally just
translate to per-queue.

Since the space is not released, do we need to keep the compat?

It's been accepted for half a year so we can't say for sure no one built this.

That is likely but we should have the ability to have the Errata/ECN to correct 
it, specially for unrelease spec.


The way I propose is just a bit of firmware on device that scans all queues and
copies same parameters everywhere.

This scanning loop in sw appears cheaper to me than some embedded fw.
But is not a lot of concern.


Seems easier than worrying about this,
and we get disabling coalescing for free which you wanted. With an extra mode
its extra logic in the device fast path. Maybe it's cheap on hardware side but 
in
software it's an extra branch, not free.

Most performant data path wouldn't implement and read the extra mode.
It is always fw that is going to program same value, or per queue valued or 
disable value in each Q regardless whichever way we craft the CVQ cmd.

The sequence that bothers me is below.
1. driver set global params
2. few minutes later, now driver set param for Q=1

On this command, a device need to decide:
Should Q = 2 to N
(a) either work with previous globals, or
(b) because per Q was set for one queue, they rest of the queues implicitly 
disable it.

If it is (b),
When a command on Q object =1 is issued, it affects other Q objects. <- This I 
want to avoid.
A cmd that modifies the object, should only modify that object.

If it is (a), it is mixed mode operation, which is ambiguous definition.

A better semantic is to define such change at device level and no extra cost in 
the data path.

I think that (a) is the way to go.
I don't think that we should work with operation modes at all.


I agree to keep the current global settings (VIRTIO_NET_F_NOTF_COAL and 
its corresponding commands),
because our hardware team has limited resources for the control queue, 
and they don't want to send a separate

cmd for each queue when send a global setting cmd.

Then adding a VIRTIO_NET_F_PERQUEUE_NOTF_COAL or 
VIRTIO_NET_F_VQ_NOTF_COAL feature bit and new commands for per-queue or 
VQ setting looks better to me.

In my opinion:

We should have 2 features:
VIRTIO_NET_F_PERQUEUE_NOTF_COAL and VIRTIO_NET_F_NOTF_COAL.

VIRTIO_NET_F_PERQUEUE_NOTF_COAL sets per queue parameters, and
VIRTIO_NET_F_NOTF_COAL sets parameters for all queues.

VIRTIO_NET_F_NOTF_COAL has 2 commands:
 VIRTIO_NET_CTRL_NOTF_COAL_RX_SET
 VIRTIO_NET_CTRL_NOTF_COAL_TX_SET

VIRTIO_NET_F_PERQUEUE_NOTF_COAL has 2 commands:
 VIRTIO_NET_CTRL_NOTF_COAL_PER_QUEUE_TX_SET
 VIRTIO_NET_CTRL_NOTF_COAL_PER_QUEUE_RX_SET

We can see VIRTIO_NET_CTRL_NOTF_COAL_RX_SET as a virtio level shortcut
for setting all queues with one command, exactly as intended with
rx_qid= 

[virtio-dev] Re: [virtio-comment] Re: [virtio-dev] Re: [virtio-comment] [PATCH] virtio-net: support per-queue coalescing moderation

2023-02-08 Thread Michael S. Tsirkin
On Wed, Feb 08, 2023 at 07:23:10PM +0800, Heng Qi wrote:
> 
> 
> 在 2023/2/8 下午6:04, Michael S. Tsirkin 写道:
> > On Wed, Feb 08, 2023 at 09:57:54AM +0800, Heng Qi wrote:
> > > 
> > > 在 2023/2/7 下午10:06, Michael S. Tsirkin 写道:
> > > > On Tue, Feb 07, 2023 at 07:16:34PM +0800, Heng Qi wrote:
> > > > > 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
> > > > > control coalescing parameters at the queue granularity.
> > > > ethtool does not support this though, does it? what's the plan?
> > > Yes, ethtool does not support this function yet, and this function will 
> > > not
> > > conflict with ethool.
> > > Our current goal is to let virtio-net support netdim first.
> > > 
> > > > > Signed-off-by: Heng Qi 
> > > > > Reviewed-by: Xuan Zhuo 
> > > > What I dislike about this interface is that if
> > > > VIRTIO_NET_F_PERQUEUE_NOTF_COAL is negotiated, then
> > > > in the common case of same parameters for all queues
> > > > driver has to issue multiple commands.
> > > > I can see either a special vq index (0x ?) or a special
> > > > command used to set it for all queues.
> > > > 
> > > > 
> > > > > ---
> > > > >content.tex | 49 ++---
> > > > >1 file changed, 42 insertions(+), 7 deletions(-)
> > > > > 
> > > > > diff --git a/content.tex b/content.tex
> > > > > index e863709..049c0e4 100644
> > > > > --- a/content.tex
> > > > > +++ b/content.tex
> > > > > @@ -3084,6 +3084,9 @@ \subsection{Feature bits}\label{sec:Device 
> > > > > Types / Network Device / Feature bits
> > > > >\item[VIRTIO_NET_F_CTRL_MAC_ADDR(23)] Set MAC address through 
> > > > > control
> > > > >channel.
> > > > > +\item[VIRTIO_NET_F_PERQUEUE_NOTF_COAL(52)] Device supports per-queue
> > > > > + 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 @@ \subsubsection{Feature bit 
> > > > > requirements}\label{sec:Device Types / Network Device
> > > > >\item[VIRTIO_NET_F_NOTF_COAL] Requires VIRTIO_NET_F_CTRL_VQ.
> > > > >\item[VIRTIO_NET_F_RSC_EXT] Requires VIRTIO_NET_F_HOST_TSO4 or 
> > > > > VIRTIO_NET_F_HOST_TSO6.
> > > > >\item[VIRTIO_NET_F_RSS] Requires VIRTIO_NET_F_CTRL_VQ.
> > > > > +\item[VIRTIO_NET_F_PERQUEUE_NOTF_COAL] Requires VIRTIO_NET_F_CTRL_VQ 
> > > > > and VIRTIO_NET_F_NOTF_COAL.
> > > > >\end{description}
> > > > >\subsubsection{Legacy Interface: Feature bits}\label{sec:Device 
> > > > > Types / Network Device / Feature bits / Legacy Interface: Feature 
> > > > > bits}
> > > > > @@ -4488,16 +4492,21 @@ \subsubsection{Control 
> > > > > Virtqueue}\label{sec:Device Types / Network Device / Devi
> > > > >If the VIRTIO_NET_F_NOTF_COAL feature is negotiated, the driver can
> > > > >send control commands for dynamically changing the coalescing 
> > > > > parameters.
> > > > > +If additionally VIRTIO_NET_F_PERQUEUE_NOTF_COAL is negotiated, the 
> > > > > driver
> > > > > +can send control commands to configure the coalescing parameters of a
> > > > > +specified receiveq or transmitq.
> > > > >\begin{lstlisting}
> > > > >struct virtio_net_ctrl_coal_rx {
> > > > >le32 rx_max_packets;
> > > > >le32 rx_usecs;
> > > > > +le16 rx_qid;  (Only if VIRTIO_NET_F_PERQUEUE_NOTF_COAL 
> > > > > negotiated)
> > > > >};
> > > > >struct virtio_net_ctrl_coal_tx {
> > > > >le32 tx_max_packets;
> > > > >le32 tx_usecs;
> > > > > +le16 tx_qid;  (Only if VIRTIO_NET_F_PERQUEUE_NOTF_COAL 
> > > > > negotiated)
> > > > >};
> > > > >#define VIRTIO_NET_CTRL_NOTF_COAL 6
> > > > I think it's a good idea to do this on top of Alvaro's patch
> > > > unifying these two structures.
> > > I saw Alvaro's patch, but it doesn't seem to be stable yet, is there a 
> > > good
> > > way for me to
> > > unify the two structures, since a patch should only do one thing.
> > > 
> > > > > @@ -4507,17 +4516,34 @@ \subsubsection{Control 
> > > > > Virtqueue}\label{sec:Device Types / Network Device / Devi
> > > > >Coalescing parameters:
> > > > >\begin{itemize}
> > > > > -\item \field{rx_usecs}: Maximum number of usecs to delay a RX 
> > > > > notification.
> > > > > -\item \field{tx_usecs}: Maximum number of usecs to delay a TX 
> > > > > notification.
> > > > > +\item \field{rx_qid}: Index of which receiveq to change the 
> > > > > coalescing parameters.
> > > > > + If the value is between 0 and 0x7FFF, it represents the index 
> > > > > of the specified
> > > > > + receiveq. Otherwise, if the value is 0x, it indicates to 
> > > > > change the

Re: [virtio-dev] Re: [virtio-comment] [PATCH] virtio-net: support per-queue coalescing moderation

2023-02-08 Thread Heng Qi




在 2023/2/8 下午6:10, Michael S. Tsirkin 写道:

On Wed, Feb 08, 2023 at 09:57:54AM +0800, Heng Qi wrote:

I think it's a good idea to do this on top of Alvaro's patch
unifying these two structures.

I saw Alvaro's patch, but it doesn't seem to be stable yet, is there a good
way for me to
unify the two structures, since a patch should only do one thing.

Problem is you were trying to change these existing structures too
so the patches conflicted. However I think at this point
we are in agreement on a new command with a new structure.
In this case there won't be a conflict as you are
not touching existing commands so no need for you
to depend on Alvaro's patch.


I get it.

Thanks.






-
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: [virtio-comment] [PATCH] virtio-net: support per-queue coalescing moderation

2023-02-08 Thread Michael S. Tsirkin
On Wed, Feb 08, 2023 at 09:57:54AM +0800, Heng Qi wrote:
> > I think it's a good idea to do this on top of Alvaro's patch
> > unifying these two structures.
> 
> I saw Alvaro's patch, but it doesn't seem to be stable yet, is there a good
> way for me to
> unify the two structures, since a patch should only do one thing.

Problem is you were trying to change these existing structures too
so the patches conflicted. However I think at this point
we are in agreement on a new command with a new structure.
In this case there won't be a conflict as you are
not touching existing commands so no need for you
to depend on Alvaro's patch.

-- 
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: [virtio-comment] [PATCH] virtio-net: support per-queue coalescing moderation

2023-02-08 Thread Michael S. Tsirkin
On Wed, Feb 08, 2023 at 09:57:54AM +0800, Heng Qi wrote:
> 
> 
> 在 2023/2/7 下午10:06, Michael S. Tsirkin 写道:
> > On Tue, Feb 07, 2023 at 07:16:34PM +0800, Heng Qi wrote:
> > > 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
> > > control coalescing parameters at the queue granularity.
> > ethtool does not support this though, does it? what's the plan?
> 
> Yes, ethtool does not support this function yet, and this function will not
> conflict with ethool.
> Our current goal is to let virtio-net support netdim first.
> 
> > 
> > > Signed-off-by: Heng Qi 
> > > Reviewed-by: Xuan Zhuo 
> > What I dislike about this interface is that if
> > VIRTIO_NET_F_PERQUEUE_NOTF_COAL is negotiated, then
> > in the common case of same parameters for all queues
> > driver has to issue multiple commands.
> > I can see either a special vq index (0x ?) or a special
> > command used to set it for all queues.
> > 
> > 
> > > ---
> > >   content.tex | 49 ++---
> > >   1 file changed, 42 insertions(+), 7 deletions(-)
> > > 
> > > diff --git a/content.tex b/content.tex
> > > index e863709..049c0e4 100644
> > > --- a/content.tex
> > > +++ b/content.tex
> > > @@ -3084,6 +3084,9 @@ \subsection{Feature bits}\label{sec:Device Types / 
> > > Network Device / Feature bits
> > >   \item[VIRTIO_NET_F_CTRL_MAC_ADDR(23)] Set MAC address through control
> > >   channel.
> > > +\item[VIRTIO_NET_F_PERQUEUE_NOTF_COAL(52)] Device supports per-queue
> > > + 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 @@ \subsubsection{Feature bit 
> > > requirements}\label{sec:Device Types / Network Device
> > >   \item[VIRTIO_NET_F_NOTF_COAL] Requires VIRTIO_NET_F_CTRL_VQ.
> > >   \item[VIRTIO_NET_F_RSC_EXT] Requires VIRTIO_NET_F_HOST_TSO4 or 
> > > VIRTIO_NET_F_HOST_TSO6.
> > >   \item[VIRTIO_NET_F_RSS] Requires VIRTIO_NET_F_CTRL_VQ.
> > > +\item[VIRTIO_NET_F_PERQUEUE_NOTF_COAL] Requires VIRTIO_NET_F_CTRL_VQ and 
> > > VIRTIO_NET_F_NOTF_COAL.
> > >   \end{description}
> > >   \subsubsection{Legacy Interface: Feature bits}\label{sec:Device Types / 
> > > Network Device / Feature bits / Legacy Interface: Feature bits}
> > > @@ -4488,16 +4492,21 @@ \subsubsection{Control 
> > > Virtqueue}\label{sec:Device Types / Network Device / Devi
> > >   If the VIRTIO_NET_F_NOTF_COAL feature is negotiated, the driver can
> > >   send control commands for dynamically changing the coalescing 
> > > parameters.
> > > +If additionally VIRTIO_NET_F_PERQUEUE_NOTF_COAL is negotiated, the driver
> > > +can send control commands to configure the coalescing parameters of a
> > > +specified receiveq or transmitq.
> > >   \begin{lstlisting}
> > >   struct virtio_net_ctrl_coal_rx {
> > >   le32 rx_max_packets;
> > >   le32 rx_usecs;
> > > +le16 rx_qid;  (Only if VIRTIO_NET_F_PERQUEUE_NOTF_COAL negotiated)
> > >   };
> > >   struct virtio_net_ctrl_coal_tx {
> > >   le32 tx_max_packets;
> > >   le32 tx_usecs;
> > > +le16 tx_qid;  (Only if VIRTIO_NET_F_PERQUEUE_NOTF_COAL negotiated)
> > >   };
> > >   #define VIRTIO_NET_CTRL_NOTF_COAL 6
> > I think it's a good idea to do this on top of Alvaro's patch
> > unifying these two structures.
> 
> I saw Alvaro's patch, but it doesn't seem to be stable yet, is there a good
> way for me to
> unify the two structures, since a patch should only do one thing.
> 
> > 
> > > @@ -4507,17 +4516,34 @@ \subsubsection{Control 
> > > Virtqueue}\label{sec:Device Types / Network Device / Devi
> > >   Coalescing parameters:
> > >   \begin{itemize}
> > > -\item \field{rx_usecs}: Maximum number of usecs to delay a RX 
> > > notification.
> > > -\item \field{tx_usecs}: Maximum number of usecs to delay a TX 
> > > notification.
> > > +\item \field{rx_qid}: Index of which receiveq to change the coalescing 
> > > parameters.
> > > + If the value is between 0 and 0x7FFF, it represents the index of the 
> > > specified
> > > + receiveq. Otherwise, if the value is 0x, it indicates to change the
> > > + coalescing parameters for all receiveqs.
> > what if the index does not map to a receive queue?
> 
> The spec mentioned
> "A device SHOULD respond to the VIRTIO_NET_CTRL_NOTF_COAL commands with
> VIRTIO_NET_ERR if it was not able to change the parameters.",
> I think this belongs to this situation.
> 
> Thanks.

Maybe but it's worth calling out. Because another interpretation
is that if qid matches rx queue we change rx parameters and
if it matches tx queue we change tx parameters.
There's a redundancy here I don't like different devices
might handle the error differently.

RE: [virtio-dev] RE: [virtio-comment] [PATCH] virtio-net: support per-queue coalescing moderation

2023-02-07 Thread Parav Pandit

> From: Heng Qi 
> Sent: Tuesday, February 7, 2023 9:36 PM
> 
> 
> 在 2023/2/8 上午10:20, Parav Pandit 写道:
> >> From: Xuan Zhuo 
> >> Sent: Tuesday, February 7, 2023 8:46 PM
> >>
> >> On Tue, 7 Feb 2023 09:06:19 -0500, "Michael S. Tsirkin"
> >> 
> >> wrote:
> >>> On Tue, Feb 07, 2023 at 07:16:34PM +0800, Heng Qi wrote:
>  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 control coalescing parameters at the queue granularity.
> >>> ethtool does not support this though, does it? what's the plan?
> >>
> >> Although it can be done, I think it is difficult to let users use
> >> ethtool to modify the parameters of each queue.
> >>
> >> At present ethtool supports adaptive-rx/adaptive-tx. This is that the
> >> driver automatically adapted to the appropriate parameter. Generally,
> >> it is implemented using netdim in the driver. At this time, this interface 
> >> is
> needed.
> >>
>  Signed-off-by: Heng Qi 
>  Reviewed-by: Xuan Zhuo 
> >>> What I dislike about this interface is that if
> >>> VIRTIO_NET_F_PERQUEUE_NOTF_COAL is negotiated, then in the common
> >> case
> >>> of same parameters for all queues driver has to issue multiple
> >>> commands.
> >>> I can see either a special vq index (0x ?) or a special command
> >>> used to set it for all queues.
> >> Although the structure is very similar, in fact, adding a new command
> >> may be clearer.
> > O(N) loop is not that bad when user want to issue change it per device, as 
> > this
> is something done very often.
> 
> However, a global configuration requires sending a command for each queue,
> which makes the device suffer.
> I think the hardware might not want this.
> 
There are many commands that a hw nic device needs to service.
So I agree that for high q count, a single command is better.
However, when a device is with such high q count, the possibility of needing it 
as per q params is also extremely high for best perf (and variance each q might 
have).
So couldn’t see a lot of value in doing as global param.

But yes, if it helps, a command with explicit mode to avoid ambiguity is fine 
to have.
Both approaches of [1] looks good to me.

[1] https://lists.oasis-open.org/archives/virtio-dev/202302/msg00088.html