| Message ID | 20081212125656.GY10372@solarflare.com |
|---|---|
| State | Rejected, archived |
| Delegated to: | David Miller |
| Headers | show |
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
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.
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
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.
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;
Signed-off-by: Ben Hutchings <bhutchings@solarflare.com> --- drivers/net/sfc/efx.c | 5 ++++- 1 files changed, 4 insertions(+), 1 deletions(-)