[ovs-dev] [PATCH 3/3] netdev-dpdk: Use uint8_t for port_id.
Darrell Ball
dball at vmware.com
Sat May 13 04:00:02 UTC 2017
On 5/12/17, 8:04 AM, "Ilya Maximets" <i.maximets at samsung.com> wrote:
On 05.04.2017 22:34, Darrell Ball wrote:
>
>
> On 4/3/17, 8:04 AM, "ovs-dev-bounces at openvswitch.org on behalf of Ilya Maximets" <ovs-dev-bounces at openvswitch.org on behalf of i.maximets at samsung.com> wrote:
>
> Currently, signed integer is used for 'port_id' variable and
> '-1' as identifier of bad or uninitialized 'port_id'.
>
> This inconsistent with dpdk library and, also, in few cases,
> leads to passing '-1' to dpdk functions where uint8_t expected.
>
> Such behaviour doesn't produce any issues, but it's better to
> use same type as in dpdk library for consistency.
>
> Also, magic number '-1' replaced with DPDK_ETH_PORT_ID_INVALID
> macro.
>
> Signed-off-by: Ilya Maximets <i.maximets at samsung.com>
> ---
> lib/netdev-dpdk.c | 61 +++++++++++++++++++++++++++++++------------------------
> 1 file changed, 35 insertions(+), 26 deletions(-)
>
> diff --git a/lib/netdev-dpdk.c b/lib/netdev-dpdk.c
> index 658a454..216ced8 100644
> --- a/lib/netdev-dpdk.c
> +++ b/lib/netdev-dpdk.c
> @@ -140,6 +140,8 @@ BUILD_ASSERT_DECL((MAX_NB_MBUF / ROUND_DOWN_POW2(MAX_NB_MBUF/MIN_NB_MBUF))
> #define OVS_VHOST_QUEUE_DISABLED (-2) /* Queue was disabled by guest and not
> * yet mapped to another queue. */
>
> +#define DPDK_ETH_PORT_ID_INVALID RTE_MAX_ETHPORTS
> +
> #define VHOST_ENQ_RETRY_NUM 8
> #define IF_NAME_SZ (PATH_MAX > IFNAMSIZ ? PATH_MAX : IFNAMSIZ)
>
> @@ -309,7 +311,7 @@ struct dpdk_ring {
> struct rte_ring *cring_tx;
> struct rte_ring *cring_rx;
> unsigned int user_port_id; /* User given port no, parsed from port name */
> - int eth_port_id; /* ethernet device port id */
> + uint8_t eth_port_id; /* ethernet device port id */
>
> The rte does not have a typedef for port_id.
> One optional change is for OVS to have a typedef for this external type.
> /* The dpdk rte uses uint8_t for port_id. */
> typedef uint8_t rte_port_id;
I don't think that it is a good change to do because we will have to
create additional typedef at least for PRIu8. This will look ugly.
No, see my following diff
Also, if DPDK someday will create typedef with different name
we will have typedef of the typedef?
I don’t follow this comment; see the diff below.
The reasons for the typedef here are:
1) Easier to maintain if the size of type changes in future
2) Serves as documentation that the origin of the type is the rte library and
and originates externally to ovs
Here is a few examples of typedefs in the OVS code base:
dpif-netdev.c:5587: typedef uint32_t map_type;
stp.h:41:typedef uint64_t stp_identifier;
timeval.c:45:typedef unsigned int clockid_t;
dball at ubuntu:~/ovs$ git diff
diff --git a/lib/netdev-dpdk.c b/lib/netdev-dpdk.c
index 609b8da..6667274 100644
--- a/lib/netdev-dpdk.c
+++ b/lib/netdev-dpdk.c
@@ -143,6 +143,9 @@ BUILD_ASSERT_DECL((MAX_NB_MBUF / ROUND_DOWN_POW2(MAX_NB_MBUF/MIN_NB_MBUF))
#define VHOST_ENQ_RETRY_NUM 8
#define IF_NAME_SZ (PATH_MAX > IFNAMSIZ ? PATH_MAX : IFNAMSIZ)
+#define DPDK_ETH_PORT_ID_INVALID RTE_MAX_ETHPORTS
+typedef uint8_t rte_port_id;
+
static const struct rte_eth_conf port_conf = {
.rxmode = {
.mq_mode = ETH_MQ_RX_RSS,
@@ -309,7 +312,7 @@ struct dpdk_ring {
struct rte_ring *cring_tx;
struct rte_ring *cring_rx;
unsigned int user_port_id; /* User given port no, parsed from port name */
- int eth_port_id; /* ethernet device port id */
+ rte_port_id eth_port_id; /* ethernet device port id */
struct ovs_list list_node OVS_GUARDED_BY(dpdk_mutex);
};
@@ -325,7 +328,7 @@ enum dpdk_hw_ol_features {
struct netdev_dpdk {
struct netdev up;
- int port_id;
+ rte_port_id port_id;
int max_packet_len;
enum dpdk_dev_type type;
@@ -399,7 +402,7 @@ struct netdev_dpdk {
struct netdev_rxq_dpdk {
struct netdev_rxq up;
- int port_id;
+ rte_port_id port_id;
};
static int netdev_dpdk_class_init(void);
@@ -600,12 +603,12 @@ check_link_status(struct netdev_dpdk *dev)
dev->link_reset_cnt++;
dev->link = link;
if (dev->link.link_status) {
- VLOG_DBG_RL(&rl, "Port %d Link Up - speed %u Mbps - %s",
+ VLOG_DBG_RL(&rl, "Port %"PRIu8" Link Up - speed %u Mbps - %s",
dev->port_id, (unsigned) dev->link.link_speed,
(dev->link.link_duplex == ETH_LINK_FULL_DUPLEX) ?
("full-duplex") : ("half-duplex"));
} else {
- VLOG_DBG_RL(&rl, "Port %d Link Down", dev->port_id);
+ VLOG_DBG_RL(&rl, "Port %"PRIu8" Link Down", dev->port_id);
}
}
}
@@ -723,7 +726,7 @@ dpdk_eth_checksum_offload_configure(struct netdev_dpdk *dev)
if (rx_csum_ol_flag &&
(info.rx_offload_capa & rx_chksm_offload_capa) !=
rx_chksm_offload_capa) {
- VLOG_WARN_ONCE("Rx checksum offload is not supported on device %d",
+ VLOG_WARN_ONCE("Rx checksum offload is not supported on device %"PRIu8,
dev->port_id);
dev->hw_ol_features &= ~NETDEV_RX_CHECKSUM_OFFLOAD;
return;
@@ -735,7 +738,8 @@ static void
dpdk_eth_flow_ctrl_setup(struct netdev_dpdk *dev) OVS_REQUIRES(dev->mutex)
{
if (rte_eth_dev_flow_ctrl_set(dev->port_id, &dev->fc_conf)) {
- VLOG_WARN("Failed to enable flow control on device %d", dev->port_id);
+ VLOG_WARN("Failed to enable flow control on device %"PRIu8,
+ dev->port_id);
}
}
@@ -773,7 +777,7 @@ dpdk_eth_dev_init(struct netdev_dpdk *dev)
memset(ð_addr, 0x0, sizeof(eth_addr));
rte_eth_macaddr_get(dev->port_id, ð_addr);
- VLOG_INFO_RL(&rl, "Port %d: "ETH_ADDR_FMT,
+ VLOG_INFO_RL(&rl, "Port %"PRIu8": "ETH_ADDR_FMT,
dev->port_id, ETH_ADDR_BYTES_ARGS(eth_addr.addr_bytes));
memcpy(dev->hwaddr.ea, eth_addr.addr_bytes, ETH_ADDR_LEN);
@@ -785,7 +789,7 @@ dpdk_eth_dev_init(struct netdev_dpdk *dev)
/* Get the Flow control configuration for DPDK-ETH */
diag = rte_eth_dev_flow_ctrl_get(dev->port_id, &dev->fc_conf);
if (diag) {
- VLOG_DBG("cannot get flow control parameters on port=%d, err=%d",
+ VLOG_DBG("cannot get flow control parameters on port=%"PRIu8", err=%d",
dev->port_id, diag);
}
@@ -830,7 +834,7 @@ netdev_dpdk_alloc_txq(unsigned int n_txqs)
}
static int
-common_construct(struct netdev *netdev, unsigned int port_no,
+common_construct(struct netdev *netdev, rte_port_id port_no,
enum dpdk_dev_type type, int socket_id)
OVS_REQUIRES(dpdk_mutex)
{
@@ -914,7 +918,8 @@ vhost_common_construct(struct netdev *netdev)
return ENOMEM;
}
- return common_construct(netdev, -1, DPDK_DEV_VHOST, socket_id);
+ return common_construct(netdev, DPDK_ETH_PORT_ID_INVALID,
+ DPDK_DEV_VHOST, socket_id);
}
static int
@@ -974,7 +979,8 @@ netdev_dpdk_construct(struct netdev *netdev)
int err;
ovs_mutex_lock(&dpdk_mutex);
- err = common_construct(netdev, -1, DPDK_DEV_ETH, SOCKET0);
+ err = common_construct(netdev, DPDK_ETH_PORT_ID_INVALID,
+ DPDK_DEV_ETH, SOCKET0);
ovs_mutex_unlock(&dpdk_mutex);
return err;
}
@@ -1097,7 +1103,7 @@ netdev_dpdk_get_config(const struct netdev *netdev, struct smap *args)
}
static struct netdev_dpdk *
-netdev_dpdk_lookup_by_port_id(int port_id)
+netdev_dpdk_lookup_by_port_id(rte_port_id port_id)
OVS_REQUIRES(dpdk_mutex)
{
struct netdev_dpdk *dev;
@@ -1111,7 +1117,7 @@ netdev_dpdk_lookup_by_port_id(int port_id)
return NULL;
}
-static int
+static rte_port_id
netdev_dpdk_process_devargs(const char *devargs, char **errp)
{
uint8_t new_port_id = UINT8_MAX;
@@ -1126,7 +1132,7 @@ netdev_dpdk_process_devargs(const char *devargs, char **errp)
} else {
/* Attach unsuccessful */
VLOG_WARN_BUF(errp, "Error attaching device '%s' to DPDK", devargs);
- return -1;
+ new_port_id = DPDK_ETH_PORT_ID_INVALID;
}
}
@@ -1206,7 +1212,8 @@ netdev_dpdk_set_config(struct netdev *netdev, const struct smap *args,
* is valid */
if (!(dev->devargs && !strcmp(dev->devargs, new_devargs)
&& rte_eth_dev_is_valid_port(dev->port_id))) {
- int new_port_id = netdev_dpdk_process_devargs(new_devargs, errp);
+ rte_port_id new_port_id = netdev_dpdk_process_devargs(new_devargs,
+ errp);
if (!rte_eth_dev_is_valid_port(new_port_id)) {
err = EINVAL;
} else if (new_port_id == dev->port_id) {
@@ -2028,7 +2035,7 @@ netdev_dpdk_get_stats(const struct netdev *netdev, struct netdev_stats *stats)
int rte_xstats_len, rte_xstats_new_len, rte_xstats_ret;
if (rte_eth_stats_get(dev->port_id, &rte_stats)) {
- VLOG_ERR("Can't get ETH statistics for port: %i.", dev->port_id);
+ VLOG_ERR("Can't get ETH statistics for port: %"PRIu8, dev->port_id);
ovs_mutex_unlock(&dev->mutex);
return EPROTO;
}
@@ -2036,7 +2043,8 @@ netdev_dpdk_get_stats(const struct netdev *netdev, struct netdev_stats *stats)
/* Get length of statistics */
rte_xstats_len = rte_eth_xstats_get_names(dev->port_id, NULL, 0);
if (rte_xstats_len < 0) {
- VLOG_WARN("Cannot get XSTATS values for port: %i", dev->port_id);
+ VLOG_WARN("Cannot get XSTATS names for port: %"PRIu8, dev->port_id);
+
goto out;
}
/* Reserve memory for xstats names and values */
@@ -2059,7 +2067,7 @@ netdev_dpdk_get_stats(const struct netdev *netdev, struct netdev_stats *stats)
netdev_dpdk_convert_xstats(stats, rte_xstats, rte_xstats_names,
rte_xstats_len);
} else {
- VLOG_WARN("Cannot get XSTATS values for port: %i.", dev->port_id);
+ VLOG_WARN("Cannot get XSTATS values for port: %"PRIu8, dev->port_id);
}
out:
@@ -2785,7 +2793,7 @@ netdev_dpdk_vhost_class_init(void)
static int
dpdk_ring_create(const char dev_name[], unsigned int port_no,
- unsigned int *eth_port_id)
+ rte_port_id *eth_port_id)
{
struct dpdk_ring *ring_pair;
char *ring_name;
@@ -2837,7 +2845,7 @@ dpdk_ring_create(const char dev_name[], unsigned int port_no,
}
static int
-dpdk_ring_open(const char dev_name[], unsigned int *eth_port_id)
+dpdk_ring_open(const char dev_name[], rte_port_id *eth_port_id)
OVS_REQUIRES(dpdk_mutex)
{
struct dpdk_ring *ring_pair;
@@ -2886,7 +2894,7 @@ netdev_dpdk_ring_send(struct netdev *netdev, int qid,
static int
netdev_dpdk_ring_construct(struct netdev *netdev)
{
- unsigned int port_no = 0;
+ rte_port_id port_no = 0;
int err = 0;
ovs_mutex_lock(&dpdk_mutex);
(END)
>
> struct ovs_list list_node OVS_GUARDED_BY(dpdk_mutex);
> };
>
> @@ -325,7 +327,7 @@ enum dpdk_hw_ol_features {
>
> struct netdev_dpdk {
> struct netdev up;
> - int port_id;
> + uint8_t port_id;
> int max_packet_len;
> enum dpdk_dev_type type;
>
> @@ -402,7 +404,7 @@ struct netdev_dpdk {
>
> struct netdev_rxq_dpdk {
> struct netdev_rxq up;
> - int port_id;
> + uint8_t port_id;
> };
>
> static int netdev_dpdk_class_init(void);
> @@ -602,12 +604,12 @@ check_link_status(struct netdev_dpdk *dev)
> dev->link_reset_cnt++;
> dev->link = link;
> if (dev->link.link_status) {
> - VLOG_DBG_RL(&rl, "Port %d Link Up - speed %u Mbps - %s",
> + VLOG_DBG_RL(&rl, "Port %"PRIu8" Link Up - speed %u Mbps - %s",
> dev->port_id, (unsigned) dev->link.link_speed,
> (dev->link.link_duplex == ETH_LINK_FULL_DUPLEX) ?
> ("full-duplex") : ("half-duplex"));
> } else {
> - VLOG_DBG_RL(&rl, "Port %d Link Down", dev->port_id);
> + VLOG_DBG_RL(&rl, "Port %"PRIu8" Link Down", dev->port_id);
> }
> }
> }
> @@ -725,8 +727,8 @@ dpdk_eth_checksum_offload_configure(struct netdev_dpdk *dev)
> if (rx_csum_ol_flag &&
> (info.rx_offload_capa & rx_chksm_offload_capa) !=
> rx_chksm_offload_capa) {
> - VLOG_WARN_ONCE("Rx checksum offload is not supported on device %d",
> - dev->port_id);
> + VLOG_WARN_ONCE("Rx checksum offload is not supported on device %"PRIu8,
> + dev->port_id);
> dev->hw_ol_features &= ~NETDEV_RX_CHECKSUM_OFFLOAD;
> return;
> }
> @@ -737,7 +739,8 @@ static void
> dpdk_eth_flow_ctrl_setup(struct netdev_dpdk *dev) OVS_REQUIRES(dev->mutex)
> {
> if (rte_eth_dev_flow_ctrl_set(dev->port_id, &dev->fc_conf)) {
> - VLOG_WARN("Failed to enable flow control on device %d", dev->port_id);
> + VLOG_WARN("Failed to enable flow control on device %"PRIu8,
> + dev->port_id);
> }
> }
>
> @@ -775,7 +778,7 @@ dpdk_eth_dev_init(struct netdev_dpdk *dev)
>
> memset(ð_addr, 0x0, sizeof(eth_addr));
> rte_eth_macaddr_get(dev->port_id, ð_addr);
> - VLOG_INFO_RL(&rl, "Port %d: "ETH_ADDR_FMT,
> + VLOG_INFO_RL(&rl, "Port %"PRIu8": "ETH_ADDR_FMT,
> dev->port_id, ETH_ADDR_BYTES_ARGS(eth_addr.addr_bytes));
>
> memcpy(dev->hwaddr.ea, eth_addr.addr_bytes, ETH_ADDR_LEN);
> @@ -787,7 +790,7 @@ dpdk_eth_dev_init(struct netdev_dpdk *dev)
> /* Get the Flow control configuration for DPDK-ETH */
> diag = rte_eth_dev_flow_ctrl_get(dev->port_id, &dev->fc_conf);
> if (diag) {
> - VLOG_DBG("cannot get flow control parameters on port=%d, err=%d",
> + VLOG_DBG("cannot get flow control parameters on port=%"PRIu8", err=%d",
> dev->port_id, diag);
> }
>
> @@ -832,7 +835,7 @@ netdev_dpdk_alloc_txq(unsigned int n_txqs)
> }
>
> static int
> -common_construct(struct netdev *netdev, unsigned int port_no,
> +common_construct(struct netdev *netdev, uint8_t port_no,
> enum dpdk_dev_type type, int socket_id)
> OVS_REQUIRES(dpdk_mutex)
> {
> @@ -917,7 +920,8 @@ vhost_common_construct(struct netdev *netdev)
> return ENOMEM;
> }
>
> - return common_construct(netdev, -1, DPDK_DEV_VHOST, socket_id);
> + return common_construct(netdev, DPDK_ETH_PORT_ID_INVALID,
> + DPDK_DEV_VHOST, socket_id);
> }
>
> static int
> @@ -977,7 +981,8 @@ netdev_dpdk_construct(struct netdev *netdev)
> int err;
>
> ovs_mutex_lock(&dpdk_mutex);
> - err = common_construct(netdev, -1, DPDK_DEV_ETH, SOCKET0);
> + err = common_construct(netdev, DPDK_ETH_PORT_ID_INVALID,
> + DPDK_DEV_ETH, SOCKET0);
> ovs_mutex_unlock(&dpdk_mutex);
> return err;
> }
> @@ -1111,7 +1116,7 @@ netdev_dpdk_get_config(const struct netdev *netdev, struct smap *args)
> }
>
> static struct netdev_dpdk *
> -netdev_dpdk_lookup_by_port_id(int port_id)
> +netdev_dpdk_lookup_by_port_id(uint8_t port_id)
> OVS_REQUIRES(dpdk_mutex)
> {
> struct netdev_dpdk *dev;
> @@ -1125,10 +1130,11 @@ netdev_dpdk_lookup_by_port_id(int port_id)
> return NULL;
> }
>
> -static int
> -netdev_dpdk_process_devargs(const char *devargs, char **errp)
> +static uint8_t
> +netdev_dpdk_process_devargs(struct netdev_dpdk *dev,
> + const char *devargs, char **errp)
> {
> - uint8_t new_port_id = UINT8_MAX;
> + uint8_t new_port_id = DPDK_ETH_PORT_ID_INVALID;
> char *ind, *name = xstrdup(devargs);
>
> /* Get the name from the comma separated list of arguments. */
> @@ -1148,7 +1154,7 @@ netdev_dpdk_process_devargs(const char *devargs, char **errp)
> } else {
> /* Attach unsuccessful */
> VLOG_WARN_BUF(errp, "Error attaching device '%s' to DPDK", devargs);
> - return -1;
> + new_port_id = DPDK_ETH_PORT_ID_INVALID;
> }
> }
>
> @@ -1229,7 +1235,8 @@ netdev_dpdk_set_config(struct netdev *netdev, const struct smap *args,
> * is valid */
> if (!(dev->devargs && !strcmp(dev->devargs, new_devargs)
> && rte_eth_dev_is_valid_port(dev->port_id))) {
> - int new_port_id = netdev_dpdk_process_devargs(new_devargs, errp);
> + uint8_t new_port_id = netdev_dpdk_process_devargs(dev, new_devargs,
> + errp);
> if (!rte_eth_dev_is_valid_port(new_port_id)) {
> err = EINVAL;
> } else if (new_port_id == dev->port_id) {
> @@ -1237,6 +1244,7 @@ netdev_dpdk_set_config(struct netdev *netdev, const struct smap *args,
> err = 0;
> } else {
> struct netdev_dpdk *dup_dev;
> +
> dup_dev = netdev_dpdk_lookup_by_port_id(new_port_id);
> if (dup_dev) {
> VLOG_WARN_BUF(errp, "'%s' is trying to use device '%s' "
> @@ -1246,6 +1254,7 @@ netdev_dpdk_set_config(struct netdev *netdev, const struct smap *args,
> err = EADDRINUSE;
> } else {
> int sid = rte_eth_dev_socket_id(new_port_id);
> +
> dev->requested_socket_id = sid < 0 ? SOCKET0 : sid;
> dev->devargs = xstrdup(new_devargs);
> dev->port_id = new_port_id;
> @@ -2051,7 +2060,7 @@ netdev_dpdk_get_stats(const struct netdev *netdev, struct netdev_stats *stats)
> int rte_xstats_len, rte_xstats_new_len, rte_xstats_ret;
>
> if (rte_eth_stats_get(dev->port_id, &rte_stats)) {
> - VLOG_ERR("Can't get ETH statistics for port: %i.", dev->port_id);
> + VLOG_ERR("Can't get ETH statistics for port: %"PRIu8, dev->port_id);
> ovs_mutex_unlock(&dev->mutex);
> return EPROTO;
> }
> @@ -2059,7 +2068,7 @@ netdev_dpdk_get_stats(const struct netdev *netdev, struct netdev_stats *stats)
> /* Get length of statistics */
> rte_xstats_len = rte_eth_xstats_get_names(dev->port_id, NULL, 0);
> if (rte_xstats_len < 0) {
> - VLOG_WARN("Cannot get XSTATS values for port: %i", dev->port_id);
> + VLOG_WARN("Cannot get XSTATS values for port: %"PRIu8, dev->port_id);
> goto out;
> }
> /* Reserve memory for xstats names and values */
> @@ -2071,7 +2080,7 @@ netdev_dpdk_get_stats(const struct netdev *netdev, struct netdev_stats *stats)
> rte_xstats_names,
> rte_xstats_len);
> if (rte_xstats_new_len != rte_xstats_len) {
> - VLOG_WARN("Cannot get XSTATS names for port: %i.", dev->port_id);
> + VLOG_WARN("Cannot get XSTATS names for port: %"PRIu8, dev->port_id);
> goto out;
> }
> /* Retreive xstats values */
> @@ -2082,7 +2091,7 @@ netdev_dpdk_get_stats(const struct netdev *netdev, struct netdev_stats *stats)
> netdev_dpdk_convert_xstats(stats, rte_xstats, rte_xstats_names,
> rte_xstats_len);
> } else {
> - VLOG_WARN("Cannot get XSTATS values for port: %i.", dev->port_id);
> + VLOG_WARN("Cannot get XSTATS values for port: %"PRIu8, dev->port_id);
> }
>
> out:
> @@ -2759,7 +2768,7 @@ netdev_dpdk_vhost_class_init(void)
>
> static int
> dpdk_ring_create(const char dev_name[], unsigned int port_no,
> - unsigned int *eth_port_id)
> + uint8_t *eth_port_id)
> {
> struct dpdk_ring *ring_pair;
> char *ring_name;
> @@ -2811,7 +2820,7 @@ dpdk_ring_create(const char dev_name[], unsigned int port_no,
> }
>
> static int
> -dpdk_ring_open(const char dev_name[], unsigned int *eth_port_id)
> +dpdk_ring_open(const char dev_name[], uint8_t *eth_port_id)
> OVS_REQUIRES(dpdk_mutex)
> {
> struct dpdk_ring *ring_pair;
> @@ -2860,7 +2869,7 @@ netdev_dpdk_ring_send(struct netdev *netdev, int qid,
> static int
> netdev_dpdk_ring_construct(struct netdev *netdev)
> {
> - unsigned int port_no = 0;
> + uint8_t port_no = 0;
> int err = 0;
>
> ovs_mutex_lock(&dpdk_mutex);
> --
> 2.7.4
>
> _______________________________________________
> dev mailing list
> dev at openvswitch.org
> https://urldefense.proofpoint.com/v2/url?u=https-3A__mail.openvswitch.org_mailman_listinfo_ovs-2Ddev&d=DwICAg&c=uilaK90D4TOVoH58JNXRgQ&r=BVhFA09CGX7JQ5Ih-uZnsw&m=CsrWZo9SNec-_DnojNdU2sRwAPpUXsyy5cTNk5hkhaQ&s=6OANH43fV6QTakoajvWrqyZoezwlGPQF4pLjlhA50ss&e=
>
>
More information about the dev
mailing list