Hi Lijuan,

Sorry, what you suggested makes sense. The V2 I send was some changes that will make the code cleaner by combining some duplicated code in the V1. To solve this issue, I will submit a patch that combines both of these versions. 

Thanks
David

On Thu, Jul 23, 2020 at 10:01 PM Tu, Lijuan <lijuan.tu@intel.com> wrote:
Hi David,

V2 should be also based on DTS remote master, but not your local code.

When V1 are not merged, we submit V2, V2 should cover V1+ fix comments based on DTS remote code, maintainer will ignore V1 and merge V2. It's not make sense to merge patches with obvious defect.

> -----Original Message-----
> From: dts <dts-bounces@dpdk.org> On Behalf Of David Liu
> Sent: 2020年7月16日 4:09
> To: dts@dpdk.org
> Cc: lylavoie@iol.unh.edu; David Liu <dliu@iol.unh.edu>
> Subject: [dts] [PATCH v2]Add RSS Key Update Feature
>
>  Merge duplicated code. Add flag in send packet and verify  result to identify
> symmetric used.
>
> Signed-off-by: David Liu <dliu@iol.unh.edu>
> ---
>  tests/TestSuite_rss_key_update.py | 183 ++++++++----------------------
>  1 file changed, 46 insertions(+), 137 deletions(-)
>
> diff --git a/tests/TestSuite_rss_key_update.py
> b/tests/TestSuite_rss_key_update.py
> index 0985f30..adb44f1 100644
> --- a/tests/TestSuite_rss_key_update.py
> +++ b/tests/TestSuite_rss_key_update.py
> @@ -39,7 +39,6 @@ Test the support of RSS Key Update by Poll Mode Drivers.
>
>  import time
>  import re
> -import packet
>  import random
>  import utils
>
> @@ -63,85 +62,37 @@ iptypes = {'ipv4-sctp': 'sctp',
>
>  class TestRssKeyUpdate(TestCase):
>
> -    def send_packet(self, itf, tran_type):
> +    def send_packet(self, itf, tran_type, symmetric):
>          """
>          Sends packets.
>          """
> +        packet_list = {
> +            'ipv4-sctp': 'IP(src="192.168.0.%d",
> dst="192.168.0.%d")/SCTP(sport=1024,dport=1024,tag=1)',
> +            'ipv4-other': 'IP(src="192.168.0.%d", dst="192.168.0.%d")',
> +            'ipv4-frag': 'IP(src="192.168.0.%d",
> dst="192.168.0.%d",frag=1,flags="MF")',
> +            'ipv4-udp': 'IP(src="192.168.0.%d",
> dst="192.168.0.%d")/UDP(sport=1024,dport=1024)',
> +            'ipv4-tcp': 'IP(src="192.168.0.%d",
> dst="192.168.0.%d")/TCP(sport=1024,dport=1024)',
> +            'ipv6-other':'IPv6(src="3ffe:2501:200:1fff::%d",
> dst="3ffe:2501:200:3::%d")',
> +            'ipv6-sctp': 'IPv6(src="3ffe:2501:200:1fff::%d",
> dst="3ffe:2501:200:3::%d", nh=132)/SCTP(sport=1024,dport=1024,tag=1)',
> +            'ipv6-udp': 'IPv6(src="3ffe:2501:200:1fff::%d",
> dst="3ffe:2501:200:3::%d")/UDP(sport=1024,dport=1024)',
> +            'ipv6-tcp': 'IPv6(src="3ffe:2501:200:1fff::%d",
> dst="3ffe:2501:200:3::%d")/TCP(sport=1024,dport=1024)',
> +            'ipv6-frag': 'IPv6(src="3ffe:2501:200:1fff::%d",
> dst="3ffe:2501:200:3::%d",nh=44)/IPv6ExtHdrFragment()'
> +        }
> +
>          received_pkts = []
>          self.tester.scapy_foreground()
>          self.dut.send_expect("start", "testpmd>")
>          mac = self.dut.get_mac_address(0)
>
>          # send packet with different source and dest ip
> -        if tran_type == "ipv4-other":
> -            for i in range(10):
> -                packet = r'sendp([Ether(dst="%s",
> src=get_if_hwaddr("%s"))/IP(src="192.168.0.%d", dst="192.168.0.%d")],
> iface="%s")' % (
> -                    mac, itf, i + 1, i + 2, itf)
> -                self.tester.scapy_append(packet)
> -            self.tester.scapy_execute()
> -            time.sleep(.5)
> -        elif tran_type == "ipv4-tcp":
> -            for i in range(10):
> -                packet = r'sendp([Ether(dst="%s",
> src=get_if_hwaddr("%s"))/IP(src="192.168.0.%d",
> dst="192.168.0.%d")/TCP(sport=1024,dport=1024)], iface="%s")' % (
> -                    mac, itf, i + 1, i + 2, itf)
> -                self.tester.scapy_append(packet)
> -            self.tester.scapy_execute()
> -            time.sleep(.5)
> -        elif tran_type == "ipv4-udp":
> -            for i in range(10):
> -                packet = r'sendp([Ether(dst="%s",
> src=get_if_hwaddr("%s"))/IP(src="192.168.0.%d",
> dst="192.168.0.%d")/UDP(sport=1024,dport=1024)], iface="%s")' % (
> -                    mac, itf, i + 1, i + 2, itf)
> -                self.tester.scapy_append(packet)
> -            self.tester.scapy_execute()
> -            time.sleep(.5)
> -        elif tran_type == "ipv4-sctp":
> -            for i in range(10):
> -                packet = r'sendp([Ether(dst="%s",
> src=get_if_hwaddr("%s"))/IP(src="192.168.0.%d",
> dst="192.168.0.%d")/SCTP(sport=1024,dport=1024,tag=1)], iface="%s")' % (
> -                    mac, itf, i + 1, i + 2, itf)
> -                self.tester.scapy_append(packet)
> -            self.tester.scapy_execute()
> -            time.sleep(.5)
> -        elif tran_type == "ipv4-frag":
> -            for i in range(10):
> -                packet = r'sendp([Ether(dst="%s",
> src=get_if_hwaddr("%s"))/IP(src="192.168.0.%d",
> dst="192.168.0.%d",frag=1,flags="MF")], iface="%s")' % (
> -                    mac, itf, i + 1, i + 2, itf)
> -                self.tester.scapy_append(packet)
> -            self.tester.scapy_execute()
> -            time.sleep(.5)
> -
> -        elif tran_type == "ipv6-other":
> -            for i in range(10):
> -                packet = r'sendp([Ether(dst="%s",
> src=get_if_hwaddr("%s"))/IPv6(src="3ffe:2501:200:1fff::%d",
> dst="3ffe:2501:200:3::%d")], iface="%s")' % (
> -                    mac, itf, i + 1, i + 2, itf)
> -                self.tester.scapy_append(packet)
> -            self.tester.scapy_execute()
> -            time.sleep(.5)
> -        elif tran_type == "ipv6-tcp":
> -            for i in range(10):
> -                packet = r'sendp([Ether(dst="%s",
> src=get_if_hwaddr("%s"))/IPv6(src="3ffe:2501:200:1fff::%d",
> dst="3ffe:2501:200:3::%d")/TCP(sport=1024,dport=1024)], iface="%s")' % (
> -                    mac, itf, i + 1, i + 2, itf)
> -                self.tester.scapy_append(packet)
> -            self.tester.scapy_execute()
> -            time.sleep(.5)
> -        elif tran_type == "ipv6-udp":
> -            for i in range(10):
> -                packet = r'sendp([Ether(dst="%s",
> src=get_if_hwaddr("%s"))/IPv6(src="3ffe:2501:200:1fff::%d",
> dst="3ffe:2501:200:3::%d")/UDP(sport=1024,dport=1024)], iface="%s")' % (
> -                    mac, itf, i + 1, i + 2, itf)
> -                self.tester.scapy_append(packet)
> -            self.tester.scapy_execute()
> -            time.sleep(.5)
> -        elif tran_type == "ipv6-sctp":
> +        if tran_type in packet_list.keys():
> +            packet_temp = r'sendp([Ether(dst="%s",
> + src=get_if_hwaddr("%s"))/%s], iface="%s")' % (mac, itf,
> + packet_list[tran_type], itf)
>              for i in range(10):
> -                packet = r'sendp([Ether(dst="%s",
> src=get_if_hwaddr("%s"))/IPv6(src="3ffe:2501:200:1fff::%d",
> dst="3ffe:2501:200:3::%d", nh=132)/SCTP(sport=1024,dport=1024,tag=1)],
> iface="%s")' % (
> -                    mac, itf, i + 1, i + 2, itf)
> -                self.tester.scapy_append(packet)
> -            self.tester.scapy_execute()
> -            time.sleep(.5)
> -        elif tran_type == "ipv6-frag":
> -            for i in range(10):
> -                packet = r'sendp([Ether(dst="%s",
> src=get_if_hwaddr("%s"))/IPv6(src="3ffe:2501:200:1fff::%d",
> dst="3ffe:2501:200:3::%d",nh=44)/IPv6ExtHdrFragment()], iface="%s")' % (
> -                    mac, itf, i + 1, i + 2, itf)
> +                packet = packet_temp % (i + 1, i + 2)
>                  self.tester.scapy_append(packet)
> +                if symmetric:
> +                    packet2 = packet_list[tran_type] % (mac, itf, i + 2, i + 1, itf)
> +                    self.tester.scapy_append(packet2)
>              self.tester.scapy_execute()
>              time.sleep(.5)
>          else:
> @@ -154,102 +105,64 @@ class TestRssKeyUpdate(TestCase):
>          # collect the hash result and the queue id
>          for line in lines:
>              line = line.strip()
> -            if len(line) != 0 and line.strip().startswith("port "):
> +            if len(line) != 0 and line.startswith("port "):
>                  reta_line = {}
> -                rexp = r"port (\d)/queue (\d{1,2}): received (\d) packets"
> -                m = re.match(rexp, line.strip())
> +                rexp = r"port (\d+)/queue (\d+): received (\d+) packets"
> +                m = re.match(rexp, line)
>                  if m:
>                      reta_line["port"] = m.group(1)
>                      reta_line["queue"] = m.group(2)
>
> -            elif len(line) != 0 and line.startswith(("src=",)):
> +            elif len(line) != 0 and line.startswith("src="):
>                  if "RSS hash" not in line:
>                      continue
>                  for item in line.split("-"):
>                      item = item.strip()
> -                    if(item.startswith("RSS hash")):
> +                    if item.startswith("RSS hash"):
>                          name, value = item.split("=", 1)
>
>                  reta_line[name.strip()] = value.strip()
>                  received_pkts.append(reta_line)
>
> -        return(self.verifyResult(received_pkts))
> +        return(self.verifyResult(received_pkts, symmetric))
>
> -    def verifyResult(self, reta_lines):
> +    def verifyResult(self, reta_lines, symmetric):
>          """
>          Verify whether or not the result passes.
>          """
> -
> -        global reta_num
>          result = []
>          key_id = {}
>          self.verify(len(reta_lines) > 0, 'No packet received!')
>          self.result_table_create(
>              ['packet index', 'hash value', 'hash index', 'queue id', 'actual queue id',
> 'pass '])
>
> -        i = 0
> -
> -        for tmp_reta_line in reta_lines:
> +        for i, tmp_reta_line in enumerate(reta_lines):
>              status = "false"
>              # compute the hash result of five tuple into the 7 LSBs value.
>              hash_index = int(tmp_reta_line["RSS hash"], 16) % reta_num
> -            print(reta_entries[hash_index], tmp_reta_line)
>              if(reta_entries[hash_index] == int(tmp_reta_line["queue"])):
>                  status = "true"
>                  result.insert(i, 0)
> +                if symmetric:
> +                    if(i % 2 == 1):
> +                        if(pre_RSS_hash == tmp_reta_line["RSS hash"]):
> +                            status = "true"
> +                            result.insert(len(reta_lines) + (i - 1) // 2, 0)
> +                        else:
> +                            status = "fail"
> +                            result.insert(len(reta_lines) + (i - 1) // 2, 1)
> +                    pre_RSS_hash = tmp_reta_line["RSS hash"]
>              else:
>                  status = "fail"
>                  result.insert(i, 1)
>              self.result_table_add(
>                  [i, tmp_reta_line["RSS hash"], hash_index, reta_entries[hash_index],
> tmp_reta_line["queue"], status])
> -            i = i + 1
> -            key_id[tmp_reta_line["RSS hash"]]=reta_entries[hash_index]
> +            key_id[tmp_reta_line["RSS hash"]] =
> + reta_entries[hash_index]
>
>          self.result_table_print()
>          self.verify(sum(result) == 0, "the reta update function failed!")
>          return key_id
>
> -    def verifyResult_symmetric(self, reta_lines):
> -        """
> -        Verify whether or not the result passes.
> -        """
> -
> -        global reta_num
> -        result = []
> -        key_id = {}
> -        self.verify(len(reta_lines) > 0, 'No packet received!')
> -        self.result_table_create(
> -            ['packet index', 'RSS hash', 'hash index', 'queue id', 'actual queue id', 'pass
> '])
> -
> -        i = 0
> -        for tmp_reta_line in reta_lines:
> -            status = "false"
> -            # compute the hash result of five tuple into the 7 LSBs value.
> -            hash_index = int(tmp_reta_line["RSS hash"], 16) % reta_num
> -            if(reta_entries[hash_index] == int(tmp_reta_line["queue"])):
> -                status = "true"
> -                result.insert(i, 0)
> -                if(i % 2 == 1):
> -                    if(pre_RSS_hash == tmp_reta_line["RSS hash"]):
> -                        status = "true"
> -                        result.insert(len(reta_lines) + (i - 1) // 2, 0)
> -                    else:
> -                        status = "fail"
> -                        result.insert(len(reta_lines) + (i - 1) // 2, 1)
> -                pre_RSS_hash = tmp_reta_line["RSS hash"]
> -            else:
> -                status = "fail"
> -                result.insert(i, 1)
> -            self.result_table_add(
> -                [i, tmp_reta_line["RSS hash"], hash_index, reta_entries[hash_index],
> tmp_reta_line["queue"], status])
> -            i = i + 1
> -            key_id[tmp_reta_line["RSS hash"]]=reta_entries[hash_index]
> -
> -        self.result_table_print()
> -        self.verify(
> -            sum(result) == 0, "the symmetric RSS hash function failed!")
> -        return key_id
> -
>      def set_up_all(self):
>          """
>          Run at the start of each test suite.
> @@ -282,7 +195,7 @@ class TestRssKeyUpdate(TestCase):
>          elif self.nic in ["redrockcanyou", "atwood", "boulderrapid"]:
>              reta_num = 128
>          else:
> -            self.verify(False, f"NIC Unsupported:{self.nic}")
> +            self.verify(False, f"NIC Unsupported: {self.nic}")
>
>          cores = self.dut.get_core_list("all")
>          self.coremask = utils.create_mask(cores) @@ -300,8 +213,6 @@ class
> TestRssKeyUpdate(TestCase):
>          dutPorts = self.dut.get_ports(self.nic)
>          localPort = self.tester.get_local_port(dutPorts[0])
>          self.itf = self.tester.get_interface(localPort)
> -        global reta_num
> -        global iptypes
>
>          self.dut.kill_all()
>
> @@ -326,12 +237,12 @@ class TestRssKeyUpdate(TestCase):
>                  reta_entries.insert(i, random.randint(0, queue - 1))
>                  self.dut.send_expect(f"port config 0 rss reta ({i},{reta_entries[i]})",
> "testpmd> ")
>
> -            ori_output = self.send_packet(self.itf, iptype)
> +            ori_output = self.send_packet(self.itf, iptype, False)
>
>              self.dut.send_expect("show port 0 rss-hash key", "testpmd> ")
>              self.dut.send_expect(f"port config 0 rss-hash-key {iptype}
> 4439796BB54C50f3B675EF5B124F9F30B8A2C0FFFFDC4D02A08C9B334FF64A4C
> 05C6FA343958D855FFF9583AE138C92E81150FFF", "testpmd> ")
>
> -            new_output = self.send_packet(self.itf, iptype)
> +            new_output = self.send_packet(self.itf, iptype, False)
>
>              self.verify(ori_output != new_output, "before and after results are the
> same, hash key configuration failed!")
>
> @@ -356,16 +267,15 @@ class TestRssKeyUpdate(TestCase):
>                  reta_entries.insert(i, random.randint(0, queue - 1))
>                  self.dut.send_expect(f"port config 0 rss reta ({i},{reta_entries[i]})",
> "testpmd> ")
>
> -            ori_output = self.send_packet(self.itf, iptype)
> +            ori_output = self.send_packet(self.itf, iptype, True)
>
>              out = self.dut.send_expect("show port 0 rss-hash key", "testpmd> ")
>              self.verify("rss disable" not in out, "rss is disable!")
>              self.dut.send_expect(f"port config 0 rss-hash-key {iptype}
> 4439796BB54C50f3B675EF5B124F9F30B8A2C0FFFFDC4D02A08C9B334FF64A4C
> 05C6FA343958D855FFF9583AE138C92E81150FFF", "testpmd> ")
>
> -            new_output = self.send_packet(self.itf, iptype)
> +            new_output = self.send_packet(self.itf, iptype, True)
>
>              self.verify(ori_output != new_output, "before and after results are the
> same, hash key configuration failed!")
> -        self.dut.send_expect("quit", "# ", 30)
>
>      def test_set_hash_key_short_long(self):
>
> @@ -384,26 +294,25 @@ class TestRssKeyUpdate(TestCase):
>          out = self.dut.send_expect("show port info all", "testpmd> ", 120)
>          self.verify(f"Hash key size in bytes: {nic_rss_key_size[self.nic]}" in out, "not
> expected hash key size!")
>
> -        test_keies = {
> +        test_keys = {
>
> "4439796BB54C50f3B675EF5B124F9F30B8A2C0FFFFDC4D02A08C9B334FF64A4
> C05C6FA343958D855FFF9583AE138C92E81150FFFFF": "longer",
>
> "4439796BB54C50f3B675EF5B124F9F30B8A2C0DC4D02A08C9B334FF64A4C05C
> 6FA343958D855FFF9583AE138C92E81150FFF": "shorter",
>          }
>
>          # config key length longer/shorter than 104 hexa-decimal numbers
> -        for key, error in test_keies.items():
> +        for key, error in test_keys.items():
>              out = self.dut.send_expect(f"port config 0 rss-hash-key ipv4-udp {key}",
> "testpmd> ")
>              self.verify("invalid" in out, f"Try to set hash key {error} than 104 hexa-
> decimal numbers!")
>
>          # config ket length same as 104 hex-decimal numbers and keep the config
>          key =
> "4439796BB54C50f3B675EF5B124F9F30B8A2C0FFFFDC4D02A08C9B334FF64A4
> C05C6FA343958D855FFF9583AE138C92E81150FFF"
>          out = self.dut.send_expect(f"port config 0 rss-hash-key ipv4-udp {key}",
> "testpmd> ")
> -        self.dut.send_expect("quit", "# ", 30)
>
>      def tear_down(self):
>          """
>          Run after each test case.
>          """
> -        self.dut.send_expect("quit", "# ", 30)
> +        self.pmdout.quit()
>
>      def tear_down_all(self):
>          """
> --
> 2.17.1