RE: Re: [v1,net-next 3/4] net: qos: police action add index for tc flower offloading

RE: Re: [v1,net-next 3/4] net: qos: police action add index for tc flower offloading

[Po Liu]

Hi Jamal,


> -----Original Message-----
> From: Jamal Hadi Salim <jhs@mojatatu.com>
> Sent: 2020年6月23日 20:18
> To: Po Liu <po.liu@nxp.com>; davem@davemloft.net; linux-
> kernel@vger.kernel.org; netdev@vger.kernel.org; idosch@idosch.org
> Cc: jiri@resnulli.us; vinicius.gomes@intel.com; vlad@buslov.dev; Claudiu
> Manoil <claudiu.manoil@nxp.com>; Vladimir Oltean
> <vladimir.oltean@nxp.com>; Alexandru Marginean
> <alexandru.marginean@nxp.com>; michael.chan@broadcom.com;
> vishal@chelsio.com; saeedm@mellanox.com; leon@kernel.org;
> jiri@mellanox.com; idosch@mellanox.com;
> alexandre.belloni@bootlin.com; UNGLinuxDriver@microchip.com;
> kuba@kernel.org; xiyou.wangcong@gmail.com;
> simon.horman@netronome.com; pablo@netfilter.org;
> moshe@mellanox.com; m-karicheri2@ti.com;
> andre.guedes@linux.intel.com; stephen@networkplumber.org; Edward
> Cree <ecree@solarflare.com>
> Subject: Re: [v1,net-next 3/4] net: qos: police action add index for tc
> flower offloading
> 
> On 2020-06-23 7:55 a.m., Po Liu wrote:
> 
> 
> [..]
> >> My question: Is this any different from how stats are structured?
> >
> > I don't know I fully catch the question. Are you trying to get how many
> frames for each filter chain passing one index policing action?
> > If one index police action bind to multiple tc filter(they should have
> differnt chain index ). All those filter should get same index police action
> stats value since they are sharing the same hardware entry. But I don't
> think this is the problem.
> >
> 
> This is a good thing. What is nice is i can use the same index for s/w and
> h/w (and no need for a translation/remapping).
> 
> > With index provide to device driver(map the s/w action index to a h/w
> table index ), user could list the police actions list by command:
> > # tc actions show action police
> > Shows the police action table by index.
> 
> This is also nice.
> 
> My question: Why cant you apply the same semantics for the counters?
> Does your hardware have an indexed counter/stats table? If yes then you

Yes,  but I think tc flower can only care about the  counters of that chain. And action police care about how many frames for each police entry.

> should be able to do similar thing for counters as you do for policer (i.e
> use an index and share counters across actions). So when i say:
> tc action drop index 5

Do you mean something like "tc xxx flower action police index 5 drop"  since '' tc action drop index 5" is not a proper command? (there is 'action drop'  follow the tc filter command but not with index assigned). 

> and
> tc action ok index 5
> infact they use the same counter.

Maybe you are saying if action police follow with 'CONTROL' (reclassify | pipe | drop | continue | ok)  when offloading to hardware. With different 'CONTROL', the hardware counter won't changed since hardware never known what the 'CONTROL' is. This is still software part and will do at software part(although software seems not deal with this, I also suggest to after offloading should back to tcf_police_act() continue the action). 

When set to be offloading mode, the counters only showing the hardware counters(even different vendor could set different counter register.). But I don't think the index offloading could break anything.

> 
> 
> cheers,
> jamal


Br,
Po Liu

Re: [v1,net-next 3/4] net: qos: police action add index for tc flower offloading

[Jamal Hadi Salim]

On 2020-06-23 7:52 p.m., Po Liu wrote:
> Hi Jamal,
> 
> 

>>>> My question: Is this any different from how stats are structured?
>>>

[..]
>> My question: Why cant you apply the same semantics for the counters?
>> Does your hardware have an indexed counter/stats table? If yes then you
> 
> Yes, 

