| Message ID | 20230525210110.2785470-1-luzmaximilian@gmail.com (mailing list archive) |
|---|---|
| State | Accepted, archived |
| Headers | show |
| Series | platform/surface: aggregator: Allow completion work-items to be executed in parallel | expand |
Hi, On 5/25/23 23:01, Maximilian Luz wrote: > Currently, event completion work-items are restricted to be run strictly > in non-parallel fashion by the respective workqueue. However, this has > lead to some problems: > > In some instances, the event notifier function called inside this > completion workqueue takes a non-negligible amount of time to execute. > One such example is the battery event handling code (surface_battery.c), > which can result in a full battery information refresh, involving > further synchronous communication with the EC inside the event handler. > This is made worse if the communication fails spuriously, generally > incurring a multi-second timeout. > > Since the event completions are run strictly non-parallel, this blocks > other events from being propagated to the respective subsystems. This > becomes especially noticeable for keyboard and touchpad input, which > also funnel their events through this system. Here, users have reported > occasional multi-second "freezes". > > Note, however, that the event handling system was never intended to run > purely sequentially. Instead, we have one work struct per EC/SAM > subsystem, processing the event queue for that subsystem. These work > structs were intended to run in parallel, allowing sequential processing > of work items for each subsystem but parallel processing of work items > across subsystems. > > The only restriction to this is the way the workqueue is created. > Therefore, replace create_workqueue() with alloc_workqueue() and do not > restrict the maximum number of parallel work items to be executed on > that queue, resolving any cross-subsystem blockage. > > Fixes: c167b9c7e3d6 ("platform/surface: Add Surface Aggregator subsystem") > Link: https://github.com/linux-surface/linux-surface/issues/1026 > Signed-off-by: Maximilian Luz <luzmaximilian@gmail.com> Thank you for your patch, I've applied this patch to my fixes branch: https://git.kernel.org/pub/scm/linux/kernel/git/pdx86/platform-drivers-x86.git/log/?h=fixes Note it will show up in my fixes branch once I've pushed my local branch there, which might take a while. I will include this patch in my next fixes pull-req to Linus for the current kernel development cycle. Regards, Hans > --- > drivers/platform/surface/aggregator/controller.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/drivers/platform/surface/aggregator/controller.c b/drivers/platform/surface/aggregator/controller.c > index 535581c0471c5..7fc602e01487d 100644 > --- a/drivers/platform/surface/aggregator/controller.c > +++ b/drivers/platform/surface/aggregator/controller.c > @@ -825,7 +825,7 @@ static int ssam_cplt_init(struct ssam_cplt *cplt, struct device *dev) > > cplt->dev = dev; > > - cplt->wq = create_workqueue(SSAM_CPLT_WQ_NAME); > + cplt->wq = alloc_workqueue(SSAM_CPLT_WQ_NAME, WQ_UNBOUND | WQ_MEM_RECLAIM, 0); > if (!cplt->wq) > return -ENOMEM; >
diff --git a/drivers/platform/surface/aggregator/controller.c b/drivers/platform/surface/aggregator/controller.c index 535581c0471c5..7fc602e01487d 100644 --- a/drivers/platform/surface/aggregator/controller.c +++ b/drivers/platform/surface/aggregator/controller.c @@ -825,7 +825,7 @@ static int ssam_cplt_init(struct ssam_cplt *cplt, struct device *dev) cplt->dev = dev; - cplt->wq = create_workqueue(SSAM_CPLT_WQ_NAME); + cplt->wq = alloc_workqueue(SSAM_CPLT_WQ_NAME, WQ_UNBOUND | WQ_MEM_RECLAIM, 0); if (!cplt->wq) return -ENOMEM;
Currently, event completion work-items are restricted to be run strictly in non-parallel fashion by the respective workqueue. However, this has lead to some problems: In some instances, the event notifier function called inside this completion workqueue takes a non-negligible amount of time to execute. One such example is the battery event handling code (surface_battery.c), which can result in a full battery information refresh, involving further synchronous communication with the EC inside the event handler. This is made worse if the communication fails spuriously, generally incurring a multi-second timeout. Since the event completions are run strictly non-parallel, this blocks other events from being propagated to the respective subsystems. This becomes especially noticeable for keyboard and touchpad input, which also funnel their events through this system. Here, users have reported occasional multi-second "freezes". Note, however, that the event handling system was never intended to run purely sequentially. Instead, we have one work struct per EC/SAM subsystem, processing the event queue for that subsystem. These work structs were intended to run in parallel, allowing sequential processing of work items for each subsystem but parallel processing of work items across subsystems. The only restriction to this is the way the workqueue is created. Therefore, replace create_workqueue() with alloc_workqueue() and do not restrict the maximum number of parallel work items to be executed on that queue, resolving any cross-subsystem blockage. Fixes: c167b9c7e3d6 ("platform/surface: Add Surface Aggregator subsystem") Link: https://github.com/linux-surface/linux-surface/issues/1026 Signed-off-by: Maximilian Luz <luzmaximilian@gmail.com> --- drivers/platform/surface/aggregator/controller.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)