| Message ID | 1611045110-682-1-git-send-email-wenxu@ucloud.cn |
|---|---|
| State | New |
| Headers | show |
| Series | [v2,net-next] net/sched: cls_flower add CT_FLAGS_INVALID flag support | expand |
Acked-by: Marcelo Ricardo Leitner <marcelo.leitner@gmail.com> The patch looks good to me, just some side comments below. On Tue, Jan 19, 2021 at 04:31:50PM +0800, wenxu@ucloud.cn wrote: > --- a/net/core/dev.c > +++ b/net/core/dev.c > @@ -3878,6 +3878,7 @@ int dev_loopback_xmit(struct net *net, struct sock *sk, struct sk_buff *skb) I don't know why, but your patches often have function names here that are not accurate. > > /* qdisc_skb_cb(skb)->pkt_len was already set by the caller. */ > qdisc_skb_cb(skb)->mru = 0; > + qdisc_skb_cb(skb)->post_ct = false; > mini_qdisc_bstats_cpu_update(miniq, skb); > > switch (tcf_classify(skb, miniq->filter_list, &cl_res, false)) { > @@ -4960,6 +4961,7 @@ static __latent_entropy void net_tx_action(struct softirq_action *h) Here as well. > > qdisc_skb_cb(skb)->pkt_len = skb->len; > qdisc_skb_cb(skb)->mru = 0; > + qdisc_skb_cb(skb)->post_ct = false; > skb->tc_at_ingress = 1; > mini_qdisc_bstats_cpu_update(miniq, skb); > > diff --git a/net/core/flow_dissector.c b/net/core/flow_dissector.c > index 2d70ded..c565c7a 100644 > --- a/net/core/flow_dissector.c > +++ b/net/core/flow_dissector.c > @@ -237,9 +237,8 @@ void skb_flow_dissect_meta(const struct sk_buff *skb, Here, I would expect to see a label/function name just before the skb_flow_dissect_ct definition. But that's skb_flow_dissect_set_enc_addr_type. skb_flow_dissect_meta is still one other function up. > void > skb_flow_dissect_ct(const struct sk_buff *skb, > struct flow_dissector *flow_dissector, > - void *target_container, > - u16 *ctinfo_map, > - size_t mapsize) > + void *target_container, u16 *ctinfo_map, > + size_t mapsize, bool post_ct) > { > #if IS_ENABLED(CONFIG_NF_CONNTRACK) > struct flow_dissector_key_ct *key;
On Tue, Jan 19, 2021 at 12:33 AM <wenxu@ucloud.cn> wrote: > diff --git a/net/core/flow_dissector.c b/net/core/flow_dissector.c > index 2d70ded..c565c7a 100644 > --- a/net/core/flow_dissector.c > +++ b/net/core/flow_dissector.c > @@ -237,9 +237,8 @@ void skb_flow_dissect_meta(const struct sk_buff *skb, > void > skb_flow_dissect_ct(const struct sk_buff *skb, > struct flow_dissector *flow_dissector, > - void *target_container, > - u16 *ctinfo_map, > - size_t mapsize) > + void *target_container, u16 *ctinfo_map, > + size_t mapsize, bool post_ct) Why do you pass this boolean as a parameter when you can just read it from qdisc_skb_cb(skb)? Thanks.
On Wed, Jan 20, 2021 at 02:18:41PM -0800, Cong Wang wrote: > On Tue, Jan 19, 2021 at 12:33 AM <wenxu@ucloud.cn> wrote: > > diff --git a/net/core/flow_dissector.c b/net/core/flow_dissector.c > > index 2d70ded..c565c7a 100644 > > --- a/net/core/flow_dissector.c > > +++ b/net/core/flow_dissector.c > > @@ -237,9 +237,8 @@ void skb_flow_dissect_meta(const struct sk_buff *skb, > > void > > skb_flow_dissect_ct(const struct sk_buff *skb, > > struct flow_dissector *flow_dissector, > > - void *target_container, > > - u16 *ctinfo_map, > > - size_t mapsize) > > + void *target_container, u16 *ctinfo_map, > > + size_t mapsize, bool post_ct) > > Why do you pass this boolean as a parameter when you > can just read it from qdisc_skb_cb(skb)? In this case, yes, but this way skb_flow_dissect_ct() can/is able to not care about what the ->cb actually is. It could be called from somewhere else too. That's my rationale on it, not sure if wenxu thought the same. Thanks, Marcelo
On Wed, Jan 20, 2021 at 3:40 PM Marcelo Ricardo Leitner <marcelo.leitner@gmail.com> wrote: > > On Wed, Jan 20, 2021 at 02:18:41PM -0800, Cong Wang wrote: > > On Tue, Jan 19, 2021 at 12:33 AM <wenxu@ucloud.cn> wrote: > > > diff --git a/net/core/flow_dissector.c b/net/core/flow_dissector.c > > > index 2d70ded..c565c7a 100644 > > > --- a/net/core/flow_dissector.c > > > +++ b/net/core/flow_dissector.c > > > @@ -237,9 +237,8 @@ void skb_flow_dissect_meta(const struct sk_buff *skb, > > > void > > > skb_flow_dissect_ct(const struct sk_buff *skb, > > > struct flow_dissector *flow_dissector, > > > - void *target_container, > > > - u16 *ctinfo_map, > > > - size_t mapsize) > > > + void *target_container, u16 *ctinfo_map, > > > + size_t mapsize, bool post_ct) > > > > Why do you pass this boolean as a parameter when you > > can just read it from qdisc_skb_cb(skb)? > > In this case, yes, but this way skb_flow_dissect_ct() can/is able to > not care about what the ->cb actually is. It could be called from > somewhere else too. This sounds reasonable, it is in net/core/ directory anyway, so should be independent of tc even though cls_flower is its only caller. Thanks.
On 1/21/2021 9:09 AM, Cong Wang wrote: > On Wed, Jan 20, 2021 at 3:40 PM Marcelo Ricardo Leitner > <marcelo.leitner@gmail.com> wrote: >> On Wed, Jan 20, 2021 at 02:18:41PM -0800, Cong Wang wrote: >>> On Tue, Jan 19, 2021 at 12:33 AM <wenxu@ucloud.cn> wrote: >>>> diff --git a/net/core/flow_dissector.c b/net/core/flow_dissector.c >>>> index 2d70ded..c565c7a 100644 >>>> --- a/net/core/flow_dissector.c >>>> +++ b/net/core/flow_dissector.c >>>> @@ -237,9 +237,8 @@ void skb_flow_dissect_meta(const struct sk_buff *skb, >>>> void >>>> skb_flow_dissect_ct(const struct sk_buff *skb, >>>> struct flow_dissector *flow_dissector, >>>> - void *target_container, >>>> - u16 *ctinfo_map, >>>> - size_t mapsize) >>>> + void *target_container, u16 *ctinfo_map, >>>> + size_t mapsize, bool post_ct) >>> Why do you pass this boolean as a parameter when you >>> can just read it from qdisc_skb_cb(skb)? >> In this case, yes, but this way skb_flow_dissect_ct() can/is able to >> not care about what the ->cb actually is. It could be called from >> somewhere else too. > This sounds reasonable, it is in net/core/ directory anyway, > so should be independent of tc even though cls_flower is its > only caller. yes. This is the same what I think. > > Thanks. >
Hello: This patch was applied to netdev/net-next.git (refs/heads/master): On Tue, 19 Jan 2021 16:31:50 +0800 you wrote: > From: wenxu <wenxu@ucloud.cn> > > This patch add the TCA_FLOWER_KEY_CT_FLAGS_INVALID flag to > match the ct_state with invalid for conntrack. > > Signed-off-by: wenxu <wenxu@ucloud.cn> > > [...] Here is the summary with links: - [v2,net-next] net/sched: cls_flower add CT_FLAGS_INVALID flag support https://git.kernel.org/netdev/net-next/c/7baf2429a1a9 You are awesome, thank you! -- Deet-doot-dot, I am a bot. https://korg.docs.kernel.org/patchwork/pwbot.html
diff --git a/include/linux/skbuff.h b/include/linux/skbuff.h index c9568cf..e22ccf0 100644 --- a/include/linux/skbuff.h +++ b/include/linux/skbuff.h @@ -1353,8 +1353,8 @@ void skb_flow_dissect_meta(const struct sk_buff *skb, skb_flow_dissect_ct(const struct sk_buff *skb, struct flow_dissector *flow_dissector, void *target_container, - u16 *ctinfo_map, - size_t mapsize); + u16 *ctinfo_map, size_t mapsize, + bool post_ct); void skb_flow_dissect_tunnel_info(const struct sk_buff *skb, struct flow_dissector *flow_dissector, diff --git a/include/net/sch_generic.h b/include/net/sch_generic.h index 639e465..e7bee99 100644 --- a/include/net/sch_generic.h +++ b/include/net/sch_generic.h @@ -388,6 +388,7 @@ struct qdisc_skb_cb { #define QDISC_CB_PRIV_LEN 20 unsigned char data[QDISC_CB_PRIV_LEN]; u16 mru; + bool post_ct; }; typedef void tcf_chain_head_change_t(struct tcf_proto *tp_head, void *priv); diff --git a/include/uapi/linux/pkt_cls.h b/include/uapi/linux/pkt_cls.h index ee95f42..709668e 100644 --- a/include/uapi/linux/pkt_cls.h +++ b/include/uapi/linux/pkt_cls.h @@ -591,6 +591,7 @@ enum { TCA_FLOWER_KEY_CT_FLAGS_ESTABLISHED = 1 << 1, /* Part of an existing connection. */ TCA_FLOWER_KEY_CT_FLAGS_RELATED = 1 << 2, /* Related to an established connection. */ TCA_FLOWER_KEY_CT_FLAGS_TRACKED = 1 << 3, /* Conntrack has occurred. */ + TCA_FLOWER_KEY_CT_FLAGS_INVALID = 1 << 4, /* Conntrack is invalid. */ }; enum { diff --git a/net/core/dev.c b/net/core/dev.c index bae35c1..9dce3f7 100644 --- a/net/core/dev.c +++ b/net/core/dev.c @@ -3878,6 +3878,7 @@ int dev_loopback_xmit(struct net *net, struct sock *sk, struct sk_buff *skb) /* qdisc_skb_cb(skb)->pkt_len was already set by the caller. */ qdisc_skb_cb(skb)->mru = 0; + qdisc_skb_cb(skb)->post_ct = false; mini_qdisc_bstats_cpu_update(miniq, skb); switch (tcf_classify(skb, miniq->filter_list, &cl_res, false)) { @@ -4960,6 +4961,7 @@ static __latent_entropy void net_tx_action(struct softirq_action *h) qdisc_skb_cb(skb)->pkt_len = skb->len; qdisc_skb_cb(skb)->mru = 0; + qdisc_skb_cb(skb)->post_ct = false; skb->tc_at_ingress = 1; mini_qdisc_bstats_cpu_update(miniq, skb); diff --git a/net/core/flow_dissector.c b/net/core/flow_dissector.c index 2d70ded..c565c7a 100644 --- a/net/core/flow_dissector.c +++ b/net/core/flow_dissector.c @@ -237,9 +237,8 @@ void skb_flow_dissect_meta(const struct sk_buff *skb, void skb_flow_dissect_ct(const struct sk_buff *skb, struct flow_dissector *flow_dissector, - void *target_container, - u16 *ctinfo_map, - size_t mapsize) + void *target_container, u16 *ctinfo_map, + size_t mapsize, bool post_ct) { #if IS_ENABLED(CONFIG_NF_CONNTRACK) struct flow_dissector_key_ct *key; @@ -251,13 +250,19 @@ void skb_flow_dissect_meta(const struct sk_buff *skb, return; ct = nf_ct_get(skb, &ctinfo); - if (!ct) + if (!ct && !post_ct) return; key = skb_flow_dissector_target(flow_dissector, FLOW_DISSECTOR_KEY_CT, target_container); + if (!ct) { + key->ct_state = TCA_FLOWER_KEY_CT_FLAGS_TRACKED | + TCA_FLOWER_KEY_CT_FLAGS_INVALID; + return; + } + if (ctinfo < mapsize) key->ct_state = ctinfo_map[ctinfo]; #if IS_ENABLED(CONFIG_NF_CONNTRACK_ZONES) diff --git a/net/sched/act_ct.c b/net/sched/act_ct.c index 83a5c67..b344207 100644 --- a/net/sched/act_ct.c +++ b/net/sched/act_ct.c @@ -1030,6 +1030,7 @@ static int tcf_ct_act(struct sk_buff *skb, const struct tc_action *a, out: tcf_action_update_bstats(&c->common, skb); + qdisc_skb_cb(skb)->post_ct = true; if (defrag) qdisc_skb_cb(skb)->pkt_len = skb->len; return retval; diff --git a/net/sched/cls_flower.c b/net/sched/cls_flower.c index 1319986..0dcb5a0 100644 --- a/net/sched/cls_flower.c +++ b/net/sched/cls_flower.c @@ -302,6 +302,7 @@ static int fl_classify(struct sk_buff *skb, const struct tcf_proto *tp, struct tcf_result *res) { struct cls_fl_head *head = rcu_dereference_bh(tp->root); + bool post_ct = qdisc_skb_cb(skb)->post_ct; struct fl_flow_key skb_key; struct fl_flow_mask *mask; struct cls_fl_filter *f; @@ -318,7 +319,8 @@ static int fl_classify(struct sk_buff *skb, const struct tcf_proto *tp, skb_flow_dissect_tunnel_info(skb, &mask->dissector, &skb_key); skb_flow_dissect_ct(skb, &mask->dissector, &skb_key, fl_ct_info_to_flower_map, - ARRAY_SIZE(fl_ct_info_to_flower_map)); + ARRAY_SIZE(fl_ct_info_to_flower_map), + post_ct); skb_flow_dissect_hash(skb, &mask->dissector, &skb_key); skb_flow_dissect(skb, &mask->dissector, &skb_key, 0);