| Message ID | 20231009144048.73130-1-wenjia@linux.ibm.com (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | a950a5921db450c74212327f69950ff03419483a |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | [net] net/smc: Fix pos miscalculation in statistics | expand |
On Mon, Oct 09, 2023 at 04:40:48PM +0200, Wenjia Zhang wrote: >From: Nils Hoppmann <niho@linux.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@linux.ibm.com> >Signed-off-by: Nils Hoppmann <niho@linux.ibm.com> >Reviewed-by: Halil Pasic <pasic@linux.ibm.com> >Reviewed-by: Wenjia Zhang <wenjia@linux.ibm.com> Good catch, thanks ! Reviewed-by: Dust Li <dust.li@linux.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
Hello: This patch was applied to netdev/net.git (main) by David S. Miller <davem@davemloft.net>: On Mon, 9 Oct 2023 16:40:48 +0200 you wrote: > From: Nils Hoppmann <niho@linux.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. > > [...] Here is the summary with links: - [net] net/smc: Fix pos miscalculation in statistics https://git.kernel.org/netdev/net/c/a950a5921db4 You are awesome, thank you!
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)