RE: [EXT] Re: [RFC,net-next 3/9] net: schedule: add action gate offloading

[Po Liu]

Hi Jakub,


> -----Original Message-----
> From: Jakub Kicinski <kuba@xxxxxxxxxx>
> Sent: 2020年3月7日 3:19
> 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; jhs@xxxxxxxxxxxx;
> xiyou.wangcong@xxxxxxxxx; john.hurley@xxxxxxxxxxxxx;
> simon.horman@xxxxxxxxxxxxx;
> pieter.jansenvanvuuren@xxxxxxxxxxxxx; pablo@xxxxxxxxxxxxx;
> moshe@xxxxxxxxxxxx; ivan.khoronzhuk@xxxxxxxxxx; m-karicheri2@xxxxxx;
> andre.guedes@xxxxxxxxxxxxxxx; jakub.kicinski@xxxxxxxxxxxxx
> Subject: [EXT] Re: [RFC,net-next 3/9] net: schedule: add action gate
> offloading
> 
> Caution: EXT Email
> 
> On Fri, 6 Mar 2020 11:02:00 -0800 Jakub Kicinski wrote:
> > On Fri,  6 Mar 2020 20:56:01 +0800 Po Liu wrote:
> > > +static int tcf_gate_get_entries(struct flow_action_entry *entry,
> > > +                           const struct tc_action *act) {
> > > +   entry->gate.entries = tcf_gate_get_list(act);
> > > +
> > > +   if (!entry->gate.entries)
> > > +           return -EINVAL;
> > > +
> > > +   entry->destructor = tcf_gate_entry_destructor;
> > > +   entry->destructor_priv = entry->gate.entries;
> >
> > What's this destructor stuff doing? I don't it being called.

It prepare a gate list array parameters for offloading.  So the driver side would not link the data with protocol side. Destructor would free the temporary gate list array.

> 
> Ah, it's the action destructor, not something gate specific. Disregard.

I understand here with actions are: 
Each tc flower follow up with actions. Each action defined:

struct flow_action_entry {
	enum flow_action_id             id;
	action_destr                    destructor;
	void                            *destructor_priv;
	union {
		......
		{}sample,
		{}police,
		{}gate,
	}
}

So for the destructor and destructor_priv are provided specific for the union action. So it is not gate specific. For mirror action, destructor_priv would be point to a mirror device data. 
I suppose it is for destroy the temporary  data like it name.  And after tc_setup_flow_action() loaded, the destructor function would be loaded by tc_cleanup_flow_action() to destroy and free the temporary data.

Code flow is :
net/sched/cls_flower.c
	static int fl_hw_replace_filter()
	{
		......
		tc_setup_flow_action(); ---------------------------------------> assign action parameters (with the destructor and destructor_priv if the action needed)
		......
		tc_setup_cb_add() ----------------------------------------------> call the driver provide rules with actions datas for device
		......
		tc_cleanup_flow_action(&cls_flower.rule->action);  ---> loading each action''s destructor(destructor_priv)
	}

So for each action would be with its private destructor and destructor_priv if the action needed, and then destroyed at tc_cleanup_flow_action(). 

Did I misunderstand anything?

Thanks!

Br,
Po Liu