Re: [PATCH v2 1/3] rpmsg: virtio_rpmsg_bus: allow different size of tx and rx bufs

[Shah, Tanmay]

Hello,

Thanks for the reviews.

On 5/19/2026 11:57 AM, Mathieu Poirier wrote:
> Hi Tanmay
> 
> (Apologies for the late review)
> 

No problem.

> On Wed, Apr 29, 2026 at 09:10:51AM -0700, Tanmay Shah wrote:
>> Current design allocates memory for tx and rx buffers equally. The
>> throughput can be increased if the user is allowed to configure number
>> of tx and rx buffers as required. Hence, do not split number of tx & rx
>> buffers into half, but decide based on respective vring size.
>>
>> Signed-off-by: Tanmay Shah <tanmay.shah@xxxxxxx>
>> ---
>>
>> Test performed:
>>   - Test this patch with existing firmware as it is, rpmsg working.
>>
>> Changes in v2:
>>   - Change author
>>   - fix commit message with better explanation
>>   - %s/sbuf/tx_buf
>>   - %s/rbuf/rx_buf
>>   - %s/num_rbuf/num_rx_buf/
>>   - %s/num_sbuf/num_tx_buf/
> 
> Please split this patch in two parts - one to do the refactoring of the
> tx/rx_buf and another one for the varying size.

Ack.

