[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <9fbf960d-f279-4e31-90f0-0243eeb7298f@linux.alibaba.com>
Date: Tue, 6 Aug 2024 21:07:40 +0800
From: Wen Gu <guwen@...ux.alibaba.com>
To: Simon Horman <horms@...nel.org>
Cc: wenjia@...ux.ibm.com, jaka@...ux.ibm.com, davem@...emloft.net,
edumazet@...gle.com, kuba@...nel.org, pabeni@...hat.com,
alibuda@...ux.alibaba.com, tonylu@...ux.alibaba.com,
linux-kernel@...r.kernel.org, linux-s390@...r.kernel.org,
netdev@...r.kernel.org
Subject: Re: [PATCH net-next 2/2] net/smc: introduce statistics for ringbufs
usage of net namespace
On 2024/8/6 18:49, Simon Horman wrote:
> On Mon, Aug 05, 2024 at 05:05:51PM +0800, Wen Gu wrote:
>> The buffer size histograms in smc_stats, namely rx/tx_rmbsize, record
>> the sizes of ringbufs for all connections that have ever appeared in
>> the net namespace. They are incremental and we cannot know the actual
>> ringbufs usage from these. So here introduces statistics for current
>> ringbufs usage of existing smc connections in the net namespace into
>> smc_stats, it will be incremented when new connection uses a ringbuf
>> and decremented when the ringbuf is unused.
>>
>> Signed-off-by: Wen Gu <guwen@...ux.alibaba.com>
>
> ...
>
>> diff --git a/net/smc/smc_stats.h b/net/smc/smc_stats.h
>
> ...
>
>> @@ -135,38 +137,45 @@ do { \
>> } \
>> while (0)
>>
>> -#define SMC_STAT_RMB_SIZE_SUB(_smc_stats, _tech, k, _len) \
>> +#define SMC_STAT_RMB_SIZE_SUB(_smc_stats, _tech, k, _is_add, _len) \
>> do { \
>> + typeof(_is_add) is_a = (_is_add); \
>> typeof(_len) _l = (_len); \
>> typeof(_tech) t = (_tech); \
>> int _pos; \
>> int m = SMC_BUF_MAX - 1; \
>> 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]); \
>> + if (is_a) { \
>> + _pos = fls((_l - 1) >> 13); \
>> + _pos = (_pos <= m) ? _pos : m; \
>> + this_cpu_inc((*(_smc_stats)).smc[t].k ## _rmbsize.buf[_pos]); \
>> + this_cpu_add((*(_smc_stats)).smc[t].k ## _rmbuse, _l); \
>
> Nit:
>
> I see that due to the construction of the caller, SMC_STAT_RMB_SIZE(),
> it will not occur. But checkpatch warns of possible side effects
> from reuse of _smc_stats.
>
> As great care seems to have been taken in these macros to avoid such
> problems, even if theoretical, perhaps it is worth doing so here too.
>
> f.e. A macro-local variable could store (*(_smc_stats)).smc[t] which
> I think would both resolve the problem mentioned, and make some
> lines shorter (and maybe easier to read).
>
It makes sense. I will use a macro-local variable of smc_stats. Thank you!
>> + } else { \
>> + this_cpu_sub((*(_smc_stats)).smc[t].k ## _rmbuse, _l); \
>> + } \
>> } \
>> while (0)
>>
>> #define SMC_STAT_RMB_SUB(_smc_stats, type, t, key) \
>> this_cpu_inc((*(_smc_stats)).smc[t].rmb ## _ ## key.type ## _cnt)
>>
>> -#define SMC_STAT_RMB_SIZE(_smc, _is_smcd, _is_rx, _len) \
>> +#define SMC_STAT_RMB_SIZE(_smc, _is_smcd, _is_rx, _is_add, _len) \
>> do { \
>> struct net *_net = sock_net(&(_smc)->sk); \
>> struct smc_stats __percpu *_smc_stats = _net->smc.smc_stats; \
>> + typeof(_is_add) is_add = (_is_add); \
>> typeof(_is_smcd) is_d = (_is_smcd); \
>> typeof(_is_rx) is_r = (_is_rx); \
>> typeof(_len) l = (_len); \
>> if ((is_d) && (is_r)) \
>> - SMC_STAT_RMB_SIZE_SUB(_smc_stats, SMC_TYPE_D, rx, l); \
>> + SMC_STAT_RMB_SIZE_SUB(_smc_stats, SMC_TYPE_D, rx, is_add, l); \
>> if ((is_d) && !(is_r)) \
>> - SMC_STAT_RMB_SIZE_SUB(_smc_stats, SMC_TYPE_D, tx, l); \
>> + SMC_STAT_RMB_SIZE_SUB(_smc_stats, SMC_TYPE_D, tx, is_add, l); \
>> if (!(is_d) && (is_r)) \
>> - SMC_STAT_RMB_SIZE_SUB(_smc_stats, SMC_TYPE_R, rx, l); \
>> + SMC_STAT_RMB_SIZE_SUB(_smc_stats, SMC_TYPE_R, rx, is_add, l); \
>> if (!(is_d) && !(is_r)) \
>> - SMC_STAT_RMB_SIZE_SUB(_smc_stats, SMC_TYPE_R, tx, l); \
>> + SMC_STAT_RMB_SIZE_SUB(_smc_stats, SMC_TYPE_R, tx, is_add, l); \
>> } \
>> while (0)
>>
>> --
>> 2.32.0.3.g01195cf9f
>>
>>
Powered by blists - more mailing lists