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.dyu...@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 <jba...@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 <jba...@akamai.com>
Cc: "Michael S. Tsirkin" <m...@redhat.com>
Cc: Jason Wang <jasow...@redhat.com>
Cc: virtio-...@lists.oasis-open.org
Reviewed-by: Michael S. Tsirkin <m...@redhat.com>
Signed-off-by: Michael S. Tsirkin <m...@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.
>>>
>>
>
