On 05.10.23 09:42, Simon Horman wrote: > On Wed, Oct 04, 2023 at 02:27:17PM +0200, Jakob Meng wrote: >> On 04.10.23 12:18, Ilya Maximets wrote: >>> On 10/2/23 16:44, David Marchand wrote: >>>> On Mon, Oct 2, 2023 at 1:52 PM Simon Horman <[email protected]> wrote: >>>>> On Wed, Sep 27, 2023 at 03:24:07PM +0200, [email protected] wrote: >>>>>> From: Jakob Meng <[email protected]> >>>>>> >>>>>> For better usability, the function pairs get_config() and >>>>>> set_config() for each netdev should be symmetric: Options which are >>>>>> accepted by set_config() should be returned by get_config() and the >>>>>> latter should output valid options for set_config() only. >>>>>> >>>>>> This patch moves key-value pairs which are no valid options from >>>>>> get_config() to the get_status() callback. For example, get_config() >>>>>> in lib/netdev-dpdk.c returned {configured,requested}_{rx,tx}_queues >>>>>> previously. For requested rx queues the proper option name is n_rxq, >>>>>> so requested_rx_queues has been renamed respectively. Tx queues >>>>>> cannot be changed by the user, hence requested_tx_queues has been >>>>>> dropped. Both configured_{rx,tx}_queues will be returned as >>>>>> n_{r,t}xq in the get_status() callback. >>>>>> >>>>>> The documentation in vswitchd/vswitch.xml for status columns as well >>>>>> as tests have been updated accordingly. >>>>>> >>>>>> Reported-at: https://bugzilla.redhat.com/1949855 >>>>>> Signed-off-by: Jakob Meng <[email protected]> >>>>> Hi Jacob, >>>>> >>>>> I see some CI failures with this change, >>>>> but I'm unsure if they are related to the change or not. >>>>> >>>>> Could you take a look? >>>> I can reproduce issues on vhost-user port mtu tests. >>>> >>>> I understand we have a race now. >>>> >>>> Previously, the test was calling an explicit dpctl/show which >>>> synchronuously invoked each port get_config(), then matching this >>>> command output against mtu=9000. >>>> Now with this patch, the test looks at the interface status field in ovsdb. >>>> My understanding is that this status field may not have been refreshed >>>> *yet* (or there is another issue and I missed some sync point..). >>>> >>>> >>>> Adding a sleep 1 before "ovs-vsctl get Interface dpdkvhostuserclient0 >>>> mtu" does the trick for me... >>>> >>>> I tested adding a check on the vhost-user port link status (see below). >>>> Link status of a vhu port is supposed to be up once the port has been >>>> reconfigured (vhost_reconfigured == which means that >>>> netdev_dpdk_mempool_configure() has been called). >>>> >>>> This seems to work, though I never noticed a "down" status in my runs. >>>> So maybe adding one ovs-vsctl command was enough to put some delay in the >>>> test.. >>>> >>>> $ git diff >>>> diff --git a/tests/system-dpdk.at b/tests/system-dpdk.at >>>> index 95aa3e906b..59fec6903f 100644 >>>> --- a/tests/system-dpdk.at >>>> +++ b/tests/system-dpdk.at >>>> @@ -749,6 +749,7 @@ tail -f /dev/null | dpdk-testpmd >>>> --socket-mem="$(cat NUMA_NODE)" --no-pci\ >>>> --single-file-segments -- -a >>>>> $OVS_RUNDIR/testpmd-dpdkvhostuserclient0.log 2>&1 & >>>> OVS_WAIT_UNTIL([grep "virtio is now ready for processing" >>>> ovs-vswitchd.log]) >>>> +OVS_WAIT_UNTIL([ovs-vsctl get Interface dpdkvhostuserclient0 >>>> link_state | grep -w up]) >>>> >>>> dnl Check MTU value in the datapath >>>> AT_CHECK([ovs-vsctl get Interface dpdkvhostuserclient0 mtu], [0], [dnl >>>> @@ -887,6 +888,7 @@ tail -f /dev/null | dpdk-testpmd >>>> --socket-mem="$(cat NUMA_NODE)" --no-pci\ >>>> --single-file-segments -- -a >>>>> $OVS_RUNDIR/testpmd-dpdkvhostuserclient0.log 2>&1 & >>>> OVS_WAIT_UNTIL([grep "virtio is now ready for processing" >>>> ovs-vswitchd.log]) >>>> +OVS_WAIT_UNTIL([ovs-vsctl get Interface dpdkvhostuserclient0 >>>> link_state | grep -w up]) >>>> >>>> dnl Check MTU value in the datapath >>>> AT_CHECK([ovs-vsctl get Interface dpdkvhostuserclient0 mtu], [0], [dnl >>> Thanks, David. Yeah, there is a slight delay for the database update. >>> Waiting for the port to come 'up' seems reasonable. >>> >>> Best regards, Ilya Maximets. >>> >> Thank you all for your suggestions and David for finding a fix for the DPDK >> tests ☺️ >> >> I have incorporated everything into patch v4 which is out now: >> >> https://patchwork.ozlabs.org/project/openvswitch/patch/[email protected]/ >> > Thanks, I concur that v4 addresses the issues discussed here. >
Yes, v4 applies all of your suggestions: It includes David's fix to wait until ports are up and follows Ilya's suggestions to skip reporting default values for config options, to fix indentation and to drop the docs for dummy ports/datapaths. _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
