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

[Jakub Kicinski]

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.