diff mbox series

[net-next,1/5] netdevice: convert private flags > BIT(31) to bitfields

Message ID 20240625114432.1398320-2-aleksander.lobakin@intel.com (mailing list archive)
State Superseded
Delegated to: Netdev Maintainers
Headers show
Series netdev_features: start cleaning netdev_features_t up | expand

Checks

Context Check Description
netdev/series_format success Posting correctly formatted
netdev/tree_selection success Clearly marked for net-next, async
netdev/ynl success Generated files up to date; no warnings/errors; no diff in generated;
netdev/fixes_present success Fixes tag not required for -next series
netdev/header_inline success No static functions without inline keyword in header files
netdev/build_32bit fail Errors and warnings before: 889 this patch: 890
netdev/build_tools success Errors and warnings before: 0 this patch: 0
netdev/cc_maintainers warning 8 maintainers not CCed: glipus@gmail.com horatiu.vultur@microchip.com vladimir.oltean@nxp.com UNGLinuxDriver@microchip.com amcohen@nvidia.com idosch@nvidia.com b.galvani@gmail.com kory.maincent@bootlin.com
netdev/build_clang success Errors and warnings before: 943 this patch: 943
netdev/verify_signedoff success Signed-off-by tag matches author and committer
netdev/deprecated_api success None detected
netdev/check_selftest success No net selftest shell script
netdev/verify_fixes success No Fixes tag
netdev/build_allmodconfig_warn success Errors and warnings before: 5029 this patch: 5029
netdev/checkpatch warning CHECK: Alignment should match open parenthesis WARNING: line length of 91 exceeds 80 columns
netdev/build_clang_rust success No Rust files in patch. Skipping build
netdev/kdoc fail Errors and warnings before: 110 this patch: 111
netdev/source_inline success Was 0 now: 0

Commit Message

Alexander Lobakin June 25, 2024, 11:44 a.m. UTC
Make dev->priv_flags `u32` back and define bits higher than 31 as
bitfield booleans as per Jakub's suggestion. This simplifies code
which accesses these bits with no optimization loss (testb both
before/after), allows to not extend &netdev_priv_flags each time,
but also scales better as bits > 63 in the future would only add
a new u64 to the structure with no complications, comparing to
that extending ::priv_flags would require converting it to a bitmap.
Note that I picked `unsigned long :1` to not lose any potential
optimizations comparing to `bool :1` etc.

Suggested-by: Jakub Kicinski <kuba@kernel.org>
Signed-off-by: Alexander Lobakin <aleksander.lobakin@intel.com>
---
 include/linux/netdevice.h                     | 27 ++++++++++++-------
 .../ethernet/microchip/lan966x/lan966x_main.c |  2 +-
 drivers/net/macvlan.c                         |  3 ++-
 drivers/net/vxlan/vxlan_core.c                |  3 ++-
 net/8021q/vlanproc.c                          |  2 +-
 net/core/dev.c                                |  4 +--
 net/core/dev_ioctl.c                          |  9 +++----
 net/core/rtnetlink.c                          |  2 +-
 8 files changed, 30 insertions(+), 22 deletions(-)

Comments

Jakub Kicinski June 26, 2024, 2:51 p.m. UTC | #1
On Tue, 25 Jun 2024 13:44:28 +0200 Alexander Lobakin wrote:
> +	struct_group(__priv_flags,
> +		unsigned long		priv_flags:32;
> +		unsigned long		see_all_hwtstamp_requests:1;
> +		unsigned long		change_proto_down:1;
> +	);

I don't think we should group them indiscriminately. Better to add the
asserts flag by flag. Neither of the flags you're breaking out in this
patch are used on the fast path.

Or is the problem that CACHELINE_ASSERT_GROUP_MEMBER doesn't work on
bitfields?
Jakub Kicinski June 26, 2024, 2:54 p.m. UTC | #2
On Tue, 25 Jun 2024 13:44:28 +0200 Alexander Lobakin wrote:
> -		   "%s  VID: %d	 REORDER_HDR: %i  dev->priv_flags: %llx\n",
> +		   "%s  VID: %d	 REORDER_HDR: %i  dev->priv_flags: %x\n",

compiler says %lx

net/8021q/vlanproc.c: In function ‘vlandev_seq_show’:
net/8021q/vlanproc.c:241:69: warning: format ‘%x’ expects argument of type ‘unsigned int’, but argument 6 has type ‘long unsigned int’ [-Wformat=]
  241 |                    "%s  VID: %d  REORDER_HDR: %i  dev->priv_flags: %x\n",
      |                                                                    ~^
      |                                                                     |
      |                                                                     unsigned int
      |                                                                    %lx
  242 |                    vlandev->name, vlan->vlan_id,
  243 |                    (int)(vlan->flags & 1), vlandev->priv_flags);
      |                                            ~~~~~~~~~~~~~~~~~~~       
      |                                                   |
      |                                                   long unsigned int
