[PATCH 0/1] rpmsg: virtio: Make buffer size and number configurable

[PATCH 0/1] rpmsg: virtio: Make buffer size and number configurable

[Divin Raj]

This change is necessary to support the allocation of memory for an out-of-tree
rpmsg ethernet device driver over virtio, enabling support for packet sizes
based on the MTU (Maximum Transmission Unit) size (1500).

Peter Hoyes (1):
  rpmsg: virtio: Make buffer size and number configurable

 drivers/rpmsg/Kconfig            | 23 +++++++++++++++++++++++
 drivers/rpmsg/virtio_rpmsg_bus.c | 27 +++------------------------
 2 files changed, 26 insertions(+), 24 deletions(-)

-- 
2.25.1

[PATCH 1/1] rpmsg: virtio: Make buffer size and number configurable

[Divin Raj]

From: Peter Hoyes <Peter.Hoyes@arm.com>

Replace the MAX_RPMSG_BUF_SIZE and MAX_RPMSG_NUM_BUFS #define in
virtio_rpmsg_bus.c with the Kconfig parameters CONFIG_RPMSG_VIRTIO_BUF_SIZE
and CONFIG_RPMSG_VIRTIO_MAX_NUM_BUFS, allowing user-provided customization.

Making both the number of buffers and size configurable facilitates aligning
memory requirements between vdev-buffer and vdev-vrings for client drivers
that require larger buffer sizes, for example.

Signed-off-by: Peter Hoyes <Peter.Hoyes@arm.com>
Signed-off-by: Divin Raj <divin.raj@arm.com>
---
 drivers/rpmsg/Kconfig            | 23 +++++++++++++++++++++++
 drivers/rpmsg/virtio_rpmsg_bus.c | 27 +++------------------------
 2 files changed, 26 insertions(+), 24 deletions(-)

diff --git a/drivers/rpmsg/Kconfig b/drivers/rpmsg/Kconfig
index d3795860f5c0..677f4a1ac8bb 100644
--- a/drivers/rpmsg/Kconfig
+++ b/drivers/rpmsg/Kconfig
@@ -81,4 +81,27 @@ config RPMSG_VIRTIO
 	select RPMSG_NS
 	select VIRTIO
 
+config RPMSG_VIRTIO_MAX_BUF_SIZE
+	int "Virtio RPMSG max buffer size (in bytes)"
+	default 512
+	depends on RPMSG_VIRTIO
+	help
+	  This option allows you to configure the maximum buffer size (in bytes)
+	  for Virtio RPMSG communications. The number of buffers will be computed
+	  based on the number of buffers (CONFIG_RPMSG_VIRTIO_MAX_NUM_BUFS)
+	  supported by the vring. By default, it supports up to a maximum of 512
+	  buffers (256 in each direction). Each buffer consists of 16 bytes for the
+	  message header and the remaining bytes for the payload.The default values
+	  will utilize a maximum total space of 256KB for the buffers.
+
+config RPMSG_VIRTIO_MAX_NUM_BUFS
+	int "Virtio RPMSG max buffer count (even number for TX and Rx)"
+	default 512
+	depends on RPMSG_VIRTIO
+	help
+	  This option allows you to configure the maximum number of buffers used
+	  for Virtio RPMSG communication. By default, it supports up to a maximum
+	  of 512 buffers (256 in each direction). Please note that this value
+	  should be an even number, as it accounts for both transmit (TX) and
+	  receive (Rx) buffers.
 endmenu