That is the point i was trying to get to. Basically:
You have a counter table which is referenced by "index"
You also have a meter/policer table which is referenced by "index".

For policers, they maintain their own stats. So when i say:
tc ... flower ... action police ... index 5
The index referred to is in the policer table

But for other actions, example when i say:
tc ... flower ... action drop index 10
The index is in the counter/stats table.
It is not exactly "10" in hardware, the driver magically hides
it from the user - so it could be hw counter index 1234

The old approach is to assume the classifier (flower in this
case) has a counter. The reason for this assumption is older
hardware was designed to deal with a single action per match.
So a counter to the filter is also the counter to the
(single) action. I get the feeling your hardware fits in that
space.

Modern use cases have evolved from the ACL single match and action
approach. Maintaining the old thought/architecture breaks in two
use cases:
1) when you have multiple actions per policy filter. You need
counter-per-action for various reasons
2) Sharing of counters across filters and action. This can
be achieve

tc supports the above and is sufficient to cover the old use
cases.
I am just worried, architecturally, we are restricting ourselves
to the old scheme.

Another reason this is important is for the sake of analytics.
A user space app can poll just for the stats table in hardware
(or the cached version in the kernel) and reduce the amount of
data crossing to user space..

cheers,
jamal

RE:Re: [v1,net-next 3/4] net: qos: police action add index for tc flower offloading

[Po Liu]

> -----Original Message-----
> From: Jamal Hadi Salim <jhs@mojatatu.com>
> Sent: 2020年6月24日 20:45
> To: Po Liu <po.liu@nxp.com>; davem@davemloft.net; linux-
> kernel@vger.kernel.org; netdev@vger.kernel.org; idosch@idosch.org
> Cc: jiri@resnulli.us; vinicius.gomes@intel.com; vlad@buslov.dev; Claudiu
> Manoil <claudiu.manoil@nxp.com>; Vladimir Oltean
> <vladimir.oltean@nxp.com>; Alexandru Marginean
> <alexandru.marginean@nxp.com>; michael.chan@broadcom.com;
> vishal@chelsio.com; saeedm@mellanox.com; leon@kernel.org;
> jiri@mellanox.com; idosch@mellanox.com;
> alexandre.belloni@bootlin.com; UNGLinuxDriver@microchip.com;
> kuba@kernel.org; xiyou.wangcong@gmail.com;
> simon.horman@netronome.com; pablo@netfilter.org;
> moshe@mellanox.com; m-karicheri2@ti.com;
> andre.guedes@linux.intel.com; stephen@networkplumber.org; Edward
> Cree <ecree@solarflare.com>
> Subject: Re: [v1,net-next 3/4] net: qos: police action add index for tc
> flower offloading
> > 
> On 2020-06-23 7:52 p.m., Po Liu wrote:
> > Hi Jamal,
> >
> >
> 
> >>>> My question: Is this any different from how stats are structured?
> >>>
> 
> [..]
> >> My question: Why cant you apply the same semantics for the counters?
> >> Does your hardware have an indexed counter/stats table? If yes then
> >> you
> >
> > Yes,
> 
> That is the point i was trying to get to. Basically:
> You have a counter table which is referenced by "index"
> You also have a meter/policer table which is referenced by "index".

They should be one same group and same meaning.

> 
> For policers, they maintain their own stats. So when i say:
> tc ... flower ... action police ... index 5 The index referred to is in the
> policer table
> 

Sure. Means police with No. 5 entry. 

> But for other actions, example when i say:
> tc ... flower ... action drop index 10

Still the question, does gact action drop could bind with index? It doesn't meanful.

> The index is in the counter/stats table.
> It is not exactly "10" in hardware, the driver magically hides it from the
> user - so it could be hw counter index 1234

