diff mbox series

[ovs-dev,branch-2.14,2/3] netdev-offload-tc: Probe for support for any of the ct_state flags

Message ID 20210409133505.3691096-3-aconole@redhat.com
State Changes Requested
Headers show
Series Backport ct-offload state flag support | expand

Commit Message

Aaron Conole April 9, 2021, 1:35 p.m. UTC 
From: Paul Blakey <paulb@nvidia.com>

Upstream kernel now rejects unsupported ct_state flags.
Earlier kernels, ignored it but still echoed back the requested ct_state,
if ct_state was supported. ct_state initial support had trk, new, est,
and rel flags.

If kernel echos back ct_state, assume support for trk, new, est, and
rel. If kernel rejects a specific unsupported flag, continue and
use reject mechanisim to probe for flags rep and inv.

Disallow inserting rules with unnsupported ct_state flags.

Signed-off-by: Paul Blakey <paulb@nvidia.com>
Acked-by: Roi Dayan <roid@nvidia.com>
Reviewed-by: Marcelo Ricardo Leitner <marcelo.leitner@gmail.com>
Signed-off-by: Simon Horman <simon.horman@netronome.com>
(cherry picked from commit 1e4aa061ac8b71196c0b0c8ab23d1627cd2e4a27)
Signed-off-by: Aaron Conole <aconole@redhat.com>
---
 lib/netdev-offload-tc.c | 205 ++++++++++++++++++++++++++++++----------
 1 file changed, 157 insertions(+), 48 deletions(-)

Comments

0-day Robot April 9, 2021, 2:01 p.m. UTC | #1 
Bleep bloop.  Greetings Aaron Conole, I am a robot and I have tried out your patch.
Thanks for your contribution.

I encountered some error that I wasn't expecting.  See the details below.


checkpatch:
WARNING: Unexpected sign-offs from developers who are not authors or co-authors or committers: Simon Horman <simon.horman@netronome.com>, Aaron Conole <aconole@redhat.com>
Lines checked: 280, Warnings: 1, Errors: 0


