* [PATCH net-next] net: core: change bool members of struct net_device to bitfield members
@ 2018-10-08 20:00 Heiner Kallweit
2018-10-08 20:07 ` Randy Dunlap
` (2 more replies)
0 siblings, 3 replies; 6+ messages in thread
From: Heiner Kallweit @ 2018-10-08 20:00 UTC (permalink / raw)
To: David Miller; +Cc: netdev
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
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH net-next] net: core: change bool members of struct net_device to bitfield members
2018-10-08 20:00 [PATCH net-next] net: core: change bool members of struct net_device to bitfield members Heiner Kallweit
@ 2018-10-08 20:07 ` Randy Dunlap
2018-10-08 20:14 ` Heiner Kallweit
2018-10-08 21:20 ` Stephen Hemminger
2018-10-09 8:47 ` David Laight
2 siblings, 1 reply; 6+ messages in thread
From: Randy Dunlap @ 2018-10-08 20:07 UTC (permalink / raw)
To: Heiner Kallweit, David Miller; +Cc: netdev
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
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH net-next] net: core: change bool members of struct net_device to bitfield members
2018-10-08 20:07 ` Randy Dunlap
@ 2018-10-08 20:14 ` Heiner Kallweit
0 siblings, 0 replies; 6+ messages in thread
From: Heiner Kallweit @ 2018-10-08 20:14 UTC (permalink / raw)
To: Randy Dunlap, David Miller; +Cc: netdev
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,
>
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH net-next] net: core: change bool members of struct net_device to bitfield members
2018-10-08 20:00 [PATCH net-next] net: core: change bool members of struct net_device to bitfield members Heiner Kallweit
2018-10-08 20:07 ` Randy Dunlap
@ 2018-10-08 21:20 ` Stephen Hemminger
2018-10-08 21:21 ` Heiner Kallweit
2018-10-09 8:47 ` David Laight
2 siblings, 1 reply; 6+ messages in thread
From: Stephen Hemminger @ 2018-10-08 21:20 UTC (permalink / raw)
To: Heiner Kallweit; +Cc: David Miller, netdev
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.
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH net-next] net: core: change bool members of struct net_device to bitfield members
2018-10-08 21:20 ` Stephen Hemminger
@ 2018-10-08 21:21 ` Heiner Kallweit
0 siblings, 0 replies; 6+ messages in thread
From: Heiner Kallweit @ 2018-10-08 21:21 UTC (permalink / raw)
To: Stephen Hemminger; +Cc: David Miller, netdev
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.
^ permalink raw reply [flat|nested] 6+ messages in thread
* RE: [PATCH net-next] net: core: change bool members of struct net_device to bitfield members
2018-10-08 20:00 [PATCH net-next] net: core: change bool members of struct net_device to bitfield members Heiner Kallweit
2018-10-08 20:07 ` Randy Dunlap
2018-10-08 21:20 ` Stephen Hemminger
@ 2018-10-09 8:47 ` David Laight
2 siblings, 0 replies; 6+ messages in thread
From: David Laight @ 2018-10-09 8:47 UTC (permalink / raw)
To: 'Heiner Kallweit', David Miller; +Cc: netdev
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)
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2018-10-09 16:03 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2018-10-08 20:00 [PATCH net-next] net: core: change bool members of struct net_device to bitfield members Heiner Kallweit
2018-10-08 20:07 ` Randy Dunlap
2018-10-08 20:14 ` Heiner Kallweit
2018-10-08 21:20 ` Stephen Hemminger
2018-10-08 21:21 ` Heiner Kallweit
2018-10-09 8:47 ` David Laight
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).