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@xxxxxxx>
> Reviewed-by: jin.qi <jin.qi@xxxxxxxxxx>
> Tested-by: jin.qi <jin.qi@xxxxxxxxxx>
> Cc: ni.liqiang <ni.liqiang@xxxxxxxxxx>
> ---
> 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
