diff mbox series

[v2] net/virtio: add link speed tuning

Message ID	20191213144442.32048-1-i.dyukov@samsung.com (mailing list archive)
State	Superseded, archived
Delegated to:	Maxime Coquelin
Headers	From: Ivan Dyukov <i.dyukov@samsung.com> To: dev@dpdk.org, maxime.coquelin@redhat.com, tiwei.bie@intel.com Cc: Ivan Dyukov <i.dyukov@samsung.com> Date: Fri, 13 Dec 2019 17:44:42 +0300 Message-Id: <20191213144442.32048-1-i.dyukov@samsung.com> Content-Type: text/plain; charset="utf-8" CMS-TYPE: 201P References: <CGME20191213144450eucas1p189850559438d6f9d5ead48b645e27a79@eucas1p1.samsung.com> Subject: [dpdk-dev] [PATCH v2] net/virtio: add link speed tuning Precedence: list Errors-To: dev-bounces@dpdk.org Sender: "dev" <dev-bounces@dpdk.org>
Series	[v2] net/virtio: add link speed tuning \| [v2] net/virtio: add link speed tuning

Checks

Context	Check	Description
ci/checkpatch	warning	coding style issues
ci/travis-robot	warning	Travis build: failed
ci/iol-testing	success	Testing PASS
ci/iol-mellanox-Performance	success	Performance Testing PASS
ci/iol-intel-Performance	success	Performance Testing PASS
ci/Intel-compilation	success	Compilation OK

Commit Message

Ivan Dyukov Dec. 13, 2019, 2:44 p.m. UTC

  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(-)

Comments

Maxime Coquelin Dec. 13, 2019, 2:59 p.m. UTC | #1

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

Stephen Hemminger Dec. 13, 2019, 4:26 p.m. UTC | #2

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.

Maxime Coquelin Dec. 13, 2019, 4:31 p.m. UTC | #3

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.

Ivan Dyukov Dec. 13, 2019, 5:39 p.m. UTC | #4

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.

Tiwei Bie Dec. 16, 2019, 6:06 a.m. UTC | #5

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.
>

Maxime Coquelin Dec. 16, 2019, 9:44 a.m. UTC | #6

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.
>>
>

Maxime Coquelin Jan. 14, 2020, 9:47 a.m. UTC | #7

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.
>>>
>>
>

Ivan Dyukov Jan. 30, 2020, 7:13 a.m. UTC | #8

Hi Maxime,

14.01.2020 12:47, Maxime Coquelin пишет:
> 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://protect2.fireeye.com/url?k=6b9e2079-365021cd-6b9fab36-000babdfecba-2c7f1ed6043989c5&u=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?

Sorry for delay, I missed this email.  I need to reconfigure my emails 
filters.

Ok I'll take care about it, but I can't promise any dates for it. May be 
this winter or spring.


Best regards,

Ivan.

>
> 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.
>>>>
>

diff mbox series

Patch

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 =