| Message ID | 20240816215605.36240-3-bvanassche@acm.org |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Simplify multiple create*_workqueue() invocations | expand |
On 8/17/24 06:55, Bart Van Assche wrote: > Let alloc*_workqueue() format the workqueue names instead of calling > snprintf() explicitly. > > Signed-off-by: Bart Van Assche <bvanassche@acm.org> In patch 1, you have all the changes for removing the use of create_singlethread_workqueue() in a single patch, touching different drivers. But the series has 17 more patches to further cleanup the workqueue API use in various drivers. So why not have the changes in patch 1 split into these different driver patches with a title like "Cleanup and simplify workqueue API use" ? That would make reviewing easier I think and avoid having the patch 2-17 changing again code that was changed in patch 1... > --- > drivers/message/fusion/mptbase.c | 10 +++------- > drivers/message/fusion/mptbase.h | 3 --- > drivers/message/fusion/mptfc.c | 7 ++----- > 3 files changed, 5 insertions(+), 15 deletions(-) > > diff --git a/drivers/message/fusion/mptbase.c b/drivers/message/fusion/mptbase.c > index 4bf669c55649..738bc4e60a18 100644 > --- a/drivers/message/fusion/mptbase.c > +++ b/drivers/message/fusion/mptbase.c > @@ -1856,10 +1856,8 @@ mpt_attach(struct pci_dev *pdev, const struct pci_device_id *id) > /* Initialize workqueue */ > INIT_DELAYED_WORK(&ioc->fault_reset_work, mpt_fault_reset_work); > > - snprintf(ioc->reset_work_q_name, MPT_KOBJ_NAME_LEN, > - "mpt_poll_%d", ioc->id); > - ioc->reset_work_q = alloc_workqueue(ioc->reset_work_q_name, > - WQ_MEM_RECLAIM, 0); > + ioc->reset_work_q = > + alloc_workqueue("mpt_poll_%d", WQ_MEM_RECLAIM, 0, ioc->id); > if (!ioc->reset_work_q) { > printk(MYIOC_s_ERR_FMT "Insufficient memory to add adapter!\n", > ioc->name); > @@ -1986,9 +1984,7 @@ mpt_attach(struct pci_dev *pdev, const struct pci_device_id *id) > > INIT_LIST_HEAD(&ioc->fw_event_list); > spin_lock_init(&ioc->fw_event_lock); > - snprintf(ioc->fw_event_q_name, MPT_KOBJ_NAME_LEN, "mpt/%d", ioc->id); > - ioc->fw_event_q = alloc_workqueue(ioc->fw_event_q_name, > - WQ_MEM_RECLAIM, 0); > + ioc->fw_event_q = alloc_workqueue("mpt/%d", WQ_MEM_RECLAIM, 0, ioc->id); > if (!ioc->fw_event_q) { > printk(MYIOC_s_ERR_FMT "Insufficient memory to add adapter!\n", > ioc->name); > diff --git a/drivers/message/fusion/mptbase.h b/drivers/message/fusion/mptbase.h > index 8031173c3655..b406fd676da0 100644 > --- a/drivers/message/fusion/mptbase.h > +++ b/drivers/message/fusion/mptbase.h > @@ -729,7 +729,6 @@ typedef struct _MPT_ADAPTER > struct list_head fw_event_list; > spinlock_t fw_event_lock; > u8 fw_events_off; /* if '1', then ignore events */ > - char fw_event_q_name[MPT_KOBJ_NAME_LEN]; > > struct mutex sas_discovery_mutex; > u8 sas_discovery_runtime; > @@ -764,7 +763,6 @@ typedef struct _MPT_ADAPTER > u8 fc_link_speed[2]; > spinlock_t fc_rescan_work_lock; > struct work_struct fc_rescan_work; > - char fc_rescan_work_q_name[MPT_KOBJ_NAME_LEN]; > struct workqueue_struct *fc_rescan_work_q; > > /* driver forced bus resets count */ > @@ -778,7 +776,6 @@ typedef struct _MPT_ADAPTER > spinlock_t scsi_lookup_lock; > u64 dma_mask; > u32 broadcast_aen_busy; > - char reset_work_q_name[MPT_KOBJ_NAME_LEN]; > struct workqueue_struct *reset_work_q; > struct delayed_work fault_reset_work; > > diff --git a/drivers/message/fusion/mptfc.c b/drivers/message/fusion/mptfc.c > index a3c17c4fe69c..91242f26defb 100644 > --- a/drivers/message/fusion/mptfc.c > +++ b/drivers/message/fusion/mptfc.c > @@ -1349,11 +1349,8 @@ mptfc_probe(struct pci_dev *pdev, const struct pci_device_id *id) > > /* initialize workqueue */ > > - snprintf(ioc->fc_rescan_work_q_name, sizeof(ioc->fc_rescan_work_q_name), > - "mptfc_wq_%d", sh->host_no); > - ioc->fc_rescan_work_q = > - alloc_ordered_workqueue(ioc->fc_rescan_work_q_name, > - WQ_MEM_RECLAIM); > + ioc->fc_rescan_work_q = alloc_ordered_workqueue( > + "mptfc_wq_%d", WQ_MEM_RECLAIM, sh->host_no); > if (!ioc->fc_rescan_work_q) { > error = -ENOMEM; > goto out_mptfc_host; >
On 8/18/24 4:51 PM, Damien Le Moal wrote: > On 8/17/24 06:55, Bart Van Assche wrote: >> Let alloc*_workqueue() format the workqueue names instead of calling >> snprintf() explicitly. >> >> Signed-off-by: Bart Van Assche <bvanassche@acm.org> > > In patch 1, you have all the changes for removing the use of > create_singlethread_workqueue() in a single patch, touching different drivers. > But the series has 17 more patches to further cleanup the workqueue API use in > various drivers. So why not have the changes in patch 1 split into these > different driver patches with a title like "Cleanup and simplify workqueue API > use" ? That would make reviewing easier I think and avoid having the patch 2-17 > changing again code that was changed in patch 1... Hi Damien, Thanks for having taken a look at this patch series. Would splitting patch 01/18 really help? Splitting that patch would make the description of the split patches longer than the actual code changes. That might annoy other reviewers. Additionally, isn't typical that Coccinelle patches are applied tree-wide instead of one driver at a time? A few examples: * 795f90c6f13c ("sysctl: treewide: constify argument ctl_table_root::permissions(table)"). * e8058a49e67f ("netlink: introduce type-checking attribute iteration"). Thanks, Bart.
On 8/20/24 02:08, Bart Van Assche wrote: > On 8/18/24 4:51 PM, Damien Le Moal wrote: >> On 8/17/24 06:55, Bart Van Assche wrote: >>> Let alloc*_workqueue() format the workqueue names instead of calling >>> snprintf() explicitly. >>> >>> Signed-off-by: Bart Van Assche <bvanassche@acm.org> >> >> In patch 1, you have all the changes for removing the use of >> create_singlethread_workqueue() in a single patch, touching different drivers. >> But the series has 17 more patches to further cleanup the workqueue API use in >> various drivers. So why not have the changes in patch 1 split into these >> different driver patches with a title like "Cleanup and simplify workqueue API >> use" ? That would make reviewing easier I think and avoid having the patch 2-17 >> changing again code that was changed in patch 1... > > Hi Damien, > > Thanks for having taken a look at this patch series. Would splitting > patch 01/18 really help? Splitting that patch would make the description > of the split patches longer than the actual code changes. That might > annoy other reviewers. Additionally, isn't typical that Coccinelle > patches are applied tree-wide instead of one driver at a time? A few > examples: > * 795f90c6f13c ("sysctl: treewide: constify argument > ctl_table_root::permissions(table)"). > * e8058a49e67f ("netlink: introduce type-checking attribute iteration"). I know about script-based patches. But in this case, the script generated patch changes lines of code that following patches change again (not all of them though). So I thought splitting patch 1 may be a good idea as that would as well isolate driver changes in their own patches, which definitely should facilitate reviewing by the driver maintainers. But no strong feelings about all this. If you do not want to do that, fine. > > Thanks, > > Bart. >
diff --git a/drivers/message/fusion/mptbase.c b/drivers/message/fusion/mptbase.c index 4bf669c55649..738bc4e60a18 100644 --- a/drivers/message/fusion/mptbase.c +++ b/drivers/message/fusion/mptbase.c @@ -1856,10 +1856,8 @@ mpt_attach(struct pci_dev *pdev, const struct pci_device_id *id) /* Initialize workqueue */ INIT_DELAYED_WORK(&ioc->fault_reset_work, mpt_fault_reset_work); - snprintf(ioc->reset_work_q_name, MPT_KOBJ_NAME_LEN, - "mpt_poll_%d", ioc->id); - ioc->reset_work_q = alloc_workqueue(ioc->reset_work_q_name, - WQ_MEM_RECLAIM, 0); + ioc->reset_work_q = + alloc_workqueue("mpt_poll_%d", WQ_MEM_RECLAIM, 0, ioc->id); if (!ioc->reset_work_q) { printk(MYIOC_s_ERR_FMT "Insufficient memory to add adapter!\n", ioc->name); @@ -1986,9 +1984,7 @@ mpt_attach(struct pci_dev *pdev, const struct pci_device_id *id) INIT_LIST_HEAD(&ioc->fw_event_list); spin_lock_init(&ioc->fw_event_lock); - snprintf(ioc->fw_event_q_name, MPT_KOBJ_NAME_LEN, "mpt/%d", ioc->id); - ioc->fw_event_q = alloc_workqueue(ioc->fw_event_q_name, - WQ_MEM_RECLAIM, 0); + ioc->fw_event_q = alloc_workqueue("mpt/%d", WQ_MEM_RECLAIM, 0, ioc->id); if (!ioc->fw_event_q) { printk(MYIOC_s_ERR_FMT "Insufficient memory to add adapter!\n", ioc->name); diff --git a/drivers/message/fusion/mptbase.h b/drivers/message/fusion/mptbase.h index 8031173c3655..b406fd676da0 100644 --- a/drivers/message/fusion/mptbase.h +++ b/drivers/message/fusion/mptbase.h @@ -729,7 +729,6 @@ typedef struct _MPT_ADAPTER struct list_head fw_event_list; spinlock_t fw_event_lock; u8 fw_events_off; /* if '1', then ignore events */ - char fw_event_q_name[MPT_KOBJ_NAME_LEN]; struct mutex sas_discovery_mutex; u8 sas_discovery_runtime; @@ -764,7 +763,6 @@ typedef struct _MPT_ADAPTER u8 fc_link_speed[2]; spinlock_t fc_rescan_work_lock; struct work_struct fc_rescan_work; - char fc_rescan_work_q_name[MPT_KOBJ_NAME_LEN]; struct workqueue_struct *fc_rescan_work_q; /* driver forced bus resets count */ @@ -778,7 +776,6 @@ typedef struct _MPT_ADAPTER spinlock_t scsi_lookup_lock; u64 dma_mask; u32 broadcast_aen_busy; - char reset_work_q_name[MPT_KOBJ_NAME_LEN]; struct workqueue_struct *reset_work_q; struct delayed_work fault_reset_work; diff --git a/drivers/message/fusion/mptfc.c b/drivers/message/fusion/mptfc.c index a3c17c4fe69c..91242f26defb 100644 --- a/drivers/message/fusion/mptfc.c +++ b/drivers/message/fusion/mptfc.c @@ -1349,11 +1349,8 @@ mptfc_probe(struct pci_dev *pdev, const struct pci_device_id *id) /* initialize workqueue */ - snprintf(ioc->fc_rescan_work_q_name, sizeof(ioc->fc_rescan_work_q_name), - "mptfc_wq_%d", sh->host_no); - ioc->fc_rescan_work_q = - alloc_ordered_workqueue(ioc->fc_rescan_work_q_name, - WQ_MEM_RECLAIM); + ioc->fc_rescan_work_q = alloc_ordered_workqueue( + "mptfc_wq_%d", WQ_MEM_RECLAIM, sh->host_no); if (!ioc->fc_rescan_work_q) { error = -ENOMEM; goto out_mptfc_host;
Let alloc*_workqueue() format the workqueue names instead of calling snprintf() explicitly. Signed-off-by: Bart Van Assche <bvanassche@acm.org> --- drivers/message/fusion/mptbase.c | 10 +++------- drivers/message/fusion/mptbase.h | 3 --- drivers/message/fusion/mptfc.c | 7 ++----- 3 files changed, 5 insertions(+), 15 deletions(-)