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 2/9] platform/surface: aggregator: Add control packet allocation caching
Date: Tue, 15 Dec 2020 14:42:38 +0100 [thread overview]
Message-ID: <879bdec2-3efd-8eac-c19e-cd8282367bef@redhat.com> (raw)
In-Reply-To: <20201203212640.663931-3-luzmaximilian@gmail.com>
Hi,
On 12/3/20 10:26 PM, Maximilian Luz wrote:
> Surface Serial Hub communication is, in its core, packet based. Each
> sequenced packet requires to be acknowledged, via an ACK-type control
> packet. In case invalid data has been received by the driver, a NAK-type
> (not-acknowledge/negative acknowledge) control packet is sent,
> triggering retransmission.
>
> Control packets are therefore a core communication primitive and used
> frequently enough (with every sequenced packet transmission sent by the
> embedded controller, including events and request responses) that it may
> warrant caching their allocations to reduce possible memory
> fragmentation.
>
> Signed-off-by: Maximilian Luz <luzmaximilian@gmail.com>
Thanks, patch looks good to me:
Reviewed-by: Hans de Goede <hdegoede@redhat.com>
Regards,
Hans
> ---
> drivers/platform/surface/aggregator/core.c | 27 ++++++++++-
> .../surface/aggregator/ssh_packet_layer.c | 47 +++++++++++++++----
> .../surface/aggregator/ssh_packet_layer.h | 3 ++
> 3 files changed, 67 insertions(+), 10 deletions(-)
>
> diff --git a/drivers/platform/surface/aggregator/core.c b/drivers/platform/surface/aggregator/core.c
> index ec6c7f40ad36..77bc4c87541b 100644
> --- a/drivers/platform/surface/aggregator/core.c
> +++ b/drivers/platform/surface/aggregator/core.c
> @@ -774,7 +774,32 @@ static struct serdev_device_driver ssam_serial_hub = {
> .probe_type = PROBE_PREFER_ASYNCHRONOUS,
> },
> };
> -module_serdev_device_driver(ssam_serial_hub);
> +
> +
> +/* -- Module setup. --------------------------------------------------------- */
> +
> +static int __init ssam_core_init(void)
> +{
> + int status;
> +
> + status = ssh_ctrl_packet_cache_init();
> + if (status)
> + return status;
> +
> + status = serdev_device_driver_register(&ssam_serial_hub);
> + if (status)
> + ssh_ctrl_packet_cache_destroy();
> +
> + return status;
> +}
> +module_init(ssam_core_init);
> +
> +static void __exit ssam_core_exit(void)
> +{
> + serdev_device_driver_unregister(&ssam_serial_hub);
> + ssh_ctrl_packet_cache_destroy();
> +}
> +module_exit(ssam_core_exit);
>
> MODULE_AUTHOR("Maximilian Luz <luzmaximilian@gmail.com>");
> MODULE_DESCRIPTION("Subsystem and Surface Serial Hub driver for Surface System Aggregator Module");
> diff --git a/drivers/platform/surface/aggregator/ssh_packet_layer.c b/drivers/platform/surface/aggregator/ssh_packet_layer.c
> index 237d28c90e4b..8bc19837cde0 100644
> --- a/drivers/platform/surface/aggregator/ssh_packet_layer.c
> +++ b/drivers/platform/surface/aggregator/ssh_packet_layer.c
> @@ -302,24 +302,53 @@ void ssh_packet_init(struct ssh_packet *packet, unsigned long type,
> packet->ops = ops;
> }
>
> +static struct kmem_cache *ssh_ctrl_packet_cache;
> +
> +/**
> + * ssh_ctrl_packet_cache_init() - Initialize the control packet cache.
> + */
> +int ssh_ctrl_packet_cache_init(void)
> +{
> + const unsigned int size = sizeof(struct ssh_packet) + SSH_MSG_LEN_CTRL;
> + const unsigned int align = __alignof__(struct ssh_packet);
> + struct kmem_cache *cache;
> +
> + cache = kmem_cache_create("ssam_ctrl_packet", size, align, 0, NULL);
> + if (!cache)
> + return -ENOMEM;
> +
> + ssh_ctrl_packet_cache = cache;
> + return 0;
> +}
> +
> +/**
> + * ssh_ctrl_packet_cache_destroy() - Deinitialize the control packet cache.
> + */
> +void ssh_ctrl_packet_cache_destroy(void)
> +{
> + kmem_cache_destroy(ssh_ctrl_packet_cache);
> + ssh_ctrl_packet_cache = NULL;
> +}
> +
> /**
> - * ssh_ctrl_packet_alloc() - Allocate control packet.
> + * ssh_ctrl_packet_alloc() - Allocate packet from control packet cache.
> * @packet: Where the pointer to the newly allocated packet should be stored.
> * @buffer: The buffer corresponding to this packet.
> * @flags: Flags used for allocation.
> *
> - * Allocates a packet and corresponding transport buffer. Sets the packet's
> - * buffer reference to the allocated buffer. The packet must be freed via
> - * ssh_ctrl_packet_free(), which will also free the corresponding buffer. The
> - * corresponding buffer must not be freed separately. Intended to be used with
> - * %ssh_ptl_ctrl_packet_ops as packet operations.
> + * Allocates a packet and corresponding transport buffer from the control
> + * packet cache. Sets the packet's buffer reference to the allocated buffer.
> + * The packet must be freed via ssh_ctrl_packet_free(), which will also free
> + * the corresponding buffer. The corresponding buffer must not be freed
> + * separately. Intended to be used with %ssh_ptl_ctrl_packet_ops as packet
> + * operations.
> *
> * Return: Returns zero on success, %-ENOMEM if the allocation failed.
> */
> static int ssh_ctrl_packet_alloc(struct ssh_packet **packet,
> struct ssam_span *buffer, gfp_t flags)
> {
> - *packet = kzalloc(sizeof(**packet) + SSH_MSG_LEN_CTRL, flags);
> + *packet = kmem_cache_alloc(ssh_ctrl_packet_cache, flags);
> if (!*packet)
> return -ENOMEM;
>
> @@ -330,12 +359,12 @@ static int ssh_ctrl_packet_alloc(struct ssh_packet **packet,
> }
>
> /**
> - * ssh_ctrl_packet_free() - Free control packet.
> + * ssh_ctrl_packet_free() - Free packet allocated from control packet cache.
> * @p: The packet to free.
> */
> static void ssh_ctrl_packet_free(struct ssh_packet *p)
> {
> - kfree(p);
> + kmem_cache_free(ssh_ctrl_packet_cache, p);
> }
>
> static const struct ssh_packet_ops ssh_ptl_ctrl_packet_ops = {
> diff --git a/drivers/platform/surface/aggregator/ssh_packet_layer.h b/drivers/platform/surface/aggregator/ssh_packet_layer.h
> index 058f111292ca..e8757d03f279 100644
> --- a/drivers/platform/surface/aggregator/ssh_packet_layer.h
> +++ b/drivers/platform/surface/aggregator/ssh_packet_layer.h
> @@ -184,4 +184,7 @@ static inline void ssh_ptl_tx_wakeup_transfer(struct ssh_ptl *ptl)
> void ssh_packet_init(struct ssh_packet *packet, unsigned long type,
> u8 priority, const struct ssh_packet_ops *ops);
>
> +int ssh_ctrl_packet_cache_init(void);
> +void ssh_ctrl_packet_cache_destroy(void);
> +
> #endif /* _SURFACE_AGGREGATOR_SSH_PACKET_LAYER_H */
> --
> 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 [this message]
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
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=879bdec2-3efd-8eac-c19e-cd8282367bef@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).