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

[Stephen Hemminger <stephen@networkplumber.org>]

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.