On Thu, Mar 28, 2024 at 4:16 PM Xuan Zhuo [off-list ref] wrote:

On Thu, 28 Mar 2024 16:07:14 +0800, Jason Wang [off-list ref] wrote:

quoted

On Thu, Mar 28, 2024 at 3:32 PM Xuan Zhuo [off-list ref] wrote:

quoted

On Thu, 28 Mar 2024 14:56:47 +0800, Jason Wang [off-list ref] wrote:

quoted

On Wed, Mar 27, 2024 at 7:14 PM Xuan Zhuo [off-list ref] wrote:

quoted

In the functions vring_unmap_extra_packed and vring_unmap_desc_packed,
multiple checks are made whether unmap is performed and whether it is
INDIRECT.

These two functions are usually called in a loop, and we should put the
check outside the loop.

And we unmap the descs with VRING_DESC_F_INDIRECT on the same path with
other descs, that make the thing more complex. If we distinguish the
descs with VRING_DESC_F_INDIRECT before unmap, thing will be clearer.

For desc with VRING_DESC_F_INDIRECT flag:
1. only one desc of the desc table is used, we do not need the loop
    Theoretically, indirect descriptors could be chained.
    But now, that is not supported by "add", so we ignore this case.
2. the called unmap api is difference from the other desc
3. the vq->premapped is not needed to check
4. the vq->indirect is not needed to check
5. the state->indir_desc must not be null

It doesn't explain the connection to the goal of this series. If it's
not a must I'd suggest moving it to a separate patch.

The "no store dma ..." depends this.

I will add this message in next version.

quoted

quoted

Signed-off-by: Xuan Zhuo <xuanzhuo@linux.alibaba.com>

Rethink this, it looks to me it would complicate the codes furtherly.

For example, vring_map_xxx() helpers will check premappred and
use_dma_api by itself. But in the case of vring_unmap() you want to
move those checks to the caller. This will result in tricky codes that
are hard to understand.

We need to be consistent here.

If we try to optimize unmap we need to optimize map as well. But
generally it would complicate the logic of the caller if we want to
let the caller to differ. Ideally, the caller of those function should
know nothing about use_dma_api, premapped and other.

The key is that we can check "use_dma_api, premapped" to skip the loop.
If the vring_unmap_xxx is called, the "use_dma_api, premapped" is checked in
advance, so that is a waste to check thest again.

Right, but we have the same logic for map.

But we can not skip the loop for map.

Ok, right. So I'm fine to leave it as is. We can optimize the checking
on top anyhow.

Thanks