Alexander Lobakin June 27, 2024, 9:48 a.m. UTC | #3
From: Jakub Kicinski <kuba@kernel.org>
Date: Wed, 26 Jun 2024 07:54:21 -0700

> On Tue, 25 Jun 2024 13:44:28 +0200 Alexander Lobakin wrote:
>> -		   "%s  VID: %d	 REORDER_HDR: %i  dev->priv_flags: %llx\n",
>> +		   "%s  VID: %d	 REORDER_HDR: %i  dev->priv_flags: %x\n",
> 
> compiler says %lx
> 
> net/8021q/vlanproc.c: In function ‘vlandev_seq_show’:
> net/8021q/vlanproc.c:241:69: warning: format ‘%x’ expects argument of type ‘unsigned int’, but argument 6 has type ‘long unsigned int’ [-Wformat=]
>   241 |                    "%s  VID: %d  REORDER_HDR: %i  dev->priv_flags: %x\n",
>       |                                                                    ~^
>       |                                                                     |
>       |                                                                     unsigned int
>       |                                                                    %lx
>   242 |                    vlandev->name, vlan->vlan_id,
>   243 |                    (int)(vlan->flags & 1), vlandev->priv_flags);
>       |                                            ~~~~~~~~~~~~~~~~~~~       
>       |                                                   |
>       |                                                   long unsigned int

Yeah, GCC wants %lx here, but Clang wants %x since priv_flags is
declared as long:32 >_<
I think I'll add a cast here.

Thanks,
Olek
Alexander Lobakin June 27, 2024, 9:50 a.m. UTC | #4
From: Jakub Kicinski <kuba@kernel.org>
Date: Wed, 26 Jun 2024 07:51:17 -0700

> On Tue, 25 Jun 2024 13:44:28 +0200 Alexander Lobakin wrote:
>> +	struct_group(__priv_flags,
>> +		unsigned long		priv_flags:32;
>> +		unsigned long		see_all_hwtstamp_requests:1;
>> +		unsigned long		change_proto_down:1;
>> +	);
> 
> I don't think we should group them indiscriminately. Better to add the
> asserts flag by flag. Neither of the flags you're breaking out in this
> patch are used on the fast path.
> 
> Or is the problem that CACHELINE_ASSERT_GROUP_MEMBER doesn't work on
> bitfields?

It generates sizeof(bitfield) which the compilers don't like and don't
want to compile ._.

Thanks,
Olek
Jakub Kicinski June 27, 2024, 7:55 p.m. UTC | #5
On Thu, 27 Jun 2024 11:50:40 +0200 Alexander Lobakin wrote:
> > I don't think we should group them indiscriminately. Better to add the
> > asserts flag by flag. Neither of the flags you're breaking out in this
> > patch are used on the fast path.
> > 
> > Or is the problem that CACHELINE_ASSERT_GROUP_MEMBER doesn't work on
> > bitfields?  
> 
> It generates sizeof(bitfield) which the compilers don't like and don't
> want to compile ._.

Mm. Okay, I have no better ideas then.

Do consider moving the cold flags next to wol_enabled, tho?
Alexander Lobakin June 28, 2024, 10:37 a.m. UTC | #6
From: Jakub Kicinski <kuba@kernel.org>
Date: Thu, 27 Jun 2024 12:55:41 -0700

> On Thu, 27 Jun 2024 11:50:40 +0200 Alexander Lobakin wrote:
>>> I don't think we should group them indiscriminately. Better to add the
>>> asserts flag by flag. Neither of the flags you're breaking out in this
>>> patch are used on the fast path.
>>>
>>> Or is the problem that CACHELINE_ASSERT_GROUP_MEMBER doesn't work on
>>> bitfields?  
>>
>> It generates sizeof(bitfield) which the compilers don't like and don't
>> want to compile ._.
> 
> Mm. Okay, I have no better ideas then.
> 
> Do consider moving the cold flags next to wol_enabled, tho?

Hmm, sounds good!