build:
depbase=`echo lib/if-notifier.lo | sed 's|[^/]*$|.deps/&|;s|\.lo$||'`;\
/bin/sh ./libtool  --tag=CC   --mode=compile gcc -std=gnu99 -DHAVE_CONFIG_H -I.    -I ./include -I ./include -I ./lib -I ./lib    -Wstrict-prototypes -Wall -Wextra -Wno-sign-compare -Wpointer-arith -Wformat -Wformat-security -Wswitch-enum -Wunused-parameter -Wbad-function-cast -Wcast-align -Wstrict-prototypes -Wold-style-definition -Wmissing-prototypes -Wmissing-field-initializers -fno-strict-aliasing -Wshadow -Werror -Werror   -g -O2 -DHAVE_LD_AVX512_GOOD -MT lib/if-notifier.lo -MD -MP -MF $depbase.Tpo -c -o lib/if-notifier.lo lib/if-notifier.c &&\
mv -f $depbase.Tpo $depbase.Plo
libtool: compile:  gcc -std=gnu99 -DHAVE_CONFIG_H -I. -I ./include -I ./include -I ./lib -I ./lib -Wstrict-prototypes -Wall -Wextra -Wno-sign-compare -Wpointer-arith -Wformat -Wformat-security -Wswitch-enum -Wunused-parameter -Wbad-function-cast -Wcast-align -Wstrict-prototypes -Wold-style-definition -Wmissing-prototypes -Wmissing-field-initializers -fno-strict-aliasing -Wshadow -Werror -Werror -g -O2 -DHAVE_LD_AVX512_GOOD -MT lib/if-notifier.lo -MD -MP -MF lib/.deps/if-notifier.Tpo -c lib/if-notifier.c -o lib/if-notifier.o
depbase=`echo lib/netdev-linux.lo | sed 's|[^/]*$|.deps/&|;s|\.lo$||'`;\
/bin/sh ./libtool  --tag=CC   --mode=compile gcc -std=gnu99 -DHAVE_CONFIG_H -I.    -I ./include -I ./include -I ./lib -I ./lib    -Wstrict-prototypes -Wall -Wextra -Wno-sign-compare -Wpointer-arith -Wformat -Wformat-security -Wswitch-enum -Wunused-parameter -Wbad-function-cast -Wcast-align -Wstrict-prototypes -Wold-style-definition -Wmissing-prototypes -Wmissing-field-initializers -fno-strict-aliasing -Wshadow -Werror -Werror   -g -O2 -DHAVE_LD_AVX512_GOOD -MT lib/netdev-linux.lo -MD -MP -MF $depbase.Tpo -c -o lib/netdev-linux.lo lib/netdev-linux.c &&\
mv -f $depbase.Tpo $depbase.Plo
libtool: compile:  gcc -std=gnu99 -DHAVE_CONFIG_H -I. -I ./include -I ./include -I ./lib -I ./lib -Wstrict-prototypes -Wall -Wextra -Wno-sign-compare -Wpointer-arith -Wformat -Wformat-security -Wswitch-enum -Wunused-parameter -Wbad-function-cast -Wcast-align -Wstrict-prototypes -Wold-style-definition -Wmissing-prototypes -Wmissing-field-initializers -fno-strict-aliasing -Wshadow -Werror -Werror -g -O2 -DHAVE_LD_AVX512_GOOD -MT lib/netdev-linux.lo -MD -MP -MF lib/.deps/netdev-linux.Tpo -c lib/netdev-linux.c -o lib/netdev-linux.o
depbase=`echo lib/netdev-offload-tc.lo | sed 's|[^/]*$|.deps/&|;s|\.lo$||'`;\
/bin/sh ./libtool  --tag=CC   --mode=compile gcc -std=gnu99 -DHAVE_CONFIG_H -I.    -I ./include -I ./include -I ./lib -I ./lib    -Wstrict-prototypes -Wall -Wextra -Wno-sign-compare -Wpointer-arith -Wformat -Wformat-security -Wswitch-enum -Wunused-parameter -Wbad-function-cast -Wcast-align -Wstrict-prototypes -Wold-style-definition -Wmissing-prototypes -Wmissing-field-initializers -fno-strict-aliasing -Wshadow -Werror -Werror   -g -O2 -DHAVE_LD_AVX512_GOOD -MT lib/netdev-offload-tc.lo -MD -MP -MF $depbase.Tpo -c -o lib/netdev-offload-tc.lo lib/netdev-offload-tc.c &&\
mv -f $depbase.Tpo $depbase.Plo
libtool: compile:  gcc -std=gnu99 -DHAVE_CONFIG_H -I. -I ./include -I ./include -I ./lib -I ./lib -Wstrict-prototypes -Wall -Wextra -Wno-sign-compare -Wpointer-arith -Wformat -Wformat-security -Wswitch-enum -Wunused-parameter -Wbad-function-cast -Wcast-align -Wstrict-prototypes -Wold-style-definition -Wmissing-prototypes -Wmissing-field-initializers -fno-strict-aliasing -Wshadow -Werror -Werror -g -O2 -DHAVE_LD_AVX512_GOOD -MT lib/netdev-offload-tc.lo -MD -MP -MF lib/.deps/netdev-offload-tc.Tpo -c lib/netdev-offload-tc.c -o lib/netdev-offload-tc.o
lib/netdev-offload-tc.c: In function 'probe_insert_ct_state_rule':
lib/netdev-offload-tc.c:1983:11: error: 'struct tc_flower' has no member named 'tc_policy'
     flower.tc_policy = TC_POLICY_SKIP_HW;
           ^
lib/netdev-offload-tc.c:1983:24: error: 'TC_POLICY_SKIP_HW' undeclared (first use in this function)
     flower.tc_policy = TC_POLICY_SKIP_HW;
                        ^
lib/netdev-offload-tc.c:1983:24: note: each undeclared identifier is reported only once for each function it appears in
make[2]: *** [lib/netdev-offload-tc.lo] Error 1
make[2]: Leaving directory `/var/lib/jenkins/jobs/0day_robot_upstream_build_from_pw/workspace'
make[1]: *** [all-recursive] Error 1
make[1]: Leaving directory `/var/lib/jenkins/jobs/0day_robot_upstream_build_from_pw/workspace'
make: *** [all] Error 2


Please check this out.  If you feel there has been an error, please email aconole@redhat.com

