Re: [PATCH v1 net-next 5/5] net: dsa: felix: add police action for tc flower offload

Vladimir Oltean
2020-10-20T23:39:28Z

On Tue, Oct 20, 2020 at 03:23:21PM +0800, Xiaoliang Yang wrote:
> This patch add police action to set flow meter table which is defined
> in IEEE802.1Qci. Flow metering is two rates two buckets and three color
> marker to policing the frames, we only enable one rate one bucket in
> this patch.
>
> Flow metering shares a same policer pool with VCAP policers, it calls
> ocelot_vcap_policer_add() and ocelot_vcap_policer_del() to set flow
> meter table.
>
> Signed-off-by: Xiaoliang Yang <xiaoliang.yang_1@nxp.com>
> ---

It would be good to provide a functional example in
tools/testing/selftests/drivers/net/ocelot/ which sets a policer up and
explains what is the practical difference between a PSFP policer and a
regular VCAP policer, except for the fact that the PSFP policer can only
match on {DMAC, VLAN} and the regular one can match on a lot more (L2,
L3 and L4 headers).
Specifically, why would the user ever want to use a PSFP policer.

> drivers/net/dsa/ocelot/felix_flower.c | 32 +++++++++++++++++++++++++++
> 1 file changed, 32 insertions(+)
>
> diff --git a/drivers/net/dsa/ocelot/felix_flower.c b/drivers/net/dsa/ocelot/felix_flower.c
> index 71894dcc0af2..d58a2357bab1 100644
> --- a/drivers/net/dsa/ocelot/felix_flower.c
> +++ b/drivers/net/dsa/ocelot/felix_flower.c
> @@ -363,6 +363,8 @@ static void felix_list_stream_filter_del(struct ocelot *ocelot, u32 index)
> if (tmp->index == index) {
> if (tmp->sg_valid)
> felix_list_gate_del(ocelot, tmp->sgid);
> + if (tmp->fm_valid)
> + ocelot_vcap_policer_del(ocelot, tmp->fmid);
>
> z = refcount_dec_and_test(&tmp->refcount);
> if (z) {
> @@ -466,6 +468,8 @@ static int felix_psfp_set(struct ocelot *ocelot,
> if (ret) {
> if (sfi->sg_valid)
> felix_list_gate_del(ocelot, sfi->sgid);
> + if (sfi->fm_valid)
> + ocelot_vcap_policer_del(ocelot, sfi->fmid);
> return ret;
> }
>
> @@ -559,7 +563,9 @@ int felix_flower_stream_replace(struct ocelot *ocelot, int port,
> struct felix_streamid stream = {0};
> struct felix_stream_gate_conf *sgi;
> const struct flow_action_entry *a;
> + struct ocelot_policer pol;
> int ret, size, i;
> + u64 rate, burst;
> u32 index;
>
> ret = felix_flower_parse_key(f, &stream);
> @@ -595,6 +601,32 @@ int felix_flower_stream_replace(struct ocelot *ocelot, int port,
> stream.sfid_valid = 1;
> kfree(sgi);
> break;
> + case FLOW_ACTION_POLICE:
> + if (f->common.chain_index != OCELOT_PSFP_CHAIN) {
> + NL_SET_ERR_MSG_MOD(extack,
> + "Police action only be offloaded to PSFP chain");

This message is confusing and outright incorrect. The policing action
can be offloaded to VCAP IS2 too.

> + return -EOPNOTSUPP;
> + }
> +
> + index = a->police.index + FELIX_POLICER_PSFP_BASE;
> + if (index > FELIX_POLICER_PSFP_MAX)
> + return -EINVAL;
> +
> + rate = a->police.rate_bytes_ps;
> + burst = rate * PSCHED_NS2TICKS(a->police.burst);
> + pol = (struct ocelot_policer) {
> + .burst = div_u64(burst, PSCHED_TICKS_PER_SEC),
> + .rate = div_u64(rate, 1000) * 8,
> + };
> + ret = ocelot_vcap_policer_add(ocelot, index, &pol);
> + if (ret)
> + return ret;
> +
> + sfi.fm_valid = 1;
> + sfi.fmid = index;
> + sfi.maxsdu = a->police.mtu;
> + stream.sfid_valid = 1;
> + break;
> default:
> return -EOPNOTSUPP;
> }
> --
> 2.17.1
>