Wang, Haiyue
2018-12-10 16:59:39 UTC
-----Original Message-----
From: Zhang, Qi Z
Sent: Monday, December 10, 2018 22:08
Subject: RE: [PATCH v1] net/i40e: remove redundant code in free queues.
PMD nb_rx/tx_queues is modified anywhere.
so I'm not very sure if nb_rx/tx_queues should only be modified in
rte_ethdev_rx/tx_queue_config is the original design, and I don't have
confidence those cleanup is always correct, that's why I will prefer an
explanation that prove this code clean is correct. I think your comment is much
fit for a patch set that do all the cleanup.
I searched the " dev->data->nb_rx_queues = 0 ", 10 PMDs drivers write like this, andFrom: Zhang, Qi Z
Sent: Monday, December 10, 2018 22:08
Subject: RE: [PATCH v1] net/i40e: remove redundant code in free queues.
-----Original Message-----
From: Wang, Haiyue
Sent: Monday, December 10, 2018 8:47 PM
Subject: RE: [PATCH v1] net/i40e: remove redundant code in free queues.
How about this commit message ?
net/i40e: queue management data set is out of the API scope.
Function i40e_dev_free_queues is used to free specific i40e queue
entries, but should not be used to set the upper level queue
management data in 'struct rte_eth_dev_data', such as '
nb_rx/tx_queues ', which are maintained by 'rte_eth_dev_rx/tx_queue_config'.
Ideally it should be what you described , but if you look at exist code in differentFrom: Wang, Haiyue
Sent: Monday, December 10, 2018 8:47 PM
Subject: RE: [PATCH v1] net/i40e: remove redundant code in free queues.
-----Original Message-----
From: Zhang, Qi Z
Sent: Monday, December 10, 2018 20:36
Subject: RE: [PATCH v1] net/i40e: remove redundant code in free queues.
comprehensive.From: Zhang, Qi Z
Sent: Monday, December 10, 2018 20:36
Subject: RE: [PATCH v1] net/i40e: remove redundant code in free queues.
-----Original Message-----
From: Yan, Zhirun
Sent: Monday, December 10, 2018 9:32 PM
Subject: [PATCH v1] net/i40e: remove redundant code in free queues.
Before this patch, there are two functions that will clear RX/TX
queues
number: rte_eth_dev_close() and i40e_dev_free_queues().
I agree with what the patch changed, but the commit log is notFrom: Yan, Zhirun
Sent: Monday, December 10, 2018 9:32 PM
Subject: [PATCH v1] net/i40e: remove redundant code in free queues.
Before this patch, there are two functions that will clear RX/TX
queues
number: rte_eth_dev_close() and i40e_dev_free_queues().
I40e_dev_free_queues is not only in the path of rte_eth_dev_close
but also rte_eth_dev_reset.
So
For rte_eth_dev_close, its redundant because of duplication.
For rte_eth_dev_reset its redundant because of not necessary, since
following dev_configure is required after dev_reset and it will be
updated
correctly.but also rte_eth_dev_reset.
So
For rte_eth_dev_close, its redundant because of duplication.
For rte_eth_dev_reset its redundant because of not necessary, since
following dev_configure is required after dev_reset and it will be
updated
How about this commit message ?
net/i40e: queue management data set is out of the API scope.
Function i40e_dev_free_queues is used to free specific i40e queue
entries, but should not be used to set the upper level queue
management data in 'struct rte_eth_dev_data', such as '
nb_rx/tx_queues ', which are maintained by 'rte_eth_dev_rx/tx_queue_config'.
PMD nb_rx/tx_queues is modified anywhere.
so I'm not very sure if nb_rx/tx_queues should only be modified in
rte_ethdev_rx/tx_queue_config is the original design, and I don't have
confidence those cleanup is always correct, that's why I will prefer an
explanation that prove this code clean is correct. I think your comment is much
fit for a patch set that do all the cleanup.
these functions nearly look like the same, like *copy*. :)
Well, from the feature ' VIRTCHNL_OP_REQUEST_QUEUES ', it will cause the VF PMD
internally reset after the application like testpmd calls the ' rte_eth_dev_configure ' API.
The internal reset has to free the internal resource firstly, which will call free functions
such as ' i40e_dev_free_queues ', if this function reset the upper level parameters like
nb_rx/tx_queues from ' rte_eth_dev_configure ', then this standard API will be changed
to be more ugly, like:
' rte_eth_dev_configure '
--> diag = (*dev->dev_ops->dev_configure)(dev);
the PMD's dev_configure needs to be changed the new return value, such as 1 to
tell the ' rte_eth_dev_configure ' reconfigures the nb_rx/tx_queues again, such as
diag = (*dev->dev_ops->dev_configure)(dev);
if (diag == 1) {
rte_eth_dev_rx_queue_config(dev, nb_rx_q);
rte_eth_dev_tx_queue_config(dev, nb_tx_q);
} else if (diag != 0) {
RTE_PMD_DEBUG_TRACE("port%d dev_configure = %d\n",
port_id, diag);
rte_eth_dev_rx_queue_config(dev, 0);
rte_eth_dev_tx_queue_config(dev, 0);
return eth_err(port_id, diag);
}
This makes VIRTCHNL_OP_REQUEST_QUEUES work, but patch will be 100% rejected.
Finally, we found that it we made i40e a good PMD boy, then all will be happy. This is
the long history.
And if ' nb_rx/tx_queues ' stands for the number of TX/RX queues array entries, we'd
better make code looks clean, like an OOP programming: let the rte_eth_xxx API
handle its data.
struct rte_eth_dev_data {
char name[RTE_ETH_NAME_MAX_LEN]; /**< Unique identifier name */
void **rx_queues; /**< Array of pointers to RX queues. */
void **tx_queues; /**< Array of pointers to TX queues. */
uint16_t nb_rx_queues; /**< Number of RX queues. */
uint16_t nb_tx_queues; /**< Number of TX queues. */
And the API 'void rte_eth_dev_close(uint16_t port_id)' will make things clean:
/* old behaviour: only free queue arrays */
dev->data->nb_rx_queues = 0;
rte_free(dev->data->rx_queues);
dev->data->rx_queues = NULL;
dev->data->nb_tx_queues = 0;
rte_free(dev->data->tx_queues);
dev->data->tx_queues = NULL;
This patch remove redundant code in i40e_dev_free_queues().
---
drivers/net/i40e/i40e_rxtx.c | 2 --
1 file changed, 2 deletions(-)
diff --git a/drivers/net/i40e/i40e_rxtx.c
b/drivers/net/i40e/i40e_rxtx.c index
e1152ff0e..cc953ad58 100644
--- a/drivers/net/i40e/i40e_rxtx.c
+++ b/drivers/net/i40e/i40e_rxtx.c
@@ -2753,7 +2753,6 @@ i40e_dev_free_queues(struct rte_eth_dev *dev)
i40e_dev_rx_queue_release(dev->data->rx_queues[i]);
dev->data->rx_queues[i] = NULL;
}
- dev->data->nb_rx_queues = 0;
for (i = 0; i < dev->data->nb_tx_queues; i++) {
if (!dev->data->tx_queues[i])
@@ -2761,7 +2760,6 @@ i40e_dev_free_queues(struct rte_eth_dev *dev)
i40e_dev_tx_queue_release(dev->data->tx_queues[i]);
dev->data->tx_queues[i] = NULL;
}
- dev->data->nb_tx_queues = 0;
}
#define I40E_FDIR_NUM_TX_DESC I40E_MIN_RING_DESC
--
2.17.1
---
drivers/net/i40e/i40e_rxtx.c | 2 --
1 file changed, 2 deletions(-)
diff --git a/drivers/net/i40e/i40e_rxtx.c
b/drivers/net/i40e/i40e_rxtx.c index
e1152ff0e..cc953ad58 100644
--- a/drivers/net/i40e/i40e_rxtx.c
+++ b/drivers/net/i40e/i40e_rxtx.c
@@ -2753,7 +2753,6 @@ i40e_dev_free_queues(struct rte_eth_dev *dev)
i40e_dev_rx_queue_release(dev->data->rx_queues[i]);
dev->data->rx_queues[i] = NULL;
}
- dev->data->nb_rx_queues = 0;
for (i = 0; i < dev->data->nb_tx_queues; i++) {
if (!dev->data->tx_queues[i])
@@ -2761,7 +2760,6 @@ i40e_dev_free_queues(struct rte_eth_dev *dev)
i40e_dev_tx_queue_release(dev->data->tx_queues[i]);
dev->data->tx_queues[i] = NULL;
}
- dev->data->nb_tx_queues = 0;
}
#define I40E_FDIR_NUM_TX_DESC I40E_MIN_RING_DESC
--
2.17.1