Message ID | 20210409133505.3691096-3-aconole@redhat.com |
---|---|
State | Changes Requested |
Headers | show |
Series | Backport ct-offload state flag support | expand |
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
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
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 --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); }