On Tue, Jan 16, 2024 at 3:59 PM Xuan Zhuo [off-list ref] wrote:

introduce virtqueue_get_buf_ctx_dma() to collect the dma info when
get buf from virtio core for premapped mode.

If the virtio queue is premapped mode, the virtio-net send buf may
have many desc.

This feature is not specific to virtio-net, so we can let "for example ..."

Every desc dma address need to be unmap. So here we
introduce a new helper to collect the dma address of the buffer from
the virtio core.

Let's explain why we can't (or suboptimal to) depend on a driver to do this.

So the code has a check for dma:

        if (!dma)
                return false;

For whatever the case, we need a better comment here.

So we had: use_dma_api, premapped, do_unmap and dma now. I must say
I'm totally lost in this maze. It's a strong hint the API is too
complicated and needs to be tweaked.

For example, is it legal to have do_unmap be false buf DMA is true?

Here're suggestions:

1) rename premapped to buffer_is_premapped
2) rename do_unmap to buffer_need_unmap or introduce a helper

bool vring_need_unmap_buffer()
{
        return use_dma_api && !premapped;
}

3) split the getting dma info logic into an helper like vritqueue_get_dma_info()

so we can do

if (!vring_need_unmap_buffer()) {
        virtqueue_get_dma_info()
        return;
}

4) explain why we still need to check dma assuming we had
vring_need_unmap_buffer():

If vring_need_unmap_buffer() is true, we don't need to care about dma at all.

If vring_need_unmap_buffer() is false, we must return dma info
otherwise there's a leak?

+ *
+ * Return:
+ * true: the virtio core must unmap the desc
+ * false: the virtio core skip the desc unmap

Might it be better to say "It's up to the driver to unmap"?

+ */
+static bool vring_need_unmap(struct vring_virtqueue *vq,
+                            struct virtio_dma_head *dma,
+                            dma_addr_t addr, unsigned int length)
+{
+       if (vq->do_unmap)
+               return true;
+
+       if (!vq->premapped)
+               return false;
+
+       if (!dma)
+               return false;

So the logic here is odd.

if (!dma)
        return false;
...
        return false;

A strong hint to split the below getting info logic into another
helper. The root cause is the function do more than just a "yes or
no".

+
+       if (unlikely(dma->next >= dma->num)) {
+               BAD_RING(vq, "premapped vq: collect dma overflow: %pad %u\n",
+                        &addr, length);
+               return false;
+       }
+
+       dma->items[dma->next].addr = addr;
+       dma->items[dma->next].length = length;
+
+       ++dma->next;
+
+       return false;
+}
+

Thanks

....