[dpdk-dev] [PATCH] app/testpmd: remove most uses of rte_eth_devices

[dpdk-dev] [PATCH] app/testpmd: remove most uses of rte_eth_devices

[Paulis Gributs]

This patch removes most uses of the global variable rte_eth_devices
from testpmd. This was done to avoid using the object directly which
applications should not do.

Most uses have been replaced with standard function calls, however
the use of it in the show_macs function could not be replaced as no
function call exists to get all mac addresses of a given port.

Signed-off-by: Paulis Gributs <paulis.gributs@intel.com>
---
 app/test-pmd/testpmd.c | 80 +++++++++++++++++++++++++++++-------------
 app/test-pmd/txonly.c  | 18 ++++++++--
 2 files changed, 71 insertions(+), 27 deletions(-)

diff --git a/app/test-pmd/testpmd.c b/app/test-pmd/testpmd.c
index 1cdd3cdd12..42d9804dab 100644
--- a/app/test-pmd/testpmd.c
+++ b/app/test-pmd/testpmd.c
@@ -857,16 +857,23 @@ dma_unmap_cb(struct rte_mempool *mp __rte_unused, void *opaque __rte_unused,
 	int ret;
 
 	RTE_ETH_FOREACH_DEV(pid) {
-		struct rte_eth_dev *dev =
-			&rte_eth_devices[pid];
+		struct rte_eth_dev_info dev_info;
 
-		ret = rte_dev_dma_unmap(dev->device, memhdr->addr, 0,
-					memhdr->len);
+		ret = eth_dev_info_get_print_err(pid, &dev_info);
+		if (ret != 0) {
+			TESTPMD_LOG(DEBUG,
+				    "unable to get device info for port %d on addr 0x%p,"
+				    "mempool unmapping will not be performed\n",
+				    pid, memhdr->addr);
+			continue;
+		}
+
+		ret = rte_dev_dma_unmap(dev_info.device, memhdr->addr, 0, memhdr->len);
 		if (ret) {
 			TESTPMD_LOG(DEBUG,
 				    "unable to DMA unmap addr 0x%p "
 				    "for device %s\n",
-				    memhdr->addr, dev->data->name);
+				    memhdr->addr, dev_info.device->name);
 		}
 	}
 	ret = rte_extmem_unregister(memhdr->addr, memhdr->len);
@@ -892,16 +899,22 @@ dma_map_cb(struct rte_mempool *mp __rte_unused, void *opaque __rte_unused,
 		return;
 	}
 	RTE_ETH_FOREACH_DEV(pid) {
-		struct rte_eth_dev *dev =
-			&rte_eth_devices[pid];
+		struct rte_eth_dev_info dev_info;
 
-		ret = rte_dev_dma_map(dev->device, memhdr->addr, 0,
-				      memhdr->len);
+		ret = eth_dev_info_get_print_err(pid, &dev_info);
+		if (ret != 0) {
+			TESTPMD_LOG(DEBUG,
+				    "unable to get device info for port %d on addr 0x%p,"
+				    "mempool mapping will not be performed\n",
+				    pid, memhdr->addr);
+			continue;
+		}
+		ret = rte_dev_dma_map(dev_info.device, memhdr->addr, 0, memhdr->len);
 		if (ret) {
 			TESTPMD_LOG(DEBUG,
 				    "unable to DMA map addr 0x%p "
 				    "for device %s\n",
-				    memhdr->addr, dev->data->name);
+				    memhdr->addr, dev_info.device->name);
 		}
 	}
 }
@@ -2968,6 +2981,9 @@ detach_device(struct rte_device *dev)
 void
 detach_port_device(portid_t port_id)
 {
+	int ret;
+	struct rte_eth_dev_info dev_info;
+
 	if (port_id_is_invalid(port_id, ENABLED_WARN))
 		return;
 
@@ -2979,7 +2995,14 @@ detach_port_device(portid_t port_id)
 		printf("Port was not closed\n");
 	}
 
-	detach_device(rte_eth_devices[port_id].device);
+	ret = eth_dev_info_get_print_err(port_id, &dev_info);
+	if (ret != 0) {
+		TESTPMD_LOG(ERR,
+			"Failed to get device info for port %d, not detaching\n",
+			port_id);
+		return;
+	}
+	detach_device(dev_info.device);
 }
 
 void
