diff mbox series

[v11,1/6] net/virtio: replace default virtio speed

Message ID 20200416055309.19679-2-i.dyukov@samsung.com (mailing list archive)
State Superseded, archived
Delegated to: Maxime Coquelin
Headers
Series net/virtio: add link speed devarg |

Checks

Context Check Description
ci/checkpatch success coding style OK
ci/iol-intel-Performance fail Performance Testing issues
ci/iol-mellanox-Performance success Performance Testing PASS
ci/iol-testing success Testing PASS
ci/Intel-compilation fail apply issues

Commit Message

Ivan Dyukov April 16, 2020, 5:53 a.m. UTC 
  This patch set speed to unknown

Signed-off-by: Ivan Dyukov <i.dyukov@samsung.com>
---
 drivers/net/virtio/virtio_ethdev.c |  4 ++--
 lib/librte_ethdev/rte_ethdev.h     | 27 ++++++++++++++-------------
 2 files changed, 16 insertions(+), 15 deletions(-)

Comments

Maxime Coquelin April 16, 2020, 11:44 a.m. UTC | #1 
On 4/16/20 7:53 AM, Ivan Dyukov wrote:
> This patch set speed to unknown
> 
> Signed-off-by: Ivan Dyukov <i.dyukov@samsung.com>
> ---
>  drivers/net/virtio/virtio_ethdev.c |  4 ++--
>  lib/librte_ethdev/rte_ethdev.h     | 27 ++++++++++++++-------------
>  2 files changed, 16 insertions(+), 15 deletions(-)
> 
> diff --git a/drivers/net/virtio/virtio_ethdev.c b/drivers/net/virtio/virtio_ethdev.c
> index f9d0ea70d..e98a76ea2 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  = ETH_SPEED_NUM_UNKNOWN;
>  	link.link_autoneg = ETH_LINK_FIXED;
>  
>  	if (!hw->started) {
> @@ -2427,7 +2427,7 @@ 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 */
> +	dev_info->speed_capa = ETH_LINK_SPEED_AUTONEG; /* fake value */
>  
>  	dev_info->max_rx_queues =
>  		RTE_MIN(hw->max_queue_pairs, VIRTIO_MAX_RX_QUEUES);
> diff --git a/lib/librte_ethdev/rte_ethdev.h b/lib/librte_ethdev/rte_ethdev.h
> index d1a593ad1..a15ea572e 100644
> --- a/lib/librte_ethdev/rte_ethdev.h
> +++ b/lib/librte_ethdev/rte_ethdev.h
> @@ -287,19 +287,20 @@ struct rte_eth_stats {
>  /**
>   * Ethernet numeric link speeds in Mbps
>   */
> -#define ETH_SPEED_NUM_NONE         0 /**< Not defined */
> -#define ETH_SPEED_NUM_10M         10 /**<  10 Mbps */
> -#define ETH_SPEED_NUM_100M       100 /**< 100 Mbps */
> -#define ETH_SPEED_NUM_1G        1000 /**<   1 Gbps */
> -#define ETH_SPEED_NUM_2_5G      2500 /**< 2.5 Gbps */
> -#define ETH_SPEED_NUM_5G        5000 /**<   5 Gbps */
> -#define ETH_SPEED_NUM_10G      10000 /**<  10 Gbps */
> -#define ETH_SPEED_NUM_20G      20000 /**<  20 Gbps */
> -#define ETH_SPEED_NUM_25G      25000 /**<  25 Gbps */
> -#define ETH_SPEED_NUM_40G      40000 /**<  40 Gbps */
> -#define ETH_SPEED_NUM_50G      50000 /**<  50 Gbps */
> -#define ETH_SPEED_NUM_56G      56000 /**<  56 Gbps */
> -#define ETH_SPEED_NUM_100G    100000 /**< 100 Gbps */
> +#define ETH_SPEED_NUM_NONE             0 /**< Not defined */
> +#define ETH_SPEED_NUM_10M             10 /**<  10 Mbps */
> +#define ETH_SPEED_NUM_100M           100 /**< 100 Mbps */
> +#define ETH_SPEED_NUM_1G            1000 /**<   1 Gbps */
> +#define ETH_SPEED_NUM_2_5G          2500 /**< 2.5 Gbps */
> +#define ETH_SPEED_NUM_5G            5000 /**<   5 Gbps */
> +#define ETH_SPEED_NUM_10G          10000 /**<  10 Gbps */
> +#define ETH_SPEED_NUM_20G          20000 /**<  20 Gbps */
> +#define ETH_SPEED_NUM_25G          25000 /**<  25 Gbps */
> +#define ETH_SPEED_NUM_40G          40000 /**<  40 Gbps */
> +#define ETH_SPEED_NUM_50G          50000 /**<  50 Gbps */
> +#define ETH_SPEED_NUM_56G          56000 /**<  56 Gbps */
> +#define ETH_SPEED_NUM_100G        100000 /**< 100 Gbps */
> +#define ETH_SPEED_NUM_UNKNOWN 0xffffffff /**< Unknown */
>  
>  /**
>   * A structure used to retrieve link-level information of an Ethernet port.
> 

I actually meant to have the rte_ethdev.h change in a dedicated patch,
as it is in a different subsystem. Someone could want to backport only
the lib change without touching the Virtio driver.

If OK for you, I can do that while applying.

Thanks,
Maxime
Morten Brørup April 16, 2020, 11:55 a.m. UTC | #2 
> From: Ivan Dyukov [mailto:i.dyukov@samsung.com]
> Sent: Thursday, April 16, 2020 7:53 AM
> 
> This patch set speed to unknown
> 
> Signed-off-by: Ivan Dyukov <i.dyukov@samsung.com>
> ---
>  drivers/net/virtio/virtio_ethdev.c |  4 ++--
>  lib/librte_ethdev/rte_ethdev.h     | 27 ++++++++++++++-------------
>  2 files changed, 16 insertions(+), 15 deletions(-)
> 
> diff --git a/drivers/net/virtio/virtio_ethdev.c
> b/drivers/net/virtio/virtio_ethdev.c
> index f9d0ea70d..e98a76ea2 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  = ETH_SPEED_NUM_UNKNOWN;
>  	link.link_autoneg = ETH_LINK_FIXED;
> 
>  	if (!hw->started) {
> @@ -2427,7 +2427,7 @@ 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 */
> +	dev_info->speed_capa = ETH_LINK_SPEED_AUTONEG; /* fake value */

If you indicate that the NIC supports Auto Negotiation here,
then I suggest that you also change the link status as follows:
- link.link_autoneg = ETH_LINK_FIXED;
+ link.link_autoneg = ETH_LINK_AUTONEG;

I considered the opposite change, but if we define that the underlying environment determines the actual speed, then Auto Negotiation seems more correct than Fixed speed.

Med venlig hilsen / kind regards
- Morten Brørup
Maxime Coquelin April 16, 2020, 11:58 a.m. UTC | #3 
On 4/16/20 1:55 PM, Morten Brørup wrote:
>> From: Ivan Dyukov [mailto:i.dyukov@samsung.com]
>> Sent: Thursday, April 16, 2020 7:53 AM
>>
>> This patch set speed to unknown
>>
>> Signed-off-by: Ivan Dyukov <i.dyukov@samsung.com>
>> ---
>>  drivers/net/virtio/virtio_ethdev.c |  4 ++--
>>  lib/librte_ethdev/rte_ethdev.h     | 27 ++++++++++++++-------------
>>  2 files changed, 16 insertions(+), 15 deletions(-)
>>
>> diff --git a/drivers/net/virtio/virtio_ethdev.c
>> b/drivers/net/virtio/virtio_ethdev.c
>> index f9d0ea70d..e98a76ea2 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  = ETH_SPEED_NUM_UNKNOWN;
>>  	link.link_autoneg = ETH_LINK_FIXED;
>>
>>  	if (!hw->started) {
>> @@ -2427,7 +2427,7 @@ 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 */
>> +	dev_info->speed_capa = ETH_LINK_SPEED_AUTONEG; /* fake value */
> 
> If you indicate that the NIC supports Auto Negotiation here,
> then I suggest that you also change the link status as follows:
> - link.link_autoneg = ETH_LINK_FIXED;
> + link.link_autoneg = ETH_LINK_AUTONEG;
> 
> I considered the opposite change, but if we define that the underlying environment determines the actual speed, then Auto Negotiation seems more correct than Fixed speed.

That's a valid point.
Thank you Morten for spotting this!

Ivan, I can do the change while applying if you are fine with it.

Thanks,
Maxime

> Med venlig hilsen / kind regards
> - Morten Brørup
> 
> 
>
Ivan Dyukov April 16, 2020, 12:20 p.m. UTC | #4 
Hi Maxime, Morten,

Thank you for comments. I'll prepare one more revision of the change.

Best regards,
Ivan
16.04.2020 14:58, Maxime Coquelin пишет:
>
> On 4/16/20 1:55 PM, Morten Brørup wrote:
>>> From: Ivan Dyukov [mailto:i.dyukov@samsung.com]
>>> Sent: Thursday, April 16, 2020 7:53 AM
>>>
>>> This patch set speed to unknown
>>>
>>> Signed-off-by: Ivan Dyukov <i.dyukov@samsung.com>
>>> ---
>>>   drivers/net/virtio/virtio_ethdev.c |  4 ++--
>>>   lib/librte_ethdev/rte_ethdev.h     | 27 ++++++++++++++-------------
>>>   2 files changed, 16 insertions(+), 15 deletions(-)
>>>
>>> diff --git a/drivers/net/virtio/virtio_ethdev.c
>>> b/drivers/net/virtio/virtio_ethdev.c
>>> index f9d0ea70d..e98a76ea2 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  = ETH_SPEED_NUM_UNKNOWN;
>>>   	link.link_autoneg = ETH_LINK_FIXED;
>>>
>>>   	if (!hw->started) {
>>> @@ -2427,7 +2427,7 @@ 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 */
>>> +	dev_info->speed_capa = ETH_LINK_SPEED_AUTONEG; /* fake value */
>> If you indicate that the NIC supports Auto Negotiation here,
>> then I suggest that you also change the link status as follows:
>> - link.link_autoneg = ETH_LINK_FIXED;
>> + link.link_autoneg = ETH_LINK_AUTONEG;
>>
>> I considered the opposite change, but if we define that the underlying environment determines the actual speed, then Auto Negotiation seems more correct than Fixed speed.
> That's a valid point.
> Thank you Morten for spotting this!
>
> Ivan, I can do the change while applying if you are fine with it.
>
> Thanks,
> Maxime
>
>> Med venlig hilsen / kind regards
>> - Morten Brørup
>>
>>
>>
>
diff mbox series

Patch

diff --git a/drivers/net/virtio/virtio_ethdev.c b/drivers/net/virtio/virtio_ethdev.c
index f9d0ea70d..e98a76ea2 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  = ETH_SPEED_NUM_UNKNOWN;
 	link.link_autoneg = ETH_LINK_FIXED;
 
 	if (!hw->started) {
@@ -2427,7 +2427,7 @@  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 */
+	dev_info->speed_capa = ETH_LINK_SPEED_AUTONEG; /* fake value */
 
 	dev_info->max_rx_queues =
 		RTE_MIN(hw->max_queue_pairs, VIRTIO_MAX_RX_QUEUES);
diff --git a/lib/librte_ethdev/rte_ethdev.h b/lib/librte_ethdev/rte_ethdev.h
index d1a593ad1..a15ea572e 100644
--- a/lib/librte_ethdev/rte_ethdev.h
+++ b/lib/librte_ethdev/rte_ethdev.h
@@ -287,19 +287,20 @@  struct rte_eth_stats {
 /**
  * Ethernet numeric link speeds in Mbps
  */
-#define ETH_SPEED_NUM_NONE         0 /**< Not defined */
-#define ETH_SPEED_NUM_10M         10 /**<  10 Mbps */
-#define ETH_SPEED_NUM_100M       100 /**< 100 Mbps */
-#define ETH_SPEED_NUM_1G        1000 /**<   1 Gbps */
-#define ETH_SPEED_NUM_2_5G      2500 /**< 2.5 Gbps */
-#define ETH_SPEED_NUM_5G        5000 /**<   5 Gbps */
-#define ETH_SPEED_NUM_10G      10000 /**<  10 Gbps */
-#define ETH_SPEED_NUM_20G      20000 /**<  20 Gbps */
-#define ETH_SPEED_NUM_25G      25000 /**<  25 Gbps */
-#define ETH_SPEED_NUM_40G      40000 /**<  40 Gbps */
-#define ETH_SPEED_NUM_50G      50000 /**<  50 Gbps */
-#define ETH_SPEED_NUM_56G      56000 /**<  56 Gbps */
-#define ETH_SPEED_NUM_100G    100000 /**< 100 Gbps */
+#define ETH_SPEED_NUM_NONE             0 /**< Not defined */
+#define ETH_SPEED_NUM_10M             10 /**<  10 Mbps */
+#define ETH_SPEED_NUM_100M           100 /**< 100 Mbps */
+#define ETH_SPEED_NUM_1G            1000 /**<   1 Gbps */
+#define ETH_SPEED_NUM_2_5G          2500 /**< 2.5 Gbps */
+#define ETH_SPEED_NUM_5G            5000 /**<   5 Gbps */
+#define ETH_SPEED_NUM_10G          10000 /**<  10 Gbps */
+#define ETH_SPEED_NUM_20G          20000 /**<  20 Gbps */
+#define ETH_SPEED_NUM_25G          25000 /**<  25 Gbps */
+#define ETH_SPEED_NUM_40G          40000 /**<  40 Gbps */
+#define ETH_SPEED_NUM_50G          50000 /**<  50 Gbps */
+#define ETH_SPEED_NUM_56G          56000 /**<  56 Gbps */
+#define ETH_SPEED_NUM_100G        100000 /**< 100 Gbps */
+#define ETH_SPEED_NUM_UNKNOWN 0xffffffff /**< Unknown */
 
 /**
  * A structure used to retrieve link-level information of an Ethernet port.