netdev.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH 0/3 net-next] Bring sizeof(net_device) down to < 2K bytes
@ 2015-04-09 23:43 Thomas Graf
  2015-04-09 23:43 ` [PATCH 1/3 net-next] e1000: Allocate pm_qos_req as needed Thomas Graf
                   ` (3 more replies)
  0 siblings, 4 replies; 14+ messages in thread
From: Thomas Graf @ 2015-04-09 23:43 UTC (permalink / raw)
  To: davem; +Cc: jeffrey.t.kirsher, intel-wired-lan, netdev, eric.dumazet

The size of struct net_device crossed the 2K boundary a while ago which
is a waste in combination with many net namespaces. This series brings
the size of struct net_device down to well below 2K in total size with
a typical configuration. Some reserves a several holes leave room for
further expansion.

Thomas Graf (3):
  e1000: Allocate pm_qos_req as needed
  net_device: Reorder members to fill holes
  net_device: Add minimal padding to allow for net_device pointer
    alignment

 drivers/net/ethernet/intel/e1000e/netdev.c | 15 ++++++++---
 include/linux/kernel.h                     |  2 ++
 include/linux/netdevice.h                  | 41 ++++++++++++++----------------
 net/core/dev.c                             |  3 +--
 4 files changed, 33 insertions(+), 28 deletions(-)

-- 
@Jeff: I only boot tested the e1000 in a virtual machine.

^ permalink raw reply	[flat|nested] 14+ messages in thread

* [PATCH 1/3 net-next] e1000: Allocate pm_qos_req as needed
  2015-04-09 23:43 [PATCH 0/3 net-next] Bring sizeof(net_device) down to < 2K bytes Thomas Graf
@ 2015-04-09 23:43 ` Thomas Graf
  2015-04-10  0:08   ` Jeff Kirsher
  2015-04-09 23:43 ` [PATCH 2/3 net-next] net_device: Reorder members to fill holes Thomas Graf
                   ` (2 subsequent siblings)
  3 siblings, 1 reply; 14+ messages in thread
From: Thomas Graf @ 2015-04-09 23:43 UTC (permalink / raw)
  To: davem; +Cc: jeffrey.t.kirsher, intel-wired-lan, netdev, eric.dumazet

e1000 is the only driver requiring pm_qos_req, instead of causing
every device to waste up to 240 bytes. Allocate it for the specific
driver.

Signed-off-by: Thomas Graf <tgraf@suug.ch>
---
 drivers/net/ethernet/intel/e1000e/netdev.c | 15 +++++++++++----
 include/linux/netdevice.h                  |  2 +-
 2 files changed, 12 insertions(+), 5 deletions(-)

diff --git a/drivers/net/ethernet/intel/e1000e/netdev.c b/drivers/net/ethernet/intel/e1000e/netdev.c
index 74ec185..ae9f6ed 100644
--- a/drivers/net/ethernet/intel/e1000e/netdev.c
+++ b/drivers/net/ethernet/intel/e1000e/netdev.c
@@ -3297,9 +3297,9 @@ static void e1000_configure_rx(struct e1000_adapter *adapter)
 			ew32(RXDCTL(0), rxdctl | 0x3);
 		}
 
