Re: [PATCH v4 3/4] vhost-user: introduce vhost-user API

[Stefano Brivio <sbrivio@redhat.com>]

On Thu, 12 Sep 2024 14:41:53 +0200
Laurent Vivier <lvivier@redhat.com> wrote:

> On 10/09/2024 17:47, Stefano Brivio wrote:
> > Nits and a couple of questions only:
> > 
> > On Fri,  6 Sep 2024 18:04:48 +0200
> > Laurent Vivier <lvivier@redhat.com> wrote:
> >   
> >> Add vhost_user.c and vhost_user.h that define the functions needed
> >> to implement vhost-user backend.
> >>
> >> Signed-off-by: Laurent Vivier <lvivier@redhat.com>
> >> ---
> >>   Makefile     |    4 +-
> >>   iov.c        |    1 -
> >>   vhost_user.c | 1265 ++++++++++++++++++++++++++++++++++++++++++++++++++
> >>   vhost_user.h |  203 ++++++++
> >>   virtio.c     |    5 -
> >>   virtio.h     |    2 +-
> >>   6 files changed, 1471 insertions(+), 9 deletions(-)
> >>   create mode 100644 vhost_user.c
> >>   create mode 100644 vhost_user.h  
> ...
> >> diff --git a/vhost_user.c b/vhost_user.c
> >> new file mode 100644
> >> index 000000000000..6008a8adc967
> >> --- /dev/null
> >> +++ b/vhost_user.c  
> ...
> >> +/**
> >> + * vu_wait_queue() - wait for new free entries in the virtqueue
> >> + * @vq:		virtqueue to wait on
> >> + */
> >> +static int vu_wait_queue(const struct vu_virtq *vq)
> >> +{
> >> +	eventfd_t kick_data;
> >> +	ssize_t rc;
> >> +	int status;
> >> +
> >> +	/* wait for the kernel to put new entries in the queue */
> >> +	status = fcntl(vq->kick_fd, F_GETFL);
> >> +	if (status == -1)
> >> +		return -1;  
> > 
> > Same as on v3 (I see you changed this below, but not here): if you
> > don't use status later, you can omit storing it.  
> 
> We need status with F_SETFL below:

Oops, sorry, of course!

> >> +
> >> +	if (fcntl(vq->kick_fd, F_SETFL, status & ~O_NONBLOCK))
> >> +		return -1;
> >> +
> >> +	rc = eventfd_read(vq->kick_fd, &kick_data);
> >> +
> >> +	if (fcntl(vq->kick_fd, F_SETFL, status))
> >> +		return -1;
> >> +
> >> +	if (rc == -1)
> >> +		return -1;
> >> +
> >> +	return 0;
> >> +}  
> ...
> >> +/**
> >> + * vu_handle_tx() - Receive data from the TX virtqueue
> >> + * @vdev:	vhost-user device
> >> + * @index:	index of the virtqueue
> >> + * @now:	Current timestamp
> >> + */
> >> +static void vu_handle_tx(struct vu_dev *vdev, int index,
> >> +			 const struct timespec *now)
> >> +{
> >> +	struct vu_virtq_element elem[VIRTQUEUE_MAX_SIZE];
> >> +	struct iovec out_sg[VIRTQUEUE_MAX_SIZE];
> >> +	struct vu_virtq *vq = &vdev->vq[index];
> >> +	int hdrlen = vdev->hdrlen;
> >> +	int out_sg_count;
> >> +	int count;
> >> +  
> > 
> > Excess newline (same as v3).  
> 
> Done.
> 
> >   
> >> +
> >> +	if (!VHOST_USER_IS_QUEUE_TX(index)) {
> >> +		debug("vhost-user: index %d is not a TX queue", index);
> >> +		return;
> >> +	}
> >> +
> >> +	tap_flush_pools();
> >> +
> >> +	count = 0;
> >> +	out_sg_count = 0;
> >> +	while (count < VIRTQUEUE_MAX_SIZE) {  
> > 
> > So, I see that this is limited to 1024 iterations now (it was limited
> > also earlier, but I didn't realise that).
> > 
> > If we loop at most VIRTQUEUE_MAX_SIZE times, that means, I guess, that
> > while we're popping elements, the queue can't be written to, correct?  
> 
> No, I think the queue can be read and write at the same time.
> 
> > Or it can be written to, but we'll get an additional kick after
> > vu_queue_notify() if that happens?  
> 
> I could check the protocol and the code, but I think it should work like that.

Well, okay, it should be obvious enough.

-- 
Stefano