rpmsg: Allow to send virtqueue_kick only when RX queue is empty

[wyr-7]

Add VQ_RX_EMPTY_NOTIFY config to define the behavior. If VQ_RX_EMPTY_NOTIFY is disabled, notify the other side each time a buffer is released. If VQ_RX_EMPTY_NOTIFY is enabled, only send one notification when the RX queue is empty to improve performance.

[CV-Bowen]

@arnopo Hi, about your concerns:

• If the remote side is waiting for a free buffer, it has to wait until all buffers are processed. This is not compatible with some real-time use cases, such as audio streaming.

The rpmsg virtio default implementation in OpenAMP use polling method to wait the tx buffer, so in this case, the real-time performance is not affected and the number of interrupts are reduced.

But if we implemented the notify_wait_cb in struct rpmsg_virtio_device and make the tx buffer wait method from polling mode to event mode, things would be different and the real-time performance is indeed affected (Only when the buffers is not enough), and also the interrupts number are increased.

Is the default behavior more important?

• my main concern: It seems that your proposal is not compliant with the VIRTIO protocol, so it cannot be the default behavior.

As you said, this behavior indeed does not conform to the VIRTIO standard, the virtio standard says:

• VirtIO Device --> VirtIO Driver: the device side must send a notification if writes a descriptor index into the used ring (see 2.7.7.2 Device Requirements: Used Buffer Notification Suppression)
• VirtIO Driver --> VirtIO Device: the driver side must send a notification if writes a descriptor index into the available ring (see 2.7.10.1 Driver Requirements: Available Buffer Notification Suppression).

But actually, Linux does the same thing like this PR, please see: https://github.com/torvalds/linux/blob/master/drivers/rpmsg/virtio_rpmsg_bus.c#L778

static void rpmsg_recv_done(struct virtqueue *rvq)
{
	struct virtproc_info *vrp = rvq->vdev->priv;
	struct device *dev = &rvq->vdev->dev;
	struct rpmsg_hdr *msg;
	unsigned int len, msgs_received = 0;
	int err;

	msg = virtqueue_get_buf(rvq, &len);
	if (!msg) {
		dev_err(dev, "uhm, incoming signal, but no used buffer ?\n");
		return;
	}

	while (msg) {
		err = rpmsg_recv_single(vrp, dev, msg, len);
		if (err)
			break;

		msgs_received++;

		msg = virtqueue_get_buf(rvq, &len);
	}

	dev_dbg(dev, "Received %u messages\n", msgs_received);

	/* tell the remote processor we added another available rx buffer */
	if (msgs_received)
		virtqueue_kick(vrp->rvq);
}

[arnopo]

@arnopo Hi, about your concerns:

• If the remote side is waiting for a free buffer, it has to wait until all buffers are processed. This is not compatible with some real-time use cases, such as audio streaming.

The rpmsg virtio default implementation in OpenAMP use polling method to wait the tx buffer, so in this case, the real-time performance is not affected and the number of interrupts are reduced.

But if we implemented the notify_wait_cb in struct rpmsg_virtio_device and make the tx buffer wait method from polling mode to event mode, things would be different and the real-time performance is indeed affected (Only when the buffers is not enough), and also the interrupts number are increased.

Is the default behavior more important?

Yes, it is, as it can affect existing projects. If we want to maintain the stability of the library, we have to take legacy into account.

• my main concern: It seems that your proposal is not compliant with the VIRTIO protocol, so it cannot be the default behavior.

As you said, this behavior indeed does not conform to the VIRTIO standard, the virtio standard says:

• VirtIO Device --> VirtIO Driver: the device side must send a notification if writes a descriptor index into the used ring (see 2.7.7.2 Device Requirements: Used Buffer Notification Suppression)
• VirtIO Driver --> VirtIO Device: the driver side must send a notification if writes a descriptor index into the available ring (see 2.7.10.1 Driver Requirements: Available Buffer Notification Suppression).

But actually, Linux does the same thing like this PR, please see: https://github.com/torvalds/linux/blob/master/drivers/rpmsg/virtio_rpmsg_bus.c#L778

static void rpmsg_recv_done(struct virtqueue *rvq)
{
	struct virtproc_info *vrp = rvq->vdev->priv;
	struct device *dev = &rvq->vdev->dev;
	struct rpmsg_hdr *msg;
	unsigned int len, msgs_received = 0;
	int err;

	msg = virtqueue_get_buf(rvq, &len);
	if (!msg) {
		dev_err(dev, "uhm, incoming signal, but no used buffer ?\n");
		return;
	}

	while (msg) {
		err = rpmsg_recv_single(vrp, dev, msg, len);
		if (err)
			break;

		msgs_received++;

		msg = virtqueue_get_buf(rvq, &len);
	}

	dev_dbg(dev, "Received %u messages\n", msgs_received);

	/* tell the remote processor we added another available rx buffer */
	if (msgs_received)
		virtqueue_kick(vrp->rvq);
}