-		pm_qos_update_request(&adapter->netdev->pm_qos_req, lat);
+		pm_qos_update_request(adapter->netdev->pm_qos_req, lat);
 	} else {
-		pm_qos_update_request(&adapter->netdev->pm_qos_req,
+		pm_qos_update_request(adapter->netdev->pm_qos_req,
 				      PM_QOS_DEFAULT_VALUE);
 	}
 
@@ -4402,8 +4402,12 @@ static int e1000_open(struct net_device *netdev)
 	if ((adapter->hw.mng_cookie.status & E1000_MNG_DHCP_COOKIE_STATUS_VLAN))
 		e1000_update_mng_vlan(adapter);
 
+	netdev->pm_qos_req = kzalloc(sizeof(struct pm_qos_request), GFP_KERNEL);
+	if (!netdev->pm_qos_req)
+		goto err_pm_qos_req;
+
 	/* DMA latency requirement to workaround jumbo issue */
-	pm_qos_add_request(&adapter->netdev->pm_qos_req, PM_QOS_CPU_DMA_LATENCY,
+	pm_qos_add_request(adapter->netdev->pm_qos_req, PM_QOS_CPU_DMA_LATENCY,
 			   PM_QOS_DEFAULT_VALUE);
 
 	/* before we allocate an interrupt, we must be ready to handle it.
@@ -4451,6 +4455,8 @@ static int e1000_open(struct net_device *netdev)
 	return 0;
 
 err_req_irq:
+	kfree(netdev->pm_qos_req);
+err_pm_qos_req:
 	e1000e_release_hw_control(adapter);
 	e1000_power_down_phy(adapter);
 	e1000e_free_rx_resources(adapter->rx_ring);
@@ -4514,7 +4520,8 @@ static int e1000_close(struct net_device *netdev)
 	    !test_bit(__E1000_TESTING, &adapter->state))
 		e1000e_release_hw_control(adapter);
 
-	pm_qos_remove_request(&adapter->netdev->pm_qos_req);
+	pm_qos_remove_request(adapter->netdev->pm_qos_req);
+	kfree(adapter->netdev->pm_qos_req);
 
 	pm_runtime_put_sync(&pdev->dev);
 
diff --git a/include/linux/netdevice.h b/include/linux/netdevice.h
index bf6d9df..4653422 100644
--- a/include/linux/netdevice.h
+++ b/include/linux/netdevice.h
@@ -1743,7 +1743,7 @@ struct net_device {
 #endif
 	struct phy_device *phydev;
 	struct lock_class_key *qdisc_tx_busylock;
-	struct pm_qos_request	pm_qos_req;
+	struct pm_qos_request *pm_qos_req;
 };
 #define to_net_dev(d) container_of(d, struct net_device, dev)
 
-- 
1.9.3

^ permalink raw reply related	[flat|nested] 14+ messages in thread

* [PATCH 2/3 net-next] net_device: Reorder members to fill holes
  2015-04-09 23:43 [PATCH 0/3 net-next] Bring sizeof(net_device) down to < 2K bytes Thomas Graf
  2015-04-09 23:43 ` [PATCH 1/3 net-next] e1000: Allocate pm_qos_req as needed Thomas Graf
@ 2015-04-09 23:43 ` Thomas Graf
  2015-04-09 23:43 ` [PATCH 3/3 net-next] net_device: Add minimal padding to allow for net_device pointer alignment Thomas Graf
  2015-04-10  0:01 ` [PATCH 0/3 net-next] Bring sizeof(net_device) down to < 2K bytes Alexei Starovoitov
  3 siblings, 0 replies; 14+ messages in thread
From: Thomas Graf @ 2015-04-09 23:43 UTC (permalink / raw)
  To: davem; +Cc: jeffrey.t.kirsher, intel-wired-lan, netdev, eric.dumazet

Some trivial reorders while preserving the RX/TX cache lines
split to fill a couple of holes.

Signed-off-by: Thomas Graf <tgraf@suug.ch>
---
 include/linux/netdevice.h | 39 ++++++++++++++++++---------------------
 1 file changed, 18 insertions(+), 21 deletions(-)

diff --git a/include/linux/netdevice.h b/include/linux/netdevice.h
index 4653422..af8e9b0 100644
--- a/include/linux/netdevice.h
+++ b/include/linux/netdevice.h
@@ -1305,6 +1305,8 @@ enum netdev_priv_flags {
  *	@base_addr:	Device I/O address
  *	@irq:		Device IRQ number
  *
+ *	@carrier_changes:	Stats to monitor carrier on<->off transitions
+ *
  *	@state:		Generic network queuing layer state, see netdev_state_t
  *	@dev_list:	The global list of network devices
  *	@napi_list:	List entry, that is used for polling napi devices
@@ -1338,8 +1340,6 @@ enum netdev_priv_flags {
  *	@tx_dropped:	Dropped packets by core network,
  *			do not use this in drivers
  *
- *	@carrier_changes:	Stats to monitor carrier on<->off transitions
- *
  *	@wireless_handlers:	List of functions to handle Wireless Extensions,
  *				instead of ioctl,
  *				see <net/iw_handler.h> for details.
@@ -1382,14 +1382,14 @@ 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:			unicast mac addresses
- *	@mc:			multicast mac addresses
- *	@dev_addrs:		list of device hw addresses
- *	@queues_kset:		Group of all Kobjects in the Tx and RX queues
  *	@uc_promisc:		Counter, that indicates, that 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
+ *	@queues_kset:		Group of all Kobjects in the Tx and RX queues
  *	@promiscuity:		Number of times, the NIC is told to work in
  *				Promiscuous mode, if it becomes 0 the NIC will
  *				exit from working in Promiscuous mode
@@ -1419,6 +1419,11 @@ enum netdev_priv_flags {
  *	@ingress_queue:		XXX: need comments on this one
  *	@broadcast:		hw bcast address
  *
+ *	@rx_cpu_rmap:	CPU reverse-mapping for RX completion interrupts,
+ *			indexed by RX queue number. Assigned by driver.
+ *			This must only be set if the ndo_rx_flow_steer
+ *			operation is defined
+ *
  *	@_tx:			Array of TX queues
  *	@num_tx_queues:		Number of TX queues allocated at alloc_netdev_mq() time
  *	@real_num_tx_queues: 	Number of TX queues currently active in device
@@ -1428,11 +1433,6 @@ enum netdev_priv_flags {
  *
  *	@xps_maps:	XXX: need comments on this one
  *
- *	@rx_cpu_rmap:	CPU reverse-mapping for RX completion interrupts,
- *			indexed by RX queue number. Assigned by driver.
- *			This must only be set if the ndo_rx_flow_steer
- *			operation is defined
- *
  *	@trans_start:		Time (in jiffies) of last Tx
  *	@watchdog_timeo:	Represents the timeout that is used by
  *				the watchdog ( see dev_watchdog() )
@@ -1507,6 +1507,8 @@ struct net_device {
 	unsigned long		base_addr;
 	int			irq;
 
+	atomic_t		carrier_changes;
+
 	/*
 	 *	Some hardware also needs these fields (state,dev_list,
 	 *	napi_list,unreg_list,close_list) but they are not
@@ -1547,8 +1549,6 @@ struct net_device {
 	atomic_long_t		rx_dropped;
 	atomic_long_t		tx_dropped;
 
-	atomic_t		carrier_changes;
-
 #ifdef CONFIG_WIRELESS_EXT
 	const struct iw_handler_def *	wireless_handlers;
 	struct iw_public_data *	wireless_data;
@@ -1588,6 +1588,8 @@ struct net_device {
 	unsigned short          dev_id;
 	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;
@@ -1595,10 +1597,6 @@ struct net_device {
 #ifdef CONFIG_SYSFS
 	struct kset		*queues_kset;
 #endif
-
-	unsigned char		name_assign_type;
-
-	bool			uc_promisc;
 	unsigned int		promiscuity;
 	unsigned int		allmulti;
 
@@ -1645,7 +1643,9 @@ struct net_device {
 
 	struct netdev_queue __rcu *ingress_queue;
 	unsigned char		broadcast[MAX_ADDR_LEN];
-
+#ifdef CONFIG_RFS_ACCEL
+	struct cpu_rmap		*rx_cpu_rmap;
+#endif
 
 /*
  * Cache lines mostly used on transmit path
@@ -1660,9 +1660,6 @@ struct net_device {
 #ifdef CONFIG_XPS
 	struct xps_dev_maps __rcu *xps_maps;
 #endif
-#ifdef CONFIG_RFS_ACCEL
-	struct cpu_rmap		*rx_cpu_rmap;
-#endif
 
 	/* These may be needed for future network-power-down code. */
 
-- 
1.9.3

^ permalink raw reply related	[flat|nested] 14+ messages in thread

* [PATCH 3/3 net-next] net_device: Add minimal padding to allow for net_device pointer alignment
  2015-04-09 23:43 [PATCH 0/3 net-next] Bring sizeof(net_device) down to < 2K bytes Thomas Graf
  2015-04-09 23:43 ` [PATCH 1/3 net-next] e1000: Allocate pm_qos_req as needed Thomas Graf
  2015-04-09 23:43 ` [PATCH 2/3 net-next] net_device: Reorder members to fill holes Thomas Graf
@ 2015-04-09 23:43 ` Thomas Graf
  2015-04-10  0:34   ` Eric Dumazet
  2015-04-10  0:01 ` [PATCH 0/3 net-next] Bring sizeof(net_device) down to < 2K bytes Alexei Starovoitov
  3 siblings, 1 reply; 14+ messages in thread
From: Thomas Graf @ 2015-04-09 23:43 UTC (permalink / raw)
  To: davem; +Cc: jeffrey.t.kirsher, intel-wired-lan, netdev, eric.dumazet

Makes use of the __alignof__ GCC builtin to determine the minimal
amount of padding required to align the pointer afterwards.

Due to use of ____cacheline_aligned_in_smp inside net_device,
struct net_device is actually always a multiple of SMP_CACHE_BYTES
so typically no padding is needed but this logic is kept in place
in case that is no longer the case.

Signed-off-by: Thomas Graf <tgraf@suug.ch>
---
 include/linux/kernel.h | 2 ++
 net/core/dev.c         | 3 +--
 2 files changed, 3 insertions(+), 2 deletions(-)

diff --git a/include/linux/kernel.h b/include/linux/kernel.h
index d6d630d..4ec018f 100644
--- a/include/linux/kernel.h
+++ b/include/linux/kernel.h
@@ -50,6 +50,8 @@
 #define __ALIGN_MASK(x, mask)	__ALIGN_KERNEL_MASK((x), (mask))
 #define PTR_ALIGN(p, a)		((typeof(p))ALIGN((unsigned long)(p), (a)))
 #define IS_ALIGNED(x, a)		(((x) & ((typeof(x))(a) - 1)) == 0)
+#define PTR_ALIGN_PAD(type, a) \
+	({ (__alignof__(type) < (a)) ? (a) - __alignof__(type) : 0; })
 
 #define ARRAY_SIZE(arr) (sizeof(arr) / sizeof((arr)[0]) + __must_be_array(arr))
 
diff --git a/net/core/dev.c b/net/core/dev.c
index b2775f0..2b43aba 100644
--- a/net/core/dev.c
+++ b/net/core/dev.c
@@ -6794,8 +6794,7 @@ struct net_device *alloc_netdev_mqs(int sizeof_priv, const char *name,
 		alloc_size = ALIGN(alloc_size, NETDEV_ALIGN);
 		alloc_size += sizeof_priv;
 	}
-	/* ensure 32-byte alignment of whole construct */
-	alloc_size += NETDEV_ALIGN - 1;
+	alloc_size += PTR_ALIGN_PAD(struct net_device, NETDEV_ALIGN);
 
 	p = kzalloc(alloc_size, GFP_KERNEL | __GFP_NOWARN | __GFP_REPEAT);
 	if (!p)
-- 
1.9.3

^ permalink raw reply related	[flat|nested] 14+ messages in thread

* Re: [PATCH 0/3 net-next] Bring sizeof(net_device) down to < 2K bytes
  2015-04-09 23:43 [PATCH 0/3 net-next] Bring sizeof(net_device) down to < 2K bytes Thomas Graf
                   ` (2 preceding siblings ...)
  2015-04-09 23:43 ` [PATCH 3/3 net-next] net_device: Add minimal padding to allow for net_device pointer alignment Thomas Graf
@ 2015-04-10  0:01 ` Alexei Starovoitov
  2015-04-10  8:41   ` Thomas Graf
  3 siblings, 1 reply; 14+ messages in thread
From: Alexei Starovoitov @ 2015-04-10  0:01 UTC (permalink / raw)
  To: Thomas Graf
  Cc: davem, jeffrey.t.kirsher, intel-wired-lan, netdev, eric.dumazet

On Fri, Apr 10, 2015 at 01:43:25AM +0200, Thomas Graf wrote:
> The size of struct net_device crossed the 2K boundary a while ago which
> is a waste in combination with many net namespaces. This series brings
> the size of struct net_device down to well below 2K in total size with
> a typical configuration. Some reserves a several holes leave room for
> further expansion.

Great stuff!
Can you share what was the sizeof(struct net_device) with typical
x86 config before and after?

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH 1/3 net-next] e1000: Allocate pm_qos_req as needed
  2015-04-09 23:43 ` [PATCH 1/3 net-next] e1000: Allocate pm_qos_req as needed Thomas Graf
@ 2015-04-10  0:08   ` Jeff Kirsher
  2015-04-10  4:35     ` [Intel-wired-lan] " Jeff Kirsher
  0 siblings, 1 reply; 14+ messages in thread
From: Jeff Kirsher @ 2015-04-10  0:08 UTC (permalink / raw)
  To: Thomas Graf; +Cc: davem, intel-wired-lan, netdev, eric.dumazet

[-- Attachment #1: Type: text/plain, Size: 505 bytes --]

On Fri, 2015-04-10 at 01:43 +0200, Thomas Graf wrote:
> e1000 is the only driver requiring pm_qos_req, instead of causing
> every device to waste up to 240 bytes. Allocate it for the specific
> driver.
> 
> Signed-off-by: Thomas Graf <tgraf@suug.ch>
> ---
>  drivers/net/ethernet/intel/e1000e/netdev.c | 15 +++++++++++----
>  include/linux/netdevice.h                  |  2 +-
>  2 files changed, 12 insertions(+), 5 deletions(-)

Small nitpick, it is e1000e not e1000 that you are modifying.

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH 3/3 net-next] net_device: Add minimal padding to allow for net_device pointer alignment
  2015-04-09 23:43 ` [PATCH 3/3 net-next] net_device: Add minimal padding to allow for net_device pointer alignment Thomas Graf
@ 2015-04-10  0:34   ` Eric Dumazet
  2015-04-10  8:48     ` Thomas Graf
  0 siblings, 1 reply; 14+ messages in thread
From: Eric Dumazet @ 2015-04-10  0:34 UTC (permalink / raw)
  To: Thomas Graf; +Cc: davem, jeffrey.t.kirsher, intel-wired-lan, netdev

On Fri, 2015-04-10 at 01:43 +0200, Thomas Graf wrote:
> Makes use of the __alignof__ GCC builtin to determine the minimal
> amount of padding required to align the pointer afterwards.
> 
> Due to use of ____cacheline_aligned_in_smp inside net_device,
> struct net_device is actually always a multiple of SMP_CACHE_BYTES
> so typically no padding is needed but this logic is kept in place
> in case that is no longer the case.
> 
> Signed-off-by: Thomas Graf <tgraf@suug.ch>
> ---
>  include/linux/kernel.h | 2 ++
>  net/core/dev.c         | 3 +--
>  2 files changed, 3 insertions(+), 2 deletions(-)
> 
> diff --git a/include/linux/kernel.h b/include/linux/kernel.h
> index d6d630d..4ec018f 100644
> --- a/include/linux/kernel.h
> +++ b/include/linux/kernel.h
> @@ -50,6 +50,8 @@
>  #define __ALIGN_MASK(x, mask)	__ALIGN_KERNEL_MASK((x), (mask))
>  #define PTR_ALIGN(p, a)		((typeof(p))ALIGN((unsigned long)(p), (a)))
>  #define IS_ALIGNED(x, a)		(((x) & ((typeof(x))(a) - 1)) == 0)
> +#define PTR_ALIGN_PAD(type, a) \
> +	({ (__alignof__(type) < (a)) ? (a) - __alignof__(type) : 0; })
>  
>  #define ARRAY_SIZE(arr) (sizeof(arr) / sizeof((arr)[0]) + __must_be_array(arr))
>  
> diff --git a/net/core/dev.c b/net/core/dev.c
> index b2775f0..2b43aba 100644
> --- a/net/core/dev.c
> +++ b/net/core/dev.c
> @@ -6794,8 +6794,7 @@ struct net_device *alloc_netdev_mqs(int sizeof_priv, const char *name,
>  		alloc_size = ALIGN(alloc_size, NETDEV_ALIGN);
>  		alloc_size += sizeof_priv;
>  	}
> -	/* ensure 32-byte alignment of whole construct */
> -	alloc_size += NETDEV_ALIGN - 1;
> +	alloc_size += PTR_ALIGN_PAD(struct net_device, NETDEV_ALIGN);
>  
>  	p = kzalloc(alloc_size, GFP_KERNEL | __GFP_NOWARN | __GFP_REPEAT);
>  	if (!p)


I believe code intent was to get an alignment even if kmalloc() returns
say 0xxxxx0008.

AFAIK, SLAB in debug mode was able to do this.

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [Intel-wired-lan] [PATCH 1/3 net-next] e1000: Allocate pm_qos_req as needed
  2015-04-10  0:08   ` Jeff Kirsher
@ 2015-04-10  4:35     ` Jeff Kirsher
  2015-04-10  8:01       ` Daniel Borkmann
  0 siblings, 1 reply; 14+ messages in thread
From: Jeff Kirsher @ 2015-04-10  4:35 UTC (permalink / raw)
  To: Thomas Graf; +Cc: netdev, intel-wired-lan, davem, eric.dumazet

[-- Attachment #1: Type: text/plain, Size: 746 bytes --]

On Thu, 2015-04-09 at 17:08 -0700, Jeff Kirsher wrote:
> On Fri, 2015-04-10 at 01:43 +0200, Thomas Graf wrote:
> > e1000 is the only driver requiring pm_qos_req, instead of causing
> > every device to waste up to 240 bytes. Allocate it for the specific
> > driver.
> > 
> > Signed-off-by: Thomas Graf <tgraf@suug.ch>
> > ---
> >  drivers/net/ethernet/intel/e1000e/netdev.c | 15 +++++++++++----
> >  include/linux/netdevice.h                  |  2 +-
> >  2 files changed, 12 insertions(+), 5 deletions(-)
> 
> Small nitpick, it is e1000e not e1000 that you are modifying.

So other than the patch title and description referencing e1000 instead
of e1000e, patch looks fine.

Acked-by: Jeff Kirsher <jeffrey.t.kirsher@intel.com>

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [Intel-wired-lan] [PATCH 1/3 net-next] e1000: Allocate pm_qos_req as needed
  2015-04-10  4:35     ` [Intel-wired-lan] " Jeff Kirsher
@ 2015-04-10  8:01       ` Daniel Borkmann
  2015-04-10  8:48         ` Thomas Graf
  0 siblings, 1 reply; 14+ messages in thread
From: Daniel Borkmann @ 2015-04-10  8:01 UTC (permalink / raw)
  To: Jeff Kirsher, Thomas Graf; +Cc: netdev, intel-wired-lan, davem, eric.dumazet

On 04/10/2015 06:35 AM, Jeff Kirsher wrote:
> On Thu, 2015-04-09 at 17:08 -0700, Jeff Kirsher wrote:
>> On Fri, 2015-04-10 at 01:43 +0200, Thomas Graf wrote:
>>> e1000 is the only driver requiring pm_qos_req, instead of causing
>>> every device to waste up to 240 bytes. Allocate it for the specific
>>> driver.
>>>
>>> Signed-off-by: Thomas Graf <tgraf@suug.ch>
>>> ---
>>>   drivers/net/ethernet/intel/e1000e/netdev.c | 15 +++++++++++----
>>>   include/linux/netdevice.h                  |  2 +-
>>>   2 files changed, 12 insertions(+), 5 deletions(-)
>>
>> Small nitpick, it is e1000e not e1000 that you are modifying.
>
> So other than the patch title and description referencing e1000 instead
> of e1000e, patch looks fine.
>
> Acked-by: Jeff Kirsher <jeffrey.t.kirsher@intel.com>

Thanks for working towards reducing struct net_device, that's awesome!

Wrt this patch, I'm wondering if that couldn't be pushed down into
struct e1000_adapter entirely?

Looks like e1000e is the only user of this, would save every other
net_device 8 more bytes:

$ git grep -n "\->pm_qos_req" drivers/net/
drivers/net/ethernet/intel/e1000e/netdev.c:3300:                pm_qos_update_request(&adapter->netdev->pm_qos_req, lat);
drivers/net/ethernet/intel/e1000e/netdev.c:3302:                pm_qos_update_request(&adapter->netdev->pm_qos_req,
drivers/net/ethernet/intel/e1000e/netdev.c:4406:        pm_qos_add_request(&adapter->netdev->pm_qos_req, PM_QOS_CPU_DMA_LATENCY,
drivers/net/ethernet/intel/e1000e/netdev.c:4517:        pm_qos_remove_request(&adapter->netdev->pm_qos_req);

Thanks,
Daniel

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH 0/3 net-next] Bring sizeof(net_device) down to < 2K bytes
  2015-04-10  0:01 ` [PATCH 0/3 net-next] Bring sizeof(net_device) down to < 2K bytes Alexei Starovoitov
@ 2015-04-10  8:41   ` Thomas Graf
  0 siblings, 0 replies; 14+ messages in thread
From: Thomas Graf @ 2015-04-10  8:41 UTC (permalink / raw)
  To: Alexei Starovoitov
  Cc: davem, jeffrey.t.kirsher, intel-wired-lan, netdev, eric.dumazet

On 04/09/15 at 05:01pm, Alexei Starovoitov wrote:
> On Fri, Apr 10, 2015 at 01:43:25AM +0200, Thomas Graf wrote:
> > The size of struct net_device crossed the 2K boundary a while ago which
> > is a waste in combination with many net namespaces. This series brings
> > the size of struct net_device down to well below 2K in total size with
> > a typical configuration. Some reserves a several holes leave room for
> > further expansion.
> 
> Great stuff!
> Can you share what was the sizeof(struct net_device) with typical
> x86 config before and after?

Before:

/* BRAIN FART ALERT! 2176 != 2091 + 41(holes), diff = 44 */

After:

/* size: 1984, cachelines: 31, members: 121 */
/* sum members: 1947, holes: 6, sum holes: 33 */
/* padding: 8 */
/* paddings: 2, sum paddings: 11 */

/* BRAIN FART ALERT! 1984 != 1947 + 33(holes), diff = 4 */

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH 3/3 net-next] net_device: Add minimal padding to allow for net_device pointer alignment
  2015-04-10  0:34   ` Eric Dumazet
@ 2015-04-10  8:48     ` Thomas Graf
  2015-04-10  9:03       ` Eric Dumazet
  0 siblings, 1 reply; 14+ messages in thread
From: Thomas Graf @ 2015-04-10  8:48 UTC (permalink / raw)
  To: Eric Dumazet; +Cc: davem, jeffrey.t.kirsher, intel-wired-lan, netdev

On 04/09/15 at 05:34pm, Eric Dumazet wrote:
> On Fri, 2015-04-10 at 01:43 +0200, Thomas Graf wrote:
> > diff --git a/net/core/dev.c b/net/core/dev.c
> > index b2775f0..2b43aba 100644
> > --- a/net/core/dev.c
> > +++ b/net/core/dev.c
> > @@ -6794,8 +6794,7 @@ struct net_device *alloc_netdev_mqs(int sizeof_priv, const char *name,
> >  		alloc_size = ALIGN(alloc_size, NETDEV_ALIGN);
> >  		alloc_size += sizeof_priv;
> >  	}
> > -	/* ensure 32-byte alignment of whole construct */
> > -	alloc_size += NETDEV_ALIGN - 1;
> > +	alloc_size += PTR_ALIGN_PAD(struct net_device, NETDEV_ALIGN);
> >  
> >  	p = kzalloc(alloc_size, GFP_KERNEL | __GFP_NOWARN | __GFP_REPEAT);
> >  	if (!p)
> 
> 
> I believe code intent was to get an alignment even if kmalloc() returns
> say 0xxxxx0008.
> 
> AFAIK, SLAB in debug mode was able to do this.

I assumed kmalloc would guarantee alignment of the return pointer as
indicated by __alignof__ but I now realize that this can't possibly be the
case. kmalloc only knows about the size. We'll have to drop patch 3.
Fortunately it's still below 2K.

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [Intel-wired-lan] [PATCH 1/3 net-next] e1000: Allocate pm_qos_req as needed
  2015-04-10  8:01       ` Daniel Borkmann
@ 2015-04-10  8:48         ` Thomas Graf
  2015-04-10  8:58           ` Jeff Kirsher
  0 siblings, 1 reply; 14+ messages in thread
From: Thomas Graf @ 2015-04-10  8:48 UTC (permalink / raw)
  To: Daniel Borkmann
  Cc: Jeff Kirsher, netdev, intel-wired-lan, davem, eric.dumazet

On 04/10/15 at 10:01am, Daniel Borkmann wrote:
> On 04/10/2015 06:35 AM, Jeff Kirsher wrote:
> >On Thu, 2015-04-09 at 17:08 -0700, Jeff Kirsher wrote:
> >>On Fri, 2015-04-10 at 01:43 +0200, Thomas Graf wrote:
> >>>e1000 is the only driver requiring pm_qos_req, instead of causing
> >>>every device to waste up to 240 bytes. Allocate it for the specific
> >>>driver.
> >>>
> >>>Signed-off-by: Thomas Graf <tgraf@suug.ch>
> >>>---
> >>>  drivers/net/ethernet/intel/e1000e/netdev.c | 15 +++++++++++----
> >>>  include/linux/netdevice.h                  |  2 +-
> >>>  2 files changed, 12 insertions(+), 5 deletions(-)
> >>
> >>Small nitpick, it is e1000e not e1000 that you are modifying.
> >
> >So other than the patch title and description referencing e1000 instead
> >of e1000e, patch looks fine.
> >
> >Acked-by: Jeff Kirsher <jeffrey.t.kirsher@intel.com>
> 
> Thanks for working towards reducing struct net_device, that's awesome!
> 
> Wrt this patch, I'm wondering if that couldn't be pushed down into
> struct e1000_adapter entirely?

Sure, since I need to respin this anyway. Jeff are you OK with that?

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [Intel-wired-lan] [PATCH 1/3 net-next] e1000: Allocate pm_qos_req as needed
  2015-04-10  8:48         ` Thomas Graf
@ 2015-04-10  8:58           ` Jeff Kirsher
  0 siblings, 0 replies; 14+ messages in thread
From: Jeff Kirsher @ 2015-04-10  8:58 UTC (permalink / raw)
  To: Thomas Graf; +Cc: Daniel Borkmann, netdev, intel-wired-lan, davem, eric.dumazet

[-- Attachment #1: Type: text/plain, Size: 1470 bytes --]

On Fri, 2015-04-10 at 09:48 +0100, Thomas Graf wrote:
> On 04/10/15 at 10:01am, Daniel Borkmann wrote:
> > On 04/10/2015 06:35 AM, Jeff Kirsher wrote:
> > >On Thu, 2015-04-09 at 17:08 -0700, Jeff Kirsher wrote:
> > >>On Fri, 2015-04-10 at 01:43 +0200, Thomas Graf wrote:
> > >>>e1000 is the only driver requiring pm_qos_req, instead of causing
> > >>>every device to waste up to 240 bytes. Allocate it for the specific
> > >>>driver.
> > >>>
> > >>>Signed-off-by: Thomas Graf <tgraf@suug.ch>
> > >>>---
> > >>>  drivers/net/ethernet/intel/e1000e/netdev.c | 15 +++++++++++----
> > >>>  include/linux/netdevice.h                  |  2 +-
> > >>>  2 files changed, 12 insertions(+), 5 deletions(-)
> > >>
> > >>Small nitpick, it is e1000e not e1000 that you are modifying.
> > >
> > >So other than the patch title and description referencing e1000 instead
> > >of e1000e, patch looks fine.
> > >
> > >Acked-by: Jeff Kirsher <jeffrey.t.kirsher@intel.com>
> > 
> > Thanks for working towards reducing struct net_device, that's awesome!
> > 
> > Wrt this patch, I'm wondering if that couldn't be pushed down into
> > struct e1000_adapter entirely?
> 
> Sure, since I need to respin this anyway. Jeff are you OK with that?

I am trying to think why we did not do it earlier, but I am not coming
up with anything at the moment.  So, go ahead.

In the meantime, I will talk with Bruce Allan tomorrow to see if he
remembers why we did not do it.

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH 3/3 net-next] net_device: Add minimal padding to allow for net_device pointer alignment
  2015-04-10  8:48     ` Thomas Graf
@ 2015-04-10  9:03       ` Eric Dumazet
  0 siblings, 0 replies; 14+ messages in thread
From: Eric Dumazet @ 2015-04-10  9:03 UTC (permalink / raw)
  To: Thomas Graf; +Cc: davem, jeffrey.t.kirsher, intel-wired-lan, netdev

On Fri, 2015-04-10 at 09:48 +0100, Thomas Graf wrote:

> 
> I assumed kmalloc would guarantee alignment of the return pointer as
> indicated by __alignof__ but I now realize that this can't possibly be the
> case. kmalloc only knows about the size. We'll have to drop patch 3.
> Fortunately it's still below 2K.


If you really care for this, take a look at qdisc_alloc()
and commit d276055c4e90a7278cd5167ba9755c9b214bcff7
("net_sched: reduce fifo qdisc size")

(Note that some setups require ~3 network devices but 3000+ qdisc)

I guess it might be nice to define a common helper and reuse it.

^ permalink raw reply	[flat|nested] 14+ messages in thread

end of thread, other threads:[~2015-04-10  9:03 UTC | newest]

Thread overview: 14+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2015-04-09 23:43 [PATCH 0/3 net-next] Bring sizeof(net_device) down to < 2K bytes Thomas Graf
2015-04-09 23:43 ` [PATCH 1/3 net-next] e1000: Allocate pm_qos_req as needed Thomas Graf
2015-04-10  0:08   ` Jeff Kirsher
2015-04-10  4:35     ` [Intel-wired-lan] " Jeff Kirsher
2015-04-10  8:01       ` Daniel Borkmann
2015-04-10  8:48         ` Thomas Graf
2015-04-10  8:58           ` Jeff Kirsher
2015-04-09 23:43 ` [PATCH 2/3 net-next] net_device: Reorder members to fill holes Thomas Graf
2015-04-09 23:43 ` [PATCH 3/3 net-next] net_device: Add minimal padding to allow for net_device pointer alignment Thomas Graf
2015-04-10  0:34   ` Eric Dumazet
2015-04-10  8:48     ` Thomas Graf
2015-04-10  9:03       ` Eric Dumazet
2015-04-10  0:01 ` [PATCH 0/3 net-next] Bring sizeof(net_device) down to < 2K bytes Alexei Starovoitov
2015-04-10  8:41   ` Thomas Graf

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