RE: [virtio-dev] Re: [virtio-comment] Re: [PATCH v7] virtio-net: support inner header hash
> From: Heng Qi > Sent: Thursday, February 9, 2023 12:21 AM > Thanks for your reply. > >>> I had one last question. Why do we need to inform the > >> hash_report_tunnel_type of the outer header in the virtio_net_hdr? > >>> Is this for debug? Or is there a use case that will process this value? > >> The driver may use it to do some statistical information, or do some > >> rx classification based on the rx hash, and we'd better not hide > >> information from the driver. > >> > > Statistical information is better gathered via stats, instead of adding > > such code > in driver data path. > > A cautionary note : the source of hash_report_tunnel comes from the > discussion here > https://lists.oasis-open.org/archives/virtio-dev/202211/msg00063.html > If I understand correctly, if virtio-net-hdr also has hash_report_tunnel, how > does the driver do statistics? I am asking to avoid having hash_report_tunnel in virtio_net_hdr if it is only for the statistics purpose. If it has more use than just statistics, I like to understand its use in rx packet processing flow, if there is any and to describe in commit log too. Below [1] says about driver doing some rx classification. It is unclear on inner or outer. The response is not very clear to me, about how this field is useful. [1] https://lists.oasis-open.org/archives/virtio-dev/202211/msg00063.html If driver needs to know the statistics of it, it can query them from the device by introducing statistics cmd. - 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 v1] virtio-net: Define configuration field layout before its description
Hi Cornelia, > From: Parav Pandit > Sent: Wednesday, February 8, 2023 8:43 PM > > Currently some fields of the virtio_net_config structure are defined before > introducing the structure and some are defined after introducing > virtio_net_config. > Better to define the configuration layout first followed by description of > all the > fields. > > Device configuration fields are described in the section. Change wording from > 'listed' to 'described' as suggested in patch [1]. > > [1] https://lists.oasis-open.org/archives/virtio-dev/202302/msg4.html > > Signed-off-by: Parav Pandit > --- > changelog: > v0->v1: > - Change wording about device configuration field introduction > - remove duplicate read-only wording for status field > - reword sentence to read it better > --- This patch is superseded by v2 at [1]. Hence, please review [1]. [1] https://lists.oasis-open.org/archives/virtio-dev/202302/msg00163.html - 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] [PATCH v2] virtio-net: Define configuration field layout before its description
Currently some fields of the virtio_net_config structure are defined
before introducing the structure and some are defined after
introducing virtio_net_config.
Better to define the configuration layout first followed by
description of all the fields.
Device configuration fields are described in the section. Change wording
from 'listed' to 'described' as suggested in patch [1].
While rewording the configuration layout, mention only one time that all
the fields of the configuration layout are read-only. Remove read-only
wording from individual fields.
[1] https://lists.oasis-open.org/archives/virtio-dev/202302/msg4.html
Signed-off-by: Parav Pandit
---
changelog:
v1->v2:
- remove read-only wording from multiple places
v0->v1:
- Change wording about device configuration field introduction
- remove duplicate read-only wording for status field
- reword sentence to read it better
---
device-types/net/description.tex | 48 +---
1 file changed, 26 insertions(+), 22 deletions(-)
diff --git a/device-types/net/description.tex b/device-types/net/description.tex
index bdf4810..719ebd4 100644
--- a/device-types/net/description.tex
+++ b/device-types/net/description.tex
@@ -156,26 +156,43 @@ \subsubsection{Legacy Interface: Feature
bits}\label{sec:Device Types / Network
\subsection{Device configuration layout}\label{sec:Device Types / Network
Device / Device configuration layout}
\label{sec:Device Types / Block Device / Feature bits / Device configuration
layout}
-Device configuration fields are listed below, they are read-only for a driver.
The \field{mac} address field
-always exists (though is only valid if VIRTIO_NET_F_MAC is set), and
-\field{status} only exists if VIRTIO_NET_F_STATUS is set. Two
-read-only bits (for the driver) are currently defined for the status field:
-VIRTIO_NET_S_LINK_UP and VIRTIO_NET_S_ANNOUNCE.
+The network device has the following device configuration layout.
+These device configuration fields are read-only for the driver.
+
+\begin{lstlisting}
+struct virtio_net_config {
+u8 mac[6];
+le16 status;
+le16 max_virtqueue_pairs;
+le16 mtu;
+le32 speed;
+u8 duplex;
+u8 rss_max_key_size;
+le16 rss_max_indirection_table_length;
+le32 supported_hash_types;
+};
+\end{lstlisting}
+
+The \field{mac} address field always exists (although it is only
+valid if VIRTIO_NET_F_MAC is set).
+
+The \field{status} only exists if VIRTIO_NET_F_STATUS is set.
+Two bits are currently defined for the status field: VIRTIO_NET_S_LINK_UP
+and VIRTIO_NET_S_ANNOUNCE.
\begin{lstlisting}
#define VIRTIO_NET_S_LINK_UP 1
#define VIRTIO_NET_S_ANNOUNCE2
\end{lstlisting}
-The following driver-read-only field, \field{max_virtqueue_pairs} only exists
if
+The following field, \field{max_virtqueue_pairs} only exists if
VIRTIO_NET_F_MQ or VIRTIO_NET_F_RSS is set. This field specifies the maximum
number
of each of transmit and receive virtqueues (receiveq1\ldots receiveqN
and transmitq1\ldots transmitqN respectively) that can be configured once at
least one of these features
is negotiated.
-The following driver-read-only field, \field{mtu} only exists if
-VIRTIO_NET_F_MTU is set. This field specifies the maximum MTU for the driver to
-use.
+The following field, \field{mtu} only exists if VIRTIO_NET_F_MTU is set.
+This field specifies the maximum MTU for the driver to use.
The following two fields, \field{speed} and \field{duplex}, only
exist if VIRTIO_NET_F_SPEED_DUPLEX is set.
@@ -190,19 +207,6 @@ \subsection{Device configuration layout}\label{sec:Device
Types / Network Device
is expected to re-read these values after receiving a
configuration change notification.
-\begin{lstlisting}
-struct virtio_net_config {
-u8 mac[6];
-le16 status;
-le16 max_virtqueue_pairs;
-le16 mtu;
-le32 speed;
-u8 duplex;
-u8 rss_max_key_size;
-le16 rss_max_indirection_table_length;
-le32 supported_hash_types;
-};
-\end{lstlisting}
The following field, \field{rss_max_key_size} only exists if VIRTIO_NET_F_RSS
or VIRTIO_NET_F_HASH_REPORT is set.
It specifies the maximum supported length of RSS key in bytes.
--
2.26.2
-
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: RE: [virtio-comment] [PATCH v3 1/1] virtio-ism: introduce new device virtio-ism
On Thu, 9 Feb 2023 03:35:33 +, Parav Pandit wrote: > Hi Xuan, > > > From: virtio-comm...@lists.oasis-open.org > open.org> On Behalf Of Xuan Zhuo > > > conformance.tex | 26 +++ > > content.tex | 1 + > > virtio-ism.tex | 573 > > Can you please rebase your patches? > Devices are placed under their own directory and maintained in multiple > smaller files. > > device-types/ism/... OK, will fix. Thanks. - 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] [PATCH v3 1/1] virtio-ism: introduce new device virtio-ism
Hi Xuan, > From: virtio-comm...@lists.oasis-open.org open.org> On Behalf Of Xuan Zhuo > conformance.tex | 26 +++ > content.tex | 1 + > virtio-ism.tex | 573 Can you please rebase your patches? Devices are placed under their own directory and maintained in multiple smaller files. device-types/ism/... - 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/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
[virtio-dev] Re: [virtio-comment] [PATCH] virtio-net: support per-queue coalescing moderation
在 2023/2/9 上午4:52, Michael S. Tsirkin 写道: 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 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. 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= 0x, and without breaking devices following the current spec. The device's FW can decide if it stores parameters received with VIRTIO_NET_CTRL_NOTF_COAL_RX_SET in a global set, or if it iterates through all queues, but IMO the best way it to iterate through all queues. Seems like a win-win situation to me. We achieve the same functionality as
Re: [virtio-dev] Re: [virtio-comment] [PATCH] virtio-net: support per-queue coalescing moderation
在 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/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] [PATCH v1] virtio-net: Define configuration field layout before its description
Currently some fields of the virtio_net_config structure are defined
before introducing the structure and some are defined after
introducing virtio_net_config.
Better to define the configuration layout first followed by
description of all the fields.
Device configuration fields are described in the section. Change wording
from 'listed' to 'described' as suggested in patch [1].
[1] https://lists.oasis-open.org/archives/virtio-dev/202302/msg4.html
Signed-off-by: Parav Pandit
---
changelog:
v0->v1:
- Change wording about device configuration field introduction
- remove duplicate read-only wording for status field
- reword sentence to read it better
---
device-types/net/description.tex | 41 ++--
1 file changed, 23 insertions(+), 18 deletions(-)
diff --git a/device-types/net/description.tex b/device-types/net/description.tex
index bdf4810..e11856f 100644
--- a/device-types/net/description.tex
+++ b/device-types/net/description.tex
@@ -156,11 +156,29 @@ \subsubsection{Legacy Interface: Feature
bits}\label{sec:Device Types / Network
\subsection{Device configuration layout}\label{sec:Device Types / Network
Device / Device configuration layout}
\label{sec:Device Types / Block Device / Feature bits / Device configuration
layout}
-Device configuration fields are listed below, they are read-only for a driver.
The \field{mac} address field
-always exists (though is only valid if VIRTIO_NET_F_MAC is set), and
-\field{status} only exists if VIRTIO_NET_F_STATUS is set. Two
-read-only bits (for the driver) are currently defined for the status field:
-VIRTIO_NET_S_LINK_UP and VIRTIO_NET_S_ANNOUNCE.
+The network device has the following device configuration layout.
+These device configuration fields are read-only for the driver.
+
+\begin{lstlisting}
+struct virtio_net_config {
+u8 mac[6];
+le16 status;
+le16 max_virtqueue_pairs;
+le16 mtu;
+le32 speed;
+u8 duplex;
+u8 rss_max_key_size;
+le16 rss_max_indirection_table_length;
+le32 supported_hash_types;
+};
+\end{lstlisting}
+
+The \field{mac} address field always exists (although it is only
+valid if VIRTIO_NET_F_MAC is set).
+
+The \field{status} only exists if VIRTIO_NET_F_STATUS is set.
+Two bits are currently defined for the status field: VIRTIO_NET_S_LINK_UP
+and VIRTIO_NET_S_ANNOUNCE.
\begin{lstlisting}
#define VIRTIO_NET_S_LINK_UP 1
@@ -190,19 +208,6 @@ \subsection{Device configuration layout}\label{sec:Device
Types / Network Device
is expected to re-read these values after receiving a
configuration change notification.
-\begin{lstlisting}
-struct virtio_net_config {
-u8 mac[6];
-le16 status;
-le16 max_virtqueue_pairs;
-le16 mtu;
-le32 speed;
-u8 duplex;
-u8 rss_max_key_size;
-le16 rss_max_indirection_table_length;
-le32 supported_hash_types;
-};
-\end{lstlisting}
The following field, \field{rss_max_key_size} only exists if VIRTIO_NET_F_RSS
or VIRTIO_NET_F_HASH_REPORT is set.
It specifies the maximum supported length of RSS key in bytes.
--
2.26.2
-
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] [PATCH v3] virtio-net: Improve introductory description
The control VQ of the virtio network device is used beyond advance
steering control. The control VQ dynamically changes multiple features
of the initialized device.
Hence, update this area of control VQ introductory description at few
places and also place the link to its description.
Also update the introduction section to better describe receive and
transmit virtqueues.
Fixes: https://github.com/oasis-tcs/virtio-spec/issues/156
Signed-off-by: Parav Pandit
---
This patch is on top of [1].
[1] https://lists.oasis-open.org/archives/virtio-dev/202301/msg00280.html
---
changelog:
v2->v3:
- further improve introduction
v1->v2:
- corrected/added article before transmit and receive virtqueue
- replaced listed with described
- updated set to negotiated for CTRL_VQ
- improved introduction wordings
v0->v1:
- replaced command queue to control virtqueue to reflect current state
- added link of control vq to its detailed section
- removed reference of cvq from more places that limits it to steering
- Addressed Cornelia's review comments:
- updated introduction section to better describe rq, sq, cvq.
- explicitly wrote that cvq is optional along with its feature bit
related description
- dropped few items from Cornelia's review as they do not exactly
fit with latest 1.2 specification as below.
- avoided listing number of queues in introduction as its linked to q
reset which can disable all queues
- avoid mentioning cvq as optional as there is better section for it
- avoid talking about queue pairs as spec and use cases allows not
operate in pairs mode
---
device-types/net/description.tex | 23 +--
1 file changed, 13 insertions(+), 10 deletions(-)
diff --git a/device-types/net/description.tex b/device-types/net/description.tex
index 88a5770..bdf4810 100644
--- a/device-types/net/description.tex
+++ b/device-types/net/description.tex
@@ -2,13 +2,15 @@ \section{Network Device}\label{sec:Device Types / Network
Device}
The virtio network device is a virtual network interface controller.
It consists of a virtual Ethernet link which connects the device
-to the Ethernet network. It is the most complex of the devices
-supported so far by virtio. It has enhanced rapidly and demonstrates
-clearly how support for new features are added to an existing
-device. Empty buffers are placed in one virtqueue for receiving
-packets, and outgoing packets are enqueued into another for
-transmission in that order. A third command queue is used to
-control advanced filtering features.
+to the Ethernet network. The device has transmit and receive
+queues. The driver posts empty buffers in the receive virtqueue.
+The device receives the incoming packets from the link; the device
+places these incoming packets in the receive virtqueue buffers.
+The driver enqueues outgoing packets to the transmit virtqueue. The device
+dequeues these packets from the transmit virtqueue and sends them to
+the link. The device may have a control virtuqueue. The driver
+uses the control virtqueue to dynamically manipulate various
+features of the initialized device.
\subsection{Device ID}\label{sec:Device Types / Network Device / Device ID}
@@ -28,7 +30,8 @@ \subsection{Virtqueues}\label{sec:Device Types / Network
Device / Virtqueues}
N=1 if neither VIRTIO_NET_F_MQ nor VIRTIO_NET_F_RSS are negotiated, otherwise
N is set by
\field{max_virtqueue_pairs}.
- controlq only exists if VIRTIO_NET_F_CTRL_VQ set.
+controlq is optional; it only exists if VIRTIO_NET_F_CTRL_VQ is
+negotiated.
\subsection{Feature bits}\label{sec:Device Types / Network Device / Feature
bits}
@@ -389,8 +392,8 @@ \subsection{Device Operation}\label{sec:Device Types /
Network Device / Device O
};
\end{lstlisting}
-The controlq is used to control device features such as
-filtering.
+The controlq is used to control many device features described further in
+section \ref{sec:Device Types / Network Device / Device Operation / Control
Virtqueue}.
\subsubsection{Legacy Interface: Device Operation}\label{sec:Device Types /
Network Device / Device Operation / Legacy Interface: Device Operation}
When using the legacy interface, transitional devices and drivers
--
2.26.2
-
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 v2] virtio-net: Improve introductory description
> From: Michael S. Tsirkin > Sent: Wednesday, February 8, 2023 4:46 AM [..] > > > General question: Is it better to talk about "the receive virtqueue" > > > (to keep it simple), or "a receive virtqueue" (as there may be several)? > > > > For sure Michael can better answer this than me. :) The receive > > virtqueue aligns to me with "the device", the driver etc. > > It also aligns with existing sections in device and driver requirements that > refers to "the receiveq". > > Since there are many queues, better to refer as "the". > > A receive queue may imply that there is only one. > > My take is that it should be "a queue" the first time we talk about it. Then > if it's > the same queue we say "the queue". > I also feel an intro sentence saying something along the lines of "the device > has > receive and transmit queues". Then rest of uses can be "the". Will update this in v3. - 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] [PATCH] virtio-net: support per-queue coalescing moderation
> From: Alvaro Karsz > Sent: Wednesday, February 8, 2023 5:36 PM > > > > 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 Ok. so, there is sw using it. This global feature bit stays. > > 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.. He he. What an example. 64K control cmds. 64 register writes and poll. All same from sw pov. The conclusion is there is sw using the feature bit defined in spec 1.3. Discussion ends. I wish you pointed to this sw way earlier in the discussion. Please proceed with the new feature bit and new command. - 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] [PATCH] virtio-net: support per-queue coalescing moderation
On Thu, Feb 09, 2023 at 12:35:42AM +0200, Alvaro Karsz wrote: > > > 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.. Amount of time spent on bike-shedding here is mind-boggling. Pls just send a new version and we'll go from there. In the end it's a majority we don't have to have consensus if we can't get it. -- 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] [PATCH] virtio-net: support per-queue coalescing moderation
On Thu, Feb 09, 2023 at 12:45:42AM +0200, Alvaro Karsz wrote: > 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 > to something like VIRTIO_NET_F_ADVANCED_NOTF_COAL, and introduce a new > bitmask in the virto_net_config. > > We can implement just one bit at first. > BIT 0 - device supports per queue coalescing sets > > Then, we could add more features in the future without having to use > more feature bits. > > Every device that offers VIRTIO_NET_F_ADVANCED_NOTF_COAL should > maintain a valid bitmask. Without at least two bits there it sounds like over-engineering to me: we won't really know whether the fancy infrastructure is good for any extensions. -- 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] [PATCH] virtio-net: support per-queue coalescing moderation
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 to something like VIRTIO_NET_F_ADVANCED_NOTF_COAL, and introduce a new bitmask in the virto_net_config. We can implement just one bit at first. BIT 0 - device supports per queue coalescing sets Then, we could add more features in the future without having to use more feature bits. Every device that offers VIRTIO_NET_F_ADVANCED_NOTF_COAL should maintain a valid bitmask. - 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] [PATCH] virtio-net: support per-queue coalescing moderation
> > 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.. - 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] [PATCH] virtio-net: support per-queue coalescing moderation
> From: Michael S. Tsirkin > Sent: Wednesday, February 8, 2023 5:29 PM > > On Wed, Feb 08, 2023 at 10:23:00PM +, Parav Pandit wrote: > > > From: Michael S. Tsirkin > > > Sent: Wednesday, February 8, 2023 5:16 PM > > > > > 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. > > > > > > I think we can split this effort in two parts: > > > > > > 1. add a new command and feature bit 2. drop the old command > > > > > > > > > Looks like there's consensus on 1? And it looks like Alvaro wants > > > to work on 1 but not 2. Once that is in Parav can work on 2 if he wants > > > to. > > > > 3. rename the current one if there is no sw driver that used it. > > That's probably the worst of all options, silently changing behaviour. > How come? Once #2 is done, only #1 is left and that is equivalent silence. It is effectively #3. So why not do in same series? > > May be lets wait for Alvaro's answer if there is any sw that used. > > > > Doing #3 would be the simplest for all devices and sw to implement and > consume respectively. > > > > I can work with Alvaro to have 1 and 2 as part of same series as two > > patches, > so that we don't have to re-discuss this again. > > Without that > > a. negotiating two bits at same time is hard. > > b. Wording #1 also need to mention about what happens when global is also > negotiated and device is in partial mode. > > There's no mode, partial or otherwise, in either Alvaro's or my proposal. It is clearly ambitious spec for some unknown software to be in mixed mode that unlikely to find user for. - 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] [PATCH] virtio-net: support per-queue coalescing moderation
On Wed, Feb 08, 2023 at 10:23:00PM +, Parav Pandit wrote: > > From: Michael S. Tsirkin > > Sent: Wednesday, February 8, 2023 5:16 PM > > > > 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. > > > > I think we can split this effort in two parts: > > > > 1. add a new command and feature bit > > 2. drop the old command > > > > > > Looks like there's consensus on 1? And it looks like Alvaro wants to work > > on 1 > > but not 2. Once that is in Parav can work on 2 if he wants to. > > 3. rename the current one if there is no sw driver that used it. That's probably the worst of all options, silently changing behaviour. > May be lets wait for Alvaro's answer if there is any sw that used. > > Doing #3 would be the simplest for all devices and sw to implement and > consume respectively. > > I can work with Alvaro to have 1 and 2 as part of same series as two patches, > so that we don't have to re-discuss this again. > Without that > a. negotiating two bits at same time is hard. > b. Wording #1 also need to mention about what happens when global is also > negotiated and device is in partial mode. There's no mode, partial or otherwise, in either Alvaro's or my proposal. -- 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] [PATCH] virtio-net: support per-queue coalescing moderation
> From: Michael S. Tsirkin > Sent: Wednesday, February 8, 2023 5:16 PM > > > 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. > > I think we can split this effort in two parts: > > 1. add a new command and feature bit > 2. drop the old command > > > Looks like there's consensus on 1? And it looks like Alvaro wants to work on > 1 > but not 2. Once that is in Parav can work on 2 if he wants to. 3. rename the current one if there is no sw driver that used it. May be lets wait for Alvaro's answer if there is any sw that used. Doing #3 would be the simplest for all devices and sw to implement and consume respectively. I can work with Alvaro to have 1 and 2 as part of same series as two patches, so that we don't have to re-discuss this again. Without that a. negotiating two bits at same time is hard. b. Wording #1 also need to mention about what happens when global is also negotiated and device is in partial mode. - 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] [PATCH] virtio-net: support per-queue coalescing moderation
On Wed, Feb 08, 2023 at 10:08:41PM +, Parav Pandit wrote: > > > 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? > > > (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. I think we can split this effort in two parts: 1. add a new command and feature bit 2. drop the old command Looks like there's consensus on 1? And it looks like Alvaro wants to work on 1 but not 2. Once that is in Parav can work on 2 if he wants to. -- 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] [PATCH] virtio-net: support per-queue coalescing moderation
On Wed, Feb 08, 2023 at 09:05:47PM +, Parav Pandit wrote: > > > 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 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 > > > >
[virtio-dev] RE: [virtio-comment] [PATCH] virtio-net: support per-queue coalescing moderation
> 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? > (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.
[virtio-dev] Re: [virtio-comment] [PATCH] virtio-net: support per-queue coalescing moderation
> > 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 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
[virtio-dev] Re: [virtio-comment] [PATCH] virtio-net: support per-queue coalescing moderation
> 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 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. > > > > > >
[virtio-dev] RE: [virtio-comment] [PATCH] virtio-net: support per-queue coalescing moderation
> 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 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
[virtio-dev] Re: [virtio-comment] [PATCH] virtio-net: support per-queue coalescing moderation
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 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
[virtio-dev] Re: [virtio-comment] [PATCH] virtio-net: support per-queue coalescing moderation
On Wed, Feb 08, 2023 at 07:23:12PM +, Parav Pandit wrote: > You are right. Two commands are there. > Regardless the ambiguity remains > ━━━ > From: Parav Pandit > Sent: Wednesday, February 8, 2023 8:57 PM > To: Michael S. Tsirkin > Cc: Heng Qi ; Xuan Zhuo > ; > virtio-comment @ lists . oasis-open . org > ; virtio-dev @ lists . oasis-open . org > ; Jason Wang ; Alvaro > Karsz > Subject: RE: [virtio-comment] [PATCH] virtio-net: support per-queue coalescing > moderation > > > > From: Michael S. Tsirkin > > Sent: Wednesday, February 8, 2023 10:20 AM > > [..] > > > > > > > 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. > > > > Ugh. Looks like I didn't explain it well, yet again :(. > > Here is my proposal in pseudo-code: > > > > > > if (cmd == VQ_SET) > >vq[cmd.index].param = cmd.param; > > > > if (cmd == TX_SET) > >for (i = 0; ++i; i < maxvqn / 2) > >vq[i * 2].param = cmd.param; > > > > if (cmd == RX_SET) > >for (i = 0; ++i; i < maxvqn / 2) > >vq[i * 2 + 1].param = cmd.param; > > > > > Currently we have cmd = GLOBAL_SET the code is: > > if (cmd == GLOBAL_SET) > for (i = 0; i < maxvqn; i++) > vq[i].param = cmd.param; exactly. > > > > there's nothing to decide at all. No modes. TX_SET and RX_SET affect half > vqs, > > VQ_SET affects one vq. > > > I explained above that, when cmd=GLOBAL_SET and after that if driver issues > cmd > =VQ_SET, at that point, the device is in partial mode. > One VQ is running with its own param, and rest are in global mode. Because there's no "global mode" - it is always per queue and each queue uses a set of parameters, that is all. No ambiguity. You can quickly set them all with a special command, or you can set them one by one. For example consider a driver that loops over all vqs and does RX_SET(0). Are we in global mode, per queue mode or disabled mode? The questions is pointless is it not? -- 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] [PATCH] virtio-net: support per-queue coalescing moderation
You are right. Two commands are there. Regardless the ambiguity remains From: Parav Pandit Sent: Wednesday, February 8, 2023 8:57 PM To: Michael S. Tsirkin Cc: Heng Qi ; Xuan Zhuo ; virtio-comment @ lists . oasis-open . org ; virtio-dev @ lists . oasis-open . org ; Jason Wang ; Alvaro Karsz Subject: RE: [virtio-comment] [PATCH] virtio-net: support per-queue coalescing moderation > From: Michael S. Tsirkin > Sent: Wednesday, February 8, 2023 10:20 AM [..] > > > > 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. > > Ugh. Looks like I didn't explain it well, yet again :(. > Here is my proposal in pseudo-code: > > > if (cmd == VQ_SET) >vq[cmd.index].param = cmd.param; > > if (cmd == TX_SET) >for (i = 0; ++i; i < maxvqn / 2) >vq[i * 2].param = cmd.param; > > if (cmd == RX_SET) >for (i = 0; ++i; i < maxvqn / 2) >vq[i * 2 + 1].param = cmd.param; > > Currently we have cmd = GLOBAL_SET the code is: if (cmd == GLOBAL_SET) for (i = 0; i < maxvqn; i++) vq[i].param = cmd.param; > > there's nothing to decide at all. No modes. TX_SET and RX_SET affect half vqs, > VQ_SET affects one vq. > I explained above that, when cmd=GLOBAL_SET and after that if driver issues cmd=VQ_SET, at that point, the device is in partial mode. One VQ is running with its own param, and rest are in global mode.
[virtio-dev] Re: [virtio-comment] RE: [virtio-dev] [PATCH] virtio-net: Fix and update VIRTIO_NET_F_NOTF_COAL feature
> > 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? > > > > > > > > Because, you might want to migrate from hardware with to hardware > > > > without coalescing features. So you just tell guest "sure I will > > > > coalesce" but in fact send interrupts normally. > > > > > > For the hardware that has fake coalescing, HV wouldn't know it anyway > > without doing pre verification. > > > And HV may not migrate in such case for best experience. > > > HV may choose to migrate with low accuracy as you say, which is fine. > > > > > > But the spec guidance for the device implementations is to promote some > > reasonable level of accuracy. > > > Hard to define in words here. > > > Best effort is wide spectrum of range. :) > > > > > > Typically, we say in the spec as SHOULD. > > > So, lets skip the best-effort wording and stick to SHOULD part like rest > > > of the > > spec. > > > > I think the point of best-effort is that driver must handle interrupts that > > arrive > > earlier. > Ok. so Let's put this must requirement in the driver section like the rest. But this requirement is already written in "Driver Requirements: Used Buffer Notification Suppression". Handling spurious interrupts is not NOTF_COAL specific, but generic, regardless of NOTF_COAL. So I don't think that this should be written in the NOTF_COAL driver section. > > This is how we used it elsewhere. > In a quick grep I see best effort shows two matches one for rx filter and one > for vlan. > Vlan we lately know was (close) to incorrect. > > > What else does it include in your > > opinion that we absolutely must exclude? > > I feel it's a good fit for a non-conformance section which is by nature a > > bit > > informal. > > > > For a conformance section SHOULD is indeed a good fit. > > > Yes, must in driver section and should in device section looks good to me too. The whole NOTF_COAL section is about explaining when a device should issue an interrupt and when not, so writing "The device SHOULD NOT issue an interrupt when it's not supposed to" seems a bit redundant 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
[virtio-dev] Re: [virtio-comment] [PATCH] virtio-net: support per-queue coalescing moderation
> > 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. 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-dev] RE: [virtio-comment] RE: [virtio-dev] [PATCH] virtio-net: Fix and update VIRTIO_NET_F_NOTF_COAL feature
> 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? > > > > > > Because, you might want to migrate from hardware with to hardware > > > without coalescing features. So you just tell guest "sure I will > > > coalesce" but in fact send interrupts normally. > > > > For the hardware that has fake coalescing, HV wouldn't know it anyway > without doing pre verification. > > And HV may not migrate in such case for best experience. > > HV may choose to migrate with low accuracy as you say, which is fine. > > > > But the spec guidance for the device implementations is to promote some > reasonable level of accuracy. > > Hard to define in words here. > > Best effort is wide spectrum of range. :) > > > > Typically, we say in the spec as SHOULD. > > So, lets skip the best-effort wording and stick to SHOULD part like rest of > > the > spec. > > I think the point of best-effort is that driver must handle interrupts that > arrive > earlier. Ok. so Let's put this must requirement in the driver section like the rest. > This is how we used it elsewhere. In a quick grep I see best effort shows two matches one for rx filter and one for vlan. Vlan we lately know was (close) to incorrect. > What else does it include in your > opinion that we absolutely must exclude? > I feel it's a good fit for a non-conformance section which is by nature a bit > informal. > > For a conformance section SHOULD is indeed a good fit. > Yes, must in driver section and should in device section looks good to me too. - 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] [PATCH] virtio-net: Fix and update VIRTIO_NET_F_NOTF_COAL feature
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? > > > > Because, you might want to migrate from hardware with to hardware without > > coalescing features. So you just tell guest "sure I will coalesce" but in > > fact send > > interrupts normally. > > For the hardware that has fake coalescing, HV wouldn't know it anyway without > doing pre verification. > And HV may not migrate in such case for best experience. > HV may choose to migrate with low accuracy as you say, which is fine. > > But the spec guidance for the device implementations is to promote some > reasonable level of accuracy. > Hard to define in words here. > Best effort is wide spectrum of range. :) > > Typically, we say in the spec as SHOULD. > So, lets skip the best-effort wording and stick to SHOULD part like rest of > the spec. I think the point of best-effort is that driver must handle interrupts that arrive earlier. This is how we used it elsewhere. What else does it include in your opinion that we absolutely must exclude? I feel it's a good fit for a non-conformance section which is by nature a bit informal. For a conformance section SHOULD is indeed a good fit. -- 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] [PATCH] virtio-net: Fix and update VIRTIO_NET_F_NOTF_COAL feature
> From: Michael S. Tsirkin > Sent: Tuesday, February 7, 2023 9:18 AM > > Why migration generate too many spurious interrupts? > > Because, you might want to migrate from hardware with to hardware without > coalescing features. So you just tell guest "sure I will coalesce" but in > fact send > interrupts normally. For the hardware that has fake coalescing, HV wouldn't know it anyway without doing pre verification. And HV may not migrate in such case for best experience. HV may choose to migrate with low accuracy as you say, which is fine. But the spec guidance for the device implementations is to promote some reasonable level of accuracy. Hard to define in words here. Best effort is wide spectrum of range. :) Typically, we say in the spec as SHOULD. So, lets skip the best-effort wording and stick to SHOULD part like rest of the spec. - 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] [PATCH] virtio-net: support per-queue coalescing moderation
> From: Michael S. Tsirkin > Sent: Wednesday, February 8, 2023 10:20 AM [..] > > > > 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. > > Ugh. Looks like I didn't explain it well, yet again :(. > Here is my proposal in pseudo-code: > > > if (cmd == VQ_SET) > vq[cmd.index].param = cmd.param; > > if (cmd == TX_SET) > for (i = 0; ++i; i < maxvqn / 2) > vq[i * 2].param = cmd.param; > > if (cmd == RX_SET) > for (i = 0; ++i; i < maxvqn / 2) > vq[i * 2 + 1].param = cmd.param; > > Currently we have cmd = GLOBAL_SET the code is: if (cmd == GLOBAL_SET) for (i = 0; i < maxvqn; i++) vq[i].param = cmd.param; > > there's nothing to decide at all. No modes. TX_SET and RX_SET affect half vqs, > VQ_SET affects one vq. > I explained above that, when cmd=GLOBAL_SET and after that if driver issues cmd=VQ_SET, at that point, the device is in partial mode. One VQ is running with its own param, and rest are in global mode. - 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] [PATCH] virtio-net: support per-queue coalescing moderation
On Wed, Feb 08, 2023 at 03:04:03PM +, Parav Pandit 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 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. There's an errata process for sure. > > 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. Ugh. Looks like I didn't explain it well, yet again :(. Here is my proposal in pseudo-code: if (cmd == VQ_SET) vq[cmd.index].param = cmd.param; if (cmd == TX_SET) for (i = 0; ++i; i < maxvqn / 2) vq[i * 2].param =
[virtio-dev] RE: [virtio-comment] [PATCH] virtio-net: support per-queue coalescing moderation
> 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. - 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] [PATCH] virtio-net: support per-queue coalescing moderation
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. The way I propose is just a bit of firmware on device that scans all queues and copies same parameters everywhere. 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. -- 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] [PATCH] virtio-net: support per-queue coalescing moderation
> 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? - 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] [PATCH] virtio-net: support per-queue coalescing moderation
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. It can't reasonably be read from RAM at each packet, you need to cache it in memory. - 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] [PATCH] virtio-net: support per-queue coalescing moderation
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. -- 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] [PATCH] virtio-net: support per-queue coalescing moderation
> 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. 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. - 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: [virtio-comment] Re: [PATCH v7] virtio-net: support inner header hash
> From: Michael S. Tsirkin > Sent: Wednesday, February 8, 2023 9:09 AM > > > header: it allow users inside the tunnel control queueing outside. > > > By observing packet loss some information leaks between tunnels. > > > > > I likely didn't understand. Can you please explain? > > > > Queuing is always done on the inner header with/without encapsulation. > > Hash is always reported for inner header. > > It is only adding the ability to hash even when outer header exists. > > > If hashing just on outer header (currently the only option) then a given > tunnel > all lands in a given queue. > Just keep that queue separate and users of this tunnel can not learn whether > other queues are overflowing, and can not overflow other queues. > > > If you hash inner header then user can flood device with packets of a given > connection and the same connection in a different tunnel hashes to the same > queue. Now one tunnel can > - cause DoS for another tunnel > - cause packet loss or latency triggering possible security bugs within guest > - detect that another tunnel is using the connection by > detecting its own packet loss or increased latency > Yes. It can lead to above issues. Steering on inner is on best effort based sw implementations running on top of net device. To avoid above issues, a hierarchical model is needed. I am not aware of any. To my knowledge, usually who care for above issues end up using a different net device for each VNI and achieve the desired hierarchy. > > > If queuing to be decided based on outer header (hash), then that is > > different. > > Hashing both inner and outer in a flat q structure unlikely works, right? > > Because both hashes can result in different q selection. > > > That's the point. > > Is there any precedent in OSes for configuring things like this that we can > look > at? > ethtool -N (not yet part of virtio) is the closest match that can steer based on inner and outer both, but it is not hierarchical, and it is orthogonal to this feature. > > > > > > > Ideas for solving this they all involve hashing both inner and outer > > > header: > > > 1- report two sets of hashes. overkill? > > > 2- hash both headers together > > > 2- add salt. can come from driver or device itself > > > > > > More ideas? > > > > > > -- > > > 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] VirtIO Sound (audio control support)
Hi Sameer, On 08.02.2023 14:51, Sameer Pujar wrote: Thanks Anton for your response. On 08-02-2023 18:46, Anton Yakovlev wrote: Hi Sameer, We were waiting for the extension for audio controls to be accepted into the VIRTIO specification. Is audio control extension proposal to VIRTIO sound specification accepted? Yes, it is: https://www.oasis-open.org/committees/ballot.php?id=3709 Also, Michael pointed to the right patch for the spec. We recently resumed our upstreaming efforts and a colleague of mine should resubmit the patch soon. I will look forward to it and can probably help in testing this. Great, it would be helpful! -- Anton Yakovlev Senior Software Engineer OpenSynergy GmbH Rotherstr. 20, 10245 Berlin - 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] [PATCH] virtio-net: support per-queue coalescing moderation
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. 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. ) -- 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: [virtio-comment] Re: [PATCH v7] virtio-net: support inner header hash
On Wed, Feb 08, 2023 at 02:05:52PM +, Parav Pandit wrote: > > From: Michael S. Tsirkin > > Sent: Wednesday, February 8, 2023 8:52 AM > > > > On Wed, Feb 08, 2023 at 01:38:36PM +, Parav Pandit wrote: > > > > > > > From: Michael S. Tsirkin > > > > Sent: Wednesday, February 8, 2023 8:32 AM > > > > > > > > On Wed, Feb 08, 2023 at 05:18:32AM +, Parav Pandit wrote: > > > > > > From: Heng Qi > > > > > > Sent: Tuesday, February 7, 2023 10:25 PM > > > > > > > > > > [..] > > > > > > >> > > > > > > >> Do you think we need both hash_types and hash_tunnel_types? > > > > > > > In struct virtio_net_config we need two fields. > > > > > > > a. supported_hash_types (already exists) b. > > > > > > > supported_hash_tunnel_type > > > > > > > -> bitmap indicating for which outer headers, inner hash > > > > > > > -> calculation is > > > > > > supported. > > > > > > > > > > > > Thanks for the suggestion, we seem to have reached an agreement. > > > > > > > > > > > > > > > > > > > > In struct virtio_net_hdr we need two fields. > > > > > > > a. hash_report (already exists) b. hash_tunnel_type 8 bits -> > > > > > > > absolute value indicating which outer header > > > > > > exists when inner header hash calculated. > > > > > > > You already have it in your patch named as hash_report_tunnel. > > > > > > > May be better to name as hash_report_tunnel_type to make it > > > > > > > clearer that its > > > > > > type. > > > > > > > > > > > > Sure. > > > > > > > > > > > > Thanks for your reply. > > > > > > > > > > I had one last question. Why do we need to inform the > > > > hash_report_tunnel_type of the outer header in the virtio_net_hdr? > > > > > Is this for debug? Or is there a use case that will process this > > > > > value? > > > > > > > > Well we have hash_report which is kind of similar (and also kind of > > > > pointless but I think it's there because WHQL wants it). > > > Hash_report is useful. It tells hash_value is in which namespace > > > (ipv4-tcp/ipv4 > > udp etc). > > > OS can use this value to find tcp connection in a given namespace. > > > > > > > Maybe we can steal some bits > > > > from there instead of a new field? > > > > > > > I do not have problem adding extra bits. I just don't find that just > > > telling that > > its vxlan or nvgre to the OS is useful. > > > If OS needs to know about outer header details, it needs to know the VNI > > information than just telling vxlan. > > > > This does make sense. > > > > > > > > > > > > I have a follow up question though: are we only hashing the inner > > > > header or both inner and outer header? Somewhat confused on this. > > > > > > > I understood as inner header. But worth to describe it. May be there. > > > Need to > > read v8 patch. > > > > Hmm. I just realized that there's a security problem with hashing just the > > inner > > header: it allow users inside the tunnel control queueing outside. > > By observing packet loss some information leaks between tunnels. > > > Ah I know now. > We are leaking outer header information inside the virtio net hdr, and outer > header might be already stripped off by a different entity. > > I think the use case here is it's the same sw entity that owns the virtio net > device does the encap/decap too. No not exactly, we are leaking info between encap tunnels. -- 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: [virtio-comment] Re: [PATCH v7] virtio-net: support inner header hash
On Wed, Feb 08, 2023 at 02:00:14PM +, Parav Pandit wrote: > > From: Michael S. Tsirkin > > Sent: Wednesday, February 8, 2023 8:52 AM > > > > On Wed, Feb 08, 2023 at 01:38:36PM +, Parav Pandit wrote: > > > > > > > From: Michael S. Tsirkin > > > > Sent: Wednesday, February 8, 2023 8:32 AM > > > > > > > > On Wed, Feb 08, 2023 at 05:18:32AM +, Parav Pandit wrote: > > > > > > From: Heng Qi > > > > > > Sent: Tuesday, February 7, 2023 10:25 PM > > > > > > > > > > [..] > > > > > > >> > > > > > > >> Do you think we need both hash_types and hash_tunnel_types? > > > > > > > In struct virtio_net_config we need two fields. > > > > > > > a. supported_hash_types (already exists) b. > > > > > > > supported_hash_tunnel_type > > > > > > > -> bitmap indicating for which outer headers, inner hash > > > > > > > -> calculation is > > > > > > supported. > > > > > > > > > > > > Thanks for the suggestion, we seem to have reached an agreement. > > > > > > > > > > > > > > > > > > > > In struct virtio_net_hdr we need two fields. > > > > > > > a. hash_report (already exists) b. hash_tunnel_type 8 bits -> > > > > > > > absolute value indicating which outer header > > > > > > exists when inner header hash calculated. > > > > > > > You already have it in your patch named as hash_report_tunnel. > > > > > > > May be better to name as hash_report_tunnel_type to make it > > > > > > > clearer that its > > > > > > type. > > > > > > > > > > > > Sure. > > > > > > > > > > > > Thanks for your reply. > > > > > > > > > > I had one last question. Why do we need to inform the > > > > hash_report_tunnel_type of the outer header in the virtio_net_hdr? > > > > > Is this for debug? Or is there a use case that will process this > > > > > value? > > > > > > > > Well we have hash_report which is kind of similar (and also kind of > > > > pointless but I think it's there because WHQL wants it). > > > Hash_report is useful. It tells hash_value is in which namespace > > > (ipv4-tcp/ipv4 > > udp etc). > > > OS can use this value to find tcp connection in a given namespace. > > > > > > > Maybe we can steal some bits > > > > from there instead of a new field? > > > > > > > I do not have problem adding extra bits. I just don't find that just > > > telling that > > its vxlan or nvgre to the OS is useful. > > > If OS needs to know about outer header details, it needs to know the VNI > > information than just telling vxlan. > > > > This does make sense. > > > > > > > > > > > > I have a follow up question though: are we only hashing the inner > > > > header or both inner and outer header? Somewhat confused on this. > > > > > > > I understood as inner header. But worth to describe it. May be there. > > > Need to > > read v8 patch. > > > > Hmm. I just realized that there's a security problem with hashing just the > > inner > > header: it allow users inside the tunnel control queueing outside. > > By observing packet loss some information leaks between tunnels. > > > I likely didn't understand. Can you please explain? > > Queuing is always done on the inner header with/without encapsulation. > Hash is always reported for inner header. > It is only adding the ability to hash even when outer header exists. If hashing just on outer header (currently the only option) then a given tunnel all lands in a given queue. Just keep that queue separate and users of this tunnel can not learn whether other queues are overflowing, and can not overflow other queues. If you hash inner header then user can flood device with packets of a given connection and the same connection in a different tunnel hashes to the same queue. Now one tunnel can - cause DoS for another tunnel - cause packet loss or latency triggering possible security bugs within guest - detect that another tunnel is using the connection by detecting its own packet loss or increased latency > If queuing to be decided based on outer header (hash), then that is different. > Hashing both inner and outer in a flat q structure unlikely works, right? > Because both hashes can result in different q selection. That's the point. Is there any precedent in OSes for configuring things like this that we can look at? > > > > Ideas for solving this they all involve hashing both inner and outer > > header: > > 1- report two sets of hashes. overkill? > > 2- hash both headers together > > 2- add salt. can come from driver or device itself > > > > More ideas? > > > > -- > > 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: [virtio-comment] Re: [PATCH v7] virtio-net: support inner header hash
> From: Michael S. Tsirkin > Sent: Wednesday, February 8, 2023 8:52 AM > > On Wed, Feb 08, 2023 at 01:38:36PM +, Parav Pandit wrote: > > > > > From: Michael S. Tsirkin > > > Sent: Wednesday, February 8, 2023 8:32 AM > > > > > > On Wed, Feb 08, 2023 at 05:18:32AM +, Parav Pandit wrote: > > > > > From: Heng Qi > > > > > Sent: Tuesday, February 7, 2023 10:25 PM > > > > > > > > [..] > > > > > >> > > > > > >> Do you think we need both hash_types and hash_tunnel_types? > > > > > > In struct virtio_net_config we need two fields. > > > > > > a. supported_hash_types (already exists) b. > > > > > > supported_hash_tunnel_type > > > > > > -> bitmap indicating for which outer headers, inner hash > > > > > > -> calculation is > > > > > supported. > > > > > > > > > > Thanks for the suggestion, we seem to have reached an agreement. > > > > > > > > > > > > > > > > > In struct virtio_net_hdr we need two fields. > > > > > > a. hash_report (already exists) b. hash_tunnel_type 8 bits -> > > > > > > absolute value indicating which outer header > > > > > exists when inner header hash calculated. > > > > > > You already have it in your patch named as hash_report_tunnel. > > > > > > May be better to name as hash_report_tunnel_type to make it > > > > > > clearer that its > > > > > type. > > > > > > > > > > Sure. > > > > > > > > > > Thanks for your reply. > > > > > > > > I had one last question. Why do we need to inform the > > > hash_report_tunnel_type of the outer header in the virtio_net_hdr? > > > > Is this for debug? Or is there a use case that will process this value? > > > > > > Well we have hash_report which is kind of similar (and also kind of > > > pointless but I think it's there because WHQL wants it). > > Hash_report is useful. It tells hash_value is in which namespace > > (ipv4-tcp/ipv4 > udp etc). > > OS can use this value to find tcp connection in a given namespace. > > > > > Maybe we can steal some bits > > > from there instead of a new field? > > > > > I do not have problem adding extra bits. I just don't find that just > > telling that > its vxlan or nvgre to the OS is useful. > > If OS needs to know about outer header details, it needs to know the VNI > information than just telling vxlan. > > This does make sense. > > > > > > > > I have a follow up question though: are we only hashing the inner > > > header or both inner and outer header? Somewhat confused on this. > > > > > I understood as inner header. But worth to describe it. May be there. Need > > to > read v8 patch. > > Hmm. I just realized that there's a security problem with hashing just the > inner > header: it allow users inside the tunnel control queueing outside. > By observing packet loss some information leaks between tunnels. > Ah I know now. We are leaking outer header information inside the virtio net hdr, and outer header might be already stripped off by a different entity. I think the use case here is it's the same sw entity that owns the virtio net device does the encap/decap too. - 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: [virtio-comment] Re: [PATCH v7] virtio-net: support inner header hash
> From: Michael S. Tsirkin > Sent: Wednesday, February 8, 2023 8:52 AM > > On Wed, Feb 08, 2023 at 01:38:36PM +, Parav Pandit wrote: > > > > > From: Michael S. Tsirkin > > > Sent: Wednesday, February 8, 2023 8:32 AM > > > > > > On Wed, Feb 08, 2023 at 05:18:32AM +, Parav Pandit wrote: > > > > > From: Heng Qi > > > > > Sent: Tuesday, February 7, 2023 10:25 PM > > > > > > > > [..] > > > > > >> > > > > > >> Do you think we need both hash_types and hash_tunnel_types? > > > > > > In struct virtio_net_config we need two fields. > > > > > > a. supported_hash_types (already exists) b. > > > > > > supported_hash_tunnel_type > > > > > > -> bitmap indicating for which outer headers, inner hash > > > > > > -> calculation is > > > > > supported. > > > > > > > > > > Thanks for the suggestion, we seem to have reached an agreement. > > > > > > > > > > > > > > > > > In struct virtio_net_hdr we need two fields. > > > > > > a. hash_report (already exists) b. hash_tunnel_type 8 bits -> > > > > > > absolute value indicating which outer header > > > > > exists when inner header hash calculated. > > > > > > You already have it in your patch named as hash_report_tunnel. > > > > > > May be better to name as hash_report_tunnel_type to make it > > > > > > clearer that its > > > > > type. > > > > > > > > > > Sure. > > > > > > > > > > Thanks for your reply. > > > > > > > > I had one last question. Why do we need to inform the > > > hash_report_tunnel_type of the outer header in the virtio_net_hdr? > > > > Is this for debug? Or is there a use case that will process this value? > > > > > > Well we have hash_report which is kind of similar (and also kind of > > > pointless but I think it's there because WHQL wants it). > > Hash_report is useful. It tells hash_value is in which namespace > > (ipv4-tcp/ipv4 > udp etc). > > OS can use this value to find tcp connection in a given namespace. > > > > > Maybe we can steal some bits > > > from there instead of a new field? > > > > > I do not have problem adding extra bits. I just don't find that just > > telling that > its vxlan or nvgre to the OS is useful. > > If OS needs to know about outer header details, it needs to know the VNI > information than just telling vxlan. > > This does make sense. > > > > > > > > I have a follow up question though: are we only hashing the inner > > > header or both inner and outer header? Somewhat confused on this. > > > > > I understood as inner header. But worth to describe it. May be there. Need > > to > read v8 patch. > > Hmm. I just realized that there's a security problem with hashing just the > inner > header: it allow users inside the tunnel control queueing outside. > By observing packet loss some information leaks between tunnels. > I likely didn't understand. Can you please explain? Queuing is always done on the inner header with/without encapsulation. Hash is always reported for inner header. It is only adding the ability to hash even when outer header exists. If queuing to be decided based on outer header (hash), then that is different. Hashing both inner and outer in a flat q structure unlikely works, right? Because both hashes can result in different q selection. > > Ideas for solving this they all involve hashing both inner and outer > header: > 1- report two sets of hashes. overkill? > 2- hash both headers together > 2- add salt. can come from driver or device itself > > More ideas? > > -- > 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] VirtIO Sound (audio control support)
On Wed, Feb 08, 2023 at 07:21:45PM +0530, Sameer Pujar wrote: > Thanks Anton for your response. > > > On 08-02-2023 18:46, Anton Yakovlev wrote: > > Hi Sameer, > > > > We were waiting for the extension for audio controls to be accepted into > > the > > VIRTIO specification. > > Is audio control extension proposal to VIRTIO sound specification accepted? Is this it? commit 49ff7805924c421a0ca06e75d1741e0216c8f11c Author: Anton Yakovlev Date: Tue Apr 20 20:32:03 2021 +0200 virtio-snd: add support for audio controls This patch extends the virtio sound device specification by adding support for audio controls. Audio controls can be used to set the volume level, mute/unmute the audio signal, switch different modes/states of the virtual sound device, etc. Fixes: https://github.com/oasis-tcs/virtio-spec/issues/107 Signed-off-by: Anton Yakovlev Signed-off-by: Cornelia Huck > > We recently resumed our upstreaming efforts and a > > colleague of mine should resubmit the patch soon. > > > > > > I will look forward to it and can probably help in testing this. > > - > 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
[virtio-dev] RE: RE: [virtio-comment] [PATCH] virtio-net: support per-queue coalescing moderation
> From: Michael S. Tsirkin > Sent: Wednesday, February 8, 2023 5:07 AM > > > > 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 like 2 better. I also think a and b are enough. c can be achieved through > a. > Yep. (c) is just to indicate that its disabled via the mode in a command. - 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: [virtio-comment] Re: [PATCH v7] virtio-net: support inner header hash
On Wed, Feb 08, 2023 at 01:38:36PM +, Parav Pandit wrote: > > > From: Michael S. Tsirkin > > Sent: Wednesday, February 8, 2023 8:32 AM > > > > On Wed, Feb 08, 2023 at 05:18:32AM +, Parav Pandit wrote: > > > > From: Heng Qi > > > > Sent: Tuesday, February 7, 2023 10:25 PM > > > > > > [..] > > > > >> > > > > >> Do you think we need both hash_types and hash_tunnel_types? > > > > > In struct virtio_net_config we need two fields. > > > > > a. supported_hash_types (already exists) b. > > > > > supported_hash_tunnel_type > > > > > -> bitmap indicating for which outer headers, inner hash > > > > > -> calculation is > > > > supported. > > > > > > > > Thanks for the suggestion, we seem to have reached an agreement. > > > > > > > > > > > > > > In struct virtio_net_hdr we need two fields. > > > > > a. hash_report (already exists) > > > > > b. hash_tunnel_type 8 bits -> absolute value indicating which > > > > > outer header > > > > exists when inner header hash calculated. > > > > > You already have it in your patch named as hash_report_tunnel. > > > > > May be better to name as hash_report_tunnel_type to make it > > > > > clearer that its > > > > type. > > > > > > > > Sure. > > > > > > > > Thanks for your reply. > > > > > > I had one last question. Why do we need to inform the > > hash_report_tunnel_type of the outer header in the virtio_net_hdr? > > > Is this for debug? Or is there a use case that will process this value? > > > > Well we have hash_report which is kind of similar (and also kind of > > pointless > > but I think it's there because WHQL wants it). > Hash_report is useful. It tells hash_value is in which namespace > (ipv4-tcp/ipv4 udp etc). > OS can use this value to find tcp connection in a given namespace. > > > Maybe we can steal some bits > > from there instead of a new field? > > > I do not have problem adding extra bits. I just don't find that just telling > that its vxlan or nvgre to the OS is useful. > If OS needs to know about outer header details, it needs to know the VNI > information than just telling vxlan. This does make sense. > > > > I have a follow up question though: are we only hashing the inner header or > > both inner and outer header? Somewhat confused on this. > > > I understood as inner header. But worth to describe it. May be there. Need to > read v8 patch. Hmm. I just realized that there's a security problem with hashing just the inner header: it allow users inside the tunnel control queueing outside. By observing packet loss some information leaks between tunnels. Ideas for solving this they all involve hashing both inner and outer header: 1- report two sets of hashes. overkill? 2- hash both headers together 2- add salt. can come from driver or device itself More ideas? -- 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] VirtIO Sound (audio control support)
Thanks Anton for your response. On 08-02-2023 18:46, Anton Yakovlev wrote: Hi Sameer, We were waiting for the extension for audio controls to be accepted into the VIRTIO specification. Is audio control extension proposal to VIRTIO sound specification accepted? We recently resumed our upstreaming efforts and a colleague of mine should resubmit the patch soon. I will look forward to it and can probably help in testing this. - 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: [PATCH] virtio-net: support per-queue coalescing moderation
> From: Michael S. Tsirkin
> Sent: Wednesday, February 8, 2023 4:56 AM
>
> On Wed, Feb 08, 2023 at 10:20:08AM +0800, Heng Qi wrote:
> >
> >
> > 在 2023/2/7 下午10:29, Michael S. Tsirkin 写道:
> > > On Tue, Feb 07, 2023 at 12:51:26PM +, Parav Pandit wrote:
> > > > > From: Xuan Zhuo
> > > > > Sent: Tuesday, February 7, 2023 6:50 AM
> > > > >
> > > > > On Tue, 7 Feb 2023 13:25:13 +0200, Alvaro Karsz
> > > > >
> > > > > wrote:
> > > > > > 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 control coalescing parameters at the queue
> granularity.
> > > > > > >
> > > > > > > Signed-off-by: Heng Qi
> > > > > > > Reviewed-by: Xuan Zhuo
> > > > > > > ---
> > > > > > > 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.
> > > > > > > +
> > > > > > Since this feature allows us to change the coalescing
> > > > > > parameters for all the queues when rx/tx_qid = 0x, and
> > > > > > since version 1.3 wasn't released yet, maybe the "per-vq"
> > > > > > functionality can be added to VIRTIO_NET_F_NOTF_COAL instead of
> adding a new feature?
> > > > >
> > > > > According to my understanding, all the features of voting are
> > > > > formal. It can be used by the manufacturer.
> > > > >
> > > > > Of course, as far as I know, no manufacturer has used this
> > > > > feature for the time being. But I think we should add a new feature.
> > > > >
> > > > > Or other people have other ideas.
> > > > I believe we should treat it as fix and avoid a new feature bit as spec
> > > > is not
> released, and it is very recent change.
> > > 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?
> >
> > Good idea, the new command seems to make compatibility easier to achieve.
> > An error can be returned when the old device does not recognize the
> > new command.
>
> Not that is a bad way to figure out what is supported. E.g. driver can't
> reasonably probe a ton of commands just to check there's compatibility for
> migration. If it's optional pls gate by feature bit or some such.
There is no need to issue ton of commands.
One command can return lot of capabilities.
But it is different discussion. Not applicable here.
>
> > > Also if we are going to change things how about adding a query command
> too?
> >
This is useful.
> > Yes, we should.
> >
> > Thanks.
> >
> > >
> > > Also Alvaro what is your take on whether the gloabal version counts
> > > packets and time for all queues together or per queue? The spec does
> > > not make it clear ATM.
> > >
[virtio-dev] Re: [virtio-comment] Re: [virtio-dev] Re: [virtio-comment] [PATCH] virtio-net: support per-queue coalescing moderation
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
[virtio-dev] RE: [virtio-comment] RE: [virtio-dev] Re: [virtio-comment] Re: [PATCH v7] virtio-net: support inner header hash
> From: Michael S. Tsirkin > Sent: Wednesday, February 8, 2023 8:32 AM > > On Wed, Feb 08, 2023 at 05:18:32AM +, Parav Pandit wrote: > > > From: Heng Qi > > > Sent: Tuesday, February 7, 2023 10:25 PM > > > > [..] > > > >> > > > >> Do you think we need both hash_types and hash_tunnel_types? > > > > In struct virtio_net_config we need two fields. > > > > a. supported_hash_types (already exists) b. > > > > supported_hash_tunnel_type > > > > -> bitmap indicating for which outer headers, inner hash > > > > -> calculation is > > > supported. > > > > > > Thanks for the suggestion, we seem to have reached an agreement. > > > > > > > > > > > In struct virtio_net_hdr we need two fields. > > > > a. hash_report (already exists) > > > > b. hash_tunnel_type 8 bits -> absolute value indicating which > > > > outer header > > > exists when inner header hash calculated. > > > > You already have it in your patch named as hash_report_tunnel. > > > > May be better to name as hash_report_tunnel_type to make it > > > > clearer that its > > > type. > > > > > > Sure. > > > > > > Thanks for your reply. > > > > I had one last question. Why do we need to inform the > hash_report_tunnel_type of the outer header in the virtio_net_hdr? > > Is this for debug? Or is there a use case that will process this value? > > Well we have hash_report which is kind of similar (and also kind of pointless > but I think it's there because WHQL wants it). Hash_report is useful. It tells hash_value is in which namespace (ipv4-tcp/ipv4 udp etc). OS can use this value to find tcp connection in a given namespace. > Maybe we can steal some bits > from there instead of a new field? > I do not have problem adding extra bits. I just don't find that just telling that its vxlan or nvgre to the OS is useful. If OS needs to know about outer header details, it needs to know the VNI information than just telling vxlan. > > I have a follow up question though: are we only hashing the inner header or > both inner and outer header? Somewhat confused on this. > I understood as inner header. But worth to describe it. May be there. Need to read v8 patch. - 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: [virtio-comment] Re: [PATCH v7] virtio-net: support inner header hash
On Wed, Feb 08, 2023 at 05:18:32AM +, Parav Pandit wrote: > > From: Heng Qi > > Sent: Tuesday, February 7, 2023 10:25 PM > > [..] > > >> > > >> Do you think we need both hash_types and hash_tunnel_types? > > > In struct virtio_net_config we need two fields. > > > a. supported_hash_types (already exists) b. supported_hash_tunnel_type > > > -> bitmap indicating for which outer headers, inner hash calculation is > > supported. > > > > Thanks for the suggestion, we seem to have reached an agreement. > > > > > > > > In struct virtio_net_hdr we need two fields. > > > a. hash_report (already exists) > > > b. hash_tunnel_type 8 bits -> absolute value indicating which outer header > > exists when inner header hash calculated. > > > You already have it in your patch named as hash_report_tunnel. > > > May be better to name as hash_report_tunnel_type to make it clearer that > > > its > > type. > > > > Sure. > > > > Thanks for your reply. > > I had one last question. Why do we need to inform the hash_report_tunnel_type > of the outer header in the virtio_net_hdr? > Is this for debug? Or is there a use case that will process this value? Well we have hash_report which is kind of similar (and also kind of pointless but I think it's there because WHQL wants it). Maybe we can steal some bits from there instead of a new field? I have a follow up question though: are we only hashing the inner header or both inner and outer header? Somewhat confused on this. In fact, CC Yuri for thoughts and suggestions from windows side of things. -- 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] VirtIO Sound (audio control support)
Hi Sameer, We were waiting for the extension for audio controls to be accepted into the VIRTIO specification. We recently resumed our upstreaming efforts and a colleague of mine should resubmit the patch soon. On 07.02.2023 08:37, Sameer Pujar wrote: Hi Anton, Of late I have been looking into VirtIO and sound device specification. I came across your series which proposes audio control support [0]. I don't find this implementation in the mainline linux today and hence was wondering if there were any concerns on the series or if there were any alternate options available or if this is a WIP item. Can you please add relevant details? Thanks in advance. Regards, Sameer. [0] https://patchwork.kernel.org/project/alsa-devel/patch/20220307194716.1517565-2-anton.yakov...@opensynergy.com/ - To unsubscribe, e-mail: virtio-dev-unsubscr...@lists.oasis-open.org For additional commands, e-mail: virtio-dev-h...@lists.oasis-open.org -- Anton Yakovlev Senior Software Engineer OpenSynergy GmbH Rotherstr. 20, 10245 Berlin - 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] Re: [PATCH v7] virtio-net: support inner header hash
> From: Heng Qi > Sent: Wednesday, February 8, 2023 1:11 AM > > > 在 2023/2/8 下午1:18, Parav Pandit 写道: > >> From: Heng Qi > >> Sent: Tuesday, February 7, 2023 10:25 PM > > [..] > Do you think we need both hash_types and hash_tunnel_types? > >>> In struct virtio_net_config we need two fields. > >>> a. supported_hash_types (already exists) b. > >>> supported_hash_tunnel_type > >>> -> bitmap indicating for which outer headers, inner hash calculation > >>> -> is > >> supported. > >> > >> Thanks for the suggestion, we seem to have reached an agreement. > >> > >>> In struct virtio_net_hdr we need two fields. > >>> a. hash_report (already exists) > >>> b. hash_tunnel_type 8 bits -> absolute value indicating which outer > >>> header > >> exists when inner header hash calculated. > >>> You already have it in your patch named as hash_report_tunnel. > >>> May be better to name as hash_report_tunnel_type to make it clearer > >>> that its > >> type. > >> > >> Sure. > >> > >> Thanks for your reply. > > I had one last question. Why do we need to inform the > hash_report_tunnel_type of the outer header in the virtio_net_hdr? > > Is this for debug? Or is there a use case that will process this value? > > The driver may use it to do some statistical information, or do some rx > classification based on the rx hash, and we'd better not hide information from > the driver. > Statistical information is better gathered via stats, instead of adding such code in driver data path. It is not about hiding. It has onus on the data path to detect the tunnel type and place that virtio_net_hdr. So lets find one use of it in data path which sw entity will use it.
Re: [virtio-dev] Re: [virtio-comment] [PATCH] virtio-net: support per-queue coalescing moderation
在 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
[virtio-dev] Re: [virtio-comment] RE: [virtio-dev] [PATCH v3] virtio-net: Mention VIRTIO_NET_F_HASH_REPORT dependency on VIRTIO_NET_F_CTRL_VQ
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
[virtio-dev] Re: [virtio-comment] RE: [virtio-dev] [PATCH] virtio-net: Fix and update VIRTIO_NET_F_NOTF_COAL feature
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 maybe we should do it there? Should I rephrase the "Notifications When Coalescing Parameters Change" paragraph? 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
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
[virtio-dev] Re: RE: [virtio-comment] [PATCH] virtio-net: support per-queue coalescing moderation
On Wed, Feb 08, 2023 at 02:43:02AM +, Parav Pandit wrote: > > > From: Xuan Zhuo > > Sent: Tuesday, February 7, 2023 9:25 PM > > > > On Wed, 8 Feb 2023 02:20:27 +, Parav Pandit wrote: > > > > > > > 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. > > > Once per VQ is supported, driver may just use the default net-dim to have > > best out of box experience, whenever device supports it. > > > > > > So you mean, we only support the parameters based on Per-Queue. My > > original idea is to support two methods at the same time. > > Yes, if we want to support both the modes, that better to have such command. > Because otherwise the device implementation is bit clumsy which needs to do > the guess work. > For example, driver has configured global params per device. > Now suddenly driver configured first queues parameter. > At this point device should run N-1 queues using global mode and 1 queue with > per Q param. Seems fine to me. > Similar sequence also occurs when there is per q params configured and > suddenly driver configures global param. In this case I feel global one should win. I think global is just a shortcut for running per queue. > Even in this case now device either must iterate internally and move per q to > global values. > > Better to avoid such complexity in device around implicit and confusing > behavior. > > 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 like 2 better. I also think a and b are enough. c can be achieved through a. -- 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
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.
[virtio-dev] Re: [virtio-comment] Re: [PATCH] virtio-net: support per-queue coalescing moderation
On Wed, Feb 08, 2023 at 10:20:08AM +0800, Heng Qi wrote:
>
>
> 在 2023/2/7 下午10:29, Michael S. Tsirkin 写道:
> > On Tue, Feb 07, 2023 at 12:51:26PM +, Parav Pandit wrote:
> > > > From: Xuan Zhuo
> > > > Sent: Tuesday, February 7, 2023 6:50 AM
> > > >
> > > > On Tue, 7 Feb 2023 13:25:13 +0200, Alvaro Karsz
> > > >
> > > > wrote:
> > > > > 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 control coalescing parameters at the queue granularity.
> > > > > >
> > > > > > Signed-off-by: Heng Qi
> > > > > > Reviewed-by: Xuan Zhuo
> > > > > > ---
> > > > > > 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.
> > > > > > +
> > > > > Since this feature allows us to change the coalescing parameters for
> > > > > all the queues when rx/tx_qid = 0x, and since version 1.3 wasn't
> > > > > released yet, maybe the "per-vq" functionality can be added to
> > > > > VIRTIO_NET_F_NOTF_COAL instead of adding a new feature?
> > > >
> > > > According to my understanding, all the features of voting are formal.
> > > > It can be
> > > > used by the manufacturer.
> > > >
> > > > Of course, as far as I know, no manufacturer has used this feature for
> > > > the time
> > > > being. But I think we should add a new feature.
> > > >
> > > > Or other people have other ideas.
> > > I believe we should treat it as fix and avoid a new feature bit as spec
> > > is not released, and it is very recent change.
> > 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?
>
> Good idea, the new command seems to make compatibility easier to achieve.
> An error can be returned when the old device does not recognize the new
> command.
Not that is a bad way to figure out what is supported. E.g. driver
can't reasonably probe a ton of commands just to check there's
compatibility for migration. If it's optional pls gate by feature
bit or some such.
> > Also if we are going to change things how about adding a query command too?
>
> Yes, we should.
>
> Thanks.
>
> >
> > Also Alvaro what is your take on whether the gloabal version counts
> > packets and time for all queues together or per queue? The spec
> > does not make it clear ATM.
> >
-
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 v2] virtio-net: Improve introductory description
On Wed, Feb 08, 2023 at 01:17:55AM +, Parav Pandit wrote:
>
>
> > From: Cornelia Huck
> > Sent: Tuesday, February 7, 2023 5:07 AM
> >
> > On Thu, Feb 02 2023, Parav Pandit wrote:
> >
> > > The control VQ of the virtio network device is used beyond advance
> > > steering control. The control VQ dynamically changes multiple features
> > > of the initialized device.
> > >
> > > Hence, update this area of control VQ introductory description at few
> > > places and also place the link to its description.
> > >
> > > Also update the introduction section to better describe receive and
> > > transmit virtqueues.
> > >
> > > Fixes: https://github.com/oasis-tcs/virtio-spec/issues/156
> > > Signed-off-by: Parav Pandit
> > > ---
> > > device-types/net/description.tex | 21 +++--
> > > 1 file changed, 11 insertions(+), 10 deletions(-)
> > >
> > > diff --git a/device-types/net/description.tex
> > > b/device-types/net/description.tex
> > > index 88a5770..dedd6b1 100644
> > > --- a/device-types/net/description.tex
> > > +++ b/device-types/net/description.tex
> > > @@ -2,13 +2,13 @@ \section{Network Device}\label{sec:Device Types /
> > > Network Device}
> > >
> > > The virtio network device is a virtual network interface controller.
> > > It consists of a virtual Ethernet link which connects the device -to
> > > the Ethernet network. It is the most complex of the devices -supported
> > > so far by virtio. It has enhanced rapidly and demonstrates -clearly
> > > how support for new features are added to an existing -device. Empty
> > > buffers are placed in one virtqueue for receiving -packets, and
> > > outgoing packets are enqueued into another for -transmission in that
> > > order. A third command queue is used to -control advanced filtering
> > > features.
> > > +to the Ethernet network. The driver posts empty buffers in the
> > > +receive virtqueue. The device receives the incoming packets from the
> > > +link; the device places these incoming packets in the receive virtqueue
> > buffers.
> > > +The driver enqueues outgoing packets to the transmit virtqueue. The
> > > +device dequeues these packets from the transmit virtqueue and sends
> > > +them to the link. A control virtqueue is used to dynamically
> > > +manipulate various features of the initialized device.
What happened here? Parav it looks like your client mangled Cornelia's
response. Pls take a look at your client config.
> > General question: Is it better to talk about "the receive virtqueue" (to
> > keep it
> > simple), or "a receive virtqueue" (as there may be several)?
>
> For sure Michael can better answer this than me. :)
> The receive virtqueue aligns to me with "the device", the driver etc.
> It also aligns with existing sections in device and driver requirements that
> refers to "the receiveq".
> Since there are many queues, better to refer as "the".
> A receive queue may imply that there is only one.
My take is that it should be "a queue" the first time
we talk about it. Then if it's the same queue we say "the queue".
I also feel an intro sentence saying something along the lines
of "the device has receive and transmit queues". Then
rest of uses can be "the".
--
MST
-
To unsubscribe, e-mail: virtio-dev-unsubscr...@lists.oasis-open.org
For additional commands, e-mail: virtio-dev-h...@lists.oasis-open.org
