From: Saeed Mahameed <saeed@kernel.org>
To: Arnd Bergmann <arnd@kernel.org>
Cc: "David S. Miller" <davem@davemloft.net>,
Jakub Kicinski <kuba@kernel.org>,
Networking <netdev@vger.kernel.org>,
Parav Pandit <parav@nvidia.com>, Eli Cohen <elic@nvidia.com>
Subject: Re: [net 01/15] net/mlx5e: E-switch, Fix rate calculation for overflow
Date: Mon, 01 Mar 2021 16:52:35 -0800 [thread overview]
Message-ID: <47a3455638c908e3dd7301de3ff41c896bdb765e.camel@kernel.org> (raw)
In-Reply-To: <CAK8P3a0b88NUZUebzxLx5J9Lz4r=s2AmxsWPRTvvQm6ieFnuww@mail.gmail.com>
On Sat, 2021-02-27 at 13:14 +0100, Arnd Bergmann wrote:
> On Fri, Feb 12, 2021 at 3:59 AM Saeed Mahameed <saeed@kernel.org>
> wrote:
> >
> > From: Parav Pandit <parav@nvidia.com>
> >
> > rate_bytes_ps is a 64-bit field. It passed as 32-bit field to
> > apply_police_params(). Due to this when police rate is higher
> > than 4Gbps, 32-bit calculation ignores the carry. This results
> > in incorrect rate configurationn the device.
> >
> > Fix it by performing 64-bit calculation.
>
> I just stumbled over this commit while looking at an unrelated
> problem.
>
> > diff --git a/drivers/net/ethernet/mellanox/mlx5/core/en_tc.c
> > b/drivers/net/ethernet/mellanox/mlx5/core/en_tc.c
> > index dd0bfbacad47..717fbaa6ce73 100644
> > --- a/drivers/net/ethernet/mellanox/mlx5/core/en_tc.c
> > +++ b/drivers/net/ethernet/mellanox/mlx5/core/en_tc.c
> > @@ -5040,7 +5040,7 @@ static int apply_police_params(struct
> > mlx5e_priv *priv, u64 rate,
> > */
> > if (rate) {
> > rate = (rate * BITS_PER_BYTE) + 500000;
> > - rate_mbps = max_t(u32, do_div(rate, 1000000), 1);
> > + rate_mbps = max_t(u64, do_div(rate, 1000000), 1);
>
> I think there are still multiple issues with this line:
>
> - Before commit 1fe3e3166b35 ("net/mlx5e: E-switch, Fix rate
> calculation for
> overflow"), it was trying to calculate rate divided by 1000000, but
> now
> it uses the remainder of the division rather than the quotient. I
> assume
> this was meant to use div_u64() instead of do_div().
>
Yes, I already have a patch lined up to fix this issue.
Thanks for spotting this.
> - Both div_u64() and do_div() return a 32-bit number, and '1' is a
> constant
> that also comfortably fits into a 32-bit number, so changing the
> max_t
> to return a 64-bit type has no effect on the result
>
as of the above comment, we shouldn't be using the return value of
do_div().
> - The maximum of an arbitrary unsigned integer and '1' is either one
> or zero,
> so there doesn't need to be an expensive division here at all.
> From the
> comment it sounds like the intention was to use 'min_t()' instead
> of 'max_t()'.
> It has however used 'max_t' since the code was first introduced.
>
if the input rate is less that 1mbps then the quotient will be 0,
otherwise we want the quotient, and we don't allow 0, so max_t(rate, 1)
should be used, what am I missing ?
next prev parent reply other threads:[~2021-03-02 8:40 UTC|newest]
Thread overview: 19+ messages / expand[flat|nested] mbox.gz Atom feed top
2021-02-12 2:56 [pull request][net 00/15] mlx5 fixes 2021-02-11 Saeed Mahameed
2021-02-12 2:56 ` [net 01/15] net/mlx5e: E-switch, Fix rate calculation for overflow Saeed Mahameed
2021-02-27 12:14 ` Arnd Bergmann
2021-03-02 0:52 ` Saeed Mahameed [this message]
2021-03-02 9:01 ` Arnd Bergmann
2021-02-12 2:56 ` [net 02/15] net/mlx5e: Enable striding RQ for Connect-X IPsec capable devices Saeed Mahameed
2021-02-12 2:56 ` [net 03/15] net/mlx5e: Enable XDP " Saeed Mahameed
2021-02-12 2:56 ` [net 04/15] net/mlx5e: Don't change interrupt moderation params when DIM is enabled Saeed Mahameed
2021-02-12 2:56 ` [net 05/15] net/mlx5e: Change interrupt moderation channel params also when channels are closed Saeed Mahameed
2021-02-12 2:56 ` [net 06/15] net/mlx5: Fix health error state handling Saeed Mahameed
2021-02-12 2:56 ` [net 07/15] net/mlx5e: Replace synchronize_rcu with synchronize_net Saeed Mahameed
2021-02-12 2:56 ` [net 08/15] net/mlx5e: Fix CQ params of ICOSQ and async ICOSQ Saeed Mahameed
2021-02-12 2:56 ` [net 09/15] net/mlx5e: kTLS, Use refcounts to free kTLS RX priv context Saeed Mahameed
2021-02-12 2:56 ` [net 10/15] net/mlx5: Disable devlink reload for multi port slave device Saeed Mahameed
2021-02-12 2:56 ` [net 11/15] net/mlx5: Disallow RoCE on " Saeed Mahameed
2021-02-12 2:56 ` [net 12/15] net/mlx5: Disallow RoCE on lag device Saeed Mahameed
2021-02-12 2:56 ` [net 13/15] net/mlx5: Disable devlink reload for lag devices Saeed Mahameed
2021-02-12 2:56 ` [net 14/15] net/mlx5e: CT: manage the lifetime of the ct entry object Saeed Mahameed
2021-02-12 2:56 ` [net 15/15] net/mlx5e: Check tunnel offload is required before setting SWP Saeed Mahameed
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=47a3455638c908e3dd7301de3ff41c896bdb765e.camel@kernel.org \
--to=saeed@kernel.org \
--cc=arnd@kernel.org \
--cc=davem@davemloft.net \
--cc=elic@nvidia.com \
--cc=kuba@kernel.org \
--cc=netdev@vger.kernel.org \
--cc=parav@nvidia.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).