From: Hans de Goede <hdegoede@redhat.com>
To: Maximilian Luz <luzmaximilian@gmail.com>, linux-kernel@vger.kernel.org
Cc: "Mark Gross" <mgross@linux.intel.com>,
"Andy Shevchenko" <andy.shevchenko@gmail.com>,
"Barnabás Pőcze" <pobrn@protonmail.com>,
"Arnd Bergmann" <arnd@arndb.de>,
"Greg Kroah-Hartman" <gregkh@linuxfoundation.org>,
"Blaž Hrastnik" <blaz@mxxn.io>,
"Dorian Stoll" <dorian.stoll@tmsp.io>,
platform-driver-x86@vger.kernel.org
Subject: Re: [PATCH v2 3/9] platform/surface: aggregator: Add event item allocation caching
Date: Tue, 15 Dec 2020 14:44:12 +0100 [thread overview]
Message-ID: <979fd103-aaf7-13cf-296f-cb9b30627bdd@redhat.com> (raw)
In-Reply-To: <20201203212640.663931-4-luzmaximilian@gmail.com>
Hi,
On 12/3/20 10:26 PM, Maximilian Luz wrote:
> Event items are used for completing Surface Aggregator EC events, i.e.
> placing event command data and payload on a workqueue for later
> processing to avoid doing said processing directly on the receiver
> thread. This means that event items are allocated for each incoming
> event, regardless of that event being transmitted via sequenced or
> unsequenced packets.
>
> On the Surface Book 3 and Surface Laptop 3, touchpad HID input events
> (unsequenced), can constitute a larger amount of traffic, and therefore
> allocation of event items. This warrants caching event items to reduce
> memory fragmentation. The size of the cached objects is specifically
> tuned to accommodate keyboard and touchpad input events and their
> payloads on those devices. As a result, this effectively also covers
> most other event types. In case of a larger event payload, event item
> allocation will fall back to kzalloc().
>
> Signed-off-by: Maximilian Luz <luzmaximilian@gmail.com>
Thanks, patch looks good to me:
Reviewed-by: Hans de Goede <hdegoede@redhat.com>
Regards,
Hans
> ---
> .../platform/surface/aggregator/controller.c | 86 +++++++++++++++++--
> .../platform/surface/aggregator/controller.h | 9 ++
> drivers/platform/surface/aggregator/core.c | 16 +++-
> 3 files changed, 101 insertions(+), 10 deletions(-)
>
> diff --git a/drivers/platform/surface/aggregator/controller.c b/drivers/platform/surface/aggregator/controller.c
> index 8d9811cc2943..89ffd8e45787 100644
> --- a/drivers/platform/surface/aggregator/controller.c
> +++ b/drivers/platform/surface/aggregator/controller.c
> @@ -514,14 +514,74 @@ static void ssam_nf_destroy(struct ssam_nf *nf)
> */
> #define SSAM_CPLT_WQ_BATCH 10
>
> +/*
> + * SSAM_EVENT_ITEM_CACHE_PAYLOAD_LEN - Maximum payload length for a cached
> + * &struct ssam_event_item.
> + *
> + * This length has been chosen to be accommodate standard touchpad and
> + * keyboard input events. Events with larger payloads will be allocated
> + * separately.
> + */
> +#define SSAM_EVENT_ITEM_CACHE_PAYLOAD_LEN 32
> +
> +static struct kmem_cache *ssam_event_item_cache;
> +
> +/**
> + * ssam_event_item_cache_init() - Initialize the event item cache.
> + */
> +int ssam_event_item_cache_init(void)
> +{
> + const unsigned int size = sizeof(struct ssam_event_item)
> + + SSAM_EVENT_ITEM_CACHE_PAYLOAD_LEN;
> + const unsigned int align = __alignof__(struct ssam_event_item);
> + struct kmem_cache *cache;
> +
> + cache = kmem_cache_create("ssam_event_item", size, align, 0, NULL);
> + if (!cache)
> + return -ENOMEM;
> +
> + ssam_event_item_cache = cache;
> + return 0;
> +}
> +
> +/**
> + * ssam_event_item_cache_destroy() - Deinitialize the event item cache.
> + */
> +void ssam_event_item_cache_destroy(void)
> +{
> + kmem_cache_destroy(ssam_event_item_cache);
> + ssam_event_item_cache = NULL;
> +}
> +
> +static void __ssam_event_item_free_cached(struct ssam_event_item *item)
> +{
> + kmem_cache_free(ssam_event_item_cache, item);
> +}
> +
> +static void __ssam_event_item_free_generic(struct ssam_event_item *item)
> +{
> + kfree(item);
> +}
> +
> +/**
> + * ssam_event_item_free() - Free the provided event item.
> + * @item: The event item to free.
> + */
> +static void ssam_event_item_free(struct ssam_event_item *item)
> +{
> + item->ops.free(item);
> +}
> +
> /**
> * ssam_event_item_alloc() - Allocate an event item with the given payload size.
> * @len: The event payload length.
> * @flags: The flags used for allocation.
> *
> - * Allocate an event item with the given payload size. Sets the item
> - * operations and payload length values. The item free callback (``ops.free``)
> - * should not be overwritten after this call.
> + * Allocate an event item with the given payload size, preferring allocation
> + * from the event item cache if the payload is small enough (i.e. smaller than
> + * %SSAM_EVENT_ITEM_CACHE_PAYLOAD_LEN). Sets the item operations and payload
> + * length values. The item free callback (``ops.free``) should not be
> + * overwritten after this call.
> *
> * Return: Returns the newly allocated event item.
> */
> @@ -529,9 +589,19 @@ static struct ssam_event_item *ssam_event_item_alloc(size_t len, gfp_t flags)
> {
> struct ssam_event_item *item;
>
> - item = kzalloc(struct_size(item, event.data, len), flags);
> - if (!item)
> - return NULL;
> + if (len <= SSAM_EVENT_ITEM_CACHE_PAYLOAD_LEN) {
> + item = kmem_cache_alloc(ssam_event_item_cache, flags);
> + if (!item)
> + return NULL;
> +
> + item->ops.free = __ssam_event_item_free_cached;
> + } else {
> + item = kzalloc(struct_size(item, event.data, len), flags);
> + if (!item)
> + return NULL;
> +
> + item->ops.free = __ssam_event_item_free_generic;
> + }
>
> item->event.length = len;
> return item;
> @@ -693,7 +763,7 @@ static void ssam_event_queue_work_fn(struct work_struct *work)
> return;
>
> ssam_nf_call(nf, dev, item->rqid, &item->event);
> - kfree(item);
> + ssam_event_item_free(item);
> } while (--iterations);
>
> if (!ssam_event_queue_is_empty(queue))
> @@ -893,7 +963,7 @@ static void ssam_handle_event(struct ssh_rtl *rtl,
> memcpy(&item->event.data[0], data->ptr, data->len);
>
> if (WARN_ON(ssam_cplt_submit_event(&ctrl->cplt, item)))
> - kfree(item);
> + ssam_event_item_free(item);
> }
>
> static const struct ssh_rtl_ops ssam_rtl_ops = {
> diff --git a/drivers/platform/surface/aggregator/controller.h b/drivers/platform/surface/aggregator/controller.h
> index 5ee9e966f1d7..8297d34e7489 100644
> --- a/drivers/platform/surface/aggregator/controller.h
> +++ b/drivers/platform/surface/aggregator/controller.h
> @@ -80,12 +80,18 @@ struct ssam_cplt;
> * struct ssam_event_item - Struct for event queuing and completion.
> * @node: The node in the queue.
> * @rqid: The request ID of the event.
> + * @ops: Instance specific functions.
> + * @ops.free: Callback for freeing this event item.
> * @event: Actual event data.
> */
> struct ssam_event_item {
> struct list_head node;
> u16 rqid;
>
> + struct {
> + void (*free)(struct ssam_event_item *event);
> + } ops;
> +
> struct ssam_event event; /* must be last */
> };
>
> @@ -273,4 +279,7 @@ int ssam_ctrl_notif_d0_entry(struct ssam_controller *ctrl);
> int ssam_controller_suspend(struct ssam_controller *ctrl);
> int ssam_controller_resume(struct ssam_controller *ctrl);
>
> +int ssam_event_item_cache_init(void);
> +void ssam_event_item_cache_destroy(void);
> +
> #endif /* _SURFACE_AGGREGATOR_CONTROLLER_H */
> diff --git a/drivers/platform/surface/aggregator/core.c b/drivers/platform/surface/aggregator/core.c
> index 77bc4c87541b..1a53d7ce66a1 100644
> --- a/drivers/platform/surface/aggregator/core.c
> +++ b/drivers/platform/surface/aggregator/core.c
> @@ -784,12 +784,23 @@ static int __init ssam_core_init(void)
>
> status = ssh_ctrl_packet_cache_init();
> if (status)
> - return status;
> + goto err_cpkg;
> +
> + status = ssam_event_item_cache_init();
> + if (status)
> + goto err_evitem;
>
> status = serdev_device_driver_register(&ssam_serial_hub);
> if (status)
> - ssh_ctrl_packet_cache_destroy();
> + goto err_register;
>
> + return 0;
> +
> +err_register:
> + ssam_event_item_cache_destroy();
> +err_evitem:
> + ssh_ctrl_packet_cache_destroy();
> +err_cpkg:
> return status;
> }
> module_init(ssam_core_init);
> @@ -797,6 +808,7 @@ module_init(ssam_core_init);
> static void __exit ssam_core_exit(void)
> {
> serdev_device_driver_unregister(&ssam_serial_hub);
> + ssam_event_item_cache_destroy();
> ssh_ctrl_packet_cache_destroy();
> }
> module_exit(ssam_core_exit);
> --
> 2.29.2
>
next prev parent reply other threads:[~2020-12-15 13:45 UTC|newest]
Thread overview: 47+ messages / expand[flat|nested] mbox.gz Atom feed top
2020-12-03 21:26 [PATCH v2 0/9] Add support for Microsoft Surface System Aggregator Module Maximilian Luz
2020-12-03 21:26 ` [PATCH v2 1/9] platform/surface: Add Surface Aggregator subsystem Maximilian Luz
2020-12-08 13:01 ` Hans de Goede
2020-12-08 14:37 ` Maximilian Luz
2020-12-08 14:43 ` Hans de Goede
2020-12-08 14:54 ` Maximilian Luz
2020-12-03 21:26 ` [PATCH v2 2/9] platform/surface: aggregator: Add control packet allocation caching Maximilian Luz
2020-12-15 13:42 ` Hans de Goede
2020-12-03 21:26 ` [PATCH v2 3/9] platform/surface: aggregator: Add event item " Maximilian Luz
2020-12-15 13:44 ` Hans de Goede [this message]
2020-12-03 21:26 ` [PATCH v2 4/9] platform/surface: aggregator: Add trace points Maximilian Luz
2020-12-15 14:20 ` Hans de Goede
2020-12-03 21:26 ` [PATCH v2 5/9] platform/surface: aggregator: Add error injection capabilities Maximilian Luz
2020-12-15 14:43 ` Hans de Goede
2020-12-03 21:26 ` [PATCH v2 6/9] platform/surface: aggregator: Add dedicated bus and device type Maximilian Luz
2020-12-15 14:49 ` Hans de Goede
2020-12-15 14:50 ` Hans de Goede
2020-12-03 21:26 ` [PATCH v2 7/9] docs: driver-api: Add Surface Aggregator subsystem documentation Maximilian Luz
2020-12-15 16:25 ` Hans de Goede
2020-12-03 21:26 ` [PATCH v2 8/9] platform/surface: Add Surface Aggregator user-space interface Maximilian Luz
2020-12-15 16:35 ` Hans de Goede
2020-12-15 20:00 ` Maximilian Luz
2020-12-03 21:26 ` [PATCH v2 9/9] platform/surface: Add Surface ACPI Notify driver Maximilian Luz
2020-12-15 17:18 ` Hans de Goede
2020-12-06 7:07 ` [PATCH v2 0/9] Add support for Microsoft Surface System Aggregator Module Leon Romanovsky
2020-12-06 8:32 ` Greg Kroah-Hartman
2020-12-06 8:35 ` Leon Romanovsky
2020-12-06 11:13 ` Maximilian Luz
2020-12-06 8:41 ` Hans de Goede
2020-12-06 8:56 ` Leon Romanovsky
2020-12-06 10:04 ` Hans de Goede
2020-12-06 10:33 ` Leon Romanovsky
2020-12-06 10:41 ` Hans de Goede
2020-12-06 11:41 ` Leon Romanovsky
2020-12-06 13:43 ` Maximilian Luz
2020-12-06 10:51 ` Maximilian Luz
2020-12-06 8:58 ` Blaž Hrastnik
2020-12-06 9:06 ` Leon Romanovsky
2020-12-06 10:33 ` Maximilian Luz
2020-12-06 10:43 ` Hans de Goede
2020-12-06 10:56 ` Maximilian Luz
2020-12-06 11:30 ` Leon Romanovsky
2020-12-06 13:27 ` Maximilian Luz
2020-12-06 15:58 ` Maximilian Luz
2020-12-07 6:15 ` Leon Romanovsky
2020-12-07 8:49 ` Hans de Goede
2020-12-07 9:12 ` Greg Kroah-Hartman
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=979fd103-aaf7-13cf-296f-cb9b30627bdd@redhat.com \
--to=hdegoede@redhat.com \
--cc=andy.shevchenko@gmail.com \
--cc=arnd@arndb.de \
--cc=blaz@mxxn.io \
--cc=dorian.stoll@tmsp.io \
--cc=gregkh@linuxfoundation.org \
--cc=linux-kernel@vger.kernel.org \
--cc=luzmaximilian@gmail.com \
--cc=mgross@linux.intel.com \
--cc=platform-driver-x86@vger.kernel.org \
--cc=pobrn@protonmail.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).