diff mbox series

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

Message ID 20240528134846.148890-2-aleksander.lobakin@intel.com (mailing list archive)
State Awaiting Upstream
Delegated to: Netdev Maintainers
Headers show
Series idpf: XDP chapter I: convert Rx to libeth | expand

Checks

Context Check Description
netdev/tree_selection success Guessing tree name failed - patch did not apply, async

Commit Message

Alexander Lobakin May 28, 2024, 1:48 p.m. UTC
Following the latest netdev trend, i.e. effective and usage-based field
cacheline placement, add helpers to group and then assert struct fields
by cachelines.
For 64-bit with 64-byte cachelines, the assertions are more strict as
the size can then be easily predicted. For the rest, just make sure
they don't cross the specified bound.

Signed-off-by: Alexander Lobakin <aleksander.lobakin@intel.com>
---
 scripts/kernel-doc         |   1 +
 include/net/libeth/cache.h | 100 +++++++++++++++++++++++++++++++++++++
 2 files changed, 101 insertions(+)
 create mode 100644 include/net/libeth/cache.h

Comments

Jakub Kicinski May 30, 2024, 1:34 a.m. UTC | #1
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.
Do you need the defines in the first place?
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
much for a driver..
Przemek Kitszel June 12, 2024, 10:07 a.m. UTC | #2
On 5/30/24 03:34, Jakub Kicinski wrote:
> 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.

[1]

> Do you need the defines in the first place?

this patch was a tough one for me too, but I see the idea promising

> 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
> much for a driver..
> 

definitively some of the patch 5 should be added here as doc/example,
but it would be even better to simplify a bit

--

I think that "mark this struct as explicit split into cachelines" is
a hard hard C problem in general, especially in the kernel context, 
*but* I think that this could be simplified for your use case - split
into exactly 3 (possibly empty) sections: mostly-Read, RW, COLD?

Given that it will be a generic solution (would fix the [1] above),
and be also easier to use, like:

  CACHELINE_STRUCT_GROUP(idpf_q_vector,
	CACHELINE_STRUCT_GROUP_RD(/* read mostly */
		struct idpf_vport *vport;
		u16 num_rxq;
		u16 num_txq;
		u16 num_bufq;
		u16 num_complq;
		struct idpf_rx_queue **rx;
		struct idpf_tx_queue **tx;
		struct idpf_buf_queue **bufq;
		struct idpf_compl_queue **complq;
		struct idpf_intr_reg intr_reg;
	),
	CACHELINE_STRUCT_GROUP_RW(
		struct napi_struct napi;
		u16 total_events;
		struct dim tx_dim;
		u16 tx_itr_value;
		bool tx_intr_mode;
		u32 tx_itr_idx;
		struct dim rx_dim;
		u16 rx_itr_value;
		bool rx_intr_mode;
		u32 rx_itr_idx;
	),
	CACHELINE_STRUCT_GROUP_COLD(
		u16 v_idx;
		cpumask_var_t affinity_mask;
	)
);

Note that those three inner macros have distinct meaningful names not to
have this working, but to aid human reader, then checkpatch/check-kdoc.
Technically could be all the same CACHELINE_GROUP().

