diff mbox series

test/bonding: fix failures after hiding ethdev internal structures

Message ID 20211022132642.30669-1-konstantin.ananyev@intel.com (mailing list archive)
State Accepted, archived
Delegated to: Ferruh Yigit
Headers
Series test/bonding: fix failures after hiding ethdev internal structures |

Checks

Context Check Description
ci/checkpatch success coding style OK
ci/iol-broadcom-Functional success Functional Testing PASS
ci/iol-broadcom-Performance success Performance Testing PASS
ci/github-robot: build success github build: passed
ci/iol-aarch64-compile-testing success Testing PASS
ci/iol-x86_64-unit-testing success Testing PASS
ci/iol-x86_64-compile-testing success Testing PASS
ci/iol-intel-Performance success Performance Testing PASS
ci/iol-intel-Functional success Functional Testing PASS
ci/iol-mellanox-Performance success Performance Testing PASS
ci/Intel-compilation fail Compilation issues
ci/intel-Testing success Testing PASS

Commit Message

Ananyev, Konstantin Oct. 22, 2021, 1:26 p.m. UTC 
  link bounding auto-test internally creates emulated ethdev.
Some tests change Rx/Tx functions of this emulated device on the fly:
by directly modifying rte_eth_dev fields and without doing stop/start
for these devices.
As now ethdev uses rte_eth_fp_ops[] for fast-path functions, these
direct changes doesn't make expected effect.
Fix the problem by guarding fast-path functions changes with
rte_eth_dev_stop()/rte_eth_dev_start().

Fixes: 7a0935239b9e ("ethdev: make fast-path functions to use new flat array")

Reported-by: Lewei Yang <leweix.yang@intel.com>
Signed-off-by: Konstantin Ananyev <konstantin.ananyev@intel.com>
---
 app/test/virtual_pmd.c | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

Comments

Ferruh Yigit Oct. 22, 2021, 1:47 p.m. UTC | #1 
On 10/22/2021 2:26 PM, Konstantin Ananyev wrote:
> link bounding auto-test internally creates emulated ethdev.
> Some tests change Rx/Tx functions of this emulated device on the fly:
> by directly modifying rte_eth_dev fields and without doing stop/start
> for these devices.
> As now ethdev uses rte_eth_fp_ops[] for fast-path functions, these
> direct changes doesn't make expected effect.
> Fix the problem by guarding fast-path functions changes with
> rte_eth_dev_stop()/rte_eth_dev_start().
> 
> Fixes: 7a0935239b9e ("ethdev: make fast-path functions to use new flat array")
> 
> Reported-by: Lewei Yang <leweix.yang@intel.com>
> Signed-off-by: Konstantin Ananyev <konstantin.ananyev@intel.com>

Tested 'link_bonding_autotest', it succeed after this patch (failing before)

Tested-by: Ferruh Yigit <ferruh.yigit@intel.com>
Ferruh Yigit Oct. 22, 2021, 2:02 p.m. UTC | #2 
On 10/22/2021 2:47 PM, Ferruh Yigit wrote:
> On 10/22/2021 2:26 PM, Konstantin Ananyev wrote:
>> link bounding auto-test internally creates emulated ethdev.
>> Some tests change Rx/Tx functions of this emulated device on the fly:
>> by directly modifying rte_eth_dev fields and without doing stop/start
>> for these devices.
>> As now ethdev uses rte_eth_fp_ops[] for fast-path functions, these
>> direct changes doesn't make expected effect.
>> Fix the problem by guarding fast-path functions changes with
>> rte_eth_dev_stop()/rte_eth_dev_start().
>>
>> Fixes: 7a0935239b9e ("ethdev: make fast-path functions to use new flat array")
>>
>> Reported-by: Lewei Yang <leweix.yang@intel.com>
>> Signed-off-by: Konstantin Ananyev <konstantin.ananyev@intel.com>
> 
> Tested 'link_bonding_autotest', it succeed after this patch (failing before)
> 
> Tested-by: Ferruh Yigit <ferruh.yigit@intel.com>
> 

Hi Connor, Chas,

Unrelated with this patch, while I am testing bonding unit test, I recognized
that there are other bonding unit tests failing:
link_bonding_rssconf_autotest
link_bonding_mode4_autotest

They are testing with v21.08 too.
I am not sure if I am missing in my environment, but can you please check
them?

Thanks,
ferruh
Ferruh Yigit Oct. 22, 2021, 4:04 p.m. UTC | #3 
On 10/22/2021 2:47 PM, Ferruh Yigit wrote:
> On 10/22/2021 2:26 PM, Konstantin Ananyev wrote:
>> link bounding auto-test internally creates emulated ethdev.
>> Some tests change Rx/Tx functions of this emulated device on the fly:
>> by directly modifying rte_eth_dev fields and without doing stop/start
>> for these devices.
>> As now ethdev uses rte_eth_fp_ops[] for fast-path functions, these
>> direct changes doesn't make expected effect.
>> Fix the problem by guarding fast-path functions changes with
>> rte_eth_dev_stop()/rte_eth_dev_start().
>>
>> Fixes: 7a0935239b9e ("ethdev: make fast-path functions to use new flat array")
>>
>> Reported-by: Lewei Yang <leweix.yang@intel.com>
>> Signed-off-by: Konstantin Ananyev <konstantin.ananyev@intel.com>
> 
> Tested 'link_bonding_autotest', it succeed after this patch (failing before)
> 
> Tested-by: Ferruh Yigit <ferruh.yigit@intel.com>
> 

Applied to dpdk-next-net/main, thanks.
David Marchand Oct. 24, 2021, 10:39 a.m. UTC | #4 
Hello guys,

