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

[Ferruh Yigit <ferruh.yigit@intel.com>]

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.