Re: [Intel-wired-lan] [PATCH iwl-next 05/12] idpf: strictly assert cachelines of queue and queue vector structures

[Jacob Keller <jacob.e.keller@intel.com>]

On 6/12/2024 6:08 AM, Alexander Lobakin wrote:
> From: Alexander Lobakin <aleksander.lobakin@intel.com>
> Date: Wed, 12 Jun 2024 15:03:07 +0200
> 
>> From: Jacob Keller <jacob.e.keller@intel.com>
>> Date: Tue, 28 May 2024 17:43:34 -0700
>>
>>>
>>>
>>> On 5/28/2024 6:48 AM, Alexander Lobakin wrote:
>>>> Now that the queue and queue vector structures are separated and laid
>>>> out optimally, group the fields as read-mostly, read-write, and cold
>>>> cachelines and add size assertions to make sure new features won't push
>>>> something out of its place and provoke perf regression.
>>>
>>>
>>>
>>>> Despite looking innocent, this gives up to 2% of perf bump on Rx.
>>>>
>>>
>>> Could you explain this a bit more for my education? This patch does
>>> clearly change the layout from what it was before this patch, but the
>>> commit message here claims it was already laid out optimally? I guess
>>> that wasn't 100% true? Or do these group field macros also provide
>>> further hints to the compiler about read_mostly or cold, etc?
>>
>> Queue structure split placed fields grouped more optimally, but didn't
>> place ro/rw/cold into separate cachelines. This commit performs the
>> separation via libeth_cacheline_group(). Doing that in one commit didn't
>> look atomically, especially given that the queue split is already big
>> enough.
>>
>>>
>>>> Reviewed-by: Przemek Kitszel <przemyslaw.kitszel@intel.com>
>>>> Signed-off-by: Alexander Lobakin <aleksander.lobakin@intel.com>
>>>> ---
>>>
>>> Having the compiler assert some of this so that we can more easily spot
>>> regressions in the layout is a big benefit.
>>
>> [...]
>>
>>>> @@ -504,59 +505,70 @@ struct idpf_intr_reg {
>>>>  
>>>>  /**
>>>>   * struct idpf_q_vector
>>>> + * @read_mostly: CL group with rarely written hot fields
>>>
>>> I wonder if there is a good way to format the doc here since we almost
>>> want read_mostly to be some sort of header making it clear which fields
>>> belong to it? I don't know how we'd achieve that with current kdoc though.
>>
>> Since commit [0], we need to explicitly describe struct groups in kdocs.
>> @read_mostly and friends are struct groups themselves and in the first
>> patch, where I add these macros, I also add them to the kdoc script, so
>> that it treats them as struct groups, thus they also need to be described.
>> Given that one may use libeth_cacheline_group() to declare some custom
>> groups, like
>>
>> 	libeth_cacheline_group(my_cl,
>> 		fields
>> 	);
>>
>> it makes sense as I'd like to know what this @my_cl is about. Here I use
>> "default" CL names, so this kdocs looks like Ctrl-{C,V} explaining
>> obvious things :D
> 
> Sorry, I read your comment badly =\
> I think this is enough to have it the way it is right now, as you anyway
> has something like:
> 
> * @read_mostly: read-mostly hotpath fields
> * @rm_field1: first read-mostly field
> * @rm_field2: second read-mostly field
> * @read_write: read-write hotpath fields
> * @rw_field1: first read-write field
> ...
> 
> I mean, they are already sorta headers, aren't they? By looking at where
> the next group is described, you can have a picture of which fields
> belong to this one, given that the fields must be described in the same
> order as they're defined in the structure.
> 
> Perhaps we could do
> 
> * @read_mostly: read-mostly hotpath fields
> *  @rm_field1: first read-mostlyfields
> * @read_write: read-write hotpath fields
> 
> i.e. indent the "child" fields, but it doesn't look good I'd say?
>

I was thinking like put a blank line between groups or something, but ya
I think its not really a big deal. Its more than "@read_mostly" looks
like a field name when in reality its more like a group of fields.