On Tue, 2 Feb 2021, Marcelo Leitner wrote:
> On Tue, Feb 02, 2021 at 04:59:44PM +0100, Ilya Maximets wrote:
> > On 2/2/21 4:52 PM, wenxu wrote:
> > >
> > > Hi,
> > >
> > > just ingore my patch. Now kernel can support match invalid
> > > ct_state in th tc flower.
> >
> > I don't think that we can ignore it. At least we need this
> > fix on stable branches. And also there are other conntrack
> > flags that are simply ignored, not only inv and rpl. These
> > are snat, dnat, rel, probably some other. So, we need this
> > change in some form anyway.
>
> That would be great, indeed.
>
> The issue is also present in tc layer, btw, but maybe Paul has his on
> the charts already. The effect here could be that if you use a newer
> ovs that supports inv ct_state, for example, up to wenxu's patch the
> kernel will simply ignore it.
>
> Best regards,
> Marcelo
Hi,
Regarding the inv flag, yes if you offload a +inv rule, and not have
an updated kernel then tc rule won't match it and it will fall back to
software. Drivers should see this flag and reject offload, same with rpl.
We can probe for support by adding a ct_state rule with +inv,+rpl and see
the echo back. Add that for V2?
As for missing other flags, I suggest to add a check that all flags were
parsed, the same as we do with
match's mask, set translated flags mask to 0, and then check if we
missed some new flag by checking mask the ct_state mask was zeroed.
Like this:
----
>From 48a4cb1b537d3837df8c40c9c265ae95e9729255 Mon Sep 17 00:00:00 2001
From: Paul Blakey <[email protected]>
Date: Wed, 3 Feb 2021 10:27:21 +0200
Subject: [PATCH] netdev-offload-tc: Check for unparsed ct_state flags
Don't offload flows where we didn't fully parse ct_state flags.
Signed-off-by: Paul Blakey <[email protected]>
---
lib/netdev-offload-tc.c | 6 +++++-
1 file changed, 5 insertions(+), 1 deletion(-)
diff --git a/lib/netdev-offload-tc.c b/lib/netdev-offload-tc.c
index 08a4735..4362c6e 100644
--- a/lib/netdev-offload-tc.c
+++ b/lib/netdev-offload-tc.c
@@ -1705,7 +1705,11 @@ netdev_tc_flow_put(struct netdev *netdev, struct match
*match,
flower.mask.ct_state |= TCA_FLOWER_KEY_CT_FLAGS_INVALID;
}
- mask->ct_state = 0;
+ mask->ct_state &= ~(OVS_CS_F_INVALID |
+ OVS_CS_F_REPLY_DIR |
+ OVS_CS_F_TRACKED |
+ OVS_CS_F_ESTABLISHED |
+ OVS_CS_F_NEW);
}
if (mask->ct_zone) {
--
Does that sound good to you?
If so, as standalone patch, or in this patchset (will rebase the above as
first patch in this series)?
Thanks,
Paul.
>
> >
> > Best regards, Ilya Maximets.
> >
> > >
> > > BR
> > > wenxu
> > >
> > >
> > > From: Ilya Maximets <[email protected]>
> > > Date: 2021-02-02 23:33:41
> > > To: Paul Blakey <[email protected]>,[email protected]
> > > Cc: Oz Shlomo <[email protected]>,[email protected],Marcelo Leitner
> > > <[email protected]>,wenxu <[email protected]>
> > > Subject: Re: [ovs-dev] [PATCH 0/2] Add offload support for ct_state rpl
> > > and inv flags>On 2/2/21 1:47 PM, Paul Blakey wrote:
> > >>> Add offload support for ct_state rpl and inv flags.
> > >>
> > >>Hi.
> > >>
> > >>Since you're working on this, could you, please, review the following
> > >>patch:
> > >>
> > >> http://patchwork.ozlabs.org/project/openvswitch/patch/[email protected]/
> > >>
> > >>It seems like current OVS just ignores all the flags that can not
> > >>be passed to TC and this doesn't look right.
> > >>
> > >>>
> > >>> For example:
> > >>> ovs-ofctl del-flows br-ovs
> > >>> ovs-ofctl add-flow br-ovs arp,actions=normal
> > >>> ovs-ofctl add-flow br-ovs "table=0, ip,ct_state=-trk
> > >>> actions=ct(table=1,zone=5)"
> > >>> ovs-ofctl add-flow br-ovs "table=1, ip,ct_state=+trk+new
> > >>> actions=ct(zone=5, commit),normal"
> > >>> ovs-ofctl add-flow br-ovs "table=1, ip,ct_zone=5,ct_state=+trk+est+rpl
> > >>> actions=normal"
> > >>> ovs-ofctl add-flow br-ovs "table=1, ip,ct_zone=5,ct_state=+trk+est-rpl
> > >>> actions=normal"
> > >>>
> > >>> Paul Blakey (2):
> > >>> compat: Add TCA_FLOWER_KEY_CT_FLAGS_REPLY/INVALID definitions
> > >>> netdev-offload-tc: Add support for ct_state rpl and inv flags
> > >>>
> > >>> acinclude.m4 | 6 +++---
> > >>> include/linux/pkt_cls.h | 4 +++-
> > >>> lib/netdev-offload-tc.c | 28 ++++++++++++++++++++++++++++
> > >>> 3 files changed, 34 insertions(+), 4 deletions(-)
> > >>>
> > >>
> > >
> > >
> >
>
> _______________________________________________
> dev mailing list
> [email protected]
> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
>
_______________________________________________
dev mailing list
[email protected]
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
