Re: [dts] [PATCH V1] fix link status interrupt failed case

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

Hi Huilong,
Some answers inline.

> -----Original Message-----
> From: Xu, HuilongX
> Sent: Thursday, August 31, 2017 1:46 PM
> To: Liu, Yong <yong.liu@intel.com>; dts@dpdk.org
> Subject: RE: [dts] [PATCH V1] fix link status interrupt failed case
> 
> Hi  yong,
> 
> -----Original Message-----
> From: Liu, Yong
> Sent: Wednesday, August 30, 2017 5:07 PM
> To: Xu, HuilongX <huilongx.xu@intel.com>; dts@dpdk.org
> Cc: Xu, HuilongX <huilongx.xu@intel.com>
> Subject: RE: [dts] [PATCH V1] fix link status interrupt failed case
> 
> Huilong,
> Some comments and questions below.
> 
> > -----Original Message-----
> > From: dts [mailto:dts-bounces@dpdk.org] On Behalf Of xu,huilong
> > Sent: Friday, August 25, 2017 3:12 PM
> > To: dts@dpdk.org
> > Cc: Xu, HuilongX <huilongx.xu@intel.com>
> > Subject: [dts] [PATCH V1] fix link status interrupt failed case
> >
> > update list:
> > 1. update fortville nic test interrupt mode list 2. fix example setup
> > failed 3. fix check port stats in example 4. fix check rx/tx packet
> > get count number error
> >
> > Signed-off-by: xu,huilong <huilongx.xu@intel.com>
> > ---
> >  tests/TestSuite_link_status_interrupt.py | 34
> > +++++++++++++++++++++++----
> > -----
> >  1 file changed, 25 insertions(+), 9 deletions(-)
> >
> > diff --git a/tests/TestSuite_link_status_interrupt.py
> > b/tests/TestSuite_link_status_interrupt.py
> > index ae205cc..2249cf1 100644
> > --- a/tests/TestSuite_link_status_interrupt.py
> > +++ b/tests/TestSuite_link_status_interrupt.py
> > @@ -61,6 +61,15 @@ class TestLinkStatusInterrupt(TestCase):
> >          out =
> self.dut.build_dpdk_apps("./examples/link_status_interrupt")
> >          self.verify("Error" not in out, "compilation error 1")
> >          self.verify("No such file" not in out, "compilation error 2")
> > +        # fortville NIC not support legacy intr mode
> > +        if self.nic in ["fortville_eagle", "fortville_spirit",
> > +                        "fortville_spirt_single", "fortville_25g"]:
> > +            self.basic_intr_mode = ["msix"]
> > +        else:
> > +            self.basic_intr_mode = ["msix", "legacy"]
> > +        # vfio support msi intr mode
> > +        if self.drivername == "vfio-pci":
> > +            self.basic_intr_mode.append("msi")
> 
> 
> Please add link of dpdk release note in the commit log.
> This is a linux kernel issue, I will add kernel patch url next version
> 

Just dpdk release note is enough, just let other guys known where to get detail information.

> >
> >      def set_link_status_and_verify(self, dutPort, status):
> >          """
> > @@ -71,7 +80,9 @@ class TestLinkStatusInterrupt(TestCase):
> >          self.tester.send_expect("ifconfig %s %s" %
> >                                  (self.intf, status.lower()), "# ", 10)
> >          verify_point = "Port %s Link %s" % (dutPort, status)
> > -        self.dut.send_expect("", verify_point, 60)
> > +
> > +        out = self.dut.get_session_output(timeout=60)
> > +        self.verify(verify_point in out,"link status update error")
> >
> >      def set_up(self):
> >          """
> > @@ -86,20 +97,23 @@ class TestLinkStatusInterrupt(TestCase):
> >          if self.drivername == "igb_uio":
> >              cmdline = self.path + " -c %s -n %s -- -p %s " % (
> >                  self.coremask, self.dut.get_memory_channels(),
> > self.portmask)
> > -            for mode in ["legacy", "msix"]:
> > +            for mode in self.basic_intr_mode:
> >                  self.dut.send_expect("rmmod -f igb_uio", "#", 20)
> >                  self.dut.send_expect(
> >                      'insmod %s/kmod/igb_uio.ko "intr_mode=%s"' %
> > (self.target, mode), "# ")
> >                  self.dut.bind_interfaces_linux()
> > -                self.dut.send_expect(cmdline, "Aggregate statistics",
> 60)
> > +                self.dut.send_expect(cmdline, "", 180)
> 
> If there's nothing to check in output, you can use function
> "self.dut.send_command(timeout=time)".
> And 180 seconds is too long, what is waiting for?
> I will use send_command to replace send_expect, and example setup need
> more time, because this example will print info in loop mode.
> 
> > +                out = self.dut.get_session_output(timeout=60)
> 
> Function "get_session_output" just return all output after timeout.
> Normally we will use default value, why enlarge time out value to 60
> seconds?
> Infor "Port statistics" form example by loop mode, so need more time.

