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

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

Date: Wed, 12 Jun 2024 15:03:07 +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: 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

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

Thanks,
Olek