RE: [virtio-dev] [PATCH v2] virtio_net: support low and high rate of notification coalescing
> From: Michael S. Tsirkin > Sent: Monday, January 16, 2023 7:28 PM > > On Tue, Jan 17, 2023 at 12:07:59AM +, Parav Pandit wrote: > > > > > From: Michael S. Tsirkin > > > Sent: Monday, January 16, 2023 4:30 PM > > > > > > > > So, can you please describe the motivation, what problem does > > > > > > the new > > > > > command solve? > > > > > > It is likely that control entity which want to set different > > > > > > coalescing > > > > > parameters likely wants to more than just two values. > > > > > > This is something already supported today. > > > > > > > > > > Interesting. Parav can you give some examples? > > > > > > > > In a Linux OS based system, following drivers [1] uses the > > > > interrupt coalescing > > > parameters by using net dim [2]. > > > > > > > > Net dim is the control entity which enables to use more than 2 > > > > moderation > > > parameters based on the workload instead of two low and high static > values. > > > > > > > > [1] list of drivers > > > > a. drivers/net/ethernet/amazon/ena/ena_netdev.c > > > > b. drivers/net/ethernet/broadcom/bcmsysport.c > > > > c. drivers/net/ethernet/broadcom/bnxt/bnxt.c > > > > d. drivers/net/ethernet/broadcom/genet/bcmgenet.c > > > > e. drivers/net/ethernet/hisilicon/hns3/hns3_enet.c > > > > f. drivers/net/ethernet/intel/ice/ice_txrx.c > > > > g. drivers/net/ethernet/mellanox/mlx5/core/en_txrx.c > > > > and more... > > > > > > > > [2] https://docs.kernel.org/networking/net_dim.html > > > > > > But this is net-dim. IIUC it's a software mechanism which tweaks > > > coalescing parameters depending on traffic, right? > > > > > It is more than tweak. > > It adjusts the parameters to avoid system oscillate and constant manual > tuning. > > > > > Consider for example bnxt_dim_work: all this does is set a single > > > set of usecs/packets parameters. > > > IOW it's need already seem to be addressed in the spec. > > > > > > By comparison this patch is an attempt to offload ethtool's > > > --coalesce parameters to the card. IMO what it misses is > > > completeness, e.g. sample- interval is not specified. > > Yes. fallback rate above low and below high are also missing. > > I think what author meant is just mirroring ethtool here. > > > > Also, introspection is missing and it's useful to avoid keeping all > > > state in the driver. > > > > > > Now clearly net dim has potential to make more clever decisions, but > > > OTOH it's clearly heavier weight. > > > > > Its heavy weight that already exists in one OS. > > OTOH constantly invoking ethtool from user space for a admin/sw is heavy > weight too. > > > > > So I personally see potential for both to be useful. > > > If no why not? > > > > > Lets describe the problem, value and its usefulness to begin. > > I am not against it. I just fail to find its good use with the existence of > > net dim. > > So lets explain how it is better. > > If all you want is what ethtool provides the new interface will do exactly > that > completely on the card without involving scheduler. > That seems pretty clear. > What is clear from the commit message. The motivation was not clear to me, with the advent of net dim support. Proposed ethtool matching parameters exists for more than decade in ethtool. > Which bits? For high and low? Maybe. It's really a single feature just for 2 > thresholds. Which looks like a trivial generalization of one. They are not as general the existing one. It has specific meaning to support two different thresholds that a device needs to monitor for. > But I wonder whether any devices have more than 2 thresholds, or just one. > Do we want N thresholds? How about taking a look at some other OS-es? I > never looked at net dim internal logic to see whether it can reasonably be > offloaded to a card. > > All these are valid concerns that need research to address. Any input here? > Feature bits, use of cvq are all cosmetic issues by comparison. > The proposed ethtool counter part is implemented by only handful of devices. Almost all those vendors have superseded those devices. And those modern devices which superseded are not supporting them. I am part of at least two device history as benet and mlx. Both of them have not carried forward these parameters. Compare to that net dim logic introduced in 2018 has found more than 10 active devices. And it even extended to non nic device too. This statistic clearly shows the value of it. A next step would be as you suggest offloading this as well to the device. However, this proposal is not addressing to offload it. This patch relies on sw entity to configure some static values for two range. So, it may be addressing some specific use case or OS scenario. I like to see this motivation described in the commit log of v3, which helps to dial in the spec faster. All I am asking is plan motivation on how it helps, instead of saying hey it exists in ethtool a decade ago, lets do it...
Re: [virtio-dev] [PATCH v2] virtio_net: support low and high rate of notification coalescing
On Tue, Jan 17, 2023 at 12:07:59AM +, Parav Pandit wrote: > > > From: Michael S. Tsirkin > > Sent: Monday, January 16, 2023 4:30 PM > > > > > > So, can you please describe the motivation, what problem does the > > > > > new > > > > command solve? > > > > > It is likely that control entity which want to set different > > > > > coalescing > > > > parameters likely wants to more than just two values. > > > > > This is something already supported today. > > > > > > > > Interesting. Parav can you give some examples? > > > > > > In a Linux OS based system, following drivers [1] uses the interrupt > > > coalescing > > parameters by using net dim [2]. > > > > > > Net dim is the control entity which enables to use more than 2 moderation > > parameters based on the workload instead of two low and high static values. > > > > > > [1] list of drivers > > > a. drivers/net/ethernet/amazon/ena/ena_netdev.c > > > b. drivers/net/ethernet/broadcom/bcmsysport.c > > > c. drivers/net/ethernet/broadcom/bnxt/bnxt.c > > > d. drivers/net/ethernet/broadcom/genet/bcmgenet.c > > > e. drivers/net/ethernet/hisilicon/hns3/hns3_enet.c > > > f. drivers/net/ethernet/intel/ice/ice_txrx.c > > > g. drivers/net/ethernet/mellanox/mlx5/core/en_txrx.c > > > and more... > > > > > > [2] https://docs.kernel.org/networking/net_dim.html > > > > But this is net-dim. IIUC it's a software mechanism which tweaks coalescing > > parameters depending on traffic, right? > > > It is more than tweak. > It adjusts the parameters to avoid system oscillate and constant manual > tuning. > > > Consider for example bnxt_dim_work: all this does is set a single set of > > usecs/packets parameters. > > IOW it's need already seem to be addressed in the spec. > > > > By comparison this patch is an attempt to offload ethtool's --coalesce > > parameters to the card. IMO what it misses is completeness, e.g. sample- > > interval is not specified. > Yes. fallback rate above low and below high are also missing. I think what author meant is just mirroring ethtool here. > > Also, introspection is missing and it's useful to avoid keeping all state > > in the > > driver. > > > > Now clearly net dim has potential to make more clever decisions, but OTOH > > it's > > clearly heavier weight. > > > Its heavy weight that already exists in one OS. > OTOH constantly invoking ethtool from user space for a admin/sw is heavy > weight too. > > > So I personally see potential for both to be useful. > > If no why not? > > > Lets describe the problem, value and its usefulness to begin. > I am not against it. I just fail to find its good use with the existence of > net dim. > So lets explain how it is better. If all you want is what ethtool provides the new interface will do exactly that completely on the card without involving scheduler. That seems pretty clear. > > > > > They are best to be handled a pre-patch to this one. > > > > > Please split them so that spec improvement work can progress while > > > > > new > > > > features are under review. > > > > > > > > Yea it's not a bad idea. I figured it out but > > > > > > > > > > > > > > > > +\item[VIRTIO_NET_F_NOTF_COAL_LOW_HIGH(50)] Device supports > > > > > > notifications coalescing low rate and high rate sets. > > > > > > + > > > > > Even though historically, many feature bits exists, adding more > > > > > feature bits is > > > > not scalable. > > > > > > > > I disagree here. We have unlimited feature bits and I see no problem > > > > with their scalability. Devices (such as SIOV as opposed to SRIOV) > > > > which have very limited memory need to use the proposed vq transport > > > > that intel's working on, pci transport is wasting too much memory > > > > anyway. I hope we are not going to repeat this discussion for each > > > > and every new feature :) > > > > > > > Pci transport of the virtio in current form asks to put things in device > > > memory. > > > And it doesn't need to continue this way regardless of SIOV. > > > It doesn't have any relation to SR-IOV either. > > > > > > There are even PF devices too. > > > > > > And putting things in feature bit limits the way these devices are > > > composed. > > > So lets discuss to use control vq for get and set both. > > > > > > Feature bit doesn't even cut what is needed. Software doesn't even know > > what the valid range is. > > > It needs to be discovered through cvq. > > > > > > All well and good but let's not have this discussion in the thread please. > > > Ok. This feature bit trying to do multiple features under single bit and all > commands are not always useful. > It needs two feature bits. > And things start not to scale. > But fine. Which bits? For high and low? Maybe. It's really a single feature just for 2 thresholds. Which looks like a trivial generalization of one. But I wonder whether any devices have more than 2 thresholds, or just one. Do we want N thresholds? How about taking a look at some other OS-es? I never looked at net dim internal
RE: [virtio-dev] [PATCH v2] virtio_net: support low and high rate of notification coalescing
> From: Michael S. Tsirkin > Sent: Monday, January 16, 2023 4:30 PM > > > > So, can you please describe the motivation, what problem does the > > > > new > > > command solve? > > > > It is likely that control entity which want to set different > > > > coalescing > > > parameters likely wants to more than just two values. > > > > This is something already supported today. > > > > > > Interesting. Parav can you give some examples? > > > > In a Linux OS based system, following drivers [1] uses the interrupt > > coalescing > parameters by using net dim [2]. > > > > Net dim is the control entity which enables to use more than 2 moderation > parameters based on the workload instead of two low and high static values. > > > > [1] list of drivers > > a. drivers/net/ethernet/amazon/ena/ena_netdev.c > > b. drivers/net/ethernet/broadcom/bcmsysport.c > > c. drivers/net/ethernet/broadcom/bnxt/bnxt.c > > d. drivers/net/ethernet/broadcom/genet/bcmgenet.c > > e. drivers/net/ethernet/hisilicon/hns3/hns3_enet.c > > f. drivers/net/ethernet/intel/ice/ice_txrx.c > > g. drivers/net/ethernet/mellanox/mlx5/core/en_txrx.c > > and more... > > > > [2] https://docs.kernel.org/networking/net_dim.html > > But this is net-dim. IIUC it's a software mechanism which tweaks coalescing > parameters depending on traffic, right? > It is more than tweak. It adjusts the parameters to avoid system oscillate and constant manual tuning. > Consider for example bnxt_dim_work: all this does is set a single set of > usecs/packets parameters. > IOW it's need already seem to be addressed in the spec. > > By comparison this patch is an attempt to offload ethtool's --coalesce > parameters to the card. IMO what it misses is completeness, e.g. sample- > interval is not specified. Yes. fallback rate above low and below high are also missing. > Also, introspection is missing and it's useful to avoid keeping all state in > the > driver. > > Now clearly net dim has potential to make more clever decisions, but OTOH it's > clearly heavier weight. > Its heavy weight that already exists in one OS. OTOH constantly invoking ethtool from user space for a admin/sw is heavy weight too. > So I personally see potential for both to be useful. > If no why not? > Lets describe the problem, value and its usefulness to begin. I am not against it. I just fail to find its good use with the existence of net dim. So lets explain how it is better. > > > > They are best to be handled a pre-patch to this one. > > > > Please split them so that spec improvement work can progress while > > > > new > > > features are under review. > > > > > > Yea it's not a bad idea. I figured it out but > > > > > > > > > > > > > +\item[VIRTIO_NET_F_NOTF_COAL_LOW_HIGH(50)] Device supports > > > > > notifications coalescing low rate and high rate sets. > > > > > + > > > > Even though historically, many feature bits exists, adding more > > > > feature bits is > > > not scalable. > > > > > > I disagree here. We have unlimited feature bits and I see no problem > > > with their scalability. Devices (such as SIOV as opposed to SRIOV) > > > which have very limited memory need to use the proposed vq transport > > > that intel's working on, pci transport is wasting too much memory > > > anyway. I hope we are not going to repeat this discussion for each > > > and every new feature :) > > > > > Pci transport of the virtio in current form asks to put things in device > > memory. > > And it doesn't need to continue this way regardless of SIOV. > > It doesn't have any relation to SR-IOV either. > > > > There are even PF devices too. > > > > And putting things in feature bit limits the way these devices are composed. > > So lets discuss to use control vq for get and set both. > > > > Feature bit doesn't even cut what is needed. Software doesn't even know > what the valid range is. > > It needs to be discovered through cvq. > > > All well and good but let's not have this discussion in the thread please. > Ok. This feature bit trying to do multiple features under single bit and all commands are not always useful. It needs two feature bits. And things start not to scale. But fine. > > > > > > It is also limits to enable such functionality at very early stage > > > > in the device > > > configuration. > > > > > > Not really, it just handles compatibility and enables the commands. > > > > > Command is nothing but a functionality. > > No, during init time it is useful to know whether to hook up to OS mechanisms > dealing with specific offloads. > It doesn't make sense to register a net device and set its feature bits without really enumerating it fully. And if that requires CVQ, it is more efficient anyway. > > > > So, if at all this proceeds, we want the new admin q to discover > > > > and negotiate > > > the new features. > > > > (Instead of features bits). > > > > > > Sounds like a new transport. I wouldn't make it gate this proposal. > > > > > I am not gating the proposal. > > what you are
Re: [virtio-dev] [PATCH v2] virtio_net: support low and high rate of notification coalescing
On Mon, Jan 16, 2023 at 08:52:43PM +, Parav Pandit wrote: > > > From: Michael S. Tsirkin > > Sent: Monday, January 16, 2023 3:34 PM > > > > On Mon, Jan 16, 2023 at 07:42:01PM +, Parav Pandit wrote: > > > Hi Alvaro, > > > > > > > From: virtio-dev@lists.oasis-open.org > > > > On Behalf Of Alvaro Karsz > > > > > > > > This patch adds a new feature, VIRTIO_NET_F_NOTF_COAL_LOW_HIGH, > > > > while clarifying/fixing existing coalescing concepts. > > > > > > > While its visible in the patch itself, what is being done, its > > > important to mention the problem statement/motivation for the feature at > > start of the commit log as first step. > > > > > > So, can you please describe the motivation, what problem does the new > > command solve? > > > It is likely that control entity which want to set different coalescing > > parameters likely wants to more than just two values. > > > This is something already supported today. > > > > Interesting. Parav can you give some examples? > > In a Linux OS based system, following drivers [1] uses the interrupt > coalescing parameters by using net dim [2]. > > Net dim is the control entity which enables to use more than 2 moderation > parameters based on the workload instead of two low and high static values. > > [1] list of drivers > a. drivers/net/ethernet/amazon/ena/ena_netdev.c > b. drivers/net/ethernet/broadcom/bcmsysport.c > c. drivers/net/ethernet/broadcom/bnxt/bnxt.c > d. drivers/net/ethernet/broadcom/genet/bcmgenet.c > e. drivers/net/ethernet/hisilicon/hns3/hns3_enet.c > f. drivers/net/ethernet/intel/ice/ice_txrx.c > g. drivers/net/ethernet/mellanox/mlx5/core/en_txrx.c > and more... > > [2] https://docs.kernel.org/networking/net_dim.html But this is net-dim. IIUC it's a software mechanism which tweaks coalescing parameters depending on traffic, right? Consider for example bnxt_dim_work: all this does is set a single set of usecs/packets parameters. IOW it's need already seem to be addressed in the spec. By comparison this patch is an attempt to offload ethtool's --coalesce parameters to the card. IMO what it misses is completeness, e.g. sample-interval is not specified. Also, introspection is missing and it's useful to avoid keeping all state in the driver. Now clearly net dim has potential to make more clever decisions, but OTOH it's clearly heavier weight. So I personally see potential for both to be useful. If no why not? > > > They are best to be handled a pre-patch to this one. > > > Please split them so that spec improvement work can progress while new > > features are under review. > > > > Yea it's not a bad idea. I figured it out but > > > > > > > > > > +\item[VIRTIO_NET_F_NOTF_COAL_LOW_HIGH(50)] Device supports > > > > notifications coalescing low rate and high rate sets. > > > > + > > > Even though historically, many feature bits exists, adding more feature > > > bits is > > not scalable. > > > > I disagree here. We have unlimited feature bits and I see no problem with > > their > > scalability. Devices (such as SIOV as opposed to SRIOV) which have very > > limited > > memory need to use the proposed vq transport that intel's working on, pci > > transport is wasting too much memory anyway. I hope we are not going to > > repeat this discussion for each and every new feature :) > > > Pci transport of the virtio in current form asks to put things in device > memory. > And it doesn't need to continue this way regardless of SIOV. > It doesn't have any relation to SR-IOV either. > > There are even PF devices too. > > And putting things in feature bit limits the way these devices are composed. > So lets discuss to use control vq for get and set both. > > Feature bit doesn't even cut what is needed. Software doesn't even know what > the valid range is. > It needs to be discovered through cvq. All well and good but let's not have this discussion in the thread please. > > > It is also limits to enable such functionality at very early stage in the > > > device > > configuration. > > > > Not really, it just handles compatibility and enables the commands. > > > Command is nothing but a functionality. No, during init time it is useful to know whether to hook up to OS mechanisms dealing with specific offloads. > > > So, if at all this proceeds, we want the new admin q to discover and > > > negotiate > > the new features. > > > (Instead of features bits). > > > > Sounds like a new transport. I wouldn't make it gate this proposal. > > > I am not gating the proposal. what you are however doing is bringing issues unrealted to the patch in question. Let's focus on discussing coalescing here please. > Ctrl vq only does set. > Control VQ needs the ability to do get also on deciding what to control. > Hence, new things like this can be discovered by net specific ctrol vq. > No need for involving generic device feature bits framework here. I see even less value in moving compatibility machinery over to a device specific
RE: [virtio-dev] [PATCH v2] virtio_net: support low and high rate of notification coalescing
> From: Michael S. Tsirkin > Sent: Monday, January 16, 2023 3:34 PM > > On Mon, Jan 16, 2023 at 07:42:01PM +, Parav Pandit wrote: > > Hi Alvaro, > > > > > From: virtio-dev@lists.oasis-open.org > > > On Behalf Of Alvaro Karsz > > > > > > This patch adds a new feature, VIRTIO_NET_F_NOTF_COAL_LOW_HIGH, > > > while clarifying/fixing existing coalescing concepts. > > > > > While its visible in the patch itself, what is being done, its > > important to mention the problem statement/motivation for the feature at > start of the commit log as first step. > > > > So, can you please describe the motivation, what problem does the new > command solve? > > It is likely that control entity which want to set different coalescing > parameters likely wants to more than just two values. > > This is something already supported today. > > Interesting. Parav can you give some examples? In a Linux OS based system, following drivers [1] uses the interrupt coalescing parameters by using net dim [2]. Net dim is the control entity which enables to use more than 2 moderation parameters based on the workload instead of two low and high static values. [1] list of drivers a. drivers/net/ethernet/amazon/ena/ena_netdev.c b. drivers/net/ethernet/broadcom/bcmsysport.c c. drivers/net/ethernet/broadcom/bnxt/bnxt.c d. drivers/net/ethernet/broadcom/genet/bcmgenet.c e. drivers/net/ethernet/hisilicon/hns3/hns3_enet.c f. drivers/net/ethernet/intel/ice/ice_txrx.c g. drivers/net/ethernet/mellanox/mlx5/core/en_txrx.c and more... [2] https://docs.kernel.org/networking/net_dim.html > > They are best to be handled a pre-patch to this one. > > Please split them so that spec improvement work can progress while new > features are under review. > > Yea it's not a bad idea. I figured it out but > > > > > > > +\item[VIRTIO_NET_F_NOTF_COAL_LOW_HIGH(50)] Device supports > > > notifications coalescing low rate and high rate sets. > > > + > > Even though historically, many feature bits exists, adding more feature > > bits is > not scalable. > > I disagree here. We have unlimited feature bits and I see no problem with > their > scalability. Devices (such as SIOV as opposed to SRIOV) which have very > limited > memory need to use the proposed vq transport that intel's working on, pci > transport is wasting too much memory anyway. I hope we are not going to > repeat this discussion for each and every new feature :) > Pci transport of the virtio in current form asks to put things in device memory. And it doesn't need to continue this way regardless of SIOV. It doesn't have any relation to SR-IOV either. There are even PF devices too. And putting things in feature bit limits the way these devices are composed. So lets discuss to use control vq for get and set both. Feature bit doesn't even cut what is needed. Software doesn't even know what the valid range is. It needs to be discovered through cvq. > > It is also limits to enable such functionality at very early stage in the > > device > configuration. > > Not really, it just handles compatibility and enables the commands. > Command is nothing but a functionality. > > So, if at all this proceeds, we want the new admin q to discover and > > negotiate > the new features. > > (Instead of features bits). > > Sounds like a new transport. I wouldn't make it gate this proposal. > I am not gating the proposal. Ctrl vq only does set. Control VQ needs the ability to do get also on deciding what to control. Hence, new things like this can be discovered by net specific ctrol vq. No need for involving generic device feature bits framework here. - 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] [PATCH v2] virtio_net: support low and high rate of notification coalescing
On Mon, Jan 16, 2023 at 07:42:01PM +, Parav Pandit wrote:
> Hi Alvaro,
>
> > From: virtio-dev@lists.oasis-open.org On
> > Behalf Of Alvaro Karsz
> >
> > This patch adds a new feature, VIRTIO_NET_F_NOTF_COAL_LOW_HIGH, while
> > clarifying/fixing existing coalescing concepts.
> >
> While its visible in the patch itself, what is being done,
> its important to mention the problem statement/motivation for the feature at
> start of the commit log as first step.
>
> So, can you please describe the motivation, what problem does the new command
> solve?
> It is likely that control entity which want to set different coalescing
> parameters likely wants to more than just two values.
> This is something already supported today.
Interesting. Parav can you give some examples?
> > The new feature adds 4 new commands to VIRTIO_NET_CTRL_NOTF_COAL
> > class:
> > - VIRTIO_NET_CTRL_NOTF_COAL_LOW_TX_SET - coalescing
> > parameters
> > to use when the packet rate is equal or lower then the low
> > threshold for TX.
> > - VIRTIO_NET_CTRL_NOTF_COAL_LOW_RX_SET - coalescing parameters
> > to use when the packet rate is equal or lower then the low
> > threshold for RX.
> > - VIRTIO_NET_CTRL_NOTF_COAL_HIGH_TX_SET - coalescing parameters
> > to use when the packet rate is equal or higher then the high
> > threshold for TX.
> > - VIRTIO_NET_CTRL_NOTF_COAL_HIGH_RX_SET - coalescing parameters
> > to use when the packet rate is equal or higher then the high
> > threshold for RX.
> >
> > On top of the new feature, this patch:
> > - Unifies the virtio_net_ctrl_coal structs, since the one for tx
> > and the one for rx are identical.
> > - Clarifies that the coalescing commands are best-effort.
> > - Specifies coalescing in respect to delivering interrupts when config
> > changes.
> >
> They are best to be handled a pre-patch to this one.
> Please split them so that spec improvement work can progress while new
> features are under review.
Yea it's not a bad idea. I figured it out but
> >
> > +\item[VIRTIO_NET_F_NOTF_COAL_LOW_HIGH(50)] Device supports
> > notifications coalescing low rate and high rate sets.
> > +
> Even though historically, many feature bits exists, adding more feature bits
> is not scalable.
I disagree here. We have unlimited feature bits and I see no problem
with their scalability. Devices (such as SIOV as opposed to SRIOV) which
have very limited memory need to use the proposed vq transport
that intel's working on, pci transport is wasting too much memory
anyway. I hope we are not going to repeat this discussion for each and
every new feature :)
> It is also limits to enable such functionality at very early stage in the
> device configuration.
Not really, it just handles compatibility and enables the commands.
> So, if at all this proceeds, we want the new admin q to discover and
> negotiate the new features.
> (Instead of features bits).
Sounds like a new transport. I wouldn't make it gate this proposal.
> > \item[VIRTIO_NET_F_NOTF_COAL(53)] Device supports notifications
> > coalescing.
> >
> > \item[VIRTIO_NET_F_GUEST_USO4 (54)] Driver can receive USOv4 packets.
> > @@ -3142,6 +3144,7 @@ \subsubsection{Feature bit
> > requirements}\label{sec:Device Types / Network Device
> > \item[VIRTIO_NET_F_MQ] Requires VIRTIO_NET_F_CTRL_VQ.
> > \item[VIRTIO_NET_F_CTRL_MAC_ADDR] Requires VIRTIO_NET_F_CTRL_VQ.
> > \item[VIRTIO_NET_F_NOTF_COAL] Requires VIRTIO_NET_F_CTRL_VQ.
> > +\item[VIRTIO_NET_F_NOTF_COAL_LOW_HIGH] Requires
> > VIRTIO_NET_F_NOTF_COAL.
> > \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.
> > \end{description}
> > @@ -4493,43 +4496,62 @@ \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.
> >
> > +Note: In general, these commands are best-effort: A device could send a
> > notification even if it is not supposed to.
> > +
> > \begin{lstlisting}
> > -struct virtio_net_ctrl_coal_rx {
> > -le32 rx_max_packets;
> > -le32 rx_usecs;
> > +struct virtio_net_ctrl_coal {
> > +le32 max_packets;
> > +le32 usecs;
> > };
> >
> Please put this change in a pre-patch along with additional rework of below.
>
>
> > -struct virtio_net_ctrl_coal_tx {
> > -le32 tx_max_packets;
> > -le32 tx_usecs;
> > +struct virtio_net_ctrl_coal_threshold {
> > +le32 pkt_rate;
> > +struct virtio_net_ctrl_coal params;
> > };
> >
> > #define VIRTIO_NET_CTRL_NOTF_COAL 6
> > #define VIRTIO_NET_CTRL_NOTF_COAL_TX_SET 0
> > #define VIRTIO_NET_CTRL_NOTF_COAL_RX_SET 1
> > + #define VIRTIO_NET_CTRL_NOTF_COAL_LOW_TX_SET 2 //Only if
> > + VIRTIO_NET_F_NOTF_COAL_LOW_HIGH
RE: [virtio-dev] [PATCH v2] virtio_net: support low and high rate of notification coalescing
Hi Alvaro,
> From: virtio-dev@lists.oasis-open.org On
> Behalf Of Alvaro Karsz
>
> This patch adds a new feature, VIRTIO_NET_F_NOTF_COAL_LOW_HIGH, while
> clarifying/fixing existing coalescing concepts.
>
While its visible in the patch itself, what is being done,
its important to mention the problem statement/motivation for the feature at
start of the commit log as first step.
So, can you please describe the motivation, what problem does the new command
solve?
It is likely that control entity which want to set different coalescing
parameters likely wants to more than just two values.
This is something already supported today.
> The new feature adds 4 new commands to VIRTIO_NET_CTRL_NOTF_COAL
> class:
> - VIRTIO_NET_CTRL_NOTF_COAL_LOW_TX_SET - coalescing
> parameters
> to use when the packet rate is equal or lower then the low
> threshold for TX.
> - VIRTIO_NET_CTRL_NOTF_COAL_LOW_RX_SET - coalescing parameters
> to use when the packet rate is equal or lower then the low
> threshold for RX.
> - VIRTIO_NET_CTRL_NOTF_COAL_HIGH_TX_SET - coalescing parameters
> to use when the packet rate is equal or higher then the high
> threshold for TX.
> - VIRTIO_NET_CTRL_NOTF_COAL_HIGH_RX_SET - coalescing parameters
> to use when the packet rate is equal or higher then the high
> threshold for RX.
>
> On top of the new feature, this patch:
> - Unifies the virtio_net_ctrl_coal structs, since the one for tx
> and the one for rx are identical.
> - Clarifies that the coalescing commands are best-effort.
> - Specifies coalescing in respect to delivering interrupts when config
> changes.
>
They are best to be handled a pre-patch to this one.
Please split them so that spec improvement work can progress while new features
are under review.
>
> +\item[VIRTIO_NET_F_NOTF_COAL_LOW_HIGH(50)] Device supports
> notifications coalescing low rate and high rate sets.
> +
Even though historically, many feature bits exists, adding more feature bits is
not scalable.
It is also limits to enable such functionality at very early stage in the
device configuration.
So, if at all this proceeds, we want the new admin q to discover and negotiate
the new features.
(Instead of features bits).
> \item[VIRTIO_NET_F_NOTF_COAL(53)] Device supports notifications
> coalescing.
>
> \item[VIRTIO_NET_F_GUEST_USO4 (54)] Driver can receive USOv4 packets.
> @@ -3142,6 +3144,7 @@ \subsubsection{Feature bit
> requirements}\label{sec:Device Types / Network Device
> \item[VIRTIO_NET_F_MQ] Requires VIRTIO_NET_F_CTRL_VQ.
> \item[VIRTIO_NET_F_CTRL_MAC_ADDR] Requires VIRTIO_NET_F_CTRL_VQ.
> \item[VIRTIO_NET_F_NOTF_COAL] Requires VIRTIO_NET_F_CTRL_VQ.
> +\item[VIRTIO_NET_F_NOTF_COAL_LOW_HIGH] Requires
> VIRTIO_NET_F_NOTF_COAL.
> \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.
> \end{description}
> @@ -4493,43 +4496,62 @@ \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.
>
> +Note: In general, these commands are best-effort: A device could send a
> notification even if it is not supposed to.
> +
> \begin{lstlisting}
> -struct virtio_net_ctrl_coal_rx {
> -le32 rx_max_packets;
> -le32 rx_usecs;
> +struct virtio_net_ctrl_coal {
> +le32 max_packets;
> +le32 usecs;
> };
>
Please put this change in a pre-patch along with additional rework of below.
> -struct virtio_net_ctrl_coal_tx {
> -le32 tx_max_packets;
> -le32 tx_usecs;
> +struct virtio_net_ctrl_coal_threshold {
> +le32 pkt_rate;
> +struct virtio_net_ctrl_coal params;
> };
>
> #define VIRTIO_NET_CTRL_NOTF_COAL 6
> #define VIRTIO_NET_CTRL_NOTF_COAL_TX_SET 0
> #define VIRTIO_NET_CTRL_NOTF_COAL_RX_SET 1
> + #define VIRTIO_NET_CTRL_NOTF_COAL_LOW_TX_SET 2 //Only if
> + VIRTIO_NET_F_NOTF_COAL_LOW_HIGH negotiated #define
> + VIRTIO_NET_CTRL_NOTF_COAL_LOW_RX_SET 3 //Only if
> + VIRTIO_NET_F_NOTF_COAL_LOW_HIGH negotiated #define
> + VIRTIO_NET_CTRL_NOTF_COAL_HIGH_TX_SET 4 //Only if
> + VIRTIO_NET_F_NOTF_COAL_LOW_HIGH negotiated #define
> + VIRTIO_NET_CTRL_NOTF_COAL_HIGH_RX_SET 5 //Only if
> + VIRTIO_NET_F_NOTF_COAL_LOW_HIGH negotiated
> \end{lstlisting}
>
> -Coalescing parameters:
> +TX Coalescing parameters:
> +\begin{itemize}
> +\item \field{usecs}: Maximum number of usecs to delay a TX notification.
> +\item \field{max_packets}: Maximum number of packets to send before a TX
> notification.
> +\end{itemize}
> +
> +RX Coalescing parameters:
> +\begin{itemize}
> +\item \field{usecs}: Maximum number of usecs to delay a RX notification.
> +\item \field{max_packets}: Maximum number of packets to receive before a
> RX notification.
> +\end{itemize}
>
Re: [virtio-dev] Re: [PATCH RFC 0/3] virtio-rng based entropy leak reporting
On Mon, Jan 16, 2023 at 05:45:40PM +0100, Jason A. Donenfeld wrote: > Hey guys, > > Just FYI, I am still interested in this, but it's currently taken a > backseat while I focus on some other parts of the ecosystem. I'll be > back to moving this forward in March. > > Jason OK sure, no rush. I will drop this for now and just ping me when there's interest. -- 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: [PATCH v2] virtio_net: support low and high rate of notification coalescing
On Mon, Jan 16, 2023 at 04:32:11PM +0200, Alvaro Karsz wrote: > ping You only posted v2 Wednesday, that's just 2 days for people to review. Sit tight next time pls :) -- 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: [PATCH v2] virtio_net: support low and high rate of notification coalescing
On Wed, Jan 11, 2023 at 05:21:23PM +0200, Alvaro Karsz wrote:
> This patch adds a new feature, VIRTIO_NET_F_NOTF_COAL_LOW_HIGH, while
> clarifying/fixing existing coalescing concepts.
>
> The new feature adds 4 new commands to VIRTIO_NET_CTRL_NOTF_COAL class:
> - VIRTIO_NET_CTRL_NOTF_COAL_LOW_TX_SET - coalescing parameters
> to use when the packet rate is equal or lower then the low
> threshold for TX.
> - VIRTIO_NET_CTRL_NOTF_COAL_LOW_RX_SET - coalescing parameters
> to use when the packet rate is equal or lower then the low
> threshold for RX.
> - VIRTIO_NET_CTRL_NOTF_COAL_HIGH_TX_SET - coalescing parameters
> to use when the packet rate is equal or higher then the high
> threshold for TX.
> - VIRTIO_NET_CTRL_NOTF_COAL_HIGH_RX_SET - coalescing parameters
> to use when the packet rate is equal or higher then the high
> threshold for RX.
>
> On top of the new feature, this patch:
> - Unifies the virtio_net_ctrl_coal structs, since the one for tx
> and the one for rx are identical.
> - Clarifies that the coalescing commands are best-effort.
> - Specifies coalescing in respect to delivering interrupts when config
> changes.
>
> Signed-off-by: Alvaro Karsz
> ---
> v2:
> - Remove the "set of coalescing parameters" concept, use
> "coalescing parameters" instead.
> - Unify struct virtio_net_ctrl_coal_rx and strcut
> virtio_net_ctrl_coal_tx
> to struct virtio_net_ctrl_coal.
> - Separate the commands to tx and rx, so devices could have
> different low/high rate coalescing parameters for tx and rx.
> - Unify struct virtio_net_ctrl_coal_high and struct
> virtio_net_ctrl_coal_low to struct virtio_net_ctrl_coal_threshold.
> - Clarify that the packet rate is in packet per second units.
> - Clarify that any notification coalescing command is best-effort.
> - Specify coalescing in respect to delivering interrupts when config
> changes.
> ---
> content.tex | 105 +++-
> 1 file changed, 87 insertions(+), 18 deletions(-)
>
> diff --git a/content.tex b/content.tex
> index 96f4723..82b2597 100644
> --- a/content.tex
> +++ b/content.tex
> @@ -3088,6 +3088,8 @@ \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_NOTF_COAL_LOW_HIGH(50)] Device supports notifications
> coalescing low rate and high rate sets.
> +
> \item[VIRTIO_NET_F_NOTF_COAL(53)] Device supports notifications coalescing.
>
> \item[VIRTIO_NET_F_GUEST_USO4 (54)] Driver can receive USOv4 packets.
> @@ -3142,6 +3144,7 @@ \subsubsection{Feature bit
> requirements}\label{sec:Device Types / Network Device
> \item[VIRTIO_NET_F_MQ] Requires VIRTIO_NET_F_CTRL_VQ.
> \item[VIRTIO_NET_F_CTRL_MAC_ADDR] Requires VIRTIO_NET_F_CTRL_VQ.
> \item[VIRTIO_NET_F_NOTF_COAL] Requires VIRTIO_NET_F_CTRL_VQ.
> +\item[VIRTIO_NET_F_NOTF_COAL_LOW_HIGH] Requires VIRTIO_NET_F_NOTF_COAL.
> \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.
> \end{description}
> @@ -4493,43 +4496,62 @@ \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.
>
> +Note: In general, these commands are best-effort: A device could send a
> notification even if it is not supposed to.
> +
> \begin{lstlisting}
> -struct virtio_net_ctrl_coal_rx {
> -le32 rx_max_packets;
> -le32 rx_usecs;
> +struct virtio_net_ctrl_coal {
> +le32 max_packets;
> +le32 usecs;
> };
>
> -struct virtio_net_ctrl_coal_tx {
> -le32 tx_max_packets;
> -le32 tx_usecs;
> +struct virtio_net_ctrl_coal_threshold {
> +le32 pkt_rate;
> +struct virtio_net_ctrl_coal params;
> };
>
> #define VIRTIO_NET_CTRL_NOTF_COAL 6
> #define VIRTIO_NET_CTRL_NOTF_COAL_TX_SET 0
> #define VIRTIO_NET_CTRL_NOTF_COAL_RX_SET 1
> + #define VIRTIO_NET_CTRL_NOTF_COAL_LOW_TX_SET 2 //Only if
> VIRTIO_NET_F_NOTF_COAL_LOW_HIGH negotiated
> + #define VIRTIO_NET_CTRL_NOTF_COAL_LOW_RX_SET 3 //Only if
> VIRTIO_NET_F_NOTF_COAL_LOW_HIGH negotiated
> + #define VIRTIO_NET_CTRL_NOTF_COAL_HIGH_TX_SET 4 //Only if
> VIRTIO_NET_F_NOTF_COAL_LOW_HIGH negotiated
> + #define VIRTIO_NET_CTRL_NOTF_COAL_HIGH_RX_SET 5 //Only if
> VIRTIO_NET_F_NOTF_COAL_LOW_HIGH negotiated
add conformance statements for this requirement of
VIRTIO_NET_F_NOTF_COAL_LOW_HIGH too?
> \end{lstlisting}
>
> -Coalescing parameters:
> +TX Coalescing parameters:
> +\begin{itemize}
> +\item \field{usecs}: Maximum number of usecs to delay a TX notification.
> +\item
[virtio-dev] Re: [PATCH v2] virtio_net: support low and high rate of notification coalescing
ping - To unsubscribe, e-mail: virtio-dev-unsubscr...@lists.oasis-open.org For additional commands, e-mail: virtio-dev-h...@lists.oasis-open.org
Re: [virtio-dev] Re: [PATCH RFC 0/3] virtio-rng based entropy leak reporting
Hi Michael, On 12/1/23 08:02, Michael S. Tsirkin wrote: CAUTION: This email originated from outside of the organization. Do not click links or open attachments unless you can confirm the sender and know the content is safe. On Mon, Nov 21, 2022 at 11:30:19AM -0500, Michael S. Tsirkin wrote: Generally, entropy only grows. However, there are cases where it goes down - for example, consider generating a one time pad where someone managed to use a side channel to steal its contents. By combining the seemingly random pad with the stolen contents we have reversed the entropy. This actually happens within VMs e.g. when time is reversed due to snapshoting. Existing approaches for VMs include Microsoft's VM GEN ID. This draft proposes a feature in virtio rng for reporting such leaks. Patches 1,2 refactor existing draft text. Patch 3 adds new functionality. TODO: document theory of operation add conformance clauses Guys any input on this? Anyone going to use this? I plan to post an RFC patch for linux virtio-rng show-casing this with Firecracker, this week. Also, I had sent an e-mail: https://www.mail-archive.com/virtio-dev@lists.oasis-open.org/msg09128.html with some questions, not sure whether you missed it? Michael S. Tsirkin (3): rng: move to a file of its own rng: be specific about the virtqueue rng: leak detection support content.tex| 43 + virtio-rng.tex | 102 + 2 files changed, 103 insertions(+), 42 deletions(-) create mode 100644 virtio-rng.tex -- MST - To unsubscribe, e-mail: virtio-dev-unsubscr...@lists.oasis-open.org For additional commands, e-mail: virtio-dev-h...@lists.oasis-open.org Cheers, Babis Amazon Spain Services sociedad limitada unipersonal, Calle Ramirez de Prado 5, 28045 Madrid. Registro Mercantil de Madrid . Tomo 22458 . Folio 102 . Hoja M-401234 . CIF B84570936
[virtio-dev] Re: [virtio-comment] Re: [PATCH v7] virtio-net: support inner header hash
在 2023/1/16 16:01, Heng Qi 写道:
On Wed, Jan 11, 2023 at 04:45:12AM -0500, Michael S. Tsirkin wrote:
On Wed, Jan 04, 2023 at 03:14:01PM +0800, Heng Qi wrote:
If the tunnel is used to encapsulate the packets, the hash calculated
using the outer header of the receive packets is always fixed for the
same flow packets, i.e. they will be steered to the same receive queue.
We add a feature bit VIRTIO_NET_F_HASH_TUNNEL and related bitmasks
in \field{hash_types}, which instructs the device to calculate the
hash using the inner headers of tunnel-encapsulated packets. Besides,
values in \field{hash_report_tunnel} are added to report tunnel types.
Fixes: https://github.com/oasis-tcs/virtio-spec/issues/151
Reviewed-by: Jason Wang
Signed-off-by: Heng Qi
Signed-off-by: Xuan Zhuo
---
v6:
1. Modify the wording of some sentences for clarity. @Michael S. Tsirkin
2. Fix some syntax issues. @Michael S. Tsirkin
v5:
1. Fix some syntax and capitalization issues. @Michael S. Tsirkin
2. Use encapsulated/encaptulation uniformly. @Michael S. Tsirkin
3. Move the links to introduction section. @Michael S. Tsirkin
4. Clarify some sentences. @Michael S. Tsirkin
v4:
1. Clarify some paragraphs. @Cornelia Huck
2. Fix the u8 type. @Cornelia Huck
v3:
1. Rename VIRTIO_NET_F_HASH_GRE_VXLAN_GENEVE_INNER to
VIRTIO_NET_F_HASH_TUNNEL. @Jason Wang
2. Make things clearer. @Jason Wang @Michael S. Tsirkin
3. Keep the possibility to use inner hash for automatic receive
steering. @Jason Wang
4. Add the "Tunnel packet" paragraph to avoid repeating the GRE etc.
many times. @Michael S. Tsirkin
v2:
1. Add a feature bit for GRE/VXLAN/GENEVE inner hash. @Jason Wang
2. Chang \field{hash_tunnel} to \field{hash_report_tunnel}. @Jason
Wang, @Michael S. Tsirkin
v1:
1. Remove the patch for the bitmask fix. @Michael S. Tsirkin
2. Clarify some paragraphs. @Jason Wang
3. Add \field{hash_tunnel} and VIRTIO_NET_HASH_REPORT_GRE. @Yuri
Benditovich
content.tex | 191 +--
introduction.tex | 19 +
2 files changed, 203 insertions(+), 7 deletions(-)
diff --git a/content.tex b/content.tex
index e863709..7845f6c 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_HASH_TUNNEL(52)] Device supports inner
+header hash for GRE, VXLAN and GENEVE tunnel-encapsulated packets.
+
\item[VIRTIO_NET_F_NOTF_COAL(53)] Device supports notifications coalescing.
\item[VIRTIO_NET_F_GUEST_USO4 (54)] Driver can receive USOv4 packets.
@@ -3095,7 +3098,8 @@ \subsection{Feature bits}\label{sec:Device Types /
Network Device / Feature bits
to several segments when each of these smaller packets has UDP header.
\item[VIRTIO_NET_F_HASH_REPORT(57)] Device can report per-packet hash
-value and a type of calculated hash.
+value, a type of calculated hash, and, if VIRTIO_NET_F_HASH_TUNNEL
+is negotiated, an encapsulation packet type.
\item[VIRTIO_NET_F_GUEST_HDRLEN(59)] Driver can provide the exact \field{hdr_len}
value. Device benefits from knowing the exact header length.
@@ -3140,6 +3144,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_HASH_TUNNEL] Requires VIRTIO_NET_F_CTRL_VQ.
\end{description}
\subsubsection{Legacy Interface: Feature bits}\label{sec:Device Types / Network Device / Feature bits / Legacy Interface: Feature bits}
@@ -3386,7 +3391,8 @@ \subsection{Device Operation}\label{sec:Device Types /
Network Device / Device O
le16 num_buffers;
le32 hash_value;(Only if VIRTIO_NET_F_HASH_REPORT negotiated)
le16 hash_report; (Only if VIRTIO_NET_F_HASH_REPORT negotiated)
-le16 padding_reserved; (Only if VIRTIO_NET_F_HASH_REPORT negotiated)
+u8 hash_report_tunnel; (Only if VIRTIO_NET_F_HASH_REPORT negotiated,
only valid of VIRTIO_NET_F_HASH_TUNNEL negotiated)
of -> if?
Sorry for the late reply.
It's Cornelia's suggestion, and 'of' seems to work fine.
also let's add: otherwise reserved?
Sure.
+u8 padding_reserved;(Only if VIRTIO_NET_F_HASH_REPORT negotiated)
};
\end{lstlisting}
@@ -3837,7 +3843,8 @@ \subsubsection{Processing of Incoming Packets}\label{sec:Device Types / Network
A device attempts to calculate a per-packet hash in the following cases:
\begin{itemize}
\item The feature VIRTIO_NET_F_RSS was negotiated. The device uses the hash
to determine the receive virtqueue to
[virtio-dev] Re: [virtio-comment] Re: [PATCH v7] virtio-net: support inner header hash
On Mon, Jan 16 2023, Heng Qi wrote:
> On Wed, Jan 11, 2023 at 04:45:12AM -0500, Michael S. Tsirkin wrote:
>> On Wed, Jan 04, 2023 at 03:14:01PM +0800, Heng Qi wrote:
>> > @@ -3386,7 +3391,8 @@ \subsection{Device Operation}\label{sec:Device Types
>> > / Network Device / Device O
>> > le16 num_buffers;
>> > le32 hash_value;(Only if VIRTIO_NET_F_HASH_REPORT
>> > negotiated)
>> > le16 hash_report; (Only if VIRTIO_NET_F_HASH_REPORT
>> > negotiated)
>> > -le16 padding_reserved; (Only if VIRTIO_NET_F_HASH_REPORT
>> > negotiated)
>> > +u8 hash_report_tunnel; (Only if VIRTIO_NET_F_HASH_REPORT
>> > negotiated, only valid of VIRTIO_NET_F_HASH_TUNNEL negotiated)
>>
>> of -> if?
>
> Sorry for the late reply.
> It's Cornelia's suggestion, and 'of' seems to work fine.
I think that was simply a typo, should have been 'if'.
-
To unsubscribe, e-mail: virtio-dev-unsubscr...@lists.oasis-open.org
For additional commands, e-mail: virtio-dev-h...@lists.oasis-open.org
