Patchwork e1000e: get rid of wasted space

login
register
mail settings
Submitter stephen hemminger
Date Oct. 21, 2008, 2:11 a.m.
Message ID <20081020191149.1dbf8e3b@extreme>
Download mbox | patch
Permalink /patch/5176/
State Deferred
Delegated to: Jeff Garzik
Headers show

Comments

stephen hemminger - Oct. 21, 2008, 2:11 a.m.
The name in the ring is only used once during setup so it shouldn't
be in the data structure.

Signed-off-by: Stephen Hemminger <shemminger@vyatta.com>

--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Jeff Garzik - Oct. 22, 2008, 11:03 a.m.
Stephen Hemminger wrote:
> The name in the ring is only used once during setup so it shouldn't
> be in the data structure.
> 
> Signed-off-by: Stephen Hemminger <shemminger@vyatta.com>
> 
> --- a/drivers/net/e1000e/e1000.h	2008-10-20 18:13:53.000000000 -0700
> +++ b/drivers/net/e1000e/e1000.h	2008-10-20 18:18:30.000000000 -0700
> @@ -153,7 +153,6 @@ struct e1000_ring {
>  	/* array of buffer information structs */
>  	struct e1000_buffer *buffer_info;
>  
> -	char name[IFNAMSIZ + 5];
>  	u32 ims_val;
>  	u32 itr_val;
>  	u16 itr_register;
> --- a/drivers/net/e1000e/netdev.c	2008-10-20 18:19:22.000000000 -0700
> +++ b/drivers/net/e1000e/netdev.c	2008-10-20 18:21:00.000000000 -0700
> @@ -1473,27 +1473,20 @@ static int e1000_request_msix(struct e10
>  {
>  	struct net_device *netdev = adapter->netdev;
>  	int err = 0, vector = 0;
> +	char irqname[IFNAMSIZ+5];
>  
> -	if (strlen(netdev->name) < (IFNAMSIZ - 5))
> -		sprintf(adapter->rx_ring->name, "%s-rx0", netdev->name);
> -	else
> -		memcpy(adapter->rx_ring->name, netdev->name, IFNAMSIZ);
> +	snprintf(irqname, sizeof(irqname), "%s-rx0", netdev->name);
>  	err = request_irq(adapter->msix_entries[vector].vector,
> -			  &e1000_intr_msix_rx, 0, adapter->rx_ring->name,
> -			  netdev);
> +			  &e1000_intr_msix_rx, 0, irqname, netdev);
>  	if (err)
>  		goto out;
>  	adapter->rx_ring->itr_register = E1000_EITR_82574(vector);
>  	adapter->rx_ring->itr_val = adapter->itr;
>  	vector++;
>  
> -	if (strlen(netdev->name) < (IFNAMSIZ - 5))
> -		sprintf(adapter->tx_ring->name, "%s-tx0", netdev->name);
> -	else
> -		memcpy(adapter->tx_ring->name, netdev->name, IFNAMSIZ);
> +	snprintf(irqname, sizeof(irqname), "%s-tx0", netdev->name);
>  	err = request_irq(adapter->msix_entries[vector].vector,
> -			  &e1000_intr_msix_tx, 0, adapter->tx_ring->name,
> -			  netdev);
> +			  &e1000_intr_msix_tx, 0, irqname, netdev);
>  	if (err)
>  		goto out;
>  	adapter->tx_ring->itr_register = E1000_EITR_82574(vector);

ACK

for what it's worth...  I am following DaveM's lead, and waiting until 
net-next opens to start taking patches for 2.6.29.  He wants us to focus 
on bug fixing for now.

So, please resend once the merge window opens... thanks.

	Jeff




--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Williams, Mitch A - Oct. 22, 2008, 6:01 p.m.
Jeff Garzik wrote:

>
>Stephen Hemminger wrote:
>> The name in the ring is only used once during setup so it shouldn't
>> be in the data structure.
>>
>> Signed-off-by: Stephen Hemminger <shemminger@vyatta.com>
[snip]
>
>ACK
>
>for what it's worth...  I am following DaveM's lead, and waiting until
>net-next opens to start taking patches for 2.6.29.  He wants
>us to focus
>on bug fixing for now.
>
>So, please resend once the merge window opens... thanks.
>

NAK.  Seriously. Don't resend this.  NAK.

That string DOES get used later, like when you cat /proc/interrupts.
So you can't allocate it on the stack.  It's got to be persistent,
just like we have it.  Heck, look at the declaration for
request_irq().  The devname param is declared as const char *.

So NAK.  Same for igb and ixgbe.  Don't do this.

-Mitch
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
stephen hemminger - Oct. 22, 2008, 6:07 p.m.
On Wed, 22 Oct 2008 12:01:16 -0600
"Williams, Mitch A" <mitch.a.williams@intel.com> wrote:

> Jeff Garzik wrote:
> 
> >
> >Stephen Hemminger wrote:
> >> The name in the ring is only used once during setup so it shouldn't
> >> be in the data structure.
> >>
> >> Signed-off-by: Stephen Hemminger <shemminger@vyatta.com>
> [snip]
> >
> >ACK
> >
> >for what it's worth...  I am following DaveM's lead, and waiting until
> >net-next opens to start taking patches for 2.6.29.  He wants
> >us to focus
> >on bug fixing for now.
> >
> >So, please resend once the merge window opens... thanks.
> >
> 
> NAK.  Seriously. Don't resend this.  NAK.
> 
> That string DOES get used later, like when you cat /proc/interrupts.
> So you can't allocate it on the stack.  It's got to be persistent,
> just like we have it.  Heck, look at the declaration for
> request_irq().  The devname param is declared as const char *.
> 
> So NAK.  Same for igb and ixgbe.  Don't do this.
> 
> -Mitch

The comments were right, this needs to stay, although it might be better
to use a bigger field to allow full width of IFNAME+5

--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Patch

--- a/drivers/net/e1000e/e1000.h	2008-10-20 18:13:53.000000000 -0700
+++ b/drivers/net/e1000e/e1000.h	2008-10-20 18:18:30.000000000 -0700
@@ -153,7 +153,6 @@  struct e1000_ring {
 	/* array of buffer information structs */
 	struct e1000_buffer *buffer_info;
 
-	char name[IFNAMSIZ + 5];
 	u32 ims_val;
 	u32 itr_val;
 	u16 itr_register;
--- a/drivers/net/e1000e/netdev.c	2008-10-20 18:19:22.000000000 -0700
+++ b/drivers/net/e1000e/netdev.c	2008-10-20 18:21:00.000000000 -0700
@@ -1473,27 +1473,20 @@  static int e1000_request_msix(struct e10
 {
 	struct net_device *netdev = adapter->netdev;
 	int err = 0, vector = 0;
+	char irqname[IFNAMSIZ+5];
 
-	if (strlen(netdev->name) < (IFNAMSIZ - 5))
-		sprintf(adapter->rx_ring->name, "%s-rx0", netdev->name);
-	else
-		memcpy(adapter->rx_ring->name, netdev->name, IFNAMSIZ);
+	snprintf(irqname, sizeof(irqname), "%s-rx0", netdev->name);
 	err = request_irq(adapter->msix_entries[vector].vector,
-			  &e1000_intr_msix_rx, 0, adapter->rx_ring->name,
-			  netdev);
+			  &e1000_intr_msix_rx, 0, irqname, netdev);
 	if (err)
 		goto out;
 	adapter->rx_ring->itr_register = E1000_EITR_82574(vector);
 	adapter->rx_ring->itr_val = adapter->itr;
 	vector++;
 
-	if (strlen(netdev->name) < (IFNAMSIZ - 5))
-		sprintf(adapter->tx_ring->name, "%s-tx0", netdev->name);
-	else
-		memcpy(adapter->tx_ring->name, netdev->name, IFNAMSIZ);
+	snprintf(irqname, sizeof(irqname), "%s-tx0", netdev->name);
 	err = request_irq(adapter->msix_entries[vector].vector,
-			  &e1000_intr_msix_tx, 0, adapter->tx_ring->name,
-			  netdev);
+			  &e1000_intr_msix_tx, 0, irqname, netdev);
 	if (err)
 		goto out;
 	adapter->tx_ring->itr_register = E1000_EITR_82574(vector);