netdev - Re: [PATCH iwl-next 01/12] libeth: add cacheline / struct alignment helpers

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] [thread-next>] [day] [month] [year] [list]

Date: Thu, 13 Jun 2024 06:47:06 -0700
From: Jakub Kicinski <kuba@...nel.org>
To: Alexander Lobakin <aleksander.lobakin@...el.com>
Cc: <intel-wired-lan@...ts.osuosl.org>, Tony Nguyen
 <anthony.l.nguyen@...el.com>, "David S. Miller" <davem@...emloft.net>,
 "Eric Dumazet" <edumazet@...gle.com>, Paolo Abeni <pabeni@...hat.com>, Mina
 Almasry <almasrymina@...gle.com>,
 <nex.sw.ncis.osdt.itp.upstreaming@...el.com>, <netdev@...r.kernel.org>,
 <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH iwl-next 01/12] libeth: add cacheline / struct alignment
 helpers

On Thu, 13 Jun 2024 12:47:33 +0200 Alexander Lobakin wrote:
> > Having per-driver grouping defines is a no-go.  
> 
> Without it, kdoc warns when I want to describe group fields =\
> 
> > Do you need the defines in the first place?  
> 
> They allow to describe CLs w/o repeating boilerplates like
> 
> 	cacheline_group_begin(blah) __aligned(blah)
> 	fields
> 	cacheline_group_end(blah)

And you assert that your boilerplate is somehow nicer than this?
See my reply to Przemek, I don't think so, and neither do other
maintainers, judging by how the socket grouping was done.
You can add new markers to include the align automatically too, etc.

> > Are you sure the assert you're adding are not going to explode
> > on some weird arch? Honestly, patch 5 feels like a little too  
> 
> I was adjusting and testing it a lot and CI finally started building
> every arch with no issues some time ago, so yes, I'm sure.
> 64-byte CL on 64-bit arch behaves the same everywhere, so the assertions
> for it can be more strict. On other arches, the behaviour is the same as
> how Eric asserts netdev cachelines in the core code.
> 
> > much for a driver..  
> 
> We had multiple situations when our team were optimizing the structure
> layout and then someone added a new field and messed up the layout
> again. So I ended up with strict assertions.

I understand. Not 100% sure I agree but depends on the team, so okay.

> Why is it too much if we have the same stuff for the netdev core?

But we didn't add tcp_* macros and sock_* macros etc.
Improve the stuff in cache.h is you think its worth it.
And no struct_groups() please.