rproc virtio/rpmsg virtio in Linux is probably not the best example in terms of virtio specification conformance. 😃

That said, your proposal is still valid in terms of optimization. However, I would be in favor of supporting both, even if it would add a new build configuration to test.

@edmooring @tnmysh, any opinions on that?

[CV-Bowen]

rproc virtio/rpmsg virtio in Linux is probably not the best example in terms of virtio specification conformance.

Yes, kick only one time when we need add serveral buffers to the virtqueue is a very easy way to optimize the performance and I found more virtio drivers' implementations in linux do this thing:

virtio_net: https://github.com/torvalds/linux/blob/master/drivers/net/virtio_net.c#L3053
vsock_virtio: https://github.com/torvalds/linux/blob/master/net/vmw_vsock/virtio_transport.c#L155

@arnopo So what's your plan about this? If it's OK, you could give us some advices so that we can continue this PR.

CC @edmooring @tnmysh

[edmooring]

I agree with @arnopo at least in part. If we do support this, it should be in addition to the current approach, and it should not be the default. Current users of the library will be surprised if the default behavior changes.

This might be an optimization for performance in some applications, but it might also be a pessimization (the opposite of optimization) for others because it could affect latency.

[arnopo]

rproc virtio/rpmsg virtio in Linux is probably not the best example in terms of virtio specification conformance.

Yes, kick only one time when we need add serveral buffers to the virtqueue is a very easy way to optimize the performance and I found more virtio drivers' implementations in linux do this thing:

virtio_net: https://github.com/torvalds/linux/blob/master/drivers/net/virtio_net.c#L3053 vsock_virtio: https://github.com/torvalds/linux/blob/master/net/vmw_vsock/virtio_transport.c#L155

@arnopo So what's your plan about this? If it's OK, you could give us some advices so that we can continue this PR.

CC @edmooring @tnmysh

Concerning the implementation, one way to do it could be to define an RPMSG_VIRTIO_ENABLED macro similar to VIRTIO_ENABLED and a VQ_RX_EMPTY_NOTIFY build config. Then, use if (RPMSG_VIRTIO_ENABLED(VQ_RX_EMPTY_NOTIFY)) to define the behavior.

if (RPMSG_VIRTIO_ENABLED(VQ_RX_EMPTY_NOTIFY))
   /* only notify when no more Message in queue */
else
  /*  notify for each buffer released */

[CV-Bowen]

@arnopo @edmooring Thanks for the reply and we will update this PR based on your suggestions @arnopo.

[wyr-7]

Concerning the implementation, one way to do it could be to define an RPMSG_VIRTIO_ENABLED macro similar to VIRTIO_ENABLED and a VQ_RX_EMPTY_NOTIFY build config. Then, use if (RPMSG_VIRTIO_ENABLED(VQ_RX_EMPTY_NOTIFY)) to define the behavior.

if (RPMSG_VIRTIO_ENABLED(VQ_RX_EMPTY_NOTIFY))
   /* only notify when no more Message in queue */
else
  /*  notify for each buffer released */

@arnopo rpmsg_virtio.c already have use VIRTIO_ENABLED，so can we use VIRTIO_ENABLED directly?

[arnopo]

implementation seems LGTM, few typos to fix.

Please also fix commit message, here is a proposal:

rpmsg: Allow to send virtqueue_kick only when RX queue is empty

Add VQ_RX_EMPTY_NOTIFY config to define the behavior. If
VQ_RX_EMPTY_NOTIFY is disabled, notify the other side each
time a buffer is released. If VQ_RX_EMPTY_NOTIFY is enabled,
only send one notification when the RX queue is empty to
improve performance.

[wyr-7]

impementation seemms LGTM, few typos to fix.

Please also fix commit message, here is a proposal:

rpmsg: Allow to send virtqueue_kick only when RX queue is empty

Add VQ_RX_EMPTY_NOTIFY config to define the behavior. If VQ_RX_EMPTY_NOTIFY is disabled, notify the other side each time a buffer is released. If VQ_RX_EMPTY_NOTIFY is enabled, only send one notification when the RX queue is empty to improve performance.

Thanks for pointing out the grammatical error, done.

[edmooring]

Looks good to go.