Message-ID: <4e53555a-c89e-4621-964b-52b9b694e1d6@intel.com>
Date: Wed, 12 Jun 2024 15:08:14 +0200
From: Alexander Lobakin <aleksander.lobakin@...el.com>
To: Jacob Keller <jacob.e.keller@...el.com>
CC: <intel-wired-lan@...osl.org>, "netdev@...r.kernel.org"
	<netdev@...r.kernel.org>
Subject: Re: [Intel-wired-lan] [PATCH iwl-next 05/12] idpf: strictly assert
 cachelines of queue and queue vector structures

From: Alexander Lobakin <aleksander.lobakin@...el.com>
Date: Wed, 12 Jun 2024 15:03:07 +0200

> From: Jacob Keller <jacob.e.keller@...el.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@...el.com>
>>> Signed-off-by: Alexander Lobakin <aleksander.lobakin@...el.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?

> 
> [0]
> https://git.kernel.org/pub/scm/linux/kernel/git/netdev/net-next.git/commit/?id=5f8e4007c10d
> 
> Thanks,
> Olek

Thanks,
Olek

Powered by blists - more mailing lists