| Message ID | 20231011092728.105904-2-xuanzhuo@linux.alibaba.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | virtio-net: support AF_XDP zero copy | expand |
Hi Xuan,
kernel test robot noticed the following build warnings:
[auto build test WARNING on linus/master]
[also build test WARNING on v6.6-rc5 next-20231011]
[cannot apply to mst-vhost/linux-next]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]
url: https://github.com/intel-lab-lkp/linux/commits/Xuan-Zhuo/virtio_ring-virtqueue_set_dma_premapped-support-disable/20231011-180709
base: linus/master
patch link: https://lore.kernel.org/r/20231011092728.105904-2-xuanzhuo%40linux.alibaba.com
patch subject: [PATCH vhost 01/22] virtio_ring: virtqueue_set_dma_premapped support disable
config: m68k-allyesconfig (https://download.01.org/0day-ci/archive/20231011/202310112204.h03TUDpH-lkp@intel.com/config)
compiler: m68k-linux-gcc (GCC) 13.2.0
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20231011/202310112204.h03TUDpH-lkp@intel.com/reproduce)
If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202310112204.h03TUDpH-lkp@intel.com/
All warnings (new ones prefixed by >>):
>> drivers/virtio/virtio_ring.c:2788: warning: Function parameter or member 'mode' not described in 'virtqueue_set_dma_premapped'
vim +2788 drivers/virtio/virtio_ring.c
c790e8e1817f1a Xuan Zhuo 2022-08-01 2765
8daafe9ebbd21a Xuan Zhuo 2023-08-10 2766 /**
8daafe9ebbd21a Xuan Zhuo 2023-08-10 2767 * virtqueue_set_dma_premapped - set the vring premapped mode
8daafe9ebbd21a Xuan Zhuo 2023-08-10 2768 * @_vq: the struct virtqueue we're talking about.
8daafe9ebbd21a Xuan Zhuo 2023-08-10 2769 *
8daafe9ebbd21a Xuan Zhuo 2023-08-10 2770 * Enable the premapped mode of the vq.
8daafe9ebbd21a Xuan Zhuo 2023-08-10 2771 *
8daafe9ebbd21a Xuan Zhuo 2023-08-10 2772 * The vring in premapped mode does not do dma internally, so the driver must
8daafe9ebbd21a Xuan Zhuo 2023-08-10 2773 * do dma mapping in advance. The driver must pass the dma_address through
8daafe9ebbd21a Xuan Zhuo 2023-08-10 2774 * dma_address of scatterlist. When the driver got a used buffer from
8daafe9ebbd21a Xuan Zhuo 2023-08-10 2775 * the vring, it has to unmap the dma address.
8daafe9ebbd21a Xuan Zhuo 2023-08-10 2776 *
8daafe9ebbd21a Xuan Zhuo 2023-08-10 2777 * This function must be called immediately after creating the vq, or after vq
8daafe9ebbd21a Xuan Zhuo 2023-08-10 2778 * reset, and before adding any buffers to it.
8daafe9ebbd21a Xuan Zhuo 2023-08-10 2779 *
8daafe9ebbd21a Xuan Zhuo 2023-08-10 2780 * Caller must ensure we don't call this with other virtqueue operations
8daafe9ebbd21a Xuan Zhuo 2023-08-10 2781 * at the same time (except where noted).
8daafe9ebbd21a Xuan Zhuo 2023-08-10 2782 *
8daafe9ebbd21a Xuan Zhuo 2023-08-10 2783 * Returns zero or a negative error.
8daafe9ebbd21a Xuan Zhuo 2023-08-10 2784 * 0: success.
8daafe9ebbd21a Xuan Zhuo 2023-08-10 2785 * -EINVAL: vring does not use the dma api, so we can not enable premapped mode.
8daafe9ebbd21a Xuan Zhuo 2023-08-10 2786 */
f8d1a236ad114f Xuan Zhuo 2023-10-11 2787 int virtqueue_set_dma_premapped(struct virtqueue *_vq, bool mode)
8daafe9ebbd21a Xuan Zhuo 2023-08-10 @2788 {
8daafe9ebbd21a Xuan Zhuo 2023-08-10 2789 struct vring_virtqueue *vq = to_vvq(_vq);
8daafe9ebbd21a Xuan Zhuo 2023-08-10 2790 u32 num;
8daafe9ebbd21a Xuan Zhuo 2023-08-10 2791
8daafe9ebbd21a Xuan Zhuo 2023-08-10 2792 START_USE(vq);
8daafe9ebbd21a Xuan Zhuo 2023-08-10 2793
8daafe9ebbd21a Xuan Zhuo 2023-08-10 2794 num = vq->packed_ring ? vq->packed.vring.num : vq->split.vring.num;
8daafe9ebbd21a Xuan Zhuo 2023-08-10 2795
8daafe9ebbd21a Xuan Zhuo 2023-08-10 2796 if (num != vq->vq.num_free) {
8daafe9ebbd21a Xuan Zhuo 2023-08-10 2797 END_USE(vq);
8daafe9ebbd21a Xuan Zhuo 2023-08-10 2798 return -EINVAL;
8daafe9ebbd21a Xuan Zhuo 2023-08-10 2799 }
8daafe9ebbd21a Xuan Zhuo 2023-08-10 2800
8daafe9ebbd21a Xuan Zhuo 2023-08-10 2801 if (!vq->use_dma_api) {
8daafe9ebbd21a Xuan Zhuo 2023-08-10 2802 END_USE(vq);
8daafe9ebbd21a Xuan Zhuo 2023-08-10 2803 return -EINVAL;
8daafe9ebbd21a Xuan Zhuo 2023-08-10 2804 }
8daafe9ebbd21a Xuan Zhuo 2023-08-10 2805
f8d1a236ad114f Xuan Zhuo 2023-10-11 2806 if (mode) {
8daafe9ebbd21a Xuan Zhuo 2023-08-10 2807 vq->premapped = true;
b319940f83c21b Xuan Zhuo 2023-08-10 2808 vq->do_unmap = false;
f8d1a236ad114f Xuan Zhuo 2023-10-11 2809 } else {
f8d1a236ad114f Xuan Zhuo 2023-10-11 2810 vq->premapped = false;
f8d1a236ad114f Xuan Zhuo 2023-10-11 2811 vq->do_unmap = vq->use_dma_api;
f8d1a236ad114f Xuan Zhuo 2023-10-11 2812 }
8daafe9ebbd21a Xuan Zhuo 2023-08-10 2813
8daafe9ebbd21a Xuan Zhuo 2023-08-10 2814 END_USE(vq);
8daafe9ebbd21a Xuan Zhuo 2023-08-10 2815
8daafe9ebbd21a Xuan Zhuo 2023-08-10 2816 return 0;
8daafe9ebbd21a Xuan Zhuo 2023-08-10 2817 }
8daafe9ebbd21a Xuan Zhuo 2023-08-10 2818 EXPORT_SYMBOL_GPL(virtqueue_set_dma_premapped);
8daafe9ebbd21a Xuan Zhuo 2023-08-10 2819
On Wed, Oct 11, 2023 at 05:27:07PM +0800, Xuan Zhuo wrote: > virtqueue_set_dma_premapped() adds a new parameter to disable the > virtqueue premapped mode. > > Signed-off-by: Xuan Zhuo <xuanzhuo@linux.alibaba.com> > --- > drivers/net/virtio_net.c | 2 +- > drivers/virtio/virtio_ring.c | 11 ++++++++--- > include/linux/virtio.h | 2 +- > 3 files changed, 10 insertions(+), 5 deletions(-) > > diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c > index fe7f314d65c9..6b5f47ebf9b2 100644 > --- a/drivers/net/virtio_net.c > +++ b/drivers/net/virtio_net.c > @@ -737,7 +737,7 @@ static void virtnet_rq_set_premapped(struct virtnet_info *vi) > return; > > for (i = 0; i < vi->max_queue_pairs; i++) { > - if (virtqueue_set_dma_premapped(vi->rq[i].vq)) > + if (virtqueue_set_dma_premapped(vi->rq[i].vq, true)) > continue; > > vi->rq[i].do_dma = true; > diff --git a/drivers/virtio/virtio_ring.c b/drivers/virtio/virtio_ring.c > index 51d8f3299c10..b3ded56722f4 100644 > --- a/drivers/virtio/virtio_ring.c > +++ b/drivers/virtio/virtio_ring.c > @@ -2784,7 +2784,7 @@ EXPORT_SYMBOL_GPL(virtqueue_resize); > * 0: success. > * -EINVAL: vring does not use the dma api, so we can not enable premapped mode. > */ > -int virtqueue_set_dma_premapped(struct virtqueue *_vq) > +int virtqueue_set_dma_premapped(struct virtqueue *_vq, bool mode) > { > struct vring_virtqueue *vq = to_vvq(_vq); > u32 num; > @@ -2803,8 +2803,13 @@ int virtqueue_set_dma_premapped(struct virtqueue *_vq) > return -EINVAL; > } > > - vq->premapped = true; > - vq->do_unmap = false; > + if (mode) { > + vq->premapped = true; > + vq->do_unmap = false; > + } else { > + vq->premapped = false; > + vq->do_unmap = vq->use_dma_api; > + } > > END_USE(vq); > > diff --git a/include/linux/virtio.h b/include/linux/virtio.h > index 4cc614a38376..1cf7b004348b 100644 > --- a/include/linux/virtio.h > +++ b/include/linux/virtio.h > @@ -81,7 +81,7 @@ bool virtqueue_enable_cb(struct virtqueue *vq); > > unsigned virtqueue_enable_cb_prepare(struct virtqueue *vq); > > -int virtqueue_set_dma_premapped(struct virtqueue *_vq); > +int virtqueue_set_dma_premapped(struct virtqueue *_vq, bool mode); > > bool virtqueue_poll(struct virtqueue *vq, unsigned); Wait a sec I thought we never change premapped. If you make this dynamic don't you need a bunch of locking? Or maybe queue is empty when you change this? If yes pls add a bunch of BUG_ON checks to make sure this is not misused. > -- > 2.32.0.3.g01195cf9f
On Thu, 12 Oct 2023 05:15:52 -0400, "Michael S. Tsirkin" <mst@redhat.com> wrote: > On Wed, Oct 11, 2023 at 05:27:07PM +0800, Xuan Zhuo wrote: > > virtqueue_set_dma_premapped() adds a new parameter to disable the > > virtqueue premapped mode. > > > > Signed-off-by: Xuan Zhuo <xuanzhuo@linux.alibaba.com> > > --- > > drivers/net/virtio_net.c | 2 +- > > drivers/virtio/virtio_ring.c | 11 ++++++++--- > > include/linux/virtio.h | 2 +- > > 3 files changed, 10 insertions(+), 5 deletions(-) > > > > diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c > > index fe7f314d65c9..6b5f47ebf9b2 100644 > > --- a/drivers/net/virtio_net.c > > +++ b/drivers/net/virtio_net.c > > @@ -737,7 +737,7 @@ static void virtnet_rq_set_premapped(struct virtnet_info *vi) > > return; > > > > for (i = 0; i < vi->max_queue_pairs; i++) { > > - if (virtqueue_set_dma_premapped(vi->rq[i].vq)) > > + if (virtqueue_set_dma_premapped(vi->rq[i].vq, true)) > > continue; > > > > vi->rq[i].do_dma = true; > > diff --git a/drivers/virtio/virtio_ring.c b/drivers/virtio/virtio_ring.c > > index 51d8f3299c10..b3ded56722f4 100644 > > --- a/drivers/virtio/virtio_ring.c > > +++ b/drivers/virtio/virtio_ring.c > > @@ -2784,7 +2784,7 @@ EXPORT_SYMBOL_GPL(virtqueue_resize); > > * 0: success. > > * -EINVAL: vring does not use the dma api, so we can not enable premapped mode. > > */ > > -int virtqueue_set_dma_premapped(struct virtqueue *_vq) > > +int virtqueue_set_dma_premapped(struct virtqueue *_vq, bool mode) > > { > > struct vring_virtqueue *vq = to_vvq(_vq); > > u32 num; > > @@ -2803,8 +2803,13 @@ int virtqueue_set_dma_premapped(struct virtqueue *_vq) > > return -EINVAL; > > } > > > > - vq->premapped = true; > > - vq->do_unmap = false; > > + if (mode) { > > + vq->premapped = true; > > + vq->do_unmap = false; > > + } else { > > + vq->premapped = false; > > + vq->do_unmap = vq->use_dma_api; > > + } > > > > END_USE(vq); > > > > diff --git a/include/linux/virtio.h b/include/linux/virtio.h > > index 4cc614a38376..1cf7b004348b 100644 > > --- a/include/linux/virtio.h > > +++ b/include/linux/virtio.h > > @@ -81,7 +81,7 @@ bool virtqueue_enable_cb(struct virtqueue *vq); > > > > unsigned virtqueue_enable_cb_prepare(struct virtqueue *vq); > > > > -int virtqueue_set_dma_premapped(struct virtqueue *_vq); > > +int virtqueue_set_dma_premapped(struct virtqueue *_vq, bool mode); > > > > bool virtqueue_poll(struct virtqueue *vq, unsigned); > > Wait a sec I thought we never change premapped. If you make this > dynamic don't you need a bunch of locking? > Or maybe queue is empty when you change this? > If yes pls add a bunch of BUG_ON checks to make sure this is not misused. Actually, this api is called immediately after the vq init or vq reset. We already have such a check. Thanks. /** * 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. */ int virtqueue_set_dma_premapped(struct virtqueue *_vq, bool mode) { 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; } if (!vq->use_dma_api) { END_USE(vq); return -EINVAL; } if (mode) { vq->premapped = true; vq->do_unmap = false; } else { vq->premapped = false; vq->do_unmap = vq->use_dma_api; } END_USE(vq); return 0; } EXPORT_SYMBOL_GPL(virtqueue_set_dma_premapped); > > > > -- > > 2.32.0.3.g01195cf9f >
On Thu, Oct 12, 2023 at 05:18:54PM +0800, Xuan Zhuo wrote: > On Thu, 12 Oct 2023 05:15:52 -0400, "Michael S. Tsirkin" <mst@redhat.com> wrote: > > On Wed, Oct 11, 2023 at 05:27:07PM +0800, Xuan Zhuo wrote: > > > virtqueue_set_dma_premapped() adds a new parameter to disable the > > > virtqueue premapped mode. > > > > > > Signed-off-by: Xuan Zhuo <xuanzhuo@linux.alibaba.com> > > > --- > > > drivers/net/virtio_net.c | 2 +- > > > drivers/virtio/virtio_ring.c | 11 ++++++++--- > > > include/linux/virtio.h | 2 +- > > > 3 files changed, 10 insertions(+), 5 deletions(-) > > > > > > diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c > > > index fe7f314d65c9..6b5f47ebf9b2 100644 > > > --- a/drivers/net/virtio_net.c > > > +++ b/drivers/net/virtio_net.c > > > @@ -737,7 +737,7 @@ static void virtnet_rq_set_premapped(struct virtnet_info *vi) > > > return; > > > > > > for (i = 0; i < vi->max_queue_pairs; i++) { > > > - if (virtqueue_set_dma_premapped(vi->rq[i].vq)) > > > + if (virtqueue_set_dma_premapped(vi->rq[i].vq, true)) > > > continue; > > > > > > vi->rq[i].do_dma = true; > > > diff --git a/drivers/virtio/virtio_ring.c b/drivers/virtio/virtio_ring.c > > > index 51d8f3299c10..b3ded56722f4 100644 > > > --- a/drivers/virtio/virtio_ring.c > > > +++ b/drivers/virtio/virtio_ring.c > > > @@ -2784,7 +2784,7 @@ EXPORT_SYMBOL_GPL(virtqueue_resize); > > > * 0: success. > > > * -EINVAL: vring does not use the dma api, so we can not enable premapped mode. > > > */ > > > -int virtqueue_set_dma_premapped(struct virtqueue *_vq) > > > +int virtqueue_set_dma_premapped(struct virtqueue *_vq, bool mode) > > > { > > > struct vring_virtqueue *vq = to_vvq(_vq); > > > u32 num; > > > @@ -2803,8 +2803,13 @@ int virtqueue_set_dma_premapped(struct virtqueue *_vq) > > > return -EINVAL; > > > } > > > > > > - vq->premapped = true; > > > - vq->do_unmap = false; > > > + if (mode) { > > > + vq->premapped = true; > > > + vq->do_unmap = false; > > > + } else { > > > + vq->premapped = false; > > > + vq->do_unmap = vq->use_dma_api; > > > + } > > > > > > END_USE(vq); > > > > > > diff --git a/include/linux/virtio.h b/include/linux/virtio.h > > > index 4cc614a38376..1cf7b004348b 100644 > > > --- a/include/linux/virtio.h > > > +++ b/include/linux/virtio.h > > > @@ -81,7 +81,7 @@ bool virtqueue_enable_cb(struct virtqueue *vq); > > > > > > unsigned virtqueue_enable_cb_prepare(struct virtqueue *vq); > > > > > > -int virtqueue_set_dma_premapped(struct virtqueue *_vq); > > > +int virtqueue_set_dma_premapped(struct virtqueue *_vq, bool mode); > > > > > > bool virtqueue_poll(struct virtqueue *vq, unsigned); > > > > Wait a sec I thought we never change premapped. If you make this > > dynamic don't you need a bunch of locking? > > Or maybe queue is empty when you change this? > > If yes pls add a bunch of BUG_ON checks to make sure this is not misused. > > > Actually, this api is called immediately after the vq init or vq reset. > > We already have such a check. > > Thanks. > > /** > * 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. > */ > int virtqueue_set_dma_premapped(struct virtqueue *_vq, bool mode) > { > 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; > } But it turns out virtnet_rq_set_premapped actually just ignores errors. So returning EINVAL here does nothing caller just proceeds? And checking num_free without locks is never safe anyway. I think the point is that this never triggers then just BUG_ON. > > if (!vq->use_dma_api) { > END_USE(vq); > return -EINVAL; > } > > if (mode) { > vq->premapped = true; > vq->do_unmap = false; > } else { > vq->premapped = false; > vq->do_unmap = vq->use_dma_api; > } > > END_USE(vq); > > return 0; > } > EXPORT_SYMBOL_GPL(virtqueue_set_dma_premapped); > > > > > > > > > -- > > > 2.32.0.3.g01195cf9f > >
On Thu, 12 Oct 2023 05:40:38 -0400, "Michael S. Tsirkin" <mst@redhat.com> wrote: > On Thu, Oct 12, 2023 at 05:18:54PM +0800, Xuan Zhuo wrote: > > On Thu, 12 Oct 2023 05:15:52 -0400, "Michael S. Tsirkin" <mst@redhat.com> wrote: > > > On Wed, Oct 11, 2023 at 05:27:07PM +0800, Xuan Zhuo wrote: > > > > virtqueue_set_dma_premapped() adds a new parameter to disable the > > > > virtqueue premapped mode. > > > > > > > > Signed-off-by: Xuan Zhuo <xuanzhuo@linux.alibaba.com> > > > > --- > > > > drivers/net/virtio_net.c | 2 +- > > > > drivers/virtio/virtio_ring.c | 11 ++++++++--- > > > > include/linux/virtio.h | 2 +- > > > > 3 files changed, 10 insertions(+), 5 deletions(-) > > > > > > > > diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c > > > > index fe7f314d65c9..6b5f47ebf9b2 100644 > > > > --- a/drivers/net/virtio_net.c > > > > +++ b/drivers/net/virtio_net.c > > > > @@ -737,7 +737,7 @@ static void virtnet_rq_set_premapped(struct virtnet_info *vi) > > > > return; > > > > > > > > for (i = 0; i < vi->max_queue_pairs; i++) { > > > > - if (virtqueue_set_dma_premapped(vi->rq[i].vq)) > > > > + if (virtqueue_set_dma_premapped(vi->rq[i].vq, true)) > > > > continue; > > > > > > > > vi->rq[i].do_dma = true; > > > > diff --git a/drivers/virtio/virtio_ring.c b/drivers/virtio/virtio_ring.c > > > > index 51d8f3299c10..b3ded56722f4 100644 > > > > --- a/drivers/virtio/virtio_ring.c > > > > +++ b/drivers/virtio/virtio_ring.c > > > > @@ -2784,7 +2784,7 @@ EXPORT_SYMBOL_GPL(virtqueue_resize); > > > > * 0: success. > > > > * -EINVAL: vring does not use the dma api, so we can not enable premapped mode. > > > > */ > > > > -int virtqueue_set_dma_premapped(struct virtqueue *_vq) > > > > +int virtqueue_set_dma_premapped(struct virtqueue *_vq, bool mode) > > > > { > > > > struct vring_virtqueue *vq = to_vvq(_vq); > > > > u32 num; > > > > @@ -2803,8 +2803,13 @@ int virtqueue_set_dma_premapped(struct virtqueue *_vq) > > > > return -EINVAL; > > > > } > > > > > > > > - vq->premapped = true; > > > > - vq->do_unmap = false; > > > > + if (mode) { > > > > + vq->premapped = true; > > > > + vq->do_unmap = false; > > > > + } else { > > > > + vq->premapped = false; > > > > + vq->do_unmap = vq->use_dma_api; > > > > + } > > > > > > > > END_USE(vq); > > > > > > > > diff --git a/include/linux/virtio.h b/include/linux/virtio.h > > > > index 4cc614a38376..1cf7b004348b 100644 > > > > --- a/include/linux/virtio.h > > > > +++ b/include/linux/virtio.h > > > > @@ -81,7 +81,7 @@ bool virtqueue_enable_cb(struct virtqueue *vq); > > > > > > > > unsigned virtqueue_enable_cb_prepare(struct virtqueue *vq); > > > > > > > > -int virtqueue_set_dma_premapped(struct virtqueue *_vq); > > > > +int virtqueue_set_dma_premapped(struct virtqueue *_vq, bool mode); > > > > > > > > bool virtqueue_poll(struct virtqueue *vq, unsigned); > > > > > > Wait a sec I thought we never change premapped. If you make this > > > dynamic don't you need a bunch of locking? > > > Or maybe queue is empty when you change this? > > > If yes pls add a bunch of BUG_ON checks to make sure this is not misused. > > > > > > Actually, this api is called immediately after the vq init or vq reset. > > > > We already have such a check. > > > > Thanks. > > > > /** > > * 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. > > */ > > int virtqueue_set_dma_premapped(struct virtqueue *_vq, bool mode) > > { > > 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; > > } > > But it turns out virtnet_rq_set_premapped actually just ignores errors. > So returning EINVAL here does nothing caller just proceeds? > And checking num_free without locks is never safe anyway. The premise of all this is that this is called immediately after reset or init. So here, this error doesn't occur, so I didn't check. Regarding the lock issue, there will be no race for either rq or sq. rq is called after init vq, and there is no race at this time. In this patch set, sq is called during reset, and there will be no race. So I think it's safe. > I think the point is that this never triggers then just BUG_ON. > Yes, I agree, it would be better to use BUG_ON. The next verion will remove this commit, because I will put the patch set to the net-next. So I will not let the premapped to be dynamic. About the BUG_ON, I will post a patch to fix that. Thanks. > > > > > if (!vq->use_dma_api) { > > END_USE(vq); > > return -EINVAL; > > } > > > > if (mode) { > > vq->premapped = true; > > vq->do_unmap = false; > > } else { > > vq->premapped = false; > > vq->do_unmap = vq->use_dma_api; > > } > > > > END_USE(vq); > > > > return 0; > > } > > EXPORT_SYMBOL_GPL(virtqueue_set_dma_premapped); > > > > > > > > > > > > > > -- > > > > 2.32.0.3.g01195cf9f > > > >
diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c index fe7f314d65c9..6b5f47ebf9b2 100644 --- a/drivers/net/virtio_net.c +++ b/drivers/net/virtio_net.c @@ -737,7 +737,7 @@ static void virtnet_rq_set_premapped(struct virtnet_info *vi) return; for (i = 0; i < vi->max_queue_pairs; i++) { - if (virtqueue_set_dma_premapped(vi->rq[i].vq)) + if (virtqueue_set_dma_premapped(vi->rq[i].vq, true)) continue; vi->rq[i].do_dma = true; diff --git a/drivers/virtio/virtio_ring.c b/drivers/virtio/virtio_ring.c index 51d8f3299c10..b3ded56722f4 100644 --- a/drivers/virtio/virtio_ring.c +++ b/drivers/virtio/virtio_ring.c @@ -2784,7 +2784,7 @@ EXPORT_SYMBOL_GPL(virtqueue_resize); * 0: success. * -EINVAL: vring does not use the dma api, so we can not enable premapped mode. */ -int virtqueue_set_dma_premapped(struct virtqueue *_vq) +int virtqueue_set_dma_premapped(struct virtqueue *_vq, bool mode) { struct vring_virtqueue *vq = to_vvq(_vq); u32 num; @@ -2803,8 +2803,13 @@ int virtqueue_set_dma_premapped(struct virtqueue *_vq) return -EINVAL; } - vq->premapped = true; - vq->do_unmap = false; + if (mode) { + vq->premapped = true; + vq->do_unmap = false; + } else { + vq->premapped = false; + vq->do_unmap = vq->use_dma_api; + } END_USE(vq); diff --git a/include/linux/virtio.h b/include/linux/virtio.h index 4cc614a38376..1cf7b004348b 100644 --- a/include/linux/virtio.h +++ b/include/linux/virtio.h @@ -81,7 +81,7 @@ bool virtqueue_enable_cb(struct virtqueue *vq); unsigned virtqueue_enable_cb_prepare(struct virtqueue *vq); -int virtqueue_set_dma_premapped(struct virtqueue *_vq); +int virtqueue_set_dma_premapped(struct virtqueue *_vq, bool mode); bool virtqueue_poll(struct virtqueue *vq, unsigned);
virtqueue_set_dma_premapped() adds a new parameter to disable the virtqueue premapped mode. Signed-off-by: Xuan Zhuo <xuanzhuo@linux.alibaba.com> --- drivers/net/virtio_net.c | 2 +- drivers/virtio/virtio_ring.c | 11 ++++++++--- include/linux/virtio.h | 2 +- 3 files changed, 10 insertions(+), 5 deletions(-)