[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <20231011031422.GJ92403@linux.alibaba.com>
Date: Wed, 11 Oct 2023 11:14:22 +0800
From: Dust Li <dust.li@...ux.alibaba.com>
To: Wenjia Zhang <wenjia@...ux.ibm.com>, David Miller <davem@...emloft.net>,
Jakub Kicinski <kuba@...nel.org>
Cc: netdev@...r.kernel.org, linux-s390@...r.kernel.org,
Eric Dumazet <edumazet@...gle.com>, Paolo Abeni <pabeni@...hat.com>,
Heiko Carstens <hca@...ux.ibm.com>,
Jan Karcher <jaka@...ux.ibm.com>,
Alexandra Winter <wintera@...ux.ibm.com>,
Karsten Graul <kgraul@...ux.ibm.com>,
Stefan Raspl <raspl@...ux.ibm.com>,
Gerd Bayer <gbayer@...ux.ibm.com>,
Thorsten Winkler <twinkler@...ux.ibm.com>,
Halil Pasic <pasic@...ux.ibm.com>,
Nils Hoppmann <niho@...ux.ibm.com>,
Niklas Schnell <schnelle@...ux.ibm.com>,
Tony Lu <tonylu@...ux.alibaba.com>,
Wen Gu <guwen@...ux.alibaba.com>,
"D. Wythe" <alibuda@...ux.alibaba.com>
Subject: Re: [PATCH net] net/smc: Fix pos miscalculation in statistics
On Mon, Oct 09, 2023 at 04:40:48PM +0200, Wenjia Zhang wrote:
>From: Nils Hoppmann <niho@...ux.ibm.com>
>
>SMC_STAT_PAYLOAD_SUB(_smc_stats, _tech, key, _len, _rc) will calculate
>wrong bucket positions for payloads of exactly 4096 bytes and
>(1 << (m + 12)) bytes, with m == SMC_BUF_MAX - 1.
>
>Intended bucket distribution:
>Assume l == size of payload, m == SMC_BUF_MAX - 1.
>
>Bucket 0 : 0 < l <= 2^13
>Bucket n, 1 <= n <= m-1 : 2^(n+12) < l <= 2^(n+13)
>Bucket m : l > 2^(m+12)
>
>Current solution:
>_pos = fls64((l) >> 13)
>[...]
>_pos = (_pos < m) ? ((l == 1 << (_pos + 12)) ? _pos - 1 : _pos) : m
>
>For l == 4096, _pos == -1, but should be _pos == 0.
>For l == (1 << (m + 12)), _pos == m, but should be _pos == m - 1.
>
>In order to avoid special treatment of these corner cases, the
>calculation is adjusted. The new solution first subtracts the length by
>one, and then calculates the correct bucket by shifting accordingly,
>i.e. _pos = fls64((l - 1) >> 13), l > 0.
>This not only fixes the issues named above, but also makes the whole
>bucket assignment easier to follow.
>
>Same is done for SMC_STAT_RMB_SIZE_SUB(_smc_stats, _tech, k, _len),
>where the calculation of the bucket position is similar to the one
>named above.
>
>Fixes: e0e4b8fa5338 ("net/smc: Add SMC statistics support")
>Suggested-by: Halil Pasic <pasic@...ux.ibm.com>
>Signed-off-by: Nils Hoppmann <niho@...ux.ibm.com>
>Reviewed-by: Halil Pasic <pasic@...ux.ibm.com>
>Reviewed-by: Wenjia Zhang <wenjia@...ux.ibm.com>
Good catch, thanks !
Reviewed-by: Dust Li <dust.li@...ux.alibaba.com>
>---
> net/smc/smc_stats.h | 14 +++++++++-----
> 1 file changed, 9 insertions(+), 5 deletions(-)
>
>diff --git a/net/smc/smc_stats.h b/net/smc/smc_stats.h
>index aa8928975cc6..9d32058db2b5 100644
>--- a/net/smc/smc_stats.h
>+++ b/net/smc/smc_stats.h
>@@ -92,13 +92,14 @@ do { \
> typeof(_smc_stats) stats = (_smc_stats); \
> typeof(_tech) t = (_tech); \
> typeof(_len) l = (_len); \
>- int _pos = fls64((l) >> 13); \
>+ int _pos; \
> typeof(_rc) r = (_rc); \
> int m = SMC_BUF_MAX - 1; \
> this_cpu_inc((*stats).smc[t].key ## _cnt); \
>- if (r <= 0) \
>+ if (r <= 0 || l <= 0) \
> break; \
>- _pos = (_pos < m) ? ((l == 1 << (_pos + 12)) ? _pos - 1 : _pos) : m; \
>+ _pos = fls64((l - 1) >> 13); \
>+ _pos = (_pos <= m) ? _pos : m; \
> this_cpu_inc((*stats).smc[t].key ## _pd.buf[_pos]); \
> this_cpu_add((*stats).smc[t].key ## _bytes, r); \
> } \
>@@ -138,9 +139,12 @@ while (0)
> do { \
> typeof(_len) _l = (_len); \
> typeof(_tech) t = (_tech); \
>- int _pos = fls((_l) >> 13); \
>+ int _pos; \
> int m = SMC_BUF_MAX - 1; \
>- _pos = (_pos < m) ? ((_l == 1 << (_pos + 12)) ? _pos - 1 : _pos) : m; \
>+ if (_l <= 0) \
>+ break; \
>+ _pos = fls((_l - 1) >> 13); \
>+ _pos = (_pos <= m) ? _pos : m; \
> this_cpu_inc((*(_smc_stats)).smc[t].k ## _rmbsize.buf[_pos]); \
> } \
> while (0)
>--
>2.40.1
Powered by blists - more mailing lists