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

[Dust Li <dust.li@linux.alibaba.com>]

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