On 12/13/19 5:26 PM, Stephen Hemminger wrote:
> On Fri, 13 Dec 2019 15:59:23 +0100
> Maxime Coquelin <maxime.coque...@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.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.
>>
>> 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.