@@ -3160,7 +3183,8 @@ rmv_port_callback(void *arg)
 	int need_to_start = 0;
 	int org_no_link_check = no_link_check;
 	portid_t port_id = (intptr_t)arg;
-	struct rte_device *dev;
+	struct rte_eth_dev_info dev_info;
+	int ret;
 
 	RTE_ETH_VALID_PORTID_OR_RET(port_id);
 
@@ -3172,11 +3196,14 @@ rmv_port_callback(void *arg)
 	stop_port(port_id);
 	no_link_check = org_no_link_check;
 
-	/* Save rte_device pointer before closing ethdev port */
-	dev = rte_eth_devices[port_id].device;
 	close_port(port_id);
-	detach_device(dev); /* might be already removed or have more ports */
-
+	ret = eth_dev_info_get_print_err(port_id, &dev_info);
+	if (ret != 0)
+		TESTPMD_LOG(ERR,
+			"Failed to get device info for port %d, not detaching\n",
+			port_id);
+	else
+		detach_device(dev_info.device); /* might be already removed or have more ports */
 	if (need_to_start)
 		start_packet_forwarding(0);
 }
@@ -3467,13 +3494,9 @@ init_port_config(void)
 		rte_pmd_ixgbe_bypass_init(pid);
 #endif
 
-		if (lsc_interrupt &&
-		    (rte_eth_devices[pid].data->dev_flags &
-		     RTE_ETH_DEV_INTR_LSC))
+		if (lsc_interrupt && (*port->dev_info.dev_flags & RTE_ETH_DEV_INTR_LSC))
 			port->dev_conf.intr_conf.lsc = 1;
-		if (rmv_interrupt &&
-		    (rte_eth_devices[pid].data->dev_flags &
-		     RTE_ETH_DEV_INTR_RMV))
+		if (rmv_interrupt && (*port->dev_info.dev_flags & RTE_ETH_DEV_INTR_RMV))
 			port->dev_conf.intr_conf.rmv = 1;
 	}
 }
@@ -3497,10 +3520,19 @@ void clear_port_slave_flag(portid_t slave_pid)
 uint8_t port_is_bonding_slave(portid_t slave_pid)
 {
 	struct rte_port *port;
+	struct rte_eth_dev_info dev_info;
+	int ret;
 
 	port = &ports[slave_pid];
-	if ((rte_eth_devices[slave_pid].data->dev_flags &
-	    RTE_ETH_DEV_BONDED_SLAVE) || (port->slave_flag == 1))
+	ret = eth_dev_info_get_print_err(slave_pid, &dev_info);
+	if (ret != 0) {
+		TESTPMD_LOG(ERR,
+			"Failed to get device info for port id %d "
+			"cannot determine if the port is a bonded slave",
+			slave_pid);
+		return 0;
+	}
+	if ((*dev_info.dev_flags & RTE_ETH_DEV_BONDED_SLAVE) || (port->slave_flag == 1))
 		return 1;
 	return 0;
 }
