On Thu, Mar 28, 2024 at 4:16 PM Xuan Zhuo <xuanzhuo@xxxxxxxxxxxxxxxxx> wrote: > > On Thu, 28 Mar 2024 16:07:14 +0800, Jason Wang <jasowang@xxxxxxxxxx> wrote: > > On Thu, Mar 28, 2024 at 3:32 PM Xuan Zhuo <xuanzhuo@xxxxxxxxxxxxxxxxx> wrote: > > > > > > On Thu, 28 Mar 2024 14:56:47 +0800, Jason Wang <jasowang@xxxxxxxxxx> wrote: > > > > On Wed, Mar 27, 2024 at 7:14 PM Xuan Zhuo <xuanzhuo@xxxxxxxxxxxxxxxxx> wrote: > > > > > > > > > > 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. > > > > > > > > > > > > > > > > > > > > Signed-off-by: Xuan Zhuo <xuanzhuo@xxxxxxxxxxxxxxxxx> > > > > > > > > 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
