netdev - Re: [PATCH net-next 2/2] net/smc: introduce statistics for ringbufs usage of net namespace

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <20240806152632.GB2636630@kernel.org>
Date: Tue, 6 Aug 2024 16:26:32 +0100
From: Simon Horman <horms@...nel.org>
To: Wen Gu <guwen@...ux.alibaba.com>
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 Tue, Aug 06, 2024 at 09:07:40PM +0800, Wen Gu wrote:
> 
> 
> 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!

Great, thanks.