DPDK-dev Archive on lore.kernel.org
 help / color / Atom feed
* [dpdk-dev] [PATCH v2] net/virtio: add link speed tuning
       [not found] <CGME20191213144450eucas1p189850559438d6f9d5ead48b645e27a79@eucas1p1.samsung.com>
@ 2019-12-13 14:44 ` Ivan Dyukov
  2019-12-13 14:59   ` Maxime Coquelin
  0 siblings, 1 reply; 8+ messages in thread
From: Ivan Dyukov @ 2019-12-13 14:44 UTC (permalink / raw)
  To: dev, maxime.coquelin, tiwei.bie; +Cc: Ivan Dyukov

Some applications like pktgen use link_speed to calculate transmit
rate. It limits outcome traffic to hardcoded 10G.

This patch makes link_speed configurable at compile time.

Signed-off-by: Ivan Dyukov <i.dyukov@samsung.com>
---
 config/common_base                 |  1 +
 config/meson.build                 |  1 +
 drivers/net/vhost/rte_eth_vhost.c  |  2 +-
 drivers/net/virtio/virtio_ethdev.c | 20 ++++++++++++++++----
 4 files changed, 19 insertions(+), 5 deletions(-)

diff --git a/config/common_base b/config/common_base
index 7dec7ed45..8589ca4ec 100644
--- a/config/common_base
+++ b/config/common_base
@@ -433,6 +433,7 @@ CONFIG_RTE_LIBRTE_AVP_DEBUG_BUFFERS=n
 # Compile burst-oriented VIRTIO PMD driver
 #
 CONFIG_RTE_LIBRTE_VIRTIO_PMD=y
+CONFIG_RTE_LIBRTE_VIRTIO_LINK_SPEED=10000
 CONFIG_RTE_LIBRTE_VIRTIO_DEBUG_RX=n
 CONFIG_RTE_LIBRTE_VIRTIO_DEBUG_TX=n
 CONFIG_RTE_LIBRTE_VIRTIO_DEBUG_DUMP=n
diff --git a/config/meson.build b/config/meson.build
index 364a8d739..78c30f334 100644
--- a/config/meson.build
+++ b/config/meson.build
@@ -202,6 +202,7 @@ dpdk_conf.set('RTE_LIBEAL_USE_HPET', get_option('use_hpet'))
 dpdk_conf.set('RTE_MAX_VFIO_GROUPS', 64)
 dpdk_conf.set('RTE_DRIVER_MEMPOOL_BUCKET_SIZE_KB', 64)
 dpdk_conf.set('RTE_LIBRTE_DPAA2_USE_PHYS_IOVA', true)
+dpdk_conf.set('RTE_LIBRTE_VIRTIO_LINK_SPEED', 10000)
 
 
 compile_time_cpuflags = []
diff --git a/drivers/net/vhost/rte_eth_vhost.c b/drivers/net/vhost/rte_eth_vhost.c
index 46f01a7f4..38eaa5955 100644
--- a/drivers/net/vhost/rte_eth_vhost.c
+++ b/drivers/net/vhost/rte_eth_vhost.c
@@ -115,7 +115,7 @@ static struct internal_list_head internal_list =
 static pthread_mutex_t internal_list_lock = PTHREAD_MUTEX_INITIALIZER;
 
 static struct rte_eth_link pmd_link = {
-		.link_speed = 10000,
+		.link_speed = RTE_LIBRTE_VIRTIO_LINK_SPEED,
 		.link_duplex = ETH_LINK_FULL_DUPLEX,
 		.link_status = ETH_LINK_DOWN
 };