diff --git a/drivers/rpmsg/virtio_rpmsg_bus.c b/drivers/rpmsg/virtio_rpmsg_bus.c
index 905ac7910c98..87a9a4fa30e0 100644
--- a/drivers/rpmsg/virtio_rpmsg_bus.c
+++ b/drivers/rpmsg/virtio_rpmsg_bus.c
@@ -109,27 +109,6 @@ struct virtio_rpmsg_channel {
 #define to_virtio_rpmsg_channel(_rpdev) \
 	container_of(_rpdev, struct virtio_rpmsg_channel, rpdev)
 
-/*
- * We're allocating buffers of 512 bytes each for communications. The
- * number of buffers will be computed from the number of buffers supported
- * by the vring, upto a maximum of 512 buffers (256 in each direction).
- *
- * Each buffer will have 16 bytes for the msg header and 496 bytes for
- * the payload.
- *
- * This will utilize a maximum total space of 256KB for the buffers.
- *
- * We might also want to add support for user-provided buffers in time.
- * This will allow bigger buffer size flexibility, and can also be used
- * to achieve zero-copy messaging.
- *
- * Note that these numbers are purely a decision of this driver - we
- * can change this without changing anything in the firmware of the remote
- * processor.
- */
-#define MAX_RPMSG_NUM_BUFS	(512)
-#define MAX_RPMSG_BUF_SIZE	(512)
-
 /*
  * Local addresses are dynamically allocated on-demand.
  * We do not dynamically assign addresses from the low 1024 range,
@@ -902,12 +881,12 @@ static int rpmsg_probe(struct virtio_device *vdev)
 		virtqueue_get_vring_size(vrp->svq));
 
 	/* we need less buffers if vrings are small */
-	if (virtqueue_get_vring_size(vrp->rvq) < MAX_RPMSG_NUM_BUFS / 2)
+	if (virtqueue_get_vring_size(vrp->rvq) < CONFIG_RPMSG_VIRTIO_MAX_NUM_BUFS / 2)
 		vrp->num_bufs = virtqueue_get_vring_size(vrp->rvq) * 2;
 	else
-		vrp->num_bufs = MAX_RPMSG_NUM_BUFS;
+		vrp->num_bufs = CONFIG_RPMSG_VIRTIO_MAX_NUM_BUFS;
 
-	vrp->buf_size = MAX_RPMSG_BUF_SIZE;
+	vrp->buf_size = CONFIG_RPMSG_VIRTIO_MAX_BUF_SIZE;
 
 	total_buf_space = vrp->num_bufs * vrp->buf_size;
 
-- 
2.25.1

Re: [PATCH 1/1] rpmsg: virtio: Make buffer size and number configurable

[Randy Dunlap]

Hi,

On 9/28/23 08:38, Divin Raj wrote:
> From: Peter Hoyes <Peter.Hoyes@arm.com>
> 
> Replace the MAX_RPMSG_BUF_SIZE and MAX_RPMSG_NUM_BUFS #define in
> virtio_rpmsg_bus.c with the Kconfig parameters CONFIG_RPMSG_VIRTIO_BUF_SIZE
> and CONFIG_RPMSG_VIRTIO_MAX_NUM_BUFS, allowing user-provided customization.
> 
> Making both the number of buffers and size configurable facilitates aligning
> memory requirements between vdev-buffer and vdev-vrings for client drivers
> that require larger buffer sizes, for example.
> 
> Signed-off-by: Peter Hoyes <Peter.Hoyes@arm.com>
> Signed-off-by: Divin Raj <divin.raj@arm.com>
> ---
>  drivers/rpmsg/Kconfig            | 23 +++++++++++++++++++++++
>  drivers/rpmsg/virtio_rpmsg_bus.c | 27 +++------------------------
>  2 files changed, 26 insertions(+), 24 deletions(-)
> 
> diff --git a/drivers/rpmsg/Kconfig b/drivers/rpmsg/Kconfig
> index d3795860f5c0..677f4a1ac8bb 100644
> --- a/drivers/rpmsg/Kconfig
> +++ b/drivers/rpmsg/Kconfig
> @@ -81,4 +81,27 @@ config RPMSG_VIRTIO
>  	select RPMSG_NS
>  	select VIRTIO
>  
> +config RPMSG_VIRTIO_MAX_BUF_SIZE
> +	int "Virtio RPMSG max buffer size (in bytes)"
> +	default 512

Looks to me like you need to:

(a) use the "range" kconfig keyword (Documentation/kbuild/kconfig-language.rst)
and/or
(b) change the source code (driver) to check that both of these new config
variables' values make sense.

As is (in this patch), I could enter a value of 1 for each of them
and see what happens.

> +	depends on RPMSG_VIRTIO
> +	help
> +	  This option allows you to configure the maximum buffer size (in bytes)
> +	  for Virtio RPMSG communications. The number of buffers will be computed
> +	  based on the number of buffers (CONFIG_RPMSG_VIRTIO_MAX_NUM_BUFS)
> +	  supported by the vring. By default, it supports up to a maximum of 512
> +	  buffers (256 in each direction). Each buffer consists of 16 bytes for the
> +	  message header and the remaining bytes for the payload.The default values

	                                                 payload. The

> +	  will utilize a maximum total space of 256KB for the buffers.
> +
> +config RPMSG_VIRTIO_MAX_NUM_BUFS
> +	int "Virtio RPMSG max buffer count (even number for TX and Rx)"

	                                                    Tx and Rx)"

