Patchwork [25/33] sfc: Generate unique names for per-NIC workqueues

login
register
mail settings
Submitter Ben Hutchings
Date Dec. 12, 2008, 12:56 p.m.
Message ID <20081212125656.GY10372@solarflare.com>
Download mbox | patch
Permalink /patch/13727/
State Rejected
Delegated to: David Miller
Headers show

Comments

Ben Hutchings - Dec. 12, 2008, 12:56 p.m.
Signed-off-by: Ben Hutchings <bhutchings@solarflare.com>
---
 drivers/net/sfc/efx.c |    5 ++++-
 1 files changed, 4 insertions(+), 1 deletions(-)
David Miller - Dec. 13, 2008, 6:04 a.m.
From: Ben Hutchings <bhutchings@solarflare.com>
Date: Fri, 12 Dec 2008 12:56:57 +0000

> @@ -1854,6 +1854,7 @@ static int efx_init_struct(struct efx_nic *efx, struct efx_nic_type *type,
>  	struct efx_channel *channel;
>  	struct efx_tx_queue *tx_queue;
>  	struct efx_rx_queue *rx_queue;
> +	char name[16];
...
> @@ -1924,7 +1925,9 @@ static int efx_init_struct(struct efx_nic *efx, struct efx_nic_type *type,
>  	efx->interrupt_mode = max(efx->type->max_interrupt_mode,
>  				  interrupt_mode);
>  
> -	efx->workqueue = create_singlethread_workqueue("sfc_work");
> +	/* Would be good to use the net_dev name, but we're too early */
> +	snprintf(name, sizeof(name), "sfc%s", pci_name(pci_dev));
> +	efx->workqueue = create_singlethread_workqueue(name);
>  	if (!efx->workqueue)

This change is buggy.

create_singlethread_workqueue() is going to reference this
name buffer on the stack for the life of the workqueue, but
once this function returns that reference will no longer
be valid.

You'll need to use kmalloc()'d memory and free it later,
or something like that.

Patch not applied.
--
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
Ben Hutchings - Dec. 13, 2008, 6:23 a.m.
David Miller wrote:
> From: Ben Hutchings <bhutchings@solarflare.com>
> Date: Fri, 12 Dec 2008 12:56:57 +0000
> 
> > @@ -1854,6 +1854,7 @@ static int efx_init_struct(struct efx_nic *efx, struct efx_nic_type *type,
> >  	struct efx_channel *channel;
> >  	struct efx_tx_queue *tx_queue;
> >  	struct efx_rx_queue *rx_queue;
> > +	char name[16];
> ...
> > @@ -1924,7 +1925,9 @@ static int efx_init_struct(struct efx_nic *efx, struct efx_nic_type *type,
> >  	efx->interrupt_mode = max(efx->type->max_interrupt_mode,
> >  				  interrupt_mode);
> >  
> > -	efx->workqueue = create_singlethread_workqueue("sfc_work");
> > +	/* Would be good to use the net_dev name, but we're too early */
> > +	snprintf(name, sizeof(name), "sfc%s", pci_name(pci_dev));
> > +	efx->workqueue = create_singlethread_workqueue(name);
> >  	if (!efx->workqueue)
> 
> This change is buggy.
> 
> create_singlethread_workqueue() is going to reference this
> name buffer on the stack for the life of the workqueue, but
> once this function returns that reference will no longer
> be valid.
 
No, it's copied into the "comm" field of task_struct.

Ben.
David Miller - Dec. 13, 2008, 6:29 a.m.
From: Ben Hutchings <bhutchings@solarflare.com>
Date: Sat, 13 Dec 2008 06:23:03 +0000

> David Miller wrote:
> > create_singlethread_workqueue() is going to reference this
> > name buffer on the stack for the life of the workqueue, but
> > once this function returns that reference will no longer
> > be valid.
>  
> No, it's copied into the "comm" field of task_struct.

Hmmm...

create_singlethread_workqueue(name...
	--> __create_workqueue(name...
	--> __create_workqueue_key(name...

which goes:
	wq->cpu_wq = alloc_percpu(struct cpu_workqueue_struct);
 ...
	wq->name = name;

Which looks like a dynamic memory assumption to me.
--
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
Ben Hutchings - Dec. 13, 2008, 2:01 p.m.
David Miller wrote:
> From: Ben Hutchings <bhutchings@solarflare.com>
> Date: Sat, 13 Dec 2008 06:23:03 +0000
> 
> > David Miller wrote:
> > > create_singlethread_workqueue() is going to reference this
> > > name buffer on the stack for the life of the workqueue, but
> > > once this function returns that reference will no longer
> > > be valid.
> >  
> > No, it's copied into the "comm" field of task_struct.
> 
> Hmmm...
> 
> create_singlethread_workqueue(name...
> 	--> __create_workqueue(name...
> 	--> __create_workqueue_key(name...
> 
> which goes:
> 	wq->cpu_wq = alloc_percpu(struct cpu_workqueue_struct);
>  ...
> 	wq->name = name;
> 
> Which looks like a dynamic memory assumption to me.

If you look carefully you'll see that wq->name is never used after
initialisation of single-threaded workqueues.  But I'd agree it's not
nice to have that dangling pointer around, so I'll respin this.

Ben.

Patch

diff --git a/drivers/net/sfc/efx.c b/drivers/net/sfc/efx.c
index ce1c7d3..c094845 100644
--- a/drivers/net/sfc/efx.c
+++ b/drivers/net/sfc/efx.c
@@ -1854,6 +1854,7 @@  static int efx_init_struct(struct efx_nic *efx, struct efx_nic_type *type,
 	struct efx_channel *channel;
 	struct efx_tx_queue *tx_queue;
 	struct efx_rx_queue *rx_queue;
+	char name[16];
 	int i;
 
 	/* Initialise common structures */
@@ -1924,7 +1925,9 @@  static int efx_init_struct(struct efx_nic *efx, struct efx_nic_type *type,
 	efx->interrupt_mode = max(efx->type->max_interrupt_mode,
 				  interrupt_mode);
 
-	efx->workqueue = create_singlethread_workqueue("sfc_work");
+	/* Would be good to use the net_dev name, but we're too early */
+	snprintf(name, sizeof(name), "sfc%s", pci_name(pci_dev));
+	efx->workqueue = create_singlethread_workqueue(name);
 	if (!efx->workqueue)
 		return -ENOMEM;