Not exactly. Current flower offloading stats means get the chain index for that flow filter. The other actions should bind to that chain index. Like IEEE802.1Qci, what I am doing is bind gate action to filter chain(mandatory). And also police action as optional. There is stream counter table which summary the counters pass gate action entry and police action entry for that chain index(there is a bit different if two chain sharing same action list).
One chain counter which tc show stats get counter source:
struct psfp_streamfilter_counters {
        u64 matching_frames_count;
        u64 passing_frames_count;
        u64 not_passing_frames_count;
        u64 passing_sdu_count;
        u64 not_passing_sdu_count;
        u64 red_frames_count;
};

When pass to the user space, summarize as:
        stats.pkts = counters.matching_frames_count +  counters.not_passing_sdu_count - filter->stats.pkts;
        stats.drops = counters.not_passing_frames_count + counters.not_passing_sdu_count +   counters.red_frames_count - filter->stats.drops;

But in software side, it is showing in the action list. And action gate and police exactly showing the counters that chain index. Not the true counters of index action gate or index police. This is the limitation of get the offloading stats.


> 
> The old approach is to assume the classifier (flower in this
> case) has a counter. The reason for this assumption is older hardware was
> designed to deal with a single action per match.
> So a counter to the filter is also the counter to the
> (single) action. I get the feeling your hardware fits in that space.

No, hardware could have gate+police actions but bind to one stream filter counter table in IEEE 802.1Qci.

> 
> Modern use cases have evolved from the ACL single match and action
> approach. Maintaining the old thought/architecture breaks in two use
> cases:
> 1) when you have multiple actions per policy filter. You need counter-per-
> action for various reasons

Action index only for set an action entry in hardware, and not get stats by that index.
So I don't think it is problem of  exposing action index to the driver break the rule. This is the limitation of get the offloading stats, there is no counters get by action index. 

> 2) Sharing of counters across filters and action. This can be achieve
> 
> tc supports the above and is sufficient to cover the old use cases.
> I am just worried, architecturally, we are restricting ourselves to the old
> scheme.
> 
> Another reason this is important is for the sake of analytics.
> A user space app can poll just for the stats table in hardware (or the
> cached version in the kernel) and reduce the amount of data crossing to
> user space..
> 
> cheers,
> jamal
> 
> 
> 
> 


Br,
Po Liu

Re: [v1,net-next 3/4] net: qos: police action add index for tc flower offloading

[Jamal Hadi Salim]

On 2020-06-24 8:34 p.m., Po Liu wrote:
> 
> 
>> -----Original Message-----

>> That is the point i was trying to get to. Basically:
>> You have a counter table which is referenced by "index"
>> You also have a meter/policer table which is referenced by "index".
> 
> They should be one same group and same meaning.
> 

Didnt follow. You mean the index is the same for both the
stat and policer?

>>
>> For policers, they maintain their own stats. So when i say:
>> tc ... flower ... action police ... index 5 The index referred to is in the
>> policer table
>>
> 
> Sure. Means police with No. 5 entry.
> 
>> But for other actions, example when i say:
>> tc ... flower ... action drop index 10
> 
> Still the question, does gact action drop could bind with index? It doesn't meanful.
> 

Depends on your hardware. From this discussion i am
trying to understand where the constraint is for your case.
Whether it is your h/w or the TSN spec.
For a sample counting which is flexible see here:
https://p4.org/p4-spec/docs/PSA.html#sec-counters

That concept is not specific to P4 but rather to
newer flow-based hardware.

More context:
The assumption these days is we can have a _lot_ of flows with a lot
of actions.
Then you want to be able to collect the stats separately, possibly one
counter entry for each action of interest.
Why is this important?f For analytics uses cases,
when you are retrieving the stats you want to reduce the amount of
data being retrieved. Typically these stats are polled every X seconds.
For starters, you dont dump filters (which in your case seems to be
the only way to get the stats).
In current tc, you dump the actions. But that could be improved so
you can just dump the stats. The mapping of stats index to actions
is known to the entity doing the dump.

Does that make sense?

>> The index is in the counter/stats table.
>> It is not exactly "10" in hardware, the driver magically hides it from the
>> user - so it could be hw counter index 1234
> 
> Not exactly. Current flower offloading stats means get the chain index for that flow filter. The other actions should bind to that chain index.
 >

