Re: [dpdk-dev] [PATCH v5 2/2] ethdev: change queue release callback

On Wed, 2021-10-06 at 09:04 +0100, Ferruh Yigit wrote:

On 10/6/2021 8:55 AM, Xueming(Steven) Li wrote:

quoted

On Tue, 2021-10-05 at 17:38 +0100, Ferruh Yigit wrote:

quoted

On 9/29/2021 2:57 PM, Xueming(Steven) Li wrote:

quoted

On Wed, 2021-09-22 at 12:54 +0000, Xueming(Steven) Li wrote:

quoted

On Wed, 2021-09-22 at 11:57 +0100, Ferruh Yigit wrote:

quoted

quoted

quoted

<...>

quoted

   void
-i40e_dev_rx_queue_release(void *rxq)
+i40e_dev_rx_queue_release(struct rte_eth_dev *dev, uint16_t qid)
+{
+	i40e_rx_queue_release(dev->data->rx_queues[qid]);
+}
+
+void
+i40e_dev_tx_queue_release(struct rte_eth_dev *dev, uint16_t qid)
+{
+	i40e_tx_queue_release(dev->data->tx_queues[qid]);
+}
+

Is there any specific reason to not update driver but add wrappers for it?

Some caller don't have queue ID on hand, adding wrapper seems more
convinient.

Convinient for the patch, but not sure convinient for the driver.

As mentioned before, not sure about approach to update some driver and add
wrappers for some others.

qede, ice and i40e seems not updated, I am for syncronizing with their
maintainers before proceed.

quoted

For qede, qede_tx_queue_release(txq_obj) is called by
qede_alloc_tx_queue_mem(dev, qid), while upper caller
qede_tx_queue_setup() doesn't always save txq_obj to dev->data->txqs[].

For ice and i40e, it's similar, ice_tx_queue_release() is used to free
txq, but some txq isn't saved into dev, please refer to
ice_fdir_setup(), wrapper is needed.

These 3 PMDs create rxq/txq that not saved in dev->data, can't change
parameter to dev+qid for such case, that's why wrapper was there.

Hi Ferruh,

No response from qede, ice and i40e. Basically the original queue
release api is shared by private queues(not registered to ethdev),
can't access by index, that why a warpper was there. To avoid more
rebase in last minute for this big patch, do you think we could close
it?

I see the reason and since there is no update from maintainers, to keep
the ball rolling agree to continue with wrappers, those PMDs can send
incremental patches if required.

quoted

BTW, from feedback from hns3, I will post a new version to add the
macro.

I have concern about this one, how accessing to the global variable
'rte_eth_devices' via a macro improves the situation?

Can you please make wrappers for hns3 driver too, we can follow it later
with driver maintainer?

hns3 doesn't need a wrapper. The macro isn't related to wrapper, just
for the rte_eth_devices access as you suggested:
	&rte_eth_devices[hw->data->port_id]

I suggested not to access global 'rte_eth_devices' variable from driver, and v6 has
a macro in the driver [1] to access the same variable, hiding it behind a macro
is not changing anything.
Since your updates adds more access to 'rte_eth_devices' [2], my suggestion was
do a quick wrapper solution for the driver until it is properly updated.

[1]
#define HNS3_DEV(hw) &rte_eth_devices[(hw)->data->port_id]

[2]
	-	hns3_dev_rx_queue_release(rxq[i]);
	+	hns3_dev_rx_queue_release(HNS3_DEV(hw), i);

Sorry I misunderstood, v7 posted, please check.

Just curios, what's the proper way to access device by port ID? ethdev
is a local struct, saving it in private data struct seems won't make it
work in secondary process, do you think an API should be there to get
device from port ID?