[PATCH net-next] net: core: change bool members of struct net_device to bitfield members

[PATCH net-next] net: core: change bool members of struct net_device to bitfield members

[Heiner Kallweit]

bool is good as parameter type or function return type, but if used
for struct members it consumes more memory than needed.
Changing the bool members of struct net_device to bitfield members
allows to decrease the memory footprint of this struct.

Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>
---
 include/linux/netdevice.h | 24 +++++++++++++-----------
 1 file changed, 13 insertions(+), 11 deletions(-)

diff --git a/include/linux/netdevice.h b/include/linux/netdevice.h
index 76603ee13..3d7b8df2e 100644
--- a/include/linux/netdevice.h
+++ b/include/linux/netdevice.h
@@ -1651,10 +1651,6 @@ enum netdev_priv_flags {
  * 	@dev_port:		Used to differentiate devices that share
  * 				the same function
  *	@addr_list_lock:	XXX: need comments on this one
- *	@uc_promisc:		Counter that indicates promiscuous mode
- *				has been enabled due to the need to listen to
- *				additional unicast addresses in a device that
- *				does not implement ndo_set_rx_mode()
  *	@uc:			unicast mac addresses
  *	@mc:			multicast mac addresses
  *	@dev_addrs:		list of device hw addresses
@@ -1714,11 +1710,9 @@ enum netdev_priv_flags {
  *	@link_watch_list:	XXX: need comments on this one
  *
  *	@reg_state:		Register/unregister state machine
- *	@dismantle:		Device is going to be freed
  *	@rtnl_link_state:	This enum represents the phases of creating
  *				a new link
  *
- *	@needs_free_netdev:	Should unregister perform free_netdev?
  *	@priv_destructor:	Called from unregister
  *	@npinfo:		XXX: need comments on this one
  * 	@nd_net:		Network namespace this network device is inside
@@ -1758,6 +1752,15 @@ enum netdev_priv_flags {
  *	@qdisc_tx_busylock: lockdep class annotating Qdisc->busylock spinlock
  *	@qdisc_running_key: lockdep class annotating Qdisc->running seqcount
  *
+ *	@uc_promisc:	Counter that indicates promiscuous mode
+ *			has been enabled due to the need to listen to
+ *			additional unicast addresses in a device that
+ *			does not implement ndo_set_rx_mode()
+ *
+ *	@dismantle:	Device is going to be freed
+ *
+ *	@needs_free_netdev:	Should unregister perform free_netdev?
+ *
  *	@proto_down:	protocol port state information can be sent to the
  *			switch driver and used to set the phys state of the
  *			switch port.
@@ -1879,7 +1882,6 @@ struct net_device {
 	unsigned short          dev_port;
 	spinlock_t		addr_list_lock;
 	unsigned char		name_assign_type;
-	bool			uc_promisc;
 	struct netdev_hw_addr_list	uc;
 	struct netdev_hw_addr_list	mc;
 	struct netdev_hw_addr_list	dev_addrs;
@@ -1986,14 +1988,11 @@ struct net_device {
 	       NETREG_DUMMY,		/* dummy device for NAPI poll */
 	} reg_state:8;
 
-	bool dismantle;
-
 	enum {
 		RTNL_LINK_INITIALIZED,
 		RTNL_LINK_INITIALIZING,
 	} rtnl_link_state:16;
 
-	bool needs_free_netdev;
 	void (*priv_destructor)(struct net_device *dev);
 
 #ifdef CONFIG_NETPOLL
@@ -2046,7 +2045,10 @@ struct net_device {
 	struct sfp_bus		*sfp_bus;
 	struct lock_class_key	*qdisc_tx_busylock;
 	struct lock_class_key	*qdisc_running_key;
-	bool			proto_down;
+	unsigned		uc_promisc:1;
+	unsigned		dismantle:1;
+	unsigned		needs_free_netdev:1;
+	unsigned		proto_down:1;
 	unsigned		wol_enabled:1;
 };
 #define to_net_dev(d) container_of(d, struct net_device, dev)
-- 
2.19.1

Re: [PATCH net-next] net: core: change bool members of struct net_device to bitfield members

[Randy Dunlap]

On 10/8/18 1:00 PM, Heiner Kallweit wrote:
> bool is good as parameter type or function return type, but if used
> for struct members it consumes more memory than needed.
> Changing the bool members of struct net_device to bitfield members
> allows to decrease the memory footprint of this struct.
> 
> Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>
> ---
>  include/linux/netdevice.h | 24 +++++++++++++-----------
>  1 file changed, 13 insertions(+), 11 deletions(-)
> 
> diff --git a/include/linux/netdevice.h b/include/linux/netdevice.h
> index 76603ee13..3d7b8df2e 100644
> --- a/include/linux/netdevice.h
> +++ b/include/linux/netdevice.h
> @@ -1651,10 +1651,6 @@ enum netdev_priv_flags {
>   * 	@dev_port:		Used to differentiate devices that share
>   * 				the same function
>   *	@addr_list_lock:	XXX: need comments on this one
> - *	@uc_promisc:		Counter that indicates promiscuous mode
> - *				has been enabled due to the need to listen to
> - *				additional unicast addresses in a device that
> - *				does not implement ndo_set_rx_mode()
>   *	@uc:			unicast mac addresses
>   *	@mc:			multicast mac addresses
>   *	@dev_addrs:		list of device hw addresses
> @@ -1714,11 +1710,9 @@ enum netdev_priv_flags {
>   *	@link_watch_list:	XXX: need comments on this one
>   *
>   *	@reg_state:		Register/unregister state machine
> - *	@dismantle:		Device is going to be freed
>   *	@rtnl_link_state:	This enum represents the phases of creating
>   *				a new link
>   *
> - *	@needs_free_netdev:	Should unregister perform free_netdev?
>   *	@priv_destructor:	Called from unregister
>   *	@npinfo:		XXX: need comments on this one
>   * 	@nd_net:		Network namespace this network device is inside
> @@ -1758,6 +1752,15 @@ enum netdev_priv_flags {
>   *	@qdisc_tx_busylock: lockdep class annotating Qdisc->busylock spinlock
>   *	@qdisc_running_key: lockdep class annotating Qdisc->running seqcount
>   *
> + *	@uc_promisc:	Counter that indicates promiscuous mode
> + *			has been enabled due to the need to listen to
> + *			additional unicast addresses in a device that
> + *			does not implement ndo_set_rx_mode()

Hi,

I see that all you did is copy/paste that text (above), but I wouldn't call
a single bit a [1-bit] Counter.

thanks,
-- 
~Randy

Re: [PATCH net-next] net: core: change bool members of struct net_device to bitfield members

[Heiner Kallweit]

On 08.10.2018 22:07, Randy Dunlap wrote:
> On 10/8/18 1:00 PM, Heiner Kallweit wrote:
>> bool is good as parameter type or function return type, but if used
>> for struct members it consumes more memory than needed.
>> Changing the bool members of struct net_device to bitfield members
>> allows to decrease the memory footprint of this struct.
>>
>> Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>
>> ---
>>  include/linux/netdevice.h | 24 +++++++++++++-----------
>>  1 file changed, 13 insertions(+), 11 deletions(-)
>>
>> diff --git a/include/linux/netdevice.h b/include/linux/netdevice.h
>> index 76603ee13..3d7b8df2e 100644
>> --- a/include/linux/netdevice.h
>> +++ b/include/linux/netdevice.h
>> @@ -1651,10 +1651,6 @@ enum netdev_priv_flags {
>>   * 	@dev_port:		Used to differentiate devices that share
>>   * 				the same function
>>   *	@addr_list_lock:	XXX: need comments on this one
>> - *	@uc_promisc:		Counter that indicates promiscuous mode
>> - *				has been enabled due to the need to listen to
>> - *				additional unicast addresses in a device that
>> - *				does not implement ndo_set_rx_mode()
>>   *	@uc:			unicast mac addresses
>>   *	@mc:			multicast mac addresses
>>   *	@dev_addrs:		list of device hw addresses
>> @@ -1714,11 +1710,9 @@ enum netdev_priv_flags {
>>   *	@link_watch_list:	XXX: need comments on this one
>>   *
>>   *	@reg_state:		Register/unregister state machine
>> - *	@dismantle:		Device is going to be freed
>>   *	@rtnl_link_state:	This enum represents the phases of creating
>>   *				a new link
>>   *
>> - *	@needs_free_netdev:	Should unregister perform free_netdev?
>>   *	@priv_destructor:	Called from unregister
>>   *	@npinfo:		XXX: need comments on this one
>>   * 	@nd_net:		Network namespace this network device is inside
>> @@ -1758,6 +1752,15 @@ enum netdev_priv_flags {
>>   *	@qdisc_tx_busylock: lockdep class annotating Qdisc->busylock spinlock
>>   *	@qdisc_running_key: lockdep class annotating Qdisc->running seqcount
>>   *
>> + *	@uc_promisc:	Counter that indicates promiscuous mode
>> + *			has been enabled due to the need to listen to
>> + *			additional unicast addresses in a device that
>> + *			does not implement ndo_set_rx_mode()
> 
> Hi,
> 
> I see that all you did is copy/paste that text (above), but I wouldn't call
> a single bit a [1-bit] Counter.
> 
I stumbled across this comment too. Neither a bool member nor a 1-bit
bitfield member should be called a counter. I kept the original comment, 
but I'm totally fine with changing Counter -> Flag and will provide a v2.

> thanks,
> 

Re: [PATCH net-next] net: core: change bool members of struct net_device to bitfield members

[Stephen Hemminger]

On Mon, 8 Oct 2018 22:00:51 +0200
Heiner Kallweit <hkallweit1@gmail.com> wrote:

>   *
> + *	@uc_promisc:	Counter that indicates promiscuous mode
> + *			has been enabled due to the need to listen to
> + *			additional unicast addresses in a device that
> + *			does not implement ndo_set_rx_mode()
> + *

I see you just moved the pre-existing comment, but it the comment
looks incorrect. uc_promisc is not a counter but a flag. A counter would
have more than two states normally.

Re: [PATCH net-next] net: core: change bool members of struct net_device to bitfield members

[Heiner Kallweit]

On 08.10.2018 23:20, Stephen Hemminger wrote:
> On Mon, 8 Oct 2018 22:00:51 +0200
> Heiner Kallweit <hkallweit1@gmail.com> wrote:
> 
>>   *
>> + *	@uc_promisc:	Counter that indicates promiscuous mode
>> + *			has been enabled due to the need to listen to
>> + *			additional unicast addresses in a device that
>> + *			does not implement ndo_set_rx_mode()
>> + *
> 
> I see you just moved the pre-existing comment, but it the comment
> looks incorrect. uc_promisc is not a counter but a flag. A counter would
> have more than two states normally.
> 
Right. A v2 fixing the comment has been submitted already.

RE: [PATCH net-next] net: core: change bool members of struct net_device to bitfield members

[David Laight]

From: Heiner Kallweit
> Sent: 08 October 2018 21:01
> 
> bool is good as parameter type or function return type, but if used
> for struct members it consumes more memory than needed.

Actually it can generate extra code when used as a parameter or
return type - but DM doesn't seem to worry about that,

> Changing the bool members of struct net_device to bitfield members
> allows to decrease the memory footprint of this struct.

On archs where bool is 8 bit does this actually make any difference?
Especially since the structure is probably malloced.

> Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>
> ---
>  include/linux/netdevice.h | 24 +++++++++++++-----------
>  1 file changed, 13 insertions(+), 11 deletions(-)
> 
> diff --git a/include/linux/netdevice.h b/include/linux/netdevice.h
> index 76603ee13..3d7b8df2e 100644
> --- a/include/linux/netdevice.h
> +++ b/include/linux/netdevice.h
...
> @@ -1879,7 +1882,6 @@ struct net_device {
>  	unsigned short          dev_port;
>  	spinlock_t		addr_list_lock;
>  	unsigned char		name_assign_type;
> -	bool			uc_promisc;

You've a great big hole here.
I suspect there is one after dev_port as well.

>  	struct netdev_hw_addr_list	uc;
>  	struct netdev_hw_addr_list	mc;
>  	struct netdev_hw_addr_list	dev_addrs;
> @@ -1986,14 +1988,11 @@ struct net_device {
>  	       NETREG_DUMMY,		/* dummy device for NAPI poll */
>  	} reg_state:8;
> 
> -	bool dismantle;
> -
>  	enum {
>  		RTNL_LINK_INITIALIZED,
>  		RTNL_LINK_INITIALIZING,
>  	} rtnl_link_state:16;
> 
> -	bool needs_free_netdev;

This one might remove some padding.
But it could be moved into one of the holes.

>  	void (*priv_destructor)(struct net_device *dev);
> 
>  #ifdef CONFIG_NETPOLL
> @@ -2046,7 +2045,10 @@ struct net_device {
>  	struct sfp_bus		*sfp_bus;
>  	struct lock_class_key	*qdisc_tx_busylock;
>  	struct lock_class_key	*qdisc_running_key;
> -	bool			proto_down;
> +	unsigned		uc_promisc:1;
> +	unsigned		dismantle:1;
> +	unsigned		needs_free_netdev:1;
> +	unsigned		proto_down:1;
>  	unsigned		wol_enabled:1;
>  };
>  #define to_net_dev(d) container_of(d, struct net_device, dev)
> --
> 2.19.1

	David

-
Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
Registration No: 1397386 (Wales)