So if i read correctly: You have an index per filter pointing to the
counter table.
Is this something _you_ decided to do in software or is it how the
hardware works? (note i referred to this as "legacy ACL" approach
earlier. It worked like that in old hardware because the main use
case was to have one action on a match (drop/accept kind).

>Like IEEE802.1Qci, what I am doing is bind gate action to filter chain(mandatory). And also police action as optional.

I cant seem to find this spec online. Is it freely available?
Also, if i understand you correctly you are saying according to this
spec you can only have the following type of policy:
tc .. filter match-spec-here .. \
action gate gate-action-attributes \
action police ...

That "action gate" MUST always be present
but "action police" is optional?

> There is stream counter table which summary the counters pass gate action entry and police action entry for that chain index(there is a bit different if two chain sharing same action list).
> One chain counter which tc show stats get counter source:
> struct psfp_streamfilter_counters {
>          u64 matching_frames_count;
>          u64 passing_frames_count;
>          u64 not_passing_frames_count;
>          u64 passing_sdu_count;
>          u64 not_passing_sdu_count;
>          u64 red_frames_count;
> };
>

Assuming psfp is something defined in IEEE802.1Qci and the spec will
describe these?
Is the filter  "index" pointing to one of those in some counter table?


> When pass to the user space, summarize as:
>          stats.pkts = counters.matching_frames_count +  counters.not_passing_sdu_count - filter->stats.pkts;
 >
>          stats.drops = counters.not_passing_frames_count + counters.not_passing_sdu_count +   counters.red_frames_count - filter->stats.drops;
>

Thanks for the explanation.
What is filter->stats?
The rest of those counters seem related to the gate action.
How do you account for policing actions?

cheers,
jamal

RE: Re: [v1,net-next 3/4] net: qos: police action add index for tc flower offloading

[Po Liu]

Hi Jamal,


> -----Original Message-----
> From: Jamal Hadi Salim <jhs@mojatatu.com>
> Sent: 2020年6月23日 18:09
> To: Po Liu <po.liu@nxp.com>; davem@davemloft.net; linux-
> kernel@vger.kernel.org; netdev@vger.kernel.org; idosch@idosch.org
> Cc: jiri@resnulli.us; vinicius.gomes@intel.com; vlad@buslov.dev; Claudiu
> Manoil <claudiu.manoil@nxp.com>; Vladimir Oltean
> <vladimir.oltean@nxp.com>; Alexandru Marginean
> <alexandru.marginean@nxp.com>; michael.chan@broadcom.com;
> vishal@chelsio.com; saeedm@mellanox.com; leon@kernel.org;
> jiri@mellanox.com; idosch@mellanox.com;
> alexandre.belloni@bootlin.com; UNGLinuxDriver@microchip.com;
> kuba@kernel.org; xiyou.wangcong@gmail.com;
> simon.horman@netronome.com; pablo@netfilter.org;
> moshe@mellanox.com; m-karicheri2@ti.com;
> andre.guedes@linux.intel.com; stephen@networkplumber.org; Edward
> Cree <ecree@solarflare.com>
> Subject: Re: [v1,net-next 3/4] net: qos: police action add index for tc
> flower offloading
> 
> 
> This certainly brings an interesting point which i brought up earlier when
> Jiri was doing offloading of stats.
> In this case the action index is being used as the offloaded policer index
> (note: there'd need to be a check whether the index is infact acceptable to
> the h/w etc unless there
> 2^32 meters available in the hardware).

Yes, device should report invalid if index is out of range which means hardware not support.

> 
> My question: Is this any different from how stats are structured?

I don't know I fully catch the question. Are you trying to get how many frames for each filter chain passing one index policing action? 
If one index police action bind to multiple tc filter(they should have differnt chain index ). All those filter should get same index police action stats value since they are sharing the same hardware entry. But I don't think this is the problem.

With index provide to device driver(map the s/w action index to a h/w table index ), user could list the police actions list by command:
# tc actions show action police
Shows the police action table by index.
Thanks!

> In this case you can map the s/w action index to a h/w table index (of
> meters).
> My comment then was: hardware i have encountered (and i pointed to P4
> model as well) assumes an indexed table of stats.
> 
> cheers,
> jamal
> 
> On 2020-06-23 2:34 a.m., Po Liu wrote:
> > From: Po Liu <Po.Liu@nxp.com>
> >
> > Hardware may own many entries for police flow. So that make one(or
> >   multi) flow to be policed by one hardware entry. This patch add the
> > police action index provide to the driver side make it mapping the
> > driver hardware entry index.
> >
> > Signed-off-by: Po Liu <Po.Liu@nxp.com>
> > ---
> >   include/net/flow_offload.h | 1 +
> >   net/sched/cls_api.c        | 1 +
> >   2 files changed, 2 insertions(+)
> >
> > diff --git a/include/net/flow_offload.h b/include/net/flow_offload.h
> > index c2ef19c6b27d..eed98075b1ae 100644
> > --- a/include/net/flow_offload.h
> > +++ b/include/net/flow_offload.h
> > @@ -232,6 +232,7 @@ struct flow_action_entry {
> >                       bool                    truncate;
> >               } sample;
> >               struct {                                /* FLOW_ACTION_POLICE */
> > +                     u32                     index;
> >                       s64                     burst;
> >                       u64                     rate_bytes_ps;
> >                       u32                     mtu;
> > diff --git a/net/sched/cls_api.c b/net/sched/cls_api.c index
> > 6aba7d5ba1ec..fdc4c89ca1fa 100644
> > --- a/net/sched/cls_api.c
> > +++ b/net/sched/cls_api.c
> > @@ -3659,6 +3659,7 @@ int tc_setup_flow_action(struct flow_action
> *flow_action,
> >                       entry->police.rate_bytes_ps =
> >                               tcf_police_rate_bytes_ps(act);
> >                       entry->police.mtu = tcf_police_tcfp_mtu(act);
> > +                     entry->police.index = act->tcfa_index;
> >               } else if (is_tcf_ct(act)) {
> >                       entry->id = FLOW_ACTION_CT;
> >                       entry->ct.action = tcf_ct_action(act);
> >

Br,
Po Liu

Re: [v1,net-next 3/4] net: qos: police action add index for tc flower offloading

[Jamal Hadi Salim]

On 2020-06-23 7:55 a.m., Po Liu wrote:


[..]
>> My question: Is this any different from how stats are structured?
> 
> I don't know I fully catch the question. Are you trying to get how many frames for each filter chain passing one index policing action?
> If one index police action bind to multiple tc filter(they should have differnt chain index ). All those filter should get same index police action stats value since they are sharing the same hardware entry. But I don't think this is the problem.
>

This is a good thing. What is nice is i can use the same index for
s/w and h/w (and no need for a translation/remapping).

> With index provide to device driver(map the s/w action index to a h/w table index ), user could list the police actions list by command:
> # tc actions show action police
> Shows the police action table by index.

This is also nice.

My question: Why cant you apply the same semantics for the counters?
Does your hardware have an indexed counter/stats table? If yes
then you should be able to do similar thing for counters
as you do for policer (i.e use an index and share counters across
actions). So when i say:
tc action drop index 5
and
tc action ok index 5
infact they use the same counter.


cheers,
jamal

[RFC,net-next 6/9] net: qos: add tc police offloading action with max frame size limit

[Po Liu]

Current police offloading support the 'burst'' and 'rate_bytes_ps'. Some
hardware own the capability to limit the frame size. If the frame size
larger than the setting, the frame would be dropped. For the police
action itself already accept the 'mtu' parameter in tc command. But not
extend to tc flower offloading. So extend 'mtu' to tc flower offloading.

Signed-off-by: Po Liu <Po.Liu@nxp.com>
---
 include/net/flow_offload.h     |  1 +
 include/net/tc_act/tc_police.h | 10 ++++++++++
 net/sched/cls_api.c            |  1 +
 3 files changed, 12 insertions(+)

diff --git a/include/net/flow_offload.h b/include/net/flow_offload.h
index 7f5a097f5072..54df87328edc 100644
--- a/include/net/flow_offload.h
+++ b/include/net/flow_offload.h
@@ -203,6 +203,7 @@ struct flow_action_entry {
 		struct {				/* FLOW_ACTION_POLICE */
 			s64			burst;
 			u64			rate_bytes_ps;
+			u32			mtu;
 		} police;
 		struct {				/* FLOW_ACTION_CT */
 			int action;
diff --git a/include/net/tc_act/tc_police.h b/include/net/tc_act/tc_police.h
index f098ad4424be..39fbf28f8f3e 100644
--- a/include/net/tc_act/tc_police.h
+++ b/include/net/tc_act/tc_police.h
@@ -69,4 +69,14 @@ static inline s64 tcf_police_tcfp_burst(const struct tc_action *act)
 	return params->tcfp_burst;
 }
 
+static inline u32 tcf_police_mtu(const struct tc_action *act)
+{
+	struct tcf_police *police = to_police(act);
+	struct tcf_police_params *params;
+
+	params = rcu_dereference_protected(police->params,
+					   lockdep_is_held(&police->tcf_lock));
+
+	return params->tcfp_mtu;
+}
 #endif /* __NET_TC_POLICE_H */
diff --git a/net/sched/cls_api.c b/net/sched/cls_api.c
index 0ada7b2a5c2c..363d3991793d 100644
--- a/net/sched/cls_api.c
+++ b/net/sched/cls_api.c
@@ -3583,6 +3583,7 @@ int tc_setup_flow_action(struct flow_action *flow_action,
 			entry->police.burst = tcf_police_tcfp_burst(act);
 			entry->police.rate_bytes_ps =
 				tcf_police_rate_bytes_ps(act);
+			entry->police.mtu = tcf_police_mtu(act);
 		} else if (is_tcf_ct(act)) {
 			entry->id = FLOW_ACTION_CT;
 			entry->ct.action = tcf_ct_action(act);
-- 
2.17.1

[v1,net-next 1/4] net: qos: add tc police offloading action with max frame size limit

[Po Liu]

From: Po Liu <Po.Liu@nxp.com>

Current police offloading support the 'burst'' and 'rate_bytes_ps'. Some
hardware own the capability to limit the frame size. If the frame size
larger than the setting, the frame would be dropped. For the police
action itself already accept the 'mtu' parameter in tc command. But not
extend to tc flower offloading. So extend 'mtu' to tc flower offloading.

Signed-off-by: Po Liu <Po.Liu@nxp.com>
---
continue the thread 20200306125608.11717-7-Po.Liu@nxp.com for the police
action offloading.

 include/net/flow_offload.h     |  1 +
 include/net/tc_act/tc_police.h | 10 ++++++++++
 net/sched/cls_api.c            |  1 +
 3 files changed, 12 insertions(+)

diff --git a/include/net/flow_offload.h b/include/net/flow_offload.h
index 00c15f14c434..c2ef19c6b27d 100644
--- a/include/net/flow_offload.h
+++ b/include/net/flow_offload.h
@@ -234,6 +234,7 @@ struct flow_action_entry {
 		struct {				/* FLOW_ACTION_POLICE */
 			s64			burst;
 			u64			rate_bytes_ps;
+			u32			mtu;
 		} police;
 		struct {				/* FLOW_ACTION_CT */
 			int action;
diff --git a/include/net/tc_act/tc_police.h b/include/net/tc_act/tc_police.h
index f098ad4424be..cd973b10ae8c 100644
--- a/include/net/tc_act/tc_police.h
+++ b/include/net/tc_act/tc_police.h
@@ -69,4 +69,14 @@ static inline s64 tcf_police_tcfp_burst(const struct tc_action *act)
 	return params->tcfp_burst;
 }
 
+static inline u32 tcf_police_tcfp_mtu(const struct tc_action *act)
+{
+	struct tcf_police *police = to_police(act);
+	struct tcf_police_params *params;
+
+	params = rcu_dereference_protected(police->params,
+					   lockdep_is_held(&police->tcf_lock));
+	return params->tcfp_mtu;
+}
+
 #endif /* __NET_TC_POLICE_H */
diff --git a/net/sched/cls_api.c b/net/sched/cls_api.c
index a00a203b2ef5..6aba7d5ba1ec 100644
--- a/net/sched/cls_api.c
+++ b/net/sched/cls_api.c
@@ -3658,6 +3658,7 @@ int tc_setup_flow_action(struct flow_action *flow_action,
 			entry->police.burst = tcf_police_tcfp_burst(act);
 			entry->police.rate_bytes_ps =
 				tcf_police_rate_bytes_ps(act);
+			entry->police.mtu = tcf_police_tcfp_mtu(act);
 		} else if (is_tcf_ct(act)) {
 			entry->id = FLOW_ACTION_CT;
 			entry->ct.action = tcf_ct_action(act);
-- 
2.17.1

[v1,net-next 3/4] net: qos: police action add index for tc flower offloading

[Po Liu]

From: Po Liu <Po.Liu@nxp.com>

Hardware may own many entries for police flow. So that make one(or
 multi) flow to be policed by one hardware entry. This patch add the
police action index provide to the driver side make it mapping the
driver hardware entry index.

Signed-off-by: Po Liu <Po.Liu@nxp.com>
---
 include/net/flow_offload.h | 1 +
 net/sched/cls_api.c        | 1 +
 2 files changed, 2 insertions(+)

diff --git a/include/net/flow_offload.h b/include/net/flow_offload.h
index c2ef19c6b27d..eed98075b1ae 100644
--- a/include/net/flow_offload.h
+++ b/include/net/flow_offload.h
@@ -232,6 +232,7 @@ struct flow_action_entry {
 			bool			truncate;
 		} sample;
 		struct {				/* FLOW_ACTION_POLICE */
+			u32			index;
 			s64			burst;
 			u64			rate_bytes_ps;
 			u32			mtu;
diff --git a/net/sched/cls_api.c b/net/sched/cls_api.c
index 6aba7d5ba1ec..fdc4c89ca1fa 100644
--- a/net/sched/cls_api.c
+++ b/net/sched/cls_api.c
@@ -3659,6 +3659,7 @@ int tc_setup_flow_action(struct flow_action *flow_action,
 			entry->police.rate_bytes_ps =
 				tcf_police_rate_bytes_ps(act);
 			entry->police.mtu = tcf_police_tcfp_mtu(act);
+			entry->police.index = act->tcfa_index;
 		} else if (is_tcf_ct(act)) {
 			entry->id = FLOW_ACTION_CT;
 			entry->ct.action = tcf_ct_action(act);
-- 
2.17.1

Re: [v1,net-next 3/4] net: qos: police action add index for tc flower offloading

[Ido Schimmel]

On Tue, Jun 23, 2020 at 02:34:11PM +0800, Po Liu wrote:
> From: Po Liu <Po.Liu@nxp.com>
> 
> Hardware may own many entries for police flow. So that make one(or
>  multi) flow to be policed by one hardware entry. This patch add the
> police action index provide to the driver side make it mapping the
> driver hardware entry index.

Maybe first mention that it is possible for multiple filters in software
to share the same policer. Something like:

"
It is possible for several tc filters to share the same police action by
specifying the action's index when installing the filters.
    
Propagate this index to device drivers through the flow offload
intermediate representation, so that drivers could share a single
hardware policer between multiple filters.
"

> 
> Signed-off-by: Po Liu <Po.Liu@nxp.com>
> ---
>  include/net/flow_offload.h | 1 +
>  net/sched/cls_api.c        | 1 +
>  2 files changed, 2 insertions(+)
> 
> diff --git a/include/net/flow_offload.h b/include/net/flow_offload.h
> index c2ef19c6b27d..eed98075b1ae 100644
> --- a/include/net/flow_offload.h
> +++ b/include/net/flow_offload.h
> @@ -232,6 +232,7 @@ struct flow_action_entry {
>  			bool			truncate;
>  		} sample;
>  		struct {				/* FLOW_ACTION_POLICE */
> +			u32			index;
>  			s64			burst;
>  			u64			rate_bytes_ps;
>  			u32			mtu;
> diff --git a/net/sched/cls_api.c b/net/sched/cls_api.c
> index 6aba7d5ba1ec..fdc4c89ca1fa 100644
> --- a/net/sched/cls_api.c
> +++ b/net/sched/cls_api.c
> @@ -3659,6 +3659,7 @@ int tc_setup_flow_action(struct flow_action *flow_action,
>  			entry->police.rate_bytes_ps =
>  				tcf_police_rate_bytes_ps(act);
>  			entry->police.mtu = tcf_police_tcfp_mtu(act);
> +			entry->police.index = act->tcfa_index;
>  		} else if (is_tcf_ct(act)) {
>  			entry->id = FLOW_ACTION_CT;
>  			entry->ct.action = tcf_ct_action(act);
> -- 
> 2.17.1
> 

Re: [v1,net-next 3/4] net: qos: police action add index for tc flower offloading

[Jamal Hadi Salim]

This certainly brings an interesting point which i brought up earlier
when Jiri was doing offloading of stats.
In this case the action index is being used as the offloaded
policer index (note: there'd need to be a check whether the
index is infact acceptable to the h/w etc unless there
2^32 meters available in the hardware).

My question: Is this any different from how stats are structured?
In this case you can map the s/w action index to a h/w table index
(of meters).
My comment then was: hardware i have encountered (and i pointed to P4
model as well) assumes an indexed table of stats.

cheers,
jamal

On 2020-06-23 2:34 a.m., Po Liu wrote:
> From: Po Liu <Po.Liu@nxp.com>
> 
> Hardware may own many entries for police flow. So that make one(or
>   multi) flow to be policed by one hardware entry. This patch add the
> police action index provide to the driver side make it mapping the
> driver hardware entry index.
> 
> Signed-off-by: Po Liu <Po.Liu@nxp.com>
> ---
>   include/net/flow_offload.h | 1 +
>   net/sched/cls_api.c        | 1 +
>   2 files changed, 2 insertions(+)
> 
> diff --git a/include/net/flow_offload.h b/include/net/flow_offload.h
> index c2ef19c6b27d..eed98075b1ae 100644
> --- a/include/net/flow_offload.h
> +++ b/include/net/flow_offload.h
> @@ -232,6 +232,7 @@ struct flow_action_entry {
>   			bool			truncate;
>   		} sample;
>   		struct {				/* FLOW_ACTION_POLICE */
> +			u32			index;
>   			s64			burst;
>   			u64			rate_bytes_ps;
>   			u32			mtu;
> diff --git a/net/sched/cls_api.c b/net/sched/cls_api.c
> index 6aba7d5ba1ec..fdc4c89ca1fa 100644
> --- a/net/sched/cls_api.c
> +++ b/net/sched/cls_api.c
> @@ -3659,6 +3659,7 @@ int tc_setup_flow_action(struct flow_action *flow_action,
>   			entry->police.rate_bytes_ps =
>   				tcf_police_rate_bytes_ps(act);
>   			entry->police.mtu = tcf_police_tcfp_mtu(act);
> +			entry->police.index = act->tcfa_index;
>   		} else if (is_tcf_ct(act)) {
>   			entry->id = FLOW_ACTION_CT;
>   			entry->ct.action = tcf_ct_action(act);
>