Re: [PATCH v5 29/29] packet: use buf to store iovec array

[Stefano Brivio <sbrivio@redhat.com>]

On Thu, 17 Apr 2025 18:51:36 +0200
Laurent Vivier <lvivier@redhat.com> wrote:

> When we use vhost-user we don't use the memory buffer
> of the pool to store the packet, so we can use it to
> store iovec array that points to the memory provided
> by vhost-user.
> 
> Signed-off-by: Laurent Vivier <lvivier@redhat.com>
> ---
>  packet.c | 163 +++++++++++++++++++++++++++++++++++++++++++++++++------
>  1 file changed, 147 insertions(+), 16 deletions(-)
> 
> diff --git a/packet.c b/packet.c
> index 0a2ad28d3d25..ccd98ff3aa36 100644
> --- a/packet.c
> +++ b/packet.c
> @@ -84,6 +84,121 @@ bool pool_full(const struct pool *p)
>  	return p->count >= p->size;
>  }
>  
> +/**
> + * packet_iov_max_cnt() - Return the maximum number of iovec we can store

I'd say "iovecs" (same just below in this same comment).

> + * @p:		Pointer to packet pool
> + *
> + * Return: the maximum number of iovec we can store in the memory of
> + *         the pool buffer
> + */
> +static size_t packet_iov_max_cnt(const struct pool *p)
> +{
> +	return p->buf_size / sizeof(struct iovec);
> +}
> +
> +/**
> + * packet_iov_idx() - For a given packet index, return the iovec index and
> + * 		      the number of iovec entry of the packet
> + * @p:		Pointer to packet pool
> + * @idx:	Index of packet descriptor in pool
> + * @iov_cnt:	Pointer to store the number of the iovec entry of the packet
> + * @func:	For tracing: name of calling function
> + * @line:	For tracing: caller line of function call
> + *
> + * Return: the iovec index for the given packet index, @iov_cnt is set
> + *         to the number of the iovec entry of the packet
> + */
> +static int packet_iov_idx(const struct pool *p, size_t idx, size_t *iov_cnt,
> +			  const char *func, int line)
> +{
> +	size_t iov_idx, max = packet_iov_max_cnt(p);
> +
> +	iov_idx = (size_t)p->pkt[idx].iov_base;
> +	*iov_cnt = p->pkt[idx].iov_len;
> +
> +	ASSERT_WITH_MSG(iov_idx + *iov_cnt <= max,
> +		       "Corrupt iov entry: (%zu, %zu), max: %zu, %s:%i",
> +			iov_idx, *iov_cnt, max, func, line);
> +
> +	return iov_idx;
> +}
> +
> +/**
> + * packet_iov_next_idx() - Give the the next available iovec index
> + * @p:		Pointer to packet pool
> + * @idx:	Index of packet descriptor in pool
> + * @func:	For tracing: name of calling function
> + * @line:	For tracing: caller line of function call
> + *
> + * Return: the next available iovec index
> + */
> +static size_t packet_iov_next_idx(const struct pool *p, size_t idx,
> +				  const char *func, int line)
> +{
> +	size_t iov_idx, iov_cnt;
> +
> +	if (idx == 0)
> +		return 0;

I'm a bit lost here. Why is 0 a special value now?

> +
> +	iov_idx = packet_iov_idx(p, idx - 1, &iov_cnt, func, line);
> +
> +	return iov_idx + iov_cnt;
> +}
> +
> +/**
> + * packet_iov_data() - For a given packet index, provide the iovec array
> + * @p:		Pointer to packet pool
> + * @idx:	Index of packet descriptor in pool
> + * @data:	iov_tail to store iovec array and count
> + * 		(offset is always set to 0)
> + * @func:	For tracing: name of calling function
> + * @line:	For tracing: caller line of function call
> + */
> +static void packet_iov_data(const struct pool *p, size_t idx,
> +			    struct iov_tail *data,
> +			    const char *func, int line)
> +{
> +	struct iovec *iov = (struct iovec *)p->buf;
> +	size_t iov_idx, iov_cnt;
> +
> +	iov_idx = packet_iov_idx(p, idx, &iov_cnt, func, line);
> +
> +	data->iov = &iov[iov_idx];
> +	data->cnt = iov_cnt;
> +	data->off = 0;
> +}
> +
> +/**
> + * packet_iov_check_range() - Check if iovec array is valid for a pool
> + * @p:		Pointer to packet pool
> + * @data:	iov_tail that stores the iovec array to check
> + * @func:	For tracing: name of calling function
> + * @line:	For tracing: caller line of function call
> + *
> + * Return: 0 if the range is valid, -1 otherwise
> + */
> +static int packet_iov_check_range(const struct pool *p,
> +				  const struct iov_tail *data,
> +				  const char *func, int line)
> +{
> +	size_t offset, i;
> +
> +	offset = data->off;
> +	for (i = 0; i < data->cnt; i++) {
> +		int ret;
> +
> +		ret = packet_check_range(p,
> +				(char *)data->iov[i].iov_base + offset,
> +				data->iov[i].iov_len - offset,
> +				func, line);

Nit: the usual argument alignment would be nice to have.

-- 
Stefano