From: Xuan Zhuo <xuanzhuo@linux.alibaba.com> Date: 2024-03-12 03:36:01
As discussed:
http://lore.kernel.org/all/CACGkMEvq0No8QGC46U4mGsMtuD44fD_cfLcPaVmJ3rHYqRZxYg@mail.gmail.com
If the virtio is premapped mode, the driver should manage the dma info by self.
So the virtio core should not store the dma info. We can release the memory used
to store the dma info.
For virtio-net xmit queue, if the virtio-net maintains the dma info,
the virtio-net must allocate too much memory(19 * queue_size for per-queue), so
we do not plan to make the virtio-net to maintain the dma info by default. The
virtio-net xmit queue only maintain the dma info when premapped mode is enable
(such as AF_XDP is enable).
So this patch set try to do:
1. make the virtio core to do not store the dma info
- But if the desc_extra has not dma info, we face a new question,
it is hard to get the dma info of the desc with indirect flag.
For split mode, that is easy from desc, but for the packed mode,
it is hard to get the dma info from the desc. And hardening
the dma unmap is safe, we should store the dma info of indirect
descs when the virtio core does not store the bufer dma info.
So I introduce the "structure the indirect desc table" to
allocate space to store dma info of the desc table.
+struct vring_split_desc_indir {
+ dma_addr_t addr; /* Descriptor Array DMA addr. */
+ u32 len; /* Descriptor Array length. */
+ u32 num;
+ struct vring_desc desc[];
+};
The follow patches to this:
* virtio_ring: packed: structure the indirect desc table
* virtio_ring: split: structure the indirect desc table
- On the other side, in the umap handle, we mix the indirect descs with
other descs. That make things too complex. I found if we we distinguish
the descs with VRING_DESC_F_INDIRECT before unmap, thing will be clearer.
The follow patches do this.
* virtio_ring: packed: remove double check of the unmap ops
* virtio_ring: split: structure the indirect desc table
2. make the virtio core to enable premapped mode by find_vqs() params
- Because the find_vqs() will try to allocate memory for the dma info.
If we set the premapped mode after find_vqs() and release the
dma info, that is odd.
Please review.
Thanks
v4:
1. virtio-net xmit queue does not enable premapped mode by default
v3:
1. fix the conflict with the vp_modern_create_avq().
v2:
1. change the dma item of virtio-net, every item have MAX_SKB_FRAGS + 2 addr + len pairs.
2. introduce virtnet_sq_free_stats for __free_old_xmit
v1:
1. rename transport_vq_config to vq_transport_config
2. virtio-net set dma meta number to (ring-size + 1)(MAX_SKB_FRGAS +2)
3. introduce virtqueue_dma_map_sg_attrs
4. separate vring_create_virtqueue to an independent commit
Xuan Zhuo (10):
virtio_ring: introduce vring_need_unmap_buffer
virtio_ring: packed: remove double check of the unmap ops
virtio_ring: packed: structure the indirect desc table
virtio_ring: split: remove double check of the unmap ops
virtio_ring: split: structure the indirect desc table
virtio_ring: no store dma info when unmap is not needed
virtio: find_vqs: add new parameter premapped
virtio_ring: export premapped to driver by struct virtqueue
virtio_net: set premapped mode by find_vqs()
virtio_ring: virtqueue_set_dma_premapped support disable
drivers/net/virtio_net.c | 57 +++--
drivers/virtio/virtio_ring.c | 436 +++++++++++++++++++++-------------
include/linux/virtio.h | 3 +-
include/linux/virtio_config.h | 17 +-
4 files changed, 307 insertions(+), 206 deletions(-)
--
2.32.0.3.g01195cf9f
@@ -175,11 +175,6 @@ struct vring_virtqueue {/* Do DMA mapping by driver */boolpremapped;-/* Do unmap or not for desc. Just when premapped is False and-*use_dma_apiistrue,thisistrue.-*/-booldo_unmap;-/* Head of free buffer list. */unsignedintfree_head;/* Number we've added since last sync. */
@@ -641,7 +641,7 @@ static inline int virtqueue_add_split(struct virtqueue *_vq,}/* Last one doesn't continue. */desc[prev].flags&=cpu_to_virtio16(_vq->vdev,~VRING_DESC_F_NEXT);-if(!indirect&&vq->do_unmap)+if(!indirect&&vring_need_unmap_buffer(vq))vq->split.desc_extra[prev&(vq->split.vring.num-1)].flags&=~VRING_DESC_F_NEXT;
From: Xuan Zhuo <xuanzhuo@linux.alibaba.com> Date: 2024-03-12 03:36:03
This commit structure the indirect desc table.
Then we can get the desc num directly when doing unmap.
And save the dma info to the struct, then the indirect
will not use the dma fields of the desc_extra. The subsequent
commits will make the dma fields are optional. But for
the indirect case, we must record the dma info.
Signed-off-by: Xuan Zhuo <xuanzhuo@linux.alibaba.com>
---
drivers/virtio/virtio_ring.c | 66 +++++++++++++++++++++---------------
1 file changed, 38 insertions(+), 28 deletions(-)
@@ -72,9 +72,16 @@ struct vring_desc_state_split {structvring_desc*indir_desc;/* Indirect descriptor, if any. */};+structvring_packed_desc_indir{+dma_addr_taddr;/* Descriptor Array DMA addr. */+u32len;/* Descriptor Array length. */+u32num;+structvring_packed_descdesc[];+};+structvring_desc_state_packed{void*data;/* Data for callback. */-structvring_packed_desc*indir_desc;/* Indirect descriptor, if any. */+structvring_packed_desc_indir*indir_desc;/* Indirect descriptor, if any. */u16num;/* Descriptor list length. */u16last;/* The last desc state in a list. */};
@@ -1616,27 +1631,22 @@ static void detach_buf_packed(struct vring_virtqueue *vq,if(ctx)*ctx=state->indir_desc;}else{-conststructvring_desc_extra*extra;-u32len;+structvring_packed_desc_indir*in_desc;++in_desc=state->indir_desc;if(vq->use_dma_api){-extra=&vq->packed.desc_extra[id];dma_unmap_single(vring_dma_dev(vq),-extra->addr,extra->len,+in_desc->addr,in_desc->len,(flags&VRING_DESC_F_WRITE)?DMA_FROM_DEVICE:DMA_TO_DEVICE);}-/* Free the indirect table, if any, now that it's unmapped. */-desc=state->indir_desc;-if(vring_need_unmap_buffer(vq)){-len=vq->packed.desc_extra[id].len;-for(i=0;i<len/sizeof(structvring_packed_desc);-i++)-vring_unmap_desc_packed(vq,&desc[i]);+for(i=0;i<in_desc->num;i++)+vring_unmap_desc_packed(vq,&in_desc->desc[i]);}-kfree(desc);+kfree(in_desc);state->indir_desc=NULL;}}
From: Xuan Zhuo <xuanzhuo@linux.alibaba.com> Date: 2024-03-12 03:36:04
In the functions vring_unmap_one_split and
vring_unmap_one_split_indirect,
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.
1. only one desc of the desc table is used, we do not need the loop
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
Signed-off-by: Xuan Zhuo <xuanzhuo@linux.alibaba.com>
---
drivers/virtio/virtio_ring.c | 79 +++++++++++++++++-------------------
1 file changed, 38 insertions(+), 41 deletions(-)
@@ -658,7 +640,7 @@ static inline int virtqueue_add_split(struct virtqueue *_vq,vq,desc,total_sg*sizeof(structvring_desc),DMA_TO_DEVICE);if(vring_mapping_error(vq,addr)){-if(vq->premapped)+if(!vring_need_unmap_buffer(vq))gotofree_indirect;gotounmap_release;
@@ -711,6 +693,9 @@ static inline int virtqueue_add_split(struct virtqueue *_vq,return0;unmap_release:++WARN_ON(!vring_need_unmap_buffer(vq));+err_idx=i;if(indirect)
@@ -772,34 +757,42 @@ static void detach_buf_split(struct vring_virtqueue *vq, unsigned int head,{unsignedinti,j;__virtio16nextflag=cpu_to_virtio16(vq->vq.vdev,VRING_DESC_F_NEXT);+u16flags;/* Clear data ptr. */vq->split.desc_state[head].data=NULL;+flags=vq->split.desc_extra[head].flags;/* Put back on free list: unmap first-level descriptors and find end */i=head;-while(vq->split.vring.desc[i].flags&nextflag){-vring_unmap_one_split(vq,i);-i=vq->split.desc_extra[i].next;-vq->vq.num_free++;-}--vring_unmap_one_split(vq,i);-vq->split.desc_extra[i].next=vq->free_head;-vq->free_head=head;+if(!(flags&VRING_DESC_F_INDIRECT)){+while(vq->split.vring.desc[i].flags&nextflag){+if(vring_need_unmap_buffer(vq))+vring_unmap_one_split(vq,i);+i=vq->split.desc_extra[i].next;+vq->vq.num_free++;+}-/* Plus final descriptor */-vq->vq.num_free++;+if(vring_need_unmap_buffer(vq))+vring_unmap_one_split(vq,i);-if(vq->indirect){+if(ctx)+*ctx=vq->split.desc_state[head].indir_desc;+}else{structvring_desc*indir_desc=vq->split.desc_state[head].indir_desc;u32len;-/* Free the indirect table, if any, now that it's unmapped. */-if(!indir_desc)-return;+if(vq->use_dma_api){+structvring_desc_extra*extra=vq->split.desc_extra;++dma_unmap_single(vring_dma_dev(vq),+extra[i].addr,+extra[i].len,+(flags&VRING_DESC_F_WRITE)?+DMA_FROM_DEVICE:DMA_TO_DEVICE);+}len=vq->split.desc_extra[head].len;
@@ -814,9 +807,13 @@ static void detach_buf_split(struct vring_virtqueue *vq, unsigned int head,kfree(indir_desc);vq->split.desc_state[head].indir_desc=NULL;-}elseif(ctx){-*ctx=vq->split.desc_state[head].indir_desc;}++vq->split.desc_extra[i].next=vq->free_head;+vq->free_head=head;++/* Plus final descriptor */+vq->vq.num_free++;}staticboolmore_used_split(conststructvring_virtqueue*vq)
From: Xuan Zhuo <xuanzhuo@linux.alibaba.com> Date: 2024-03-12 03:36:06
This commit structure the indirect desc table.
Then we can get the desc num directly when doing unmap.
And save the dma info to the struct, then the indirect
will not use the dma fields of the desc_extra. The subsequent
commits will make the dma fields are optional. But for
the indirect case, we must record the dma info.
Signed-off-by: Xuan Zhuo <xuanzhuo@linux.alibaba.com>
---
drivers/virtio/virtio_ring.c | 91 ++++++++++++++++++++++--------------
1 file changed, 55 insertions(+), 36 deletions(-)
@@ -538,6 +552,7 @@ static inline int virtqueue_add_split(struct virtqueue *_vq,gfp_tgfp){structvring_virtqueue*vq=to_vvq(_vq);+structvring_split_desc_indir*in_desc;structscatterlist*sg;structvring_desc*desc;unsignedinti,n,avail,descs_used,prev,err_idx;
@@ -560,9 +575,13 @@ static inline int virtqueue_add_split(struct virtqueue *_vq,head=vq->free_head;-if(virtqueue_use_indirect(vq,total_sg))-desc=alloc_indirect_split(_vq,total_sg,gfp);-else{+if(virtqueue_use_indirect(vq,total_sg)){+in_desc=alloc_indirect_split(_vq,total_sg,gfp);+if(!in_desc)+desc=NULL;+else+desc=in_desc->desc;+}else{desc=NULL;WARN_ON_ONCE(total_sg>vq->split.vring.num&&!vq->indirect);}
@@ -635,10 +654,10 @@ static inline int virtqueue_add_split(struct virtqueue *_vq,~VRING_DESC_F_NEXT;if(indirect){+u32size=total_sg*sizeof(structvring_desc);+/* Now that the indirect table is filled in, map it. */-dma_addr_taddr=vring_map_single(-vq,desc,total_sg*sizeof(structvring_desc),-DMA_TO_DEVICE);+dma_addr_taddr=vring_map_single(vq,desc,size,DMA_TO_DEVICE);if(vring_mapping_error(vq,addr)){if(!vring_need_unmap_buffer(vq))gotofree_indirect;
@@ -646,11 +665,20 @@ static inline int virtqueue_add_split(struct virtqueue *_vq,gotounmap_release;}-virtqueue_add_desc_split(_vq,vq->split.vring.desc,-head,addr,-total_sg*sizeof(structvring_desc),-VRING_DESC_F_INDIRECT,-false);+desc=&vq->split.vring.desc[head];++desc->flags=cpu_to_virtio16(_vq->vdev,VRING_DESC_F_INDIRECT);+desc->addr=cpu_to_virtio64(_vq->vdev,addr);+desc->len=cpu_to_virtio32(_vq->vdev,size);++vq->split.desc_extra[head].flags=VRING_DESC_F_INDIRECT;++if(vq->use_dma_api){+in_desc->addr=addr;+in_desc->len=size;+}++in_desc->num=total_sg;}/* We're using some buffers from the free list. */
@@ -665,7 +693,7 @@ static inline int virtqueue_add_split(struct virtqueue *_vq,/* Store token and indirect buffer state. */vq->split.desc_state[head].data=data;if(indirect)-vq->split.desc_state[head].indir_desc=desc;+vq->split.desc_state[head].indir_desc=in_desc;elsevq->split.desc_state[head].indir_desc=ctx;
@@ -715,7 +743,7 @@ static inline int virtqueue_add_split(struct virtqueue *_vq,free_indirect:if(indirect)-kfree(desc);+kfree(in_desc);END_USE(vq);return-ENOMEM;
From: Xuan Zhuo <xuanzhuo@linux.alibaba.com> Date: 2024-03-12 03:36:07
As discussed:
http://lore.kernel.org/all/CACGkMEug-=C+VQhkMYSgUKMC==04m7-uem_yC21bgGkKZh845w@mail.gmail.com
When the vq is premapped mode, the driver manages the dma
info is a good way.
So this commit make the virtio core not to store the dma
info and release the memory which is used to store the dma
info.
If the use_dma_api is false, the memory is also not allocated.
Signed-off-by: Xuan Zhuo <xuanzhuo@linux.alibaba.com>
---
drivers/virtio/virtio_ring.c | 100 ++++++++++++++++++++++++++++++-----
1 file changed, 87 insertions(+), 13 deletions(-)
@@ -94,12 +94,15 @@ struct vring_desc_state_packed {};structvring_desc_extra{-dma_addr_taddr;/* Descriptor DMA addr. */-u32len;/* Descriptor length. */u16flags;/* Descriptor flags. */u16next;/* The next desc state in a list. */};+structvring_desc_dma{+dma_addr_taddr;/* Descriptor DMA addr. */+u32len;/* Descriptor length. */+};+structvring_virtqueue_split{/* Actual memory layout for this queue. */structvringvring;
@@ -116,6 +119,7 @@ struct vring_virtqueue_split {/* Per-descriptor state. */structvring_desc_state_split*desc_state;structvring_desc_extra*desc_extra;+structvring_desc_dma*desc_dma;/* DMA address and size information */dma_addr_tqueue_dma_addr;
@@ -156,6 +160,7 @@ struct vring_virtqueue_packed {/* Per-descriptor state. */structvring_desc_state_packed*desc_state;structvring_desc_extra*desc_extra;+structvring_desc_dma*desc_dma;/* DMA address and size information */dma_addr_tring_dma_addr;
From: Xuan Zhuo <xuanzhuo@linux.alibaba.com> Date: 2024-03-12 03:36:08
If the premapped mode is enabled, the dma array(struct vring_desc_dma) of
virtio core will not be allocated. That is judged when find_vqs() is
called. To avoid allocating dma array in find_vqs() and releasing it
immediately by virtqueue_set_dma_premapped(). This patch introduces a
new parameter to find_vqs(). Then we can judge should we allocate the
dma array(struct vring_desc_dma) or not inside find_vqs().
The driver must check the premapped mode of every vq after find_vqs().
Signed-off-by: Xuan Zhuo <xuanzhuo@linux.alibaba.com>
---
drivers/virtio/virtio_ring.c | 4 ++--
include/linux/virtio_config.h | 1 +
2 files changed, 3 insertions(+), 2 deletions(-)
From: Xuan Zhuo <xuanzhuo@linux.alibaba.com> Date: 2024-03-12 03:36:08
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.
1. only one desc of the desc table is used, we do not need the loop
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
Signed-off-by: Xuan Zhuo <xuanzhuo@linux.alibaba.com>
---
drivers/virtio/virtio_ring.c | 78 ++++++++++++++++++------------------
1 file changed, 40 insertions(+), 38 deletions(-)
@@ -1609,22 +1603,32 @@ static void detach_buf_packed(struct vring_virtqueue *vq,vq->free_head=id;vq->vq.num_free+=state->num;-if(unlikely(vq->use_dma_api)){-curr=id;-for(i=0;i<state->num;i++){-vring_unmap_extra_packed(vq,-&vq->packed.desc_extra[curr]);-curr=vq->packed.desc_extra[curr].next;+if(!(flags&VRING_DESC_F_INDIRECT)){+if(vring_need_unmap_buffer(vq)){+curr=id;+for(i=0;i<state->num;i++){+vring_unmap_extra_packed(vq,+&vq->packed.desc_extra[curr]);+curr=vq->packed.desc_extra[curr].next;+}}-}-if(vq->indirect){+if(ctx)+*ctx=state->indir_desc;+}else{+conststructvring_desc_extra*extra;u32len;+if(vq->use_dma_api){+extra=&vq->packed.desc_extra[id];+dma_unmap_single(vring_dma_dev(vq),+extra->addr,extra->len,+(flags&VRING_DESC_F_WRITE)?+DMA_FROM_DEVICE:DMA_TO_DEVICE);+}+/* Free the indirect table, if any, now that it's unmapped. */desc=state->indir_desc;-if(!desc)-return;if(vring_need_unmap_buffer(vq)){len=vq->packed.desc_extra[id].len;
From: Xuan Zhuo <xuanzhuo@linux.alibaba.com> Date: 2024-03-12 03:36:09
Export the premapped to drivers, then drivers can check
the vq premapped mode after the find_vqs().
Because the find_vqs() just try to enable the vq premapped mode,
the driver must check that after find_vqs().
Signed-off-by: Xuan Zhuo <xuanzhuo@linux.alibaba.com>
---
drivers/virtio/virtio_ring.c | 13 +++++--------
include/linux/virtio.h | 1 +
2 files changed, 6 insertions(+), 8 deletions(-)
@@ -191,9 +191,6 @@ struct vring_virtqueue {/* Host publishes avail event idx */boolevent;-/* Do DMA mapping by driver */-boolpremapped;-/* Head of free buffer list. */unsignedintfree_head;/* Number we've added since last sync. */
From: Xuan Zhuo <xuanzhuo@linux.alibaba.com> Date: 2024-03-12 03:36:09
Now, the virtio core can set the premapped mode by find_vqs().
If the premapped can be enabled, the dma array will not be
allocated. So virtio-net use the api of find_vqs to enable the
premapped.
Judge the premapped mode by the vq->premapped instead of saving
local variable.
Signed-off-by: Xuan Zhuo <xuanzhuo@linux.alibaba.com>
---
drivers/net/virtio_net.c | 57 +++++++++++++++++------------------
include/linux/virtio_config.h | 16 ++--------
2 files changed, 29 insertions(+), 44 deletions(-)
@@ -208,9 +208,6 @@ struct receive_queue {/* Record the last dma info to free after new pages is allocated. */structvirtnet_rq_dma*last_dma;--/* Do dma by self */-booldo_dma;};/* This structure can contain rss message with maximum settings for indirection table and keysize
@@ -4322,8 +4305,13 @@ static int virtnet_find_vqs(struct virtnet_info *vi)ctx=kcalloc(total_vqs,sizeof(*ctx),GFP_KERNEL);if(!ctx)gotoerr_ctx;++premapped=kcalloc(total_vqs,sizeof(*premapped),GFP_KERNEL);+if(!ctx)+gotoerr_premapped;}else{ctx=NULL;+premapped=NULL;}/* Parameters for control virtqueue, if any */
@@ -4342,10 +4330,19 @@ static int virtnet_find_vqs(struct virtnet_info *vi)names[txq2vq(i)]=vi->sq[i].name;if(ctx)ctx[rxq2vq(i)]=true;++if(premapped)+premapped[rxq2vq(i)]=true;}-ret=virtio_find_vqs_ctx(vi->vdev,total_vqs,vqs,callbacks,-names,ctx,NULL);+cfg.nvqs=total_vqs;+cfg.vqs=vqs;+cfg.callbacks=callbacks;+cfg.names=names;+cfg.ctx=ctx;+cfg.premapped=premapped;++ret=virtio_find_vqs_cfg(vi->vdev,&cfg);if(ret)gotoerr_find;
@@ -4365,6 +4362,8 @@ static int virtnet_find_vqs(struct virtnet_info *vi)err_find:+kfree(premapped);+err_premapped:kfree(ctx);err_ctx:kfree(names);
@@ -4437,8 +4436,6 @@ static int init_vqs(struct virtnet_info *vi)if(ret)gotoerr_free;-virtnet_rq_set_premapped(vi);-cpus_read_lock();virtnet_set_affinity(vi);cpus_read_unlock();
From: Xuan Zhuo <xuanzhuo@linux.alibaba.com> Date: 2024-03-12 03:36:10
Now, the API virtqueue_set_dma_premapped just support to
enable premapped mode.
If we allow enabling the premapped dynamically, we should
make this API to support disable the premapped mode.
Signed-off-by: Xuan Zhuo <xuanzhuo@linux.alibaba.com>
---
drivers/virtio/virtio_ring.c | 34 ++++++++++++++++++++++++++--------
include/linux/virtio.h | 2 +-
2 files changed, 27 insertions(+), 9 deletions(-)
From: "Michael S. Tsirkin" <mst@redhat.com> Date: 2024-03-19 06:56:26
On Tue, Mar 12, 2024 at 11:35:47AM +0800, Xuan Zhuo wrote:
As discussed:
http://lore.kernel.org/all/CACGkMEvq0No8QGC46U4mGsMtuD44fD_cfLcPaVmJ3rHYqRZxYg@mail.gmail.com
If the virtio is premapped mode, the driver should manage the dma info by self.
So the virtio core should not store the dma info. We can release the memory used
to store the dma info.
For virtio-net xmit queue, if the virtio-net maintains the dma info,
the virtio-net must allocate too much memory(19 * queue_size for per-queue), so
we do not plan to make the virtio-net to maintain the dma info by default. The
virtio-net xmit queue only maintain the dma info when premapped mode is enable
(such as AF_XDP is enable).
This landed when merge window was open already so I'm deferring this
to the next merge window, just to be safe. Jason can you review please?
So this patch set try to do:
1. make the virtio core to do not store the dma info
- But if the desc_extra has not dma info, we face a new question,
it is hard to get the dma info of the desc with indirect flag.
For split mode, that is easy from desc, but for the packed mode,
it is hard to get the dma info from the desc. And hardening
the dma unmap is safe, we should store the dma info of indirect
descs when the virtio core does not store the bufer dma info.
So I introduce the "structure the indirect desc table" to
allocate space to store dma info of the desc table.
+struct vring_split_desc_indir {
+ dma_addr_t addr; /* Descriptor Array DMA addr. */
+ u32 len; /* Descriptor Array length. */
+ u32 num;
+ struct vring_desc desc[];
+};
The follow patches to this:
* virtio_ring: packed: structure the indirect desc table
* virtio_ring: split: structure the indirect desc table
- On the other side, in the umap handle, we mix the indirect descs with
other descs. That make things too complex. I found if we we distinguish
the descs with VRING_DESC_F_INDIRECT before unmap, thing will be clearer.
The follow patches do this.
* virtio_ring: packed: remove double check of the unmap ops
* virtio_ring: split: structure the indirect desc table
2. make the virtio core to enable premapped mode by find_vqs() params
- Because the find_vqs() will try to allocate memory for the dma info.
If we set the premapped mode after find_vqs() and release the
dma info, that is odd.
Please review.
Thanks
v4:
1. virtio-net xmit queue does not enable premapped mode by default
v3:
1. fix the conflict with the vp_modern_create_avq().
v2:
1. change the dma item of virtio-net, every item have MAX_SKB_FRAGS + 2 addr + len pairs.
2. introduce virtnet_sq_free_stats for __free_old_xmit
v1:
1. rename transport_vq_config to vq_transport_config
2. virtio-net set dma meta number to (ring-size + 1)(MAX_SKB_FRGAS +2)
3. introduce virtqueue_dma_map_sg_attrs
4. separate vring_create_virtqueue to an independent commit
Xuan Zhuo (10):
virtio_ring: introduce vring_need_unmap_buffer
virtio_ring: packed: remove double check of the unmap ops
virtio_ring: packed: structure the indirect desc table
virtio_ring: split: remove double check of the unmap ops
virtio_ring: split: structure the indirect desc table
virtio_ring: no store dma info when unmap is not needed
virtio: find_vqs: add new parameter premapped
virtio_ring: export premapped to driver by struct virtqueue
virtio_net: set premapped mode by find_vqs()
virtio_ring: virtqueue_set_dma_premapped support disable
drivers/net/virtio_net.c | 57 +++--
drivers/virtio/virtio_ring.c | 436 +++++++++++++++++++++-------------
include/linux/virtio.h | 3 +-
include/linux/virtio_config.h | 17 +-
4 files changed, 307 insertions(+), 206 deletions(-)
--
2.32.0.3.g01195cf9f
From: Jason Wang <hidden> Date: 2024-03-20 09:25:43
On Tue, Mar 19, 2024 at 2:56 PM Michael S. Tsirkin [off-list ref] wrote:
On Tue, Mar 12, 2024 at 11:35:47AM +0800, Xuan Zhuo wrote:
quoted
As discussed:
http://lore.kernel.org/all/CACGkMEvq0No8QGC46U4mGsMtuD44fD_cfLcPaVmJ3rHYqRZxYg@mail.gmail.com
If the virtio is premapped mode, the driver should manage the dma info by self.
So the virtio core should not store the dma info. We can release the memory used
to store the dma info.
For virtio-net xmit queue, if the virtio-net maintains the dma info,
the virtio-net must allocate too much memory(19 * queue_size for per-queue), so
we do not plan to make the virtio-net to maintain the dma info by default. The
virtio-net xmit queue only maintain the dma info when premapped mode is enable
(such as AF_XDP is enable).
This landed when merge window was open already so I'm deferring this
to the next merge window, just to be safe. Jason can you review please?
From: Jason Wang <hidden> Date: 2024-03-21 04:45:24
On Tue, Mar 12, 2024 at 11:36 AM Xuan Zhuo [off-list ref] wrote:
As discussed:
http://lore.kernel.org/all/CACGkMEvq0No8QGC46U4mGsMtuD44fD_cfLcPaVmJ3rHYqRZxYg@mail.gmail.com
If the virtio is premapped mode, the driver should manage the dma info by self.
So the virtio core should not store the dma info. We can release the memory used
to store the dma info.
For virtio-net xmit queue, if the virtio-net maintains the dma info,
the virtio-net must allocate too much memory(19 * queue_size for per-queue), so
we do not plan to make the virtio-net to maintain the dma info by default. The
virtio-net xmit queue only maintain the dma info when premapped mode is enable
(such as AF_XDP is enable).
So this patch set try to do:
1. make the virtio core to do not store the dma info
I think you mean "make the virtio core to do not store the dma info
when driver can do that"
- But if the desc_extra has not dma info, we face a new question,
it is hard to get the dma info of the desc with indirect flag.
I guess you want to avoid allocating desc_extra array, otherwise you
won't have this issue.
How about keeping that?
For split mode, that is easy from desc, but for the packed mode,
it is hard to get the dma info from the desc. And hardening
the dma unmap is safe, we should store the dma info of indirect
descs when the virtio core does not store the bufer dma info.
So I introduce the "structure the indirect desc table" to
allocate space to store dma info of the desc table.
+struct vring_split_desc_indir {
+ dma_addr_t addr; /* Descriptor Array DMA addr. */
+ u32 len; /* Descriptor Array length. */
+ u32 num;
We can probably just reuse vring_desc_extra here with a known flag
(read only for device).
+ struct vring_desc desc[];
+};
The follow patches to this:
* virtio_ring: packed: structure the indirect desc table
* virtio_ring: split: structure the indirect desc table
- On the other side, in the umap handle, we mix the indirect descs with
other descs. That make things too complex. I found if we we distinguish
the descs with VRING_DESC_F_INDIRECT before unmap, thing will be clearer.
The follow patches do this.
* virtio_ring: packed: remove double check of the unmap ops
* virtio_ring: split: structure the indirect desc table
2. make the virtio core to enable premapped mode by find_vqs() params
- Because the find_vqs() will try to allocate memory for the dma info.
If we set the premapped mode after find_vqs() and release the
dma info, that is odd.
Thanks
Please review.
Thanks
v4:
1. virtio-net xmit queue does not enable premapped mode by default
v3:
1. fix the conflict with the vp_modern_create_avq().
v2:
1. change the dma item of virtio-net, every item have MAX_SKB_FRAGS + 2 addr + len pairs.
2. introduce virtnet_sq_free_stats for __free_old_xmit
v1:
1. rename transport_vq_config to vq_transport_config
2. virtio-net set dma meta number to (ring-size + 1)(MAX_SKB_FRGAS +2)
3. introduce virtqueue_dma_map_sg_attrs
4. separate vring_create_virtqueue to an independent commit
Xuan Zhuo (10):
virtio_ring: introduce vring_need_unmap_buffer
virtio_ring: packed: remove double check of the unmap ops
virtio_ring: packed: structure the indirect desc table
virtio_ring: split: remove double check of the unmap ops
virtio_ring: split: structure the indirect desc table
virtio_ring: no store dma info when unmap is not needed
virtio: find_vqs: add new parameter premapped
virtio_ring: export premapped to driver by struct virtqueue
virtio_net: set premapped mode by find_vqs()
virtio_ring: virtqueue_set_dma_premapped support disable
drivers/net/virtio_net.c | 57 +++--
drivers/virtio/virtio_ring.c | 436 +++++++++++++++++++++-------------
include/linux/virtio.h | 3 +-
include/linux/virtio_config.h | 17 +-
4 files changed, 307 insertions(+), 206 deletions(-)
--
2.32.0.3.g01195cf9f
From: Jason Wang <hidden> Date: 2024-03-21 04:47:33
On Tue, Mar 12, 2024 at 11:36 AM Xuan Zhuo [off-list ref] wrote:
quoted hunk
This commit structure the indirect desc table.
Then we can get the desc num directly when doing unmap.
And save the dma info to the struct, then the indirect
will not use the dma fields of the desc_extra. The subsequent
commits will make the dma fields are optional. But for
the indirect case, we must record the dma info.
Signed-off-by: Xuan Zhuo <xuanzhuo@linux.alibaba.com>
---
drivers/virtio/virtio_ring.c | 66 +++++++++++++++++++++---------------
1 file changed, 38 insertions(+), 28 deletions(-)
From: Jason Wang <hidden> Date: 2024-03-21 05:57:20
On Tue, Mar 12, 2024 at 11:36 AM Xuan Zhuo [off-list ref] wrote:
quoted hunk
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.
1. only one desc of the desc table is used, we do not need the loop
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
Signed-off-by: Xuan Zhuo <xuanzhuo@linux.alibaba.com>
---
drivers/virtio/virtio_ring.c | 78 ++++++++++++++++++------------------
1 file changed, 40 insertions(+), 38 deletions(-)
Theoretically, indirect descriptors could be chained. It is supported
without this patch but not here.
Thanks
quoted hunk
+
/* Free the indirect table, if any, now that it's unmapped. */
desc = state->indir_desc;
- if (!desc)
- return;
if (vring_need_unmap_buffer(vq)) {
len = vq->packed.desc_extra[id].len;
From: Jason Wang <hidden> Date: 2024-03-21 06:02:29
On Tue, Mar 12, 2024 at 11:36 AM Xuan Zhuo [off-list ref] wrote:
quoted hunk
Now, the API virtqueue_set_dma_premapped just support to
enable premapped mode.
If we allow enabling the premapped dynamically, we should
make this API to support disable the premapped mode.
Signed-off-by: Xuan Zhuo <xuanzhuo@linux.alibaba.com>
---
drivers/virtio/virtio_ring.c | 34 ++++++++++++++++++++++++++--------
include/linux/virtio.h | 2 +-
2 files changed, 27 insertions(+), 9 deletions(-)
I think we need to document the requirement for calling this.
Looking at the code, it seems it requires to stop the datapath and
detach all the used buffers?
Thanks
From: Xuan Zhuo <xuanzhuo@linux.alibaba.com> Date: 2024-03-21 08:21:10
On Thu, 21 Mar 2024 13:57:06 +0800, Jason Wang [off-list ref] wrote:
On Tue, Mar 12, 2024 at 11:36 AM 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.
1. only one desc of the desc table is used, we do not need the loop
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
Signed-off-by: Xuan Zhuo <xuanzhuo@linux.alibaba.com>
---
drivers/virtio/virtio_ring.c | 78 ++++++++++++++++++------------------
1 file changed, 40 insertions(+), 38 deletions(-)
Theoretically, indirect descriptors could be chained. It is supported
without this patch but not here.
YES. But now, that is not supported by "add", so I think we
do not need to think about it.
Thanks.
Thanks
quoted
+
/* Free the indirect table, if any, now that it's unmapped. */
desc = state->indir_desc;
- if (!desc)
- return;
if (vring_need_unmap_buffer(vq)) {
len = vq->packed.desc_extra[id].len;
From: Xuan Zhuo <xuanzhuo@linux.alibaba.com> Date: 2024-03-21 08:22:29
On Thu, 21 Mar 2024 14:02:14 +0800, Jason Wang [off-list ref] wrote:
On Tue, Mar 12, 2024 at 11:36 AM Xuan Zhuo [off-list ref] wrote:
quoted
Now, the API virtqueue_set_dma_premapped just support to
enable premapped mode.
If we allow enabling the premapped dynamically, we should
make this API to support disable the premapped mode.
Signed-off-by: Xuan Zhuo <xuanzhuo@linux.alibaba.com>
---
drivers/virtio/virtio_ring.c | 34 ++++++++++++++++++++++++++--------
include/linux/virtio.h | 2 +-
2 files changed, 27 insertions(+), 9 deletions(-)
I think we need to document the requirement for calling this.
Looking at the code, it seems it requires to stop the datapath and
detach all the used buffers?
YES. The complete document is:
/**
* virtqueue_set_dma_premapped - set the vring premapped mode
* @_vq: the struct virtqueue we're talking about.
*
* Enable the premapped mode of the vq.
*
* The vring in premapped mode does not do dma internally, so the driver must
* do dma mapping in advance. The driver must pass the dma_address through
* dma_address of scatterlist. When the driver got a used buffer from
* the vring, it has to unmap the dma address.
*
* This function must be called immediately after creating the vq, or after vq
* reset, and before adding any buffers to it.
*
* Caller must ensure we don't call this with other virtqueue operations
* at the same time (except where noted).
*
* Returns zero or a negative error.
* 0: success.
* -EINVAL: vring does not use the dma api, so we can not enable premapped mode.
*/
Thanks
From: Xuan Zhuo <xuanzhuo@linux.alibaba.com> Date: 2024-03-21 08:29:35
On Thu, 21 Mar 2024 12:47:18 +0800, Jason Wang [off-list ref] wrote:
On Tue, Mar 12, 2024 at 11:36 AM Xuan Zhuo [off-list ref] wrote:
quoted
This commit structure the indirect desc table.
Then we can get the desc num directly when doing unmap.
And save the dma info to the struct, then the indirect
will not use the dma fields of the desc_extra. The subsequent
commits will make the dma fields are optional. But for
the indirect case, we must record the dma info.
Signed-off-by: Xuan Zhuo <xuanzhuo@linux.alibaba.com>
---
drivers/virtio/virtio_ring.c | 66 +++++++++++++++++++++---------------
1 file changed, 38 insertions(+), 28 deletions(-)
@@ -72,9 +72,16 @@ struct vring_desc_state_split {structvring_desc*indir_desc;/* Indirect descriptor, if any. */};+structvring_packed_desc_indir{+dma_addr_taddr;/* Descriptor Array DMA addr. */+u32len;/* Descriptor Array length. */+u32num;+structvring_packed_descdesc[];+};+structvring_desc_state_packed{void*data;/* Data for callback. */-structvring_packed_desc*indir_desc;/* Indirect descriptor, if any. */+structvring_packed_desc_indir*indir_desc;/* Indirect descriptor, if any. */
Maybe it's better just to have a vring_desc_extra here.
Do you mean replacing vring_packed_desc_indir by vring_desc_extra?
I am ok for that. But vring_desc_extra has two extra items:
u16 flags; /* Descriptor flags. */
u16 next; /* The next desc state in a list. */
vring_packed_desc_indir has "desc". I think that is more convenient.
So, I think vring_packed_desc_indir is appropriate.
Or I missed something.
Thanks.
From: Xuan Zhuo <xuanzhuo@linux.alibaba.com> Date: 2024-03-21 08:32:48
On Thu, 21 Mar 2024 12:45:08 +0800, Jason Wang [off-list ref] wrote:
On Tue, Mar 12, 2024 at 11:36 AM Xuan Zhuo [off-list ref] wrote:
quoted
As discussed:
http://lore.kernel.org/all/CACGkMEvq0No8QGC46U4mGsMtuD44fD_cfLcPaVmJ3rHYqRZxYg@mail.gmail.com
If the virtio is premapped mode, the driver should manage the dma info by self.
So the virtio core should not store the dma info. We can release the memory used
to store the dma info.
For virtio-net xmit queue, if the virtio-net maintains the dma info,
the virtio-net must allocate too much memory(19 * queue_size for per-queue), so
we do not plan to make the virtio-net to maintain the dma info by default. The
virtio-net xmit queue only maintain the dma info when premapped mode is enable
(such as AF_XDP is enable).
So this patch set try to do:
1. make the virtio core to do not store the dma info
I think you mean "make the virtio core to do not store the dma info
when driver can do that"
YES.
quoted
- But if the desc_extra has not dma info, we face a new question,
it is hard to get the dma info of the desc with indirect flag.
I guess you want to avoid allocating desc_extra array, otherwise you
won't have this issue.
How about keeping that?
This is a way. But when we allocate the indirect desc, we alloc
more memory to save that, I think that is a good way.
And in the future, we can handen the unmap for the indirect buffer
with more memory allocated by once.
Thanks.
quoted
For split mode, that is easy from desc, but for the packed mode,
it is hard to get the dma info from the desc. And hardening
the dma unmap is safe, we should store the dma info of indirect
descs when the virtio core does not store the bufer dma info.
So I introduce the "structure the indirect desc table" to
allocate space to store dma info of the desc table.
+struct vring_split_desc_indir {
+ dma_addr_t addr; /* Descriptor Array DMA addr. */
+ u32 len; /* Descriptor Array length. */
+ u32 num;
We can probably just reuse vring_desc_extra here with a known flag
(read only for device).
quoted
+ struct vring_desc desc[];
+};
The follow patches to this:
* virtio_ring: packed: structure the indirect desc table
* virtio_ring: split: structure the indirect desc table
- On the other side, in the umap handle, we mix the indirect descs with
other descs. That make things too complex. I found if we we distinguish
the descs with VRING_DESC_F_INDIRECT before unmap, thing will be clearer.
The follow patches do this.
* virtio_ring: packed: remove double check of the unmap ops
* virtio_ring: split: structure the indirect desc table
2. make the virtio core to enable premapped mode by find_vqs() params
- Because the find_vqs() will try to allocate memory for the dma info.
If we set the premapped mode after find_vqs() and release the
dma info, that is odd.
Thanks
quoted
Please review.
Thanks
v4:
1. virtio-net xmit queue does not enable premapped mode by default
v3:
1. fix the conflict with the vp_modern_create_avq().
v2:
1. change the dma item of virtio-net, every item have MAX_SKB_FRAGS + 2 addr + len pairs.
2. introduce virtnet_sq_free_stats for __free_old_xmit
v1:
1. rename transport_vq_config to vq_transport_config
2. virtio-net set dma meta number to (ring-size + 1)(MAX_SKB_FRGAS +2)
3. introduce virtqueue_dma_map_sg_attrs
4. separate vring_create_virtqueue to an independent commit
Xuan Zhuo (10):
virtio_ring: introduce vring_need_unmap_buffer
virtio_ring: packed: remove double check of the unmap ops
virtio_ring: packed: structure the indirect desc table
virtio_ring: split: remove double check of the unmap ops
virtio_ring: split: structure the indirect desc table
virtio_ring: no store dma info when unmap is not needed
virtio: find_vqs: add new parameter premapped
virtio_ring: export premapped to driver by struct virtqueue
virtio_net: set premapped mode by find_vqs()
virtio_ring: virtqueue_set_dma_premapped support disable
drivers/net/virtio_net.c | 57 +++--
drivers/virtio/virtio_ring.c | 436 +++++++++++++++++++++-------------
include/linux/virtio.h | 3 +-
include/linux/virtio_config.h | 17 +-
4 files changed, 307 insertions(+), 206 deletions(-)
--
2.32.0.3.g01195cf9f
From: Jason Wang <hidden> Date: 2024-03-22 05:13:51
On Thu, Mar 21, 2024 at 4:22 PM Xuan Zhuo [off-list ref] wrote:
On Thu, 21 Mar 2024 14:02:14 +0800, Jason Wang [off-list ref] wrote:
quoted
On Tue, Mar 12, 2024 at 11:36 AM Xuan Zhuo [off-list ref] wrote:
quoted
Now, the API virtqueue_set_dma_premapped just support to
enable premapped mode.
If we allow enabling the premapped dynamically, we should
make this API to support disable the premapped mode.
Signed-off-by: Xuan Zhuo <xuanzhuo@linux.alibaba.com>
---
drivers/virtio/virtio_ring.c | 34 ++++++++++++++++++++++++++--------
include/linux/virtio.h | 2 +-
2 files changed, 27 insertions(+), 9 deletions(-)
I think we need to document the requirement for calling this.
Looking at the code, it seems it requires to stop the datapath and
detach all the used buffers?
YES. The complete document is:
/**
* virtqueue_set_dma_premapped - set the vring premapped mode
* @_vq: the struct virtqueue we're talking about.
*
* Enable the premapped mode of the vq.
*
* The vring in premapped mode does not do dma internally, so the driver must
* do dma mapping in advance. The driver must pass the dma_address through
* dma_address of scatterlist. When the driver got a used buffer from
* the vring, it has to unmap the dma address.
*
* This function must be called immediately after creating the vq, or after vq
* reset, and before adding any buffers to it.
I'm not sure this is a good design but we need at least some guard for
this, probably WARN for num_added or others.
Thanks
*
* Caller must ensure we don't call this with other virtqueue operations
* at the same time (except where noted).
*
* Returns zero or a negative error.
* 0: success.
* -EINVAL: vring does not use the dma api, so we can not enable premapped mode.
*/
Thanks
From: Jason Wang <hidden> Date: 2024-03-22 05:17:18
On Thu, Mar 21, 2024 at 4:21 PM Xuan Zhuo [off-list ref] wrote:
On Thu, 21 Mar 2024 13:57:06 +0800, Jason Wang [off-list ref] wrote:
quoted
On Tue, Mar 12, 2024 at 11:36 AM 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.
1. only one desc of the desc table is used, we do not need the loop
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
Signed-off-by: Xuan Zhuo <xuanzhuo@linux.alibaba.com>
---
drivers/virtio/virtio_ring.c | 78 ++++++++++++++++++------------------
1 file changed, 40 insertions(+), 38 deletions(-)
From: Jason Wang <hidden> Date: 2024-03-22 05:22:48
On Thu, Mar 21, 2024 at 4:29 PM Xuan Zhuo [off-list ref] wrote:
On Thu, 21 Mar 2024 12:47:18 +0800, Jason Wang [off-list ref] wrote:
quoted
On Tue, Mar 12, 2024 at 11:36 AM Xuan Zhuo [off-list ref] wrote:
quoted
This commit structure the indirect desc table.
Then we can get the desc num directly when doing unmap.
And save the dma info to the struct, then the indirect
will not use the dma fields of the desc_extra. The subsequent
commits will make the dma fields are optional. But for
the indirect case, we must record the dma info.
Signed-off-by: Xuan Zhuo <xuanzhuo@linux.alibaba.com>
---
drivers/virtio/virtio_ring.c | 66 +++++++++++++++++++++---------------
1 file changed, 38 insertions(+), 28 deletions(-)
@@ -72,9 +72,16 @@ struct vring_desc_state_split {structvring_desc*indir_desc;/* Indirect descriptor, if any. */};+structvring_packed_desc_indir{+dma_addr_taddr;/* Descriptor Array DMA addr. */+u32len;/* Descriptor Array length. */+u32num;+structvring_packed_descdesc[];+};+structvring_desc_state_packed{void*data;/* Data for callback. */-structvring_packed_desc*indir_desc;/* Indirect descriptor, if any. */+structvring_packed_desc_indir*indir_desc;/* Indirect descriptor, if any. */
Maybe it's better just to have a vring_desc_extra here.
Do you mean replacing vring_packed_desc_indir by vring_desc_extra?
Just add a vring_desc_extra in vring_desc_state_packed.
I am ok for that. But vring_desc_extra has two extra items:
u16 flags; /* Descriptor flags. */
u16 next; /* The next desc state in a list. */
vring_packed_desc_indir has "desc". I think that is more convenient.
So, I think vring_packed_desc_indir is appropriate.
It reuses the existing structure so we had the chance to reuse the
helper. And it could be used for future chained indirect (if it turns
out to be necessary).
Thanks
From: Xuan Zhuo <xuanzhuo@linux.alibaba.com> Date: 2024-03-22 06:03:15
On Fri, 22 Mar 2024 13:15:10 +0800, Jason Wang [off-list ref] wrote:
On Thu, Mar 21, 2024 at 4:29 PM Xuan Zhuo [off-list ref] wrote:
quoted
On Thu, 21 Mar 2024 12:47:18 +0800, Jason Wang [off-list ref] wrote:
quoted
On Tue, Mar 12, 2024 at 11:36 AM Xuan Zhuo [off-list ref] wrote:
quoted
This commit structure the indirect desc table.
Then we can get the desc num directly when doing unmap.
And save the dma info to the struct, then the indirect
will not use the dma fields of the desc_extra. The subsequent
commits will make the dma fields are optional. But for
the indirect case, we must record the dma info.
Signed-off-by: Xuan Zhuo <xuanzhuo@linux.alibaba.com>
---
drivers/virtio/virtio_ring.c | 66 +++++++++++++++++++++---------------
1 file changed, 38 insertions(+), 28 deletions(-)
@@ -72,9 +72,16 @@ struct vring_desc_state_split {structvring_desc*indir_desc;/* Indirect descriptor, if any. */};+structvring_packed_desc_indir{+dma_addr_taddr;/* Descriptor Array DMA addr. */+u32len;/* Descriptor Array length. */+u32num;+structvring_packed_descdesc[];+};+structvring_desc_state_packed{void*data;/* Data for callback. */-structvring_packed_desc*indir_desc;/* Indirect descriptor, if any. */+structvring_packed_desc_indir*indir_desc;/* Indirect descriptor, if any. */
Maybe it's better just to have a vring_desc_extra here.
Do you mean replacing vring_packed_desc_indir by vring_desc_extra?
Just add a vring_desc_extra in vring_desc_state_packed.
I am surprise to here that.
Do you mean this: #1
struct vring_desc_state_packed {
void *data; /* Data for callback. */
struct vring_packed_desc *indir_desc; /* Indirect descriptor, if any. */
u16 num; /* Descriptor list length. */
u16 last; /* The last desc state in a list. */
struct vring_desc_extra desc_extra;
};
Then desc_extra is included by desc_state. I do not think so.
I guess you mean this: #2
struct vring_desc_state_packed {
void *data; /* Data for callback. */
struct vring_desc_extra *indir_desc; /* Indirect descriptor, if any. */
u16 num; /* Descriptor list length. */
u16 last; /* The last desc state in a list. */
};
indir_desc pointers to memory:
|struct vring_desc_extra | struct vring_packed_desc desc[] |
quoted
I am ok for that. But vring_desc_extra has two extra items:
u16 flags; /* Descriptor flags. */
u16 next; /* The next desc state in a list. */
vring_packed_desc_indir has "desc". I think that is more convenient.
So, I think vring_packed_desc_indir is appropriate.
It reuses the existing structure so we had the chance to reuse the
helper.
Which helper?
But, if you mean #2. I am ok.
Thanks.
And it could be used for future chained indirect (if it turns
out to be necessary).
Thanks
From: Xuan Zhuo <xuanzhuo@linux.alibaba.com> Date: 2024-03-22 06:04:43
On Fri, 22 Mar 2024 13:13:36 +0800, Jason Wang [off-list ref] wrote:
On Thu, Mar 21, 2024 at 4:22 PM Xuan Zhuo [off-list ref] wrote:
quoted
On Thu, 21 Mar 2024 14:02:14 +0800, Jason Wang [off-list ref] wrote:
quoted
On Tue, Mar 12, 2024 at 11:36 AM Xuan Zhuo [off-list ref] wrote:
quoted
Now, the API virtqueue_set_dma_premapped just support to
enable premapped mode.
If we allow enabling the premapped dynamically, we should
make this API to support disable the premapped mode.
Signed-off-by: Xuan Zhuo <xuanzhuo@linux.alibaba.com>
---
drivers/virtio/virtio_ring.c | 34 ++++++++++++++++++++++++++--------
include/linux/virtio.h | 2 +-
2 files changed, 27 insertions(+), 9 deletions(-)
I think we need to document the requirement for calling this.
Looking at the code, it seems it requires to stop the datapath and
detach all the used buffers?
YES. The complete document is:
/**
* virtqueue_set_dma_premapped - set the vring premapped mode
* @_vq: the struct virtqueue we're talking about.
*
* Enable the premapped mode of the vq.
*
* The vring in premapped mode does not do dma internally, so the driver must
* do dma mapping in advance. The driver must pass the dma_address through
* dma_address of scatterlist. When the driver got a used buffer from
* the vring, it has to unmap the dma address.
*
* This function must be called immediately after creating the vq, or after vq
* reset, and before adding any buffers to it.
I'm not sure this is a good design but we need at least some guard for
this, probably WARN for num_added or others.
int virtqueue_set_dma_premapped(struct virtqueue *_vq)
{
struct vring_virtqueue *vq = to_vvq(_vq);
u32 num;
START_USE(vq);
num = vq->packed_ring ? vq->packed.vring.num : vq->split.vring.num;
if (num != vq->vq.num_free) {
END_USE(vq);
return -EINVAL;
}
Now, we have checked the num_free.
Thanks.
Thanks
quoted
*
* Caller must ensure we don't call this with other virtqueue operations
* at the same time (except where noted).
*
* Returns zero or a negative error.
* 0: success.
* -EINVAL: vring does not use the dma api, so we can not enable premapped mode.
*/
Thanks
From: Xuan Zhuo <xuanzhuo@linux.alibaba.com> Date: 2024-03-22 07:58:07
On Fri, 22 Mar 2024 13:15:10 +0800, Jason Wang [off-list ref] wrote:
On Thu, Mar 21, 2024 at 4:29 PM Xuan Zhuo [off-list ref] wrote:
quoted
On Thu, 21 Mar 2024 12:47:18 +0800, Jason Wang [off-list ref] wrote:
quoted
On Tue, Mar 12, 2024 at 11:36 AM Xuan Zhuo [off-list ref] wrote:
quoted
This commit structure the indirect desc table.
Then we can get the desc num directly when doing unmap.
And save the dma info to the struct, then the indirect
will not use the dma fields of the desc_extra. The subsequent
commits will make the dma fields are optional. But for
the indirect case, we must record the dma info.
Signed-off-by: Xuan Zhuo <xuanzhuo@linux.alibaba.com>
---
drivers/virtio/virtio_ring.c | 66 +++++++++++++++++++++---------------
1 file changed, 38 insertions(+), 28 deletions(-)
@@ -72,9 +72,16 @@ struct vring_desc_state_split {structvring_desc*indir_desc;/* Indirect descriptor, if any. */};+structvring_packed_desc_indir{+dma_addr_taddr;/* Descriptor Array DMA addr. */+u32len;/* Descriptor Array length. */+u32num;+structvring_packed_descdesc[];+};+structvring_desc_state_packed{void*data;/* Data for callback. */-structvring_packed_desc*indir_desc;/* Indirect descriptor, if any. */+structvring_packed_desc_indir*indir_desc;/* Indirect descriptor, if any. */
Maybe it's better just to have a vring_desc_extra here.
Do you mean replacing vring_packed_desc_indir by vring_desc_extra?
Just add a vring_desc_extra in vring_desc_state_packed.
quoted
I am ok for that. But vring_desc_extra has two extra items:
u16 flags; /* Descriptor flags. */
u16 next; /* The next desc state in a list. */
vring_packed_desc_indir has "desc". I think that is more convenient.
So, I think vring_packed_desc_indir is appropriate.
It reuses the existing structure so we had the chance to reuse the
helper.
Do you mean vring_unmap_extra_packed()?
After last commit(virtio_ring: packed: remove double check of the unmap ops):
/* caller must check vring_need_unmap_buffer() */
static void vring_unmap_extra_packed(const struct vring_virtqueue *vq,
const struct vring_desc_extra *extra)
{
u16 flags;
flags = extra->flags;
dma_unmap_page(vring_dma_dev(vq),
extra->addr, extra->len,
(flags & VRING_DESC_F_WRITE) ?
DMA_FROM_DEVICE : DMA_TO_DEVICE);
}
But we should call dma_unmap_single() for indirect desc.
We know, dma_unmap_single() and dma_unmap_page() are same in essence.
So if we call dma_unmap_page for the indirect desc, we can reuse
this function. But I do not prefer doing this.
Thanks.
And it could be used for future chained indirect (if it turns
out to be necessary).
Thanks
From: Jason Wang <hidden> Date: 2024-03-25 07:07:36
On Fri, Mar 22, 2024 at 3:58 PM Xuan Zhuo [off-list ref] wrote:
On Fri, 22 Mar 2024 13:15:10 +0800, Jason Wang [off-list ref] wrote:
quoted
On Thu, Mar 21, 2024 at 4:29 PM Xuan Zhuo [off-list ref] wrote:
quoted
On Thu, 21 Mar 2024 12:47:18 +0800, Jason Wang [off-list ref] wrote:
quoted
On Tue, Mar 12, 2024 at 11:36 AM Xuan Zhuo [off-list ref] wrote:
quoted
This commit structure the indirect desc table.
Then we can get the desc num directly when doing unmap.
And save the dma info to the struct, then the indirect
will not use the dma fields of the desc_extra. The subsequent
commits will make the dma fields are optional. But for
the indirect case, we must record the dma info.
Signed-off-by: Xuan Zhuo <xuanzhuo@linux.alibaba.com>
---
drivers/virtio/virtio_ring.c | 66 +++++++++++++++++++++---------------
1 file changed, 38 insertions(+), 28 deletions(-)
@@ -72,9 +72,16 @@ struct vring_desc_state_split {structvring_desc*indir_desc;/* Indirect descriptor, if any. */};+structvring_packed_desc_indir{+dma_addr_taddr;/* Descriptor Array DMA addr. */+u32len;/* Descriptor Array length. */+u32num;+structvring_packed_descdesc[];+};+structvring_desc_state_packed{void*data;/* Data for callback. */-structvring_packed_desc*indir_desc;/* Indirect descriptor, if any. */+structvring_packed_desc_indir*indir_desc;/* Indirect descriptor, if any. */
Maybe it's better just to have a vring_desc_extra here.
Do you mean replacing vring_packed_desc_indir by vring_desc_extra?
Just add a vring_desc_extra in vring_desc_state_packed.
quoted
I am ok for that. But vring_desc_extra has two extra items:
u16 flags; /* Descriptor flags. */
u16 next; /* The next desc state in a list. */
vring_packed_desc_indir has "desc". I think that is more convenient.
So, I think vring_packed_desc_indir is appropriate.
It reuses the existing structure so we had the chance to reuse the
helper.
Do you mean vring_unmap_extra_packed()?
Yes.
After last commit(virtio_ring: packed: remove double check of the unmap ops):
/* caller must check vring_need_unmap_buffer() */
static void vring_unmap_extra_packed(const struct vring_virtqueue *vq,
const struct vring_desc_extra *extra)
{
u16 flags;
flags = extra->flags;
dma_unmap_page(vring_dma_dev(vq),
extra->addr, extra->len,
(flags & VRING_DESC_F_WRITE) ?
DMA_FROM_DEVICE : DMA_TO_DEVICE);
}
But we should call dma_unmap_single() for indirect desc.
We know, dma_unmap_single() and dma_unmap_page() are same in essence.
Yes, it's worth tweaking in the future.
So if we call dma_unmap_page for the indirect desc, we can reuse
this function. But I do not prefer doing this.
Ok.
Thanks
Thanks.
quoted
And it could be used for future chained indirect (if it turns
out to be necessary).
Thanks
From: Jason Wang <hidden> Date: 2024-03-25 07:10:55
On Fri, Mar 22, 2024 at 2:04 PM Xuan Zhuo [off-list ref] wrote:
On Fri, 22 Mar 2024 13:13:36 +0800, Jason Wang [off-list ref] wrote:
quoted
On Thu, Mar 21, 2024 at 4:22 PM Xuan Zhuo [off-list ref] wrote:
quoted
On Thu, 21 Mar 2024 14:02:14 +0800, Jason Wang [off-list ref] wrote:
quoted
On Tue, Mar 12, 2024 at 11:36 AM Xuan Zhuo [off-list ref] wrote:
quoted
Now, the API virtqueue_set_dma_premapped just support to
enable premapped mode.
If we allow enabling the premapped dynamically, we should
make this API to support disable the premapped mode.
Signed-off-by: Xuan Zhuo <xuanzhuo@linux.alibaba.com>
---
drivers/virtio/virtio_ring.c | 34 ++++++++++++++++++++++++++--------
include/linux/virtio.h | 2 +-
2 files changed, 27 insertions(+), 9 deletions(-)
I think we need to document the requirement for calling this.
Looking at the code, it seems it requires to stop the datapath and
detach all the used buffers?
YES. The complete document is:
/**
* virtqueue_set_dma_premapped - set the vring premapped mode
* @_vq: the struct virtqueue we're talking about.
*
* Enable the premapped mode of the vq.
*
* The vring in premapped mode does not do dma internally, so the driver must
* do dma mapping in advance. The driver must pass the dma_address through
* dma_address of scatterlist. When the driver got a used buffer from
* the vring, it has to unmap the dma address.
*
* This function must be called immediately after creating the vq, or after vq
* reset, and before adding any buffers to it.
I'm not sure this is a good design but we need at least some guard for
this, probably WARN for num_added or others.
int virtqueue_set_dma_premapped(struct virtqueue *_vq)
{
struct vring_virtqueue *vq = to_vvq(_vq);
u32 num;
START_USE(vq);
num = vq->packed_ring ? vq->packed.vring.num : vq->split.vring.num;
if (num != vq->vq.num_free) {
END_USE(vq);
return -EINVAL;
}
Now, we have checked the num_free.
Ok, let's add it to the doc.
Thanks
Thanks.
quoted
Thanks
quoted
*
* Caller must ensure we don't call this with other virtqueue operations
* at the same time (except where noted).
*
* Returns zero or a negative error.
* 0: success.
* -EINVAL: vring does not use the dma api, so we can not enable premapped mode.
*/
Thanks
From: "Michael S. Tsirkin" <mst@redhat.com> Date: 2024-03-26 07:32:52
On Thu, Mar 21, 2024 at 04:20:09PM +0800, Xuan Zhuo wrote:
On Thu, 21 Mar 2024 13:57:06 +0800, Jason Wang [off-list ref] wrote:
quoted
On Tue, Mar 12, 2024 at 11:36 AM 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.
1. only one desc of the desc table is used, we do not need the loop
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
Signed-off-by: Xuan Zhuo <xuanzhuo@linux.alibaba.com>
---
drivers/virtio/virtio_ring.c | 78 ++++++++++++++++++------------------
1 file changed, 40 insertions(+), 38 deletions(-)
Theoretically, indirect descriptors could be chained. It is supported
without this patch but not here.
YES. But now, that is not supported by "add", so I think we
do not need to think about it.
Thanks.
the "add" you are referring to is virtio drivers in the linux guest?
That's not the only guest and there's no way to be sure
no one does it. We can make some unusual operations go somewhat
slower but breaking them outright is not a good idea.
quoted
Thanks
quoted
+
/* Free the indirect table, if any, now that it's unmapped. */
desc = state->indir_desc;
- if (!desc)
- return;
if (vring_need_unmap_buffer(vq)) {
len = vq->packed.desc_extra[id].len;
From: Xuan Zhuo <xuanzhuo@linux.alibaba.com> Date: 2024-03-27 07:14:51
On Tue, 26 Mar 2024 03:32:43 -0400, "Michael S. Tsirkin" [off-list ref] wrote:
On Thu, Mar 21, 2024 at 04:20:09PM +0800, Xuan Zhuo wrote:
quoted
On Thu, 21 Mar 2024 13:57:06 +0800, Jason Wang [off-list ref] wrote:
quoted
On Tue, Mar 12, 2024 at 11:36 AM 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.
1. only one desc of the desc table is used, we do not need the loop
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
Signed-off-by: Xuan Zhuo <xuanzhuo@linux.alibaba.com>
---
drivers/virtio/virtio_ring.c | 78 ++++++++++++++++++------------------
1 file changed, 40 insertions(+), 38 deletions(-)
Theoretically, indirect descriptors could be chained. It is supported
without this patch but not here.
YES. But now, that is not supported by "add", so I think we
do not need to think about it.
Thanks.
the "add" you are referring to is virtio drivers in the linux guest?
That's not the only guest and there's no way to be sure
no one does it. We can make some unusual operations go somewhat
slower but breaking them outright is not a good idea.
"add" means virtqueue_add_packed/virtqueue_add_split.
If the virtqueue_add_packed/virtqueue_add_split do not chain
the indirect desc, then I think do not consider the case that
the indirect desc wash chained.
That was all done by the linux virtio core, we do not need to
consider other cases.
Thanks.
quoted
quoted
Thanks
quoted
+
/* Free the indirect table, if any, now that it's unmapped. */
desc = state->indir_desc;
- if (!desc)
- return;
if (vring_need_unmap_buffer(vq)) {
len = vq->packed.desc_extra[id].len;