Re: [PATCH] drivers/virtio: delayed configuration descriptor flags

[Chaitanya Kulkarni <chaitanyak@nvidia.com>]

On 4/8/24 10:02, ni.liqiang wrote:
> In our testing of the virtio hardware accelerator, we found that
> configuring the flags of the descriptor after addr and len,
> as implemented in DPDK, seems to be more friendly to the hardware.
please describe in detail "friendly to the hardware" means ..

> In our Virtio hardware implementation tests, using the default
> open-source code, the hardware's bulk reads ensure performance
> but correctness is compromised. If we refer to the implementation code
> of DPDK, placing the flags configuration of the descriptor
> after addr and len, virtio backend can function properly based on
> our hardware accelerator.

you are not specifying how the correctness is compromised ..
again what the "properly" mean here ? what is the exact failure that
you are seeing ? please document ..

> I am somewhat puzzled by this. From a software process perspective,
> it seems that there should be no difference whether
> the flags configuration of the descriptor is before or after addr and len.
> However, this is not the case according to experimental test results.
> We would like to know if such a change in the configuration order
> is reasonable and acceptable?

where are the experimental results ? any particular reason those
results are not documented here ?

>
> Thanks.
>
> Signed-off-by: ni.liqiang <ni_liqiang@126.com>
> Reviewed-by: jin.qi <jin.qi@zte.com.cn>
> Tested-by: jin.qi <jin.qi@zte.com.cn>
> Cc: ni.liqiang <ni.liqiang@zte.com.cn>
> ---
>   drivers/virtio/virtio_ring.c | 9 +++++----
>   1 file changed, 5 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/virtio/virtio_ring.c b/drivers/virtio/virtio_ring.c
> index 6f7e5010a673..bea2c2fb084e 100644
> --- a/drivers/virtio/virtio_ring.c
> +++ b/drivers/virtio/virtio_ring.c
> @@ -1472,15 +1472,16 @@ static inline int virtqueue_add_packed(struct virtqueue *_vq,
>   			flags = cpu_to_le16(vq->packed.avail_used_flags |
>   				    (++c == total_sg ? 0 : VRING_DESC_F_NEXT) |
>   				    (n < out_sgs ? 0 : VRING_DESC_F_WRITE));
> -			if (i == head)
> -				head_flags = flags;
> -			else
> -				desc[i].flags = flags;
>   
>   			desc[i].addr = cpu_to_le64(addr);
>   			desc[i].len = cpu_to_le32(sg->length);
>   			desc[i].id = cpu_to_le16(id);
>   
> +			if (i == head)
> +				head_flags = flags;
> +			else
> +				desc[i].flags = flags;
> +
>   			if (unlikely(vq->use_dma_api)) {
>   				vq->packed.desc_extra[curr].addr = addr;
>   				vq->packed.desc_extra[curr].len = sg->length;

-ck