Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
Subject: Re: [PATCH v2 2/9] platform/surface: aggregator: Add control packet
 allocation caching
To:     Maximilian Luz <luzmaximilian@gmail.com>,
        linux-kernel@vger.kernel.org
Cc:     Mark Gross <mgross@linux.intel.com>,
        Andy Shevchenko <andy.shevchenko@gmail.com>,
        =?UTF-8?Q?Barnab=c3=a1s_P=c5=91cze?= <pobrn@protonmail.com>,
        Arnd Bergmann <arnd@arndb.de>,
        Greg Kroah-Hartman <gregkh@linuxfoundation.org>,
        =?UTF-8?Q?Bla=c5=be_Hrastn?= =?UTF-8?Q?ik?= <blaz@mxxn.io>,
        Dorian Stoll <dorian.stoll@tmsp.io>,
        platform-driver-x86@vger.kernel.org
References: <20201203212640.663931-1-luzmaximilian@gmail.com>
 <20201203212640.663931-3-luzmaximilian@gmail.com>
From:   Hans de Goede <hdegoede@redhat.com>
Message-ID: <879bdec2-3efd-8eac-c19e-cd8282367bef@redhat.com>
Date:   Tue, 15 Dec 2020 14:42:38 +0100
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:78.0) Gecko/20100101
 Thunderbird/78.4.0
MIME-Version: 1.0
In-Reply-To: <20201203212640.663931-3-luzmaximilian@gmail.com>
Content-Type: text/plain; charset=utf-8
Content-Language: en-US
Content-Transfer-Encoding: 7bit
Precedence: bulk

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
>