We used to busy waiting on the cvq command this tends to be
problematic since there no way for to schedule another process which
may serve for the control virtqueue. This might be the case when the
control virtqueue is emulated by software. This patch switches to use
completion to allow the CPU to sleep instead of busy waiting for the
cvq command.
Signed-off-by: Jason Wang <[email protected]>
---
Changes since V1:
- use completion for simplicity
- don't try to harden the CVQ command which requires more thought
Changes since RFC:
- break the device when timeout
- get buffer manually since the virtio core check more_used() instead
---
drivers/net/virtio_net.c | 21 ++++++++++++++-------
1 file changed, 14 insertions(+), 7 deletions(-)
diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
index 2e56bbf86894..d3eb8fd6c9dc 100644
--- a/drivers/net/virtio_net.c
+++ b/drivers/net/virtio_net.c
@@ -19,6 +19,7 @@
#include <linux/average.h>
#include <linux/filter.h>
#include <linux/kernel.h>
+#include <linux/completion.h>
#include <net/route.h>
#include <net/xdp.h>
#include <net/net_failover.h>
@@ -295,6 +296,8 @@ struct virtnet_info {
/* failover when STANDBY feature enabled */
struct failover *failover;
+
+ struct completion completion;
};
struct padded_vnet_hdr {
@@ -1709,6 +1712,13 @@ static bool try_fill_recv(struct virtnet_info *vi, struct receive_queue *rq,
return !oom;
}
+static void virtnet_cvq_done(struct virtqueue *cvq)
+{
+ struct virtnet_info *vi = cvq->vdev->priv;
+
+ complete(&vi->completion);
+}
+
static void skb_recv_done(struct virtqueue *rvq)
{
struct virtnet_info *vi = rvq->vdev->priv;
@@ -2169,12 +2179,8 @@ static bool virtnet_send_command(struct virtnet_info *vi, u8 class, u8 cmd,
if (unlikely(!virtqueue_kick(vi->cvq)))
return vi->ctrl->status == VIRTIO_NET_OK;
- /* Spin for a response, the kick causes an ioport write, trapping
- * into the hypervisor, so the request should be handled immediately.
- */
- while (!virtqueue_get_buf(vi->cvq, &tmp) &&
- !virtqueue_is_broken(vi->cvq))
- cpu_relax();
+ wait_for_completion(&vi->completion);
+ virtqueue_get_buf(vi->cvq, &tmp);
return vi->ctrl->status == VIRTIO_NET_OK;
}
@@ -3672,7 +3678,7 @@ static int virtnet_find_vqs(struct virtnet_info *vi)
/* Parameters for control virtqueue, if any */
if (vi->has_cvq) {
- callbacks[total_vqs - 1] = NULL;
+ callbacks[total_vqs - 1] = virtnet_cvq_done;
names[total_vqs - 1] = "control";
}
@@ -4122,6 +4128,7 @@ static int virtnet_probe(struct virtio_device *vdev)
if (vi->has_rss || vi->has_rss_hash_report)
virtnet_init_default_rss(vi);
+ init_completion(&vi->completion);
enable_rx_mode_work(vi);
/* serialize netdev register + virtio_device_ready() with ndo_open() */
--
2.25.1
On Thu, 13 Apr 2023 14:40:27 +0800, Jason Wang <[email protected]> wrote:
> We used to busy waiting on the cvq command this tends to be
> problematic since there no way for to schedule another process which
> may serve for the control virtqueue. This might be the case when the
> control virtqueue is emulated by software. This patch switches to use
> completion to allow the CPU to sleep instead of busy waiting for the
> cvq command.
>
> Signed-off-by: Jason Wang <[email protected]>
> ---
> Changes since V1:
> - use completion for simplicity
> - don't try to harden the CVQ command which requires more thought
> Changes since RFC:
> - break the device when timeout
> - get buffer manually since the virtio core check more_used() instead
> ---
> drivers/net/virtio_net.c | 21 ++++++++++++++-------
> 1 file changed, 14 insertions(+), 7 deletions(-)
>
> diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
> index 2e56bbf86894..d3eb8fd6c9dc 100644
> --- a/drivers/net/virtio_net.c
> +++ b/drivers/net/virtio_net.c
> @@ -19,6 +19,7 @@
> #include <linux/average.h>
> #include <linux/filter.h>
> #include <linux/kernel.h>
> +#include <linux/completion.h>
> #include <net/route.h>
> #include <net/xdp.h>
> #include <net/net_failover.h>
> @@ -295,6 +296,8 @@ struct virtnet_info {
>
> /* failover when STANDBY feature enabled */
> struct failover *failover;
> +
> + struct completion completion;
> };
>
> struct padded_vnet_hdr {
> @@ -1709,6 +1712,13 @@ static bool try_fill_recv(struct virtnet_info *vi, struct receive_queue *rq,
> return !oom;
> }
>
> +static void virtnet_cvq_done(struct virtqueue *cvq)
> +{
> + struct virtnet_info *vi = cvq->vdev->priv;
> +
> + complete(&vi->completion);
> +}
> +
> static void skb_recv_done(struct virtqueue *rvq)
> {
> struct virtnet_info *vi = rvq->vdev->priv;
> @@ -2169,12 +2179,8 @@ static bool virtnet_send_command(struct virtnet_info *vi, u8 class, u8 cmd,
> if (unlikely(!virtqueue_kick(vi->cvq)))
> return vi->ctrl->status == VIRTIO_NET_OK;
>
> - /* Spin for a response, the kick causes an ioport write, trapping
> - * into the hypervisor, so the request should be handled immediately.
> - */
> - while (!virtqueue_get_buf(vi->cvq, &tmp) &&
> - !virtqueue_is_broken(vi->cvq))
> - cpu_relax();
> + wait_for_completion(&vi->completion);
> + virtqueue_get_buf(vi->cvq, &tmp);
>
> return vi->ctrl->status == VIRTIO_NET_OK;
> }
> @@ -3672,7 +3678,7 @@ static int virtnet_find_vqs(struct virtnet_info *vi)
>
> /* Parameters for control virtqueue, if any */
> if (vi->has_cvq) {
> - callbacks[total_vqs - 1] = NULL;
> + callbacks[total_vqs - 1] = virtnet_cvq_done;
This depends the interrupt, right?
I worry that there may be some devices that may not support interruption on cvq.
Although this may not be in line with SPEC, it may cause problem on the devices
that can work normally at present.
Thanks.
> names[total_vqs - 1] = "control";
> }
>
> @@ -4122,6 +4128,7 @@ static int virtnet_probe(struct virtio_device *vdev)
> if (vi->has_rss || vi->has_rss_hash_report)
> virtnet_init_default_rss(vi);
>
> + init_completion(&vi->completion);
> enable_rx_mode_work(vi);
>
> /* serialize netdev register + virtio_device_ready() with ndo_open() */
> --
> 2.25.1
>
On Thu, Apr 13, 2023 at 3:31 PM Xuan Zhuo <[email protected]> wrote:
>
> On Thu, 13 Apr 2023 14:40:27 +0800, Jason Wang <[email protected]> wrote:
> > We used to busy waiting on the cvq command this tends to be
> > problematic since there no way for to schedule another process which
> > may serve for the control virtqueue. This might be the case when the
> > control virtqueue is emulated by software. This patch switches to use
> > completion to allow the CPU to sleep instead of busy waiting for the
> > cvq command.
> >
> > Signed-off-by: Jason Wang <[email protected]>
> > ---
> > Changes since V1:
> > - use completion for simplicity
> > - don't try to harden the CVQ command which requires more thought
> > Changes since RFC:
> > - break the device when timeout
> > - get buffer manually since the virtio core check more_used() instead
> > ---
> > drivers/net/virtio_net.c | 21 ++++++++++++++-------
> > 1 file changed, 14 insertions(+), 7 deletions(-)
> >
> > diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
> > index 2e56bbf86894..d3eb8fd6c9dc 100644
> > --- a/drivers/net/virtio_net.c
> > +++ b/drivers/net/virtio_net.c
> > @@ -19,6 +19,7 @@
> > #include <linux/average.h>
> > #include <linux/filter.h>
> > #include <linux/kernel.h>
> > +#include <linux/completion.h>
> > #include <net/route.h>
> > #include <net/xdp.h>
> > #include <net/net_failover.h>
> > @@ -295,6 +296,8 @@ struct virtnet_info {
> >
> > /* failover when STANDBY feature enabled */
> > struct failover *failover;
> > +
> > + struct completion completion;
> > };
> >
> > struct padded_vnet_hdr {
> > @@ -1709,6 +1712,13 @@ static bool try_fill_recv(struct virtnet_info *vi, struct receive_queue *rq,
> > return !oom;
> > }
> >
> > +static void virtnet_cvq_done(struct virtqueue *cvq)
> > +{
> > + struct virtnet_info *vi = cvq->vdev->priv;
> > +
> > + complete(&vi->completion);
> > +}
> > +
> > static void skb_recv_done(struct virtqueue *rvq)
> > {
> > struct virtnet_info *vi = rvq->vdev->priv;
> > @@ -2169,12 +2179,8 @@ static bool virtnet_send_command(struct virtnet_info *vi, u8 class, u8 cmd,
> > if (unlikely(!virtqueue_kick(vi->cvq)))
> > return vi->ctrl->status == VIRTIO_NET_OK;
> >
> > - /* Spin for a response, the kick causes an ioport write, trapping
> > - * into the hypervisor, so the request should be handled immediately.
> > - */
> > - while (!virtqueue_get_buf(vi->cvq, &tmp) &&
> > - !virtqueue_is_broken(vi->cvq))
> > - cpu_relax();
> > + wait_for_completion(&vi->completion);
> > + virtqueue_get_buf(vi->cvq, &tmp);
> >
> > return vi->ctrl->status == VIRTIO_NET_OK;
> > }
> > @@ -3672,7 +3678,7 @@ static int virtnet_find_vqs(struct virtnet_info *vi)
> >
> > /* Parameters for control virtqueue, if any */
> > if (vi->has_cvq) {
> > - callbacks[total_vqs - 1] = NULL;
> > + callbacks[total_vqs - 1] = virtnet_cvq_done;
>
> This depends the interrupt, right?
Not necessarily, we have ISR for at last PCI:
static irqreturn_t vp_interrupt(int irq, void *opaque)
{
struct virtio_pci_device *vp_dev = opaque;
u8 isr;
/* reading the ISR has the effect of also clearing it so it's very
* important to save off the value. */
isr = ioread8(vp_dev->isr);
...
}
>
> I worry that there may be some devices that may not support interruption on cvq.
Is the device using INTX or MSI-X?
> Although this may not be in line with SPEC, it may cause problem on the devices
> that can work normally at present.
Then the implementation is buggy, it might not work for drivers other
than Linux. Working around such buggy implementation is suboptimal.
Thanks
>
> Thanks.
>
>
> > names[total_vqs - 1] = "control";
> > }
> >
> > @@ -4122,6 +4128,7 @@ static int virtnet_probe(struct virtio_device *vdev)
> > if (vi->has_rss || vi->has_rss_hash_report)
> > virtnet_init_default_rss(vi);
> >
> > + init_completion(&vi->completion);
> > enable_rx_mode_work(vi);
> >
> > /* serialize netdev register + virtio_device_ready() with ndo_open() */
> > --
> > 2.25.1
> >
>
Adding netdev.
On Fri, Apr 14, 2023 at 1:09 PM Jason Wang <[email protected]> wrote:
>
> On Thu, Apr 13, 2023 at 3:31 PM Xuan Zhuo <[email protected]> wrote:
> >
> > On Thu, 13 Apr 2023 14:40:27 +0800, Jason Wang <[email protected]> wrote:
> > > We used to busy waiting on the cvq command this tends to be
> > > problematic since there no way for to schedule another process which
> > > may serve for the control virtqueue. This might be the case when the
> > > control virtqueue is emulated by software. This patch switches to use
> > > completion to allow the CPU to sleep instead of busy waiting for the
> > > cvq command.
> > >
> > > Signed-off-by: Jason Wang <[email protected]>
> > > ---
> > > Changes since V1:
> > > - use completion for simplicity
> > > - don't try to harden the CVQ command which requires more thought
> > > Changes since RFC:
> > > - break the device when timeout
> > > - get buffer manually since the virtio core check more_used() instead
> > > ---
> > > drivers/net/virtio_net.c | 21 ++++++++++++++-------
> > > 1 file changed, 14 insertions(+), 7 deletions(-)
> > >
> > > diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
> > > index 2e56bbf86894..d3eb8fd6c9dc 100644
> > > --- a/drivers/net/virtio_net.c
> > > +++ b/drivers/net/virtio_net.c
> > > @@ -19,6 +19,7 @@
> > > #include <linux/average.h>
> > > #include <linux/filter.h>
> > > #include <linux/kernel.h>
> > > +#include <linux/completion.h>
> > > #include <net/route.h>
> > > #include <net/xdp.h>
> > > #include <net/net_failover.h>
> > > @@ -295,6 +296,8 @@ struct virtnet_info {
> > >
> > > /* failover when STANDBY feature enabled */
> > > struct failover *failover;
> > > +
> > > + struct completion completion;
> > > };
> > >
> > > struct padded_vnet_hdr {
> > > @@ -1709,6 +1712,13 @@ static bool try_fill_recv(struct virtnet_info *vi, struct receive_queue *rq,
> > > return !oom;
> > > }
> > >
> > > +static void virtnet_cvq_done(struct virtqueue *cvq)
> > > +{
> > > + struct virtnet_info *vi = cvq->vdev->priv;
> > > +
> > > + complete(&vi->completion);
> > > +}
> > > +
> > > static void skb_recv_done(struct virtqueue *rvq)
> > > {
> > > struct virtnet_info *vi = rvq->vdev->priv;
> > > @@ -2169,12 +2179,8 @@ static bool virtnet_send_command(struct virtnet_info *vi, u8 class, u8 cmd,
> > > if (unlikely(!virtqueue_kick(vi->cvq)))
> > > return vi->ctrl->status == VIRTIO_NET_OK;
> > >
> > > - /* Spin for a response, the kick causes an ioport write, trapping
> > > - * into the hypervisor, so the request should be handled immediately.
> > > - */
> > > - while (!virtqueue_get_buf(vi->cvq, &tmp) &&
> > > - !virtqueue_is_broken(vi->cvq))
> > > - cpu_relax();
> > > + wait_for_completion(&vi->completion);
> > > + virtqueue_get_buf(vi->cvq, &tmp);
> > >
> > > return vi->ctrl->status == VIRTIO_NET_OK;
> > > }
> > > @@ -3672,7 +3678,7 @@ static int virtnet_find_vqs(struct virtnet_info *vi)
> > >
> > > /* Parameters for control virtqueue, if any */
> > > if (vi->has_cvq) {
> > > - callbacks[total_vqs - 1] = NULL;
> > > + callbacks[total_vqs - 1] = virtnet_cvq_done;
> >
> > This depends the interrupt, right?
>
> Not necessarily, we have ISR for at last PCI:
>
> static irqreturn_t vp_interrupt(int irq, void *opaque)
> {
> struct virtio_pci_device *vp_dev = opaque;
> u8 isr;
>
> /* reading the ISR has the effect of also clearing it so it's very
> * important to save off the value. */
> isr = ioread8(vp_dev->isr);
> ...
> }
>
> >
> > I worry that there may be some devices that may not support interruption on cvq.
>
> Is the device using INTX or MSI-X?
>
> > Although this may not be in line with SPEC, it may cause problem on the devices
> > that can work normally at present.
>
> Then the implementation is buggy, it might not work for drivers other
> than Linux. Working around such buggy implementation is suboptimal.
>
> Thanks
>
> >
> > Thanks.
> >
> >
> > > names[total_vqs - 1] = "control";
> > > }
> > >
> > > @@ -4122,6 +4128,7 @@ static int virtnet_probe(struct virtio_device *vdev)
> > > if (vi->has_rss || vi->has_rss_hash_report)
> > > virtnet_init_default_rss(vi);
> > >
> > > + init_completion(&vi->completion);
> > > enable_rx_mode_work(vi);
> > >
> > > /* serialize netdev register + virtio_device_ready() with ndo_open() */
> > > --
> > > 2.25.1
> > >
> >
On Thu, Apr 13, 2023 at 02:40:27PM +0800, Jason Wang wrote:
> We used to busy waiting on the cvq command this tends to be
> problematic since there no way for to schedule another process which
> may serve for the control virtqueue. This might be the case when the
> control virtqueue is emulated by software. This patch switches to use
> completion to allow the CPU to sleep instead of busy waiting for the
> cvq command.
>
> Signed-off-by: Jason Wang <[email protected]>
> ---
> Changes since V1:
> - use completion for simplicity
> - don't try to harden the CVQ command which requires more thought
> Changes since RFC:
> - break the device when timeout
> - get buffer manually since the virtio core check more_used() instead
> ---
> drivers/net/virtio_net.c | 21 ++++++++++++++-------
> 1 file changed, 14 insertions(+), 7 deletions(-)
>
> diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
> index 2e56bbf86894..d3eb8fd6c9dc 100644
> --- a/drivers/net/virtio_net.c
> +++ b/drivers/net/virtio_net.c
> @@ -19,6 +19,7 @@
> #include <linux/average.h>
> #include <linux/filter.h>
> #include <linux/kernel.h>
> +#include <linux/completion.h>
> #include <net/route.h>
> #include <net/xdp.h>
> #include <net/net_failover.h>
> @@ -295,6 +296,8 @@ struct virtnet_info {
>
> /* failover when STANDBY feature enabled */
> struct failover *failover;
> +
> + struct completion completion;
> };
>
> struct padded_vnet_hdr {
> @@ -1709,6 +1712,13 @@ static bool try_fill_recv(struct virtnet_info *vi, struct receive_queue *rq,
> return !oom;
> }
>
> +static void virtnet_cvq_done(struct virtqueue *cvq)
> +{
> + struct virtnet_info *vi = cvq->vdev->priv;
> +
> + complete(&vi->completion);
> +}
> +
> static void skb_recv_done(struct virtqueue *rvq)
> {
> struct virtnet_info *vi = rvq->vdev->priv;
> @@ -2169,12 +2179,8 @@ static bool virtnet_send_command(struct virtnet_info *vi, u8 class, u8 cmd,
> if (unlikely(!virtqueue_kick(vi->cvq)))
> return vi->ctrl->status == VIRTIO_NET_OK;
>
> - /* Spin for a response, the kick causes an ioport write, trapping
> - * into the hypervisor, so the request should be handled immediately.
> - */
> - while (!virtqueue_get_buf(vi->cvq, &tmp) &&
> - !virtqueue_is_broken(vi->cvq))
> - cpu_relax();
> + wait_for_completion(&vi->completion);
> + virtqueue_get_buf(vi->cvq, &tmp);
>
> return vi->ctrl->status == VIRTIO_NET_OK;
This seems to break surprise removal and other
situations where vq gets broken since callbacks
aren't usually invoked then.
> }
> @@ -3672,7 +3678,7 @@ static int virtnet_find_vqs(struct virtnet_info *vi)
>
> /* Parameters for control virtqueue, if any */
> if (vi->has_cvq) {
> - callbacks[total_vqs - 1] = NULL;
> + callbacks[total_vqs - 1] = virtnet_cvq_done;
> names[total_vqs - 1] = "control";
> }
>
There is a cost to this, in that we are burning an extra MSI vector
for the slow path cvq. if device has 3 vectors, suddenly we can't
allocate vectors for rx and tx, big problem.
So I'm afraid we need to pass a new flag that will share
the config changed interrupt and cvq.
> @@ -4122,6 +4128,7 @@ static int virtnet_probe(struct virtio_device *vdev)
> if (vi->has_rss || vi->has_rss_hash_report)
> virtnet_init_default_rss(vi);
>
> + init_completion(&vi->completion);
> enable_rx_mode_work(vi);
>
> /* serialize netdev register + virtio_device_ready() with ndo_open() */
> --
> 2.25.1
On Wed, May 17, 2023 at 4:54 AM Michael S. Tsirkin <[email protected]> wrote:
>
> On Thu, Apr 13, 2023 at 02:40:27PM +0800, Jason Wang wrote:
> > We used to busy waiting on the cvq command this tends to be
> > problematic since there no way for to schedule another process which
> > may serve for the control virtqueue. This might be the case when the
> > control virtqueue is emulated by software. This patch switches to use
> > completion to allow the CPU to sleep instead of busy waiting for the
> > cvq command.
> >
> > Signed-off-by: Jason Wang <[email protected]>
> > ---
> > Changes since V1:
> > - use completion for simplicity
> > - don't try to harden the CVQ command which requires more thought
> > Changes since RFC:
> > - break the device when timeout
> > - get buffer manually since the virtio core check more_used() instead
> > ---
> > drivers/net/virtio_net.c | 21 ++++++++++++++-------
> > 1 file changed, 14 insertions(+), 7 deletions(-)
> >
> > diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
> > index 2e56bbf86894..d3eb8fd6c9dc 100644
> > --- a/drivers/net/virtio_net.c
> > +++ b/drivers/net/virtio_net.c
> > @@ -19,6 +19,7 @@
> > #include <linux/average.h>
> > #include <linux/filter.h>
> > #include <linux/kernel.h>
> > +#include <linux/completion.h>
> > #include <net/route.h>
> > #include <net/xdp.h>
> > #include <net/net_failover.h>
> > @@ -295,6 +296,8 @@ struct virtnet_info {
> >
> > /* failover when STANDBY feature enabled */
> > struct failover *failover;
> > +
> > + struct completion completion;
> > };
> >
> > struct padded_vnet_hdr {
> > @@ -1709,6 +1712,13 @@ static bool try_fill_recv(struct virtnet_info *vi, struct receive_queue *rq,
> > return !oom;
> > }
> >
> > +static void virtnet_cvq_done(struct virtqueue *cvq)
> > +{
> > + struct virtnet_info *vi = cvq->vdev->priv;
> > +
> > + complete(&vi->completion);
> > +}
> > +
> > static void skb_recv_done(struct virtqueue *rvq)
> > {
> > struct virtnet_info *vi = rvq->vdev->priv;
> > @@ -2169,12 +2179,8 @@ static bool virtnet_send_command(struct virtnet_info *vi, u8 class, u8 cmd,
> > if (unlikely(!virtqueue_kick(vi->cvq)))
> > return vi->ctrl->status == VIRTIO_NET_OK;
> >
> > - /* Spin for a response, the kick causes an ioport write, trapping
> > - * into the hypervisor, so the request should be handled immediately.
> > - */
> > - while (!virtqueue_get_buf(vi->cvq, &tmp) &&
> > - !virtqueue_is_broken(vi->cvq))
> > - cpu_relax();
> > + wait_for_completion(&vi->completion);
> > + virtqueue_get_buf(vi->cvq, &tmp);
> >
> > return vi->ctrl->status == VIRTIO_NET_OK;
>
> This seems to break surprise removal and other
> situations where vq gets broken since callbacks
> aren't usually invoked then.
Yes, so I think I can go back to the original idea by simply adding
cond_resched() here.
>
>
> > }
> > @@ -3672,7 +3678,7 @@ static int virtnet_find_vqs(struct virtnet_info *vi)
> >
> > /* Parameters for control virtqueue, if any */
> > if (vi->has_cvq) {
> > - callbacks[total_vqs - 1] = NULL;
> > + callbacks[total_vqs - 1] = virtnet_cvq_done;
> > names[total_vqs - 1] = "control";
> > }
> >
>
> There is a cost to this, in that we are burning an extra MSI vector
> for the slow path cvq. if device has 3 vectors, suddenly we can't
> allocate vectors for rx and tx, big problem.
>
> So I'm afraid we need to pass a new flag that will share
> the config changed interrupt and cvq.
See above, it looks to me a simple cond_resched() is sufficient, then
we don't need a new vector.
Thanks
>
>
>
> > @@ -4122,6 +4128,7 @@ static int virtnet_probe(struct virtio_device *vdev)
> > if (vi->has_rss || vi->has_rss_hash_report)
> > virtnet_init_default_rss(vi);
> >
> > + init_completion(&vi->completion);
> > enable_rx_mode_work(vi);
> >
> > /* serialize netdev register + virtio_device_ready() with ndo_open() */
> > --
> > 2.25.1
>