Re: [dts] [PATCH V1] test: add vhost/virtio pvp performance test based on qemu

["Liu, Yong" <yong.liu@intel.com>]

Thanks Lei, some comments below.

> -----Original Message-----
> From: dts [mailto:dts-bounces@dpdk.org] On Behalf Of lei,yao
> Sent: Tuesday, December 20, 2016 2:08 PM
> To: dts@dpdk.org
> Cc: Yao, Lei A
> Subject: [dts] [PATCH V1] test: add vhost/virtio pvp performance test
> based on qemu
> 
> From: lei yao <lei.a.yao@intel.com>
> 
> The test case include 3 part:
> 1. virtio 0.95 performance test
> virtio0.95 is the default version for qemu. Vector, normal, mergeable path
> will be tested on this version
> 2. Test vector performance on differnt qemu version. For exapmle: qemu2.5,
> qemu2.6, qemu2.7
> 3. Test vector performance on virtio 1.0.
> 
> Signed-off-by: lei yao <lei.a.yao@intel.com>
> ---
>  tests/TestSuite_vhost_qemu_pvp_performance.py | 471
> ++++++++++++++++++++++++++
>  1 file changed, 471 insertions(+)
>  create mode 100644 tests/TestSuite_vhost_qemu_pvp_performance.py
> 
> diff --git a/tests/TestSuite_vhost_qemu_pvp_performance.py
> b/tests/TestSuite_vhost_qemu_pvp_performance.py
> new file mode 100644
> index 0000000..ced777f
> --- /dev/null
> +++ b/tests/TestSuite_vhost_qemu_pvp_performance.py
> @@ -0,0 +1,471 @@
> +# BSD LICENSE
> +#
> +# Copyright(c) 2010-2015 Intel Corporation. All rights reserved.
> +# All rights reserved.
> +#
> +# Redistribution and use in source and binary forms, with or without
> +# modification, are permitted provided that the following conditions
> +# are met:
> +#
> +#   * Redistributions of source code must retain the above copyright
> +#     notice, this list of conditions and the following disclaimer.
> +#   * Redistributions in binary form must reproduce the above copyright
> +#     notice, this list of conditions and the following disclaimer in
> +#     the documentation and/or other materials provided with the
> +#     distribution.
> +#   * Neither the name of Intel Corporation nor the names of its
> +#     contributors may be used to endorse or promote products derived
> +#     from this software without specific prior written permission.
> +#
> +# THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDERS AND CONTRIBUTORS
> +# "AS IS" AND ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT
> +# LIMITED TO, THE IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR
> +# A PARTICULAR PURPOSE ARE DISCLAIMED. IN NO EVENT SHALL THE COPYRIGHT
> +# OWNER OR CONTRIBUTORS BE LIABLE FOR ANY DIRECT, INDIRECT, INCIDENTAL,
> +# SPECIAL, EXEMPLARY, OR CONSEQUENTIAL DAMAGES (INCLUDING, BUT NOT
> +# LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS OR SERVICES; LOSS OF USE,
> +# DATA, OR PROFITS; OR BUSINESS INTERRUPTION) HOWEVER CAUSED AND ON ANY
> +# THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY, OR TORT
> +# (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE USE
> +# OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.
> +
> +"""
> +DPDK Test suite.
> +
> +Vhost PVP performance using Qemu test suite.
> +"""
> +import os
> +import dts

Please remove dependency on dts module, we tried to make test suite more independent. 

> +import string
> +import re
> +import time
> +import utils
> +from scapy.utils import wrpcap, rdpcap
> +from test_case import TestCase
> +from exception import VerifyFailure
> +from settings import HEADER_SIZE
> +from etgen import IxiaPacketGenerator
> +from qemu_kvm import QEMUKvm
> +
> +
> +class TestVhostUserOneCopyOneVm(TestCase, IxiaPacketGenerator):
> +
If you do not want to modify internal function of IxiaPacketGenerator, there's no need to inherit this module.

> +    def set_up_all(self):
> +        # To Extend IXIA packet generator method, call the tester's
> method.
> +
> self.tester.extend_external_packet_generator(TestVhostUserOneCopyOneVm,
> self)