Thanks,
Olek
Edward Cree June 28, 2024, 4:03 p.m. UTC | #7
On 27/06/2024 20:55, Jakub Kicinski wrote:
> On Thu, 27 Jun 2024 11:50:40 +0200 Alexander Lobakin wrote:
>>> I don't think we should group them indiscriminately. Better to add the
>>> asserts flag by flag. Neither of the flags you're breaking out in this
>>> patch are used on the fast path.
>>>
>>> Or is the problem that CACHELINE_ASSERT_GROUP_MEMBER doesn't work on
>>> bitfields?  
>>
>> It generates sizeof(bitfield) which the compilers don't like and don't
>> want to compile ._.
> 
> Mm. Okay, I have no better ideas then.
> 
> Do consider moving the cold flags next to wol_enabled, tho?

My RSS series moves wol_enabled out to struct ethtool_netdev_state [1] so
 this may not be worthwhile?

-ed

[1]: https://lore.kernel.org/netdev/293a562278371de7534ed1eb17531838ca090633.1719502239.git.ecree.xilinx@gmail.com/
Jakub Kicinski June 29, 2024, 1:59 a.m. UTC | #8
On Fri, 28 Jun 2024 17:03:10 +0100 Edward Cree wrote:
> >> It generates sizeof(bitfield) which the compilers don't like and don't
> >> want to compile ._.  
> > 
> > Mm. Okay, I have no better ideas then.
> > 
> > Do consider moving the cold flags next to wol_enabled, tho?  
> 
> My RSS series moves wol_enabled out to struct ethtool_netdev_state [1] so
>  this may not be worthwhile?

Speaking of which a new bit just appeared there, for the SFP module
flashing. I'm gonna merge your series because it technically doesn't
impact it, but could you follow up and move that bit to ethtool state?
Edward Cree July 2, 2024, 4:08 p.m. UTC | #9
On 29/06/2024 02:59, Jakub Kicinski wrote:
> Speaking of which a new bit just appeared there, for the SFP module
> flashing. I'm gonna merge your series because it technically doesn't
> impact it, but could you follow up and move that bit to ethtool state?

Sure, can do.
diff mbox series

Patch

