> To me this does not read like it is properly validating deferred_start, so I > want to double check this before we merge. > > From the link below, "All device queues (except form deferred start queues) > status should be RTE_ETH_QUEUE_STATE_STARTED after start." > https://doc.dpdk.org/api/rte__ethdev_8h.html#afdc834c1c52e9fb512301990468ca7c2 > > So, I assume that we want to validate: > 1. When deferred_start is set for a queue, the queue does not erroneously > start on testpmd.start() > 2. You can successfully start the queue after the DPDK application is started > and send packets. > > In order to do this. I would write the testcase like: > > with TestPmdShell(node=self.sut_node) as testpmd: > testpmd.set_forward_mode(SimpleForwardingModes.mac) > testpmd.stop_all_ports() > testpmd.set_queue_deferred_start(0, 0, True, True) > testpmd.start() > self.send_packet_and_verify(should_receive=False) > testpmd.start_port_queue(0, 0, True) > self.send_packet_and_verify(should_receive=True) > > What do you think?
I see what you're saying, I can modify the deferred start cases to add this functionality. > Thanks Dean. This mostly looks good to me, but I have a couple > questions/comments. > > 1. I see that you are calling self.verify() twice per testsuite, once through > send_packet_and_verify(), and once by collecting testpmd port stats. So, we > basically have two means of verifying that the ports are behaving as > expected. But, are they essentially verifying the same thing, and are thus > overlapping in responsibilities? If this is the case then we may be adding > complexity for no gain. > > Taking the test_tx_queue_deferred_start testcase as an example, instead of > using send_packet_and_verify(), can you just, directly in the testcase: > > packet = Ether() / IP() / Raw(load="xxxxx") > self.send_packet_and_capture(packet) > > And then check port stats, with that being the sole testcase assertion? Or, > we can do the opposite and drop the port stats assertion, and rely solely on > send_packet_and_capture() and reading the RAW layer, which seems equally as > good. Anyhow perhaps there is some added value from doing both assertions, > and I realize they did that in the legacy DTS testcase, but I figured I'd ask > for your thoughts. Let me know if you agree/disagree. Yes I agree, I was mostly just doing it since the legacy DTS suite did it like you mentioned. I'd be fine with removing the stats check portion though, and just checking whether testpmd receives/doesn't receive the packet. > 2. The way the testcase was written in the legacy framework involved stopping > the tx queue, sending a packet, asserting none was received on that port, > then restarting the queue, then sending a packet and asserting it is received > on the queue. You are not doing the second half of this process in your > testcases - is there a reason why not? Do you think it would be good to add > this? See this blurb from the old test plan: > > #. Run "port 0 txq 0 stop" to stop txq 0 in port 0 > #. Start packet generator to transmit and check tester port not receive > packets > and in testpmd it not has "port 0/queue 0: received 1 packets" print > > #. Run "port 0 txq 0 start" to start txq 0 in port 0 > #. Start packet generator to transmit and check tester port receive packets > and in testpmd it has "port 0/queue 0: received 1 packets" print I honestly don't remember why I took that out, I can definitely add that back it seems like it would be useful. I'll send out a new version ASAP so it's ready to merge by tomorrow, thanks Patrick.
