Re: [PATCH] vduse: make vduse_class constant
On Sun, Oct 08, 2023 at 02:41:22AM -0400, Michael S. Tsirkin wrote: > On Sun, Oct 08, 2023 at 08:40:05AM +0200, Greg Kroah-Hartman wrote: > > On Sun, Oct 08, 2023 at 02:20:52AM -0400, Michael S. Tsirkin wrote: > > > On Fri, Oct 06, 2023 at 04:30:44PM +0200, Greg Kroah-Hartman wrote: > > > > Now that the driver core allows for struct class to be in read-only > > > > memory, we should make all 'class' structures declared at build time > > > > placing them into read-only memory, instead of having to be dynamically > > > > allocated at runtime. > > > > > > > > Cc: "Michael S. Tsirkin" > > > > Cc: Jason Wang > > > > Cc: Xuan Zhuo > > > > Cc: Xie Yongji > > > > Signed-off-by: Greg Kroah-Hartman > > > > > > Acked-by: Michael S. Tsirkin > > > > > > Greg should I merge it or do you intend to merge all these patches? > > > > "all"? There's loads of them for all sorts of subsystems, so feel free > > to take it through your subsystem tree if you want. I usually scoop up > > the ones that no one picks after a release and take them through my > > tree, to pick up the stragglers. > > > > So it's your call, whatever is easier for you is fine for me. > > > > thanks, > > > > greg k-h > > To clarify which commit does this depend on? The 6.4 kernel release :) ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH] vduse: make vduse_class constant
On Sun, Oct 08, 2023 at 08:40:05AM +0200, Greg Kroah-Hartman wrote: > On Sun, Oct 08, 2023 at 02:20:52AM -0400, Michael S. Tsirkin wrote: > > On Fri, Oct 06, 2023 at 04:30:44PM +0200, Greg Kroah-Hartman wrote: > > > Now that the driver core allows for struct class to be in read-only > > > memory, we should make all 'class' structures declared at build time > > > placing them into read-only memory, instead of having to be dynamically > > > allocated at runtime. > > > > > > Cc: "Michael S. Tsirkin" > > > Cc: Jason Wang > > > Cc: Xuan Zhuo > > > Cc: Xie Yongji > > > Signed-off-by: Greg Kroah-Hartman > > > > Acked-by: Michael S. Tsirkin > > > > Greg should I merge it or do you intend to merge all these patches? > > "all"? There's loads of them for all sorts of subsystems, so feel free > to take it through your subsystem tree if you want. I usually scoop up > the ones that no one picks after a release and take them through my > tree, to pick up the stragglers. > > So it's your call, whatever is easier for you is fine for me. > > thanks, > > greg k-h To clarify which commit does this depend on? -- MST ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH] vduse: make vduse_class constant
On Sun, Oct 08, 2023 at 02:20:52AM -0400, Michael S. Tsirkin wrote: > On Fri, Oct 06, 2023 at 04:30:44PM +0200, Greg Kroah-Hartman wrote: > > Now that the driver core allows for struct class to be in read-only > > memory, we should make all 'class' structures declared at build time > > placing them into read-only memory, instead of having to be dynamically > > allocated at runtime. > > > > Cc: "Michael S. Tsirkin" > > Cc: Jason Wang > > Cc: Xuan Zhuo > > Cc: Xie Yongji > > Signed-off-by: Greg Kroah-Hartman > > Acked-by: Michael S. Tsirkin > > Greg should I merge it or do you intend to merge all these patches? "all"? There's loads of them for all sorts of subsystems, so feel free to take it through your subsystem tree if you want. I usually scoop up the ones that no one picks after a release and take them through my tree, to pick up the stragglers. So it's your call, whatever is easier for you is fine for me. thanks, greg k-h ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH] vduse: make vduse_class constant
On Fri, Oct 06, 2023 at 04:30:44PM +0200, Greg Kroah-Hartman wrote:
> Now that the driver core allows for struct class to be in read-only
> memory, we should make all 'class' structures declared at build time
> placing them into read-only memory, instead of having to be dynamically
> allocated at runtime.
>
> Cc: "Michael S. Tsirkin"
> Cc: Jason Wang
> Cc: Xuan Zhuo
> Cc: Xie Yongji
> Signed-off-by: Greg Kroah-Hartman
Acked-by: Michael S. Tsirkin
Greg should I merge it or do you intend to merge all these patches?
> ---
> drivers/vdpa/vdpa_user/vduse_dev.c | 40 --
> 1 file changed, 21 insertions(+), 19 deletions(-)
>
> diff --git a/drivers/vdpa/vdpa_user/vduse_dev.c
> b/drivers/vdpa/vdpa_user/vduse_dev.c
> index df7869537ef1..0ddd4b8abecb 100644
> --- a/drivers/vdpa/vdpa_user/vduse_dev.c
> +++ b/drivers/vdpa/vdpa_user/vduse_dev.c
> @@ -134,7 +134,6 @@ static DEFINE_MUTEX(vduse_lock);
> static DEFINE_IDR(vduse_idr);
>
> static dev_t vduse_major;
> -static struct class *vduse_class;
> static struct cdev vduse_ctrl_cdev;
> static struct cdev vduse_cdev;
> static struct workqueue_struct *vduse_irq_wq;
> @@ -1528,6 +1527,16 @@ static const struct kobj_type vq_type = {
> .default_groups = vq_groups,
> };
>
> +static char *vduse_devnode(const struct device *dev, umode_t *mode)
> +{
> + return kasprintf(GFP_KERNEL, "vduse/%s", dev_name(dev));
> +}
> +
> +static const struct class vduse_class = {
> + .name = "vduse",
> + .devnode = vduse_devnode,
> +};
> +
> static void vduse_dev_deinit_vqs(struct vduse_dev *dev)
> {
> int i;
> @@ -1638,7 +1647,7 @@ static int vduse_destroy_dev(char *name)
> mutex_unlock(&dev->lock);
>
> vduse_dev_reset(dev);
> - device_destroy(vduse_class, MKDEV(MAJOR(vduse_major), dev->minor));
> + device_destroy(&vduse_class, MKDEV(MAJOR(vduse_major), dev->minor));
> idr_remove(&vduse_idr, dev->minor);
> kvfree(dev->config);
> vduse_dev_deinit_vqs(dev);
> @@ -1805,7 +1814,7 @@ static int vduse_create_dev(struct vduse_dev_config
> *config,
>
> dev->minor = ret;
> dev->msg_timeout = VDUSE_MSG_DEFAULT_TIMEOUT;
> - dev->dev = device_create_with_groups(vduse_class, NULL,
> + dev->dev = device_create_with_groups(&vduse_class, NULL,
> MKDEV(MAJOR(vduse_major), dev->minor),
> dev, vduse_dev_groups, "%s", config->name);
> if (IS_ERR(dev->dev)) {
> @@ -1821,7 +1830,7 @@ static int vduse_create_dev(struct vduse_dev_config
> *config,
>
> return 0;
> err_vqs:
> - device_destroy(vduse_class, MKDEV(MAJOR(vduse_major), dev->minor));
> + device_destroy(&vduse_class, MKDEV(MAJOR(vduse_major), dev->minor));
> err_dev:
> idr_remove(&vduse_idr, dev->minor);
> err_idr:
> @@ -1934,11 +1943,6 @@ static const struct file_operations vduse_ctrl_fops = {
> .llseek = noop_llseek,
> };
>
> -static char *vduse_devnode(const struct device *dev, umode_t *mode)
> -{
> - return kasprintf(GFP_KERNEL, "vduse/%s", dev_name(dev));
> -}
> -
> struct vduse_mgmt_dev {
> struct vdpa_mgmt_dev mgmt_dev;
> struct device dev;
> @@ -2082,11 +2086,9 @@ static int vduse_init(void)
> int ret;
> struct device *dev;
>
> - vduse_class = class_create("vduse");
> - if (IS_ERR(vduse_class))
> - return PTR_ERR(vduse_class);
> -
> - vduse_class->devnode = vduse_devnode;
> + ret = class_register(&vduse_class);
> + if (ret)
> + return ret;
>
> ret = alloc_chrdev_region(&vduse_major, 0, VDUSE_DEV_MAX, "vduse");
> if (ret)
> @@ -2099,7 +2101,7 @@ static int vduse_init(void)
> if (ret)
> goto err_ctrl_cdev;
>
> - dev = device_create(vduse_class, NULL, vduse_major, NULL, "control");
> + dev = device_create(&vduse_class, NULL, vduse_major, NULL, "control");
> if (IS_ERR(dev)) {
> ret = PTR_ERR(dev);
> goto err_device;
> @@ -2141,13 +2143,13 @@ static int vduse_init(void)
> err_wq:
> cdev_del(&vduse_cdev);
> err_cdev:
> - device_destroy(vduse_class, vduse_major);
> + device_destroy(&vduse_class, vduse_major);
> err_device:
> cdev_del(&vduse_ctrl_cdev);
> err_ctrl_cdev:
> unregister_chrdev_region(vduse_major, VDUSE_DEV_MAX);
> err_chardev_region:
> - class_destroy(vduse_class);
> + class_unregister(&vduse_class);
> return ret;
> }
> module_init(vduse_init);
> @@ -2159,10 +2161,10 @@ static void vduse_exit(void)
> destroy_workqueue(vduse_irq_bound_wq);
> destroy_workqueue(vduse_irq_wq);
> cdev_del(&vduse_cdev);
> - device_destroy(vduse_class, vduse_major);
> + device_destroy(&vduse_class, vduse_major);
> cdev_del(&vduse_ctrl_cdev);
> unregister_chrdev_region(vduse_major, VDUSE_DEV_MAX);
> - class_destroy(vduse_class);
> + class_unregister(&vduse_class);
> }
> module_exit(vdus
Re: [PATCH] vduse: make vduse_class constant
On Fri, Oct 6, 2023 at 10:31 PM Greg Kroah-Hartman
wrote:
>
> Now that the driver core allows for struct class to be in read-only
> memory, we should make all 'class' structures declared at build time
> placing them into read-only memory, instead of having to be dynamically
> allocated at runtime.
>
> Cc: "Michael S. Tsirkin"
> Cc: Jason Wang
> Cc: Xuan Zhuo
> Cc: Xie Yongji
> Signed-off-by: Greg Kroah-Hartman
Acked-by: Jason Wang
Thanks
> ---
> drivers/vdpa/vdpa_user/vduse_dev.c | 40 --
> 1 file changed, 21 insertions(+), 19 deletions(-)
>
> diff --git a/drivers/vdpa/vdpa_user/vduse_dev.c
> b/drivers/vdpa/vdpa_user/vduse_dev.c
> index df7869537ef1..0ddd4b8abecb 100644
> --- a/drivers/vdpa/vdpa_user/vduse_dev.c
> +++ b/drivers/vdpa/vdpa_user/vduse_dev.c
> @@ -134,7 +134,6 @@ static DEFINE_MUTEX(vduse_lock);
> static DEFINE_IDR(vduse_idr);
>
> static dev_t vduse_major;
> -static struct class *vduse_class;
> static struct cdev vduse_ctrl_cdev;
> static struct cdev vduse_cdev;
> static struct workqueue_struct *vduse_irq_wq;
> @@ -1528,6 +1527,16 @@ static const struct kobj_type vq_type = {
> .default_groups = vq_groups,
> };
>
> +static char *vduse_devnode(const struct device *dev, umode_t *mode)
> +{
> + return kasprintf(GFP_KERNEL, "vduse/%s", dev_name(dev));
> +}
> +
> +static const struct class vduse_class = {
> + .name = "vduse",
> + .devnode = vduse_devnode,
> +};
> +
> static void vduse_dev_deinit_vqs(struct vduse_dev *dev)
> {
> int i;
> @@ -1638,7 +1647,7 @@ static int vduse_destroy_dev(char *name)
> mutex_unlock(&dev->lock);
>
> vduse_dev_reset(dev);
> - device_destroy(vduse_class, MKDEV(MAJOR(vduse_major), dev->minor));
> + device_destroy(&vduse_class, MKDEV(MAJOR(vduse_major), dev->minor));
> idr_remove(&vduse_idr, dev->minor);
> kvfree(dev->config);
> vduse_dev_deinit_vqs(dev);
> @@ -1805,7 +1814,7 @@ static int vduse_create_dev(struct vduse_dev_config
> *config,
>
> dev->minor = ret;
> dev->msg_timeout = VDUSE_MSG_DEFAULT_TIMEOUT;
> - dev->dev = device_create_with_groups(vduse_class, NULL,
> + dev->dev = device_create_with_groups(&vduse_class, NULL,
> MKDEV(MAJOR(vduse_major), dev->minor),
> dev, vduse_dev_groups, "%s", config->name);
> if (IS_ERR(dev->dev)) {
> @@ -1821,7 +1830,7 @@ static int vduse_create_dev(struct vduse_dev_config
> *config,
>
> return 0;
> err_vqs:
> - device_destroy(vduse_class, MKDEV(MAJOR(vduse_major), dev->minor));
> + device_destroy(&vduse_class, MKDEV(MAJOR(vduse_major), dev->minor));
> err_dev:
> idr_remove(&vduse_idr, dev->minor);
> err_idr:
> @@ -1934,11 +1943,6 @@ static const struct file_operations vduse_ctrl_fops = {
> .llseek = noop_llseek,
> };
>
> -static char *vduse_devnode(const struct device *dev, umode_t *mode)
> -{
> - return kasprintf(GFP_KERNEL, "vduse/%s", dev_name(dev));
> -}
> -
> struct vduse_mgmt_dev {
> struct vdpa_mgmt_dev mgmt_dev;
> struct device dev;
> @@ -2082,11 +2086,9 @@ static int vduse_init(void)
> int ret;
> struct device *dev;
>
> - vduse_class = class_create("vduse");
> - if (IS_ERR(vduse_class))
> - return PTR_ERR(vduse_class);
> -
> - vduse_class->devnode = vduse_devnode;
> + ret = class_register(&vduse_class);
> + if (ret)
> + return ret;
>
> ret = alloc_chrdev_region(&vduse_major, 0, VDUSE_DEV_MAX, "vduse");
> if (ret)
> @@ -2099,7 +2101,7 @@ static int vduse_init(void)
> if (ret)
> goto err_ctrl_cdev;
>
> - dev = device_create(vduse_class, NULL, vduse_major, NULL, "control");
> + dev = device_create(&vduse_class, NULL, vduse_major, NULL, "control");
> if (IS_ERR(dev)) {
> ret = PTR_ERR(dev);
> goto err_device;
> @@ -2141,13 +2143,13 @@ static int vduse_init(void)
> err_wq:
> cdev_del(&vduse_cdev);
> err_cdev:
> - device_destroy(vduse_class, vduse_major);
> + device_destroy(&vduse_class, vduse_major);
> err_device:
> cdev_del(&vduse_ctrl_cdev);
> err_ctrl_cdev:
> unregister_chrdev_region(vduse_major, VDUSE_DEV_MAX);
> err_chardev_region:
> - class_destroy(vduse_class);
> + class_unregister(&vduse_class);
> return ret;
> }
> module_init(vduse_init);
> @@ -2159,10 +2161,10 @@ static void vduse_exit(void)
> destroy_workqueue(vduse_irq_bound_wq);
> destroy_workqueue(vduse_irq_wq);
> cdev_del(&vduse_cdev);
> - device_destroy(vduse_class, vduse_major);
> + device_destroy(&vduse_class, vduse_major);
> cdev_del(&vduse_ctrl_cdev);
> unregister_chrdev_region(vduse_major, VDUSE_DEV_MAX);
> - class_destroy(vduse_class);
> + class_unregister(&vduse_clas
Re: [PATCH net-next v4 2/2] virtio-net: add cond_resched() to the command waiting loop
On Fri, Oct 6, 2023 at 3:35 AM Feng Liu wrote:
>
>
>
> On 2023-07-24 a.m.2:46, Michael S. Tsirkin wrote:
> > External email: Use caution opening links or attachments
> >
> >
> > On Fri, Jul 21, 2023 at 10:18:03PM +0200, Maxime Coquelin wrote:
> >>
> >>
> >> On 7/21/23 17:10, Michael S. Tsirkin wrote:
> >>> On Fri, Jul 21, 2023 at 04:58:04PM +0200, Maxime Coquelin wrote:
>
>
> On 7/21/23 16:45, Michael S. Tsirkin wrote:
> > On Fri, Jul 21, 2023 at 04:37:00PM +0200, Maxime Coquelin wrote:
> >>
> >>
> >> On 7/20/23 23:02, Michael S. Tsirkin wrote:
> >>> On Thu, Jul 20, 2023 at 01:26:20PM -0700, Shannon Nelson wrote:
> On 7/20/23 1:38 AM, Jason Wang wrote:
> >
> > Adding cond_resched() to the command waiting loop for a better
> > co-operation with the scheduler. This allows to give CPU a breath to
> > run other task(workqueue) instead of busy looping when preemption is
> > not allowed on a device whose CVQ might be slow.
> >
> > Signed-off-by: Jason Wang
>
> This still leaves hung processes, but at least it doesn't pin the
> CPU any
> more. Thanks.
> Reviewed-by: Shannon Nelson
>
> >>>
> >>> I'd like to see a full solution
> >>> 1- block until interrupt
> >>
> >> Would it make sense to also have a timeout?
> >> And when timeout expires, set FAILED bit in device status?
> >
> > virtio spec does not set any limits on the timing of vq
> > processing.
>
> Indeed, but I thought the driver could decide it is too long for it.
>
> The issue is we keep waiting with rtnl locked, it can quickly make the
> system unusable.
> >>>
> >>> if this is a problem we should find a way not to keep rtnl
> >>> locked indefinitely.
> >>
> >> From the tests I have done, I think it is. With OVS, a reconfiguration is
> >> performed when the VDUSE device is added, and when a MLX5 device is
> >> in the same bridge, it ends up doing an ioctl() that tries to take the
> >> rtnl lock. In this configuration, it is not possible to kill OVS because
> >> it is stuck trying to acquire rtnl lock for mlx5 that is held by virtio-
> >> net.
> >
> > So for sure, we can queue up the work and process it later.
> > The somewhat tricky part is limiting the memory consumption.
> >
> >
>
>
> Hi Jason
>
> Excuse me, is there any plan for when will v5 patch series be sent out?
> Will the v5 patches solve the problem of ctrlvq's infinite poll for
> buggy devices?
We agree to harden VDUSE and,
It would be hard if we try to solve it at the virtio-net level, see
the discussions before. It might require support from various layers
(e.g networking core etc).
We can use workqueue etc as a mitigation. If Michael is fine with
this, I can post v5.
Thanks
>
> Thanks
> Feng
>
> >>>
> >>> 2- still handle surprise removal correctly by waking in that case
> >>>
> >>>
> >>>
> > ---
> > drivers/net/virtio_net.c | 4 +++-
> > 1 file changed, 3 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
> > index 9f3b1d6ac33d..e7533f29b219 100644
> > --- a/drivers/net/virtio_net.c
> > +++ b/drivers/net/virtio_net.c
> > @@ -2314,8 +2314,10 @@ static bool virtnet_send_command(struct
> > virtnet_info *vi, u8 class, u8 cmd,
> > * into the hypervisor, so the request should be
> > handled immediately.
> > */
> > while (!virtqueue_get_buf(vi->cvq, &tmp) &&
> > - !virtqueue_is_broken(vi->cvq))
> > + !virtqueue_is_broken(vi->cvq)) {
> > + cond_resched();
> > cpu_relax();
> > + }
> >
> > return vi->ctrl->status == VIRTIO_NET_OK;
> > }
> > --
> > 2.39.3
> >
> > ___
> > Virtualization mailing list
> > Virtualization@lists.linux-foundation.org
> > https://lists.linuxfoundation.org/mailman/listinfo/virtualization
> >>>
> >
> >>>
> >
> > ___
> > Virtualization mailing list
> > Virtualization@lists.linux-foundation.org
> > https://lists.linuxfoundation.org/mailman/listinfo/virtualization
>
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH net v2 5/6] virtio-net: fix the vq coalescing setting for vq resize
On Thu, Oct 5, 2023 at 2:12 PM Heng Qi wrote:
>
>
>
> 在 2023/10/3 下午6:41, Paolo Abeni 写道:
> > On Mon, 2023-09-25 at 15:53 +0800, Heng Qi wrote:
> >> According to the definition of virtqueue coalescing spec[1]:
> >>
> >>Upon disabling and re-enabling a transmit virtqueue, the device MUST set
> >>the coalescing parameters of the virtqueue to those configured through
> >> the
> >>VIRTIO_NET_CTRL_NOTF_COAL_TX_SET command, or, if the driver did not set
> >>any TX coalescing parameters, to 0.
> >>
> >>Upon disabling and re-enabling a receive virtqueue, the device MUST set
> >>the coalescing parameters of the virtqueue to those configured through
> >> the
> >>VIRTIO_NET_CTRL_NOTF_COAL_RX_SET command, or, if the driver did not set
> >>any RX coalescing parameters, to 0.
> >>
> >> We need to add this setting for vq resize (ethtool -G) where vq_reset
> >> happens.
> >>
> >> [1] https://lists.oasis-open.org/archives/virtio-dev/202303/msg00415.html
> >>
> >> Fixes: 394bd87764b6 ("virtio_net: support per queue interrupt coalesce
> >> command")
> >> Cc: Gavin Li
> >> Signed-off-by: Heng Qi
> > @Jason, since you commented on v1, waiting for your ack.
> >
> >> ---
> >> drivers/net/virtio_net.c | 27 +++
> >> 1 file changed, 27 insertions(+)
> >>
> >> diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
> >> index 12ec3ae19b60..cb19b224419b 100644
> >> --- a/drivers/net/virtio_net.c
> >> +++ b/drivers/net/virtio_net.c
> >> @@ -2855,6 +2855,9 @@ static void virtnet_get_ringparam(struct net_device
> >> *dev,
> >> ring->tx_pending = virtqueue_get_vring_size(vi->sq[0].vq);
> >> }
> >>
> >> +static int virtnet_send_ctrl_coal_vq_cmd(struct virtnet_info *vi,
> >> + u16 vqn, u32 max_usecs, u32
> >> max_packets);
> >> +
> >> static int virtnet_set_ringparam(struct net_device *dev,
> >> struct ethtool_ringparam *ring,
> >> struct kernel_ethtool_ringparam *kernel_ring,
> >> @@ -2890,12 +2893,36 @@ static int virtnet_set_ringparam(struct net_device
> >> *dev,
> >> err = virtnet_tx_resize(vi, sq, ring->tx_pending);
> >> if (err)
> >> return err;
> >> +
> >> +/* Upon disabling and re-enabling a transmit
> >> virtqueue, the device must
> >> + * set the coalescing parameters of the virtqueue to
> >> those configured
> >> + * through the VIRTIO_NET_CTRL_NOTF_COAL_TX_SET
> >> command, or, if the driver
> >> + * did not set any TX coalescing parameters, to 0.
> >> + */
> >> +err = virtnet_send_ctrl_coal_vq_cmd(vi, txq2vq(i),
> >> +
> >> vi->intr_coal_tx.max_usecs,
> >> +
> >> vi->intr_coal_tx.max_packets);
> >> +if (err)
> >> +return err;
> >> +/* Save parameters */
> > As a very minor nit, I guess the comment could be dropped here (similar
> > to patch 4/6). @Heng Qi: please don't repost just for this, let's wait
> > for Jason' comments.
>
> Ok. We are currently on the National Day holiday, Jason may reply in a
> few days, thanks!
With the comments removed.
Acked-by: Jason Wang
Thanks
>
> > Cheers,
> >
> > Paolo
>
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH net v2 4/6] virtio-net: fix per queue coalescing parameter setting
On Mon, Sep 25, 2023 at 3:53 PM Heng Qi wrote:
>
> When the user sets a non-zero coalescing parameter to 0 for a specific
> virtqueue, it does not work as expected, so let's fix this.
>
> Fixes: 394bd87764b6 ("virtio_net: support per queue interrupt coalesce
> command")
> Reported-by: Xiaoming Zhao
> Cc: Gavin Li
> Signed-off-by: Heng Qi
Acked-by: Jason Wang
Thanks
> ---
> v1->v2:
> 1. Remove useless comments.
>
> drivers/net/virtio_net.c | 36
> 1 file changed, 16 insertions(+), 20 deletions(-)
>
> diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
> index 6120dd5343dd..12ec3ae19b60 100644
> --- a/drivers/net/virtio_net.c
> +++ b/drivers/net/virtio_net.c
> @@ -3296,27 +3296,23 @@ static int virtnet_send_notf_coal_vq_cmds(struct
> virtnet_info *vi,
> {
> int err;
>
> - if (ec->rx_coalesce_usecs || ec->rx_max_coalesced_frames) {
> - err = virtnet_send_ctrl_coal_vq_cmd(vi, rxq2vq(queue),
> - ec->rx_coalesce_usecs,
> -
> ec->rx_max_coalesced_frames);
> - if (err)
> - return err;
> - /* Save parameters */
> - vi->rq[queue].intr_coal.max_usecs = ec->rx_coalesce_usecs;
> - vi->rq[queue].intr_coal.max_packets =
> ec->rx_max_coalesced_frames;
> - }
> + err = virtnet_send_ctrl_coal_vq_cmd(vi, rxq2vq(queue),
> + ec->rx_coalesce_usecs,
> + ec->rx_max_coalesced_frames);
> + if (err)
> + return err;
>
> - if (ec->tx_coalesce_usecs || ec->tx_max_coalesced_frames) {
> - err = virtnet_send_ctrl_coal_vq_cmd(vi, txq2vq(queue),
> - ec->tx_coalesce_usecs,
> -
> ec->tx_max_coalesced_frames);
> - if (err)
> - return err;
> - /* Save parameters */
> - vi->sq[queue].intr_coal.max_usecs = ec->tx_coalesce_usecs;
> - vi->sq[queue].intr_coal.max_packets =
> ec->tx_max_coalesced_frames;
> - }
> + vi->rq[queue].intr_coal.max_usecs = ec->rx_coalesce_usecs;
> + vi->rq[queue].intr_coal.max_packets = ec->rx_max_coalesced_frames;
> +
> + err = virtnet_send_ctrl_coal_vq_cmd(vi, txq2vq(queue),
> + ec->tx_coalesce_usecs,
> + ec->tx_max_coalesced_frames);
> + if (err)
> + return err;
> +
> + vi->sq[queue].intr_coal.max_usecs = ec->tx_coalesce_usecs;
> + vi->sq[queue].intr_coal.max_packets = ec->tx_max_coalesced_frames;
>
> return 0;
> }
> --
> 2.19.1.6.gb485710b
>
>
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [RFC v2 3/4] vduse: update the vq_info in ioctl
On Fri, Sep 29, 2023 at 5:12 PM Maxime Coquelin
wrote:
>
>
>
> On 9/12/23 09:39, Jason Wang wrote:
> > On Tue, Sep 12, 2023 at 11:00 AM Cindy Lu wrote:
> >>
> >> In VDUSE_VQ_GET_INFO, the driver will sync the last_avail_idx
> >> with reconnect info, After mapping the reconnect pages to userspace
> >> The userspace App will update the reconnect_time in
> >> struct vhost_reconnect_vring, If this is not 0 then it means this
> >> vq is reconnected and will update the last_avail_idx
> >>
> >> Signed-off-by: Cindy Lu
> >> ---
> >> drivers/vdpa/vdpa_user/vduse_dev.c | 13 +
> >> include/uapi/linux/vduse.h | 6 ++
> >> 2 files changed, 19 insertions(+)
> >>
> >> diff --git a/drivers/vdpa/vdpa_user/vduse_dev.c
> >> b/drivers/vdpa/vdpa_user/vduse_dev.c
> >> index 2c69f4004a6e..680b23dbdde2 100644
> >> --- a/drivers/vdpa/vdpa_user/vduse_dev.c
> >> +++ b/drivers/vdpa/vdpa_user/vduse_dev.c
> >> @@ -1221,6 +1221,8 @@ static long vduse_dev_ioctl(struct file *file,
> >> unsigned int cmd,
> >> struct vduse_vq_info vq_info;
> >> struct vduse_virtqueue *vq;
> >> u32 index;
> >> + struct vdpa_reconnect_info *area;
> >> + struct vhost_reconnect_vring *vq_reconnect;
> >>
> >> ret = -EFAULT;
> >> if (copy_from_user(&vq_info, argp, sizeof(vq_info)))
> >> @@ -1252,6 +1254,17 @@ static long vduse_dev_ioctl(struct file *file,
> >> unsigned int cmd,
> >>
> >> vq_info.ready = vq->ready;
> >>
> >> + area = &vq->reconnect_info;
> >> +
> >> + vq_reconnect = (struct vhost_reconnect_vring *)area->vaddr;
> >> + /*check if the vq is reconnect, if yes then update the
> >> last_avail_idx*/
> >> + if ((vq_reconnect->last_avail_idx !=
> >> +vq_info.split.avail_index) &&
> >> + (vq_reconnect->reconnect_time != 0)) {
> >> + vq_info.split.avail_index =
> >> + vq_reconnect->last_avail_idx;
> >> + }
> >> +
> >> ret = -EFAULT;
> >> if (copy_to_user(argp, &vq_info, sizeof(vq_info)))
> >> break;
> >> diff --git a/include/uapi/linux/vduse.h b/include/uapi/linux/vduse.h
> >> index 11bd48c72c6c..d585425803fd 100644
> >> --- a/include/uapi/linux/vduse.h
> >> +++ b/include/uapi/linux/vduse.h
> >> @@ -350,4 +350,10 @@ struct vduse_dev_response {
> >> };
> >> };
> >>
> >> +struct vhost_reconnect_vring {
> >> + __u16 reconnect_time;
> >> + __u16 last_avail_idx;
> >> + _Bool avail_wrap_counter;
> >
> > Please add a comment for each field.
> >
> > And I never saw _Bool is used in uapi before, maybe it's better to
> > pack it with last_avail_idx into a __u32.
>
> Better as two distincts __u16 IMHO.
Fine with me.
Thanks
>
> Thanks,
> Maxime
>
> >
> > Btw, do we need to track inflight descriptors as well?
> >
> > Thanks
> >
> >> +};
> >> +
> >> #endif /* _UAPI_VDUSE_H_ */
> >> --
> >> 2.34.3
> >>
> >
>
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH vhost] virtio_net: fix the missing of the dma cpu sync
On Wed, Sep 27, 2023 at 1:53 PM Xuan Zhuo wrote:
>
> Commit 295525e29a5b ("virtio_net: merge dma operations when filling
> mergeable buffers") unmaps the buffer with DMA_ATTR_SKIP_CPU_SYNC when
> the dma->ref is zero. We do that with DMA_ATTR_SKIP_CPU_SYNC, because we
> do not want to do the sync for the entire page_frag. But that misses the
> sync for the current area.
>
> This patch does cpu sync regardless of whether the ref is zero or not.
>
> Fixes: 295525e29a5b ("virtio_net: merge dma operations when filling mergeable
> buffers")
> Reported-by: Michael Roth
> Closes: http://lore.kernel.org/all/20230926130451.axgodaa6tvwqs...@amd.com
> Signed-off-by: Xuan Zhuo
Acked-by: Jason Wang
Thanks
> ---
> drivers/net/virtio_net.c | 14 +++---
> 1 file changed, 7 insertions(+), 7 deletions(-)
>
> diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
> index 98dc9b49d56b..9ece27dc5144 100644
> --- a/drivers/net/virtio_net.c
> +++ b/drivers/net/virtio_net.c
> @@ -589,16 +589,16 @@ static void virtnet_rq_unmap(struct receive_queue *rq,
> void *buf, u32 len)
>
> --dma->ref;
>
> - if (dma->ref) {
> - if (dma->need_sync && len) {
> - offset = buf - (head + sizeof(*dma));
> + if (dma->need_sync && len) {
> + offset = buf - (head + sizeof(*dma));
>
> - virtqueue_dma_sync_single_range_for_cpu(rq->vq,
> dma->addr, offset,
> - len,
> DMA_FROM_DEVICE);
> - }
> + virtqueue_dma_sync_single_range_for_cpu(rq->vq, dma->addr,
> + offset, len,
> + DMA_FROM_DEVICE);
> + }
>
> + if (dma->ref)
> return;
> - }
>
> virtqueue_dma_unmap_single_attrs(rq->vq, dma->addr, dma->len,
> DMA_FROM_DEVICE,
> DMA_ATTR_SKIP_CPU_SYNC);
> --
> 2.32.0.3.g01195cf9f
>
>
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH 1/2] tools/virtio: Add dma sync api for virtio test
On Tue, Sep 26, 2023 at 1:00 PM wrote:
>
> From: Liming Wu
>
> Fixes: 8bd2f71054bd ("virtio_ring: introduce dma sync api for virtqueue")
> also add dma sync api for virtio test.
>
> Signed-off-by: Liming Wu
Acked-by: Jason Wang
Thanks
> ---
> tools/virtio/linux/dma-mapping.h | 12
> 1 file changed, 12 insertions(+)
>
> diff --git a/tools/virtio/linux/dma-mapping.h
> b/tools/virtio/linux/dma-mapping.h
> index 834a90bd3270..822ecaa8e4df 100644
> --- a/tools/virtio/linux/dma-mapping.h
> +++ b/tools/virtio/linux/dma-mapping.h
> @@ -24,11 +24,23 @@ enum dma_data_direction {
> #define dma_map_page(d, p, o, s, dir) (page_to_phys(p) + (o))
>
> #define dma_map_single(d, p, s, dir) (virt_to_phys(p))
> +#define dma_map_single_attrs(d, p, s, dir, a) (virt_to_phys(p))
> #define dma_mapping_error(...) (0)
>
> #define dma_unmap_single(d, a, s, r) do { (void)(d); (void)(a); (void)(s);
> (void)(r); } while (0)
> #define dma_unmap_page(d, a, s, r) do { (void)(d); (void)(a); (void)(s);
> (void)(r); } while (0)
>
> +#define sg_dma_address(sg) (0)
> +#define dma_need_sync(v, a) (0)
> +#define dma_unmap_single_attrs(d, a, s, r, t) do { \
> + (void)(d); (void)(a); (void)(s); (void)(r); (void)(t); \
> +} while (0)
> +#define dma_sync_single_range_for_cpu(d, a, o, s, r) do { \
> + (void)(d); (void)(a); (void)(o); (void)(s); (void)(r); \
> +} while (0)
> +#define dma_sync_single_range_for_device(d, a, o, s, r) do { \
> + (void)(d); (void)(a); (void)(o); (void)(s); (void)(r); \
> +} while (0)
> #define dma_max_mapping_size(...) SIZE_MAX
>
> #endif
> --
> 2.34.1
>
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH 2/2] tools/virtio: Add hints when module is not installed
On Tue, Sep 26, 2023 at 1:00 PM wrote:
>
> From: Liming Wu
>
> Need to insmod vhost_test.ko before run virtio_test.
> Give some hints to users.
>
> Signed-off-by: Liming Wu
> ---
> tools/virtio/virtio_test.c | 4
> 1 file changed, 4 insertions(+)
>
> diff --git a/tools/virtio/virtio_test.c b/tools/virtio/virtio_test.c
> index 028f54e6854a..ce2c4d93d735 100644
> --- a/tools/virtio/virtio_test.c
> +++ b/tools/virtio/virtio_test.c
> @@ -135,6 +135,10 @@ static void vdev_info_init(struct vdev_info* dev,
> unsigned long long features)
> dev->buf = malloc(dev->buf_size);
> assert(dev->buf);
> dev->control = open("/dev/vhost-test", O_RDWR);
> +
> + if (dev->control < 0)
> + fprintf(stderr, "Install vhost_test module" \
> + "(./vhost_test/vhost_test.ko) firstly\n");
There should be many other reasons to fail for open().
Let's use strerror()?
Thanks
> assert(dev->control >= 0);
> r = ioctl(dev->control, VHOST_SET_OWNER, NULL);
> assert(r >= 0);
> --
> 2.34.1
>
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH 2/2] virtio_pci: add build offset check for the new common cfg items
On Mon, Sep 25, 2023 at 11:58 AM Xuan Zhuo wrote: > > Add checks to the check_offsets(void) for queue_notify_data and > queue_reset. > > Signed-off-by: Xuan Zhuo Acked-by: Jason Wang Thanks > --- > drivers/virtio/virtio_pci_modern_dev.c | 4 > 1 file changed, 4 insertions(+) > > diff --git a/drivers/virtio/virtio_pci_modern_dev.c > b/drivers/virtio/virtio_pci_modern_dev.c > index ef6667de1b38..47cb41160e1a 100644 > --- a/drivers/virtio/virtio_pci_modern_dev.c > +++ b/drivers/virtio/virtio_pci_modern_dev.c > @@ -203,6 +203,10 @@ static inline void check_offsets(void) > offsetof(struct virtio_pci_common_cfg, queue_used_lo)); > BUILD_BUG_ON(VIRTIO_PCI_COMMON_Q_USEDHI != > offsetof(struct virtio_pci_common_cfg, queue_used_hi)); > + BUILD_BUG_ON(VIRTIO_PCI_COMMON_Q_NDATA != > +offsetof(struct virtio_pci_modern_common_cfg, > queue_notify_data)); > + BUILD_BUG_ON(VIRTIO_PCI_COMMON_Q_RESET != > +offsetof(struct virtio_pci_modern_common_cfg, > queue_reset)); > } > > /* > -- > 2.32.0.3.g01195cf9f > ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH 1/2] virtio_pci: fix the common map size and add check for vq-reset
On Mon, Sep 25, 2023 at 11:58 AM Xuan Zhuo wrote:
>
> Now, the function vp_modern_map_capability() takes the size parameter,
> which corresponds to the size of virtio_pci_common_cfg. As a result, the
> pci_iomap_range() function maps the memory area for
> virtio_pci_common_cfg.
>
> However, if the _F_RING_RESET is negotiated, the extra items will be
> used. Therefore, we need to use the size of virtio_pci_modre_common_cfg
> to map more space.
>
> Meanwhile, I have introduced a new variable called common_len in the
> mdev. This allows us to check common_len when accessing the new item of
> virtio_pci_modre_common_cfg.
>
> Signed-off-by: Xuan Zhuo
> ---
> drivers/virtio/virtio_pci_modern_dev.c | 8 ++--
> include/linux/virtio_pci_modern.h | 1 +
> 2 files changed, 7 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/virtio/virtio_pci_modern_dev.c
> b/drivers/virtio/virtio_pci_modern_dev.c
> index aad7d9296e77..ef6667de1b38 100644
> --- a/drivers/virtio/virtio_pci_modern_dev.c
> +++ b/drivers/virtio/virtio_pci_modern_dev.c
> @@ -291,8 +291,8 @@ int vp_modern_probe(struct virtio_pci_modern_device *mdev)
> err = -EINVAL;
> mdev->common = vp_modern_map_capability(mdev, common,
> sizeof(struct virtio_pci_common_cfg), 4,
> - 0, sizeof(struct virtio_pci_common_cfg),
> - NULL, NULL);
> + 0, sizeof(struct
> virtio_pci_modern_common_cfg),
> + &mdev->common_len, NULL);
> if (!mdev->common)
> goto err_map_common;
> mdev->isr = vp_modern_map_capability(mdev, isr, sizeof(u8), 1,
> @@ -493,6 +493,8 @@ int vp_modern_get_queue_reset(struct
> virtio_pci_modern_device *mdev, u16 index)
> {
> struct virtio_pci_modern_common_cfg __iomem *cfg;
>
> + BUG_ON(mdev->common_len < offsetofend(struct
> virtio_pci_modern_common_cfg, queue_reset));
Instead of using BUG for buggy hardware, why not simply fail the probe
in this case?
Thanks
> +
> cfg = (struct virtio_pci_modern_common_cfg __iomem *)mdev->common;
>
> vp_iowrite16(index, &cfg->cfg.queue_select);
> @@ -509,6 +511,8 @@ void vp_modern_set_queue_reset(struct
> virtio_pci_modern_device *mdev, u16 index)
> {
> struct virtio_pci_modern_common_cfg __iomem *cfg;
>
> + BUG_ON(mdev->common_len < offsetofend(struct
> virtio_pci_modern_common_cfg, queue_reset));
> +
> cfg = (struct virtio_pci_modern_common_cfg __iomem *)mdev->common;
>
> vp_iowrite16(index, &cfg->cfg.queue_select);
> diff --git a/include/linux/virtio_pci_modern.h
> b/include/linux/virtio_pci_modern.h
> index 067ac1d789bc..edf62bae0474 100644
> --- a/include/linux/virtio_pci_modern.h
> +++ b/include/linux/virtio_pci_modern.h
> @@ -28,6 +28,7 @@ struct virtio_pci_modern_device {
> /* So we can sanity-check accesses. */
> size_t notify_len;
> size_t device_len;
> + size_t common_len;
>
> /* Capability for when we need to map notifications per-vq. */
> int notify_map_cap;
> --
> 2.32.0.3.g01195cf9f
>
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH vfio 11/11] vfio/virtio: Introduce a vfio driver over virtio devices
On Tue, Sep 26, 2023 at 7:49 PM Michael S. Tsirkin wrote: > > On Tue, Sep 26, 2023 at 10:32:39AM +0800, Jason Wang wrote: > > It's the implementation details in legacy. The device needs to make > > sure (reset) the driver can work (is done before get_status return). > > I think that there's no way to make it reliably work for all legacy drivers. Yes, we may have ancient drivers. > > They just assumed a software backend and did not bother with DMA > ordering. You can try to avoid resets, they are not that common so > things will tend to mostly work if you don't stress them to much with > things like hot plug/unplug in a loop. Or you can try to use a driver > after 2011 which is more aware of hardware ordering and flushes the > reset write with a read. One of these two tricks, I think, is the magic > behind the device exposing memory bar 0 that you mention. Right this is what I see for hardware legacy devices shipped by some cloud vendors. Thanks > > -- > MST > ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH 09/16] vdpa/mlx5: Allow creation/deletion of any given mr struct
On Tue, Sep 26, 2023 at 3:21 PM Dragos Tatulea wrote: > > On Tue, 2023-09-26 at 12:44 +0800, Jason Wang wrote: > > On Tue, Sep 12, 2023 at 9:02 PM Dragos Tatulea wrote: > > > > > > This patch adapts the mr creation/deletion code to be able to work with > > > any given mr struct pointer. All the APIs are adapted to take an extra > > > parameter for the mr. > > > > > > mlx5_vdpa_create/delete_mr doesn't need a ASID parameter anymore. The > > > check is done in the caller instead (mlx5_set_map). > > > > > > This change is needed for a followup patch which will introduce an > > > additional mr for the vq descriptor data. > > > > > > Signed-off-by: Dragos Tatulea > > > --- > > > > Thinking of this decoupling I think I have a question. > > > > We advertise 2 address spaces and 2 groups. So we actually don't know > > for example which address spaces will be used by dvq. > > > > And actually we allow the user space to do something like > > > > set_group_asid(dvq_group, 0) > > set_map(0) > > set_group_asid(dvq_group, 1) > > set_map(1) > > > > I wonder if the decoupling like this patch can work and why. > > > This scenario could indeed work. Especially if you look at the 13'th patch [0] > where hw support is added. Are you wondering if this should work at all or if > it > should be blocked? It would be great if it can work with the following patches. But at least for this patch, it seems not: For example, what happens if we switch back to group 0 for dvq? set_group_asid(dvq_group, 0) set_map(0) set_group_asid(dvq_group, 1) set_map(1) // here we destroy the mr created for asid 0 set_group_asid(dvq_group, 0) Btw, if this is a new issue, I haven't checked whether or not it exists before this series (if yes, we can fix on top). > > > It looks to me the most easy way is to let each AS be backed by an MR. > > Then we don't even need to care about the dvq, cvq. > That's what this patch series dowes. Good to know this, I will review the series. Thanks > > Thanks, > Dragos > > [0]https://lore.kernel.org/virtualization/20230912130132.561193-14-dtatu...@nvidia.com/T/#u ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH v2 1/2] tools/virtio: Add dma sync api for virtio test
On Sat, 7 Oct 2023 14:55:46 +0800, liming...@jaguarmicro.com wrote:
> From: Liming Wu
>
> Fixes: 8bd2f71054bd ("virtio_ring: introduce dma sync api for virtqueue")
> also add dma sync api for virtio test.
>
> Signed-off-by: Liming Wu
You should post a new thread.
Thanks.
> ---
> tools/virtio/linux/dma-mapping.h | 12
> 1 file changed, 12 insertions(+)
>
> diff --git a/tools/virtio/linux/dma-mapping.h
> b/tools/virtio/linux/dma-mapping.h
> index 834a90bd3270..822ecaa8e4df 100644
> --- a/tools/virtio/linux/dma-mapping.h
> +++ b/tools/virtio/linux/dma-mapping.h
> @@ -24,11 +24,23 @@ enum dma_data_direction {
> #define dma_map_page(d, p, o, s, dir) (page_to_phys(p) + (o))
>
> #define dma_map_single(d, p, s, dir) (virt_to_phys(p))
> +#define dma_map_single_attrs(d, p, s, dir, a) (virt_to_phys(p))
> #define dma_mapping_error(...) (0)
>
> #define dma_unmap_single(d, a, s, r) do { (void)(d); (void)(a); (void)(s);
> (void)(r); } while (0)
> #define dma_unmap_page(d, a, s, r) do { (void)(d); (void)(a); (void)(s);
> (void)(r); } while (0)
>
> +#define sg_dma_address(sg) (0)
> +#define dma_need_sync(v, a) (0)
> +#define dma_unmap_single_attrs(d, a, s, r, t) do { \
> + (void)(d); (void)(a); (void)(s); (void)(r); (void)(t); \
> +} while (0)
> +#define dma_sync_single_range_for_cpu(d, a, o, s, r) do { \
> + (void)(d); (void)(a); (void)(o); (void)(s); (void)(r); \
> +} while (0)
> +#define dma_sync_single_range_for_device(d, a, o, s, r) do { \
> + (void)(d); (void)(a); (void)(o); (void)(s); (void)(r); \
> +} while (0)
> #define dma_max_mapping_size(...) SIZE_MAX
>
> #endif
> --
> 2.34.1
>
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
