[dpdk-dev] test: Fix memory corruption issues which fails the link_bonding test.
Checks
Commit Message
There were double-free problems in some test cases of link_bonding,
which will cause a duplicated mbuf will be added into mempool. After
double-free, some new allocated mbuf will hold a same address and
thus cause the memory corruption.
Another minor issue is that in some test cases, allocated mbuf will
not be released after test case exits. Hopefully these leaked mbuf
will be released by the next test case in its setup phase when
stopping the virtual pmd ports, while this do is a memory leak of
the exited test case.
To fix above 2 issues, this patch will do:
1) Release virtual pmd ports' tx queue in the clean up function
remove_slaves_and_stop_bonded_device() of each test cases.
2) Do not release allocated mbufs for test bursts. These mbufs
will be released in remove_slaves_and_stop_bonded_device() when
test case exits.
Signed-off-by: Herbert Guan <herbert.guan@arm.com>
---
test/test/test_link_bonding.c | 54 +++----------------------------------------
1 file changed, 3 insertions(+), 51 deletions(-)
Comments
On 10 July 2017 at 15:20, Herbert Guan <herbert.guan@arm.com> wrote:
> There were double-free problems in some test cases of link_bonding,
> which will cause a duplicated mbuf will be added into mempool. After
> double-free, some new allocated mbuf will hold a same address and
> thus cause the memory corruption.
>
> Another minor issue is that in some test cases, allocated mbuf will
> not be released after test case exits. Hopefully these leaked mbuf
> will be released by the next test case in its setup phase when
> stopping the virtual pmd ports, while this do is a memory leak of
> the exited test case.
>
> To fix above 2 issues, this patch will do:
> 1) Release virtual pmd ports' tx queue in the clean up function
> remove_slaves_and_stop_bonded_device() of each test cases.
> 2) Do not release allocated mbufs for test bursts. These mbufs
> will be released in remove_slaves_and_stop_bonded_device() when
> test case exits.
>
> Signed-off-by: Herbert Guan <herbert.guan@arm.com>
> ---
> test/test/test_link_bonding.c | 54 +++----------------------------------------
> 1 file changed, 3 insertions(+), 51 deletions(-)
>
> diff --git a/test/test/test_link_bonding.c b/test/test/test_link_bonding.c
> index aa2a1a2..c4f5c70 100644
> --- a/test/test/test_link_bonding.c
> +++ b/test/test/test_link_bonding.c
> @@ -221,6 +221,8 @@ struct rte_fdir_conf fdir_conf = {
>
> };
>
> +static void free_virtualpmd_tx_queue(void);
> +
> static int
> configure_ethdev(uint8_t port_id, uint8_t start, uint8_t en_isr)
> {
> @@ -684,6 +686,7 @@ struct rte_fdir_conf fdir_conf = {
> remove_slaves_and_stop_bonded_device(void)
> {
> /* Clean up and remove slaves from bonded device */
> + free_virtualpmd_tx_queue();
> while (test_params->bonded_slave_count > 0)
> TEST_ASSERT_SUCCESS(test_remove_slave_from_bonded_device(),
> "test_remove_slave_from_bonded_device failed");
> @@ -1621,9 +1624,6 @@ struct rte_fdir_conf fdir_conf = {
>
> /* free mbufs */
> for (i = 0; i < MAX_PKT_BURST; i++) {
> - if (gen_pkt_burst[i] != NULL)
> - rte_pktmbuf_free(gen_pkt_burst[i]);
> -
> if (rx_pkt_burst[i] != NULL)
> rte_pktmbuf_free(rx_pkt_burst[i]);
> }
> @@ -1970,12 +1970,6 @@ struct rte_fdir_conf fdir_conf = {
> for (i = 0; i < MAX_PKT_BURST; i++) {
> if (rx_pkt_burst[i] != NULL)
> rte_pktmbuf_free(rx_pkt_burst[i]);
> -
> - if (gen_pkt_burst[1][i] != NULL)
> - rte_pktmbuf_free(gen_pkt_burst[1][i]);
> -
> - if (gen_pkt_burst[3][i] != NULL)
> - rte_pktmbuf_free(gen_pkt_burst[1][i]);
> }
>
> /* Clean up and remove slaves from bonded device */
> @@ -2547,16 +2541,6 @@ struct rte_fdir_conf fdir_conf = {
> "(%d) port_stats.opackets not as expected",
> test_params->slave_port_ids[3]);
>
> - /* free mbufs */
> - for (i = 0; i < TEST_ACTIVE_BACKUP_RX_BURST_SLAVE_COUNT; i++) {
> - for (j = 0; j < MAX_PKT_BURST; j++) {
> - if (pkt_burst[i][j] != NULL) {
> - rte_pktmbuf_free(pkt_burst[i][j]);
> - pkt_burst[i][j] = NULL;
> - }
> - }
> - }
> -
> /* Clean up and remove slaves from bonded device */
> return remove_slaves_and_stop_bonded_device();
> }
> @@ -3456,16 +3440,6 @@ struct rte_fdir_conf fdir_conf = {
> test_params->bonded_port_id, (int)port_stats.ipackets,
> burst_size * 3);
>
> - /* free mbufs allocate for rx testing */
> - for (i = 0; i < TEST_BALANCE_RX_BURST_SLAVE_COUNT; i++) {
> - for (j = 0; j < MAX_PKT_BURST; j++) {
> - if (pkt_burst[i][j] != NULL) {
> - rte_pktmbuf_free(pkt_burst[i][j]);
> - pkt_burst[i][j] = NULL;
> - }
> - }
> - }
> -
> /* Clean up and remove slaves from bonded device */
> return remove_slaves_and_stop_bonded_device();
> }
> @@ -3984,16 +3958,6 @@ struct rte_fdir_conf fdir_conf = {
> "(%d) port_stats.ipackets not as expected\n",
> test_params->bonded_port_id);
>
> - /* free mbufs allocate for rx testing */
> - for (i = 0; i < BROADCAST_LINK_STATUS_NUM_OF_SLAVES; i++) {
> - for (j = 0; j < MAX_PKT_BURST; j++) {
> - if (pkt_burst[i][j] != NULL) {
> - rte_pktmbuf_free(pkt_burst[i][j]);
> - pkt_burst[i][j] = NULL;
> - }
> - }
> - }
> -
> /* Clean up and remove slaves from bonded device */
> return remove_slaves_and_stop_bonded_device();
> }
> @@ -4527,18 +4491,6 @@ struct rte_fdir_conf fdir_conf = {
> "(%d) port_stats.ipackets not as expected\n",
> test_params->bonded_port_id);
>
> - /* free mbufs */
> -
> - for (i = 0; i < TEST_ADAPTIVE_TRANSMIT_LOAD_BALANCING_RX_BURST_SLAVE_COUNT; i++) {
> - for (j = 0; j < MAX_PKT_BURST; j++) {
> - if (pkt_burst[i][j] != NULL) {
> - rte_pktmbuf_free(pkt_burst[i][j]);
> - pkt_burst[i][j] = NULL;
> - }
> - }
> - }
> -
> -
> /* Clean up and remove slaves from bonded device */
> return remove_slaves_and_stop_bonded_device();
> }
> --
> 1.8.3.1
>
Acked-by: Jianbo Liu <jianbo.liu@linaro.org>
On 10/07/2017 8:20 AM, Herbert Guan wrote:
> There were double-free problems in some test cases of link_bonding,
> which will cause a duplicated mbuf will be added into mempool. After
> double-free, some new allocated mbuf will hold a same address and
> thus cause the memory corruption.
>
> Another minor issue is that in some test cases, allocated mbuf will
> not be released after test case exits. Hopefully these leaked mbuf
> will be released by the next test case in its setup phase when
> stopping the virtual pmd ports, while this do is a memory leak of
> the exited test case.
>
> To fix above 2 issues, this patch will do:
> 1) Release virtual pmd ports' tx queue in the clean up function
> remove_slaves_and_stop_bonded_device() of each test cases.
> 2) Do not release allocated mbufs for test bursts. These mbufs
> will be released in remove_slaves_and_stop_bonded_device() when
> test case exits.
>
> Signed-off-by: Herbert Guan <herbert.guan@arm.com>
> ---
...
>
Hey Herbert,
I'm seeing compilation warnings for unused variables when I apply this
patch, otherwise these changes look good.
CC test_link_bonding.o
/home/declan/Development/dpdk-org/master/test/test/test_link_bonding.c:2407:9:
error: unused variable 'j'
[-Werror,-Wunused-variable]
int i, j, burst_size, slave_count, primary_port;
^
/home/declan/Development/dpdk-org/master/test/test/test_link_bonding.c:3301:9:
error: unused variable 'j'
[-Werror,-Wunused-variable]
int i, j, burst_size, slave_count;
^
/home/declan/Development/dpdk-org/master/test/test/test_link_bonding.c:3860:9:
error: unused variable 'j'
[-Werror,-Wunused-variable]
int i, j, burst_size, slave_count;
^
/home/declan/Development/dpdk-org/master/test/test/test_link_bonding.c:4372:9:
error: unused variable 'j'
[-Werror,-Wunused-variable]
int i, j, burst_size, slave_count, primary_port;
On 12/07/17 10:59, Declan Doherty wrote:
> On 10/07/2017 8:20 AM, Herbert Guan wrote:
...
>
> Hey Herbert,
>
> I'm seeing compilation warnings for unused variables when I apply this
> patch, otherwise these changes look good.
>
> CC test_link_bonding.o
> /home/declan/Development/dpdk-org/master/test/test/test_link_bonding.c:2407:9:
> error: unused variable 'j'
> [-Werror,-Wunused-variable]
> int i, j, burst_size, slave_count, primary_port;
> ^
> /home/declan/Development/dpdk-org/master/test/test/test_link_bonding.c:3301:9:
> error: unused variable 'j'
> [-Werror,-Wunused-variable]
> int i, j, burst_size, slave_count;
> ^
> /home/declan/Development/dpdk-org/master/test/test/test_link_bonding.c:3860:9:
> error: unused variable 'j'
> [-Werror,-Wunused-variable]
> int i, j, burst_size, slave_count;
> ^
> /home/declan/Development/dpdk-org/master/test/test/test_link_bonding.c:4372:9:
> error: unused variable 'j'
> [-Werror,-Wunused-variable]
> int i, j, burst_size, slave_count, primary_port;
>
Apologies, my outlook mail filter missed your v2 patch, so you can
ignore this.
@@ -221,6 +221,8 @@ struct rte_fdir_conf fdir_conf = {
};
+static void free_virtualpmd_tx_queue(void);
+
static int
configure_ethdev(uint8_t port_id, uint8_t start, uint8_t en_isr)
{
@@ -684,6 +686,7 @@ struct rte_fdir_conf fdir_conf = {
remove_slaves_and_stop_bonded_device(void)
{
/* Clean up and remove slaves from bonded device */
+ free_virtualpmd_tx_queue();
while (test_params->bonded_slave_count > 0)
TEST_ASSERT_SUCCESS(test_remove_slave_from_bonded_device(),
"test_remove_slave_from_bonded_device failed");
@@ -1621,9 +1624,6 @@ struct rte_fdir_conf fdir_conf = {
/* free mbufs */
for (i = 0; i < MAX_PKT_BURST; i++) {
- if (gen_pkt_burst[i] != NULL)
- rte_pktmbuf_free(gen_pkt_burst[i]);
-
if (rx_pkt_burst[i] != NULL)
rte_pktmbuf_free(rx_pkt_burst[i]);
}
@@ -1970,12 +1970,6 @@ struct rte_fdir_conf fdir_conf = {
for (i = 0; i < MAX_PKT_BURST; i++) {
if (rx_pkt_burst[i] != NULL)
rte_pktmbuf_free(rx_pkt_burst[i]);
-
- if (gen_pkt_burst[1][i] != NULL)
- rte_pktmbuf_free(gen_pkt_burst[1][i]);
-
- if (gen_pkt_burst[3][i] != NULL)
- rte_pktmbuf_free(gen_pkt_burst[1][i]);
}
/* Clean up and remove slaves from bonded device */
@@ -2547,16 +2541,6 @@ struct rte_fdir_conf fdir_conf = {
"(%d) port_stats.opackets not as expected",
test_params->slave_port_ids[3]);
- /* free mbufs */
- for (i = 0; i < TEST_ACTIVE_BACKUP_RX_BURST_SLAVE_COUNT; i++) {
- for (j = 0; j < MAX_PKT_BURST; j++) {
- if (pkt_burst[i][j] != NULL) {
- rte_pktmbuf_free(pkt_burst[i][j]);
- pkt_burst[i][j] = NULL;
- }
- }
- }
-
/* Clean up and remove slaves from bonded device */
return remove_slaves_and_stop_bonded_device();
}
@@ -3456,16 +3440,6 @@ struct rte_fdir_conf fdir_conf = {
test_params->bonded_port_id, (int)port_stats.ipackets,
burst_size * 3);
- /* free mbufs allocate for rx testing */
- for (i = 0; i < TEST_BALANCE_RX_BURST_SLAVE_COUNT; i++) {
- for (j = 0; j < MAX_PKT_BURST; j++) {
- if (pkt_burst[i][j] != NULL) {
- rte_pktmbuf_free(pkt_burst[i][j]);
- pkt_burst[i][j] = NULL;
- }
- }
- }
-
/* Clean up and remove slaves from bonded device */
return remove_slaves_and_stop_bonded_device();
}
@@ -3984,16 +3958,6 @@ struct rte_fdir_conf fdir_conf = {
"(%d) port_stats.ipackets not as expected\n",
test_params->bonded_port_id);
- /* free mbufs allocate for rx testing */
- for (i = 0; i < BROADCAST_LINK_STATUS_NUM_OF_SLAVES; i++) {
- for (j = 0; j < MAX_PKT_BURST; j++) {
- if (pkt_burst[i][j] != NULL) {
- rte_pktmbuf_free(pkt_burst[i][j]);
- pkt_burst[i][j] = NULL;
- }
- }
- }
-
/* Clean up and remove slaves from bonded device */
return remove_slaves_and_stop_bonded_device();
}
@@ -4527,18 +4491,6 @@ struct rte_fdir_conf fdir_conf = {
"(%d) port_stats.ipackets not as expected\n",
test_params->bonded_port_id);
- /* free mbufs */
-
- for (i = 0; i < TEST_ADAPTIVE_TRANSMIT_LOAD_BALANCING_RX_BURST_SLAVE_COUNT; i++) {
- for (j = 0; j < MAX_PKT_BURST; j++) {
- if (pkt_burst[i][j] != NULL) {
- rte_pktmbuf_free(pkt_burst[i][j]);
- pkt_burst[i][j] = NULL;
- }
- }
- }
-
-
/* Clean up and remove slaves from bonded device */
return remove_slaves_and_stop_bonded_device();
}