| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <CA+h21hqTSrmA8Q0hrJrQ23Qa-kwYjMqxH_mnVFcxPJruQJGqiA@mail.gmail.com>
Date: Mon, 29 Jun 2020 09:42:04 +0300
From: Vladimir Oltean <olteanv@...il.com>
To: Po Liu <po.liu@....com>
Cc: "David S. Miller" <davem@...emloft.net>,
lkml <linux-kernel@...r.kernel.org>,
netdev <netdev@...r.kernel.org>,
Alexandre Belloni <alexandre.belloni@...tlin.com>,
Andrew Lunn <andrew@...n.ch>,
Vivien Didelot <vivien.didelot@...il.com>,
Florian Fainelli <f.fainelli@...il.com>,
Jakub Kicinski <kuba@...nel.org>,
Jiri Pirko <jiri@...nulli.us>, vlad@...lov.dev,
Claudiu Manoil <claudiu.manoil@....com>,
Vladimir Oltean <vladimir.oltean@....com>,
Alexandru Marginean <alexandru.marginean@....com>,
michael.chan@...adcom.com, vishal@...lsio.com, saeedm@...lanox.com,
leon@...nel.org, Jiri Pirko <jiri@...lanox.com>,
Ido Schimmel <idosch@...lanox.com>,
Microchip Linux Driver Support <UNGLinuxDriver@...rochip.com>,
Jamal Hadi Salim <jhs@...atatu.com>,
Cong Wang <xiyou.wangcong@...il.com>,
simon.horman@...ronome.com, pablo@...filter.org,
moshe@...lanox.com, oss-drivers@...ronome.com
Subject: Re: [v1,net-next] net:qos: police action offloading parameter 'burst'
change to the original value
Hi Po,
On Mon, 29 Jun 2020 at 05:25, Po Liu <po.liu@....com> wrote:
>
> Since 'tcfp_burst' with TICK factor, driver side always need to recover
> it to the original value, this patch moves the generic calculation and
> recover to the 'burst' original value before offloading to device driver.
>
> Signed-off-by: Po Liu <po.liu@....com>
> Signed-off-by: Vladimir Oltean <vladimir.oltean@....com>
> ---
Just one small comment:
I think the use of my "Signed-off-by" tag here is incorrect. Typically
the first Signed-off-by tag corresponds to the patch author (and that
one is correct) and further ones correspond to the other people who
picked up this patch and submitted it to a tree. So, my Signed-off-by
in the second position would indicate that I took your patch and
submitted it to netdev, which I didn't do.
I think the Acked-by and Tested-by tags would have been more appropriate.
https://www.kernel.org/doc/html/latest/process/5.Posting.html#patch-formatting-and-changelogs
> drivers/net/dsa/ocelot/felix.c | 4 +--
> drivers/net/dsa/sja1105/sja1105_flower.c | 16 ++++------
> drivers/net/dsa/sja1105/sja1105_main.c | 4 +--
> .../net/ethernet/freescale/enetc/enetc_qos.c | 8 +----
> drivers/net/ethernet/mscc/ocelot_flower.c | 4 +--
> drivers/net/ethernet/mscc/ocelot_net.c | 4 +--
> .../ethernet/netronome/nfp/flower/qos_conf.c | 6 ++--
> include/net/dsa.h | 2 +-
> include/net/flow_offload.h | 2 +-
> include/net/tc_act/tc_police.h | 32 +++++++++++++++++--
> net/sched/cls_api.c | 2 +-
> 11 files changed, 48 insertions(+), 36 deletions(-)
>
> diff --git a/drivers/net/dsa/ocelot/felix.c b/drivers/net/dsa/ocelot/felix.c
> index 25046777c993..75020af7f7a4 100644
> --- a/drivers/net/dsa/ocelot/felix.c
> +++ b/drivers/net/dsa/ocelot/felix.c
> @@ -746,9 +746,7 @@ static int felix_port_policer_add(struct dsa_switch *ds, int port,
> struct ocelot *ocelot = ds->priv;
> struct ocelot_policer pol = {
> .rate = div_u64(policer->rate_bytes_per_sec, 1000) * 8,
> - .burst = div_u64(policer->rate_bytes_per_sec *
> - PSCHED_NS2TICKS(policer->burst),
> - PSCHED_TICKS_PER_SEC),
> + .burst = policer->burst,
> };
>
> return ocelot_port_policer_add(ocelot, port, &pol);
> diff --git a/drivers/net/dsa/sja1105/sja1105_flower.c b/drivers/net/dsa/sja1105/sja1105_flower.c
> index 9ee8968610cd..12e76020bea3 100644
> --- a/drivers/net/dsa/sja1105/sja1105_flower.c
> +++ b/drivers/net/dsa/sja1105/sja1105_flower.c
> @@ -31,7 +31,7 @@ static int sja1105_setup_bcast_policer(struct sja1105_private *priv,
> struct netlink_ext_ack *extack,
> unsigned long cookie, int port,
> u64 rate_bytes_per_sec,
> - s64 burst)
> + u32 burst)
> {
> struct sja1105_rule *rule = sja1105_rule_find(priv, cookie);
> struct sja1105_l2_policing_entry *policing;
> @@ -79,9 +79,8 @@ static int sja1105_setup_bcast_policer(struct sja1105_private *priv,
>
> policing[rule->bcast_pol.sharindx].rate = div_u64(rate_bytes_per_sec *
> 512, 1000000);
> - policing[rule->bcast_pol.sharindx].smax = div_u64(rate_bytes_per_sec *
> - PSCHED_NS2TICKS(burst),
> - PSCHED_TICKS_PER_SEC);
> + policing[rule->bcast_pol.sharindx].smax = burst;
> +
> /* TODO: support per-flow MTU */
> policing[rule->bcast_pol.sharindx].maxlen = VLAN_ETH_FRAME_LEN +
> ETH_FCS_LEN;
> @@ -103,7 +102,7 @@ static int sja1105_setup_tc_policer(struct sja1105_private *priv,
> struct netlink_ext_ack *extack,
> unsigned long cookie, int port, int tc,
> u64 rate_bytes_per_sec,
> - s64 burst)
> + u32 burst)
> {
> struct sja1105_rule *rule = sja1105_rule_find(priv, cookie);
> struct sja1105_l2_policing_entry *policing;
> @@ -152,9 +151,8 @@ static int sja1105_setup_tc_policer(struct sja1105_private *priv,
>
> policing[rule->tc_pol.sharindx].rate = div_u64(rate_bytes_per_sec *
> 512, 1000000);
> - policing[rule->tc_pol.sharindx].smax = div_u64(rate_bytes_per_sec *
> - PSCHED_NS2TICKS(burst),
> - PSCHED_TICKS_PER_SEC);
> + policing[rule->tc_pol.sharindx].smax = burst;
> +
> /* TODO: support per-flow MTU */
> policing[rule->tc_pol.sharindx].maxlen = VLAN_ETH_FRAME_LEN +
> ETH_FCS_LEN;
> @@ -177,7 +175,7 @@ static int sja1105_flower_policer(struct sja1105_private *priv, int port,
> unsigned long cookie,
> struct sja1105_key *key,
> u64 rate_bytes_per_sec,
> - s64 burst)
> + u32 burst)
> {
> switch (key->type) {
> case SJA1105_KEY_BCAST:
> diff --git a/drivers/net/dsa/sja1105/sja1105_main.c b/drivers/net/dsa/sja1105/sja1105_main.c
> index 789b288cc78b..5079e4aeef80 100644
> --- a/drivers/net/dsa/sja1105/sja1105_main.c
> +++ b/drivers/net/dsa/sja1105/sja1105_main.c
> @@ -3324,9 +3324,7 @@ static int sja1105_port_policer_add(struct dsa_switch *ds, int port,
> */
> policing[port].rate = div_u64(512 * policer->rate_bytes_per_sec,
> 1000000);
> - policing[port].smax = div_u64(policer->rate_bytes_per_sec *
> - PSCHED_NS2TICKS(policer->burst),
> - PSCHED_TICKS_PER_SEC);
> + policing[port].smax = policer->burst;
>
> return sja1105_static_config_reload(priv, SJA1105_BEST_EFFORT_POLICING);
> }
> diff --git a/drivers/net/ethernet/freescale/enetc/enetc_qos.c b/drivers/net/ethernet/freescale/enetc/enetc_qos.c
> index 4f670cbdf186..b8b336179d82 100644
> --- a/drivers/net/ethernet/freescale/enetc/enetc_qos.c
> +++ b/drivers/net/ethernet/freescale/enetc/enetc_qos.c
> @@ -1241,8 +1241,6 @@ static int enetc_psfp_parse_clsflower(struct enetc_ndev_priv *priv,
> /* Flow meter and max frame size */
> if (entryp) {
> if (entryp->police.burst) {
> - u64 temp;
> -
> fmi = kzalloc(sizeof(*fmi), GFP_KERNEL);
> if (!fmi) {
> err = -ENOMEM;
> @@ -1250,11 +1248,7 @@ static int enetc_psfp_parse_clsflower(struct enetc_ndev_priv *priv,
> }
> refcount_set(&fmi->refcount, 1);
> fmi->cir = entryp->police.rate_bytes_ps;
> - /* Convert to original burst value */
> - temp = entryp->police.burst * fmi->cir;
> - temp = div_u64(temp, 1000000000ULL);
> -
> - fmi->cbs = temp;
> + fmi->cbs = entryp->police.burst;
> fmi->index = entryp->police.index;
> filter->flags |= ENETC_PSFP_FLAGS_FMI;
> filter->fmi_index = fmi->index;
> diff --git a/drivers/net/ethernet/mscc/ocelot_flower.c b/drivers/net/ethernet/mscc/ocelot_flower.c
> index f2a85b06a6e7..ec1b6e2572ba 100644
> --- a/drivers/net/ethernet/mscc/ocelot_flower.c
> +++ b/drivers/net/ethernet/mscc/ocelot_flower.c
> @@ -12,7 +12,6 @@ static int ocelot_flower_parse_action(struct flow_cls_offload *f,
> struct ocelot_vcap_filter *filter)
> {
> const struct flow_action_entry *a;
> - s64 burst;
> u64 rate;
> int i;
>
> @@ -35,8 +34,7 @@ static int ocelot_flower_parse_action(struct flow_cls_offload *f,
> filter->action = OCELOT_VCAP_ACTION_POLICE;
> rate = a->police.rate_bytes_ps;
> filter->pol.rate = div_u64(rate, 1000) * 8;
> - burst = rate * PSCHED_NS2TICKS(a->police.burst);
> - filter->pol.burst = div_u64(burst, PSCHED_TICKS_PER_SEC);
> + filter->pol.burst = a->police.burst;
> break;
> default:
> return -EOPNOTSUPP;
> diff --git a/drivers/net/ethernet/mscc/ocelot_net.c b/drivers/net/ethernet/mscc/ocelot_net.c
> index 702b42543fb7..b69187c51fa6 100644
> --- a/drivers/net/ethernet/mscc/ocelot_net.c
> +++ b/drivers/net/ethernet/mscc/ocelot_net.c
> @@ -74,9 +74,7 @@ static int ocelot_setup_tc_cls_matchall(struct ocelot_port_private *priv,
> }
>
> pol.rate = (u32)div_u64(action->police.rate_bytes_ps, 1000) * 8;
> - pol.burst = (u32)div_u64(action->police.rate_bytes_ps *
> - PSCHED_NS2TICKS(action->police.burst),
> - PSCHED_TICKS_PER_SEC);
> + pol.burst = action->police.burst;
>
> err = ocelot_port_policer_add(ocelot, port, &pol);
> if (err) {
> diff --git a/drivers/net/ethernet/netronome/nfp/flower/qos_conf.c b/drivers/net/ethernet/netronome/nfp/flower/qos_conf.c
> index bb327d48d1ab..d4ce8f9ef3cc 100644
> --- a/drivers/net/ethernet/netronome/nfp/flower/qos_conf.c
> +++ b/drivers/net/ethernet/netronome/nfp/flower/qos_conf.c
> @@ -69,7 +69,8 @@ nfp_flower_install_rate_limiter(struct nfp_app *app, struct net_device *netdev,
> struct nfp_repr *repr;
> struct sk_buff *skb;
> u32 netdev_port_id;
> - u64 burst, rate;
> + u32 burst;
> + u64 rate;
>
> if (!nfp_netdev_is_nfp_repr(netdev)) {
> NL_SET_ERR_MSG_MOD(extack, "unsupported offload: qos rate limit offload not supported on higher level port");
> @@ -104,8 +105,7 @@ nfp_flower_install_rate_limiter(struct nfp_app *app, struct net_device *netdev,
> }
>
> rate = action->police.rate_bytes_ps;
> - burst = div_u64(rate * PSCHED_NS2TICKS(action->police.burst),
> - PSCHED_TICKS_PER_SEC);
> + burst = action->police.burst;
> netdev_port_id = nfp_repr_get_port_id(netdev);
>
> skb = nfp_flower_cmsg_alloc(repr->app, sizeof(struct nfp_police_config),
> diff --git a/include/net/dsa.h b/include/net/dsa.h
> index 50389772c597..4046ccd1945d 100644
> --- a/include/net/dsa.h
> +++ b/include/net/dsa.h
> @@ -144,7 +144,7 @@ struct dsa_mall_mirror_tc_entry {
>
> /* TC port policer entry */
> struct dsa_mall_policer_tc_entry {
> - s64 burst;
> + u32 burst;
> u64 rate_bytes_per_sec;
> };
>
> diff --git a/include/net/flow_offload.h b/include/net/flow_offload.h
> index 3bafb5124ac0..2dc25082eabf 100644
> --- a/include/net/flow_offload.h
> +++ b/include/net/flow_offload.h
> @@ -233,7 +233,7 @@ struct flow_action_entry {
> } sample;
> struct { /* FLOW_ACTION_POLICE */
> u32 index;
> - s64 burst;
> + u32 burst;
> u64 rate_bytes_ps;
> u32 mtu;
> } police;
> diff --git a/include/net/tc_act/tc_police.h b/include/net/tc_act/tc_police.h
> index cd973b10ae8c..6d1e26b709b5 100644
> --- a/include/net/tc_act/tc_police.h
> +++ b/include/net/tc_act/tc_police.h
> @@ -59,14 +59,42 @@ static inline u64 tcf_police_rate_bytes_ps(const struct tc_action *act)
> return params->rate.rate_bytes_ps;
> }
>
> -static inline s64 tcf_police_tcfp_burst(const struct tc_action *act)
> +static inline u32 tcf_police_burst(const struct tc_action *act)
> {
> struct tcf_police *police = to_police(act);
> struct tcf_police_params *params;
> + u32 burst;
>
> params = rcu_dereference_protected(police->params,
> lockdep_is_held(&police->tcf_lock));
> - return params->tcfp_burst;
> +
> + /*
> + * "rate" bytes "burst" nanoseconds
> + * ------------ * -------------------
> + * 1 second 2^6 ticks
> + *
> + * ------------------------------------
> + * NSEC_PER_SEC nanoseconds
> + * ------------------------
> + * 2^6 ticks
> + *
> + * "rate" bytes "burst" nanoseconds 2^6 ticks
> + * = ------------ * ------------------- * ------------------------
> + * 1 second 2^6 ticks NSEC_PER_SEC nanoseconds
> + *
> + * "rate" * "burst"
> + * = ---------------- bytes/nanosecond
> + * NSEC_PER_SEC^2
> + *
> + *
> + * "rate" * "burst"
> + * = ---------------- bytes/second
> + * NSEC_PER_SEC
> + */
> + burst = div_u64(params->tcfp_burst * params->rate.rate_bytes_ps,
> + NSEC_PER_SEC);
> +
> + return burst;
> }
>
> static inline u32 tcf_police_tcfp_mtu(const struct tc_action *act)
> diff --git a/net/sched/cls_api.c b/net/sched/cls_api.c
> index 5bfa6b985bb8..cf324807fc42 100644
> --- a/net/sched/cls_api.c
> +++ b/net/sched/cls_api.c
> @@ -3660,7 +3660,7 @@ int tc_setup_flow_action(struct flow_action *flow_action,
> tcf_sample_get_group(entry, act);
> } else if (is_tcf_police(act)) {
> entry->id = FLOW_ACTION_POLICE;
> - entry->police.burst = tcf_police_tcfp_burst(act);
> + entry->police.burst = tcf_police_burst(act);
> entry->police.rate_bytes_ps =
> tcf_police_rate_bytes_ps(act);
> entry->police.mtu = tcf_police_tcfp_mtu(act);
> --
> 2.17.1
>
Thanks,
-Vladimir
Powered by blists - more mailing lists