RE: [EXT] Re: [v1,net-next 1/5] net: qos offload add flow status with dropped count

[Po Liu]

Hi Jiri,

> -----Original Message-----
> From: Jiri Pirko <jiri@xxxxxxxxxxx>
> Sent: 2020年3月24日 18:02
> To: Po Liu <po.liu@xxxxxxx>
> Cc: davem@xxxxxxxxxxxxx; linux-kernel@xxxxxxxxxxxxxxx;
> netdev@xxxxxxxxxxxxxxx; vinicius.gomes@xxxxxxxxx; Claudiu Manoil
> <claudiu.manoil@xxxxxxx>; Vladimir Oltean <vladimir.oltean@xxxxxxx>;
> Alexandru Marginean <alexandru.marginean@xxxxxxx>; Xiaoliang Yang
> <xiaoliang.yang_1@xxxxxxx>; Roy Zang <roy.zang@xxxxxxx>; Mingkai Hu
> <mingkai.hu@xxxxxxx>; Jerry Huang <jerry.huang@xxxxxxx>; Leo Li
> <leoyang.li@xxxxxxx>; michael.chan@xxxxxxxxxxxx; vishal@xxxxxxxxxxx;
> saeedm@xxxxxxxxxxxx; leon@xxxxxxxxxx; jiri@xxxxxxxxxxxx;
> idosch@xxxxxxxxxxxx; alexandre.belloni@xxxxxxxxxxx;
> UNGLinuxDriver@xxxxxxxxxxxxx; kuba@xxxxxxxxxx; jhs@xxxxxxxxxxxx;
> xiyou.wangcong@xxxxxxxxx; simon.horman@xxxxxxxxxxxxx;
> pablo@xxxxxxxxxxxxx; moshe@xxxxxxxxxxxx; m-karicheri2@xxxxxx;
> andre.guedes@xxxxxxxxxxxxxxx; stephen@xxxxxxxxxxxxxxxxxx
> Subject: [EXT] Re: [v1,net-next 1/5] net: qos offload add flow status with
> dropped count
> 
> Caution: EXT Email
> 
> Tue, Mar 24, 2020 at 04:47:39AM CET, Po.Liu@xxxxxxx wrote:
> >Add the hardware tc flower offloading with dropped frame counter for
> >status update. action ops->stats_update only loaded by the
> >tcf_exts_stats_update() and tcf_exts_stats_update() only loaded by
> >matchall and tc flower hardware filter. But the stats_update only set
> >the dropped count as default false in the ops->stats_update. This patch
> >add the dropped counter to action stats update. Its dropped counter
> >update by the hardware offloading driver.
> >This is changed by replacing the drop flag with dropped frame counter.
> 
> I just read this paragraph 3 times, I'm unable to decypher :(

Sorry to make you confusing. I would make a clear description.
Before that, I just try to explain what the patch do here so you can provide more suggestion.

To get the stats in the tc flower offloading(by flag FLOW_CLS_STATS), it saves in the 'struct flow_stats' in the ' struct flow_cls_offload '. But the ' struct flow_stats ' only include the packages numbers. Some actions like policing also this 0002/0003 patch introduce stream gate action would produce dropped frames which is important for user evaluation. The packages number(pkts in struct flow_stats) and dropped number relation should be 'pkts' is how many frames were filtered, and 'dropped' number is how many frames dropped in those 'pkts'.
Eventually, the stats updated by the struct tc_action 's operation stats_update(). To implement add dropped number, then add parameter 'dropped' in the related functions: ops->stats_update/tcf_exts_stats_update() and also the tcf_action_update_stats(). There is a bool parameter 'drop' which is using now, can be understand the stats updating is for update for 'drop' count or not. But this flag is not useless as I checked in current kernel code(correct me if it is not). So replace the bool 'drop' flag with 'dropped' number in tcf_action_update_stats(). Make the tcf_action_update_stats() shows how many ' packets' updated and how many 'dropped' number in these 'packets'.
Thanks!

> 
> 
> 
> >
> >Driver side should update how many "packets" it filtered and how many
> >"dropped" in those "packets".
> >
> 
> [...]
> 
> 
> >       return action;
> > }
> >
> >-static void tcf_gact_stats_update(struct tc_action *a, u64 bytes, u32
> packets,
> >-                                u64 lastuse, bool hw)
> >+static void tcf_gact_stats_update(struct tc_action *a, u64 bytes, u64
> packets,
> >+                                u64 lastuse, u64 dropped, bool hw)
> > {
> >       struct tcf_gact *gact = to_gact(a);
> >       int action = READ_ONCE(gact->tcf_action);
> >       struct tcf_t *tm = &gact->tcf_tm;
> >
> >-      tcf_action_update_stats(a, bytes, packets, action == TC_ACT_SHOT,
> hw);
> >+      tcf_action_update_stats(a, bytes, packets,
> >+                              (action == TC_ACT_SHOT) ? packets : 0,
> >+ hw);
> 
> Avoid ()s here.

Ok.

> 
> 
> >       tm->lastuse = max_t(u64, tm->lastuse, lastuse); }
> >


Br,
Po Liu