| Message ID | 8de85fd9-50a1-aad7-86f7-24834be8bbc0@I-love.SAKURA.ne.jp (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Kalle Valo |
| Headers | show |
| Series | ath6kl: avoid flush_scheduled_work() usage | expand |
On 6/10/2022 4:12 AM, Tetsuo Handa wrote: > Use local wq in order to avoid flush_scheduled_work() usage. > > Signed-off-by: Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp> > --- > Please see commit c4f135d643823a86 ("workqueue: Wrap flush_workqueue() > using a macro") for background. > > This is a blind conversion, and is only compile tested. > > drivers/net/wireless/ath/ath6kl/usb.c | 29 +++++++++++++++++++++++---- > 1 file changed, 25 insertions(+), 4 deletions(-) > > diff --git a/drivers/net/wireless/ath/ath6kl/usb.c b/drivers/net/wireless/ath/ath6kl/usb.c [snip] > -module_usb_driver(ath6kl_usb_driver); > +static int __init ath6kl_init(void) > +{ > + int ret; > + > + ath6kl_wq = alloc_workqueue("ath6kl_wq", 0, 0); > + if (!ath6kl_wq) > + return -ENOMEM; this approach means the driver will always allocate a workqueue even if the associated hardware is never present. did you consider instead having the allocation take place within the processing of ath6kl_usb_probe() and the destroy take place within the processing of ath6kl_usb_pm_remove()?
On 6/10/2022 12:05 PM, Jeff Johnson wrote: > On 6/10/2022 4:12 AM, Tetsuo Handa wrote: >> Use local wq in order to avoid flush_scheduled_work() usage. >> >> Signed-off-by: Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp> >> --- >> Please see commit c4f135d643823a86 ("workqueue: Wrap flush_workqueue() >> using a macro") for background. >> >> This is a blind conversion, and is only compile tested. >> >> drivers/net/wireless/ath/ath6kl/usb.c | 29 +++++++++++++++++++++++---- >> 1 file changed, 25 insertions(+), 4 deletions(-) >> >> diff --git a/drivers/net/wireless/ath/ath6kl/usb.c >> b/drivers/net/wireless/ath/ath6kl/usb.c > [snip] >> -module_usb_driver(ath6kl_usb_driver); >> +static int __init ath6kl_init(void) >> +{ >> + int ret; >> + >> + ath6kl_wq = alloc_workqueue("ath6kl_wq", 0, 0); >> + if (!ath6kl_wq) >> + return -ENOMEM; > > this approach means the driver will always allocate a workqueue even if > the associated hardware is never present. > > did you consider instead having the allocation take place within the > processing of ath6kl_usb_probe() and the destroy take place within the > processing of ath6kl_usb_pm_remove()? typo: ath6kl_usb_pm_remove() => ath6kl_usb_remove()
On 2022/06/11 4:10, Jeff Johnson wrote: > On 6/10/2022 12:05 PM, Jeff Johnson wrote: >>> +static int __init ath6kl_init(void) >>> +{ >>> + int ret; >>> + >>> + ath6kl_wq = alloc_workqueue("ath6kl_wq", 0, 0); >>> + if (!ath6kl_wq) >>> + return -ENOMEM; >> >> this approach means the driver will always allocate a workqueue even if the associated hardware is never present. Creating a WQ without WQ_MEM_RECLAIM flag consumes little resource. >> >> did you consider instead having the allocation take place within the processing of ath6kl_usb_probe() and the destroy take place within the processing of ath6kl_usb_pm_remove()? > > typo: ath6kl_usb_pm_remove() => ath6kl_usb_remove() Technically possible to use ath6kl_usb_create()/ath6kl_usb_destroy() if you prefer it. Do you want ath6kl_wq be shared within this module (using a refcount), or be local to each "struct ath6kl_usb" (adding a member and accessing via usb_get_intfdata()) ?
On 2022/06/11 7:51, Tetsuo Handa wrote: > On 2022/06/11 4:10, Jeff Johnson wrote: >> On 6/10/2022 12:05 PM, Jeff Johnson wrote: >>>> +static int __init ath6kl_init(void) >>>> +{ >>>> + int ret; >>>> + >>>> + ath6kl_wq = alloc_workqueue("ath6kl_wq", 0, 0); >>>> + if (!ath6kl_wq) >>>> + return -ENOMEM; >>> >>> this approach means the driver will always allocate a workqueue even if the associated hardware is never present. > > Creating a WQ without WQ_MEM_RECLAIM flag consumes little resource. My understanding is that a loadable kernel module file is modprobe'd when the kernel detected a hardware and some userspace program determined a .ko file to use based on device IDs database). Thus, I think it is very likely that an associated hardware presents if module's __init function is called. If a module is built-in, that user is ready to tolerate memory footprint wasted by non-present hardware. And I think memory wasted by keeping an unused !WQ_MEM_RECLAIM workqueue is much smaller than memory wasted by keeping that module. Thus, I wonder who complains about creating possibly unused !WQ_MEM_RECLAIM workqueue, unless that module is designed for tiny embedded environments. > >>> >>> did you consider instead having the allocation take place within the processing of ath6kl_usb_probe() and the destroy take place within the processing of ath6kl_usb_pm_remove()? >> >> typo: ath6kl_usb_pm_remove() => ath6kl_usb_remove() > > Technically possible to use ath6kl_usb_create()/ath6kl_usb_destroy() if you prefer it. > > Do you want ath6kl_wq be shared within this module (using a refcount), or be local to > each "struct ath6kl_usb" (adding a member and accessing via usb_get_intfdata()) ? > Anyway, here goes per "struct ath6kl_usb" version. From 00c560307d72abffea29409328be8cd69abecc95 Mon Sep 17 00:00:00 2001 From: Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp> Date: Sun, 12 Jun 2022 10:38:03 +0900 Subject: [PATCH v2] ath6kl: avoid flush_scheduled_work() usage Use local wq in order to avoid flush_scheduled_work() usage. Signed-off-by: Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp> --- Changes in v2: Use per "struct ath6kl_usb" workqueue instead of per module workqueue. Please see commit c4f135d643823a86 ("workqueue: Wrap flush_workqueue() using a macro") for background. This is a blind conversion, and is only compile tested. drivers/net/wireless/ath/ath6kl/usb.c | 16 ++++++++++++---- 1 file changed, 12 insertions(+), 4 deletions(-) diff --git a/drivers/net/wireless/ath/ath6kl/usb.c b/drivers/net/wireless/ath/ath6kl/usb.c index 65e683effdcb..5220809841a6 100644 --- a/drivers/net/wireless/ath/ath6kl/usb.c +++ b/drivers/net/wireless/ath/ath6kl/usb.c @@ -71,6 +71,7 @@ struct ath6kl_usb { u8 *diag_cmd_buffer; u8 *diag_resp_buffer; struct ath6kl *ar; + struct workqueue_struct *wq; }; /* usb urb object */ @@ -478,7 +479,7 @@ static void ath6kl_usb_flush_all(struct ath6kl_usb *ar_usb) * Flushing any pending I/O may schedule work this call will block * until all scheduled work runs to completion. */ - flush_scheduled_work(); + flush_workqueue(ar_usb->wq); } static void ath6kl_usb_start_recv_pipes(struct ath6kl_usb *ar_usb) @@ -544,7 +545,7 @@ static void ath6kl_usb_recv_complete(struct urb *urb) /* note: queue implements a lock */ skb_queue_tail(&pipe->io_comp_queue, skb); - schedule_work(&pipe->io_complete_work); + queue_work(pipe->ar_usb->wq, &pipe->io_complete_work); cleanup_recv_urb: ath6kl_usb_cleanup_recv_urb(urb_context); @@ -579,7 +580,7 @@ static void ath6kl_usb_usb_transmit_complete(struct urb *urb) /* note: queue implements a lock */ skb_queue_tail(&pipe->io_comp_queue, skb); - schedule_work(&pipe->io_complete_work); + queue_work(pipe->ar_usb->wq, &pipe->io_complete_work); } static void ath6kl_usb_io_comp_work(struct work_struct *work) @@ -619,6 +620,7 @@ static void ath6kl_usb_destroy(struct ath6kl_usb *ar_usb) kfree(ar_usb->diag_cmd_buffer); kfree(ar_usb->diag_resp_buffer); + destroy_workqueue(ar_usb->wq); kfree(ar_usb); } @@ -631,9 +633,15 @@ static struct ath6kl_usb *ath6kl_usb_create(struct usb_interface *interface) int status = 0; int i; + /* ath6kl_usb_destroy() needs ar_usb != NULL && ar_usb->wq != NULL. */ ar_usb = kzalloc(sizeof(struct ath6kl_usb), GFP_KERNEL); if (ar_usb == NULL) - goto fail_ath6kl_usb_create; + return NULL; + ar_usb->wq = alloc_workqueue("ath6kl_wq", 0, 0); + if (!ar_usb->wq) { + kfree(ar_usb); + return NULL; + } usb_set_intfdata(interface, ar_usb); spin_lock_init(&(ar_usb->cs_lock));
Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp> writes: > On 2022/06/11 7:51, Tetsuo Handa wrote: >> On 2022/06/11 4:10, Jeff Johnson wrote: >>> On 6/10/2022 12:05 PM, Jeff Johnson wrote: >>>>> +static int __init ath6kl_init(void) >>>>> +{ >>>>> + int ret; >>>>> + >>>>> + ath6kl_wq = alloc_workqueue("ath6kl_wq", 0, 0); >>>>> + if (!ath6kl_wq) >>>>> + return -ENOMEM; >>>> >>>> this approach means the driver will always allocate a workqueue even if the associated hardware is never present. >> >> Creating a WQ without WQ_MEM_RECLAIM flag consumes little resource. > > My understanding is that a loadable kernel module file is modprobe'd when > the kernel detected a hardware and some userspace program determined a .ko > file to use based on device IDs database). Thus, I think it is very likely > that an associated hardware presents if module's __init function is called. > > If a module is built-in, that user is ready to tolerate memory footprint > wasted by non-present hardware. And I think memory wasted by keeping an > unused !WQ_MEM_RECLAIM workqueue is much smaller than memory wasted by > keeping that module. > > Thus, I wonder who complains about creating possibly unused !WQ_MEM_RECLAIM > workqueue, unless that module is designed for tiny embedded > environments. I complain, it's still wrong. We have patches which save few bytes everywhere we can, we shouldn't deliberately increase the kernel size. >>From 00c560307d72abffea29409328be8cd69abecc95 Mon Sep 17 00:00:00 2001 > From: Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp> > Date: Sun, 12 Jun 2022 10:38:03 +0900 > Subject: [PATCH v2] ath6kl: avoid flush_scheduled_work() usage > > Use local wq in order to avoid flush_scheduled_work() usage. > > Signed-off-by: Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp> > --- > Changes in v2: > Use per "struct ath6kl_usb" workqueue instead of per module workqueue. Please don't embed patches into email, patchwork doesn't see those: https://patchwork.kernel.org/project/linux-wireless/list/ Please submit v3. > Please see commit c4f135d643823a86 ("workqueue: Wrap flush_workqueue() > using a macro") for background. > > This is a blind conversion, and is only compile tested. This is good information to have, please include that to the commit log so that it's stored to git.
diff --git a/drivers/net/wireless/ath/ath6kl/usb.c b/drivers/net/wireless/ath/ath6kl/usb.c index 65e683effdcb..e3c65a671be1 100644 --- a/drivers/net/wireless/ath/ath6kl/usb.c +++ b/drivers/net/wireless/ath/ath6kl/usb.c @@ -21,6 +21,8 @@ #include "debug.h" #include "core.h" +static struct workqueue_struct *ath6kl_wq; + /* constants */ #define TX_URB_COUNT 32 #define RX_URB_COUNT 32 @@ -478,7 +480,7 @@ static void ath6kl_usb_flush_all(struct ath6kl_usb *ar_usb) * Flushing any pending I/O may schedule work this call will block * until all scheduled work runs to completion. */ - flush_scheduled_work(); + flush_workqueue(ath6kl_wq); } static void ath6kl_usb_start_recv_pipes(struct ath6kl_usb *ar_usb) @@ -544,7 +546,7 @@ static void ath6kl_usb_recv_complete(struct urb *urb) /* note: queue implements a lock */ skb_queue_tail(&pipe->io_comp_queue, skb); - schedule_work(&pipe->io_complete_work); + queue_work(ath6kl_wq, &pipe->io_complete_work); cleanup_recv_urb: ath6kl_usb_cleanup_recv_urb(urb_context); @@ -579,7 +581,7 @@ static void ath6kl_usb_usb_transmit_complete(struct urb *urb) /* note: queue implements a lock */ skb_queue_tail(&pipe->io_comp_queue, skb); - schedule_work(&pipe->io_complete_work); + queue_work(ath6kl_wq, &pipe->io_complete_work); } static void ath6kl_usb_io_comp_work(struct work_struct *work) @@ -1234,7 +1236,26 @@ static struct usb_driver ath6kl_usb_driver = { .disable_hub_initiated_lpm = 1, }; -module_usb_driver(ath6kl_usb_driver); +static int __init ath6kl_init(void) +{ + int ret; + + ath6kl_wq = alloc_workqueue("ath6kl_wq", 0, 0); + if (!ath6kl_wq) + return -ENOMEM; + ret = usb_register(&ath6kl_usb_driver); + if (ret) + destroy_workqueue(ath6kl_wq); + return ret; +} +module_init(ath6kl_init); + +static void __exit ath6kl_exit(void) +{ + usb_deregister(&ath6kl_usb_driver); + destroy_workqueue(ath6kl_wq); +} +module_exit(ath6kl_exit); MODULE_AUTHOR("Atheros Communications, Inc."); MODULE_DESCRIPTION("Driver support for Atheros AR600x USB devices");
Use local wq in order to avoid flush_scheduled_work() usage. Signed-off-by: Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp> --- Please see commit c4f135d643823a86 ("workqueue: Wrap flush_workqueue() using a macro") for background. This is a blind conversion, and is only compile tested. drivers/net/wireless/ath/ath6kl/usb.c | 29 +++++++++++++++++++++++---- 1 file changed, 25 insertions(+), 4 deletions(-)