diff --git a/app/test-pmd/txonly.c b/app/test-pmd/txonly.c
index d55ee7ca00..3b7f2461bb 100644
--- a/app/test-pmd/txonly.c
+++ b/app/test-pmd/txonly.c
@@ -266,9 +266,21 @@ pkt_burst_prepare(struct rte_mbuf *pkt, struct rte_mempool *mbp,
 
 		if (unlikely(timestamp_init_req !=
 				RTE_PER_LCORE(timestamp_idone))) {
-			struct rte_eth_dev *dev = &rte_eth_devices[fs->tx_port];
-			unsigned int txqs_n = dev->data->nb_tx_queues;
-			uint64_t phase = tx_pkt_times_inter * fs->tx_queue /
+			struct rte_eth_dev_info dev_info;
+			unsigned int txqs_n;
+			uint64_t phase;
+			int ret;
+
+			ret = eth_dev_info_get_print_err(fs->tx_port, &dev_info);
+			if (ret != 0) {
+				TESTPMD_LOG(ERR,
+					"Failed to get device info for port %d "
+					"could not finish timestamp init",
+					fs->tx_port);
+				return false;
+			}
+			txqs_n = dev_info.nb_tx_queues;
+			phase = tx_pkt_times_inter * fs->tx_queue /
 					 (txqs_n ? txqs_n : 1);
 			/*
 			 * Initialize the scheduling time phase shift
-- 
2.25.1

Re: [dpdk-dev] [PATCH] app/testpmd: remove most uses of rte_eth_devices

[Ferruh Yigit]

On 7/15/2021 3:20 PM, Paulis Gributs wrote:
> This patch removes most uses of the global variable rte_eth_devices
> from testpmd. This was done to avoid using the object directly which
> applications should not do.
> 
> Most uses have been replaced with standard function calls, however
> the use of it in the show_macs function could not be replaced as no
> function call exists to get all mac addresses of a given port.
> 
> Signed-off-by: Paulis Gributs <paulis.gributs@intel.com>

Reviewed-by: Ferruh Yigit <ferruh.yigit@intel.com>

+1 to eliminate 'rte_eth_devices' access from application

Re: [dpdk-dev] [PATCH] app/testpmd: remove most uses of rte_eth_devices

[Thomas Monjalon]

15/07/2021 16:52, Ferruh Yigit:
> On 7/15/2021 3:20 PM, Paulis Gributs wrote:
> > This patch removes most uses of the global variable rte_eth_devices
> > from testpmd. This was done to avoid using the object directly which
> > applications should not do.
> > 
> > Most uses have been replaced with standard function calls, however
> > the use of it in the show_macs function could not be replaced as no
> > function call exists to get all mac addresses of a given port.
> > 
> > Signed-off-by: Paulis Gributs <paulis.gributs@intel.com>
> 
> Reviewed-by: Ferruh Yigit <ferruh.yigit@intel.com>
> 
> +1 to eliminate 'rte_eth_devices' access from application

Acked-by: Xiaoyun Li <xiaoyun.li@intel.com>

Applied, thanks that's a good step.
However, I think we should not expose the rte_device pointer at all
as it is done in rte_eth_dev_info.

Re: [dpdk-dev] [PATCH] app/testpmd: remove most uses of rte_eth_devices

[Li, Xiaoyun]

> -----Original Message-----
> From: Gributs, Paulis <paulis.gributs@intel.com>
> Sent: Thursday, July 15, 2021 21:20
> To: Li, Xiaoyun <xiaoyun.li@intel.com>; Yigit, Ferruh <ferruh.yigit@intel.com>;
> Burakov, Anatoly <anatoly.burakov@intel.com>
> Cc: dev@dpdk.org; Gributs, Paulis <paulis.gributs@intel.com>
> Subject: [PATCH] app/testpmd: remove most uses of rte_eth_devices
> 
> This patch removes most uses of the global variable rte_eth_devices from
> testpmd. This was done to avoid using the object directly which applications
> should not do.
> 
> Most uses have been replaced with standard function calls, however the use of
> it in the show_macs function could not be replaced as no function call exists to
> get all mac addresses of a given port.
> 
> Signed-off-by: Paulis Gributs <paulis.gributs@intel.com>
> ---
>  app/test-pmd/testpmd.c | 80 +++++++++++++++++++++++++++++-------------
>  app/test-pmd/txonly.c  | 18 ++++++++--
>  2 files changed, 71 insertions(+), 27 deletions(-)
> 
Acked-by: Xiaoyun Li <xiaoyun.li@intel.com>

Re: [dpdk-dev] [PATCH] app/testpmd: remove most uses of rte_eth_devices

[Ferruh Yigit]

On 7/15/2021 2:20 PM, Paulis Gributs wrote:
> This patch removes most uses of the global variable rte_eth_devices
> from testpmd. This was done to avoid using the object directly which
> applications should not do.
> 
> Most uses have been replaced with standard function calls, however
> the use of it in the show_macs function could not be replaced as no
> function call exists to get all mac addresses of a given port.
> 
> Signed-off-by: Paulis Gributs <paulis.gributs@intel.com>
> ---
>  app/test-pmd/testpmd.c | 80 +++++++++++++++++++++++++++++-------------
>  app/test-pmd/txonly.c  | 18 ++++++++--
>  2 files changed, 71 insertions(+), 27 deletions(-)
> 
> diff --git a/app/test-pmd/testpmd.c b/app/test-pmd/testpmd.c
> index 1cdd3cdd12..42d9804dab 100644
> --- a/app/test-pmd/testpmd.c
> +++ b/app/test-pmd/testpmd.c
> @@ -857,16 +857,23 @@ dma_unmap_cb(struct rte_mempool *mp __rte_unused, void *opaque __rte_unused,
>  	int ret;
>  
>  	RTE_ETH_FOREACH_DEV(pid) {
> -		struct rte_eth_dev *dev =
> -			&rte_eth_devices[pid];
> +		struct rte_eth_dev_info dev_info;
>  
> -		ret = rte_dev_dma_unmap(dev->device, memhdr->addr, 0,
> -					memhdr->len);
> +		ret = eth_dev_info_get_print_err(pid, &dev_info);
> +		if (ret != 0) {
> +			TESTPMD_LOG(DEBUG,
> +				    "unable to get device info for port %d on addr 0x%p,"
> +				    "mempool unmapping will not be performed\n",
> +				    pid, memhdr->addr);
> +			continue;
> +		}
> +
> +		ret = rte_dev_dma_unmap(dev_info.device, memhdr->addr, 0, memhdr->len);
>  		if (ret) {
>  			TESTPMD_LOG(DEBUG,
>  				    "unable to DMA unmap addr 0x%p "
>  				    "for device %s\n",
> -				    memhdr->addr, dev->data->name);
> +				    memhdr->addr, dev_info.device->name);
>  		}
>  	}
>  	ret = rte_extmem_unregister(memhdr->addr, memhdr->len);
> @@ -892,16 +899,22 @@ dma_map_cb(struct rte_mempool *mp __rte_unused, void *opaque __rte_unused,
>  		return;
>  	}
>  	RTE_ETH_FOREACH_DEV(pid) {
> -		struct rte_eth_dev *dev =
> -			&rte_eth_devices[pid];
> +		struct rte_eth_dev_info dev_info;
>  
> -		ret = rte_dev_dma_map(dev->device, memhdr->addr, 0,
> -				      memhdr->len);
> +		ret = eth_dev_info_get_print_err(pid, &dev_info);
> +		if (ret != 0) {
> +			TESTPMD_LOG(DEBUG,
> +				    "unable to get device info for port %d on addr 0x%p,"
> +				    "mempool mapping will not be performed\n",
> +				    pid, memhdr->addr);
> +			continue;
> +		}
> +		ret = rte_dev_dma_map(dev_info.device, memhdr->addr, 0, memhdr->len);
>  		if (ret) {
>  			TESTPMD_LOG(DEBUG,
>  				    "unable to DMA map addr 0x%p "
>  				    "for device %s\n",
> -				    memhdr->addr, dev->data->name);
> +				    memhdr->addr, dev_info.device->name);
>  		}
>  	}
>  }

Hi Shahaf,

These callbacks are used to map/unmap anon memory and added on commit [1].

Can you please elaborate why it is required? And does xmem covers this
functionality already?

The concern I have is, it uses some DPDK details, like rte_device to implement
functionality in a test applications (testpmd). If this is a required
functionality for end user, it is very hard for them to implement this, and
perhaps we should have some APIs/wrappers to help the users in that case.
Or if it is not required, we can perhaps drop from testpmd.

But first I am trying to understand what functionality it brings, if it is
something required by end user or not.

Thanks,
ferruh

[1]
Commit 3a0968c828a6 ("app/testpmd: map anonymous memory for devices")

Re: [dpdk-dev] [PATCH] app/testpmd: remove most uses of rte_eth_devices

[Thomas Monjalon]

19/07/2021 18:42, Ferruh Yigit:
> On 7/15/2021 2:20 PM, Paulis Gributs wrote:
> > This patch removes most uses of the global variable rte_eth_devices
> > from testpmd. This was done to avoid using the object directly which
> > applications should not do.
> > 
> > Most uses have been replaced with standard function calls, however
> > the use of it in the show_macs function could not be replaced as no
> > function call exists to get all mac addresses of a given port.
> > 
> > Signed-off-by: Paulis Gributs <paulis.gributs@intel.com>
[...]
> > @@ -857,16 +857,23 @@ dma_unmap_cb(struct rte_mempool *mp __rte_unused, void *opaque __rte_unused,
> >  	int ret;
> >  
> >  	RTE_ETH_FOREACH_DEV(pid) {
> > -		struct rte_eth_dev *dev =
> > -			&rte_eth_devices[pid];
> > +		struct rte_eth_dev_info dev_info;
> >  
> > -		ret = rte_dev_dma_unmap(dev->device, memhdr->addr, 0,
> > -					memhdr->len);
> > +		ret = eth_dev_info_get_print_err(pid, &dev_info);
> > +		if (ret != 0) {
> > +			TESTPMD_LOG(DEBUG,
> > +				    "unable to get device info for port %d on addr 0x%p,"
> > +				    "mempool unmapping will not be performed\n",
> > +				    pid, memhdr->addr);
> > +			continue;
> > +		}
> > +
> > +		ret = rte_dev_dma_unmap(dev_info.device, memhdr->addr, 0, memhdr->len);
> >  		if (ret) {
> >  			TESTPMD_LOG(DEBUG,
> >  				    "unable to DMA unmap addr 0x%p "
> >  				    "for device %s\n",
> > -				    memhdr->addr, dev->data->name);
> > +				    memhdr->addr, dev_info.device->name);
> >  		}
> >  	}
> >  	ret = rte_extmem_unregister(memhdr->addr, memhdr->len);
> > @@ -892,16 +899,22 @@ dma_map_cb(struct rte_mempool *mp __rte_unused, void *opaque __rte_unused,
> >  		return;
> >  	}
> >  	RTE_ETH_FOREACH_DEV(pid) {
> > -		struct rte_eth_dev *dev =
> > -			&rte_eth_devices[pid];
> > +		struct rte_eth_dev_info dev_info;
> >  
> > -		ret = rte_dev_dma_map(dev->device, memhdr->addr, 0,
> > -				      memhdr->len);
> > +		ret = eth_dev_info_get_print_err(pid, &dev_info);
> > +		if (ret != 0) {
> > +			TESTPMD_LOG(DEBUG,
> > +				    "unable to get device info for port %d on addr 0x%p,"
> > +				    "mempool mapping will not be performed\n",
> > +				    pid, memhdr->addr);
> > +			continue;
> > +		}
> > +		ret = rte_dev_dma_map(dev_info.device, memhdr->addr, 0, memhdr->len);
> >  		if (ret) {
> >  			TESTPMD_LOG(DEBUG,
> >  				    "unable to DMA map addr 0x%p "
> >  				    "for device %s\n",
> > -				    memhdr->addr, dev->data->name);
> > +				    memhdr->addr, dev_info.device->name);
> >  		}
> >  	}
> >  }
> 
> Hi Shahaf,
> 
> These callbacks are used to map/unmap anon memory and added on commit [1].
> 
> Can you please elaborate why it is required? And does xmem covers this
> functionality already?

The external memory must be registered for DMA.
It completes the feature of external memory,
so yes it is required.

> The concern I have is, it uses some DPDK details, like rte_device to implement
> functionality in a test applications (testpmd). If this is a required
> functionality for end user, it is very hard for them to implement this, and
> perhaps we should have some APIs/wrappers to help the users in that case.
> Or if it is not required, we can perhaps drop from testpmd.

I agree the API is bad.
It should be an API in every driver classes.

> But first I am trying to understand what functionality it brings, if it is
> something required by end user or not.

We should deprecate the API and introduce a new one.
Is it urgent to drop the API? Something you would like to do in 21.11?

Re: [dpdk-dev] [PATCH] app/testpmd: remove most uses of rte_eth_devices

[Ferruh Yigit]

On 7/20/2021 11:35 AM, Thomas Monjalon wrote:
> 19/07/2021 18:42, Ferruh Yigit:
>> On 7/15/2021 2:20 PM, Paulis Gributs wrote:
>>> This patch removes most uses of the global variable rte_eth_devices
>>> from testpmd. This was done to avoid using the object directly which
>>> applications should not do.
>>>
>>> Most uses have been replaced with standard function calls, however
>>> the use of it in the show_macs function could not be replaced as no
>>> function call exists to get all mac addresses of a given port.
>>>
>>> Signed-off-by: Paulis Gributs <paulis.gributs@intel.com>
> [...]
>>> @@ -857,16 +857,23 @@ dma_unmap_cb(struct rte_mempool *mp __rte_unused, void *opaque __rte_unused,
>>>  	int ret;
>>>  
>>>  	RTE_ETH_FOREACH_DEV(pid) {
>>> -		struct rte_eth_dev *dev =
>>> -			&rte_eth_devices[pid];
>>> +		struct rte_eth_dev_info dev_info;
>>>  
>>> -		ret = rte_dev_dma_unmap(dev->device, memhdr->addr, 0,
>>> -					memhdr->len);
>>> +		ret = eth_dev_info_get_print_err(pid, &dev_info);
>>> +		if (ret != 0) {
>>> +			TESTPMD_LOG(DEBUG,
>>> +				    "unable to get device info for port %d on addr 0x%p,"
>>> +				    "mempool unmapping will not be performed\n",
>>> +				    pid, memhdr->addr);
>>> +			continue;
>>> +		}
>>> +
>>> +		ret = rte_dev_dma_unmap(dev_info.device, memhdr->addr, 0, memhdr->len);
>>>  		if (ret) {
>>>  			TESTPMD_LOG(DEBUG,
>>>  				    "unable to DMA unmap addr 0x%p "
>>>  				    "for device %s\n",
>>> -				    memhdr->addr, dev->data->name);
>>> +				    memhdr->addr, dev_info.device->name);
>>>  		}
>>>  	}
>>>  	ret = rte_extmem_unregister(memhdr->addr, memhdr->len);
>>> @@ -892,16 +899,22 @@ dma_map_cb(struct rte_mempool *mp __rte_unused, void *opaque __rte_unused,
>>>  		return;
>>>  	}
>>>  	RTE_ETH_FOREACH_DEV(pid) {
>>> -		struct rte_eth_dev *dev =
>>> -			&rte_eth_devices[pid];
>>> +		struct rte_eth_dev_info dev_info;
>>>  
>>> -		ret = rte_dev_dma_map(dev->device, memhdr->addr, 0,
>>> -				      memhdr->len);
>>> +		ret = eth_dev_info_get_print_err(pid, &dev_info);
>>> +		if (ret != 0) {
>>> +			TESTPMD_LOG(DEBUG,
>>> +				    "unable to get device info for port %d on addr 0x%p,"
>>> +				    "mempool mapping will not be performed\n",
>>> +				    pid, memhdr->addr);
>>> +			continue;
>>> +		}
>>> +		ret = rte_dev_dma_map(dev_info.device, memhdr->addr, 0, memhdr->len);
>>>  		if (ret) {
>>>  			TESTPMD_LOG(DEBUG,
>>>  				    "unable to DMA map addr 0x%p "
>>>  				    "for device %s\n",
>>> -				    memhdr->addr, dev->data->name);
>>> +				    memhdr->addr, dev_info.device->name);
>>>  		}
>>>  	}
>>>  }
>>
>> Hi Shahaf,
>>
>> These callbacks are used to map/unmap anon memory and added on commit [1].
>>
>> Can you please elaborate why it is required? And does xmem covers this
>> functionality already?
> 
> The external memory must be registered for DMA.
> It completes the feature of external memory,
> so yes it is required.
> 

External memory has its own parameter (--mp-alloc=xmem) and its own code path.
This is for anonymous memory.

As far as I understand xmem already supports mapping. Above mapping support is
for anonymous memory and it is added before xmem support, to have similar
functionality. But Anatoly & Shahaf know better.

>> The concern I have is, it uses some DPDK details, like rte_device to implement
>> functionality in a test applications (testpmd). If this is a required
>> functionality for end user, it is very hard for them to implement this, and
>> perhaps we should have some APIs/wrappers to help the users in that case.
>> Or if it is not required, we can perhaps drop from testpmd.
> 
> I agree the API is bad.
> It should be an API in every driver classes.
> 
>> But first I am trying to understand what functionality it brings, if it is
>> something required by end user or not.
> 
> We should deprecate the API and introduce a new one.
> Is it urgent to drop the API? Something you would like to do in 21.11?
> 

It is not urgent at all, just trying to clarify and cleanup if needed.
If the xmem covers what this code is trying to to with anon mem, we can drop
this from testpmd.