I'm not sure if (at most) 3 cacheline groups are fine for the general
case, but it would be best to have just one variant of the
CACHELINE_STRUCT_GROUP(), perhaps as a vararg.
Jakub Kicinski June 12, 2024, 8:55 p.m. UTC | #3
On Wed, 12 Jun 2024 12:07:05 +0200 Przemek Kitszel wrote:
> Given that it will be a generic solution (would fix the [1] above),
> and be also easier to use, like:
> 
>   CACHELINE_STRUCT_GROUP(idpf_q_vector,
> 	CACHELINE_STRUCT_GROUP_RD(/* read mostly */
> 		struct idpf_vport *vport;
> 		u16 num_rxq;
> 		u16 num_txq;
> 		u16 num_bufq;
> 		u16 num_complq;
> 		struct idpf_rx_queue **rx;
> 		struct idpf_tx_queue **tx;
> 		struct idpf_buf_queue **bufq;
> 		struct idpf_compl_queue **complq;
> 		struct idpf_intr_reg intr_reg;
> 	),
> 	CACHELINE_STRUCT_GROUP_RW(
> 		struct napi_struct napi;
> 		u16 total_events;
> 		struct dim tx_dim;
> 		u16 tx_itr_value;
> 		bool tx_intr_mode;
> 		u32 tx_itr_idx;
> 		struct dim rx_dim;
> 		u16 rx_itr_value;
> 		bool rx_intr_mode;
> 		u32 rx_itr_idx;
> 	),
> 	CACHELINE_STRUCT_GROUP_COLD(
> 		u16 v_idx;
> 		cpumask_var_t affinity_mask;
> 	)
> );
> 
> Note that those three inner macros have distinct meaningful names not to
> have this working, but to aid human reader, then checkpatch/check-kdoc.
> Technically could be all the same CACHELINE_GROUP().
> 
> I'm not sure if (at most) 3 cacheline groups are fine for the general
> case, but it would be best to have just one variant of the
> CACHELINE_STRUCT_GROUP(), perhaps as a vararg.

I almost want to CC Linus on this because I think it's mostly about
personal preferences. I dislike the struct_group()-style macros. They
don't scale (imagine having to define two partially overlapping groups)
and don't look like C to my eyes. Kees really had to do this for his
memory safety work because we need to communicate a "real struct" type
to the compiler, but if you're just doing this so fail the build and
make the developer stop to think - it's not worth the ugliness.

Can we not extend __cacheline_group_begin() and __cacheline_group_end()
-style markings?
Alexander Lobakin June 13, 2024, 10:47 a.m. UTC | #4
From: Jakub Kicinski <kuba@kernel.org>
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
Jakub Kicinski June 13, 2024, 1:47 p.m. UTC | #5
On Thu, 13 Jun 2024 12:47:33 +0200 Alexander Lobakin wrote:
> > 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)

And you assert that your boilerplate is somehow nicer than this?
See my reply to Przemek, I don't think so, and neither do other
maintainers, judging by how the socket grouping was done.
You can add new markers to include the align automatically too, etc.

> > 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.

I understand. Not 100% sure I agree but depends on the team, so okay.

> Why is it too much if we have the same stuff for the netdev core?

But we didn't add tcp_* macros and sock_* macros etc.
Improve the stuff in cache.h is you think its worth it.
And no struct_groups() please.
diff mbox series