Same comment as previous. 
> +
> +
> +        # Get and verify the ports
> +        self.dut_ports = self.dut.get_ports()
> +        self.verify(len(self.dut_ports) >= 1, "Insufficient ports for
> testing")
> +
> +        # Get the port's socket
> +        self.pf = self.dut_ports[0]
> +        netdev = self.dut.ports_info[self.pf]['port']
> +        self.socket = netdev.get_nic_socket()
> +        self.cores = self.dut.get_core_list("1S/3C/1T",
> socket=self.socket)
> +
> +        # Set the params of vhost sample
> +        self.vhost_app = "./examples/vhost/build/vhost-switch"
> +        self.vm2vm = 0
> +        # This parameter is used to define mergeable on/off
> +        self.jumbo = 0
> +
> +        # Using file to save the vhost sample output since in jumboframe
> case,
> +        # there will be lots of output
> +        self.vhost_test = self.vhost_app + \
> +            " -c %s -n %d --socket-mem 2048,2048 -- -p 0x1 --
> mergeable %d" + \
> +            " --vm2vm %d --socket-file ./vhost-net  > ./vhost.out &"

I'm not sure whether each platform can support two sockets. If there's no special requirement, please just remove socket-mem option.

> +        # build the vhost sample in vhost-user mode.
> +        if self.nic in ['niantic']:
> +            self.dut.send_expect(
> +                "sed -i -e 's/#define MAX_QUEUES.*$/#define MAX_QUEUES
> 128/' "
> +                "./examples/vhost/main.c",
> +                "#", 10)
> +        else:
> +            self.dut.send_expect(
> +                "sed -i -e 's/#define MAX_QUEUES.*$/#define MAX_QUEUES
> 512/' "
> +                "./examples/vhost/main.c",
> +                "#", 10)

Queue numbers are different between different kinds of NICS. If not sure with the number, please left it as default.
I guess the logic should be like below:

if self.nic in ['niantic']:
    ...
else if self.nic.startwith('fortville')
    ...


> +        out = self.dut.send_expect("make -C examples/vhost", "#")
> +        self.verify("Error" not in out, "compilation error")
> +        self.verify("No such file" not in out, "Not found file error")
> +
> +        self.virtio1 = "eth1"
> +        self.virtio1_mac = "52:54:00:00:00:01"
> +        self.src1 = "192.168.4.1"
> +        self.dst1 = "192.168.3.1"
> +        self.vm_dut = None
> +
> +        self.number_of_ports = 1
> +        self.header_row = ["FrameSize(B)", "Throughput(Mpps)",
> "LineRate(%)","Cycle"]
> +        self.memory_channel = 4
> +	# Specify the path of Qemu binary for different Qemu versions, user
> can modify this as necessary
> +        self.qemu_path=["/home/lei/qemu_install/qemu_2.5/bin/qemu-system-
> x86_64",\
> +                    "/home/lei/qemu_install/qemu_2.6/bin/qemu-system-
> x86_64",\
> +                    "/home/lei/qemu_install/qemu_2.7/bin/qemu-system-
> x86_64"]
> +
It's not good to add your local path into the code. You can use configuration file to work around it.
Also has question about why we need to check different versions of qemu. It's one time task or every time we need to do that.
If just for one time, you can change configuration for qemu path and rerun those cases.

> +
> +    def set_up(self):
> +        #
> +        # Run before each test case.
> +        #
> +        # Launch vhost sample using default params
> +        #
> +	self.dut.send_expect("rm -rf ./vhost.out", "#")
> +	self.dut.send_expect("rm -rf ./vhost-net*", "#")
> +	self.dut.send_expect("killall -s INT vhost-switch", "#")
> +	self.dut.send_expect("killall -s INT qemu-system-x86_64", "#")
> +
Please make sure code aligned, you can use pep8 check to help on that.

> +        self.frame_sizes = [64, 128, 256, 512, 1024, 1500]
> +        self.vm_testpmd_vector = "./x86_64-native-linuxapp-
> gcc/app/testpmd -c 0x3 -n 3" \
> +                + " -- -i --txqflags=0xf01 --disable-hw-vlan-filter"
> +        self.vm_testpmd_normal = "./x86_64-native-linuxapp-
> gcc/app/testpmd -c 0x3 -n 3" \
> +                + " -- -i --txqflags=0xf00 --disable-hw-vlan-filter"
> +        self.dut.send_expect("rm -rf ./vhost.out", "#")
> +
> +
> +    def launch_vhost_sample(self):
> +        #
> +        # Launch the vhost sample with different parameters
> +        #
> +        self.coremask = utils.create_mask(self.cores)
> +        self.vhostapp_testcmd = self.vhost_test % (
> +            self.coremask, self.memory_channel, self.jumbo, self.vm2vm)
> +        self.dut.send_expect(self.vhostapp_testcmd, "# ", 40)
> +        time.sleep(30)
> +        try:
> +            print "Launch vhost sample:"
It's better to replace print with self.logger.info function.

> +            self.dut.session.copy_file_from("/root/dpdk/vhost.out")

Dpdk folder may be changed by command line, you can use self.dut.base_dir + "vhost.out".

> +            fp = open('./vhost.out', 'r')
> +            out = fp.read()
> +            fp.close()
> +            if "Error" in out:
> +                raise Exception("Launch vhost sample failed")
> +            else:
> +                print "Launch vhost sample finished"
> +        except Exception as e:
> +            print utils.RED("Failed to launch vhost sample: %s" % str(e))
> +
It's better to replace print with self.logger.error function.
> +
> +
> +    def start_onevm(self, path="", modem=0):
> +        #
> +        # Start One VM with one virtio device
> +        #
> +
> +        self.vm = QEMUKvm(self.dut, 'vm0', 'vhost_sample')
> +        if(path!=""):
> +            self.vm.set_qemu_emulator(path)
> +        vm_params = {}
> +        vm_params['driver'] = 'vhost-user'
> +        vm_params['opt_path'] = './vhost-net'
> +        vm_params['opt_mac'] = self.virtio1_mac
> +        if(modem == 1):
> +            vm_params['opt_settings'] = 'disable-modern=false'
> +        self.vm.set_vm_device(**vm_params)
> +
> +        try:
> +            self.vm_dut = self.vm.start()
> +            if self.vm_dut is None:
> +                raise Exception("Set up VM ENV failed")
> +        except Exception as e:
> +            print utils.RED("Failure for %s" % str(e))
> +
> +        return True
> +
> +
> +
> +    def vm_testpmd_start(self):
> +        #
> +        # Start testpmd in vm
> +        #
> +        if self.vm_dut is not None:
> +            self.vm_dut.send_expect(self.vm_testpmd_vector, "testpmd>",
> 20)
> +            self.vm_dut.send_expect("start tx_first", "testpmd>")
> +
> +    def vm_tx_first_start(self):
> +        if self.vm_dut is not None:
> +            # Start tx_first
> +            self.vm_dut.send_expect("start tx_first", "testpmd>")
> +
Look like this function is redundant, vm_testpmd_start has already start traffic.


> +
> +    def send_verify(self, case, frame_sizes, mac_address, vlan_id1=0,
> tag="Performance"):
> +        self.result_table_create(self.header_row)
> +        self.virtio_mac=mac_address

This function is internal used, it's better not change class variable in this local function.
You can do that in upper layer.

> +        for frame_size in frame_sizes:
> +            info = "Running test %s, and %d frame size." % (case,
> frame_size)
> +            self.logger.info(info)
> +            payload = frame_size - HEADER_SIZE['eth'] - HEADER_SIZE['ip']
> +            flow1 =
> '[Ether(dst="%s")/Dot1Q(vlan=%s)/IP(src="%s",dst="%s")/("X"*%d)]' % (
> +                self.virtio_mac, vlan_id1, self.src1, self.dst1, payload)
> +            self.tester.scapy_append('wrpcap("flow1.pcap", %s)' % flow1)
> +            self.tester.scapy_execute()
> +
> +            tgenInput = []
> +            port = self.tester.get_local_port(self.pf)
> +            tgenInput.append((port, port, "flow1.pcap"))
> +
> +            _, pps = self.tester.traffic_generator_throughput(tgenInput,
> delay=30)
> +            Mpps = pps / 1000000.0
> +            pct  = Mpps * 100 / float(self.wirespeed(self.nic, frame_size,
> +                                      self.number_of_ports))
> +            data_row = [frame_size, str(Mpps), str(pct),tag]

Should add space after ',', please check with pep8 tool.

> +	    self.result_table_add(data_row)
> +        self.result_table_print()
> +
Look like code is not aligned.

> +
> +    def test_perf_qemu_vector_pmdfwd(self):
> +        #
> +        # Test the pvp performance for vector path
> +        #
> +        # start testpmd on VM
> +        self.jumbo = 0
> +        self.launch_vhost_sample()
> +        self.start_onevm()
> +        if self.vm_dut is not None:

When self.vm_dut is None, should we take that as error?

> +            # Start testpmd with user
> +            self.vm_dut.send_expect(self.vm_testpmd_vector, "testpmd>",
> 20)
> +            self.vm_dut.send_expect("start tx_first", "testpmd>")
> +
> +
> +        time.sleep(5)
> +        # Clean the output to ensure the commands cat ./vhost.out can be
> sent
> +        # and got answered correctly.
> +        self.dut.get_session_output(timeout=2)
> +        self.dut.session.copy_file_from("/root/dpdk/vhost.out")

Copy vhost.out and strip some information is used in several cases. It's better to abstract it to separated function and will make case code more clear.

> +        fp = open('./vhost.out', 'r')
> +        out = fp.read()
> +        fp.close()
> +        # Get the VLAN ID for virtio
> +        print "Check the vlan info: "
> +        l1 = re.findall(
> +            'MAC_ADDRESS.*?%s.*?and.*?VLAN_TAG.*?(\d+).*?registered' %
> +            (str(self.virtio1_mac)), out)
> +        if len(l1) > 0:
> +            vlan_id1 = l1[0]
> +            print vlan_id1

Please remove debug code here. If you need some special output, please use function like self.logger.info("Found vlan id %d for virtio device" % vlan_id1). 
> +        else:
> +            vlan_id1 = 1000
Without vlan information, should we consider it as error?

> +        self.send_verify(self.running_case, self.frame_sizes,
> self.virtio1_mac, vlan_id1 ,"Virtio 0.95 Vector Performance")
> +
> +
> +    def test_perf_qemu_normal_pmdfwd(self):
> +        #
> +        # Test the performance for normal path
> +        #
> +        # start testpmd on VM
> +        self.jumbo = 0
> +        self.launch_vhost_sample()
> +        self.start_onevm()
> +        if self.vm_dut is not None:
> +            # Start testpmd with user
> +            self.vm_dut.send_expect(self.vm_testpmd_normal, "testpmd>",
> 20)
> +            self.vm_dut.send_expect("start tx_first", "testpmd>")
> +
> +	time.sleep(5)
> +        # Clean the output to ensure the commands cat ./vhost.out can be
> sent
> +        # and got answered correctly.
> +        self.dut.get_session_output(timeout=2)
> +        self.dut.session.copy_file_from("/root/dpdk/vhost.out")
> +        fp = open('./vhost.out', 'r')
> +        out = fp.read()
> +        fp.close()
> +        # Get the VLAN ID for virtio
> +        print "Check the vlan info: "
> +        l1 = re.findall(
> +            'MAC_ADDRESS.*?%s.*?and.*?VLAN_TAG.*?(\d+).*?registered' %
> +            (str(self.virtio1_mac)), out)
> +        if len(l1) > 0:
> +            vlan_id1 = l1[0]
> +            print vlan_id1
> +        else:
> +            vlan_id1 = 1000
> +        self.send_verify(self.running_case, self.frame_sizes,
> self.virtio1_mac, vlan_id1, "Virtio 0.95 Normal Performance")
> +
> +
> +    def test_perf_qemu_mergeable_pmdfwd(self):
> +        #
> +        # Test the performance for mergeable path
> +        #
> +        # start testpmd on VM
> +        self.jumbo = 1
> +        self.launch_vhost_sample()
> +        self.start_onevm()
> +        if self.vm_dut is not None:
> +            # Start testpmd with user
> +            self.vm_dut.send_expect(self.vm_testpmd_vector, "testpmd>",
> 20)
> +            self.vm_dut.send_expect("start tx_first", "testpmd>")
> +
> +        time.sleep(5)
> +        # Clean the output to ensure the commands cat ./vhost.out can be
> sent
> +        # and got answered correctly.
> +        self.dut.get_session_output(timeout=2)
> +        self.dut.session.copy_file_from("/root/dpdk/vhost.out")
> +        fp = open('./vhost.out', 'r')
> +        out = fp.read()
> +        fp.close()
> +        # Get the VLAN ID for virtio
> +        print "Check the vlan info: "
> +        l1 = re.findall(
> +            'MAC_ADDRESS.*?%s.*?and.*?VLAN_TAG.*?(\d+).*?registered' %
> +            (str(self.virtio1_mac)), out)
> +        if len(l1) > 0:
> +            vlan_id1 = l1[0]
> +            print vlan_id1
> +        else:
> +            vlan_id1 = 1000
> +        self.send_verify(self.running_case, self.frame_sizes,
> self.virtio1_mac, vlan_id1 , "Virtio 0.95 Mergeable Performance")
> +
> +    def test_perf_different_qemuversion_vector_pmdfwd(self):
> +        #
> +        # Test the performance for different qemu versions, for example:
> qemu2.5,
> +        # qemu2.6, qemu2.7
> +        #
> +        # start testpmd on VM
> +        for path in self.qemu_path:
> +            self.jumbo = 0
> +            self.launch_vhost_sample()
> +            self.start_onevm(path)
> +            if self.vm_dut is not None:
> +            # Start testpmd in VM
> +                self.vm_dut.send_expect(self.vm_testpmd_vector,
> "testpmd>", 20)
> +                self.vm_dut.send_expect("start tx_first", "testpmd>")
> +            time.sleep(5)
> +
> +            self.dut.get_session_output(timeout=2)
> +            self.dut.session.copy_file_from("/root/dpdk/vhost.out")
> +            fp = open('./vhost.out', 'r')
> +            out = fp.read()
> +            fp.close()
> +            # Get the VLAN ID for virtio
> +            print "Check the vlan info: "
> +            l1 = re.findall(
> +                'MAC_ADDRESS.*?%s.*?and.*?VLAN_TAG.*?(\d+).*?registered' %
> +                (str(self.virtio1_mac)), out)
> +            if len(l1) > 0:
> +                vlan_id1 = l1[0]
> +                print vlan_id1
> +            else:
> +                vlan_id1 = 1000
> +            self.send_verify(self.running_case, self.frame_sizes,
> self.virtio1_mac, vlan_id1, path)
> +            print "This is the vector result for qemu path %s" % path
> +	    self.dut.send_expect("rm -rf ./vhost.out", "#")
> +            self.dut.send_expect("rm -rf ./vhost-net*", "#")
> +            self.dut.send_expect("killall -s INT vhost-switch", "#")
> +            self.dut.send_expect("killall -s INT qemu-system-x86_64", "#")
> +
> +    def test_perf_virtio_modern_qemu_vector_pmdfwd(self):
> +        #
> +        # Test the vhost/virtio pvp performance for virtio1.0
> +        #
> +        #
> +        # start testpmd on VM
> +        self.jumbo = 0
> +        self.launch_vhost_sample()
> +        self.start_onevm("",1)
> +        if self.vm_dut is not None:
> +            # Start testpmd with user
> +            self.vm_dut.send_expect(self.vm_testpmd_vector, "testpmd>",
> 20)
> +            self.vm_dut.send_expect("start tx_first", "testpmd>")
> +
> +        time.sleep(5)
> +        self.dut.get_session_output(timeout=2)
> +        self.dut.session.copy_file_from("/root/dpdk/vhost.out")
> +        fp = open('./vhost.out', 'r')
> +        out = fp.read()
> +        fp.close()
> +        # Get the VLAN ID for virtio
> +        print "Check the vlan info: "
> +        l1 = re.findall(
> +            'MAC_ADDRESS.*?%s.*?and.*?VLAN_TAG.*?(\d+).*?registered' %
> +            (str(self.virtio1_mac)), out)
> +        if len(l1) > 0:
> +            vlan_id1 = l1[0]
> +            print vlan_id1
> +        else:
> +            vlan_id1 = 1000
> +        self.send_verify(self.running_case, self.frame_sizes,
> self.virtio1_mac, vlan_id1, "virtio1.0, Vector")
> +
> +
> +    def test_perf_virtio_modern_qemu_normal_pmdfwd(self):
> +        #
> +        # Test the performance of one vm with 2virtio devices in legacy
> fwd
> +        #
> +        # start testpmd on VM
> +        self.jumbo = 0
> +        self.launch_vhost_sample()
> +        self.start_onevm("",1)
> +        if self.vm_dut is not None:
> +            # Start testpmd with user
> +            self.vm_dut.send_expect(self.vm_testpmd_normal, "testpmd>",
> 20)
> +            self.vm_dut.send_expect("start tx_first", "testpmd>")
> +
> +        time.sleep(5)
> +        self.dut.get_session_output(timeout=2)
> +        self.dut.session.copy_file_from("/root/dpdk/vhost.out")
> +        fp = open('./vhost.out', 'r')
> +        out = fp.read()
> +        fp.close()
> +        # Get the VLAN ID for virtio
> +        print "Check the vlan info: "
> +        l1 = re.findall(
> +            'MAC_ADDRESS.*?%s.*?and.*?VLAN_TAG.*?(\d+).*?registered' %
> +            (str(self.virtio1_mac)), out)
> +        if len(l1) > 0:
> +            vlan_id1 = l1[0]
> +            print vlan_id1
> +        else:
> +            vlan_id1 = 1000
> +        self.send_verify(self.running_case, self.frame_sizes,
> self.virtio1_mac, vlan_id1, "virtio1.0, Normal")
> +
> +
> +    def test_perf_virtio_modern_qemu_mergeable_pmdfwd(self):
> +        #
> +        # Test the performance of one vm with 2virtio devices in legacy
> fwd
> +        #
> +        # start testpmd on VM
> +        self.jumbo = 1
> +        self.launch_vhost_sample()
> +        self.start_onevm("",1)
> +        if self.vm_dut is not None:
> +            # Start testpmd with user
> +            self.vm_dut.send_expect(self.vm_testpmd_vector, "testpmd>",
> 20)
> +            self.vm_dut.send_expect("start tx_first", "testpmd>")
> +
> +        time.sleep(5)
> +        self.dut.get_session_output(timeout=2)
> +        self.dut.session.copy_file_from("/root/dpdk/vhost.out")
> +        fp = open('./vhost.out', 'r')
> +        out = fp.read()
> +        fp.close()
> +        # Get the VLAN ID for virtio
> +        print "Check the vlan info: "
> +        l1 = re.findall(
> +            'MAC_ADDRESS.*?%s.*?and.*?VLAN_TAG.*?(\d+).*?registered' %
> +            (str(self.virtio1_mac)), out)
> +        if len(l1) > 0:
> +            vlan_id1 = l1[0]
> +            print vlan_id1
> +        else:
> +            vlan_id1 = 1000
> +        self.send_verify(self.running_case, self.frame_sizes,
> self.virtio1_mac, vlan_id1, "virtio1.0, Mergeable")
> +
> +
> +
> +
> +

Too much blank line, it should be only one between each method in class.

> +    def tear_down(self):
> +        #
> +        # Run after each test case.
> +        # Clear vhost-switch and qemu to avoid blocking the following TCs
> +        #
> +        self.dut.send_expect("killall -s INT vhost-switch", "#")
> +        self.dut.send_expect("killall -s INT qemu-system-x86_64", "#")

Please use self.vm.stop to close virtual machine, there're some resources need to cleaned up by stop function.

> +        time.sleep(2)
> +
> +    def tear_down_all(self):
> +        """
> +        Run after each test suite.
> +        """
> +        pass
> --
> 2.7.4