> 
>>
>>  drivers/rpmsg/virtio_rpmsg_bus.c | 68 ++++++++++++++++----------------
>>  1 file changed, 34 insertions(+), 34 deletions(-)
>>
>> diff --git a/drivers/rpmsg/virtio_rpmsg_bus.c b/drivers/rpmsg/virtio_rpmsg_bus.c
>> index 5ae15111fb4f..e59d8cf9b975 100644
>> --- a/drivers/rpmsg/virtio_rpmsg_bus.c
>> +++ b/drivers/rpmsg/virtio_rpmsg_bus.c
>> @@ -35,13 +35,14 @@
>>   * @vdev:	the virtio device
>>   * @rvq:	rx virtqueue
>>   * @svq:	tx virtqueue
>> - * @rbufs:	kernel address of rx buffers
>> - * @sbufs:	kernel address of tx buffers
>> - * @num_bufs:	total number of buffers for rx and tx
>> - * @buf_size:   size of one rx or tx buffer
>> + * @rx_bufs:	kernel address of rx buffers
>> + * @tx_bufs:	kernel address of tx buffers
>> + * @num_rx_buf:	total number of buffers for rx
>> + * @num_tx_buf:	total number of buffers for tx
>> + * @buf_size:	size of one rx or tx buffer
>>   * @last_sbuf:	index of last tx buffer used
>>   * @bufs_dma:	dma base addr of the buffers
>> - * @tx_lock:	protects svq and sbufs, to allow concurrent senders.
>> + * @tx_lock:	protects svq and tx_bufs, to allow concurrent senders.
>>   *		sending a message might require waking up a dozing remote
>>   *		processor, which involves sleeping, hence the mutex.
>>   * @endpoints:	idr of local endpoints, allows fast retrieval
>> @@ -55,8 +56,9 @@
>>  struct virtproc_info {
>>  	struct virtio_device *vdev;
>>  	struct virtqueue *rvq, *svq;
>> -	void *rbufs, *sbufs;
>> -	unsigned int num_bufs;
>> +	void *rx_bufs, *tx_bufs;
>> +	unsigned int num_rx_buf;
>> +	unsigned int num_tx_buf;
>>  	unsigned int buf_size;
>>  	int last_sbuf;
>>  	dma_addr_t bufs_dma;
>> @@ -110,7 +112,7 @@ struct virtio_rpmsg_channel {
>>  /*
>>   * 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).
>> + * by the vring, up to a maximum of 256 in each direction.
>>   *
>>   * Each buffer will have 16 bytes for the msg header and 496 bytes for
>>   * the payload.
>> @@ -125,7 +127,7 @@ struct virtio_rpmsg_channel {
>>   * can change this without changing anything in the firmware of the remote
>>   * processor.
>>   */
>> -#define MAX_RPMSG_NUM_BUFS	(512)
>> +#define MAX_RPMSG_NUM_BUFS	(256)
>>  #define MAX_RPMSG_BUF_SIZE	(512)
>>  
>>  /*
>> @@ -440,12 +442,9 @@ static void *get_a_tx_buf(struct virtproc_info *vrp)
>>  
>>  	mutex_lock(&vrp->tx_lock);
>>  
>> -	/*
>> -	 * either pick the next unused tx buffer
>> -	 * (half of our buffers are used for sending messages)
>> -	 */
>> -	if (vrp->last_sbuf < vrp->num_bufs / 2)
>> -		ret = vrp->sbufs + vrp->buf_size * vrp->last_sbuf++;
>> +	/* either pick the next unused tx buffer */
>> +	if (vrp->last_sbuf < vrp->num_tx_buf)
>> +		ret = vrp->tx_bufs + vrp->buf_size * vrp->last_sbuf++;
>>  	/* or recycle a used one */
>>  	else
>>  		ret = virtqueue_get_buf(vrp->svq, &len);
>> @@ -631,11 +630,10 @@ static __poll_t virtio_rpmsg_poll(struct rpmsg_endpoint *ept, struct file *filp,
>>  
>>  	/*
>>  	 * check for a free buffer, either:
>> -	 * - we haven't used all of the available transmit buffers (half of the
>> -	 *   allocated buffers are used for transmit, hence num_bufs / 2), or,
>> +	 * - we haven't used all of the available transmit buffers or,
>>  	 * - we ask the virtqueue if there's a buffer available
>>  	 */
>> -	if (vrp->last_sbuf < vrp->num_bufs / 2 ||
>> +	if (vrp->last_sbuf < vrp->num_tx_buf ||
>>  	    !virtqueue_enable_cb(vrp->svq))
>>  		mask |= EPOLLOUT;
>>  
>> @@ -846,19 +844,20 @@ static int rpmsg_probe(struct virtio_device *vdev)
>>  	vrp->rvq = vqs[0];
>>  	vrp->svq = vqs[1];
>>  
>> -	/* we expect symmetric tx/rx vrings */
>> -	WARN_ON(virtqueue_get_vring_size(vrp->rvq) !=
>> -		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)
>> -		vrp->num_bufs = virtqueue_get_vring_size(vrp->rvq) * 2;
>> +	if (virtqueue_get_vring_size(vrp->rvq) < MAX_RPMSG_NUM_BUFS)
>> +		vrp->num_rx_buf = virtqueue_get_vring_size(vrp->rvq);
>> +	else
>> +		vrp->num_rx_buf = MAX_RPMSG_NUM_BUFS;
>> +
>> +	if (virtqueue_get_vring_size(vrp->svq) < MAX_RPMSG_NUM_BUFS)
>> +		vrp->num_tx_buf = virtqueue_get_vring_size(vrp->svq);
>>  	else
>> -		vrp->num_bufs = MAX_RPMSG_NUM_BUFS;
>> +		vrp->num_tx_buf = MAX_RPMSG_NUM_BUFS;
>>  
>>  	vrp->buf_size = MAX_RPMSG_BUF_SIZE;
>>  
>> -	total_buf_space = vrp->num_bufs * vrp->buf_size;
>> +	total_buf_space = (vrp->num_rx_buf + vrp->num_tx_buf) * vrp->buf_size;
>>  
>>  	/* allocate coherent memory for the buffers */
>>  	bufs_va = dma_alloc_coherent(vdev->dev.parent,
>> @@ -872,16 +871,16 @@ static int rpmsg_probe(struct virtio_device *vdev)
>>  	dev_dbg(&vdev->dev, "buffers: va %p, dma %pad\n",
>>  		bufs_va, &vrp->bufs_dma);
>>  
>> -	/* half of the buffers is dedicated for RX */
>> -	vrp->rbufs = bufs_va;
>> +	/* first part of the buffers is dedicated for RX */
>> +	vrp->rx_bufs = bufs_va;
>>  
>> -	/* and half is dedicated for TX */
>> -	vrp->sbufs = bufs_va + total_buf_space / 2;
>> +	/* and second part is dedicated for TX */
>> +	vrp->tx_bufs = bufs_va + vrp->num_rx_buf * vrp->buf_size;
>>  
>>  	/* set up the receive buffers */
>> -	for (i = 0; i < vrp->num_bufs / 2; i++) {
>> +	for (i = 0; i < vrp->num_rx_buf; i++) {
>>  		struct scatterlist sg;
>> -		void *cpu_addr = vrp->rbufs + i * vrp->buf_size;
>> +		void *cpu_addr = vrp->rx_bufs + i * vrp->buf_size;
>>  
>>  		rpmsg_sg_init(&sg, cpu_addr, vrp->buf_size);
>>  
>> @@ -966,7 +965,8 @@ static int rpmsg_remove_device(struct device *dev, void *data)
>>  static void rpmsg_remove(struct virtio_device *vdev)
>>  {
>>  	struct virtproc_info *vrp = vdev->priv;
>> -	size_t total_buf_space = vrp->num_bufs * vrp->buf_size;
>> +	unsigned int num_bufs = vrp->num_rx_buf + vrp->num_tx_buf;
>> +	size_t total_buf_space = num_bufs * vrp->buf_size;
>>  	int ret;
>>  
>>  	virtio_reset_device(vdev);
>> @@ -980,7 +980,7 @@ static void rpmsg_remove(struct virtio_device *vdev)
>>  	vdev->config->del_vqs(vrp->vdev);
>>  
>>  	dma_free_coherent(vdev->dev.parent, total_buf_space,
>> -			  vrp->rbufs, vrp->bufs_dma);
>> +			  vrp->rx_bufs, vrp->bufs_dma);
>>  
>>  	kfree(vrp);
>>  }
>> -- 
>> 2.34.1
>>