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

[Alexander Lobakin]

From: Jakub Kicinski <kuba@xxxxxxxxxx>
Date: Wed, 29 May 2024 18:34:09 -0700

> On Tue, 28 May 2024 15:48:35 +0200 Alexander Lobakin wrote:
>> diff --git a/scripts/kernel-doc b/scripts/kernel-doc
>> index 95a59ac78f82..d0cf9a2d82de 100755
>> --- a/scripts/kernel-doc
>> +++ b/scripts/kernel-doc
>> @@ -1155,6 +1155,7 @@ sub dump_struct($$) {
>>          $members =~ s/\bstruct_group_attr\s*\(([^,]*,){2}/STRUCT_GROUP(/gos;
>>          $members =~ s/\bstruct_group_tagged\s*\(([^,]*),([^,]*),/struct $1 $2; STRUCT_GROUP(/gos;
>>          $members =~ s/\b__struct_group\s*\(([^,]*,){3}/STRUCT_GROUP(/gos;
>> +        $members =~ s/\blibeth_cacheline_group\s*\(([^,]*,)/struct { } $1; STRUCT_GROUP(/gos;
>>          $members =~ s/\bSTRUCT_GROUP(\(((?:(?>[^)(]+)|(?1))*)\))[^;]*;/$2/gos;
>>  
>>          my $args = qr{([^,)]+)};
> 
> 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)

> 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.
Why is it too much if we have the same stuff for the netdev core?

Thanks,
Olek