----- Original Message -----
From: "Jan Tluka" <jtluka(a)redhat.com>
To: "Ondrej Lichtner" <olichtne(a)redhat.com>
Cc: csfakian(a)redhat.com, lnst-developers(a)lists.fedorahosted.org
Sent: Tuesday, March 12, 2019 2:06:18 PM
Subject: Re: [PATCH-next 1/7] lnst.Recipes.ENRT: edit perf pinnings in Recipes
Fri, Mar 08, 2019 at 07:17:30PM CET, olichtne(a)redhat.com wrote:
>On Tue, Mar 05, 2019 at 05:09:05PM +0100, csfakian(a)redhat.com wrote:
>> From: Christos Sfakianakis <csfakian(a)redhat.com>
>>
>> Add 'perf_intr_cpu' parameter to account for affinity
>> of perf tool. Combine it with the existing 'dev_intr_cpu'
>> parameter to mirror the old 'nperf_cpupin', 'netdev_cpupin'
>> parameters and handle the pinnings for each scenario.
>>
>> Signed-off-by: Christos Sfakianakis <csfakian(a)redhat.com>
>> ---
>> .../Perf/Measurements/BaseFlowMeasurement.py | 7 ++++-
>> .../Perf/Measurements/IperfFlowMeasurement.py | 14 ++++++++++
>> lnst/Recipes/ENRT/BaseEnrtRecipe.py | 5 +++-
>> lnst/Recipes/ENRT/BondRecipe.py | 13 +++++-----
>> lnst/Recipes/ENRT/DoubleBondRecipe.py | 15 ++++++-----
>> lnst/Recipes/ENRT/DoubleTeamRecipe.py | 15 ++++++-----
>> lnst/Recipes/ENRT/SimplePerfRecipe.py | 14 +++++-----
>> lnst/Recipes/ENRT/TeamRecipe.py | 13 +++++-----
>> lnst/Recipes/ENRT/TeamVsBondRecipe.py | 15 ++++++-----
>> .../VirtualBridgeVlanInGuestMirroredRecipe.py | 21 +++++++--------
>> .../ENRT/VirtualBridgeVlanInGuestRecipe.py | 19 +++++++-------
>> .../VirtualBridgeVlanInHostMirroredRecipe.py | 21 +++++++--------
>> .../ENRT/VirtualBridgeVlanInHostRecipe.py | 19 +++++++-------
>> .../ENRT/VirtualBridgeVlansOverBondRecipe.py | 18 ++++++++-----
>> ...rtualOvsBridgeVlanInGuestMirroredRecipe.py | 21 +++++++--------
>> .../ENRT/VirtualOvsBridgeVlanInGuestRecipe.py | 19 +++++++-------
>> ...irtualOvsBridgeVlanInHostMirroredRecipe.py | 21 +++++++--------
>> .../ENRT/VirtualOvsBridgeVlanInHostRecipe.py | 19 +++++++-------
>> .../VirtualOvsBridgeVlansOverBondRecipe.py | 26 ++++++++-----------
>> lnst/Recipes/ENRT/VlansOverBondRecipe.py | 13 +++++-----
>> lnst/Recipes/ENRT/VlansOverTeamRecipe.py | 13 +++++-----
>> lnst/Recipes/ENRT/VlansRecipe.py | 14 +++++-----
>> 22 files changed, 192 insertions(+), 163 deletions(-)
>>
>> diff --git a/lnst/RecipeCommon/Perf/Measurements/BaseFlowMeasurement.py
>> b/lnst/RecipeCommon/Perf/Measurements/BaseFlowMeasurement.py
>> index da0b5f4..faac895 100644
>> --- a/lnst/RecipeCommon/Perf/Measurements/BaseFlowMeasurement.py
>> +++ b/lnst/RecipeCommon/Perf/Measurements/BaseFlowMeasurement.py
>> @@ -8,7 +8,7 @@ class Flow(object):
>> type,
>> generator, generator_bind,
>> receiver, receiver_bind,
>> - msg_size, duration, parallel_streams):
>> + msg_size, duration, parallel_streams, cpupin):
>> self._type = type
>>
>> self._generator = generator
>> @@ -19,6 +19,7 @@ class Flow(object):
>> self._msg_size = msg_size
>> self._duration = duration
>> self._parallel_streams = parallel_streams
>> + self._cpupin = cpupin
>>
>> @property
>> def type(self):
>> @@ -52,6 +53,10 @@ class Flow(object):
>> def parallel_streams(self):
>> return self._parallel_streams
>>
>> + @property
>> + def cpupin(self):
>> + return self._cpupin
>> +
>> class NetworkFlowTest(object):
>> def __init__(self, flow, server_job, client_job):
>> self._flow = flow
>> diff --git a/lnst/RecipeCommon/Perf/Measurements/IperfFlowMeasurement.py
>> b/lnst/RecipeCommon/Perf/Measurements/IperfFlowMeasurement.py
>> index 2f8ae98..45303fe 100644
>> --- a/lnst/RecipeCommon/Perf/Measurements/IperfFlowMeasurement.py
>> +++ b/lnst/RecipeCommon/Perf/Measurements/IperfFlowMeasurement.py
>> @@ -84,6 +84,13 @@ class IperfFlowMeasurement(BaseFlowMeasurement):
>> server_params = dict(bind = ipaddress(flow.receiver_bind),
>> oneoff = True)
>>
>> + if flow.cpupin:
>> + if flow.parallel_streams == 1:
>> + server_params["cpu_bind"] = flow.cpupin
>> + else:
>> + raise RecipeError("Unsupported combination of non-zero
>> cpupin "
>> + "with parallel perf streams.")
>> +
>> return host.prepare_job(IperfServer(**server_params),
>> job_level=ResultLevel.NORMAL)
>>
>> @@ -102,6 +109,13 @@ class IperfFlowMeasurement(BaseFlowMeasurement):
>> else:
>> raise RecipeError("Unsupported flow type
>> '{}'".format(flow.type))
>>
>> + if flow.cpupin:
>> + if flow.parallel_streams == 1:
>> + client_params["cpu_bind"] = flow.cpupin
>> + else:
>> + raise RecipeError("Unsupported combination of non-zero
>> cpupin "
>> + "with parallel perf streams.")
>> +
>> if flow.parallel_streams > 1:
>> client_params["parallel"] = flow.parallel_streams
>>
>> diff --git a/lnst/Recipes/ENRT/BaseEnrtRecipe.py
>> b/lnst/Recipes/ENRT/BaseEnrtRecipe.py
>> index d7d1aec..5dc3a5c 100644
>> --- a/lnst/Recipes/ENRT/BaseEnrtRecipe.py
>> +++ b/lnst/Recipes/ENRT/BaseEnrtRecipe.py
>> @@ -77,6 +77,7 @@ class BaseEnrtRecipe(PingTestAndEvaluate, PerfRecipe):
>> mtu = IntParam(mandatory=False)
>>
>> dev_intr_cpu = IntParam(default=0)
>> + perf_intr_cpu = IntParam(default=0)
>
>I think, the default value be "None" or there should be no default value
>(parameter being optional). Reason being, cpus are indexed from 0 and
>it's therefore a perfectly acceptable value to pin a process or
>interrupts to.
>
>Which means that the dev_intr_cpu default value is also wrong and should
>be fixed.
>
Agree, both these parameters are optional and if they're not specified
no action should be taken, i.e.
no dev_intr_cpu -> no pinning of device interrupts is done (e.g. in parallel
test)
no perf_tool_cpu -> no parameter is passed to iperf/netperf
Sure, will fix it.
> >>
> >> perf_duration = IntParam(default=60)
> >> perf_iterations = IntParam(default=5)
> >> @@ -199,7 +200,9 @@ class BaseEnrtRecipe(PingTestAndEvaluate, PerfRecipe):
> >> receiver_bind = server_bind,
> >> msg_size = self.params.perf_msg_size,
> >> duration = self.params.perf_duration,
> >> - parallel_streams =
> >> self.params.perf_parallel_streams)
> >> + parallel_streams =
> >> self.params.perf_parallel_streams,
> >> + cpupin = self.params.perf_intr_cpu
> >> + )
> >>
> >> flow_measurement = self.params.net_perf_tool([flow])
> >> yield PerfRecipeConf(
> >> diff --git a/lnst/Recipes/ENRT/BondRecipe.py
> >> b/lnst/Recipes/ENRT/BondRecipe.py
> >> index 119c437..d2268fb 100644
> >> --- a/lnst/Recipes/ENRT/BondRecipe.py
> >> +++ b/lnst/Recipes/ENRT/BondRecipe.py
> >> @@ -55,10 +55,10 @@ class BondRecipe(BaseEnrtRecipe):
> >> m2.eth0.up()
> >>
> >> #TODO better service handling through HostAPI
> >> - m1.run("service irqbalance stop")
> >> - m2.run("service irqbalance stop")
> >> - for m in self.matched:
> >> - for dev in m.devices:
> >> + if self.params.dev_intr_cpu:
> >> + for m in [m1, m2]:
> >> + m.run("service irqbalance stop")
> >> + for dev in [m1.eth0, m1.eth1, m2.eth0]:
> >> self._pin_dev_interrupts(dev, self.params.dev_intr_cpu)
> >>
> >> return configuration
> >> @@ -67,5 +67,6 @@ class BondRecipe(BaseEnrtRecipe):
> >> m1, m2 = self.matched.m1, self.matched.m2
> >>
> >> #TODO better service handling through HostAPI
> >> - m1.run("service irqbalance start")
> >> - m2.run("service irqbalance start")
> >> + if self.params.dev_intr_cpu:
> >> + for m in [m1, m2]:
> >> + m.run("service irqbalance start")
> >> diff --git a/lnst/Recipes/ENRT/DoubleBondRecipe.py
> >> b/lnst/Recipes/ENRT/DoubleBondRecipe.py
> >> index b5b6512..2ddbec4 100644
> >> --- a/lnst/Recipes/ENRT/DoubleBondRecipe.py
> >> +++ b/lnst/Recipes/ENRT/DoubleBondRecipe.py
> >> @@ -54,11 +54,11 @@ class DoubleBondRecipe(BaseEnrtRecipe):
> >> m.bond.up()
> >>
> >> #TODO better service handling through HostAPI
> >> - m1.run("service irqbalance stop")
> >> - m2.run("service irqbalance stop")
> >> - for m in self.matched:
> >> - for dev in m.devices:
> >> - self._pin_dev_interrupts(dev, self.params.dev_intr_cpu)
> >> + if self.params.dev_intr_cpu:
> >> + for m in [m1, m2]:
> >> + m.run("service irqbalance stop")
> >> + for dev in [m.eth0, m.eth1]:
> >> + self._pin_dev_interrupts(m.eth0,
> >> self.params.dev_intr_cpu)
> >>
> >> return configuration
> >>
> >> @@ -66,5 +66,6 @@ class DoubleBondRecipe(BaseEnrtRecipe):
> >> m1, m2 = self.matched.m1, self.matched.m2
> >>
> >> #TODO better service handling through HostAPI
> >> - m1.run("service irqbalance start")
> >> - m2.run("service irqbalance start")
> >> + if self.params.dev_intr_cpu:
> >> + for m in [m1, m2]:
> >> + m.run("service irqbalance start")
> >> diff --git a/lnst/Recipes/ENRT/DoubleTeamRecipe.py
> >> b/lnst/Recipes/ENRT/DoubleTeamRecipe.py
> >> index 54b38de..e10ea79 100644
> >> --- a/lnst/Recipes/ENRT/DoubleTeamRecipe.py
> >> +++ b/lnst/Recipes/ENRT/DoubleTeamRecipe.py
> >> @@ -67,11 +67,11 @@ class DoubleTeamRecipe(BaseEnrtRecipe):
> >> m2.team.up()
> >>
> >> #TODO better service handling through HostAPI
> >> - m1.run("service irqbalance stop")
> >> - m2.run("service irqbalance stop")
> >> - for m in self.matched:
> >> - for dev in m.devices:
> >> - self._pin_dev_interrupts(dev, self.params.dev_intr_cpu)
> >> + if self.params.dev_intr_cpu:
> >> + for m in [m1, m2]:
> >> + m.run("service irqbalance stop")
> >> + for dev in [m.eth1, m.eth2]:
> >> + self._pin_dev_interrupts(dev,
> >> self.params.dev_intr_cpu)
> >>
> >> return configuration
> >>
> >> @@ -79,5 +79,6 @@ class DoubleTeamRecipe(BaseEnrtRecipe):
> >> m1, m2 = self.matched.m1, self.matched.m2
> >>
> >> #TODO better service handling through HostAPI
> >> - m1.run("service irqbalance start")
> >> - m2.run("service irqbalance start")
> >> + if self.params.dev_intr_cpu:
> >> + for m in [m1, m2]:
> >> + m.run("service irqbalance start")
> >> diff --git a/lnst/Recipes/ENRT/SimplePerfRecipe.py
> >> b/lnst/Recipes/ENRT/SimplePerfRecipe.py
> >> index 004e43d..92e1e1b 100644
> >> --- a/lnst/Recipes/ENRT/SimplePerfRecipe.py
> >> +++ b/lnst/Recipes/ENRT/SimplePerfRecipe.py
> >> @@ -52,11 +52,10 @@ class SimplePerfRecipe(BaseEnrtRecipe):
> >> m2.eth0.up()
> >>
> >> #TODO better service handling through HostAPI
> >> - m1.run("service irqbalance stop")
> >> - m2.run("service irqbalance stop")
> >> - for m in self.matched:
> >> - for dev in m.devices:
> >> - self._pin_dev_interrupts(dev, self.params.dev_intr_cpu)
> >> + if self.params.dev_intr_cpu:
> >> + for m in [m1, m2]:
> >> + m.run("service irqbalance stop")
> >> + self._pin_dev_interrupts(m.eth0,
> >> self.params.dev_intr_cpu)
> >>
> >> return configuration
> >>
> >> @@ -64,8 +63,9 @@ class SimplePerfRecipe(BaseEnrtRecipe):
> >> m1, m2 = self.matched.m1, self.matched.m2
> >>
> >> #TODO better service handling through HostAPI
> >> - m1.run("service irqbalance start")
> >> - m2.run("service irqbalance start")
> >> + if self.params.dev_intr_cpu:
> >> + for m in [m1, m2]:
> >> + m.run("service irqbalance start")
> >>
> >> # redo
> >> # m1.eth0.adaptive_tx_coalescing =
> >> self.saved_coalescing_state["m1_if"]["tx"]
> >> diff --git a/lnst/Recipes/ENRT/TeamRecipe.py
> >> b/lnst/Recipes/ENRT/TeamRecipe.py
> >> index 6173ec6..10546e6 100644
> >> --- a/lnst/Recipes/ENRT/TeamRecipe.py
> >> +++ b/lnst/Recipes/ENRT/TeamRecipe.py
> >> @@ -58,10 +58,10 @@ class TeamRecipe(BaseEnrtRecipe):
> >> m2.eth1.up()
> >>
> >> #TODO better service handling through HostAPI
> >> - m1.run("service irqbalance stop")
> >> - m2.run("service irqbalance stop")
> >> - for m in self.matched:
> >> - for dev in m.devices:
> >> + if self.params.dev_intr_cpu:
> >> + for m in [m1, m2]:
> >> + m.run("service irqbalance stop")
> >> + for dev in [m1.eth1, m1.eth2, m2.eth1]:
> >> self._pin_dev_interrupts(dev, self.params.dev_intr_cpu)
> >>
> >> return configuration
> >> @@ -70,5 +70,6 @@ class TeamRecipe(BaseEnrtRecipe):
> >> m1, m2 = self.matched.m1, self.matched.m2
> >>
> >> #TODO better service handling through HostAPI
> >> - m1.run("service irqbalance start")
> >> - m2.run("service irqbalance start")
> >> + if self.params.dev_intr_cpu
> >> + for m in [m1, m2]:
> >> + m.run("service irqbalance start")
> >> diff --git a/lnst/Recipes/ENRT/TeamVsBondRecipe.py
> >> b/lnst/Recipes/ENRT/TeamVsBondRecipe.py
> >> index fe39d13..612b5dc 100644
> >> --- a/lnst/Recipes/ENRT/TeamVsBondRecipe.py
> >> +++ b/lnst/Recipes/ENRT/TeamVsBondRecipe.py
> >> @@ -71,11 +71,11 @@ class TeamVsBondRecipe(BaseEnrtRecipe):
> >> m2.bond.up()
> >>
> >> #TODO better service handling through HostAPI
> >> - m1.run("service irqbalance stop")
> >> - m2.run("service irqbalance stop")
> >> - for m in self.matched:
> >> - for dev in m.devices:
> >> - self._pin_dev_interrupts(dev, self.params.dev_intr_cpu)
> >> + if self.params.dev_intr_cpu:
> >> + for m in [m1, m2]:
> >> + m.run("service irqbalance stop")
> >> + for dev in [m.eth1, m.eth2]:
> >> + self._pin_dev_interrupts(dev,
> >> self.params.dev_intr_cpu)
> >>
> >> return configuration
> >>
> >> @@ -83,5 +83,6 @@ class TeamVsBondRecipe(BaseEnrtRecipe):
> >> m1, m2 = self.matched.m1, self.matched.m2
> >>
> >> #TODO better service handling through HostAPI
> >> - m1.run("service irqbalance start")
> >> - m2.run("service irqbalance start")
> >> + if self.params.dev_intr_cpu
> >> + for m in [m1, m2]:
> >> + m.run("service irqbalance start")
> >> diff --git a/lnst/Recipes/ENRT/VirtualBridgeVlanInGuestMirroredRecipe.py
> >> b/lnst/Recipes/ENRT/VirtualBridgeVlanInGuestMirroredRecipe.py
> >> index 2fd74ae..47f53cf 100644
> >> --- a/lnst/Recipes/ENRT/VirtualBridgeVlanInGuestMirroredRecipe.py
> >> +++ b/lnst/Recipes/ENRT/VirtualBridgeVlanInGuestMirroredRecipe.py
> >> @@ -8,6 +8,7 @@ from lnst.Controller import HostReq, DeviceReq
> >> from lnst.Recipes.ENRT.BaseEnrtRecipe import BaseEnrtRecipe,
> >> EnrtConfiguration
> >> from lnst.Devices import VlanDevice
> >> from lnst.Devices import BridgeDevice
> >> +from lnst.Common.LnstError import LnstError
> >>
> >> class VirtualBridgeVlanInGuestMirroredRecipe(BaseEnrtRecipe):
> >> host1 = HostReq()
> >> @@ -92,14 +93,13 @@ class
> >> VirtualBridgeVlanInGuestMirroredRecipe(BaseEnrtRecipe):
> >> guest2.vlan1.up()
> >>
> >> #TODO better service handling through HostAPI
> >> - host1.run("service irqbalance stop")
> >> - host2.run("service irqbalance stop")
> >> - guest1.run("service irqbalance stop")
> >> - guest2.run("service irqbalance stop")
> >> + if self.params.perf_intr_cpu:
> >> + raise LnstError("'perf_intr_cpu' (%d) should not
be set for
> >> this test" % self.params.perf_intr_cpu)
> >>
> >> - for m in self.matched:
> >> - for dev in m.devices:
> >> - self._pin_dev_interrupts(dev, self.params.dev_intr_cpu)
> >> + if self.params.dev_intr_cpu:
> >> + for m in [host1, host2]:
> >> + m.run("service irqbalance stop")
> >> + self._pin_dev_interrupts(m.eth0,
> >> self.params.dev_intr_cpu)
> >
> >The old code also stopped irqbalance for guests. Is it intentional that
> >they're skipped here? It also happens in all the following recipes where
> >guests are involved.
> >
>
> I think that stopping irqbalance inside guests makes NO sense. Also the
> code on master branch stops the service on baremetal machines only.
>
> >>
> >> return configuration
> >>
> >> @@ -107,7 +107,6 @@ class
> >> VirtualBridgeVlanInGuestMirroredRecipe(BaseEnrtRecipe):
> >> host1, host2, guest1, guest2 = self.matched.host1,
> >> self.matched.host2, self.matched.guest1, self.matched.guest2
> >>
> >> #TODO better service handling through HostAPI
> >> - host1.run("service irqbalance start")
> >> - host2.run("service irqbalance start")
> >> - guest1.run("service irqbalance start")
> >> - guest2.run("service irqbalance start")
> >> + if self.params.dev_intr_cpu:
> >> + for m in [host1, host2]:
> >> + m.run("service irqbalance start")
> >> diff --git a/lnst/Recipes/ENRT/VirtualBridgeVlanInGuestRecipe.py
> >> b/lnst/Recipes/ENRT/VirtualBridgeVlanInGuestRecipe.py
> >> index 8bb7582..a72e8d4 100644
> >> --- a/lnst/Recipes/ENRT/VirtualBridgeVlanInGuestRecipe.py
> >> +++ b/lnst/Recipes/ENRT/VirtualBridgeVlanInGuestRecipe.py
> >> @@ -8,6 +8,7 @@ from lnst.Controller import HostReq, DeviceReq
> >> from lnst.Recipes.ENRT.BaseEnrtRecipe import BaseEnrtRecipe,
> >> EnrtConfiguration
> >> from lnst.Devices import VlanDevice
> >> from lnst.Devices import BridgeDevice
> >> +from lnst.Common.LnstError import LnstError
> >>
> >> class VirtualBridgeVlanInGuestRecipe(BaseEnrtRecipe):
> >> host1 = HostReq()
> >> @@ -74,13 +75,13 @@ class VirtualBridgeVlanInGuestRecipe(BaseEnrtRecipe):
> >> guest1.vlan1.up()
> >>
> >> #TODO better service handling through HostAPI
> >> - host1.run("service irqbalance stop")
> >> - host2.run("service irqbalance stop")
> >> - guest1.run("service irqbalance stop")
> >> + if self.params.dev_intr_cpu:
> >> + raise LnstError("'dev_intr_cpu' (%d) should not be
set for
> >> this test" % self.params.dev_intr_cpu)
> >
> >In the previous recipe it was the perf_intr_cpu that "should not be
> >set", is this intentional or a mistake? I also noticed it switches in
> >between a couple of times in the following recipes...
> >
>
> This ...
>
> >>
> >> - for m in self.matched:
> >> - for dev in m.devices:
> >> - self._pin_dev_interrupts(dev, self.params.dev_intr_cpu)
> >> + if self.params.perf_intr_cpu:
> >> + for m in [host1, host2]:
> >> + m.run("service irqbalance stop")
> >> + self._pin_dev_interrupts(m.eth0, 0)
> >
> >I'm also not sure about this pin devices to 0 in this case. The
> >parameter is not set indicating it shouldn't be touch but we still
> >configure it to 0?
> >
> >For now I'm just interested in if it's a mistake or intentional.
>
> and this ...
>
> I think this is a mistake in old master branch code for all non-mirrored
> tests that were left unmaintained. Many fixes went into mirrored
> versions but they did not land in the non-mirrored. As I wrote in the
> other email we should fix these tests to match the mirrored versions
> if we want to keep them.
>
> >
> >But I have this to say for a later discussion:
> >It kind of looks susipicous wihtout additional explanation somewhere.
> >Maybe we should take a global look at how to do interrupts and cpu
> >pinning and come up with some generic solution.
> >
> >At this point it also looks like there's a lot of code repetition in
> >individual recipes as well so we might be able to improve on this in a
> >later patchset.
>
> Agree.
>
> -Jan
>