Could you add some variable for the loop time? 
In my mind 60 seconds is too long, maybe this sample just require for such time.
Please use some constant value to emphasis that the output interval of this sample is special.

> > +                self.verify("Port statistics" in out, "setup example
> > error")
> > +                time.sleep(10)
> >                  self.set_link_status_and_verify(self.dut_ports[0],
> 'Down')
> >                  self.set_link_status_and_verify(self.dut_ports[0], 'Up')
> >                  self.dut.send_expect("^C", "#", 60)
> >          elif self.drivername == "vfio-pci":
> > -            for mode in ["legacy", "msi", "msix"]:
> > +            for mode in self.basic_intr_mode :
> 
> Pep issue here, please modify it.
> I will update it at next version
> >                  cmdline = self.path + " -c %s -n %s --vfio-intr=%s --
> > - p %s" % (
> >                      self.coremask, self.dut.get_memory_channels(),
> > mode,
> > self.portmask)
> > -                self.dut.send_expect(cmdline, "Aggregate statistics",
> 60)
> > +                self.dut.send_expect(cmdline, "statistics
> > + statistics",
> > 120)
> 
> 
> A bit tricky here, why double "statistics" here?
> I will update it next version
> >                  self.set_link_status_and_verify(self.dut_ports[0],
> 'Down')
> >                  self.set_link_status_and_verify(self.dut_ports[0], 'Up')
> >                  self.dut.send_expect("^C", "#", 60) @@ -112,7 +126,7
> > @@ class TestLinkStatusInterrupt(TestCase):
> >          if self.drivername == "igb_uio":
> >              cmdline = self.path + " -c %s -n %s -- -p %s " % (
> >                  self.coremask, self.dut.get_memory_channels(),
> > self.portmask)
> > -            for mode in ["legacy", "msix"]:
> > +            for mode in self.basic_intr_mode:
> >                  self.dut.send_expect("rmmod -f igb_uio", "#", 20)
> >                  self.dut.send_expect(
> >                      'insmod %s/kmod/igb_uio.ko "intr_mode=%s"' %
> > (self.target, mode), "# ") @@ -129,11 +143,12 @@ class
> > TestLinkStatusInterrupt(TestCase):
> >                  pkt_send = re.findall("Total packets sent:\s*(\d*)",
> out)
> >                  pkt_received = re.findall(
> >                      "Total packets received:\s*(\d*)", out)
> > -                self.verify(pkt_send == pkt_received == '1',
> > +                # example will print rx/tx packet info by loop so
> > + only
> > check last one
> > +                self.verify(pkt_send[-1] == pkt_received[-1] == '1',
> >                              "Error: sent packets != received packets")
> >                  self.dut.send_expect("^C", "#", 60)
> >          elif self.drivername == "vfio-pci":
> > -            for mode in ["legacy", "msi", "msix"]:
> > +            for mode in self.basic_intr_mode:
> >                  cmdline = self.path + " -c %s -n %s --vfio-intr=%s --
> > - p %s" % (
> >                      self.coremask, self.dut.get_memory_channels(),
> > mode,
> > self.portmask)
> >                  self.dut.send_expect(cmdline, "Aggregate statistics",
> > 60) @@ -148,7 +163,8 @@ class TestLinkStatusInterrupt(TestCase):
> >                  pkt_send = re.findall("Total packets sent:\s*(\d*)",
> out)
> >                  pkt_received = re.findall(
> >                      "Total packets received:\s*(\d*)", out)
> > -                self.verify(pkt_send == pkt_received == '1',
> > +                # example will print rx/tx packet info by loop so
> > + only
> > check last one
> > +                self.verify(pkt_send[-1] == pkt_received[-1] == '1',
> >                              "Error: sent packets != received packets")
> >                  self.dut.send_expect("^C", "#", 60)
> >
> > --
> > 1.9.3