[dpdk-dev] [PATCH v1] net/i40e: remove redundant code in free queues.

Discussion:

(too old to reply)

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.

-----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.

-----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.

-----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 not

comprehensive.

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.
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 different
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, and
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

Zhang, Qi Z

2018-12-11 01:21:32 UTC

Permalink

-----Original Message-----
From: Wang, Haiyue
Sent: Tuesday, December 11, 2018 1:00 AM
Subject: RE: [PATCH v1] net/i40e: remove redundant code in free queues.

-----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.

-----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.

-----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.

I agree with what the patch changed, but the commit log is not

comprehensive.

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

'rte_eth_dev_rx/tx_queue_config'.

Ideally it should be what you described , but if you look at exist
code in different 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,
and these functions nearly look like the same, like *copy*. :)

that's not all the case, if you just search "nb_rx_queues = " you will get more cases that nb_rx_queues is modified by PMD directly.
We should make sure all of them can be cleanup with that way before have our statement, but this is beyond this patch's scope.

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

Yes, that's why I also recommend Zhirun to merge this change to the patch for VIRTCHNL_OP_REQUEST_QUEUES as another option.
But as an independent patch, I think current comments should be OK, because the context here is we have no idea about VIRTCHNL_OP_REQUEST_QUEUES yet.

Again, the words in your comment
".... 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"
I think it is a good design, but I'm not sure if this is considered at original because it is broken everywhere with current implementations.

So from my view, you are introducing a new design and should be covered in a separate patch set which focus on it, but this is not current patch's purpose.

' 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. */
/* 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;

*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++) {
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

Wang, Haiyue

2018-12-11 02:28:09 UTC

Permalink

-----Original Message-----
From: Zhang, Qi Z
Sent: Tuesday, December 11, 2018 09:22
Subject: RE: [PATCH v1] net/i40e: remove redundant code in free queues.

-----Original Message-----
From: Wang, Haiyue
Sent: Tuesday, December 11, 2018 1:00 AM
Subject: RE: [PATCH v1] net/i40e: remove redundant code in free queues.

-----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.

-----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.

-----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.

-----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 not

comprehensive.

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

'rte_eth_dev_rx/tx_queue_config'.

Ideally it should be what you described , but if you look at exist
code in different 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, and these functions nearly look like the same, like *copy*.
:)

that's not all the case, if you just search "nb_rx_queues = " you will get more
cases that nb_rx_queues is modified by PMD directly.
We should make sure all of them can be cleanup with that way before have our
statement, but this is beyond this patch's scope.

I studied again, yes! I took two PMDs (pcap, ring) to understand more:
drivers/net/pcap/rte_eth_pcap.c:961: data->nb_rx_queues = (uint16_t)nb_rx_queues;
drivers/net/ring/rte_eth_ring.c:319: data->nb_rx_queues = (uint16_t)nb_rx_queues;

I found that they used internal queue management data, such as : struct pmd_internals -->
struct ring_queue rx_ring_queues[RTE_PMD_RING_MAX_RX_RINGS]; then the eth_ops
API rx/tx_queue_release have empty functions, like:
static void
eth_queue_release(void *q __rte_unused) { ; }

It seems that the queue data management design is flexible, just let the PMD handles
them carefully, and matches the rte_eth_xxx API's behavior.

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

Yes, that's why I also recommend Zhirun to merge this change to the patch for
VIRTCHNL_OP_REQUEST_QUEUES as another option.
But as an independent patch, I think current comments should be OK, because
the context here is we have no idea about VIRTCHNL_OP_REQUEST_QUEUES
yet.
Again, the words in your comment
".... 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"
I think it is a good design, but I'm not sure if this is considered at original
because it is broken everywhere with current implementations.
So from my view, you are introducing a new design and should be covered in a
separate patch set which focus on it, but this is not current patch's purpose.

' 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
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. */
/* 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;

*dev)