Hi Sunil, Thanks for taking the time to review, I have left some comments inline.
> -----Original Message----- > From: Pai G, Sunil <[email protected]> > Sent: Saturday 9 July 2022 09:50 > To: Phelan, Michael <[email protected]>; [email protected] > Cc: [email protected]; [email protected] > Subject: RE: [ovs-dev] [PATCH v2] tests: Add OVS-DPDK MTU unit tests. > > Hi Mike, > > Thanks for adding these tests, few comments inline. > > <snipped> > > > +dnl > > +--------------------------------------------------------------------- > > +-- > > +--- > > +dnl MTU increase phy port > > +AT_SETUP([OVS-DPDK - MTU increase phy port]) > > +AT_KEYWORDS([dpdk]) > > + > > +OVS_DPDK_PRE_PHY_SKIP() > > +OVS_DPDK_START() > > + > > +dnl First set MTU to its default value and confirm that value, then > > +increase the MTU value and confirm the new value > > + > > +dnl Add userspace bridge and attach it to OVS with default MTU value > > +AT_CHECK([ovs-vsctl add-br br10 -- set bridge br10 > > +datapath_type=netdev]) AT_CHECK([ovs-vsctl add-port br10 phy0 -- set > > +Interface phy0 type=dpdk options:dpdk-devargs=$(cat PCI_ADDR)], [], > > +[stdout], [stderr]) AT_CHECK([ovs-vsctl show], [], [stdout]) sleep 2 > > + > > +dnl check default MTU value in the datapath AT_CHECK([ovs-appctl > > +dpctl/show], [], [stdout]) AT_CHECK([egrep 'mtu=1500,' stdout], [], > > +[stdout]) > > + > > +dnl increase MTU value and check in the datapath AT_CHECK(ovs-vsctl > > +set Interface phy0 mtu_request=9000) > > To be consistent with the rest of the file, please add "[]" inside AT_CHECK. > (Applies to all test cases in the patch) Good catch, will change this for the next version. > > > + > > +dnl Fail if MTU is not supported > > +AT_FAIL_IF([grep "Interface phy0 does not support MTU configuration" > > +ovs-vswitchd.log], [], [stdout]) > > + > > +dnl Fail if error is encountered during MTU setup AT_FAIL_IF([grep > > +"Interface phy0 MTU (9000) setup error" ovs-vswitchd.log], [], > > +[stdout]) > > + > > +AT_CHECK([ovs-appctl dpctl/show], [], [stdout]) AT_CHECK([egrep > > +'mtu=9000,' stdout], [], [stdout]) > > Do we need to check for the "," above along with mtu=val ? we can probably > drop it. [applies to all test cases in the patch] Good point, no need for the ",". I will remove it in the next version. > > <snipped> > > > + > > +dnl > > +--------------------------------------------------------------------- > > +-- > > +--- > > +dnl MTU increase vport port > > +AT_SETUP([OVS-DPDK - MTU increase vport port]) > > +AT_KEYWORDS([dpdk]) > > + > > +OVS_DPDK_PRE_CHECK() > > +OVS_DPDK_START() > > + > > +dnl Add userspace bridge and attach it to OVS with default MTU value > > +AT_CHECK([ovs-vsctl add-br br10 -- set bridge br10 > > +datapath_type=netdev]) AT_CHECK([ovs-vsctl add-port br10 > > +dpdkvhostuserclient0 -- set Interface dpdkvhostuserclient0 > > +type=dpdkvhostuserclient > > +options:vhost-server-path=$OVS_RUNDIR/dpdkvhostclient0], [], > > +[stdout], > > +[stderr]) AT_CHECK([ovs-vsctl show], [], [stdout]) sleep 2 > > + > > +dnl check default MTU value in the datapath AT_CHECK([ovs-appctl > > +dpctl/show], [], [stdout]) AT_CHECK([egrep 'mtu=1500,' stdout], [], > > +[stdout]) > > + > > +dnl increase MTU value and check in the datapath AT_CHECK(ovs-vsctl > > +set Interface dpdkvhostuserclient0 mtu_request=9000) > > + > > +AT_CHECK([ovs-appctl dpctl/show], [], [stdout]) AT_CHECK([egrep > > +'mtu=9000,' stdout], [], [stdout]) > > + > > + > > +dnl Parse log file > > +AT_CHECK([grep "VHOST_CONFIG: vhost-user client: socket created" > > +ovs-vswitchd.log], [], [stdout]) AT_CHECK([grep "vHost User device > > +'dpdkvhostuserclient0' created in 'client' mode, using client socket" > > +ovs-vswitchd.log], [], [stdout]) AT_CHECK([grep "VHOST_CONFIG: > > +$OVS_RUNDIR/dpdkvhostclient0: reconnecting..." ovs-vswitchd.log], [], > > +[stdout]) > > + > > +dnl Clean up > > +AT_CHECK([ovs-vsctl del-port br10 dpdkvhostuserclient0], [], > > +[stdout], > > +[stderr]) OVS_VSWITCHD_STOP("m4_join([], > [SYSTEM_DPDK_ALLOWED_LOGS], > > +[ > > +\@VHOST_CONFIG: failed to connect to $OVS_RUNDIR/dpdkvhostclient0: > No > > +such file or directory@d > > +])") > > +AT_CLEANUP > > +dnl > > +--------------------------------------------------------------------- > > +-- > > > > Now that the following commit is merged: > > 0dd409c2a netdev-dpdk: Delay vhost mempool creation. > > AFAIK, we would require changing all the vhost related testcases in this patch > a bit to configure the mempool, mtu etc by establishing connection with > vhost front end (virtio-user) using testpmd first. > As an example, for the above test case : "OVS-DPDK - MTU increase vport > port" the diff would be like: Thank you for pointing this out, I will add this change for all the vhost tests in the new version. > > > OVS_DPDK_PRE_CHECK() > OVS_DPDK_START() > > +dnl Find number of sockets > +AT_CHECK([lscpu], [], [stdout]) > +AT_CHECK([cat stdout | grep "NUMA node(s)" | awk '{c=1; while > +(c++<$(3)) {printf "512,"}; print "512"}' > NUMA_NODE]) > + > dnl Add userspace bridge and attach it to OVS with default MTU value > AT_CHECK([ovs-vsctl add-br br10 -- set bridge br10 datapath_type=netdev]) > AT_CHECK([ovs-vsctl add-port br10 dpdkvhostuserclient0 -- set Interface > dpdkvhostuserclient0 type=dpdkvhostuserclient options:vhost-server- > path=$OVS_RUNDIR/dpdkvhostclient0], [], [stdout], [stderr]) > AT_CHECK([ovs-vsctl show], [], [stdout]) sleep 2 > > +dnl Parse log file > +AT_CHECK([grep "VHOST_CONFIG: vhost-user client: socket created" > +ovs-vswitchd.log], [], [stdout]) AT_CHECK([grep "vHost User device > +'dpdkvhostuserclient0' created in 'client' mode, using client socket" > +ovs-vswitchd.log], [], [stdout]) AT_CHECK([grep "VHOST_CONFIG: > +$OVS_RUNDIR/dpdkvhostclient0: reconnecting..." ovs-vswitchd.log], [], > +[stdout]) > + > +dnl Execute testpmd in background > +on_exit "pkill -f -x -9 'tail -f /dev/null'" > +tail -f /dev/null | dpdk-testpmd --socket-mem="$(cat NUMA_NODE)" --no- > pci\ > + -- > vdev="net_virtio_user,path=$OVS_RUNDIR/dpdkvhostclient0,server=1" \ > + --vdev="net_tap0,iface=tap0" --file-prefix page0 \ > + --single-file-segments -- -a > +>$OVS_RUNDIR/testpmd-dpdkvhostuserclient0.log 2>&1 & > + > +OVS_WAIT_UNTIL([grep "virtio is now ready for processing" > +ovs-vswitchd.log]) > + > dnl check default MTU value in the datapath AT_CHECK([ovs-appctl > dpctl/show], [], [stdout]) -AT_CHECK([egrep 'mtu=1500,' stdout], [], > [stdout]) > +AT_CHECK([egrep 'mtu=1500' stdout], [], [stdout]) > > dnl increase MTU value and check in the datapath -AT_CHECK(ovs-vsctl set > Interface dpdkvhostuserclient0 mtu_request=9000) > +AT_CHECK([ovs-vsctl set Interface dpdkvhostuserclient0 > +mtu_request=9000]) > > AT_CHECK([ovs-appctl dpctl/show], [], [stdout]) -AT_CHECK([egrep > 'mtu=9000,' stdout], [], [stdout]) > - > - > -dnl Parse log file > -AT_CHECK([grep "VHOST_CONFIG: vhost-user client: socket created" ovs- > vswitchd.log], [], [stdout]) -AT_CHECK([grep "vHost User device > 'dpdkvhostuserclient0' created in 'client' mode, using client socket" ovs- > vswitchd.log], [], [stdout]) -AT_CHECK([grep "VHOST_CONFIG: > $OVS_RUNDIR/dpdkvhostclient0: reconnecting..." ovs-vswitchd.log], [], > [stdout]) > +AT_CHECK([egrep 'mtu=9000' stdout], [], [stdout]) > > dnl Clean up > AT_CHECK([ovs-vsctl del-port br10 dpdkvhostuserclient0], [], [stdout], > [stderr]) > > > Thanks and regards, > Sunil Kind regards, Michael. _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