diff --git a/include/linux/netdevice.h b/include/linux/netdevice.h
index 4e81660b4462..4fddf57f40d9 100644
--- a/include/linux/netdevice.h
+++ b/include/linux/netdevice.h
@@ -1607,7 +1607,8 @@  struct net_device_ops {
  * userspace; this means that the order of these flags can change
  * during any kernel release.
  *
- * You should have a pretty good reason to be extending these flags.
+ * You should add bitfield booleans after net_device::priv_flags instead
+ * of extending these flags.
  *
  * @IFF_802_1Q_VLAN: 802.1Q VLAN device
  * @IFF_EBRIDGE: Ethernet bridging device
@@ -1646,10 +1647,6 @@  struct net_device_ops {
  * @IFF_NO_ADDRCONF: prevent ipv6 addrconf
  * @IFF_TX_SKB_NO_LINEAR: device/driver is capable of xmitting frames with
  *	skb_headlen(skb) == 0 (data starts from frag0)
- * @IFF_CHANGE_PROTO_DOWN: device supports setting carrier via IFLA_PROTO_DOWN
- * @IFF_SEE_ALL_HWTSTAMP_REQUESTS: device wants to see calls to
- *	ndo_hwtstamp_set() for all timestamp requests regardless of source,
- *	even if those aren't HWTSTAMP_SOURCE_NETDEV.
  */
 enum netdev_priv_flags {
 	IFF_802_1Q_VLAN			= 1<<0,
@@ -1684,8 +1681,6 @@  enum netdev_priv_flags {
 	IFF_L3MDEV_RX_HANDLER		= 1<<29,
 	IFF_NO_ADDRCONF			= BIT_ULL(30),
 	IFF_TX_SKB_NO_LINEAR		= BIT_ULL(31),
-	IFF_CHANGE_PROTO_DOWN		= BIT_ULL(32),
-	IFF_SEE_ALL_HWTSTAMP_REQUESTS	= BIT_ULL(33),
 };
 
 #define IFF_802_1Q_VLAN			IFF_802_1Q_VLAN
@@ -1749,6 +1744,16 @@  enum netdev_reg_state {
  *	data with strictly "high-level" data, and it has to know about
  *	almost every data structure used in the INET module.
  *
+ *	@__priv_flags:	both private flags as bits and as bitfield booleans
+ *			combined, only to assert cacheline placement
+ *	@priv_flags:	flags invisible to userspace defined as bits, see
+ *			enum netdev_priv_flags for the definitions
+ *	@see_all_hwtstamp_requests: device wants to see calls to
+ *			ndo_hwtstamp_set() for all timestamp requests
+ *			regardless of source, even if those aren't
+ *			HWTSTAMP_SOURCE_NETDEV
+ *	@change_proto_down: device supports setting carrier via IFLA_PROTO_DOWN
+ *
  *	@name:	This is the first field of the "visible" part of this structure
  *		(i.e. as seen by users in the "Space.c" file).  It is the name
  *		of the interface.
@@ -1815,8 +1820,6 @@  enum netdev_reg_state {
  *
  *	@flags:		Interface flags (a la BSD)
  *	@xdp_features:	XDP capability supported by the device
- *	@priv_flags:	Like 'flags' but invisible to userspace,
- *			see if.h for the definitions
  *	@gflags:	Global flags ( kept as legacy )
  *	@padded:	How much padding added by alloc_netdev()
  *	@operstate:	RFC2863 operstate
@@ -2039,7 +2042,11 @@  struct net_device {
 
 	/* TX read-mostly hotpath */
 	__cacheline_group_begin(net_device_read_tx);
-	unsigned long long	priv_flags;
+	struct_group(__priv_flags,
+		unsigned long		priv_flags:32;
+		unsigned long		see_all_hwtstamp_requests:1;
+		unsigned long		change_proto_down:1;
+	);
 	const struct net_device_ops *netdev_ops;
 	const struct header_ops *header_ops;
 	struct netdev_queue	*_tx;
diff --git a/drivers/net/ethernet/microchip/lan966x/lan966x_main.c b/drivers/net/ethernet/microchip/lan966x/lan966x_main.c
index ec672af12e25..534d4716d5f7 100644
--- a/drivers/net/ethernet/microchip/lan966x/lan966x_main.c
+++ b/drivers/net/ethernet/microchip/lan966x/lan966x_main.c
@@ -816,7 +816,7 @@  static int lan966x_probe_port(struct lan966x *lan966x, u32 p,
 			 NETIF_F_HW_VLAN_STAG_TX |
 			 NETIF_F_HW_TC;
 	dev->hw_features |= NETIF_F_HW_TC;
-	dev->priv_flags |= IFF_SEE_ALL_HWTSTAMP_REQUESTS;
+	dev->see_all_hwtstamp_requests = true;
 	dev->needed_headroom = IFH_LEN_BYTES;
 
 	eth_hw_addr_gen(dev, lan966x->base_mac, p + 1);
diff --git a/drivers/net/macvlan.c b/drivers/net/macvlan.c
index 67b7ef2d463f..3aa6d33efdf5 100644
--- a/drivers/net/macvlan.c
+++ b/drivers/net/macvlan.c
@@ -1213,7 +1213,8 @@  void macvlan_common_setup(struct net_device *dev)
 	dev->max_mtu		= ETH_MAX_MTU;
 	dev->priv_flags	       &= ~IFF_TX_SKB_SHARING;
 	netif_keep_dst(dev);
-	dev->priv_flags	       |= IFF_UNICAST_FLT | IFF_CHANGE_PROTO_DOWN;
+	dev->priv_flags	       |= IFF_UNICAST_FLT;
+	dev->change_proto_down	= true;
 	dev->netdev_ops		= &macvlan_netdev_ops;
 	dev->needs_free_netdev	= true;
 	dev->priv_destructor	= macvlan_dev_free;
diff --git a/drivers/net/vxlan/vxlan_core.c b/drivers/net/vxlan/vxlan_core.c
index 567cb3faab70..2f3a7c58d302 100644
--- a/drivers/net/vxlan/vxlan_core.c
+++ b/drivers/net/vxlan/vxlan_core.c
@@ -3325,7 +3325,8 @@  static void vxlan_setup(struct net_device *dev)
 	dev->hw_features |= NETIF_F_RXCSUM;
 	dev->hw_features |= NETIF_F_GSO_SOFTWARE;
 	netif_keep_dst(dev);
-	dev->priv_flags |= IFF_NO_QUEUE | IFF_CHANGE_PROTO_DOWN;
+	dev->priv_flags |= IFF_NO_QUEUE;
+	dev->change_proto_down = true;
 
 	/* MTU range: 68 - 65535 */
 	dev->min_mtu = ETH_MIN_MTU;
diff --git a/net/8021q/vlanproc.c b/net/8021q/vlanproc.c
index 87b959da00cd..e8ddf97dad63 100644
--- a/net/8021q/vlanproc.c
+++ b/net/8021q/vlanproc.c
@@ -238,7 +238,7 @@  static int vlandev_seq_show(struct seq_file *seq, void *offset)
 
 	stats = dev_get_stats(vlandev, &temp);
 	seq_printf(seq,
-		   "%s  VID: %d	 REORDER_HDR: %i  dev->priv_flags: %llx\n",
+		   "%s  VID: %d	 REORDER_HDR: %i  dev->priv_flags: %x\n",
 		   vlandev->name, vlan->vlan_id,
 		   (int)(vlan->flags & 1), vlandev->priv_flags);
 
diff --git a/net/core/dev.c b/net/core/dev.c
index b94fb4e63a28..49a88b0ff73b 100644
--- a/net/core/dev.c
+++ b/net/core/dev.c
@@ -9266,7 +9266,7 @@  EXPORT_SYMBOL(netdev_port_same_parent_id);
  */
 int dev_change_proto_down(struct net_device *dev, bool proto_down)
 {
-	if (!(dev->priv_flags & IFF_CHANGE_PROTO_DOWN))
+	if (!dev->change_proto_down)
 		return -EOPNOTSUPP;
 	if (!netif_device_present(dev))
 		return -ENODEV;
@@ -11869,7 +11869,7 @@  static struct pernet_operations __net_initdata default_device_ops = {
 static void __init net_dev_struct_check(void)
 {
 	/* TX read-mostly hotpath */
-	CACHELINE_ASSERT_GROUP_MEMBER(struct net_device, net_device_read_tx, priv_flags);
+	CACHELINE_ASSERT_GROUP_MEMBER(struct net_device, net_device_read_tx, __priv_flags);
 	CACHELINE_ASSERT_GROUP_MEMBER(struct net_device, net_device_read_tx, netdev_ops);
 	CACHELINE_ASSERT_GROUP_MEMBER(struct net_device, net_device_read_tx, header_ops);
 	CACHELINE_ASSERT_GROUP_MEMBER(struct net_device, net_device_read_tx, _tx);
diff --git a/net/core/dev_ioctl.c b/net/core/dev_ioctl.c
index b9719ed3c3fd..3868486fb71f 100644
--- a/net/core/dev_ioctl.c
+++ b/net/core/dev_ioctl.c
@@ -319,8 +319,7 @@  static int dev_get_hwtstamp(struct net_device *dev, struct ifreq *ifr)
  * should take precedence in front of hardware timestamping provided by the
  * netdev. If the netdev driver needs to perform specific actions even for PHY
  * timestamping to work properly (a switch port must trap the timestamped
- * frames and not forward them), it must set IFF_SEE_ALL_HWTSTAMP_REQUESTS in
- * dev->priv_flags.
+ * frames and not forward them), it must set dev->see_all_hwtstamp_requests.
  */
 int dev_set_hwtstamp_phylib(struct net_device *dev,
 			    struct kernel_hwtstamp_config *cfg,
@@ -334,13 +333,13 @@  int dev_set_hwtstamp_phylib(struct net_device *dev,
 
 	cfg->source = phy_ts ? HWTSTAMP_SOURCE_PHYLIB : HWTSTAMP_SOURCE_NETDEV;
 
-	if (phy_ts && (dev->priv_flags & IFF_SEE_ALL_HWTSTAMP_REQUESTS)) {
+	if (phy_ts && dev->see_all_hwtstamp_requests) {
 		err = ops->ndo_hwtstamp_get(dev, &old_cfg);
 		if (err)
 			return err;
 	}
 
-	if (!phy_ts || (dev->priv_flags & IFF_SEE_ALL_HWTSTAMP_REQUESTS)) {
+	if (!phy_ts || dev->see_all_hwtstamp_requests) {
 		err = ops->ndo_hwtstamp_set(dev, cfg, extack);
 		if (err) {
 			if (extack->_msg)
@@ -349,7 +348,7 @@  int dev_set_hwtstamp_phylib(struct net_device *dev,
 		}
 	}
 
-	if (phy_ts && (dev->priv_flags & IFF_SEE_ALL_HWTSTAMP_REQUESTS))
+	if (phy_ts && dev->see_all_hwtstamp_requests)
 		changed = kernel_hwtstamp_config_changed(&old_cfg, cfg);
 
 	if (phy_ts) {
diff --git a/net/core/rtnetlink.c b/net/core/rtnetlink.c
index eabfc8290f5e..cff172f0ff3d 100644
--- a/net/core/rtnetlink.c
+++ b/net/core/rtnetlink.c
@@ -2724,7 +2724,7 @@  static int do_set_proto_down(struct net_device *dev,
 	bool proto_down;
 	int err;
 
-	if (!(dev->priv_flags & IFF_CHANGE_PROTO_DOWN)) {
+	if (!dev->change_proto_down) {
 		NL_SET_ERR_MSG(extack,  "Protodown not supported by device");
 		return -EOPNOTSUPP;
 	}