| Message ID | 20200623063412.19180-3-po.liu@nxp.com |
|---|---|
| State | Changes Requested |
| Delegated to: | David Miller |
| Headers | show |
| Series | [v1,net-next,1/4] net: qos: add tc police offloading action with max frame size limit | expand |
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 >
Hi Ido, > -----Original Message----- > From: Ido Schimmel <idosch@idosch.org> > Sent: 2020年6月23日 15:10 > To: Po Liu <po.liu@nxp.com> > Cc: davem@davemloft.net; linux-kernel@vger.kernel.org; > netdev@vger.kernel.org; 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; jhs@mojatatu.com; 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 > Subject: [EXT] Re: [v1,net-next 3/4] net: qos: police action add index for tc > flower offloading > > Caution: EXT Email > > 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 > >
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); >
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);