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
 
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [day] [month] [year] [list]
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