| Message ID | 481a65741261fd81b0a0813e698af163477467ec.1606415787.git.marcelo.leitner@gmail.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [net-next] net/sched: act_ct: enable stats for HW offloaded entries | expand |
On 11/27/2020 2:40 AM, Marcelo Ricardo Leitner wrote: > By setting NF_FLOWTABLE_COUNTER. Otherwise, the updates added by > commit ef803b3cf96a ("netfilter: flowtable: add counter support in HW > offload") are not effective when using act_ct. > > While at it, now that we have the flag set, protect the call to > nf_ct_acct_update() by commit beb97d3a3192 ("net/sched: act_ct: update > nf_conn_acct for act_ct SW offload in flowtable") with the check on > NF_FLOWTABLE_COUNTER, as also done on other places. > > Note that this shouldn't impact performance as these stats are only > enabled when net.netfilter.nf_conntrack_acct is enabled. > > Signed-off-by: Marcelo Ricardo Leitner <marcelo.leitner@gmail.com> > --- > net/sched/act_ct.c | 6 ++++-- > 1 file changed, 4 insertions(+), 2 deletions(-) > > diff --git a/net/sched/act_ct.c b/net/sched/act_ct.c > index aba3cd85f284f3e49add31fe65e3b791f2386fa1..bb1ef3b8e77fb6fd6a74b88a65322baea2dc1ed5 100644 > --- a/net/sched/act_ct.c > +++ b/net/sched/act_ct.c > @@ -296,7 +296,8 @@ static int tcf_ct_flow_table_get(struct tcf_ct_params *params) > goto err_insert; > > ct_ft->nf_ft.type = &flowtable_ct; > - ct_ft->nf_ft.flags |= NF_FLOWTABLE_HW_OFFLOAD; > + ct_ft->nf_ft.flags |= NF_FLOWTABLE_HW_OFFLOAD | > + NF_FLOWTABLE_COUNTER; > err = nf_flow_table_init(&ct_ft->nf_ft); > if (err) > goto err_init; > @@ -540,7 +541,8 @@ static bool tcf_ct_flow_table_lookup(struct tcf_ct_params *p, > flow_offload_refresh(nf_ft, flow); > nf_conntrack_get(&ct->ct_general); > nf_ct_set(skb, ct, ctinfo); > - nf_ct_acct_update(ct, dir, skb->len); > + if (nf_ft->flags & NF_FLOWTABLE_COUNTER) > + nf_ct_acct_update(ct, dir, skb->len); > > return true; > } Acked-by: wenxu <wenxu@ucloud.cn> BR wenxu
On Thu, Nov 26, 2020 at 03:40:49PM -0300, Marcelo Ricardo Leitner wrote: > By setting NF_FLOWTABLE_COUNTER. Otherwise, the updates added by > commit ef803b3cf96a ("netfilter: flowtable: add counter support in HW > offload") are not effective when using act_ct. > > While at it, now that we have the flag set, protect the call to > nf_ct_acct_update() by commit beb97d3a3192 ("net/sched: act_ct: update > nf_conn_acct for act_ct SW offload in flowtable") with the check on > NF_FLOWTABLE_COUNTER, as also done on other places. > > Note that this shouldn't impact performance as these stats are only > enabled when net.netfilter.nf_conntrack_acct is enabled. > > Signed-off-by: Marcelo Ricardo Leitner <marcelo.leitner@gmail.com> Acked-by: Pablo Neira Ayuso <pablo@netfilter.org>
On Thu, 26 Nov 2020 15:40:49 -0300 Marcelo Ricardo Leitner wrote: > By setting NF_FLOWTABLE_COUNTER. Otherwise, the updates added by > commit ef803b3cf96a ("netfilter: flowtable: add counter support in HW > offload") are not effective when using act_ct. > > While at it, now that we have the flag set, protect the call to > nf_ct_acct_update() by commit beb97d3a3192 ("net/sched: act_ct: update > nf_conn_acct for act_ct SW offload in flowtable") with the check on > NF_FLOWTABLE_COUNTER, as also done on other places. > > Note that this shouldn't impact performance as these stats are only > enabled when net.netfilter.nf_conntrack_acct is enabled. > > Signed-off-by: Marcelo Ricardo Leitner <marcelo.leitner@gmail.com> Why no Fixes tag and not targeting net here?
On Fri, Nov 27, 2020 at 06:00:32PM -0800, Jakub Kicinski wrote: > On Thu, 26 Nov 2020 15:40:49 -0300 Marcelo Ricardo Leitner wrote: > > By setting NF_FLOWTABLE_COUNTER. Otherwise, the updates added by > > commit ef803b3cf96a ("netfilter: flowtable: add counter support in HW > > offload") are not effective when using act_ct. > > > > While at it, now that we have the flag set, protect the call to > > nf_ct_acct_update() by commit beb97d3a3192 ("net/sched: act_ct: update > > nf_conn_acct for act_ct SW offload in flowtable") with the check on > > NF_FLOWTABLE_COUNTER, as also done on other places. > > > > Note that this shouldn't impact performance as these stats are only > > enabled when net.netfilter.nf_conntrack_acct is enabled. > > > > Signed-off-by: Marcelo Ricardo Leitner <marcelo.leitner@gmail.com> > > Why no Fixes tag and not targeting net here? Well, I don't know if it was left out on purpose or not/missed. What I know is that act_ct initially had no support for stats of offloaded entries. ef803b3cf96a wasn't specific to act_ct (and didn't have to update it), while some support on act_ct was introduced with beb97d3a3192, but only for sw offload. So it seems to me that it's just a new piece(/incremental development) that nobody had cared so far. If you see it otherwise, I'm happy to change. I'll just need a hint on which commit I should use for the Fixes tag (as it's not clear to me, per above).
On Fri, 27 Nov 2020 23:31:44 -0300 Marcelo Ricardo Leitner wrote: > On Fri, Nov 27, 2020 at 06:00:32PM -0800, Jakub Kicinski wrote: > > On Thu, 26 Nov 2020 15:40:49 -0300 Marcelo Ricardo Leitner wrote: > > > By setting NF_FLOWTABLE_COUNTER. Otherwise, the updates added by > > > commit ef803b3cf96a ("netfilter: flowtable: add counter support in HW > > > offload") are not effective when using act_ct. > > > > > > While at it, now that we have the flag set, protect the call to > > > nf_ct_acct_update() by commit beb97d3a3192 ("net/sched: act_ct: update > > > nf_conn_acct for act_ct SW offload in flowtable") with the check on > > > NF_FLOWTABLE_COUNTER, as also done on other places. > > > > > > Note that this shouldn't impact performance as these stats are only > > > enabled when net.netfilter.nf_conntrack_acct is enabled. > > > > > > Signed-off-by: Marcelo Ricardo Leitner <marcelo.leitner@gmail.com> > > > > Why no Fixes tag and not targeting net here? > > Well, I don't know if it was left out on purpose or not/missed. > What I know is that act_ct initially had no support for stats of > offloaded entries. ef803b3cf96a wasn't specific to act_ct (and didn't > have to update it), while some support on act_ct was introduced with > beb97d3a3192, but only for sw offload. So it seems to me that it's > just a new piece(/incremental development) that nobody had cared so > far. > > If you see it otherwise, I'm happy to change. I'll just need a hint on > which commit I should use for the Fixes tag (as it's not clear to me, > per above). I don't know the code well enough to override, so I'll trust your judgment :) Applied to net-next, thanks!
diff --git a/net/sched/act_ct.c b/net/sched/act_ct.c index aba3cd85f284f3e49add31fe65e3b791f2386fa1..bb1ef3b8e77fb6fd6a74b88a65322baea2dc1ed5 100644 --- a/net/sched/act_ct.c +++ b/net/sched/act_ct.c @@ -296,7 +296,8 @@ static int tcf_ct_flow_table_get(struct tcf_ct_params *params) goto err_insert; ct_ft->nf_ft.type = &flowtable_ct; - ct_ft->nf_ft.flags |= NF_FLOWTABLE_HW_OFFLOAD; + ct_ft->nf_ft.flags |= NF_FLOWTABLE_HW_OFFLOAD | + NF_FLOWTABLE_COUNTER; err = nf_flow_table_init(&ct_ft->nf_ft); if (err) goto err_init; @@ -540,7 +541,8 @@ static bool tcf_ct_flow_table_lookup(struct tcf_ct_params *p, flow_offload_refresh(nf_ft, flow); nf_conntrack_get(&ct->ct_general); nf_ct_set(skb, ct, ctinfo); - nf_ct_acct_update(ct, dir, skb->len); + if (nf_ft->flags & NF_FLOWTABLE_COUNTER) + nf_ct_acct_update(ct, dir, skb->len); return true; }
By setting NF_FLOWTABLE_COUNTER. Otherwise, the updates added by commit ef803b3cf96a ("netfilter: flowtable: add counter support in HW offload") are not effective when using act_ct. While at it, now that we have the flag set, protect the call to nf_ct_acct_update() by commit beb97d3a3192 ("net/sched: act_ct: update nf_conn_acct for act_ct SW offload in flowtable") with the check on NF_FLOWTABLE_COUNTER, as also done on other places. Note that this shouldn't impact performance as these stats are only enabled when net.netfilter.nf_conntrack_acct is enabled. Signed-off-by: Marcelo Ricardo Leitner <marcelo.leitner@gmail.com> --- net/sched/act_ct.c | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-)