| Message ID | 1502885697-4870-2-git-send-email-roid@mellanox.com |
|---|---|
| State | Changes Requested |
| Headers | show |
On 16 August 2017 at 05:14, Roi Dayan <roid@mellanox.com> wrote: > Doing dump-flows also altering the netdev ports list. > So doing it pre the actual test is adding a check to > make sure we don't break the that list. > > Signed-off-by: Roi Dayan <roid@mellanox.com> > Reviewed-by: Paul Blakey <paulb@mellanox.com> I'm actually not sure what the requirements are to run these offload tests. I tried running them on a 4.4 kernel, and the first test passed while the second failed; I assume that this is because 4.4's TC flower support is not new enough. Then I tried with a 4.12 kernel and neither test passed, and I have extra flows being reported in the dump-flows output. I believe that I understand what this patch is trying to achieve, but I don't know how I'm supposed to validate it.
On 17/08/2017 01:17, Joe Stringer wrote: > On 16 August 2017 at 05:14, Roi Dayan <roid@mellanox.com> wrote: >> Doing dump-flows also altering the netdev ports list. >> So doing it pre the actual test is adding a check to >> make sure we don't break the that list. >> >> Signed-off-by: Roi Dayan <roid@mellanox.com> >> Reviewed-by: Paul Blakey <paulb@mellanox.com> > > I'm actually not sure what the requirements are to run these offload > tests. I tried running them on a 4.4 kernel, and the first test passed > while the second failed; I assume that this is because 4.4's TC flower > support is not new enough. > > Then I tried with a 4.12 kernel and neither test passed, and I have > extra flows being reported in the dump-flows output. > > I believe that I understand what this patch is trying to achieve, but > I don't know how I'm supposed to validate it. > strange. there are no special requirements and this test should work with kernel 4.10 and up I think. Without the fix the first pass and the second fails. With the fix the first and second pass. I currently I test using kernel 4.13-rc2 and this is my output: before the fix: # make check-offloads .. datapath offloads 1: offloads - ping between two ports - offloads disabled ok 2: offloads - ping between two ports - offloads enabled FAILED (system-offloads-traffic.at:55) after the fix: # make check-offloads .. datapath offloads 1: offloads - ping between two ports - offloads disabled ok 2: offloads - ping between two ports - offloads enabled ok
On 17/08/2017 08:32, Roi Dayan wrote: > > > On 17/08/2017 01:17, Joe Stringer wrote: >> On 16 August 2017 at 05:14, Roi Dayan <roid@mellanox.com> wrote: >>> Doing dump-flows also altering the netdev ports list. >>> So doing it pre the actual test is adding a check to >>> make sure we don't break the that list. >>> >>> Signed-off-by: Roi Dayan <roid@mellanox.com> >>> Reviewed-by: Paul Blakey <paulb@mellanox.com> >> >> I'm actually not sure what the requirements are to run these offload >> tests. I tried running them on a 4.4 kernel, and the first test passed >> while the second failed; I assume that this is because 4.4's TC flower >> support is not new enough. >> >> Then I tried with a 4.12 kernel and neither test passed, and I have >> extra flows being reported in the dump-flows output. >> >> I believe that I understand what this patch is trying to achieve, but >> I don't know how I'm supposed to validate it. >> > In the past you had an issue that cls_flower was not configured in your kernel. could be the same issue now? > strange. there are no special requirements and this test should work > with kernel 4.10 and up I think. > Without the fix the first pass and the second fails. > With the fix the first and second pass. > > I currently I test using kernel 4.13-rc2 and this is my output: > > > before the fix: > > > # make check-offloads > .. > datapath offloads > > 1: offloads - ping between two ports - offloads disabled ok > 2: offloads - ping between two ports - offloads enabled FAILED > (system-offloads-traffic.at:55) > > > > after the fix: > > > # make check-offloads > .. > datapath offloads > > 1: offloads - ping between two ports - offloads disabled ok > 2: offloads - ping between two ports - offloads enabled ok >
On 17 August 2017 at 00:17, Roi Dayan <roid@mellanox.com> wrote: > > > On 17/08/2017 08:32, Roi Dayan wrote: >> >> >> >> On 17/08/2017 01:17, Joe Stringer wrote: >>> >>> On 16 August 2017 at 05:14, Roi Dayan <roid@mellanox.com> wrote: >>>> >>>> Doing dump-flows also altering the netdev ports list. >>>> So doing it pre the actual test is adding a check to >>>> make sure we don't break the that list. >>>> >>>> Signed-off-by: Roi Dayan <roid@mellanox.com> >>>> Reviewed-by: Paul Blakey <paulb@mellanox.com> >>> >>> >>> I'm actually not sure what the requirements are to run these offload >>> tests. I tried running them on a 4.4 kernel, and the first test passed >>> while the second failed; I assume that this is because 4.4's TC flower >>> support is not new enough. >>> >>> Then I tried with a 4.12 kernel and neither test passed, and I have >>> extra flows being reported in the dump-flows output. >>> >>> I believe that I understand what this patch is trying to achieve, but >>> I don't know how I'm supposed to validate it. >>> >> > > In the past you had an issue that cls_flower was not configured > in your kernel. could be the same issue now? Maybe it is, I changed which setup I was testing in. I'll double-check.
On 17 August 2017 at 11:07, Joe Stringer <joe@ovn.org> wrote: > On 17 August 2017 at 00:17, Roi Dayan <roid@mellanox.com> wrote: >> >> >> On 17/08/2017 08:32, Roi Dayan wrote: >>> >>> >>> >>> On 17/08/2017 01:17, Joe Stringer wrote: >>>> >>>> On 16 August 2017 at 05:14, Roi Dayan <roid@mellanox.com> wrote: >>>>> >>>>> Doing dump-flows also altering the netdev ports list. >>>>> So doing it pre the actual test is adding a check to >>>>> make sure we don't break the that list. >>>>> >>>>> Signed-off-by: Roi Dayan <roid@mellanox.com> >>>>> Reviewed-by: Paul Blakey <paulb@mellanox.com> >>>> >>>> >>>> I'm actually not sure what the requirements are to run these offload >>>> tests. I tried running them on a 4.4 kernel, and the first test passed >>>> while the second failed; I assume that this is because 4.4's TC flower >>>> support is not new enough. >>>> >>>> Then I tried with a 4.12 kernel and neither test passed, and I have >>>> extra flows being reported in the dump-flows output. >>>> >>>> I believe that I understand what this patch is trying to achieve, but >>>> I don't know how I'm supposed to validate it. >>>> >>> >> >> In the past you had an issue that cls_flower was not configured >> in your kernel. could be the same issue now? > > Maybe it is, I changed which setup I was testing in. I'll double-check. Managed to figure it out, looks good. Applied to master.
diff --git a/tests/system-offloads-traffic.at b/tests/system-offloads-traffic.at index 7aec8a3..1f80793 100644 --- a/tests/system-offloads-traffic.at +++ b/tests/system-offloads-traffic.at @@ -46,6 +46,7 @@ ADD_NAMESPACES(at_ns0, at_ns1) ADD_VETH(p0, at_ns0, br0, "10.1.1.1/24") ADD_VETH(p1, at_ns1, br0, "10.1.1.2/24") +AT_CHECK([ovs-appctl dpctl/dump-flows], [0], [ignore]) NS_CHECK_EXEC([at_ns0], [ping -q -c 10 -i 0.1 -w 2 10.1.1.2 | FORMAT_PING], [0], [dnl 10 packets transmitted, 10 received, 0% packet loss, time 0ms