On Fri, Oct 22, 2021 at 3:27 PM Konstantin Ananyev
<konstantin.ananyev@intel.com> wrote:
>
> link bounding auto-test internally creates emulated ethdev.
> Some tests change Rx/Tx functions of this emulated device on the fly:
> by directly modifying rte_eth_dev fields and without doing stop/start
> for these devices.
> As now ethdev uses rte_eth_fp_ops[] for fast-path functions, these
> direct changes doesn't make expected effect.
> Fix the problem by guarding fast-path functions changes with
> rte_eth_dev_stop()/rte_eth_dev_start().
>
> Fixes: 7a0935239b9e ("ethdev: make fast-path functions to use new flat array")
>
> Reported-by: Lewei Yang <leweix.yang@intel.com>
> Signed-off-by: Konstantin Ananyev <konstantin.ananyev@intel.com>

I suspect there is something broken in
(bitratestats_autotest|latencystats_autotest|pdump_autotest) tests
that make use of app/test/sample_packet_forward.* code.
This code creates a portid from a ring, but I guess this port is not
properly setup wrt to API.

It goes unnoticed in CI atm, with tests returning ok.


Looking at main branch test in UNH, testlog.txt files are millions of
lines filled with:

test_packet_forward() line 104: Error sending packet to port 0
send pkts Failed

or:

tx_pkt_burst for not ready port

There may be more issues, the files are just too huge.


Example:
https://lab.dpdk.org/results/dashboard/results/results-uploads/test_runs/1dd418abe77644c18417dcb589e994aa/log_upload_file/2021/10/dpdk_2e437f1cd65e_2021-10-23_22-04-34_NA.zip
Ananyev, Konstantin Oct. 24, 2021, 11:37 a.m. UTC | #5 
Hi David,

> Hello guys,
> 
> On Fri, Oct 22, 2021 at 3:27 PM Konstantin Ananyev
> <konstantin.ananyev@intel.com> wrote:
> >
> > link bounding auto-test internally creates emulated ethdev.
> > Some tests change Rx/Tx functions of this emulated device on the fly:
> > by directly modifying rte_eth_dev fields and without doing stop/start
> > for these devices.
> > As now ethdev uses rte_eth_fp_ops[] for fast-path functions, these
> > direct changes doesn't make expected effect.
> > Fix the problem by guarding fast-path functions changes with
> > rte_eth_dev_stop()/rte_eth_dev_start().
> >
> > Fixes: 7a0935239b9e ("ethdev: make fast-path functions to use new flat array")
> >
> > Reported-by: Lewei Yang <leweix.yang@intel.com>
> > Signed-off-by: Konstantin Ananyev <konstantin.ananyev@intel.com>
> 
> I suspect there is something broken in
> (bitratestats_autotest|latencystats_autotest|pdump_autotest) tests
> that make use of app/test/sample_packet_forward.* code.
> This code creates a portid from a ring, but I guess this port is not
> properly setup wrt to API.
> It goes unnoticed in CI atm, with tests returning ok.
> 
> 
> Looking at main branch test in UNH, testlog.txt files are millions of
> lines filled with:
> 
> test_packet_forward() line 104: Error sending packet to port 0
> send pkts Failed
> 
> or:
> 
> tx_pkt_burst for not ready port
> 
> There may be more issues, the files are just too huge.
> 
> 
> Example:
> https://lab.dpdk.org/results/dashboard/results/results-
> uploads/test_runs/1dd418abe77644c18417dcb589e994aa/log_upload_file/2021/10/dpdk_2e437f1cd65e_2021-10-23_22-04-34_NA.zip
> 

Sure, I'll have a look next week.
Konstantin
diff mbox series

Patch

diff --git a/app/test/virtual_pmd.c b/app/test/virtual_pmd.c
index 7e15b47eb0..ccdb418490 100644
--- a/app/test/virtual_pmd.c
+++ b/app/test/virtual_pmd.c
@@ -415,10 +415,14 @@  virtual_ethdev_rx_burst_fn_set_success(uint16_t port_id, uint8_t success)
 {
 	struct rte_eth_dev *vrtl_eth_dev = &rte_eth_devices[port_id];
 
+	rte_eth_dev_stop(port_id);
+
 	if (success)
 		vrtl_eth_dev->rx_pkt_burst = virtual_ethdev_rx_burst_success;
 	else
 		vrtl_eth_dev->rx_pkt_burst = virtual_ethdev_rx_burst_fail;
+
+	rte_eth_dev_start(port_id);
 }
 
 
@@ -428,6 +432,7 @@  virtual_ethdev_tx_burst_fn_set_success(uint16_t port_id, uint8_t success)
 	struct virtual_ethdev_private *dev_private = NULL;
 	struct rte_eth_dev *vrtl_eth_dev = &rte_eth_devices[port_id];
 
+	rte_eth_dev_stop(port_id);
 	dev_private = vrtl_eth_dev->data->dev_private;
 
 	if (success)
@@ -436,6 +441,7 @@  virtual_ethdev_tx_burst_fn_set_success(uint16_t port_id, uint8_t success)
 		vrtl_eth_dev->tx_pkt_burst = virtual_ethdev_tx_burst_fail;
 
 	dev_private->tx_burst_fail_count = 0;
+	rte_eth_dev_start(port_id);
 }
 
 void
@@ -445,7 +451,6 @@  virtual_ethdev_tx_burst_fn_set_tx_pkt_fail_count(uint16_t port_id,
 	struct virtual_ethdev_private *dev_private = NULL;
 	struct rte_eth_dev *vrtl_eth_dev = &rte_eth_devices[port_id];
 
-
 	dev_private = vrtl_eth_dev->data->dev_private;
 	dev_private->tx_burst_fail_count = packet_fail_count;
 }