[dpdk-dev] [PATCH 0/3] ethdev: add helper functions to get eth_dev and dev private data

[dpdk-dev] [PATCH 0/3] ethdev: add helper functions to get eth_dev and dev private data

[Ferruh Yigit]

This is to provide abstraction and reduce global variable access.

Global variable rte_eth_devices kept exported to not break ABI.

Bonding driver not selected on purpose, just it seems it is using 
rte_eth_devices heavily.

There are a few more usage in drivers but they left as it is because they
are in fast path code.

Ferruh Yigit (3):
  ethdev: add helper functions to get eth_dev and dev private data
  app/test: use ethdev helper functions
  bonding: use ethdev helper functions

 app/test/test_link_bonding.c              | 35 ++++++++------
 app/test/virtual_pmd.c                    | 51 +++++++++------------
 drivers/net/bonding/rte_eth_bond_8023ad.c | 10 ++--
 drivers/net/bonding/rte_eth_bond_api.c    | 76 +++++++++++++++----------------
 drivers/net/bonding/rte_eth_bond_args.c   | 13 ++++--
 drivers/net/bonding/rte_eth_bond_pmd.c    | 47 +++++++++++--------
 lib/librte_ether/rte_ethdev.h             | 44 ++++++++++++++++++
 7 files changed, 167 insertions(+), 109 deletions(-)

-- 
2.5.0

[dpdk-dev] [PATCH 1/3] ethdev: add helper functions to get eth_dev and dev private data

[Ferruh Yigit]

This is to provide abstraction and reduce global variable access.

Global variable rte_eth_devices kept exported to not break ABI.

Signed-off-by: Ferruh Yigit <ferruh.yigit@intel.com>
---
 lib/librte_ether/rte_ethdev.h | 44 +++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 44 insertions(+)

diff --git a/lib/librte_ether/rte_ethdev.h b/lib/librte_ether/rte_ethdev.h
index 6afb5a9..7209b83 100644
--- a/lib/librte_ether/rte_ethdev.h
+++ b/lib/librte_ether/rte_ethdev.h
@@ -3881,6 +3881,50 @@ rte_eth_dma_zone_reserve(const struct rte_eth_dev *eth_dev, const char *name,
 			 uint16_t queue_id, size_t size,
 			 unsigned align, int socket_id);
 
+/**
+ * Return rte_eth_dev by port id
+ *
+ * This is to abstract device access and limit global variable access.
+ *
+ * @param port_id
+ *   port_id of the device to return.
+ */
+static inline struct rte_eth_dev *
+rte_eth_by_port(uint16_t port_id)
+{
+	return &rte_eth_devices[port_id];
+}
+
+/**
+ * Return rte_eth_dev private data structure by port id
+ *
+ * This is to abstract device private data access and
+ * limit global variable access.
+ *
+ * @param port_id
+ *   port_id of the device to return.
+ */
+static inline void *
+rte_eth_private_by_port(uint16_t port_id)
+{
+	return rte_eth_devices[port_id].data->dev_private;
+}
+
+/**
+ * Return rte_eth_dev private data structure by rte_eth_dev
+ *
+ * This is helper function to access device private data, also
+ * provides some abstraction on how private data kept.
+ *
+ * @param dev
+ *   rte_eth_dev
+ */
+static inline void *
+rte_eth_private_by_dev(struct rte_eth_dev *dev)
+{
+	return dev->data->dev_private;
+}
+
 #ifdef __cplusplus
 }
 #endif
-- 
2.5.0

[dpdk-dev] [PATCH 2/3] app/test: use ethdev helper functions

[Ferruh Yigit]

Use rte_eth_by_port(), rte_eth_private_by_port(),
rte_eth_private_by_dev() helper functions.

Signed-off-by: Ferruh Yigit <ferruh.yigit@intel.com>
---
 app/test/test_link_bonding.c | 35 +++++++++++++++++-------------
 app/test/virtual_pmd.c       | 51 +++++++++++++++++++-------------------------
 2 files changed, 42 insertions(+), 44 deletions(-)

diff --git a/app/test/test_link_bonding.c b/app/test/test_link_bonding.c
index 7cbc289..f2e364b 100644
--- a/app/test/test_link_bonding.c
+++ b/app/test/test_link_bonding.c
@@ -4567,6 +4567,8 @@ test_alb_change_mac_in_reply_sent(void)
 	struct ether_addr bond_mac, client_mac;
 	struct ether_addr *slave_mac1, *slave_mac2;
 
+	struct rte_eth_dev *dev;
+
 	TEST_ASSERT_SUCCESS(
 			initialize_bonded_device_with_slaves(BONDING_MODE_ALB,
 					0, TEST_ALB_SLAVE_COUNT, 1),
@@ -4581,9 +4583,8 @@ test_alb_change_mac_in_reply_sent(void)
 				MAX_PKT_BURST);
 	}
 
