Re: [dpdk-dev] [PATCH v2] net/virtio: add link speed tuning

[Ivan Dyukov <i.dyukov@samsung.com>]

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