| Message ID | 1540477076-8412-1-git-send-email-ian.stokes@intel.com |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | [ovs-dev,v1,1/2] netdev-dpdk: Fix netdev_dpdk_get_features(). | expand |
> Report the link speed of the device in netdev_dpdk_get_status() > function. > > Link speed is already reported as part of the netdev_get_features() > function. However only link speeds defined in the OpenFlow specs are > supported so speeds such as 25 Gbps etc. are not shown. The link > speed for the device is available in Mbps in rte_eth_link. > This commit converts the link speed for a given dpdk device to an > easy to read string and reports it in get_status(). Hi Ian, Do we really need to convert the speed from Mbps to Gbps? I'm not sure about that. Anyway, you don't need to snprintf the constant strings. You may just return them like netdev_feature_to_name() does. Another option to reduce the code size is to use few 'if's and calculate the printed value mathematically. > > Suggested-by: Flavio Leitner <fbl at sysclose.org> > Signed-off-by: Ian Stokes <ian.stokes at intel.com> > --- > lib/netdev-dpdk.c | 62 +++++++++++++++++++++++++++++++++++++++++++++++++++++++ > 1 file changed, 62 insertions(+) > > diff --git a/lib/netdev-dpdk.c b/lib/netdev-dpdk.c > index 35eb30f8d..b3a2430a0 100644 > --- a/lib/netdev-dpdk.c > +++ b/lib/netdev-dpdk.c > @@ -164,6 +164,8 @@ typedef uint16_t dpdk_port_t; > #define VHOST_ENQ_RETRY_NUM 8 > #define IF_NAME_SZ (PATH_MAX > IFNAMSIZ ? PATH_MAX : IFNAMSIZ) > > +#define MAX_LINK_SPEED_STR 12 > + > static const struct rte_eth_conf port_conf = { > .rxmode = { > .mq_mode = ETH_MQ_RX_RSS, > @@ -3022,11 +3024,62 @@ netdev_dpdk_vhost_user_get_status(const struct netdev *netdev, > return 0; > } > > +/* > + * Convert a given uint32_t link speed defined in DPDK to a string > + * equivalent. > + */ > +static void > +netdev_dpdk_link_speed_to_str__(uint32_t link_speed, char *speed_str) > +{ > + switch (link_speed) { > + case ETH_SPEED_NUM_10M: > + snprintf(speed_str, MAX_LINK_SPEED_STR,"%s", "10 Mbps"); > + break; > + case ETH_SPEED_NUM_100M: > + snprintf(speed_str, MAX_LINK_SPEED_STR,"%s", "100 Mbps"); > + break; > + case ETH_SPEED_NUM_1G: > + snprintf(speed_str, MAX_LINK_SPEED_STR,"%s", "1 Gbps"); > + break; > + case ETH_SPEED_NUM_2_5G: > + snprintf(speed_str, MAX_LINK_SPEED_STR,"%s", "2.5 Gbps"); > + break; > + case ETH_SPEED_NUM_5G: > + snprintf(speed_str, MAX_LINK_SPEED_STR,"%s", "5 Gbps"); > + break; > + case ETH_SPEED_NUM_10G: > + snprintf(speed_str, MAX_LINK_SPEED_STR,"%s", "10 Gbps"); > + break; > + case ETH_SPEED_NUM_20G: > + snprintf(speed_str, MAX_LINK_SPEED_STR,"%s", "20 Gbps"); > + break; > + case ETH_SPEED_NUM_25G: > + snprintf(speed_str, MAX_LINK_SPEED_STR,"%s", "25 Gbps"); > + break; > + case ETH_SPEED_NUM_40G: > + snprintf(speed_str, MAX_LINK_SPEED_STR,"%s", "40 Gbps"); > + break; > + case ETH_SPEED_NUM_50G: > + snprintf(speed_str, MAX_LINK_SPEED_STR,"%s", "50 Gbps"); > + break; > + case ETH_SPEED_NUM_56G: > + snprintf(speed_str, MAX_LINK_SPEED_STR,"%s", "56 Gbps"); > + break; > + case ETH_SPEED_NUM_100G: > + snprintf(speed_str, MAX_LINK_SPEED_STR,"%s", "100 Gbps"); > + break; > + default: > + snprintf(speed_str, MAX_LINK_SPEED_STR,"%s", "Not Defined"); > + } > +} > + > static int > netdev_dpdk_get_status(const struct netdev *netdev, struct smap *args) > { > struct netdev_dpdk *dev = netdev_dpdk_cast(netdev); > struct rte_eth_dev_info dev_info; > + struct rte_eth_link link; > + char link_speed_str [MAX_LINK_SPEED_STR]; Extra space not needed before the '['. > > if (!rte_eth_dev_is_valid_port(dev->port_id)) { > return ENODEV; > @@ -3034,6 +3087,7 @@ netdev_dpdk_get_status(const struct netdev *netdev, struct smap *args) > > ovs_mutex_lock(&dev->mutex); > rte_eth_dev_info_get(dev->port_id, &dev_info); > + link = dev->link; > ovs_mutex_unlock(&dev->mutex); > > smap_add_format(args, "port_no", DPDK_PORT_ID_FMT, dev->port_id); > @@ -3065,6 +3119,14 @@ netdev_dpdk_get_status(const struct netdev *netdev, struct smap *args) > dev_info.pci_dev->id.device_id); > } > > + /* Not all link speeds are defined in the OpenFlow specs e.g. 25 Gbps. > + * In that case the speed will not be reported as part of the usual > + * call to get_features(). Get the link speed of the device and add it > + * to the device status in an easy to read string format. > + */ > + netdev_dpdk_link_speed_to_str__(link.link_speed, link_speed_str); > + smap_add_format(args, "link_speed", "%s", link_speed_str); > + > return 0; > } > > -- > 2.13.6
> > Report the link speed of the device in netdev_dpdk_get_status() > > function. > > > > Link speed is already reported as part of the netdev_get_features() > > function. However only link speeds defined in the OpenFlow specs are > > supported so speeds such as 25 Gbps etc. are not shown. The link speed > > for the device is available in Mbps in rte_eth_link. > > This commit converts the link speed for a given dpdk device to an easy > > to read string and reports it in get_status(). > > Hi Ian, > Do we really need to convert the speed from Mbps to Gbps? I'm not sure > about that. Currently speeds are being defined via different measurements. i.e. when listing an interface link speed is reported in bits, the openflow specs expect speeds to be in kilobits and DPDK provides the speeds in megabits. This can be a bit ambiguous unless the user knows what measurement is being shown for instance 1Gb is "1000000000" could be mistook 10Gb "10000000000". We could just convert the DPDK value regardless to bits instead of mbps. But in that case I thought it would be easier and less ambiguous for the user to report the speed via a string along with the measurement so for a 10 Gb interface use "10 Gbps" instead of "10000000000" What are your thoughts? > Anyway, you don't need to snprintf the constant strings. You may just > return them like netdev_feature_to_name() does. Sure that looks a bit more concise. I could use that for the v2. Ian > Another option to reduce the code size is to use few 'if's and calculate > the printed value mathematically. > > > > > Suggested-by: Flavio Leitner <fbl at sysclose.org> > > Signed-off-by: Ian Stokes <ian.stokes at intel.com> > > --- > > lib/netdev-dpdk.c | 62 > > +++++++++++++++++++++++++++++++++++++++++++++++++++++++ > > 1 file changed, 62 insertions(+) > > > > diff --git a/lib/netdev-dpdk.c b/lib/netdev-dpdk.c index > > 35eb30f8d..b3a2430a0 100644 > > --- a/lib/netdev-dpdk.c > > +++ b/lib/netdev-dpdk.c > > @@ -164,6 +164,8 @@ typedef uint16_t dpdk_port_t; #define > > VHOST_ENQ_RETRY_NUM 8 #define IF_NAME_SZ (PATH_MAX > IFNAMSIZ ? > > PATH_MAX : IFNAMSIZ) > > > > +#define MAX_LINK_SPEED_STR 12 > > + > > static const struct rte_eth_conf port_conf = { > > .rxmode = { > > .mq_mode = ETH_MQ_RX_RSS, > > @@ -3022,11 +3024,62 @@ netdev_dpdk_vhost_user_get_status(const struct > netdev *netdev, > > return 0; > > } > > > > +/* > > + * Convert a given uint32_t link speed defined in DPDK to a string > > + * equivalent. > > + */ > > +static void > > +netdev_dpdk_link_speed_to_str__(uint32_t link_speed, char *speed_str) > > +{ > > + switch (link_speed) { > > + case ETH_SPEED_NUM_10M: > > + snprintf(speed_str, MAX_LINK_SPEED_STR,"%s", "10 Mbps"); > > + break; > > + case ETH_SPEED_NUM_100M: > > + snprintf(speed_str, MAX_LINK_SPEED_STR,"%s", "100 Mbps"); > > + break; > > + case ETH_SPEED_NUM_1G: > > + snprintf(speed_str, MAX_LINK_SPEED_STR,"%s", "1 Gbps"); > > + break; > > + case ETH_SPEED_NUM_2_5G: > > + snprintf(speed_str, MAX_LINK_SPEED_STR,"%s", "2.5 Gbps"); > > + break; > > + case ETH_SPEED_NUM_5G: > > + snprintf(speed_str, MAX_LINK_SPEED_STR,"%s", "5 Gbps"); > > + break; > > + case ETH_SPEED_NUM_10G: > > + snprintf(speed_str, MAX_LINK_SPEED_STR,"%s", "10 Gbps"); > > + break; > > + case ETH_SPEED_NUM_20G: > > + snprintf(speed_str, MAX_LINK_SPEED_STR,"%s", "20 Gbps"); > > + break; > > + case ETH_SPEED_NUM_25G: > > + snprintf(speed_str, MAX_LINK_SPEED_STR,"%s", "25 Gbps"); > > + break; > > + case ETH_SPEED_NUM_40G: > > + snprintf(speed_str, MAX_LINK_SPEED_STR,"%s", "40 Gbps"); > > + break; > > + case ETH_SPEED_NUM_50G: > > + snprintf(speed_str, MAX_LINK_SPEED_STR,"%s", "50 Gbps"); > > + break; > > + case ETH_SPEED_NUM_56G: > > + snprintf(speed_str, MAX_LINK_SPEED_STR,"%s", "56 Gbps"); > > + break; > > + case ETH_SPEED_NUM_100G: > > + snprintf(speed_str, MAX_LINK_SPEED_STR,"%s", "100 Gbps"); > > + break; > > + default: > > + snprintf(speed_str, MAX_LINK_SPEED_STR,"%s", "Not Defined"); > > + } > > +} > > + > > static int > > netdev_dpdk_get_status(const struct netdev *netdev, struct smap > > *args) { > > struct netdev_dpdk *dev = netdev_dpdk_cast(netdev); > > struct rte_eth_dev_info dev_info; > > + struct rte_eth_link link; > > + char link_speed_str [MAX_LINK_SPEED_STR]; > > Extra space not needed before the '['. > > > > > if (!rte_eth_dev_is_valid_port(dev->port_id)) { > > return ENODEV; > > @@ -3034,6 +3087,7 @@ netdev_dpdk_get_status(const struct netdev > > *netdev, struct smap *args) > > > > ovs_mutex_lock(&dev->mutex); > > rte_eth_dev_info_get(dev->port_id, &dev_info); > > + link = dev->link; > > ovs_mutex_unlock(&dev->mutex); > > > > smap_add_format(args, "port_no", DPDK_PORT_ID_FMT, dev->port_id); > > @@ -3065,6 +3119,14 @@ netdev_dpdk_get_status(const struct netdev > *netdev, struct smap *args) > > dev_info.pci_dev->id.device_id); > > } > > > > + /* Not all link speeds are defined in the OpenFlow specs e.g. 25 > Gbps. > > + * In that case the speed will not be reported as part of the usual > > + * call to get_features(). Get the link speed of the device and add > it > > + * to the device status in an easy to read string format. > > + */ > > + netdev_dpdk_link_speed_to_str__(link.link_speed, link_speed_str); > > + smap_add_format(args, "link_speed", "%s", link_speed_str); > > + > > return 0; > > } > > > > -- > > 2.13.6
On Tue, Oct 30, 2018 at 01:10:51PM +0000, Stokes, Ian wrote: > > > Report the link speed of the device in netdev_dpdk_get_status() > > > function. > > > > > > Link speed is already reported as part of the netdev_get_features() > > > function. However only link speeds defined in the OpenFlow specs are > > > supported so speeds such as 25 Gbps etc. are not shown. The link speed > > > for the device is available in Mbps in rte_eth_link. > > > This commit converts the link speed for a given dpdk device to an easy > > > to read string and reports it in get_status(). [...] > > Anyway, you don't need to snprintf the constant strings. You may just > > return them like netdev_feature_to_name() does. > Sure that looks a bit more concise. I could use that for the v2. That sounds reasonable, otherwise both patches look good to me. Thanks, fbl
diff --git a/lib/netdev-dpdk.c b/lib/netdev-dpdk.c index 35eb30f8d..b3a2430a0 100644 --- a/lib/netdev-dpdk.c +++ b/lib/netdev-dpdk.c @@ -164,6 +164,8 @@ typedef uint16_t dpdk_port_t; #define VHOST_ENQ_RETRY_NUM 8 #define IF_NAME_SZ (PATH_MAX > IFNAMSIZ ? PATH_MAX : IFNAMSIZ) +#define MAX_LINK_SPEED_STR 12 + static const struct rte_eth_conf port_conf = { .rxmode = { .mq_mode = ETH_MQ_RX_RSS, @@ -3022,11 +3024,62 @@ netdev_dpdk_vhost_user_get_status(const struct netdev *netdev, return 0; } +/* + * Convert a given uint32_t link speed defined in DPDK to a string + * equivalent. + */ +static void +netdev_dpdk_link_speed_to_str__(uint32_t link_speed, char *speed_str) +{ + switch (link_speed) { + case ETH_SPEED_NUM_10M: + snprintf(speed_str, MAX_LINK_SPEED_STR,"%s", "10 Mbps"); + break; + case ETH_SPEED_NUM_100M: + snprintf(speed_str, MAX_LINK_SPEED_STR,"%s", "100 Mbps"); + break; + case ETH_SPEED_NUM_1G: + snprintf(speed_str, MAX_LINK_SPEED_STR,"%s", "1 Gbps"); + break; + case ETH_SPEED_NUM_2_5G: + snprintf(speed_str, MAX_LINK_SPEED_STR,"%s", "2.5 Gbps"); + break; + case ETH_SPEED_NUM_5G: + snprintf(speed_str, MAX_LINK_SPEED_STR,"%s", "5 Gbps"); + break; + case ETH_SPEED_NUM_10G: + snprintf(speed_str, MAX_LINK_SPEED_STR,"%s", "10 Gbps"); + break; + case ETH_SPEED_NUM_20G: + snprintf(speed_str, MAX_LINK_SPEED_STR,"%s", "20 Gbps"); + break; + case ETH_SPEED_NUM_25G: + snprintf(speed_str, MAX_LINK_SPEED_STR,"%s", "25 Gbps"); + break; + case ETH_SPEED_NUM_40G: + snprintf(speed_str, MAX_LINK_SPEED_STR,"%s", "40 Gbps"); + break; + case ETH_SPEED_NUM_50G: + snprintf(speed_str, MAX_LINK_SPEED_STR,"%s", "50 Gbps"); + break; + case ETH_SPEED_NUM_56G: + snprintf(speed_str, MAX_LINK_SPEED_STR,"%s", "56 Gbps"); + break; + case ETH_SPEED_NUM_100G: + snprintf(speed_str, MAX_LINK_SPEED_STR,"%s", "100 Gbps"); + break; + default: + snprintf(speed_str, MAX_LINK_SPEED_STR,"%s", "Not Defined"); + } +} + static int netdev_dpdk_get_status(const struct netdev *netdev, struct smap *args) { struct netdev_dpdk *dev = netdev_dpdk_cast(netdev); struct rte_eth_dev_info dev_info; + struct rte_eth_link link; + char link_speed_str [MAX_LINK_SPEED_STR]; if (!rte_eth_dev_is_valid_port(dev->port_id)) { return ENODEV; @@ -3034,6 +3087,7 @@ netdev_dpdk_get_status(const struct netdev *netdev, struct smap *args) ovs_mutex_lock(&dev->mutex); rte_eth_dev_info_get(dev->port_id, &dev_info); + link = dev->link; ovs_mutex_unlock(&dev->mutex); smap_add_format(args, "port_no", DPDK_PORT_ID_FMT, dev->port_id); @@ -3065,6 +3119,14 @@ netdev_dpdk_get_status(const struct netdev *netdev, struct smap *args) dev_info.pci_dev->id.device_id); } + /* Not all link speeds are defined in the OpenFlow specs e.g. 25 Gbps. + * In that case the speed will not be reported as part of the usual + * call to get_features(). Get the link speed of the device and add it + * to the device status in an easy to read string format. + */ + netdev_dpdk_link_speed_to_str__(link.link_speed, link_speed_str); + smap_add_format(args, "link_speed", "%s", link_speed_str); + return 0; }
Report the link speed of the device in netdev_dpdk_get_status() function. Link speed is already reported as part of the netdev_get_features() function. However only link speeds defined in the OpenFlow specs are supported so speeds such as 25 Gbps etc. are not shown. The link speed for the device is available in Mbps in rte_eth_link. This commit converts the link speed for a given dpdk device to an easy to read string and reports it in get_status(). Suggested-by: Flavio Leitner <fbl@sysclose.org> Signed-off-by: Ian Stokes <ian.stokes@intel.com> --- lib/netdev-dpdk.c | 62 +++++++++++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 62 insertions(+)