Thanks,
0-day Robot
Marcelo Ricardo Leitner April 15, 2021, 5:23 p.m. UTC | #2 
On Fri, Apr 09, 2021 at 09:35:04AM -0400, Aaron Conole wrote:
> +static int
> +probe_insert_ct_state_rule(int ifindex, uint16_t ct_state, struct tcf_id *id)
> +{
> +    int prio = TC_RESERVED_PRIORITY_MAX + 1;
> +    struct tc_flower flower;
> +
> +    memset(&flower, 0, sizeof flower);
> +    flower.key.ct_state = ct_state;
> +    flower.mask.ct_state = ct_state;
> +    flower.tc_policy = TC_POLICY_SKIP_HW;

This field is introduced in patch 3/3 here and with that, if doing a
bisect or so, the line above will fail to compile. It's best if patch
3 is moved to the front of the patchset, like it is on the 2.13
patchset.

Btw, I liked how the backports are going to be probe for the new
features but not use them. That might give useful debugging
information later on.

  Marcelo
Aaron Conole April 15, 2021, 8:22 p.m. UTC | #3 
Marcelo Leitner <marcelo.leitner@gmail.com> writes:

> On Fri, Apr 09, 2021 at 09:35:04AM -0400, Aaron Conole wrote:
>> +static int
>> +probe_insert_ct_state_rule(int ifindex, uint16_t ct_state, struct tcf_id *id)
>> +{
>> +    int prio = TC_RESERVED_PRIORITY_MAX + 1;
>> +    struct tc_flower flower;
>> +
>> +    memset(&flower, 0, sizeof flower);
>> +    flower.key.ct_state = ct_state;
>> +    flower.mask.ct_state = ct_state;
>> +    flower.tc_policy = TC_POLICY_SKIP_HW;
>
> This field is introduced in patch 3/3 here and with that, if doing a
> bisect or so, the line above will fail to compile. It's best if patch
> 3 is moved to the front of the patchset, like it is on the 2.13
> patchset.

Oops... I had moved them, but forgot to regenerate the series.  I'll
repost 2.14 branch.

> Btw, I liked how the backports are going to be probe for the new
> features but not use them. That might give useful debugging
> information later on.
>
>   Marcelo
diff mbox series

Patch

diff --git a/lib/netdev-offload-tc.c b/lib/netdev-offload-tc.c
index 5bc6e5aa1a..0da8abe45b 100644
--- a/lib/netdev-offload-tc.c
+++ b/lib/netdev-offload-tc.c
@@ -48,6 +48,7 @@  static struct hmap ufid_to_tc = HMAP_INITIALIZER(&ufid_to_tc);
 static struct hmap tc_to_ufid = HMAP_INITIALIZER(&tc_to_ufid);
 static bool multi_mask_per_prio = false;
 static bool block_support = false;
+static uint16_t ct_state_support;
 
 struct netlink_field {
     int offset;
@@ -1406,6 +1407,66 @@  flower_match_to_tun_opt(struct tc_flower *flower, const struct flow_tnl *tnl,
     flower->mask.tunnel.metadata.present.len = tnl->metadata.present.len;
 }
 
+static void
+parse_match_ct_state_to_flower(struct tc_flower *flower, struct match *match)
+{
+    const struct flow *key = &match->flow;
+    struct flow *mask = &match->wc.masks;
+
+    if (!ct_state_support) {
+        return;
+    }
+
+    if ((ct_state_support & mask->ct_state) == mask->ct_state) {
+        if (mask->ct_state & OVS_CS_F_NEW) {
+            if (key->ct_state & OVS_CS_F_NEW) {
+                flower->key.ct_state |= TCA_FLOWER_KEY_CT_FLAGS_NEW;
+            }
+            flower->mask.ct_state |= TCA_FLOWER_KEY_CT_FLAGS_NEW;
+            mask->ct_state &= ~OVS_CS_F_NEW;
+        }
+
+        if (mask->ct_state & OVS_CS_F_ESTABLISHED) {
+            if (key->ct_state & OVS_CS_F_ESTABLISHED) {
+                flower->key.ct_state |= TCA_FLOWER_KEY_CT_FLAGS_ESTABLISHED;
+            }
+            flower->mask.ct_state |= TCA_FLOWER_KEY_CT_FLAGS_ESTABLISHED;
+            mask->ct_state &= ~OVS_CS_F_ESTABLISHED;
+        }
+
+        if (mask->ct_state & OVS_CS_F_TRACKED) {
+            if (key->ct_state & OVS_CS_F_TRACKED) {
+                flower->key.ct_state |= TCA_FLOWER_KEY_CT_FLAGS_TRACKED;
+            }
+            flower->mask.ct_state |= TCA_FLOWER_KEY_CT_FLAGS_TRACKED;
+            mask->ct_state &= ~OVS_CS_F_TRACKED;
+        }
+
+        if (flower->key.ct_state & TCA_FLOWER_KEY_CT_FLAGS_ESTABLISHED) {
+            flower->key.ct_state &= ~(TCA_FLOWER_KEY_CT_FLAGS_NEW);
+            flower->mask.ct_state &= ~(TCA_FLOWER_KEY_CT_FLAGS_NEW);
+        }
+    }
+
+    if (mask->ct_zone) {
+        flower->key.ct_zone = key->ct_zone;
+        flower->mask.ct_zone = mask->ct_zone;
+        mask->ct_zone = 0;
+    }
+
+    if (mask->ct_mark) {
+        flower->key.ct_mark = key->ct_mark;
+        flower->mask.ct_mark = mask->ct_mark;
+        mask->ct_mark = 0;
+    }
+
+    if (!ovs_u128_is_zero(mask->ct_label)) {
+        flower->key.ct_label = key->ct_label;
+        flower->mask.ct_label = mask->ct_label;
+        mask->ct_label = OVS_U128_ZERO;
+    }
+}
+
 static int
 netdev_tc_flow_put(struct netdev *netdev, struct match *match,
                    struct nlattr *actions, size_t actions_len,
@@ -1650,54 +1711,7 @@  netdev_tc_flow_put(struct netdev *netdev, struct match *match,
         }
     }
 
-    if (mask->ct_state) {
-        if (mask->ct_state & OVS_CS_F_NEW) {
-            if (key->ct_state & OVS_CS_F_NEW) {
-                flower.key.ct_state |= TCA_FLOWER_KEY_CT_FLAGS_NEW;
-            }
-            flower.mask.ct_state |= TCA_FLOWER_KEY_CT_FLAGS_NEW;
-            mask->ct_state &= ~OVS_CS_F_NEW;
-        }
-
-        if (mask->ct_state & OVS_CS_F_ESTABLISHED) {
-            if (key->ct_state & OVS_CS_F_ESTABLISHED) {
-                flower.key.ct_state |= TCA_FLOWER_KEY_CT_FLAGS_ESTABLISHED;
-            }
-            flower.mask.ct_state |= TCA_FLOWER_KEY_CT_FLAGS_ESTABLISHED;
-            mask->ct_state &= ~OVS_CS_F_ESTABLISHED;
-        }
-
-        if (mask->ct_state & OVS_CS_F_TRACKED) {
-            if (key->ct_state & OVS_CS_F_TRACKED) {
-                flower.key.ct_state |= TCA_FLOWER_KEY_CT_FLAGS_TRACKED;
-            }
-            flower.mask.ct_state |= TCA_FLOWER_KEY_CT_FLAGS_TRACKED;
-            mask->ct_state &= ~OVS_CS_F_TRACKED;
-        }
-
-        if (flower.key.ct_state & TCA_FLOWER_KEY_CT_FLAGS_ESTABLISHED) {
-            flower.key.ct_state &= ~(TCA_FLOWER_KEY_CT_FLAGS_NEW);
-            flower.mask.ct_state &= ~(TCA_FLOWER_KEY_CT_FLAGS_NEW);
-        }
-    }
-
-    if (mask->ct_zone) {
-        flower.key.ct_zone = key->ct_zone;
-        flower.mask.ct_zone = mask->ct_zone;
-        mask->ct_zone = 0;
-    }
-
-    if (mask->ct_mark) {
-        flower.key.ct_mark = key->ct_mark;
-        flower.mask.ct_mark = mask->ct_mark;
-        mask->ct_mark = 0;
-    }
-
-    if (!ovs_u128_is_zero(mask->ct_label)) {
-        flower.key.ct_label = key->ct_label;
-        flower.mask.ct_label = mask->ct_label;
-        mask->ct_label = OVS_U128_ZERO;
-    }
+    parse_match_ct_state_to_flower(&flower, match);
 
     /* ignore exact match on skb_mark of 0. */
     if (mask->pkt_mark == UINT32_MAX && !key->pkt_mark) {
@@ -1779,6 +1793,10 @@  netdev_tc_flow_put(struct netdev *netdev, struct match *match,
             const struct nlattr *ct = nl_attr_get(nla);
             const size_t ct_len = nl_attr_get_size(nla);
 
+            if (!ct_state_support) {
+                return -EOPNOTSUPP;
+            }
+
             err = parse_put_flow_ct_action(&flower, action, ct, ct_len);
             if (err) {
                 return err;
@@ -1952,6 +1970,96 @@  out:
     tc_add_del_qdisc(ifindex, false, block_id, TC_INGRESS);
 }
 
+
+static int
+probe_insert_ct_state_rule(int ifindex, uint16_t ct_state, struct tcf_id *id)
+{
+    int prio = TC_RESERVED_PRIORITY_MAX + 1;
+    struct tc_flower flower;
+
+    memset(&flower, 0, sizeof flower);
+    flower.key.ct_state = ct_state;
+    flower.mask.ct_state = ct_state;
+    flower.tc_policy = TC_POLICY_SKIP_HW;
+    flower.key.eth_type = htons(ETH_P_IP);
+    flower.mask.eth_type = OVS_BE16_MAX;
+
+    *id = tc_make_tcf_id(ifindex, 0, prio, TC_INGRESS);
+    return tc_replace_flower(id, &flower);
+}
+
+static void
+probe_ct_state_support(int ifindex)
+{
+    struct tc_flower flower;
+    uint16_t ct_state;
+    struct tcf_id id;
+    int error;
+
+    error = tc_add_del_qdisc(ifindex, true, 0, TC_INGRESS);
+    if (error) {
+        return;
+    }
+
+    /* Test for base ct_state match support */
+    ct_state = TCA_FLOWER_KEY_CT_FLAGS_NEW | TCA_FLOWER_KEY_CT_FLAGS_TRACKED;
+    error = probe_insert_ct_state_rule(ifindex, ct_state, &id);
+    if (error) {
+        goto out;
+    }
+
+    error = tc_get_flower(&id, &flower);
+    if (error || flower.mask.ct_state != ct_state) {
+        goto out_del;
+    }
+
+    tc_del_filter(&id);
+    ct_state_support = OVS_CS_F_NEW |
+                       OVS_CS_F_ESTABLISHED |
+                       OVS_CS_F_TRACKED |
+                       OVS_CS_F_RELATED;
+
+    /* Test for reject, ct_state >= MAX */
+    ct_state = ~0;
+    error = probe_insert_ct_state_rule(ifindex, ct_state, &id);
+    if (!error) {
+        /* No reject, can't continue probing other flags */
+        goto out_del;
+    }
+
+    tc_del_filter(&id);
+
+    /* Test for ct_state INVALID support */
+    memset(&flower, 0, sizeof flower);
+    ct_state = TCA_FLOWER_KEY_CT_FLAGS_TRACKED |
+               TCA_FLOWER_KEY_CT_FLAGS_INVALID;
+    error = probe_insert_ct_state_rule(ifindex, ct_state, &id);
+    if (error) {
+        goto out;
+    }
+
+    tc_del_filter(&id);
+    ct_state_support |= OVS_CS_F_INVALID;
+
+    /* Test for ct_state REPLY support */
+    memset(&flower, 0, sizeof flower);
+    ct_state = TCA_FLOWER_KEY_CT_FLAGS_TRACKED |
+               TCA_FLOWER_KEY_CT_FLAGS_ESTABLISHED |
+               TCA_FLOWER_KEY_CT_FLAGS_REPLY;
+    error = probe_insert_ct_state_rule(ifindex, ct_state, &id);
+    if (error) {
+        goto out;
+    }
+
+    ct_state_support |= OVS_CS_F_REPLY_DIR;
+
+out_del:
+    tc_del_filter(&id);
+out:
+    tc_add_del_qdisc(ifindex, false, 0, TC_INGRESS);
+    VLOG_INFO("probe tc: supported ovs ct_state bits: 0x%x", ct_state_support);
+}
+
 static void
 probe_tc_block_support(int ifindex)
 {
@@ -2022,6 +2130,7 @@  netdev_tc_init_flow_api(struct netdev *netdev)
 
     if (ovsthread_once_start(&multi_mask_once)) {
         probe_multi_mask_per_prio(ifindex);
+        probe_ct_state_support(ifindex);
         ovsthread_once_done(&multi_mask_once);
     }