Patch

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{([^,)]+)};
diff --git a/include/net/libeth/cache.h b/include/net/libeth/cache.h
new file mode 100644
index 000000000000..5579240913d2
--- /dev/null
+++ b/include/net/libeth/cache.h
@@ -0,0 +1,100 @@ 
+/* SPDX-License-Identifier: GPL-2.0-only */
+/* Copyright (C) 2024 Intel Corporation */
+
+#ifndef __LIBETH_CACHE_H
+#define __LIBETH_CACHE_H
+
+#include <linux/cache.h>
+
+/* ``__aligned_largest`` is architecture-dependent. Get the actual alignment */
+#define ___LIBETH_LARGEST_ALIGN						   \
+	sizeof(struct { long __UNIQUE_ID(long_); } __aligned_largest)
+#define __LIBETH_LARGEST_ALIGN						   \
+	(___LIBETH_LARGEST_ALIGN > SMP_CACHE_BYTES ?			   \
+	 ___LIBETH_LARGEST_ALIGN : SMP_CACHE_BYTES)
+#define __LIBETH_LARGEST_ALIGNED(sz)					   \
+	ALIGN(sz, __LIBETH_LARGEST_ALIGN)
+
+#define __libeth_cacheline_group_begin(grp)				   \
+	__cacheline_group_begin(grp) __aligned(__LIBETH_LARGEST_ALIGN)
+#define __libeth_cacheline_group_end(grp)				   \
+	__cacheline_group_end(grp) __aligned(sizeof(long))
+
+/**
+ * libeth_cacheline_group - declare a cacheline-aligned field group
+ * @grp: name of the group (usually 'read_mostly', 'read_write', or 'cold')
+ * @...: struct fields inside the group
+ *
+ * Note that the whole group is cacheline-aligned, but the end marker is
+ * aligned to long, so that you pass the (almost) actual field size sum to
+ * the assertion macros below instead of CL-aligned values.
+ * Each cacheline group must be described in struct's kernel-doc.
+ */
+#define libeth_cacheline_group(grp, ...)				   \
+	struct_group(grp,						   \
+		__libeth_cacheline_group_begin(grp);			   \
+		__VA_ARGS__						   \
+		__libeth_cacheline_group_end(grp);			   \
+	)
+
+/**
+ * libeth_cacheline_group_assert - make sure cacheline group size is expected
+ * @type: type of the structure containing the group
+ * @grp: group name inside the struct
+ * @sz: expected group size
+ */
+#if defined(CONFIG_64BIT) && SMP_CACHE_BYTES == 64
+#define libeth_cacheline_group_assert(type, grp, sz)			   \
+	static_assert(offsetof(type, __cacheline_group_end__##grp) -	   \
+		      offsetofend(type, __cacheline_group_begin__##grp) == \
+		      (sz))
+#define __libeth_cacheline_struct_assert(type, sz)			   \
+	static_assert(sizeof(type) == (sz))
+#else /* !CONFIG_64BIT || SMP_CACHE_BYTES != 64 */
+#define libeth_cacheline_group_assert(type, grp, sz)			   \
+	static_assert(offsetof(type, __cacheline_group_end__##grp) -	   \
+		      offsetofend(type, __cacheline_group_begin__##grp) <= \
+		      (sz))
+#define __libeth_cacheline_struct_assert(type, sz)			   \
+	static_assert(sizeof(type) <= (sz))
+#endif /* !CONFIG_64BIT || SMP_CACHE_BYTES != 64 */
+
+#define __libeth_cls1(sz1)						   \
+	__LIBETH_LARGEST_ALIGNED(sz1)
+#define __libeth_cls2(sz1, sz2)						   \
+	(__LIBETH_LARGEST_ALIGNED(sz1) + __LIBETH_LARGEST_ALIGNED(sz2))
+#define __libeth_cls3(sz1, sz2, sz3)					   \
+	(__LIBETH_LARGEST_ALIGNED(sz1) + __LIBETH_LARGEST_ALIGNED(sz2) +   \
+	 __LIBETH_LARGEST_ALIGNED(sz3))
+#define __libeth_cls(...)						   \
+	CONCATENATE(__libeth_cls, COUNT_ARGS(__VA_ARGS__))(__VA_ARGS__)
+
+/**
+ * libeth_cacheline_struct_assert - make sure CL-based struct size is expected
+ * @type: type of the struct
+ * @...: from 1 to 3 CL group sizes (read-mostly, read-write, cold)
+ *
+ * When a struct contains several CL groups, it's difficult to predict its size
+ * on different architectures. The macro instead takes sizes of all of the
+ * groups the structure contains and generates the final struct size.
+ */
+#define libeth_cacheline_struct_assert(type, ...)			   \
+	__libeth_cacheline_struct_assert(type, __libeth_cls(__VA_ARGS__)); \
+	static_assert(__alignof(type) >= __LIBETH_LARGEST_ALIGN)
+
+/**
+ * libeth_cacheline_set_assert - make sure CL-based struct layout is expected
+ * @type: type of the struct
+ * @ro: expected size of the read-mostly group
+ * @rw: expected size of the read-write group
+ * @c: expected size of the cold group
+ *
+ * Check that each group size is expected and then do final struct size check.
+ */
+#define libeth_cacheline_set_assert(type, ro, rw, c)			   \
+	libeth_cacheline_group_assert(type, read_mostly, ro);		   \
+	libeth_cacheline_group_assert(type, read_write, rw);		   \
+	libeth_cacheline_group_assert(type, cold, c);			   \
+	libeth_cacheline_struct_assert(type, ro, rw, c)
+
+#endif /* __LIBETH_CACHE_H */