From: Alexander Lobakin <[email protected]> Date: Wed, 12 Jun 2024 15:03:07 +0200
> From: Jacob Keller <[email protected]> > 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 <[email protected]> >>> Signed-off-by: Alexander Lobakin <[email protected]> >>> --- >> >> 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