-	ether_addr_copy(
-			rte_eth_devices[test_params->bonded_port_id].data->mac_addrs,
-			&bond_mac);
+	dev = rte_eth_by_port(test_params->bonded_port_id);
+	ether_addr_copy(dev->data->mac_addrs, &bond_mac);
 
 	/*
 	 * Generating four packets with different mac and ip addresses and sending
@@ -4629,10 +4630,10 @@ test_alb_change_mac_in_reply_sent(void)
 			ARP_OP_REPLY);
 	rte_eth_tx_burst(test_params->bonded_port_id, 0, &pkt, 1);
 
-	slave_mac1 =
-			rte_eth_devices[test_params->slave_port_ids[0]].data->mac_addrs;
-	slave_mac2 =
-			rte_eth_devices[test_params->slave_port_ids[1]].data->mac_addrs;
+	dev = rte_eth_by_port(test_params->slave_port_ids[0]);
+	slave_mac1 = dev->data->mac_addrs;
+	dev = rte_eth_by_port(test_params->slave_port_ids[1]);
+	slave_mac2 = dev->data->mac_addrs;
 
 	/*
 	 * Checking if packets are properly distributed on bonding ports. Packets
@@ -4681,6 +4682,8 @@ test_alb_reply_from_client(void)
 	struct ether_addr bond_mac, client_mac;
 	struct ether_addr *slave_mac1, *slave_mac2;
 
+	struct rte_eth_dev *dev;
+
 	TEST_ASSERT_SUCCESS(
 			initialize_bonded_device_with_slaves(BONDING_MODE_ALB,
 					0, TEST_ALB_SLAVE_COUNT, 1),
@@ -4694,9 +4697,8 @@ test_alb_reply_from_client(void)
 				MAX_PKT_BURST);
 	}
 
-	ether_addr_copy(
-			rte_eth_devices[test_params->bonded_port_id].data->mac_addrs,
-			&bond_mac);
+	dev = rte_eth_by_port(test_params->bonded_port_id);
+	ether_addr_copy(dev->data->mac_addrs, &bond_mac);
 
 	/*
 	 * Generating four packets with different mac and ip addresses and placing
@@ -4753,8 +4755,10 @@ test_alb_reply_from_client(void)
 	rte_eth_rx_burst(test_params->bonded_port_id, 0, pkts_sent, MAX_PKT_BURST);
 	rte_eth_tx_burst(test_params->bonded_port_id, 0, NULL, 0);
 
-	slave_mac1 = rte_eth_devices[test_params->slave_port_ids[0]].data->mac_addrs;
-	slave_mac2 = rte_eth_devices[test_params->slave_port_ids[1]].data->mac_addrs;
+	dev = rte_eth_by_port(test_params->slave_port_ids[0]);
+	slave_mac1 = dev->data->mac_addrs;
+	dev = rte_eth_by_port(test_params->slave_port_ids[1]);
+	slave_mac2 = dev->data->mac_addrs;
 
 	/*
 	 * Checking if update ARP packets were properly send on slave ports.
@@ -4808,6 +4812,8 @@ test_alb_receive_vlan_reply(void)
 
 	struct ether_addr bond_mac, client_mac;
 
+	struct rte_eth_dev *dev;
+
 	TEST_ASSERT_SUCCESS(
 			initialize_bonded_device_with_slaves(BONDING_MODE_ALB,
 					0, TEST_ALB_SLAVE_COUNT, 1),
@@ -4821,9 +4827,8 @@ test_alb_receive_vlan_reply(void)
 				MAX_PKT_BURST);
 	}
 
-	ether_addr_copy(
-			rte_eth_devices[test_params->bonded_port_id].data->mac_addrs,
-			&bond_mac);
+	dev = rte_eth_by_port(test_params->bonded_port_id);
+	ether_addr_copy(dev->data->mac_addrs, &bond_mac);
 
 	/*
 	 * Generating packet with double VLAN header and placing it in the rx queue.
diff --git a/app/test/virtual_pmd.c b/app/test/virtual_pmd.c
index a538c8a..b1af805 100644
--- a/app/test/virtual_pmd.c
+++ b/app/test/virtual_pmd.c
@@ -263,8 +263,8 @@ static const struct eth_dev_ops virtual_ethdev_default_dev_ops = {
 void
 virtual_ethdev_start_fn_set_success(uint8_t port_id, uint8_t success)
 {
-	struct rte_eth_dev *dev = &rte_eth_devices[port_id];
-	struct virtual_ethdev_private *dev_private = dev->data->dev_private;
+	struct virtual_ethdev_private *dev_private =
+		rte_eth_private_by_port(port_id);
 	struct eth_dev_ops *dev_ops = &dev_private->dev_ops;
 
 	if (success)
@@ -277,8 +277,8 @@ virtual_ethdev_start_fn_set_success(uint8_t port_id, uint8_t success)
 void
 virtual_ethdev_configure_fn_set_success(uint8_t port_id, uint8_t success)
 {
-	struct rte_eth_dev *dev = &rte_eth_devices[port_id];
-	struct virtual_ethdev_private *dev_private = dev->data->dev_private;
+	struct virtual_ethdev_private *dev_private =
+		rte_eth_private_by_port(port_id);
 	struct eth_dev_ops *dev_ops = &dev_private->dev_ops;
 
 	if (success)
@@ -290,8 +290,8 @@ virtual_ethdev_configure_fn_set_success(uint8_t port_id, uint8_t success)
 void
 virtual_ethdev_rx_queue_setup_fn_set_success(uint8_t port_id, uint8_t success)
 {
-	struct rte_eth_dev *dev = &rte_eth_devices[port_id];
-	struct virtual_ethdev_private *dev_private = dev->data->dev_private;
+	struct virtual_ethdev_private *dev_private =
+		rte_eth_private_by_port(port_id);
 	struct eth_dev_ops *dev_ops = &dev_private->dev_ops;
 
 	if (success)
@@ -303,8 +303,8 @@ virtual_ethdev_rx_queue_setup_fn_set_success(uint8_t port_id, uint8_t success)
 void
 virtual_ethdev_tx_queue_setup_fn_set_success(uint8_t port_id, uint8_t success)
 {
-	struct rte_eth_dev *dev = &rte_eth_devices[port_id];
-	struct virtual_ethdev_private *dev_private = dev->data->dev_private;
+	struct virtual_ethdev_private *dev_private =
+		rte_eth_private_by_port(port_id);
 	struct eth_dev_ops *dev_ops = &dev_private->dev_ops;
 
 	if (success)
@@ -316,8 +316,8 @@ virtual_ethdev_tx_queue_setup_fn_set_success(uint8_t port_id, uint8_t success)
 void
 virtual_ethdev_link_update_fn_set_success(uint8_t port_id, uint8_t success)
 {
-	struct rte_eth_dev *dev = &rte_eth_devices[port_id];
-	struct virtual_ethdev_private *dev_private = dev->data->dev_private;
+	struct virtual_ethdev_private *dev_private =
+		rte_eth_private_by_port(port_id);
 	struct eth_dev_ops *dev_ops = &dev_private->dev_ops;
 
 	if (success)
@@ -332,15 +332,13 @@ virtual_ethdev_rx_burst_success(void *queue __rte_unused,
 							 struct rte_mbuf **bufs,
 							 uint16_t nb_pkts)
 {
-	struct rte_eth_dev *vrtl_eth_dev;
 	struct virtual_ethdev_queue *pq_map;
 	struct virtual_ethdev_private *dev_private;
 
 	int rx_count, i;
 
 	pq_map = (struct virtual_ethdev_queue *)queue;
-	vrtl_eth_dev = &rte_eth_devices[pq_map->port_id];
-	dev_private = vrtl_eth_dev->data->dev_private;
+	dev_private = rte_eth_private_by_port(pq_map->port_id);
 
 	rx_count = rte_ring_dequeue_burst(dev_private->rx_queue, (void **) bufs,
 			nb_pkts);
@@ -374,7 +372,7 @@ virtual_ethdev_tx_burst_success(void *queue, struct rte_mbuf **bufs,
 
 	int i;
 
-	vrtl_eth_dev = &rte_eth_devices[tx_q->port_id];
+	vrtl_eth_dev = rte_eth_by_port(tx_q->port_id);
 	dev_private = vrtl_eth_dev->data->dev_private;
 
 	if (!vrtl_eth_dev->data->dev_link.link_status)
@@ -397,15 +395,13 @@ static uint16_t
 virtual_ethdev_tx_burst_fail(void *queue, struct rte_mbuf **bufs,
 		uint16_t nb_pkts)
 {
-	struct rte_eth_dev *vrtl_eth_dev = NULL;
 	struct virtual_ethdev_queue *tx_q = NULL;
 	struct virtual_ethdev_private *dev_private = NULL;
 
 	int i;
 
 	tx_q = (struct virtual_ethdev_queue *)queue;
-	vrtl_eth_dev = &rte_eth_devices[tx_q->port_id];
-	dev_private = vrtl_eth_dev->data->dev_private;
+	dev_private = rte_eth_private_by_port(tx_q->port_id);
 
 	if (dev_private->tx_burst_fail_count < nb_pkts) {
 		int successfully_txd = nb_pkts - dev_private->tx_burst_fail_count;
@@ -432,7 +428,7 @@ virtual_ethdev_tx_burst_fail(void *queue, struct rte_mbuf **bufs,
 void
 virtual_ethdev_rx_burst_fn_set_success(uint8_t port_id, uint8_t success)
 {
-	struct rte_eth_dev *vrtl_eth_dev = &rte_eth_devices[port_id];
+	struct rte_eth_dev *vrtl_eth_dev = rte_eth_by_port(port_id);
 
 	if (success)
 		vrtl_eth_dev->rx_pkt_burst = virtual_ethdev_rx_burst_success;
@@ -445,7 +441,7 @@ void
 virtual_ethdev_tx_burst_fn_set_success(uint8_t port_id, uint8_t success)
 {
 	struct virtual_ethdev_private *dev_private = NULL;
-	struct rte_eth_dev *vrtl_eth_dev = &rte_eth_devices[port_id];
+	struct rte_eth_dev *vrtl_eth_dev = rte_eth_by_port(port_id);
 
 	dev_private = vrtl_eth_dev->data->dev_private;
 
@@ -461,18 +457,17 @@ void
 virtual_ethdev_tx_burst_fn_set_tx_pkt_fail_count(uint8_t port_id,
 		uint8_t packet_fail_count)
 {
-	struct virtual_ethdev_private *dev_private = NULL;
-	struct rte_eth_dev *vrtl_eth_dev = &rte_eth_devices[port_id];
+	struct virtual_ethdev_private *dev_private;
 
 
-	dev_private = vrtl_eth_dev->data->dev_private;
+	dev_private = rte_eth_private_by_port(port_id);
 	dev_private->tx_burst_fail_count = packet_fail_count;
 }
 
 void
 virtual_ethdev_set_link_status(uint8_t port_id, uint8_t link_status)
 {
-	struct rte_eth_dev *vrtl_eth_dev = &rte_eth_devices[port_id];
+	struct rte_eth_dev *vrtl_eth_dev = rte_eth_by_port(port_id);
 
 	vrtl_eth_dev->data->dev_link.link_status = link_status;
 }
@@ -481,7 +476,7 @@ void
 virtual_ethdev_simulate_link_status_interrupt(uint8_t port_id,
 		uint8_t link_status)
 {
-	struct rte_eth_dev *vrtl_eth_dev = &rte_eth_devices[port_id];
+	struct rte_eth_dev *vrtl_eth_dev = rte_eth_by_port(port_id);
 
 	vrtl_eth_dev->data->dev_link.link_status = link_status;
 
@@ -492,9 +487,8 @@ int
 virtual_ethdev_add_mbufs_to_rx_queue(uint8_t port_id,
 		struct rte_mbuf **pkt_burst, int burst_length)
 {
-	struct rte_eth_dev *vrtl_eth_dev = &rte_eth_devices[port_id];
 	struct virtual_ethdev_private *dev_private =
-			vrtl_eth_dev->data->dev_private;
+			rte_eth_private_by_port(port_id);
 
 	return rte_ring_enqueue_burst(dev_private->rx_queue, (void **)pkt_burst,
 			burst_length);
@@ -504,10 +498,9 @@ int
 virtual_ethdev_get_mbufs_from_tx_queue(uint8_t port_id,
 		struct rte_mbuf **pkt_burst, int burst_length)
 {
-	struct virtual_ethdev_private *dev_private;
-	struct rte_eth_dev *vrtl_eth_dev = &rte_eth_devices[port_id];
+	struct virtual_ethdev_private *dev_private =
+			rte_eth_private_by_port(port_id);
 
-	dev_private = vrtl_eth_dev->data->dev_private;
 	return rte_ring_dequeue_burst(dev_private->tx_queue, (void **)pkt_burst,
 		burst_length);
 }
-- 
2.5.0

[dpdk-dev] [PATCH 3/3] bonding: use ethdev helper functions

[Ferruh Yigit]

Use rte_eth_by_port(), rte_eth_private_by_port(),
rte_eth_private_by_dev() helper functions.

Signed-off-by: Ferruh Yigit <ferruh.yigit@intel.com>
---
 drivers/net/bonding/rte_eth_bond_8023ad.c | 10 ++--
 drivers/net/bonding/rte_eth_bond_api.c    | 76 +++++++++++++++----------------
 drivers/net/bonding/rte_eth_bond_args.c   | 13 ++++--
 drivers/net/bonding/rte_eth_bond_pmd.c    | 47 +++++++++++--------
 4 files changed, 81 insertions(+), 65 deletions(-)

diff --git a/drivers/net/bonding/rte_eth_bond_8023ad.c b/drivers/net/bonding/rte_eth_bond_8023ad.c
index b3b30f6..18402c2 100644
--- a/drivers/net/bonding/rte_eth_bond_8023ad.c
+++ b/drivers/net/bonding/rte_eth_bond_8023ad.c
@@ -854,6 +854,7 @@ bond_mode_8023ad_activate_slave(struct rte_eth_dev *bond_dev, uint8_t slave_id)
 	uint32_t total_tx_desc;
 	struct bond_tx_queue *bd_tx_q;
 	uint16_t q_id;
+	struct rte_eth_dev *dev;
 
 	/* Given slave mus not be in active list */
 	RTE_VERIFY(find_slave_by_id(internals->active_slaves,
@@ -882,7 +883,8 @@ bond_mode_8023ad_activate_slave(struct rte_eth_dev *bond_dev, uint8_t slave_id)
 
 	RTE_VERIFY(port->rx_ring == NULL);
 	RTE_VERIFY(port->tx_ring == NULL);
-	socket_id = rte_eth_devices[slave_id].data->numa_node;
+	dev = rte_eth_by_port(slave_id);
+	socket_id = dev->data->numa_node;
 
 	element_size = sizeof(struct slow_protocol_frame) + sizeof(struct rte_mbuf)
 				+ RTE_PKTMBUF_HEADROOM;
@@ -1153,7 +1155,7 @@ rte_eth_bond_8023ad_conf_get(uint8_t port_id,
 	if (conf == NULL)
 		return -EINVAL;
 
-	bond_dev = &rte_eth_devices[port_id];
+	bond_dev = rte_eth_by_port(port_id);
 	bond_mode_8023ad_conf_get(bond_dev, conf);
 	return 0;
 }
@@ -1182,7 +1184,7 @@ rte_eth_bond_8023ad_setup(uint8_t port_id,
 		}
 	}
 
-	bond_dev = &rte_eth_devices[port_id];
+	bond_dev = rte_eth_by_port(port_id);
 	bond_mode_8023ad_setup(bond_dev, conf);
 
 	return 0;
@@ -1200,7 +1202,7 @@ rte_eth_bond_8023ad_slave_info(uint8_t port_id, uint8_t slave_id,
 			rte_eth_bond_mode_get(port_id) != BONDING_MODE_8023AD)
 		return -EINVAL;
 
-	bond_dev = &rte_eth_devices[port_id];
+	bond_dev = rte_eth_by_port(port_id);
 
 	internals = bond_dev->data->dev_private;
 	if (find_slave_by_id(internals->active_slaves,
diff --git a/drivers/net/bonding/rte_eth_bond_api.c b/drivers/net/bonding/rte_eth_bond_api.c
index 8a000c8..c61ad01 100644
--- a/drivers/net/bonding/rte_eth_bond_api.c
+++ b/drivers/net/bonding/rte_eth_bond_api.c
@@ -63,7 +63,7 @@ valid_bonded_port_id(uint8_t port_id)
 	if (!rte_eth_dev_is_valid_port(port_id))
 		return -1;
 
-	return check_for_bonded_ethdev(&rte_eth_devices[port_id]);
+	return check_for_bonded_ethdev(rte_eth_by_port(port_id));
 }
 
 int
@@ -74,7 +74,7 @@ valid_slave_port_id(uint8_t port_id)
 		return -1;
 
 	/* Verify that port_id refers to a non bonded port */
-	if (check_for_bonded_ethdev(&rte_eth_devices[port_id]) == 0)
+	if (check_for_bonded_ethdev(rte_eth_by_port(port_id)) == 0)
 		return -1;
 
 	return 0;
@@ -318,14 +318,14 @@ __eth_bond_slave_add_lock_free(uint8_t bonded_port_id, uint8_t slave_port_id)
 	if (valid_slave_port_id(slave_port_id) != 0)
 		return -1;
 
-	bonded_eth_dev = &rte_eth_devices[bonded_port_id];
+	bonded_eth_dev = rte_eth_by_port(bonded_port_id);
 	internals = bonded_eth_dev->data->dev_private;
 
 	/* Verify that new slave device is not already a slave of another
 	 * bonded device */
 	for (i = rte_eth_dev_count()-1; i >= 0; i--) {
-		if (check_for_bonded_ethdev(&rte_eth_devices[i]) == 0) {
-			temp_internals = rte_eth_devices[i].data->dev_private;
+		if (check_for_bonded_ethdev(rte_eth_by_port(i)) == 0) {
+			temp_internals = rte_eth_private_by_port(i);
 
 			for (j = 0; j < temp_internals->slave_count; j++) {
 				/* Device already a slave of a bonded device */
@@ -338,7 +338,7 @@ __eth_bond_slave_add_lock_free(uint8_t bonded_port_id, uint8_t slave_port_id)
 		}
 	}
 
-	slave_eth_dev = &rte_eth_devices[slave_port_id];
+	slave_eth_dev = rte_eth_by_port(slave_port_id);
 
 	/* Add slave details to bonded device */
 	slave_add(internals, slave_eth_dev);
@@ -437,7 +437,6 @@ __eth_bond_slave_add_lock_free(uint8_t bonded_port_id, uint8_t slave_port_id)
 int
 rte_eth_bond_slave_add(uint8_t bonded_port_id, uint8_t slave_port_id)
 {
-	struct rte_eth_dev *bonded_eth_dev;
 	struct bond_dev_private *internals;
 
 	int retval;
@@ -446,8 +445,7 @@ rte_eth_bond_slave_add(uint8_t bonded_port_id, uint8_t slave_port_id)
 	if (valid_bonded_port_id(bonded_port_id) != 0)
 		return -1;
 
-	bonded_eth_dev = &rte_eth_devices[bonded_port_id];
-	internals = bonded_eth_dev->data->dev_private;
+	internals = rte_eth_private_by_port(bonded_port_id);
 
 	rte_spinlock_lock(&internals->lock);
 
@@ -463,14 +461,15 @@ __eth_bond_slave_remove_lock_free(uint8_t bonded_port_id, uint8_t slave_port_id)
 {
 	struct rte_eth_dev *bonded_eth_dev;
 	struct bond_dev_private *internals;
+	struct rte_eth_dev *dev;
 
 	int i, slave_idx;
 
 	if (valid_slave_port_id(slave_port_id) != 0)
 		return -1;
 
-	bonded_eth_dev = &rte_eth_devices[bonded_port_id];
-	internals = bonded_eth_dev->data->dev_private;
+	bonded_eth_dev = rte_eth_by_port(bonded_port_id);
+	internals = rte_eth_private_by_dev(bonded_eth_dev);
 
 	/* first remove from active slave list */
 	slave_idx = find_slave_by_id(internals->active_slaves,
@@ -495,15 +494,15 @@ __eth_bond_slave_remove_lock_free(uint8_t bonded_port_id, uint8_t slave_port_id)
 
 	/* Un-register link status change callback with bonded device pointer as
 	 * argument*/
+	dev = rte_eth_by_port(bonded_port_id);
 	rte_eth_dev_callback_unregister(slave_port_id, RTE_ETH_EVENT_INTR_LSC,
-			bond_ethdev_lsc_event_callback,
-			&rte_eth_devices[bonded_port_id].data->port_id);
+			bond_ethdev_lsc_event_callback, &dev->data->port_id);
 
 	/* Restore original MAC address of slave device */
-	mac_address_set(&rte_eth_devices[slave_port_id],
+	mac_address_set(rte_eth_by_port(slave_port_id),
 			&(internals->slaves[slave_idx].persisted_mac_addr));
 
-	slave_remove(internals, &rte_eth_devices[slave_port_id]);
+	slave_remove(internals, rte_eth_by_port(slave_port_id));
 
 	/*  first slave in the active list will be the primary by default,
 	 *  otherwise use first device in list */
@@ -518,14 +517,15 @@ __eth_bond_slave_remove_lock_free(uint8_t bonded_port_id, uint8_t slave_port_id)
 
 	if (internals->active_slave_count < 1) {
 		/* reset device link properties as no slaves are active */
-		link_properties_reset(&rte_eth_devices[bonded_port_id]);
+		link_properties_reset(rte_eth_by_port(bonded_port_id));
 
 		/* if no slaves are any longer attached to bonded device and MAC is not
 		 * user defined then clear MAC of bonded device as it will be reset
 		 * when a new slave is added */
+		dev = rte_eth_by_port(bonded_port_id);
 		if (internals->slave_count < 1 && !internals->user_defined_mac)
-			memset(rte_eth_devices[bonded_port_id].data->mac_addrs, 0,
-					sizeof(*(rte_eth_devices[bonded_port_id].data->mac_addrs)));
+			memset(dev->data->mac_addrs, 0,
+					sizeof(*dev->data->mac_addrs));
 	}
 	if (internals->slave_count == 0) {
 		internals->rx_offload_capa = 0;
@@ -539,15 +539,13 @@ __eth_bond_slave_remove_lock_free(uint8_t bonded_port_id, uint8_t slave_port_id)
 int
 rte_eth_bond_slave_remove(uint8_t bonded_port_id, uint8_t slave_port_id)
 {
-	struct rte_eth_dev *bonded_eth_dev;
 	struct bond_dev_private *internals;
 	int retval;
 
 	if (valid_bonded_port_id(bonded_port_id) != 0)
 		return -1;
 
-	bonded_eth_dev = &rte_eth_devices[bonded_port_id];
-	internals = bonded_eth_dev->data->dev_private;
+	internals = rte_eth_private_by_port(bonded_port_id);
 
 	rte_spinlock_lock(&internals->lock);
 
@@ -564,7 +562,7 @@ rte_eth_bond_mode_set(uint8_t bonded_port_id, uint8_t mode)
 	if (valid_bonded_port_id(bonded_port_id) != 0)
 		return -1;
 
-	return bond_ethdev_mode_set(&rte_eth_devices[bonded_port_id], mode);
+	return bond_ethdev_mode_set(rte_eth_by_port(bonded_port_id), mode);
 }
 
 int
@@ -575,7 +573,7 @@ rte_eth_bond_mode_get(uint8_t bonded_port_id)
 	if (valid_bonded_port_id(bonded_port_id) != 0)
 		return -1;
 
-	internals = rte_eth_devices[bonded_port_id].data->dev_private;
+	internals = rte_eth_private_by_port(bonded_port_id);
 
 	return internals->mode;
 }
@@ -591,7 +589,7 @@ rte_eth_bond_primary_set(uint8_t bonded_port_id, uint8_t slave_port_id)
 	if (valid_slave_port_id(slave_port_id) != 0)
 		return -1;
 
-	internals =  rte_eth_devices[bonded_port_id].data->dev_private;
+	internals = rte_eth_private_by_port(bonded_port_id);
 
 	internals->user_defined_primary_port = 1;
 	internals->primary_port = slave_port_id;
@@ -609,7 +607,7 @@ rte_eth_bond_primary_get(uint8_t bonded_port_id)
 	if (valid_bonded_port_id(bonded_port_id) != 0)
 		return -1;
 
-	internals = rte_eth_devices[bonded_port_id].data->dev_private;
+	internals = rte_eth_private_by_port(bonded_port_id);
 
 	if (internals->slave_count < 1)
 		return -1;
@@ -629,7 +627,7 @@ rte_eth_bond_slaves_get(uint8_t bonded_port_id, uint8_t slaves[], uint8_t len)
 	if (slaves == NULL)
 		return -1;
 
-	internals = rte_eth_devices[bonded_port_id].data->dev_private;
+	internals = rte_eth_private_by_port(bonded_port_id);
 
 	if (internals->slave_count > len)
 		return -1;
@@ -652,7 +650,7 @@ rte_eth_bond_active_slaves_get(uint8_t bonded_port_id, uint8_t slaves[],
 	if (slaves == NULL)
 		return -1;
 
-	internals = rte_eth_devices[bonded_port_id].data->dev_private;
+	internals = rte_eth_private_by_port(bonded_port_id);
 
 	if (internals->active_slave_count > len)
 		return -1;
@@ -672,8 +670,8 @@ rte_eth_bond_mac_address_set(uint8_t bonded_port_id,
 	if (valid_bonded_port_id(bonded_port_id) != 0)
 		return -1;
 
-	bonded_eth_dev = &rte_eth_devices[bonded_port_id];
-	internals = bonded_eth_dev->data->dev_private;
+	bonded_eth_dev = rte_eth_by_port(bonded_port_id);
+	internals = rte_eth_private_by_dev(bonded_eth_dev);
 
 	/* Set MAC Address of Bonded Device */
 	if (mac_address_set(bonded_eth_dev, mac_addr))
@@ -697,8 +695,8 @@ rte_eth_bond_mac_address_reset(uint8_t bonded_port_id)
 	if (valid_bonded_port_id(bonded_port_id) != 0)
 		return -1;
 
-	bonded_eth_dev = &rte_eth_devices[bonded_port_id];
-	internals = bonded_eth_dev->data->dev_private;
+	bonded_eth_dev = rte_eth_by_port(bonded_port_id);
+	internals = rte_eth_private_by_dev(bonded_eth_dev);
 
 	internals->user_defined_mac = 0;
 
@@ -725,7 +723,7 @@ rte_eth_bond_xmit_policy_set(uint8_t bonded_port_id, uint8_t policy)
 	if (valid_bonded_port_id(bonded_port_id) != 0)
 		return -1;
 
-	internals = rte_eth_devices[bonded_port_id].data->dev_private;
+	internals = rte_eth_private_by_port(bonded_port_id);
 
 	switch (policy) {
 	case BALANCE_XMIT_POLICY_LAYER2:
@@ -755,7 +753,7 @@ rte_eth_bond_xmit_policy_get(uint8_t bonded_port_id)
 	if (valid_bonded_port_id(bonded_port_id) != 0)
 		return -1;
 
-	internals = rte_eth_devices[bonded_port_id].data->dev_private;
+	internals = rte_eth_private_by_port(bonded_port_id);
 
 	return internals->balance_xmit_policy;
 }
@@ -768,7 +766,7 @@ rte_eth_bond_link_monitoring_set(uint8_t bonded_port_id, uint32_t internal_ms)
 	if (valid_bonded_port_id(bonded_port_id) != 0)
 		return -1;
 
-	internals = rte_eth_devices[bonded_port_id].data->dev_private;
+	internals = rte_eth_private_by_port(bonded_port_id);
 	internals->link_status_polling_interval_ms = internal_ms;
 
 	return 0;
@@ -782,7 +780,7 @@ rte_eth_bond_link_monitoring_get(uint8_t bonded_port_id)
 	if (valid_bonded_port_id(bonded_port_id) != 0)
 		return -1;
 
-	internals = rte_eth_devices[bonded_port_id].data->dev_private;
+	internals = rte_eth_private_by_port(bonded_port_id);
 
 	return internals->link_status_polling_interval_ms;
 }
@@ -796,7 +794,7 @@ rte_eth_bond_link_down_prop_delay_set(uint8_t bonded_port_id, uint32_t delay_ms)
 	if (valid_bonded_port_id(bonded_port_id) != 0)
 		return -1;
 
-	internals = rte_eth_devices[bonded_port_id].data->dev_private;
+	internals = rte_eth_private_by_port(bonded_port_id);
 	internals->link_down_delay_ms = delay_ms;
 
 	return 0;
@@ -810,7 +808,7 @@ rte_eth_bond_link_down_prop_delay_get(uint8_t bonded_port_id)
 	if (valid_bonded_port_id(bonded_port_id) != 0)
 		return -1;
 
-	internals = rte_eth_devices[bonded_port_id].data->dev_private;
+	internals = rte_eth_private_by_port(bonded_port_id);
 
 	return internals->link_down_delay_ms;
 }
@@ -824,7 +822,7 @@ rte_eth_bond_link_up_prop_delay_set(uint8_t bonded_port_id, uint32_t delay_ms)
 	if (valid_bonded_port_id(bonded_port_id) != 0)
 		return -1;
 
-	internals = rte_eth_devices[bonded_port_id].data->dev_private;
+	internals = rte_eth_private_by_port(bonded_port_id);
 	internals->link_up_delay_ms = delay_ms;
 
 	return 0;
@@ -838,7 +836,7 @@ rte_eth_bond_link_up_prop_delay_get(uint8_t bonded_port_id)
 	if (valid_bonded_port_id(bonded_port_id) != 0)
 		return -1;
 
-	internals = rte_eth_devices[bonded_port_id].data->dev_private;
+	internals = rte_eth_private_by_port(bonded_port_id);
 
 	return internals->link_up_delay_ms;
 }
diff --git a/drivers/net/bonding/rte_eth_bond_args.c b/drivers/net/bonding/rte_eth_bond_args.c
index 02ecde6..d461ead 100644
--- a/drivers/net/bonding/rte_eth_bond_args.c
+++ b/drivers/net/bonding/rte_eth_bond_args.c
@@ -55,14 +55,16 @@ static inline int
 find_port_id_by_pci_addr(const struct rte_pci_addr *pci_addr)
 {
 	struct rte_pci_addr *eth_pci_addr;
+	struct rte_eth_dev *dev;
 	unsigned i;
 
 	for (i = 0; i < rte_eth_dev_count(); i++) {
+		dev = rte_eth_by_port(i);
 
-		if (rte_eth_devices[i].pci_dev == NULL)
+		if (dev->pci_dev == NULL)
 			continue;
 
-		eth_pci_addr = &(rte_eth_devices[i].pci_dev->addr);
+		eth_pci_addr = &dev->pci_dev->addr;
 
 		if (pci_addr->bus == eth_pci_addr->bus &&
 			pci_addr->devid == eth_pci_addr->devid &&
@@ -76,13 +78,16 @@ find_port_id_by_pci_addr(const struct rte_pci_addr *pci_addr)
 static inline int
 find_port_id_by_dev_name(const char *name)
 {
+	struct rte_eth_dev *dev;
 	unsigned i;
 
 	for (i = 0; i < rte_eth_dev_count(); i++) {
-		if (rte_eth_devices[i].data == NULL)
+		dev = rte_eth_by_port(i);
+
+		if (dev->data == NULL)
 			continue;
 
-		if (strcmp(rte_eth_devices[i].data->name, name) == 0)
+		if (strcmp(dev->data->name, name) == 0)
 			return i;
 	}
 	return -1;
diff --git a/drivers/net/bonding/rte_eth_bond_pmd.c b/drivers/net/bonding/rte_eth_bond_pmd.c
index b63c886..c316fd7 100644
--- a/drivers/net/bonding/rte_eth_bond_pmd.c
+++ b/drivers/net/bonding/rte_eth_bond_pmd.c
@@ -689,7 +689,7 @@ bond_ethdev_tx_burst_tlb(void *queue, struct rte_mbuf **bufs, uint16_t nb_pkts)
 	struct bond_dev_private *internals = bd_tx_q->dev_private;
 
 	struct rte_eth_dev *primary_port =
-			&rte_eth_devices[internals->primary_port];
+			rte_eth_by_port(internals->primary_port);
 	uint16_t num_tx_total = 0;
 	uint8_t i, j;
 
@@ -1192,7 +1192,9 @@ mac_address_set(struct rte_eth_dev *eth_dev, struct ether_addr *new_mac_addr)
 int
 mac_address_slaves_update(struct rte_eth_dev *bonded_eth_dev)
 {
-	struct bond_dev_private *internals = bonded_eth_dev->data->dev_private;
+	struct bond_dev_private *internals =
+		rte_eth_private_by_dev(bonded_eth_dev);
+	struct rte_eth_dev *dev;
 	int i;
 
 	/* Update slave devices MAC addresses */
@@ -1204,7 +1206,8 @@ mac_address_slaves_update(struct rte_eth_dev *bonded_eth_dev)
 	case BONDING_MODE_BALANCE:
 	case BONDING_MODE_BROADCAST:
 		for (i = 0; i < internals->slave_count; i++) {
-			if (mac_address_set(&rte_eth_devices[internals->slaves[i].port_id],
+			dev = rte_eth_by_port(internals->slaves[i].port_id);
+			if (mac_address_set(dev,
 					bonded_eth_dev->data->mac_addrs)) {
 				RTE_BOND_LOG(ERR, "Failed to update port Id %d MAC address",
 						internals->slaves[i].port_id);
@@ -1222,15 +1225,17 @@ mac_address_slaves_update(struct rte_eth_dev *bonded_eth_dev)
 		for (i = 0; i < internals->slave_count; i++) {
 			if (internals->slaves[i].port_id ==
 					internals->current_primary_port) {
-				if (mac_address_set(&rte_eth_devices[internals->primary_port],
+				dev = rte_eth_by_port(internals->primary_port);
+				if (mac_address_set(dev,
 						bonded_eth_dev->data->mac_addrs)) {
 					RTE_BOND_LOG(ERR, "Failed to update port Id %d MAC address",
 							internals->current_primary_port);
 					return -1;
 				}
 			} else {
-				if (mac_address_set(
-						&rte_eth_devices[internals->slaves[i].port_id],
+				dev = rte_eth_by_port(
+						internals->slaves[i].port_id);
+				if (mac_address_set(dev,
 						&internals->slaves[i].persisted_mac_addr)) {
 					RTE_BOND_LOG(ERR, "Failed to update port Id %d MAC address",
 							internals->slaves[i].port_id);
@@ -1443,6 +1448,7 @@ slave_add(struct bond_dev_private *internals,
 {
 	struct bond_slave_details *slave_details =
 			&internals->slaves[internals->slave_count];
+	struct rte_eth_dev *dev;
 
 	slave_details->port_id = slave_eth_dev->data->port_id;
 	slave_details->last_link_status = 0;
@@ -1454,10 +1460,11 @@ slave_add(struct bond_dev_private *internals,
 
 		if (!internals->link_status_polling_enabled) {
 			internals->link_status_polling_enabled = 1;
+			dev = rte_eth_by_port(internals->port_id);
 
 			rte_eal_alarm_set(internals->link_status_polling_interval_ms * 1000,
 					bond_ethdev_slave_link_status_change_monitor,
-					(void *)&rte_eth_devices[internals->port_id]);
+					(void *)dev);
 		}
 	}
 
@@ -1490,6 +1497,7 @@ static int
 bond_ethdev_start(struct rte_eth_dev *eth_dev)
 {
 	struct bond_dev_private *internals;
+	struct rte_eth_dev *dev;
 	int i;
 
 	/* slave eth dev will be started by bonded device */
@@ -1536,8 +1544,8 @@ bond_ethdev_start(struct rte_eth_dev *eth_dev)
 
 	/* Reconfigure each slave device if starting bonded device */
 	for (i = 0; i < internals->slave_count; i++) {
-		if (slave_configure(eth_dev,
-				&(rte_eth_devices[internals->slaves[i].port_id])) != 0) {
+		dev = rte_eth_by_port(internals->slaves[i].port_id);
+		if (slave_configure(eth_dev, dev) != 0) {
 			RTE_BOND_LOG(ERR,
 					"bonded port (%d) failed to reconfigure slave device (%d)",
 					eth_dev->data->port_id, internals->slaves[i].port_id);
@@ -1730,7 +1738,7 @@ bond_ethdev_slave_link_status_change_monitor(void *cb_arg)
 		return;
 
 	bonded_ethdev = (struct rte_eth_dev *)cb_arg;
-	internals = (struct bond_dev_private *)bonded_ethdev->data->dev_private;
+	internals = rte_eth_private_by_dev(bonded_ethdev);
 
 	if (!bonded_ethdev->data->dev_started ||
 		!internals->link_status_polling_enabled)
@@ -1746,7 +1754,8 @@ bond_ethdev_slave_link_status_change_monitor(void *cb_arg)
 			if (!internals->slaves[i].link_status_poll_enabled)
 				continue;
 
-			slave_ethdev = &rte_eth_devices[internals->slaves[i].port_id];
+			slave_ethdev =
+				rte_eth_by_port(internals->slaves[i].port_id);
 			polling_slave_found = 1;
 
 			/* Update slave link status */
@@ -1778,7 +1787,8 @@ static int
 bond_ethdev_link_update(struct rte_eth_dev *bonded_eth_dev,
 		int wait_to_complete)
 {
-	struct bond_dev_private *internals = bonded_eth_dev->data->dev_private;
+	struct bond_dev_private *internals =
+		rte_eth_private_by_dev(bonded_eth_dev);
 
 	if (!bonded_eth_dev->data->dev_started ||
 		internals->active_slave_count == 0) {
@@ -1789,7 +1799,8 @@ bond_ethdev_link_update(struct rte_eth_dev *bonded_eth_dev,
 		int i, link_up = 0;
 
 		for (i = 0; i < internals->active_slave_count; i++) {
-			slave_eth_dev = &rte_eth_devices[internals->active_slaves[i]];
+			slave_eth_dev =
+				rte_eth_by_port(internals->active_slaves[i]);
 
 			(*slave_eth_dev->dev_ops->link_update)(slave_eth_dev,
 					wait_to_complete);
@@ -1927,13 +1938,13 @@ bond_ethdev_lsc_event_callback(uint8_t port_id, enum rte_eth_event_type type,
 	if (type != RTE_ETH_EVENT_INTR_LSC || param == NULL)
 		return;
 
-	bonded_eth_dev = &rte_eth_devices[*(uint8_t *)param];
-	slave_eth_dev = &rte_eth_devices[port_id];
+	bonded_eth_dev = rte_eth_by_port(*(uint16_t *)param);
+	slave_eth_dev = rte_eth_by_port(port_id);
 
 	if (check_for_bonded_ethdev(bonded_eth_dev))
 		return;
 
-	internals = bonded_eth_dev->data->dev_private;
+	internals = rte_eth_private_by_dev(bonded_eth_dev);
 
 	/* If the device isn't started don't handle interrupts */
 	if (!bonded_eth_dev->data->dev_started)
@@ -2216,7 +2227,7 @@ bond_init(const char *name, const char *params)
 				"socket %u.\n",	name, bonding_mode, socket_id);
 		goto parse_error;
 	}
-	internals = rte_eth_devices[port_id].data->dev_private;
+	internals = rte_eth_private_by_port(port_id);
 	internals->kvlist = kvlist;
 
 	RTE_LOG(INFO, EAL, "Create bonded device %s on port %d in mode %u on "
@@ -2253,7 +2264,7 @@ static int
 bond_ethdev_configure(struct rte_eth_dev *dev)
 {
 	char *name = dev->data->name;
-	struct bond_dev_private *internals = dev->data->dev_private;
+	struct bond_dev_private *internals = rte_eth_private_by_dev(dev);
 	struct rte_kvargs *kvlist = internals->kvlist;
 	int arg_count;
 	uint8_t port_id = dev - rte_eth_devices;
-- 
2.5.0

Re: [dpdk-dev] [PATCH 0/3] ethdev: add helper functions to get eth_dev and dev private data

[Thomas Monjalon]

2016-02-17 14:20, Ferruh Yigit:
> This is to provide abstraction and reduce global variable access.
> 
> Global variable rte_eth_devices kept exported to not break ABI.
> 
> Bonding driver not selected on purpose, just it seems it is using 
> rte_eth_devices heavily.
> 
> There are a few more usage in drivers but they left as it is because they
> are in fast path code.

What is the benefit of these functions if you do not plan to remove the
global variables later?

Anyway this discussion targets the release 16.07.

Re: [dpdk-dev] [PATCH 0/3] ethdev: add helper functions to get eth_dev and dev private data

[Ferruh Yigit]

On 3/10/2016 12:00 AM, Thomas Monjalon wrote:
> 2016-02-17 14:20, Ferruh Yigit:
>> This is to provide abstraction and reduce global variable access.
>>
>> Global variable rte_eth_devices kept exported to not break ABI.
>>
>> Bonding driver not selected on purpose, just it seems it is using 
>> rte_eth_devices heavily.
>>
>> There are a few more usage in drivers but they left as it is because they
>> are in fast path code.
> 
> What is the benefit of these functions if you do not plan to remove the
> global variables later?
> 
- Better discipline to abstract, function call can be costly than direct
access, but I believe this is no problem outside of the fast path.
- Helper functions for common tasks, like get private data by port.

> Anyway this discussion targets the release 16.07.
> 
There is no urgency for these.

Thanks,
ferruh

Re: [dpdk-dev] [PATCH 0/3] ethdev: add helper functions to get eth_dev and dev private data

[Thomas Monjalon]

2016-02-17 14:20, Ferruh Yigit:
> This is to provide abstraction and reduce global variable access.
> 
> Global variable rte_eth_devices kept exported to not break ABI.
> 
> Bonding driver not selected on purpose, just it seems it is using 
> rte_eth_devices heavily.

The struct rte_eth_dev is marked internal.
It is a good goal to remove access to the global array rte_eth_devices,
but the fix must be in the code accessing it only (bonding).

This patchset is rejected.