[PATCH net] net/smc: Fix pos miscalculation in statistics

[PATCH net] net/smc: Fix pos miscalculation in statistics

[Wenjia Zhang]

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>
---
 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

Re: [PATCH net] net/smc: Fix pos miscalculation in statistics

[Dust Li]

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

Re: [PATCH net] net/smc: Fix pos miscalculation in statistics

[patchwork-bot+netdevbpf]

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!
-- 
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html