> +	default 512
> +	depends on RPMSG_VIRTIO
> +	help
> +	  This option allows you to configure the maximum number of buffers used
> +	  for Virtio RPMSG communication. By default, it supports up to a maximum
> +	  of 512 buffers (256 in each direction). Please note that this value
> +	  should be an even number, as it accounts for both transmit (TX) and
> +	  receive (Rx) buffers.
>  endmenu


-- 
~Randy

Re: [PATCH 1/1] rpmsg: virtio: Make buffer size and number configurable

[Tanmay Shah]

Hello,

Thanks for your patch.

Instead of having this in Kconfig, It's better to have buffer size decided dynamically. Probably by resource-table.

We still need implementation that achieves that goal. Meanwhile  I think it's best to keep buffer size fixed.

Thanks.

On 9/28/23 10:38 AM, Divin Raj wrote:

> CAUTION: This message has originated from an External Source. Please use proper judgment and caution when opening attachments, clicking links, or responding to this email.
>
>
> From: Peter Hoyes <Peter.Hoyes@arm.com>
>
> Replace the MAX_RPMSG_BUF_SIZE and MAX_RPMSG_NUM_BUFS #define in
> virtio_rpmsg_bus.c with the Kconfig parameters CONFIG_RPMSG_VIRTIO_BUF_SIZE
> and CONFIG_RPMSG_VIRTIO_MAX_NUM_BUFS, allowing user-provided customization.
>
> Making both the number of buffers and size configurable facilitates aligning
> memory requirements between vdev-buffer and vdev-vrings for client drivers
> that require larger buffer sizes, for example.
>
> Signed-off-by: Peter Hoyes <Peter.Hoyes@arm.com>
> Signed-off-by: Divin Raj <divin.raj@arm.com>
> ---
>  drivers/rpmsg/Kconfig            | 23 +++++++++++++++++++++++
>  drivers/rpmsg/virtio_rpmsg_bus.c | 27 +++------------------------
>  2 files changed, 26 insertions(+), 24 deletions(-)
>
> diff --git a/drivers/rpmsg/Kconfig b/drivers/rpmsg/Kconfig
> index d3795860f5c0..677f4a1ac8bb 100644
> --- a/drivers/rpmsg/Kconfig
> +++ b/drivers/rpmsg/Kconfig
> @@ -81,4 +81,27 @@ config RPMSG_VIRTIO
>         select RPMSG_NS
>         select VIRTIO
>
> +config RPMSG_VIRTIO_MAX_BUF_SIZE
> +       int "Virtio RPMSG max buffer size (in bytes)"
> +       default 512
> +       depends on RPMSG_VIRTIO
> +       help
> +         This option allows you to configure the maximum buffer size (in bytes)
> +         for Virtio RPMSG communications. The number of buffers will be computed
> +         based on the number of buffers (CONFIG_RPMSG_VIRTIO_MAX_NUM_BUFS)
> +         supported by the vring. By default, it supports up to a maximum of 512
> +         buffers (256 in each direction). Each buffer consists of 16 bytes for the
> +         message header and the remaining bytes for the payload.The default values
> +         will utilize a maximum total space of 256KB for the buffers.
> +
> +config RPMSG_VIRTIO_MAX_NUM_BUFS
> +       int "Virtio RPMSG max buffer count (even number for TX and Rx)"
> +       default 512
> +       depends on RPMSG_VIRTIO
> +       help
> +         This option allows you to configure the maximum number of buffers used
> +         for Virtio RPMSG communication. By default, it supports up to a maximum
> +         of 512 buffers (256 in each direction). Please note that this value
> +         should be an even number, as it accounts for both transmit (TX) and
> +         receive (Rx) buffers.
>  endmenu
> diff --git a/drivers/rpmsg/virtio_rpmsg_bus.c b/drivers/rpmsg/virtio_rpmsg_bus.c
> index 905ac7910c98..87a9a4fa30e0 100644
> --- a/drivers/rpmsg/virtio_rpmsg_bus.c
> +++ b/drivers/rpmsg/virtio_rpmsg_bus.c
> @@ -109,27 +109,6 @@ struct virtio_rpmsg_channel {
>  #define to_virtio_rpmsg_channel(_rpdev) \
>         container_of(_rpdev, struct virtio_rpmsg_channel, rpdev)
>
> -/*
> - * We're allocating buffers of 512 bytes each for communications. The
> - * number of buffers will be computed from the number of buffers supported
> - * by the vring, upto a maximum of 512 buffers (256 in each direction).
> - *
> - * Each buffer will have 16 bytes for the msg header and 496 bytes for
> - * the payload.
> - *
> - * This will utilize a maximum total space of 256KB for the buffers.
> - *
> - * We might also want to add support for user-provided buffers in time.
> - * This will allow bigger buffer size flexibility, and can also be used
> - * to achieve zero-copy messaging.
> - *
> - * Note that these numbers are purely a decision of this driver - we
> - * can change this without changing anything in the firmware of the remote
> - * processor.
> - */
> -#define MAX_RPMSG_NUM_BUFS     (512)
> -#define MAX_RPMSG_BUF_SIZE     (512)
> -
>  /*
>   * Local addresses are dynamically allocated on-demand.
>   * We do not dynamically assign addresses from the low 1024 range,
> @@ -902,12 +881,12 @@ static int rpmsg_probe(struct virtio_device *vdev)
>                 virtqueue_get_vring_size(vrp->svq));
>
>         /* we need less buffers if vrings are small */
> -       if (virtqueue_get_vring_size(vrp->rvq) < MAX_RPMSG_NUM_BUFS / 2)
> +       if (virtqueue_get_vring_size(vrp->rvq) < CONFIG_RPMSG_VIRTIO_MAX_NUM_BUFS / 2)
>                 vrp->num_bufs = virtqueue_get_vring_size(vrp->rvq) * 2;
>         else
> -               vrp->num_bufs = MAX_RPMSG_NUM_BUFS;
> +               vrp->num_bufs = CONFIG_RPMSG_VIRTIO_MAX_NUM_BUFS;
>
> -       vrp->buf_size = MAX_RPMSG_BUF_SIZE;
> +       vrp->buf_size = CONFIG_RPMSG_VIRTIO_MAX_BUF_SIZE;
>
>         total_buf_space = vrp->num_bufs * vrp->buf_size;
>
> --
> 2.25.1
>

Re: [PATCH 1/1] rpmsg: virtio: Make buffer size and number configurable

[Tanmay Shah]

On 10/3/23 10:28 AM, Divin Raj wrote:
> Hello,
>
> Thanks for all review comments.
> It makes sense to incorporate a variable-length buffer size. However, we still require an implementation that can successfully achieve this objective.
>
> We will be investigating this matter and will return with a feasible solution.

Thanks.

Here is the previous effort:

Series: "Enhance virtio rpmsg bus driver buffer allocation"

https://lore.kernel.org/all/1548949280-31794-1-git-send-email-xiaoxiang@xiaomi.com/

There was discussion to revive the patch and the activity is still pending. This can be good start if we want to achieve it.

Thanks,

Tanmay


>
> Thanks,
>
> On 9/28/23 5:10 PM, Tanmay Shah wrote:
>
> Hello,
>
> Thanks for your patch.
>
> Instead of having this in Kconfig, It's better to have buffer size decided dynamically. Probably by resource-table.
>
> We still need implementation that achieves that goal. Meanwhile  I think it's best to keep buffer size fixed.
>
> Thanks.
>
> On 9/28/23 10:38 AM, Divin Raj wrote:
>
>
>
> CAUTION: This message has originated from an External Source. Please use proper judgment and caution when opening attachments, clicking links, or responding to this email.
>
>
> From: Peter Hoyes <Peter.Hoyes@arm.com><mailto:Peter.Hoyes@arm.com>
>
> Replace the MAX_RPMSG_BUF_SIZE and MAX_RPMSG_NUM_BUFS #define in
> virtio_rpmsg_bus.c with the Kconfig parameters CONFIG_RPMSG_VIRTIO_BUF_SIZE
> and CONFIG_RPMSG_VIRTIO_MAX_NUM_BUFS, allowing user-provided customization.
>
> Making both the number of buffers and size configurable facilitates aligning
> memory requirements between vdev-buffer and vdev-vrings for client drivers
> that require larger buffer sizes, for example.
>
> Signed-off-by: Peter Hoyes <Peter.Hoyes@arm.com><mailto:Peter.Hoyes@arm.com>
> Signed-off-by: Divin Raj <divin.raj@arm.com><mailto:divin.raj@arm.com>
> ---
>  drivers/rpmsg/Kconfig            | 23 +++++++++++++++++++++++
>  drivers/rpmsg/virtio_rpmsg_bus.c | 27 +++------------------------
>  2 files changed, 26 insertions(+), 24 deletions(-)
>
> diff --git a/drivers/rpmsg/Kconfig b/drivers/rpmsg/Kconfig
> index d3795860f5c0..677f4a1ac8bb 100644
> --- a/drivers/rpmsg/Kconfig
> +++ b/drivers/rpmsg/Kconfig
> @@ -81,4 +81,27 @@ config RPMSG_VIRTIO
>         select RPMSG_NS
>         select VIRTIO
>
> +config RPMSG_VIRTIO_MAX_BUF_SIZE
> +       int "Virtio RPMSG max buffer size (in bytes)"
> +       default 512
> +       depends on RPMSG_VIRTIO
> +       help
> +         This option allows you to configure the maximum buffer size (in bytes)
> +         for Virtio RPMSG communications. The number of buffers will be computed
> +         based on the number of buffers (CONFIG_RPMSG_VIRTIO_MAX_NUM_BUFS)
> +         supported by the vring. By default, it supports up to a maximum of 512
> +         buffers (256 in each direction). Each buffer consists of 16 bytes for the
> +         message header and the remaining bytes for the payload.The default values
> +         will utilize a maximum total space of 256KB for the buffers.
> +
> +config RPMSG_VIRTIO_MAX_NUM_BUFS
> +       int "Virtio RPMSG max buffer count (even number for TX and Rx)"
> +       default 512
> +       depends on RPMSG_VIRTIO
> +       help
> +         This option allows you to configure the maximum number of buffers used
> +         for Virtio RPMSG communication. By default, it supports up to a maximum
> +         of 512 buffers (256 in each direction). Please note that this value
> +         should be an even number, as it accounts for both transmit (TX) and
> +         receive (Rx) buffers.
>  endmenu
> diff --git a/drivers/rpmsg/virtio_rpmsg_bus.c b/drivers/rpmsg/virtio_rpmsg_bus.c
> index 905ac7910c98..87a9a4fa30e0 100644
> --- a/drivers/rpmsg/virtio_rpmsg_bus.c
> +++ b/drivers/rpmsg/virtio_rpmsg_bus.c
> @@ -109,27 +109,6 @@ struct virtio_rpmsg_channel {
>  #define to_virtio_rpmsg_channel(_rpdev) \
>         container_of(_rpdev, struct virtio_rpmsg_channel, rpdev)
>
> -/*
> - * We're allocating buffers of 512 bytes each for communications. The
> - * number of buffers will be computed from the number of buffers supported
> - * by the vring, upto a maximum of 512 buffers (256 in each direction).
> - *
> - * Each buffer will have 16 bytes for the msg header and 496 bytes for
> - * the payload.
> - *
> - * This will utilize a maximum total space of 256KB for the buffers.
> - *
> - * We might also want to add support for user-provided buffers in time.
> - * This will allow bigger buffer size flexibility, and can also be used
> - * to achieve zero-copy messaging.
> - *
> - * Note that these numbers are purely a decision of this driver - we
> - * can change this without changing anything in the firmware of the remote
> - * processor.
> - */
> -#define MAX_RPMSG_NUM_BUFS     (512)
> -#define MAX_RPMSG_BUF_SIZE     (512)
> -
>  /*
>   * Local addresses are dynamically allocated on-demand.
>   * We do not dynamically assign addresses from the low 1024 range,
> @@ -902,12 +881,12 @@ static int rpmsg_probe(struct virtio_device *vdev)
>                 virtqueue_get_vring_size(vrp->svq));
>
>         /* we need less buffers if vrings are small */
> -       if (virtqueue_get_vring_size(vrp->rvq) < MAX_RPMSG_NUM_BUFS / 2)
> +       if (virtqueue_get_vring_size(vrp->rvq) < CONFIG_RPMSG_VIRTIO_MAX_NUM_BUFS / 2)
>                 vrp->num_bufs = virtqueue_get_vring_size(vrp->rvq) * 2;
>         else
> -               vrp->num_bufs = MAX_RPMSG_NUM_BUFS;
> +               vrp->num_bufs = CONFIG_RPMSG_VIRTIO_MAX_NUM_BUFS;
>
> -       vrp->buf_size = MAX_RPMSG_BUF_SIZE;
> +       vrp->buf_size = CONFIG_RPMSG_VIRTIO_MAX_BUF_SIZE;
>
>         total_buf_space = vrp->num_bufs * vrp->buf_size;
>
> --
> 2.25.1
>
>
>
> IMPORTANT NOTICE: The contents of this email and any attachments are confidential and may also be privileged. If you are not the intended recipient, please notify the sender immediately and do not disclose the contents to any other person, use it for any purpose, or store or copy the information in any medium. Thank you.
>