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@xxxxxxxxxxxx>
> Sent: 2020å6æ24æ 20:45
> To: Po Liu <po.liu@xxxxxxx>; davem@xxxxxxxxxxxxx; linux-
> kernel@xxxxxxxxxxxxxxx; netdev@xxxxxxxxxxxxxxx; idosch@xxxxxxxxxx
> Cc: jiri@xxxxxxxxxxx; vinicius.gomes@xxxxxxxxx; vlad@xxxxxxxxxx; Claudiu
> Manoil <claudiu.manoil@xxxxxxx>; Vladimir Oltean
> <vladimir.oltean@xxxxxxx>; Alexandru Marginean
> <alexandru.marginean@xxxxxxx>; michael.chan@xxxxxxxxxxxx;
> vishal@xxxxxxxxxxx; saeedm@xxxxxxxxxxxx; leon@xxxxxxxxxx;
> jiri@xxxxxxxxxxxx; idosch@xxxxxxxxxxxx;
> alexandre.belloni@xxxxxxxxxxx; UNGLinuxDriver@xxxxxxxxxxxxx;
> kuba@xxxxxxxxxx; xiyou.wangcong@xxxxxxxxx;
> simon.horman@xxxxxxxxxxxxx; pablo@xxxxxxxxxxxxx;
> moshe@xxxxxxxxxxxx; m-karicheri2@xxxxxx;
> andre.guedes@xxxxxxxxxxxxxxx; stephen@xxxxxxxxxxxxxxxxxx; Edward
> Cree <ecree@xxxxxxxxxxxxxx>
> 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