diff --git a/drivers/net/virtio/virtio_ethdev.c b/drivers/net/virtio/virtio_ethdev.c
index 044eb10a7..948091cc2 100644
--- a/drivers/net/virtio/virtio_ethdev.c
+++ b/drivers/net/virtio/virtio_ethdev.c
@@ -2371,7 +2371,7 @@ virtio_dev_link_update(struct rte_eth_dev *dev, __rte_unused int wait_to_complet
 
 	memset(&link, 0, sizeof(link));
 	link.link_duplex = ETH_LINK_FULL_DUPLEX;
-	link.link_speed  = ETH_SPEED_NUM_10G;
+	link.link_speed  = RTE_LIBRTE_VIRTIO_LINK_SPEED;
 	link.link_autoneg = ETH_LINK_FIXED;
 
 	if (!hw->started) {
@@ -2426,9 +2426,21 @@ virtio_dev_info_get(struct rte_eth_dev *dev, struct rte_eth_dev_info *dev_info)
 {
 	uint64_t tso_mask, host_features;
 	struct virtio_hw *hw = dev->data->dev_private;
-
-	dev_info->speed_capa = ETH_LINK_SPEED_10G; /* fake value */
-
+#if RTE_LIBRTE_VIRTIO_LINK_SPEED == ETH_LINK_SPEED_20G
+	dev_info->speed_capa = ETH_LINK_SPEED_20G;
+#elif RTE_LIBRTE_VIRTIO_LINK_SPEED == ETH_SPEED_NUM_25G
+	dev_info->speed_capa = ETH_LINK_SPEED_25G;
+#elif RTE_LIBRTE_VIRTIO_LINK_SPEED == ETH_SPEED_NUM_40G
+	dev_info->speed_capa = ETH_LINK_SPEED_40G;
+#elif RTE_LIBRTE_VIRTIO_LINK_SPEED == ETH_SPEED_NUM_50G
+	dev_info->speed_capa = ETH_LINK_SPEED_50G;
+#elif RTE_LIBRTE_VIRTIO_LINK_SPEED == ETH_SPEED_NUM_56G
+	dev_info->speed_capa = ETH_LINK_SPEED_56G;
+#elif RTE_LIBRTE_VIRTIO_LINK_SPEED == ETH_SPEED_NUM_100G
+	dev_info->speed_capa = ETH_LINK_SPEED_100G;
+#else
+	dev_info->speed_capa = ETH_LINK_SPEED_10G;
+#endif
 	dev_info->max_rx_queues =
 		RTE_MIN(hw->max_queue_pairs, VIRTIO_MAX_RX_QUEUES);
 	dev_info->max_tx_queues =
-- 
2.17.1


^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [dpdk-dev] [PATCH v2] net/virtio: add link speed tuning
  2019-12-13 14:44 ` [dpdk-dev] [PATCH v2] net/virtio: add link speed tuning Ivan Dyukov
@ 2019-12-13 14:59   ` Maxime Coquelin
  2019-12-13 16:26     ` Stephen Hemminger
  2019-12-13 17:39     ` Ivan Dyukov
  0 siblings, 2 replies; 8+ messages in thread
From: Maxime Coquelin @ 2019-12-13 14:59 UTC (permalink / raw)
  To: Ivan Dyukov, dev, tiwei.bie

Hi Ivan,

On 12/13/19 3:44 PM, Ivan Dyukov wrote:
> Some applications like pktgen use link_speed to calculate transmit
> rate. It limits outcome traffic to hardcoded 10G.
> 
> This patch makes link_speed configurable at compile time.
> 
> Signed-off-by: Ivan Dyukov <i.dyukov@samsung.com>
> ---
>  config/common_base                 |  1 +
>  config/meson.build                 |  1 +
>  drivers/net/vhost/rte_eth_vhost.c  |  2 +-
>  drivers/net/virtio/virtio_ethdev.c | 20 ++++++++++++++++----
>  4 files changed, 19 insertions(+), 5 deletions(-)
> 
> diff --git a/config/common_base b/config/common_base
> index 7dec7ed45..8589ca4ec 100644
> --- a/config/common_base
> +++ b/config/common_base
> @@ -433,6 +433,7 @@ CONFIG_RTE_LIBRTE_AVP_DEBUG_BUFFERS=n
>  # Compile burst-oriented VIRTIO PMD driver
>  #
>  CONFIG_RTE_LIBRTE_VIRTIO_PMD=y
> +CONFIG_RTE_LIBRTE_VIRTIO_LINK_SPEED=10000
>  CONFIG_RTE_LIBRTE_VIRTIO_DEBUG_RX=n
>  CONFIG_RTE_LIBRTE_VIRTIO_DEBUG_TX=n
>  CONFIG_RTE_LIBRTE_VIRTIO_DEBUG_DUMP=n
> diff --git a/config/meson.build b/config/meson.build
> index 364a8d739..78c30f334 100644
> --- a/config/meson.build
> +++ b/config/meson.build
> @@ -202,6 +202,7 @@ dpdk_conf.set('RTE_LIBEAL_USE_HPET', get_option('use_hpet'))
>  dpdk_conf.set('RTE_MAX_VFIO_GROUPS', 64)
>  dpdk_conf.set('RTE_DRIVER_MEMPOOL_BUCKET_SIZE_KB', 64)
>  dpdk_conf.set('RTE_LIBRTE_DPAA2_USE_PHYS_IOVA', true)
> +dpdk_conf.set('RTE_LIBRTE_VIRTIO_LINK_SPEED', 10000)
>  
>  
>  compile_time_cpuflags = []
> diff --git a/drivers/net/vhost/rte_eth_vhost.c b/drivers/net/vhost/rte_eth_vhost.c
> index 46f01a7f4..38eaa5955 100644
> --- a/drivers/net/vhost/rte_eth_vhost.c
> +++ b/drivers/net/vhost/rte_eth_vhost.c
> @@ -115,7 +115,7 @@ static struct internal_list_head internal_list =
>  static pthread_mutex_t internal_list_lock = PTHREAD_MUTEX_INITIALIZER;
>  
>  static struct rte_eth_link pmd_link = {
> -		.link_speed = 10000,
> +		.link_speed = RTE_LIBRTE_VIRTIO_LINK_SPEED,
>  		.link_duplex = ETH_LINK_FULL_DUPLEX,
>  		.link_status = ETH_LINK_DOWN
>  };
> diff --git a/drivers/net/virtio/virtio_ethdev.c b/drivers/net/virtio/virtio_ethdev.c
> index 044eb10a7..948091cc2 100644
> --- a/drivers/net/virtio/virtio_ethdev.c
> +++ b/drivers/net/virtio/virtio_ethdev.c
> @@ -2371,7 +2371,7 @@ virtio_dev_link_update(struct rte_eth_dev *dev, __rte_unused int wait_to_complet
>  
>  	memset(&link, 0, sizeof(link));
>  	link.link_duplex = ETH_LINK_FULL_DUPLEX;
> -	link.link_speed  = ETH_SPEED_NUM_10G;
> +	link.link_speed  = RTE_LIBRTE_VIRTIO_LINK_SPEED;
>  	link.link_autoneg = ETH_LINK_FIXED;
>  
>  	if (!hw->started) {
> @@ -2426,9 +2426,21 @@ virtio_dev_info_get(struct rte_eth_dev *dev, struct rte_eth_dev_info *dev_info)
>  {
>  	uint64_t tso_mask, host_features;
>  	struct virtio_hw *hw = dev->data->dev_private;
> -
> -	dev_info->speed_capa = ETH_LINK_SPEED_10G; /* fake value */
> -
> +#if RTE_LIBRTE_VIRTIO_LINK_SPEED == ETH_LINK_SPEED_20G
> +	dev_info->speed_capa = ETH_LINK_SPEED_20G;
> +#elif RTE_LIBRTE_VIRTIO_LINK_SPEED == ETH_SPEED_NUM_25G
> +	dev_info->speed_capa = ETH_LINK_SPEED_25G;
> +#elif RTE_LIBRTE_VIRTIO_LINK_SPEED == ETH_SPEED_NUM_40G
> +	dev_info->speed_capa = ETH_LINK_SPEED_40G;
> +#elif RTE_LIBRTE_VIRTIO_LINK_SPEED == ETH_SPEED_NUM_50G
> +	dev_info->speed_capa = ETH_LINK_SPEED_50G;
> +#elif RTE_LIBRTE_VIRTIO_LINK_SPEED == ETH_SPEED_NUM_56G
> +	dev_info->speed_capa = ETH_LINK_SPEED_56G;
> +#elif RTE_LIBRTE_VIRTIO_LINK_SPEED == ETH_SPEED_NUM_100G
> +	dev_info->speed_capa = ETH_LINK_SPEED_100G;
> +#else
> +	dev_info->speed_capa = ETH_LINK_SPEED_10G;
> +#endif
>  	dev_info->max_rx_queues =
>  		RTE_MIN(hw->max_queue_pairs, VIRTIO_MAX_RX_QUEUES);
>  	dev_info->max_tx_queues =
> 

I think we may need toi extend the Virtio specification so that the
device can advertise the link speed.

Problem with your proposal is that it is build time only, so:
1. It won't be configurable when using distros DPDK package.
2. All the Virtio devices on a system will have the same value

While any Virtio spec update introduce link speed support, wouldn't it
be preferable to have this as a devarg?

Thanks,
Maxime


^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [dpdk-dev] [PATCH v2] net/virtio: add link speed tuning
  2019-12-13 14:59   ` Maxime Coquelin
@ 2019-12-13 16:26     ` Stephen Hemminger
  2019-12-13 16:31       ` Maxime Coquelin
  2019-12-13 17:39     ` Ivan Dyukov
  1 sibling, 1 reply; 8+ messages in thread
From: Stephen Hemminger @ 2019-12-13 16:26 UTC (permalink / raw)
  To: Maxime Coquelin; +Cc: Ivan Dyukov, dev, tiwei.bie

On Fri, 13 Dec 2019 15:59:23 +0100
Maxime Coquelin <maxime.coquelin@redhat.com> wrote:

> Hi Ivan,
> 
> On 12/13/19 3:44 PM, Ivan Dyukov wrote:
> > Some applications like pktgen use link_speed to calculate transmit
> > rate. It limits outcome traffic to hardcoded 10G.
> > 
> > This patch makes link_speed configurable at compile time.
> > 
> > Signed-off-by: Ivan Dyukov <i.dyukov@samsung.com>
> > ---
> >  config/common_base                 |  1 +
> >  config/meson.build                 |  1 +
> >  drivers/net/vhost/rte_eth_vhost.c  |  2 +-
> >  drivers/net/virtio/virtio_ethdev.c | 20 ++++++++++++++++----
> >  4 files changed, 19 insertions(+), 5 deletions(-)
> > 
> > diff --git a/config/common_base b/config/common_base
> > index 7dec7ed45..8589ca4ec 100644
> > --- a/config/common_base
> > +++ b/config/common_base
> > @@ -433,6 +433,7 @@ CONFIG_RTE_LIBRTE_AVP_DEBUG_BUFFERS=n
> >  # Compile burst-oriented VIRTIO PMD driver
> >  #
> >  CONFIG_RTE_LIBRTE_VIRTIO_PMD=y
> > +CONFIG_RTE_LIBRTE_VIRTIO_LINK_SPEED=10000
> >  CONFIG_RTE_LIBRTE_VIRTIO_DEBUG_RX=n
> >  CONFIG_RTE_LIBRTE_VIRTIO_DEBUG_TX=n
> >  CONFIG_RTE_LIBRTE_VIRTIO_DEBUG_DUMP=n
> > diff --git a/config/meson.build b/config/meson.build
> > index 364a8d739..78c30f334 100644
> > --- a/config/meson.build
> > +++ b/config/meson.build
> > @@ -202,6 +202,7 @@ dpdk_conf.set('RTE_LIBEAL_USE_HPET', get_option('use_hpet'))
> >  dpdk_conf.set('RTE_MAX_VFIO_GROUPS', 64)
> >  dpdk_conf.set('RTE_DRIVER_MEMPOOL_BUCKET_SIZE_KB', 64)
> >  dpdk_conf.set('RTE_LIBRTE_DPAA2_USE_PHYS_IOVA', true)
> > +dpdk_conf.set('RTE_LIBRTE_VIRTIO_LINK_SPEED', 10000)
> >  
> >  
> >  compile_time_cpuflags = []
> > diff --git a/drivers/net/vhost/rte_eth_vhost.c b/drivers/net/vhost/rte_eth_vhost.c
> > index 46f01a7f4..38eaa5955 100644
> > --- a/drivers/net/vhost/rte_eth_vhost.c
> > +++ b/drivers/net/vhost/rte_eth_vhost.c
> > @@ -115,7 +115,7 @@ static struct internal_list_head internal_list =
> >  static pthread_mutex_t internal_list_lock = PTHREAD_MUTEX_INITIALIZER;
> >  
> >  static struct rte_eth_link pmd_link = {
> > -		.link_speed = 10000,
> > +		.link_speed = RTE_LIBRTE_VIRTIO_LINK_SPEED,
> >  		.link_duplex = ETH_LINK_FULL_DUPLEX,
> >  		.link_status = ETH_LINK_DOWN
> >  };
> > diff --git a/drivers/net/virtio/virtio_ethdev.c b/drivers/net/virtio/virtio_ethdev.c
> > index 044eb10a7..948091cc2 100644
> > --- a/drivers/net/virtio/virtio_ethdev.c
> > +++ b/drivers/net/virtio/virtio_ethdev.c
> > @@ -2371,7 +2371,7 @@ virtio_dev_link_update(struct rte_eth_dev *dev, __rte_unused int wait_to_complet
> >  
> >  	memset(&link, 0, sizeof(link));
> >  	link.link_duplex = ETH_LINK_FULL_DUPLEX;
> > -	link.link_speed  = ETH_SPEED_NUM_10G;
> > +	link.link_speed  = RTE_LIBRTE_VIRTIO_LINK_SPEED;
> >  	link.link_autoneg = ETH_LINK_FIXED;
> >  
> >  	if (!hw->started) {
> > @@ -2426,9 +2426,21 @@ virtio_dev_info_get(struct rte_eth_dev *dev, struct rte_eth_dev_info *dev_info)
> >  {
> >  	uint64_t tso_mask, host_features;
> >  	struct virtio_hw *hw = dev->data->dev_private;
> > -
> > -	dev_info->speed_capa = ETH_LINK_SPEED_10G; /* fake value */
> > -
> > +#if RTE_LIBRTE_VIRTIO_LINK_SPEED == ETH_LINK_SPEED_20G
> > +	dev_info->speed_capa = ETH_LINK_SPEED_20G;
> > +#elif RTE_LIBRTE_VIRTIO_LINK_SPEED == ETH_SPEED_NUM_25G
> > +	dev_info->speed_capa = ETH_LINK_SPEED_25G;
> > +#elif RTE_LIBRTE_VIRTIO_LINK_SPEED == ETH_SPEED_NUM_40G
> > +	dev_info->speed_capa = ETH_LINK_SPEED_40G;
> > +#elif RTE_LIBRTE_VIRTIO_LINK_SPEED == ETH_SPEED_NUM_50G
> > +	dev_info->speed_capa = ETH_LINK_SPEED_50G;
> > +#elif RTE_LIBRTE_VIRTIO_LINK_SPEED == ETH_SPEED_NUM_56G
> > +	dev_info->speed_capa = ETH_LINK_SPEED_56G;
> > +#elif RTE_LIBRTE_VIRTIO_LINK_SPEED == ETH_SPEED_NUM_100G
> > +	dev_info->speed_capa = ETH_LINK_SPEED_100G;
> > +#else
> > +	dev_info->speed_capa = ETH_LINK_SPEED_10G;
> > +#endif
> >  	dev_info->max_rx_queues =
> >  		RTE_MIN(hw->max_queue_pairs, VIRTIO_MAX_RX_QUEUES);
> >  	dev_info->max_tx_queues =
> >   
> 
> I think we may need toi extend the Virtio specification so that the
> device can advertise the link speed.
> 
> Problem with your proposal is that it is build time only, so:
> 1. It won't be configurable when using distros DPDK package.
> 2. All the Virtio devices on a system will have the same value
> 
> While any Virtio spec update introduce link speed support, wouldn't it
> be preferable to have this as a devarg?
> 
> Thanks,
> Maxime
> 

Why does link speed matter at all?
If some code is looking at link speed to compute values like QoS then
it will be broken based on how virtualized NIC's work (bytes are free).

It all seems just like a workaround for broke legacy code or
advertising.

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [dpdk-dev] [PATCH v2] net/virtio: add link speed tuning
  2019-12-13 16:26     ` Stephen Hemminger
@ 2019-12-13 16:31       ` Maxime Coquelin
  0 siblings, 0 replies; 8+ messages in thread
From: Maxime Coquelin @ 2019-12-13 16:31 UTC (permalink / raw)
  To: Stephen Hemminger; +Cc: Ivan Dyukov, dev, tiwei.bie



On 12/13/19 5:26 PM, Stephen Hemminger wrote:
> On Fri, 13 Dec 2019 15:59:23 +0100
> Maxime Coquelin <maxime.coquelin@redhat.com> wrote:
> 
>> Hi Ivan,
>>
>> On 12/13/19 3:44 PM, Ivan Dyukov wrote:
>>> Some applications like pktgen use link_speed to calculate transmit
>>> rate. It limits outcome traffic to hardcoded 10G.
>>>
>>> This patch makes link_speed configurable at compile time.
>>>
>>> Signed-off-by: Ivan Dyukov <i.dyukov@samsung.com>
>>> ---
>>>  config/common_base                 |  1 +
>>>  config/meson.build                 |  1 +
>>>  drivers/net/vhost/rte_eth_vhost.c  |  2 +-
>>>  drivers/net/virtio/virtio_ethdev.c | 20 ++++++++++++++++----
>>>  4 files changed, 19 insertions(+), 5 deletions(-)
>>>
>>> diff --git a/config/common_base b/config/common_base
>>> index 7dec7ed45..8589ca4ec 100644
>>> --- a/config/common_base
>>> +++ b/config/common_base
>>> @@ -433,6 +433,7 @@ CONFIG_RTE_LIBRTE_AVP_DEBUG_BUFFERS=n
>>>  # Compile burst-oriented VIRTIO PMD driver
>>>  #
>>>  CONFIG_RTE_LIBRTE_VIRTIO_PMD=y
>>> +CONFIG_RTE_LIBRTE_VIRTIO_LINK_SPEED=10000
>>>  CONFIG_RTE_LIBRTE_VIRTIO_DEBUG_RX=n
>>>  CONFIG_RTE_LIBRTE_VIRTIO_DEBUG_TX=n
>>>  CONFIG_RTE_LIBRTE_VIRTIO_DEBUG_DUMP=n
>>> diff --git a/config/meson.build b/config/meson.build
>>> index 364a8d739..78c30f334 100644
>>> --- a/config/meson.build
>>> +++ b/config/meson.build
>>> @@ -202,6 +202,7 @@ dpdk_conf.set('RTE_LIBEAL_USE_HPET', get_option('use_hpet'))
>>>  dpdk_conf.set('RTE_MAX_VFIO_GROUPS', 64)
>>>  dpdk_conf.set('RTE_DRIVER_MEMPOOL_BUCKET_SIZE_KB', 64)
>>>  dpdk_conf.set('RTE_LIBRTE_DPAA2_USE_PHYS_IOVA', true)
>>> +dpdk_conf.set('RTE_LIBRTE_VIRTIO_LINK_SPEED', 10000)
>>>  
>>>  
>>>  compile_time_cpuflags = []
>>> diff --git a/drivers/net/vhost/rte_eth_vhost.c b/drivers/net/vhost/rte_eth_vhost.c
>>> index 46f01a7f4..38eaa5955 100644
>>> --- a/drivers/net/vhost/rte_eth_vhost.c
>>> +++ b/drivers/net/vhost/rte_eth_vhost.c
>>> @@ -115,7 +115,7 @@ static struct internal_list_head internal_list =
>>>  static pthread_mutex_t internal_list_lock = PTHREAD_MUTEX_INITIALIZER;
>>>  
>>>  static struct rte_eth_link pmd_link = {
>>> -		.link_speed = 10000,
>>> +		.link_speed = RTE_LIBRTE_VIRTIO_LINK_SPEED,
>>>  		.link_duplex = ETH_LINK_FULL_DUPLEX,
>>>  		.link_status = ETH_LINK_DOWN
>>>  };
>>> diff --git a/drivers/net/virtio/virtio_ethdev.c b/drivers/net/virtio/virtio_ethdev.c
>>> index 044eb10a7..948091cc2 100644
>>> --- a/drivers/net/virtio/virtio_ethdev.c
>>> +++ b/drivers/net/virtio/virtio_ethdev.c
>>> @@ -2371,7 +2371,7 @@ virtio_dev_link_update(struct rte_eth_dev *dev, __rte_unused int wait_to_complet
>>>  
>>>  	memset(&link, 0, sizeof(link));
>>>  	link.link_duplex = ETH_LINK_FULL_DUPLEX;
>>> -	link.link_speed  = ETH_SPEED_NUM_10G;
>>> +	link.link_speed  = RTE_LIBRTE_VIRTIO_LINK_SPEED;
>>>  	link.link_autoneg = ETH_LINK_FIXED;
>>>  
>>>  	if (!hw->started) {
>>> @@ -2426,9 +2426,21 @@ virtio_dev_info_get(struct rte_eth_dev *dev, struct rte_eth_dev_info *dev_info)
>>>  {
>>>  	uint64_t tso_mask, host_features;
>>>  	struct virtio_hw *hw = dev->data->dev_private;
>>> -
>>> -	dev_info->speed_capa = ETH_LINK_SPEED_10G; /* fake value */
>>> -
>>> +#if RTE_LIBRTE_VIRTIO_LINK_SPEED == ETH_LINK_SPEED_20G
>>> +	dev_info->speed_capa = ETH_LINK_SPEED_20G;
>>> +#elif RTE_LIBRTE_VIRTIO_LINK_SPEED == ETH_SPEED_NUM_25G
>>> +	dev_info->speed_capa = ETH_LINK_SPEED_25G;
>>> +#elif RTE_LIBRTE_VIRTIO_LINK_SPEED == ETH_SPEED_NUM_40G
>>> +	dev_info->speed_capa = ETH_LINK_SPEED_40G;
>>> +#elif RTE_LIBRTE_VIRTIO_LINK_SPEED == ETH_SPEED_NUM_50G
>>> +	dev_info->speed_capa = ETH_LINK_SPEED_50G;
>>> +#elif RTE_LIBRTE_VIRTIO_LINK_SPEED == ETH_SPEED_NUM_56G
>>> +	dev_info->speed_capa = ETH_LINK_SPEED_56G;
>>> +#elif RTE_LIBRTE_VIRTIO_LINK_SPEED == ETH_SPEED_NUM_100G
>>> +	dev_info->speed_capa = ETH_LINK_SPEED_100G;
>>> +#else
>>> +	dev_info->speed_capa = ETH_LINK_SPEED_10G;
>>> +#endif
>>>  	dev_info->max_rx_queues =
>>>  		RTE_MIN(hw->max_queue_pairs, VIRTIO_MAX_RX_QUEUES);
>>>  	dev_info->max_tx_queues =
>>>   
>>
>> I think we may need toi extend the Virtio specification so that the
>> device can advertise the link speed.
>>
>> Problem with your proposal is that it is build time only, so:
>> 1. It won't be configurable when using distros DPDK package.
>> 2. All the Virtio devices on a system will have the same value
>>
>> While any Virtio spec update introduce link speed support, wouldn't it
>> be preferable to have this as a devarg?
>>
>> Thanks,
>> Maxime
>>
> 
> Why does link speed matter at all?
> If some code is looking at link speed to compute values like QoS then
> it will be broken based on how virtualized NIC's work (bytes are free).
> 
> It all seems just like a workaround for broke legacy code or
> advertising.
> 

In Ivan's case, I agree it should not matter, we could just default to
the maximum.
I was thinking about vDPA devices, where I think it could make sense to
report the HW link speed.


^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [dpdk-dev] [PATCH v2] net/virtio: add link speed tuning
  2019-12-13 14:59   ` Maxime Coquelin
  2019-12-13 16:26     ` Stephen Hemminger
@ 2019-12-13 17:39     ` Ivan Dyukov
  2019-12-16  6:06       ` Tiwei Bie
  1 sibling, 1 reply; 8+ messages in thread
From: Ivan Dyukov @ 2019-12-13 17:39 UTC (permalink / raw)
  To: Maxime Coquelin, dev, tiwei.bie

Hi Maxime,
Thank you for comments.


13.12.2019 17:59, Maxime Coquelin пишет:
> Hi Ivan,
>
> On 12/13/19 3:44 PM, Ivan Dyukov wrote:
>> Some applications like pktgen use link_speed to calculate transmit
>> rate. It limits outcome traffic to hardcoded 10G.
>>
>> This patch makes link_speed configurable at compile time.
>>
>> Signed-off-by: Ivan Dyukov <i.dyukov@samsung.com>
>> ---
>>   config/common_base                 |  1 +
>>   config/meson.build                 |  1 +
>>   drivers/net/vhost/rte_eth_vhost.c  |  2 +-
>>   drivers/net/virtio/virtio_ethdev.c | 20 ++++++++++++++++----
>>   4 files changed, 19 insertions(+), 5 deletions(-)
>>
>> diff --git a/config/common_base b/config/common_base
>> index 7dec7ed45..8589ca4ec 100644
>> --- a/config/common_base
>> +++ b/config/common_base
>> @@ -433,6 +433,7 @@ CONFIG_RTE_LIBRTE_AVP_DEBUG_BUFFERS=n
>>   # Compile burst-oriented VIRTIO PMD driver
>>   #
>>   CONFIG_RTE_LIBRTE_VIRTIO_PMD=y
>> +CONFIG_RTE_LIBRTE_VIRTIO_LINK_SPEED=10000
>>   CONFIG_RTE_LIBRTE_VIRTIO_DEBUG_RX=n
>>   CONFIG_RTE_LIBRTE_VIRTIO_DEBUG_TX=n
>>   CONFIG_RTE_LIBRTE_VIRTIO_DEBUG_DUMP=n
>> diff --git a/config/meson.build b/config/meson.build
>> index 364a8d739..78c30f334 100644
>> --- a/config/meson.build
>> +++ b/config/meson.build
>> @@ -202,6 +202,7 @@ dpdk_conf.set('RTE_LIBEAL_USE_HPET', get_option('use_hpet'))
>>   dpdk_conf.set('RTE_MAX_VFIO_GROUPS', 64)
>>   dpdk_conf.set('RTE_DRIVER_MEMPOOL_BUCKET_SIZE_KB', 64)
>>   dpdk_conf.set('RTE_LIBRTE_DPAA2_USE_PHYS_IOVA', true)
>> +dpdk_conf.set('RTE_LIBRTE_VIRTIO_LINK_SPEED', 10000)
>>   
>>   
>>   compile_time_cpuflags = []
>> diff --git a/drivers/net/vhost/rte_eth_vhost.c b/drivers/net/vhost/rte_eth_vhost.c
>> index 46f01a7f4..38eaa5955 100644
>> --- a/drivers/net/vhost/rte_eth_vhost.c
>> +++ b/drivers/net/vhost/rte_eth_vhost.c
>> @@ -115,7 +115,7 @@ static struct internal_list_head internal_list =
>>   static pthread_mutex_t internal_list_lock = PTHREAD_MUTEX_INITIALIZER;
>>   
>>   static struct rte_eth_link pmd_link = {
>> -		.link_speed = 10000,
>> +		.link_speed = RTE_LIBRTE_VIRTIO_LINK_SPEED,
>>   		.link_duplex = ETH_LINK_FULL_DUPLEX,
>>   		.link_status = ETH_LINK_DOWN
>>   };
>> diff --git a/drivers/net/virtio/virtio_ethdev.c b/drivers/net/virtio/virtio_ethdev.c
>> index 044eb10a7..948091cc2 100644
>> --- a/drivers/net/virtio/virtio_ethdev.c
>> +++ b/drivers/net/virtio/virtio_ethdev.c
>> @@ -2371,7 +2371,7 @@ virtio_dev_link_update(struct rte_eth_dev *dev, __rte_unused int wait_to_complet
>>   
>>   	memset(&link, 0, sizeof(link));
>>   	link.link_duplex = ETH_LINK_FULL_DUPLEX;
>> -	link.link_speed  = ETH_SPEED_NUM_10G;
>> +	link.link_speed  = RTE_LIBRTE_VIRTIO_LINK_SPEED;
>>   	link.link_autoneg = ETH_LINK_FIXED;
>>   
>>   	if (!hw->started) {
>> @@ -2426,9 +2426,21 @@ virtio_dev_info_get(struct rte_eth_dev *dev, struct rte_eth_dev_info *dev_info)
>>   {
>>   	uint64_t tso_mask, host_features;
>>   	struct virtio_hw *hw = dev->data->dev_private;
>> -
>> -	dev_info->speed_capa = ETH_LINK_SPEED_10G; /* fake value */
>> -
>> +#if RTE_LIBRTE_VIRTIO_LINK_SPEED == ETH_LINK_SPEED_20G
>> +	dev_info->speed_capa = ETH_LINK_SPEED_20G;
>> +#elif RTE_LIBRTE_VIRTIO_LINK_SPEED == ETH_SPEED_NUM_25G
>> +	dev_info->speed_capa = ETH_LINK_SPEED_25G;
>> +#elif RTE_LIBRTE_VIRTIO_LINK_SPEED == ETH_SPEED_NUM_40G
>> +	dev_info->speed_capa = ETH_LINK_SPEED_40G;
>> +#elif RTE_LIBRTE_VIRTIO_LINK_SPEED == ETH_SPEED_NUM_50G
>> +	dev_info->speed_capa = ETH_LINK_SPEED_50G;
>> +#elif RTE_LIBRTE_VIRTIO_LINK_SPEED == ETH_SPEED_NUM_56G
>> +	dev_info->speed_capa = ETH_LINK_SPEED_56G;
>> +#elif RTE_LIBRTE_VIRTIO_LINK_SPEED == ETH_SPEED_NUM_100G
>> +	dev_info->speed_capa = ETH_LINK_SPEED_100G;
>> +#else
>> +	dev_info->speed_capa = ETH_LINK_SPEED_10G;
>> +#endif
>>   	dev_info->max_rx_queues =
>>   		RTE_MIN(hw->max_queue_pairs, VIRTIO_MAX_RX_QUEUES);
>>   	dev_info->max_tx_queues =
>>
> I think we may need toi extend the Virtio specification so that the
> device can advertise the link speed.
I agree. It will be more flexible solution, but it will require another 
effort. I'll evalutate virtio spec and check if it's suitable for such 
changes.
> Problem with your proposal is that it is build time only, so:
> 1. It won't be configurable when using distros DPDK package.
> 2. All the Virtio devices on a system will have the same value
Current implementation is same. Nothing is broken here. :)
> While any Virtio spec update introduce link speed support, wouldn't it
> be preferable to have this as a devarg?
For my case, compile time configuration is ok. Let me prepare solution 
with devarg then we can choose the better one.
> Thanks,
> Maxime

Best regards,

Ivan.


^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [dpdk-dev] [PATCH v2] net/virtio: add link speed tuning
  2019-12-13 17:39     ` Ivan Dyukov
@ 2019-12-16  6:06       ` Tiwei Bie
  2019-12-16  9:44         ` Maxime Coquelin
  0 siblings, 1 reply; 8+ messages in thread
From: Tiwei Bie @ 2019-12-16  6:06 UTC (permalink / raw)
  To: Ivan Dyukov; +Cc: Maxime Coquelin, dev

On Fri, Dec 13, 2019 at 08:39:11PM +0300, Ivan Dyukov wrote:
> Hi Maxime,
> Thank you for comments.
> 
> 
> 13.12.2019 17:59, Maxime Coquelin пишет:
> > Hi Ivan,
> >
> > On 12/13/19 3:44 PM, Ivan Dyukov wrote:
> >> Some applications like pktgen use link_speed to calculate transmit
> >> rate. It limits outcome traffic to hardcoded 10G.
> >>
> >> This patch makes link_speed configurable at compile time.
> >>
> >> Signed-off-by: Ivan Dyukov <i.dyukov@samsung.com>
> >> ---
> >>   config/common_base                 |  1 +
> >>   config/meson.build                 |  1 +
> >>   drivers/net/vhost/rte_eth_vhost.c  |  2 +-
> >>   drivers/net/virtio/virtio_ethdev.c | 20 ++++++++++++++++----
> >>   4 files changed, 19 insertions(+), 5 deletions(-)
> >>
> >> diff --git a/config/common_base b/config/common_base
> >> index 7dec7ed45..8589ca4ec 100644
> >> --- a/config/common_base
> >> +++ b/config/common_base
> >> @@ -433,6 +433,7 @@ CONFIG_RTE_LIBRTE_AVP_DEBUG_BUFFERS=n
> >>   # Compile burst-oriented VIRTIO PMD driver
> >>   #
> >>   CONFIG_RTE_LIBRTE_VIRTIO_PMD=y
> >> +CONFIG_RTE_LIBRTE_VIRTIO_LINK_SPEED=10000
> >>   CONFIG_RTE_LIBRTE_VIRTIO_DEBUG_RX=n
> >>   CONFIG_RTE_LIBRTE_VIRTIO_DEBUG_TX=n
> >>   CONFIG_RTE_LIBRTE_VIRTIO_DEBUG_DUMP=n
> >> diff --git a/config/meson.build b/config/meson.build
> >> index 364a8d739..78c30f334 100644
> >> --- a/config/meson.build
> >> +++ b/config/meson.build
> >> @@ -202,6 +202,7 @@ dpdk_conf.set('RTE_LIBEAL_USE_HPET', get_option('use_hpet'))
> >>   dpdk_conf.set('RTE_MAX_VFIO_GROUPS', 64)
> >>   dpdk_conf.set('RTE_DRIVER_MEMPOOL_BUCKET_SIZE_KB', 64)
> >>   dpdk_conf.set('RTE_LIBRTE_DPAA2_USE_PHYS_IOVA', true)
> >> +dpdk_conf.set('RTE_LIBRTE_VIRTIO_LINK_SPEED', 10000)
> >>   
> >>   
> >>   compile_time_cpuflags = []
> >> diff --git a/drivers/net/vhost/rte_eth_vhost.c b/drivers/net/vhost/rte_eth_vhost.c
> >> index 46f01a7f4..38eaa5955 100644
> >> --- a/drivers/net/vhost/rte_eth_vhost.c
> >> +++ b/drivers/net/vhost/rte_eth_vhost.c
> >> @@ -115,7 +115,7 @@ static struct internal_list_head internal_list =
> >>   static pthread_mutex_t internal_list_lock = PTHREAD_MUTEX_INITIALIZER;
> >>   
> >>   static struct rte_eth_link pmd_link = {
> >> -		.link_speed = 10000,
> >> +		.link_speed = RTE_LIBRTE_VIRTIO_LINK_SPEED,
> >>   		.link_duplex = ETH_LINK_FULL_DUPLEX,
> >>   		.link_status = ETH_LINK_DOWN
> >>   };
> >> diff --git a/drivers/net/virtio/virtio_ethdev.c b/drivers/net/virtio/virtio_ethdev.c
> >> index 044eb10a7..948091cc2 100644
> >> --- a/drivers/net/virtio/virtio_ethdev.c
> >> +++ b/drivers/net/virtio/virtio_ethdev.c
> >> @@ -2371,7 +2371,7 @@ virtio_dev_link_update(struct rte_eth_dev *dev, __rte_unused int wait_to_complet
> >>   
> >>   	memset(&link, 0, sizeof(link));
> >>   	link.link_duplex = ETH_LINK_FULL_DUPLEX;
> >> -	link.link_speed  = ETH_SPEED_NUM_10G;
> >> +	link.link_speed  = RTE_LIBRTE_VIRTIO_LINK_SPEED;
> >>   	link.link_autoneg = ETH_LINK_FIXED;
> >>   
> >>   	if (!hw->started) {
> >> @@ -2426,9 +2426,21 @@ virtio_dev_info_get(struct rte_eth_dev *dev, struct rte_eth_dev_info *dev_info)
> >>   {
> >>   	uint64_t tso_mask, host_features;
> >>   	struct virtio_hw *hw = dev->data->dev_private;
> >> -
> >> -	dev_info->speed_capa = ETH_LINK_SPEED_10G; /* fake value */
> >> -
> >> +#if RTE_LIBRTE_VIRTIO_LINK_SPEED == ETH_LINK_SPEED_20G
> >> +	dev_info->speed_capa = ETH_LINK_SPEED_20G;
> >> +#elif RTE_LIBRTE_VIRTIO_LINK_SPEED == ETH_SPEED_NUM_25G
> >> +	dev_info->speed_capa = ETH_LINK_SPEED_25G;
> >> +#elif RTE_LIBRTE_VIRTIO_LINK_SPEED == ETH_SPEED_NUM_40G
> >> +	dev_info->speed_capa = ETH_LINK_SPEED_40G;
> >> +#elif RTE_LIBRTE_VIRTIO_LINK_SPEED == ETH_SPEED_NUM_50G
> >> +	dev_info->speed_capa = ETH_LINK_SPEED_50G;
> >> +#elif RTE_LIBRTE_VIRTIO_LINK_SPEED == ETH_SPEED_NUM_56G
> >> +	dev_info->speed_capa = ETH_LINK_SPEED_56G;
> >> +#elif RTE_LIBRTE_VIRTIO_LINK_SPEED == ETH_SPEED_NUM_100G
> >> +	dev_info->speed_capa = ETH_LINK_SPEED_100G;
> >> +#else
> >> +	dev_info->speed_capa = ETH_LINK_SPEED_10G;
> >> +#endif
> >>   	dev_info->max_rx_queues =
> >>   		RTE_MIN(hw->max_queue_pairs, VIRTIO_MAX_RX_QUEUES);
> >>   	dev_info->max_tx_queues =
> >>
> > I think we may need toi extend the Virtio specification so that the
> > device can advertise the link speed.
> I agree. It will be more flexible solution, but it will require another 
> effort. I'll evalutate virtio spec and check if it's suitable for such 
> changes.

FYI: https://lists.oasis-open.org/archives/virtio-comment/201911/msg00058.html


> > Problem with your proposal is that it is build time only, so:
> > 1. It won't be configurable when using distros DPDK package.
> > 2. All the Virtio devices on a system will have the same value
> Current implementation is same. Nothing is broken here. :)
> > While any Virtio spec update introduce link speed support, wouldn't it
> > be preferable to have this as a devarg?
> For my case, compile time configuration is ok. Let me prepare solution 
> with devarg then we can choose the better one.
> > Thanks,
> > Maxime
> 
> Best regards,
> 
> Ivan.
> 

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [dpdk-dev] [PATCH v2] net/virtio: add link speed tuning
  2019-12-16  6:06       ` Tiwei Bie
@ 2019-12-16  9:44         ` Maxime Coquelin
  2020-01-14  9:47           ` Maxime Coquelin
  0 siblings, 1 reply; 8+ messages in thread
From: Maxime Coquelin @ 2019-12-16  9:44 UTC (permalink / raw)
  To: Tiwei Bie, Ivan Dyukov; +Cc: dev



On 12/16/19 7:06 AM, Tiwei Bie wrote:
> On Fri, Dec 13, 2019 at 08:39:11PM +0300, Ivan Dyukov wrote:
>> Hi Maxime,
>> Thank you for comments.
>>
>>
>> 13.12.2019 17:59, Maxime Coquelin пишет:
>>> Hi Ivan,
>>>
>>> On 12/13/19 3:44 PM, Ivan Dyukov wrote:
>>>> Some applications like pktgen use link_speed to calculate transmit
>>>> rate. It limits outcome traffic to hardcoded 10G.
>>>>
>>>> This patch makes link_speed configurable at compile time.
>>>>
>>>> Signed-off-by: Ivan Dyukov <i.dyukov@samsung.com>
>>>> ---
>>>>   config/common_base                 |  1 +
>>>>   config/meson.build                 |  1 +
>>>>   drivers/net/vhost/rte_eth_vhost.c  |  2 +-
>>>>   drivers/net/virtio/virtio_ethdev.c | 20 ++++++++++++++++----
>>>>   4 files changed, 19 insertions(+), 5 deletions(-)
>>>>
>>>> diff --git a/config/common_base b/config/common_base
>>>> index 7dec7ed45..8589ca4ec 100644
>>>> --- a/config/common_base
>>>> +++ b/config/common_base
>>>> @@ -433,6 +433,7 @@ CONFIG_RTE_LIBRTE_AVP_DEBUG_BUFFERS=n
>>>>   # Compile burst-oriented VIRTIO PMD driver
>>>>   #
>>>>   CONFIG_RTE_LIBRTE_VIRTIO_PMD=y
>>>> +CONFIG_RTE_LIBRTE_VIRTIO_LINK_SPEED=10000
>>>>   CONFIG_RTE_LIBRTE_VIRTIO_DEBUG_RX=n
>>>>   CONFIG_RTE_LIBRTE_VIRTIO_DEBUG_TX=n
>>>>   CONFIG_RTE_LIBRTE_VIRTIO_DEBUG_DUMP=n
>>>> diff --git a/config/meson.build b/config/meson.build
>>>> index 364a8d739..78c30f334 100644
>>>> --- a/config/meson.build
>>>> +++ b/config/meson.build
>>>> @@ -202,6 +202,7 @@ dpdk_conf.set('RTE_LIBEAL_USE_HPET', get_option('use_hpet'))
>>>>   dpdk_conf.set('RTE_MAX_VFIO_GROUPS', 64)
>>>>   dpdk_conf.set('RTE_DRIVER_MEMPOOL_BUCKET_SIZE_KB', 64)
>>>>   dpdk_conf.set('RTE_LIBRTE_DPAA2_USE_PHYS_IOVA', true)
>>>> +dpdk_conf.set('RTE_LIBRTE_VIRTIO_LINK_SPEED', 10000)
>>>>   
>>>>   
>>>>   compile_time_cpuflags = []
>>>> diff --git a/drivers/net/vhost/rte_eth_vhost.c b/drivers/net/vhost/rte_eth_vhost.c
>>>> index 46f01a7f4..38eaa5955 100644
>>>> --- a/drivers/net/vhost/rte_eth_vhost.c
>>>> +++ b/drivers/net/vhost/rte_eth_vhost.c
>>>> @@ -115,7 +115,7 @@ static struct internal_list_head internal_list =
>>>>   static pthread_mutex_t internal_list_lock = PTHREAD_MUTEX_INITIALIZER;
>>>>   
>>>>   static struct rte_eth_link pmd_link = {
>>>> -		.link_speed = 10000,
>>>> +		.link_speed = RTE_LIBRTE_VIRTIO_LINK_SPEED,
>>>>   		.link_duplex = ETH_LINK_FULL_DUPLEX,
>>>>   		.link_status = ETH_LINK_DOWN
>>>>   };
>>>> diff --git a/drivers/net/virtio/virtio_ethdev.c b/drivers/net/virtio/virtio_ethdev.c
>>>> index 044eb10a7..948091cc2 100644
>>>> --- a/drivers/net/virtio/virtio_ethdev.c
>>>> +++ b/drivers/net/virtio/virtio_ethdev.c
>>>> @@ -2371,7 +2371,7 @@ virtio_dev_link_update(struct rte_eth_dev *dev, __rte_unused int wait_to_complet
>>>>   
>>>>   	memset(&link, 0, sizeof(link));
>>>>   	link.link_duplex = ETH_LINK_FULL_DUPLEX;
>>>> -	link.link_speed  = ETH_SPEED_NUM_10G;
>>>> +	link.link_speed  = RTE_LIBRTE_VIRTIO_LINK_SPEED;
>>>>   	link.link_autoneg = ETH_LINK_FIXED;
>>>>   
>>>>   	if (!hw->started) {
>>>> @@ -2426,9 +2426,21 @@ virtio_dev_info_get(struct rte_eth_dev *dev, struct rte_eth_dev_info *dev_info)
>>>>   {
>>>>   	uint64_t tso_mask, host_features;
>>>>   	struct virtio_hw *hw = dev->data->dev_private;
>>>> -
>>>> -	dev_info->speed_capa = ETH_LINK_SPEED_10G; /* fake value */
>>>> -
>>>> +#if RTE_LIBRTE_VIRTIO_LINK_SPEED == ETH_LINK_SPEED_20G
>>>> +	dev_info->speed_capa = ETH_LINK_SPEED_20G;
>>>> +#elif RTE_LIBRTE_VIRTIO_LINK_SPEED == ETH_SPEED_NUM_25G
>>>> +	dev_info->speed_capa = ETH_LINK_SPEED_25G;
>>>> +#elif RTE_LIBRTE_VIRTIO_LINK_SPEED == ETH_SPEED_NUM_40G
>>>> +	dev_info->speed_capa = ETH_LINK_SPEED_40G;
>>>> +#elif RTE_LIBRTE_VIRTIO_LINK_SPEED == ETH_SPEED_NUM_50G
>>>> +	dev_info->speed_capa = ETH_LINK_SPEED_50G;
>>>> +#elif RTE_LIBRTE_VIRTIO_LINK_SPEED == ETH_SPEED_NUM_56G
>>>> +	dev_info->speed_capa = ETH_LINK_SPEED_56G;
>>>> +#elif RTE_LIBRTE_VIRTIO_LINK_SPEED == ETH_SPEED_NUM_100G
>>>> +	dev_info->speed_capa = ETH_LINK_SPEED_100G;
>>>> +#else
>>>> +	dev_info->speed_capa = ETH_LINK_SPEED_10G;
>>>> +#endif
>>>>   	dev_info->max_rx_queues =
>>>>   		RTE_MIN(hw->max_queue_pairs, VIRTIO_MAX_RX_QUEUES);
>>>>   	dev_info->max_tx_queues =
>>>>
>>> I think we may need toi extend the Virtio specification so that the
>>> device can advertise the link speed.
>> I agree. It will be more flexible solution, but it will require another 
>> effort. I'll evalutate virtio spec and check if it's suitable for such 
>> changes.
> 
> FYI: https://lists.oasis-open.org/archives/virtio-comment/201911/msg00058.html

Great, thanks Tiwei for the pointer!

> 
>>> Problem with your proposal is that it is build time only, so:
>>> 1. It won't be configurable when using distros DPDK package.
>>> 2. All the Virtio devices on a system will have the same value
>> Current implementation is same. Nothing is broken here. :)
>>> While any Virtio spec update introduce link speed support, wouldn't it
>>> be preferable to have this as a devarg?
>> For my case, compile time configuration is ok. Let me prepare solution 
>> with devarg then we can choose the better one.
>>> Thanks,
>>> Maxime
>>
>> Best regards,
>>
>> Ivan.
>>
> 


^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [dpdk-dev] [PATCH v2] net/virtio: add link speed tuning
  2019-12-16  9:44         ` Maxime Coquelin
@ 2020-01-14  9:47           ` Maxime Coquelin
  0 siblings, 0 replies; 8+ messages in thread
From: Maxime Coquelin @ 2020-01-14  9:47 UTC (permalink / raw)
  To: Tiwei Bie, Ivan Dyukov; +Cc: dev

Hi Ivan,

On 12/16/19 10:44 AM, Maxime Coquelin wrote:
> 
> 
> On 12/16/19 7:06 AM, Tiwei Bie wrote:
>> On Fri, Dec 13, 2019 at 08:39:11PM +0300, Ivan Dyukov wrote:
>>> Hi Maxime,
>>> Thank you for comments.
>>>
>>>
>>> 13.12.2019 17:59, Maxime Coquelin пишет:
>>>> Hi Ivan,
>>>>
>>>> On 12/13/19 3:44 PM, Ivan Dyukov wrote:
>>>>> Some applications like pktgen use link_speed to calculate transmit
>>>>> rate. It limits outcome traffic to hardcoded 10G.
>>>>>
>>>>> This patch makes link_speed configurable at compile time.
>>>>>
>>>>> Signed-off-by: Ivan Dyukov <i.dyukov@samsung.com>
>>>>> ---
>>>>>   config/common_base                 |  1 +
>>>>>   config/meson.build                 |  1 +
>>>>>   drivers/net/vhost/rte_eth_vhost.c  |  2 +-
>>>>>   drivers/net/virtio/virtio_ethdev.c | 20 ++++++++++++++++----
>>>>>   4 files changed, 19 insertions(+), 5 deletions(-)
>>>>>
>>>>> diff --git a/config/common_base b/config/common_base
>>>>> index 7dec7ed45..8589ca4ec 100644
>>>>> --- a/config/common_base
>>>>> +++ b/config/common_base
>>>>> @@ -433,6 +433,7 @@ CONFIG_RTE_LIBRTE_AVP_DEBUG_BUFFERS=n
>>>>>   # Compile burst-oriented VIRTIO PMD driver
>>>>>   #
>>>>>   CONFIG_RTE_LIBRTE_VIRTIO_PMD=y
>>>>> +CONFIG_RTE_LIBRTE_VIRTIO_LINK_SPEED=10000
>>>>>   CONFIG_RTE_LIBRTE_VIRTIO_DEBUG_RX=n
>>>>>   CONFIG_RTE_LIBRTE_VIRTIO_DEBUG_TX=n
>>>>>   CONFIG_RTE_LIBRTE_VIRTIO_DEBUG_DUMP=n
>>>>> diff --git a/config/meson.build b/config/meson.build
>>>>> index 364a8d739..78c30f334 100644
>>>>> --- a/config/meson.build
>>>>> +++ b/config/meson.build
>>>>> @@ -202,6 +202,7 @@ dpdk_conf.set('RTE_LIBEAL_USE_HPET', get_option('use_hpet'))
>>>>>   dpdk_conf.set('RTE_MAX_VFIO_GROUPS', 64)
>>>>>   dpdk_conf.set('RTE_DRIVER_MEMPOOL_BUCKET_SIZE_KB', 64)
>>>>>   dpdk_conf.set('RTE_LIBRTE_DPAA2_USE_PHYS_IOVA', true)
>>>>> +dpdk_conf.set('RTE_LIBRTE_VIRTIO_LINK_SPEED', 10000)
>>>>>   
>>>>>   
>>>>>   compile_time_cpuflags = []
>>>>> diff --git a/drivers/net/vhost/rte_eth_vhost.c b/drivers/net/vhost/rte_eth_vhost.c
>>>>> index 46f01a7f4..38eaa5955 100644
>>>>> --- a/drivers/net/vhost/rte_eth_vhost.c
>>>>> +++ b/drivers/net/vhost/rte_eth_vhost.c
>>>>> @@ -115,7 +115,7 @@ static struct internal_list_head internal_list =
>>>>>   static pthread_mutex_t internal_list_lock = PTHREAD_MUTEX_INITIALIZER;
>>>>>   
>>>>>   static struct rte_eth_link pmd_link = {
>>>>> -		.link_speed = 10000,
>>>>> +		.link_speed = RTE_LIBRTE_VIRTIO_LINK_SPEED,
>>>>>   		.link_duplex = ETH_LINK_FULL_DUPLEX,
>>>>>   		.link_status = ETH_LINK_DOWN
>>>>>   };
>>>>> diff --git a/drivers/net/virtio/virtio_ethdev.c b/drivers/net/virtio/virtio_ethdev.c
>>>>> index 044eb10a7..948091cc2 100644
>>>>> --- a/drivers/net/virtio/virtio_ethdev.c
>>>>> +++ b/drivers/net/virtio/virtio_ethdev.c
>>>>> @@ -2371,7 +2371,7 @@ virtio_dev_link_update(struct rte_eth_dev *dev, __rte_unused int wait_to_complet
>>>>>   
>>>>>   	memset(&link, 0, sizeof(link));
>>>>>   	link.link_duplex = ETH_LINK_FULL_DUPLEX;
>>>>> -	link.link_speed  = ETH_SPEED_NUM_10G;
>>>>> +	link.link_speed  = RTE_LIBRTE_VIRTIO_LINK_SPEED;
>>>>>   	link.link_autoneg = ETH_LINK_FIXED;
>>>>>   
>>>>>   	if (!hw->started) {
>>>>> @@ -2426,9 +2426,21 @@ virtio_dev_info_get(struct rte_eth_dev *dev, struct rte_eth_dev_info *dev_info)
>>>>>   {
>>>>>   	uint64_t tso_mask, host_features;
>>>>>   	struct virtio_hw *hw = dev->data->dev_private;
>>>>> -
>>>>> -	dev_info->speed_capa = ETH_LINK_SPEED_10G; /* fake value */
>>>>> -
>>>>> +#if RTE_LIBRTE_VIRTIO_LINK_SPEED == ETH_LINK_SPEED_20G
>>>>> +	dev_info->speed_capa = ETH_LINK_SPEED_20G;
>>>>> +#elif RTE_LIBRTE_VIRTIO_LINK_SPEED == ETH_SPEED_NUM_25G
>>>>> +	dev_info->speed_capa = ETH_LINK_SPEED_25G;
>>>>> +#elif RTE_LIBRTE_VIRTIO_LINK_SPEED == ETH_SPEED_NUM_40G
>>>>> +	dev_info->speed_capa = ETH_LINK_SPEED_40G;
>>>>> +#elif RTE_LIBRTE_VIRTIO_LINK_SPEED == ETH_SPEED_NUM_50G
>>>>> +	dev_info->speed_capa = ETH_LINK_SPEED_50G;
>>>>> +#elif RTE_LIBRTE_VIRTIO_LINK_SPEED == ETH_SPEED_NUM_56G
>>>>> +	dev_info->speed_capa = ETH_LINK_SPEED_56G;
>>>>> +#elif RTE_LIBRTE_VIRTIO_LINK_SPEED == ETH_SPEED_NUM_100G
>>>>> +	dev_info->speed_capa = ETH_LINK_SPEED_100G;
>>>>> +#else
>>>>> +	dev_info->speed_capa = ETH_LINK_SPEED_10G;
>>>>> +#endif
>>>>>   	dev_info->max_rx_queues =
>>>>>   		RTE_MIN(hw->max_queue_pairs, VIRTIO_MAX_RX_QUEUES);
>>>>>   	dev_info->max_tx_queues =
>>>>>
>>>> I think we may need toi extend the Virtio specification so that the
>>>> device can advertise the link speed.
>>> I agree. It will be more flexible solution, but it will require another 
>>> effort. I'll evalutate virtio spec and check if it's suitable for such 
>>> changes.
>>
>> FYI: https://lists.oasis-open.org/archives/virtio-comment/201911/msg00058.html
> 
> Great, thanks Tiwei for the pointer!

The above spec patch is implemented in Qemu since v2.12:

commit 9473939ed7addcaaeb8fde5c093918fb7fa0919c
Author: Jason Baron <jbaron@akamai.com>
Date:   Wed Mar 7 22:25:41 2018 -0500

    virtio-net: add linkspeed and duplex settings to virtio-net

    Although linkspeed and duplex can be set in a linux guest via
'ethtool -s',
    this requires custom ethtool commands for virtio-net by default.

    Introduce a new feature flag, VIRTIO_NET_F_SPEED_DUPLEX, which allows
    the hypervisor to export a linkspeed and duplex setting. The user can
    subsequently overwrite it later if desired via: 'ethtool -s'.

    Linkspeed and duplex settings can be set as:
    '-device virtio-net,speed=10000,duplex=full'

    where speed is [0...INT_MAX], and duplex is ["half"|"full"].

    Signed-off-by: Jason Baron <jbaron@akamai.com>
    Cc: "Michael S. Tsirkin" <mst@redhat.com>
    Cc: Jason Wang <jasowang@redhat.com>
    Cc: virtio-dev@lists.oasis-open.org
    Reviewed-by: Michael S. Tsirkin <mst@redhat.com>
    Signed-off-by: Michael S. Tsirkin <mst@redhat.com>

I think the right way would be to implement it directly in Virtio-net
PMD.

Are you willing to do it?

Thanks,
Maxime

> 
>>
>>>> Problem with your proposal is that it is build time only, so:
>>>> 1. It won't be configurable when using distros DPDK package.
>>>> 2. All the Virtio devices on a system will have the same value
>>> Current implementation is same. Nothing is broken here. :)
>>>> While any Virtio spec update introduce link speed support, wouldn't it
>>>> be preferable to have this as a devarg?
>>> For my case, compile time configuration is ok. Let me prepare solution 
>>> with devarg then we can choose the better one.
>>>> Thanks,
>>>> Maxime
>>>
>>> Best regards,
>>>
>>> Ivan.
>>>
>>
> 


^ permalink raw reply	[flat|nested] 8+ messages in thread

end of thread, back to index

Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
     [not found] <CGME20191213144450eucas1p189850559438d6f9d5ead48b645e27a79@eucas1p1.samsung.com>
2019-12-13 14:44 ` [dpdk-dev] [PATCH v2] net/virtio: add link speed tuning Ivan Dyukov
2019-12-13 14:59   ` Maxime Coquelin
2019-12-13 16:26     ` Stephen Hemminger
2019-12-13 16:31       ` Maxime Coquelin
2019-12-13 17:39     ` Ivan Dyukov
2019-12-16  6:06       ` Tiwei Bie
2019-12-16  9:44         ` Maxime Coquelin
2020-01-14  9:47           ` Maxime Coquelin

DPDK-dev Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/dpdk-dev/0 dpdk-dev/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 dpdk-dev dpdk-dev/ https://lore.kernel.org/dpdk-dev \
		dev@dpdk.org
	public-inbox-index dpdk-dev

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.dpdk.dev


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git