[v8,2/5] net/virtio: add link speed devarg
Checks
Commit Message
Some applications like pktgen use link speed to calculate
transmission rate. It limits outcome traffic to hardcoded 10G.
This patch adds speed devarg which allows to configure
link speed of virtio device.
Signed-off-by: Ivan Dyukov <i.dyukov@samsung.com>
---
doc/guides/nics/virtio.rst | 7 +++
drivers/net/virtio/virtio_ethdev.c | 97 +++++++++++++++++++++++++-----
drivers/net/virtio/virtio_pci.h | 1 +
3 files changed, 89 insertions(+), 16 deletions(-)
Comments
30/03/2020 09:57, Ivan Dyukov:
> Some applications like pktgen use link speed to calculate
> transmission rate. It limits outcome traffic to hardcoded 10G.
>
> This patch adds speed devarg which allows to configure
> link speed of virtio device.
Is it really a good idea to fake such information?
Shouldn't it be managed differently in the application instead?
Hello Thomas,
01.04.2020 13:57, Thomas Monjalon пишет:
> 30/03/2020 09:57, Ivan Dyukov:
>> Some applications like pktgen use link speed to calculate
>> transmission rate. It limits outcome traffic to hardcoded 10G.
>>
>> This patch adds speed devarg which allows to configure
>> link speed of virtio device.
> Is it really a good idea to fake such information?
> Shouldn't it be managed differently in the application instead?
>
>
>
This is main stream of net devices. Device provides speed to
application. Application calculates the packet rate. In case of virtio,
speed is not limited by device. It could be specified by user. This
patch just gives this posibility to user.
Best regards,
Ivan.
02/04/2020 11:18, Ivan Dyukov:
> Hello Thomas,
>
> 01.04.2020 13:57, Thomas Monjalon пишет:
> > 30/03/2020 09:57, Ivan Dyukov:
> >> Some applications like pktgen use link speed to calculate
> >> transmission rate. It limits outcome traffic to hardcoded 10G.
> >>
> >> This patch adds speed devarg which allows to configure
> >> link speed of virtio device.
> > Is it really a good idea to fake such information?
> > Shouldn't it be managed differently in the application instead?
> >
> >
> >
> This is main stream of net devices. Device provides speed to
> application. Application calculates the packet rate. In case of virtio,
> speed is not limited by device. It could be specified by user. This
> patch just gives this posibility to user.
The other possibility is to return 0 meaning unknown speed.
I don't see why this information should be saved in the driver space.
The user can give this information to the application,
it would look more correct to me.
Note: the application is controlling the devargs passed to the driver,
so it can intercept such information.
I understand it is easier to have the speed info at the same place
in all cases. But it avoids differentiating what is a reliable info,
and what is user-provided info.
02.04.2020 20:33, Thomas Monjalon пишет:
> 02/04/2020 11:18, Ivan Dyukov:
>> Hello Thomas,
>>
>> 01.04.2020 13:57, Thomas Monjalon пишет:
>>> 30/03/2020 09:57, Ivan Dyukov:
>>>> Some applications like pktgen use link speed to calculate
>>>> transmission rate. It limits outcome traffic to hardcoded 10G.
>>>>
>>>> This patch adds speed devarg which allows to configure
>>>> link speed of virtio device.
>>> Is it really a good idea to fake such information?
>>> Shouldn't it be managed differently in the application instead?
>>>
>>>
>>>
>> This is main stream of net devices. Device provides speed to
>> application. Application calculates the packet rate. In case of virtio,
>> speed is not limited by device. It could be specified by user. This
>> patch just gives this posibility to user.
> The other possibility is to return 0 meaning unknown speed.
Yep. Right. Linux kernel virtio is implemented same way. They return -1,
if user haven't specified other.
Legacy dpdk virtio code was always returned 10G speed before my patchset
and I just keep this value to
prevent breakages in applications like pktgen.
> I don't see why this information should be saved in the driver space.
The speed could be provided by qemu. Please see 5th patch in my
patchset. It's not good idea to smash
this code accross application and driver side.
> The user can give this information to the application,
> it would look more correct to me.
> Note: the application is controlling the devargs passed to the driver,
> so it can intercept such information.
These changes are specific only for virtio driver. If application will
treat speed argument, then
it also should check that current driver is virtio and same code will be
duplicated in every application.
>
> I understand it is easier to have the speed info at the same place
> in all cases. But it avoids differentiating what is a reliable info,
> and what is user-provided info.
>
>
>
@@ -356,6 +356,13 @@ Below devargs are supported by the PCI virtio driver:
a virtio device needs to work in vDPA mode.
(Default: 0 (disabled))
+#. ``speed``:
+
+ It is used to specify link speed of virtio device. Link speed is a part of
+ link status structure. It could be requested by application using
+ rte_eth_link_get_nowait function.
+ (Default: 10000 (10G))
+
Below devargs are supported by the virtio-user vdev:
#. ``path``:
@@ -45,6 +45,10 @@ static int virtio_dev_promiscuous_enable(struct rte_eth_dev *dev);
static int virtio_dev_promiscuous_disable(struct rte_eth_dev *dev);
static int virtio_dev_allmulticast_enable(struct rte_eth_dev *dev);
static int virtio_dev_allmulticast_disable(struct rte_eth_dev *dev);
+static uint32_t virtio_dev_speed_capa_get(uint32_t speed);
+static int virtio_dev_devargs_parse(struct rte_devargs *devargs,
+ int *vdpa,
+ uint32_t *speed);
static int virtio_dev_info_get(struct rte_eth_dev *dev,
struct rte_eth_dev_info *dev_info);
static int virtio_dev_link_update(struct rte_eth_dev *dev,
@@ -1861,6 +1865,7 @@ int
eth_virtio_dev_init(struct rte_eth_dev *eth_dev)
{
struct virtio_hw *hw = eth_dev->data->dev_private;
+ uint32_t speed = ETH_SPEED_NUM_10G;
int ret;
if (sizeof(struct virtio_net_hdr_mrg_rxbuf) > RTE_PKTMBUF_HEADROOM) {
@@ -1886,7 +1891,11 @@ eth_virtio_dev_init(struct rte_eth_dev *eth_dev)
return 0;
}
-
+ ret = virtio_dev_devargs_parse(eth_dev->device->devargs,
+ NULL, &speed);
+ if (ret < 0)
+ return ret;
+ hw->speed = speed;
/*
* Pass the information to the rte_eth_dev_close() that it should also
* release the private port resources.
@@ -1956,6 +1965,7 @@ eth_virtio_dev_uninit(struct rte_eth_dev *eth_dev)
return 0;
}
+
static int vdpa_check_handler(__rte_unused const char *key,
const char *value, void *ret_val)
{
@@ -1967,33 +1977,89 @@ static int vdpa_check_handler(__rte_unused const char *key,
return 0;
}
+
+static uint32_t
+virtio_dev_speed_capa_get(uint32_t speed)
+{
+ switch (speed) {
+ case ETH_SPEED_NUM_10G:
+ return ETH_LINK_SPEED_10G;
+ case ETH_SPEED_NUM_20G:
+ return ETH_LINK_SPEED_20G;
+ case ETH_SPEED_NUM_25G:
+ return ETH_LINK_SPEED_25G;
+ case ETH_SPEED_NUM_40G:
+ return ETH_LINK_SPEED_40G;
+ case ETH_SPEED_NUM_50G:
+ return ETH_LINK_SPEED_50G;
+ case ETH_SPEED_NUM_56G:
+ return ETH_LINK_SPEED_56G;
+ case ETH_SPEED_NUM_100G:
+ return ETH_LINK_SPEED_100G;
+ default:
+ return 0;
+ }
+}
+
+
+#define VIRTIO_ARG_SPEED "speed"
+#define VIRTIO_ARG_VDPA "vdpa"
+
+
+static int
+link_speed_handler(const char *key __rte_unused,
+ const char *value, void *ret_val)
+{
+ uint32_t val;
+ if (!value || !ret_val)
+ return -EINVAL;
+ val = strtoul(value, NULL, 0);
+ /* validate input */
+ if (virtio_dev_speed_capa_get(val) == 0)
+ return -EINVAL;
+ *(uint32_t *)ret_val = val;
+
+ return 0;
+}
+
+
static int
-virtio_dev_devargs_parse(struct rte_devargs *devargs, int *vdpa)
+virtio_dev_devargs_parse(struct rte_devargs *devargs, int *vdpa,
+ uint32_t *speed)
{
struct rte_kvargs *kvlist;
- const char *key = "vdpa";
int ret = 0;
if (devargs == NULL)
return 0;
kvlist = rte_kvargs_parse(devargs->args, NULL);
- if (kvlist == NULL)
+ if (kvlist == NULL) {
+ PMD_INIT_LOG(ERR, "error when parsing param");
return 0;
-
- if (!rte_kvargs_count(kvlist, key))
- goto exit;
-
- if (vdpa) {
+ }
+ if (vdpa && rte_kvargs_count(kvlist, VIRTIO_ARG_VDPA) == 1) {
/* vdpa mode selected when there's a key-value pair:
* vdpa=1
*/
- ret = rte_kvargs_process(kvlist, key,
+ ret = rte_kvargs_process(kvlist, VIRTIO_ARG_VDPA,
vdpa_check_handler, vdpa);
- if (ret < 0)
+ if (ret < 0) {
+ PMD_INIT_LOG(ERR, "Failed to parse %s",
+ VIRTIO_ARG_VDPA);
goto exit;
+ }
+ }
+ if (speed && rte_kvargs_count(kvlist, VIRTIO_ARG_SPEED) == 1) {
+ ret = rte_kvargs_process(kvlist,
+ VIRTIO_ARG_SPEED,
+ link_speed_handler, speed);
+ if (ret < 0) {
+ PMD_INIT_LOG(ERR, "Failed to parse %s",
+ VIRTIO_ARG_SPEED);
+ goto exit;
+ }
}
-
exit:
rte_kvargs_free(kvlist);
@@ -2006,7 +2072,7 @@ static int eth_virtio_pci_probe(struct rte_pci_driver *pci_drv __rte_unused,
int vdpa = 0;
int ret = 0;
- ret = virtio_dev_devargs_parse(pci_dev->device.devargs, &vdpa);
+ ret = virtio_dev_devargs_parse(pci_dev->device.devargs, &vdpa, NULL);
if (ret < 0) {
PMD_INIT_LOG(ERR, "devargs parsing is failed");
return ret;
@@ -2385,7 +2451,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 = hw->speed;
link.link_autoneg = ETH_LINK_FIXED;
if (!hw->started) {
@@ -2440,8 +2506,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 = virtio_dev_speed_capa_get(hw->speed);
dev_info->max_rx_queues =
RTE_MIN(hw->max_queue_pairs, VIRTIO_MAX_RX_QUEUES);
@@ -259,6 +259,7 @@ struct virtio_hw {
uint16_t port_id;
uint8_t mac_addr[RTE_ETHER_ADDR_LEN];
uint32_t notify_off_multiplier;
+ uint32_t speed; /* link speed in MB */
uint8_t *isr;
uint16_t *notify_base;
struct virtio_pci_common_cfg *common_cfg;