diff mbox series

[4/4] virtio-net: sleep instead of busy waiting for cvq command

Message ID 20221226074908.8154-5-jasowang@redhat.com (mailing list archive)
State Not Applicable
Delegated to: Netdev Maintainers
Headers show
Series virtio-net: don't busy poll for cvq command | expand

Checks

Context Check Description
netdev/tree_selection success Guessed tree name to be net-next
netdev/fixes_present success Fixes tag not required for -next series
netdev/subject_prefix success Link
netdev/cover_letter success Series has a cover letter
netdev/patch_count success Link
netdev/header_inline success No static functions without inline keyword in header files
netdev/build_32bit success Errors and warnings before: 0 this patch: 0
netdev/cc_maintainers success CCed 8 of 8 maintainers
netdev/build_clang success Errors and warnings before: 0 this patch: 0
netdev/module_param success Was 0 now: 0
netdev/verify_signedoff success Signed-off-by tag matches author and committer
netdev/check_selftest success No net selftest shell script
netdev/verify_fixes success No Fixes tag
netdev/build_allmodconfig_warn success Errors and warnings before: 0 this patch: 0
netdev/checkpatch success total: 0 errors, 0 warnings, 0 checks, 55 lines checked
netdev/kdoc success Errors and warnings before: 0 this patch: 0
netdev/source_inline success Was 0 now: 0

Commit Message

Jason Wang Dec. 26, 2022, 7:49 a.m. UTC
We used to busy waiting on the cvq command this tends to be
problematic since:

1) CPU could wait for ever on a buggy/malicous device
2) There's no wait to terminate the process that triggers the cvq
   command

So this patch switch to use virtqueue_wait_for_used() to sleep with a
timeout (1s) instead of busy polling for the cvq command forever. This
gives the scheduler a breath and can let the process can respond to
asignal. If the device doesn't respond in the timeout, break the
device.

Signed-off-by: Jason Wang <jasowang@redhat.com>
---
Changes since V1:
- break the device when timeout
- get buffer manually since the virtio core check more_used() instead
---
 drivers/net/virtio_net.c | 24 ++++++++++++++++--------
 1 file changed, 16 insertions(+), 8 deletions(-)

Comments

Xuan Zhuo Dec. 27, 2022, 2:19 a.m. UTC | #1
On Mon, 26 Dec 2022 15:49:08 +0800, Jason Wang <jasowang@redhat.com> wrote:
> We used to busy waiting on the cvq command this tends to be
> problematic since:
>
> 1) CPU could wait for ever on a buggy/malicous device
> 2) There's no wait to terminate the process that triggers the cvq
>    command
>
> So this patch switch to use virtqueue_wait_for_used() to sleep with a
> timeout (1s) instead of busy polling for the cvq command forever. This

I don't think that a fixed 1S is a good choice. Some of the DPUs are very
lazy for cvq handle. In particular, we will also directly break the device.

I think it is necessary to add a Virtio-Net parameter to allow users to define
this timeout by themselves. Although I don't think this is a good way.

Thanks.


> gives the scheduler a breath and can let the process can respond to
> asignal. If the device doesn't respond in the timeout, break the
> device.
>
> Signed-off-by: Jason Wang <jasowang@redhat.com>
> ---
> Changes since V1:
> - break the device when timeout
> - get buffer manually since the virtio core check more_used() instead
> ---
>  drivers/net/virtio_net.c | 24 ++++++++++++++++--------
>  1 file changed, 16 insertions(+), 8 deletions(-)
>
> diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
> index efd9dd55828b..6a2ea64cfcb5 100644
> --- a/drivers/net/virtio_net.c
> +++ b/drivers/net/virtio_net.c
> @@ -405,6 +405,7 @@ static void disable_rx_mode_work(struct virtnet_info *vi)
>  	vi->rx_mode_work_enabled = false;
>  	spin_unlock_bh(&vi->rx_mode_lock);
>
> +	virtqueue_wake_up(vi->cvq);
>  	flush_work(&vi->rx_mode_work);
>  }
>
> @@ -1497,6 +1498,11 @@ static bool try_fill_recv(struct virtnet_info *vi, struct receive_queue *rq,
>  	return !oom;
>  }
>
> +static void virtnet_cvq_done(struct virtqueue *cvq)
> +{
> +	virtqueue_wake_up(cvq);
> +}
> +
>  static void skb_recv_done(struct virtqueue *rvq)
>  {
>  	struct virtnet_info *vi = rvq->vdev->priv;
> @@ -1984,6 +1990,8 @@ static int virtnet_tx_resize(struct virtnet_info *vi,
>  	return err;
>  }
>
> +static int virtnet_close(struct net_device *dev);
> +
>  /*
>   * Send command via the control virtqueue and check status.  Commands
>   * supported by the hypervisor, as indicated by feature bits, should
> @@ -2026,14 +2034,14 @@ 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();
> +	if (virtqueue_wait_for_used(vi->cvq)) {
> +		virtqueue_get_buf(vi->cvq, &tmp);
> +		return vi->ctrl->status == VIRTIO_NET_OK;
> +	}
>
> -	return vi->ctrl->status == VIRTIO_NET_OK;
> +	netdev_err(vi->dev, "CVQ command timeout, break the virtio device.");
> +	virtio_break_device(vi->vdev);
> +	return VIRTIO_NET_ERR;
>  }
>
>  static int virtnet_set_mac_address(struct net_device *dev, void *p)
> @@ -3526,7 +3534,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";
>  	}
>
> --
> 2.25.1
>
> _______________________________________________
> Virtualization mailing list
> Virtualization@lists.linux-foundation.org
> https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Jason Wang Dec. 27, 2022, 4:33 a.m. UTC | #2
On Tue, Dec 27, 2022 at 10:25 AM Xuan Zhuo <xuanzhuo@linux.alibaba.com> wrote:
>
> On Mon, 26 Dec 2022 15:49:08 +0800, Jason Wang <jasowang@redhat.com> wrote:
> > We used to busy waiting on the cvq command this tends to be
> > problematic since:
> >
> > 1) CPU could wait for ever on a buggy/malicous device
> > 2) There's no wait to terminate the process that triggers the cvq
> >    command
> >
> > So this patch switch to use virtqueue_wait_for_used() to sleep with a
> > timeout (1s) instead of busy polling for the cvq command forever. This
>
> I don't think that a fixed 1S is a good choice.

Well, it could be tweaked to be a little bit longer.

One way, as discussed, is to let the device advertise a timeout then
the driver can validate if it's valid and use that timeout. But it
needs extension to the spec.

> Some of the DPUs are very
> lazy for cvq handle.

Such design needs to be revisited, cvq (control path) should have a
better priority or QOS than datapath.

> In particular, we will also directly break the device.

It's kind of hardening for malicious devices.

>
> I think it is necessary to add a Virtio-Net parameter to allow users to define
> this timeout by themselves. Although I don't think this is a good way.

Very hard and unfriendly to the end users.

Thanks

>
> Thanks.
>
>
> > gives the scheduler a breath and can let the process can respond to
> > asignal. If the device doesn't respond in the timeout, break the
> > device.
> >
> > Signed-off-by: Jason Wang <jasowang@redhat.com>
> > ---
> > Changes since V1:
> > - break the device when timeout
> > - get buffer manually since the virtio core check more_used() instead
> > ---
> >  drivers/net/virtio_net.c | 24 ++++++++++++++++--------
> >  1 file changed, 16 insertions(+), 8 deletions(-)
> >
> > diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
> > index efd9dd55828b..6a2ea64cfcb5 100644
> > --- a/drivers/net/virtio_net.c
> > +++ b/drivers/net/virtio_net.c
> > @@ -405,6 +405,7 @@ static void disable_rx_mode_work(struct virtnet_info *vi)
> >       vi->rx_mode_work_enabled = false;
> >       spin_unlock_bh(&vi->rx_mode_lock);
> >
> > +     virtqueue_wake_up(vi->cvq);
> >       flush_work(&vi->rx_mode_work);
> >  }
> >
> > @@ -1497,6 +1498,11 @@ static bool try_fill_recv(struct virtnet_info *vi, struct receive_queue *rq,
> >       return !oom;
> >  }
> >
> > +static void virtnet_cvq_done(struct virtqueue *cvq)
> > +{
> > +     virtqueue_wake_up(cvq);
> > +}
> > +
> >  static void skb_recv_done(struct virtqueue *rvq)
> >  {
> >       struct virtnet_info *vi = rvq->vdev->priv;
> > @@ -1984,6 +1990,8 @@ static int virtnet_tx_resize(struct virtnet_info *vi,
> >       return err;
> >  }
> >
> > +static int virtnet_close(struct net_device *dev);
> > +
> >  /*
> >   * Send command via the control virtqueue and check status.  Commands
> >   * supported by the hypervisor, as indicated by feature bits, should
> > @@ -2026,14 +2034,14 @@ 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();
> > +     if (virtqueue_wait_for_used(vi->cvq)) {
> > +             virtqueue_get_buf(vi->cvq, &tmp);
> > +             return vi->ctrl->status == VIRTIO_NET_OK;
> > +     }
> >
> > -     return vi->ctrl->status == VIRTIO_NET_OK;
> > +     netdev_err(vi->dev, "CVQ command timeout, break the virtio device.");
> > +     virtio_break_device(vi->vdev);
> > +     return VIRTIO_NET_ERR;
> >  }
> >
> >  static int virtnet_set_mac_address(struct net_device *dev, void *p)
> > @@ -3526,7 +3534,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";
> >       }
> >
> > --
> > 2.25.1
> >
> > _______________________________________________
> > Virtualization mailing list
> > Virtualization@lists.linux-foundation.org
> > https://lists.linuxfoundation.org/mailman/listinfo/virtualization
>
Michael S. Tsirkin Dec. 27, 2022, 6:58 a.m. UTC | #3
On Tue, Dec 27, 2022 at 12:33:53PM +0800, Jason Wang wrote:
> On Tue, Dec 27, 2022 at 10:25 AM Xuan Zhuo <xuanzhuo@linux.alibaba.com> wrote:
> >
> > On Mon, 26 Dec 2022 15:49:08 +0800, Jason Wang <jasowang@redhat.com> wrote:
> > > We used to busy waiting on the cvq command this tends to be
> > > problematic since:
> > >
> > > 1) CPU could wait for ever on a buggy/malicous device
> > > 2) There's no wait to terminate the process that triggers the cvq
> > >    command
> > >
> > > So this patch switch to use virtqueue_wait_for_used() to sleep with a
> > > timeout (1s) instead of busy polling for the cvq command forever. This
> >
> > I don't think that a fixed 1S is a good choice.
> 
> Well, it could be tweaked to be a little bit longer.
> 
> One way, as discussed, is to let the device advertise a timeout then
> the driver can validate if it's valid and use that timeout. But it
> needs extension to the spec.

Controlling timeout from device is a good idea, e.g. hardware devices
would benefit from a shorter timeout, hypervisor devices from a longer
timeout or no timeout.

> 
> > Some of the DPUs are very
> > lazy for cvq handle.
> 
> Such design needs to be revisited, cvq (control path) should have a
> better priority or QOS than datapath.

Spec says nothing about this, so driver can't assume this either.

> > In particular, we will also directly break the device.
> 
> It's kind of hardening for malicious devices.

ATM no amount of hardening can prevent a malicious hypervisor from
blocking the guest. Recovering when a hardware device is broken would be
nice but I think if we do bother then we should try harder to recover,
such as by driving device reset.


Also, does your patch break surprise removal? There's no callback
in this case ATM.

> >
> > I think it is necessary to add a Virtio-Net parameter to allow users to define
> > this timeout by themselves. Although I don't think this is a good way.
> 
> Very hard and unfriendly to the end users.
> 
> Thanks
> 
> >
> > Thanks.
> >
> >
> > > gives the scheduler a breath and can let the process can respond to
> > > asignal. If the device doesn't respond in the timeout, break the
> > > device.
> > >
> > > Signed-off-by: Jason Wang <jasowang@redhat.com>
> > > ---
> > > Changes since V1:
> > > - break the device when timeout
> > > - get buffer manually since the virtio core check more_used() instead
> > > ---
> > >  drivers/net/virtio_net.c | 24 ++++++++++++++++--------
> > >  1 file changed, 16 insertions(+), 8 deletions(-)
> > >
> > > diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
> > > index efd9dd55828b..6a2ea64cfcb5 100644
> > > --- a/drivers/net/virtio_net.c
> > > +++ b/drivers/net/virtio_net.c
> > > @@ -405,6 +405,7 @@ static void disable_rx_mode_work(struct virtnet_info *vi)
> > >       vi->rx_mode_work_enabled = false;
> > >       spin_unlock_bh(&vi->rx_mode_lock);
> > >
> > > +     virtqueue_wake_up(vi->cvq);
> > >       flush_work(&vi->rx_mode_work);
> > >  }
> > >
> > > @@ -1497,6 +1498,11 @@ static bool try_fill_recv(struct virtnet_info *vi, struct receive_queue *rq,
> > >       return !oom;
> > >  }
> > >
> > > +static void virtnet_cvq_done(struct virtqueue *cvq)
> > > +{
> > > +     virtqueue_wake_up(cvq);
> > > +}
> > > +
> > >  static void skb_recv_done(struct virtqueue *rvq)
> > >  {
> > >       struct virtnet_info *vi = rvq->vdev->priv;
> > > @@ -1984,6 +1990,8 @@ static int virtnet_tx_resize(struct virtnet_info *vi,
> > >       return err;
> > >  }
> > >
> > > +static int virtnet_close(struct net_device *dev);
> > > +
> > >  /*
> > >   * Send command via the control virtqueue and check status.  Commands
> > >   * supported by the hypervisor, as indicated by feature bits, should
> > > @@ -2026,14 +2034,14 @@ 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();
> > > +     if (virtqueue_wait_for_used(vi->cvq)) {
> > > +             virtqueue_get_buf(vi->cvq, &tmp);
> > > +             return vi->ctrl->status == VIRTIO_NET_OK;
> > > +     }
> > >
> > > -     return vi->ctrl->status == VIRTIO_NET_OK;
> > > +     netdev_err(vi->dev, "CVQ command timeout, break the virtio device.");
> > > +     virtio_break_device(vi->vdev);
> > > +     return VIRTIO_NET_ERR;
> > >  }
> > >
> > >  static int virtnet_set_mac_address(struct net_device *dev, void *p)
> > > @@ -3526,7 +3534,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";
> > >       }
> > >
> > > --
> > > 2.25.1
> > >
> > > _______________________________________________
> > > Virtualization mailing list
> > > Virtualization@lists.linux-foundation.org
> > > https://lists.linuxfoundation.org/mailman/listinfo/virtualization
> >
Jason Wang Dec. 27, 2022, 9:17 a.m. UTC | #4
在 2022/12/27 14:58, Michael S. Tsirkin 写道:
> On Tue, Dec 27, 2022 at 12:33:53PM +0800, Jason Wang wrote:
>> On Tue, Dec 27, 2022 at 10:25 AM Xuan Zhuo <xuanzhuo@linux.alibaba.com> wrote:
>>> On Mon, 26 Dec 2022 15:49:08 +0800, Jason Wang <jasowang@redhat.com> wrote:
>>>> We used to busy waiting on the cvq command this tends to be
>>>> problematic since:
>>>>
>>>> 1) CPU could wait for ever on a buggy/malicous device
>>>> 2) There's no wait to terminate the process that triggers the cvq
>>>>     command
>>>>
>>>> So this patch switch to use virtqueue_wait_for_used() to sleep with a
>>>> timeout (1s) instead of busy polling for the cvq command forever. This
>>> I don't think that a fixed 1S is a good choice.
>> Well, it could be tweaked to be a little bit longer.
>>
>> One way, as discussed, is to let the device advertise a timeout then
>> the driver can validate if it's valid and use that timeout. But it
>> needs extension to the spec.
> Controlling timeout from device is a good idea, e.g. hardware devices
> would benefit from a shorter timeout, hypervisor devices from a longer
> timeout or no timeout.


Yes.


>
>>> Some of the DPUs are very
>>> lazy for cvq handle.
>> Such design needs to be revisited, cvq (control path) should have a
>> better priority or QOS than datapath.
> Spec says nothing about this, so driver can't assume this either.


Well, my understanding is that it's more than what spec can define or 
it's a kind of best practice.

The current code is one example, that is, driver may choose to busy poll 
which cause spike.


>
>>> In particular, we will also directly break the device.
>> It's kind of hardening for malicious devices.
> ATM no amount of hardening can prevent a malicious hypervisor from
> blocking the guest. Recovering when a hardware device is broken would be
> nice but I think if we do bother then we should try harder to recover,
> such as by driving device reset.


Probably, but as discussed in another thread, it needs co-operation in 
the upper layer (networking core).


>
>
> Also, does your patch break surprise removal? There's no callback
> in this case ATM.


I think not (see reply in another thread).

Thanks


>
>>> I think it is necessary to add a Virtio-Net parameter to allow users to define
>>> this timeout by themselves. Although I don't think this is a good way.
>> Very hard and unfriendly to the end users.
>>
>> Thanks
>>
>>> Thanks.
>>>
>>>
>>>> gives the scheduler a breath and can let the process can respond to
>>>> asignal. If the device doesn't respond in the timeout, break the
>>>> device.
>>>>
>>>> Signed-off-by: Jason Wang <jasowang@redhat.com>
>>>> ---
>>>> Changes since V1:
>>>> - break the device when timeout
>>>> - get buffer manually since the virtio core check more_used() instead
>>>> ---
>>>>   drivers/net/virtio_net.c | 24 ++++++++++++++++--------
>>>>   1 file changed, 16 insertions(+), 8 deletions(-)
>>>>
>>>> diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
>>>> index efd9dd55828b..6a2ea64cfcb5 100644
>>>> --- a/drivers/net/virtio_net.c
>>>> +++ b/drivers/net/virtio_net.c
>>>> @@ -405,6 +405,7 @@ static void disable_rx_mode_work(struct virtnet_info *vi)
>>>>        vi->rx_mode_work_enabled = false;
>>>>        spin_unlock_bh(&vi->rx_mode_lock);
>>>>
>>>> +     virtqueue_wake_up(vi->cvq);
>>>>        flush_work(&vi->rx_mode_work);
>>>>   }
>>>>
>>>> @@ -1497,6 +1498,11 @@ static bool try_fill_recv(struct virtnet_info *vi, struct receive_queue *rq,
>>>>        return !oom;
>>>>   }
>>>>
>>>> +static void virtnet_cvq_done(struct virtqueue *cvq)
>>>> +{
>>>> +     virtqueue_wake_up(cvq);
>>>> +}
>>>> +
>>>>   static void skb_recv_done(struct virtqueue *rvq)
>>>>   {
>>>>        struct virtnet_info *vi = rvq->vdev->priv;
>>>> @@ -1984,6 +1990,8 @@ static int virtnet_tx_resize(struct virtnet_info *vi,
>>>>        return err;
>>>>   }
>>>>
>>>> +static int virtnet_close(struct net_device *dev);
>>>> +
>>>>   /*
>>>>    * Send command via the control virtqueue and check status.  Commands
>>>>    * supported by the hypervisor, as indicated by feature bits, should
>>>> @@ -2026,14 +2034,14 @@ 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();
>>>> +     if (virtqueue_wait_for_used(vi->cvq)) {
>>>> +             virtqueue_get_buf(vi->cvq, &tmp);
>>>> +             return vi->ctrl->status == VIRTIO_NET_OK;
>>>> +     }
>>>>
>>>> -     return vi->ctrl->status == VIRTIO_NET_OK;
>>>> +     netdev_err(vi->dev, "CVQ command timeout, break the virtio device.");
>>>> +     virtio_break_device(vi->vdev);
>>>> +     return VIRTIO_NET_ERR;
>>>>   }
>>>>
>>>>   static int virtnet_set_mac_address(struct net_device *dev, void *p)
>>>> @@ -3526,7 +3534,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";
>>>>        }
>>>>
>>>> --
>>>> 2.25.1
>>>>
>>>> _______________________________________________
>>>> Virtualization mailing list
>>>> Virtualization@lists.linux-foundation.org
>>>> https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Michael S. Tsirkin Dec. 27, 2022, 9:31 a.m. UTC | #5
On Tue, Dec 27, 2022 at 05:17:20PM +0800, Jason Wang wrote:
> > > > In particular, we will also directly break the device.
> > > It's kind of hardening for malicious devices.
> > ATM no amount of hardening can prevent a malicious hypervisor from
> > blocking the guest. Recovering when a hardware device is broken would be
> > nice but I think if we do bother then we should try harder to recover,
> > such as by driving device reset.
> 
> 
> Probably, but as discussed in another thread, it needs co-operation in the
> upper layer (networking core).

To track all state? Yea, maybe. For sure it's doable just in virtio,
but if you can find 1-2 other drivers that do this internally
then factoring this out to net core will likely be accepted.
Jason Wang Dec. 28, 2022, 6:35 a.m. UTC | #6
在 2022/12/27 17:31, Michael S. Tsirkin 写道:
> On Tue, Dec 27, 2022 at 05:17:20PM +0800, Jason Wang wrote:
>>>>> In particular, we will also directly break the device.
>>>> It's kind of hardening for malicious devices.
>>> ATM no amount of hardening can prevent a malicious hypervisor from
>>> blocking the guest. Recovering when a hardware device is broken would be
>>> nice but I think if we do bother then we should try harder to recover,
>>> such as by driving device reset.
>>
>> Probably, but as discussed in another thread, it needs co-operation in the
>> upper layer (networking core).
> To track all state? Yea, maybe. For sure it's doable just in virtio,
> but if you can find 1-2 other drivers that do this internally
> then factoring this out to net core will likely be accepted.


One thing that might be useful is to reuse tx_timeout() but current 
virtio-net doesn't do more than a simple warning (other drivers may try 
to reset).

So I would leave it for future investigation.

Thanks


>
Xuan Zhuo Dec. 28, 2022, 8:31 a.m. UTC | #7
On Tue, 27 Dec 2022 01:58:22 -0500, "Michael S. Tsirkin" <mst@redhat.com> wrote:
> On Tue, Dec 27, 2022 at 12:33:53PM +0800, Jason Wang wrote:
> > On Tue, Dec 27, 2022 at 10:25 AM Xuan Zhuo <xuanzhuo@linux.alibaba.com> wrote:
> > >
> > > On Mon, 26 Dec 2022 15:49:08 +0800, Jason Wang <jasowang@redhat.com> wrote:
> > > > We used to busy waiting on the cvq command this tends to be
> > > > problematic since:
> > > >
> > > > 1) CPU could wait for ever on a buggy/malicous device
> > > > 2) There's no wait to terminate the process that triggers the cvq
> > > >    command
> > > >
> > > > So this patch switch to use virtqueue_wait_for_used() to sleep with a
> > > > timeout (1s) instead of busy polling for the cvq command forever. This
> > >
> > > I don't think that a fixed 1S is a good choice.
> >
> > Well, it could be tweaked to be a little bit longer.
> >
> > One way, as discussed, is to let the device advertise a timeout then
> > the driver can validate if it's valid and use that timeout. But it
> > needs extension to the spec.
>
> Controlling timeout from device is a good idea, e.g. hardware devices
> would benefit from a shorter timeout, hypervisor devices from a longer
> timeout or no timeout.

Yes. That is good.

Before introducing this feature, I personally like to use "wait", rather than
define a timeout.

Thanks.


>
> >
> > > Some of the DPUs are very
> > > lazy for cvq handle.
> >
> > Such design needs to be revisited, cvq (control path) should have a
> > better priority or QOS than datapath.
>
> Spec says nothing about this, so driver can't assume this either.
>
> > > In particular, we will also directly break the device.
> >
> > It's kind of hardening for malicious devices.
>
> ATM no amount of hardening can prevent a malicious hypervisor from
> blocking the guest. Recovering when a hardware device is broken would be
> nice but I think if we do bother then we should try harder to recover,
> such as by driving device reset.
>
>
> Also, does your patch break surprise removal? There's no callback
> in this case ATM.
>
> > >
> > > I think it is necessary to add a Virtio-Net parameter to allow users to define
> > > this timeout by themselves. Although I don't think this is a good way.
> >
> > Very hard and unfriendly to the end users.
> >
> > Thanks
> >
> > >
> > > Thanks.
> > >
> > >
> > > > gives the scheduler a breath and can let the process can respond to
> > > > asignal. If the device doesn't respond in the timeout, break the
> > > > device.
> > > >
> > > > Signed-off-by: Jason Wang <jasowang@redhat.com>
> > > > ---
> > > > Changes since V1:
> > > > - break the device when timeout
> > > > - get buffer manually since the virtio core check more_used() instead
> > > > ---
> > > >  drivers/net/virtio_net.c | 24 ++++++++++++++++--------
> > > >  1 file changed, 16 insertions(+), 8 deletions(-)
> > > >
> > > > diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
> > > > index efd9dd55828b..6a2ea64cfcb5 100644
> > > > --- a/drivers/net/virtio_net.c
> > > > +++ b/drivers/net/virtio_net.c
> > > > @@ -405,6 +405,7 @@ static void disable_rx_mode_work(struct virtnet_info *vi)
> > > >       vi->rx_mode_work_enabled = false;
> > > >       spin_unlock_bh(&vi->rx_mode_lock);
> > > >
> > > > +     virtqueue_wake_up(vi->cvq);
> > > >       flush_work(&vi->rx_mode_work);
> > > >  }
> > > >
> > > > @@ -1497,6 +1498,11 @@ static bool try_fill_recv(struct virtnet_info *vi, struct receive_queue *rq,
> > > >       return !oom;
> > > >  }
> > > >
> > > > +static void virtnet_cvq_done(struct virtqueue *cvq)
> > > > +{
> > > > +     virtqueue_wake_up(cvq);
> > > > +}
> > > > +
> > > >  static void skb_recv_done(struct virtqueue *rvq)
> > > >  {
> > > >       struct virtnet_info *vi = rvq->vdev->priv;
> > > > @@ -1984,6 +1990,8 @@ static int virtnet_tx_resize(struct virtnet_info *vi,
> > > >       return err;
> > > >  }
> > > >
> > > > +static int virtnet_close(struct net_device *dev);
> > > > +
> > > >  /*
> > > >   * Send command via the control virtqueue and check status.  Commands
> > > >   * supported by the hypervisor, as indicated by feature bits, should
> > > > @@ -2026,14 +2034,14 @@ 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();
> > > > +     if (virtqueue_wait_for_used(vi->cvq)) {
> > > > +             virtqueue_get_buf(vi->cvq, &tmp);
> > > > +             return vi->ctrl->status == VIRTIO_NET_OK;
> > > > +     }
> > > >
> > > > -     return vi->ctrl->status == VIRTIO_NET_OK;
> > > > +     netdev_err(vi->dev, "CVQ command timeout, break the virtio device.");
> > > > +     virtio_break_device(vi->vdev);
> > > > +     return VIRTIO_NET_ERR;
> > > >  }
> > > >
> > > >  static int virtnet_set_mac_address(struct net_device *dev, void *p)
> > > > @@ -3526,7 +3534,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";
> > > >       }
> > > >
> > > > --
> > > > 2.25.1
> > > >
> > > > _______________________________________________
> > > > Virtualization mailing list
> > > > Virtualization@lists.linux-foundation.org
> > > > https://lists.linuxfoundation.org/mailman/listinfo/virtualization
> > >
>
Xuan Zhuo Dec. 28, 2022, 8:39 a.m. UTC | #8
On Tue, 27 Dec 2022 12:33:53 +0800, Jason Wang <jasowang@redhat.com> wrote:
> On Tue, Dec 27, 2022 at 10:25 AM Xuan Zhuo <xuanzhuo@linux.alibaba.com> wrote:
> >
> > On Mon, 26 Dec 2022 15:49:08 +0800, Jason Wang <jasowang@redhat.com> wrote:
> > > We used to busy waiting on the cvq command this tends to be
> > > problematic since:
> > >
> > > 1) CPU could wait for ever on a buggy/malicous device
> > > 2) There's no wait to terminate the process that triggers the cvq
> > >    command
> > >
> > > So this patch switch to use virtqueue_wait_for_used() to sleep with a
> > > timeout (1s) instead of busy polling for the cvq command forever. This
> >
> > I don't think that a fixed 1S is a good choice.
>
> Well, it could be tweaked to be a little bit longer.
>
> One way, as discussed, is to let the device advertise a timeout then
> the driver can validate if it's valid and use that timeout. But it
> needs extension to the spec.
>
> > Some of the DPUs are very
> > lazy for cvq handle.
>
> Such design needs to be revisited, cvq (control path) should have a
> better priority or QOS than datapath.
>
> > In particular, we will also directly break the device.
>
> It's kind of hardening for malicious devices.

Just based on timeout, it is judged that it is a malicious device. I think it is
too arbitrary.

Thanks.


>
> >
> > I think it is necessary to add a Virtio-Net parameter to allow users to define
> > this timeout by themselves. Although I don't think this is a good way.
>
> Very hard and unfriendly to the end users.
>
> Thanks
>
> >
> > Thanks.
> >
> >
> > > gives the scheduler a breath and can let the process can respond to
> > > asignal. If the device doesn't respond in the timeout, break the
> > > device.
> > >
> > > Signed-off-by: Jason Wang <jasowang@redhat.com>
> > > ---
> > > Changes since V1:
> > > - break the device when timeout
> > > - get buffer manually since the virtio core check more_used() instead
> > > ---
> > >  drivers/net/virtio_net.c | 24 ++++++++++++++++--------
> > >  1 file changed, 16 insertions(+), 8 deletions(-)
> > >
> > > diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
> > > index efd9dd55828b..6a2ea64cfcb5 100644
> > > --- a/drivers/net/virtio_net.c
> > > +++ b/drivers/net/virtio_net.c
> > > @@ -405,6 +405,7 @@ static void disable_rx_mode_work(struct virtnet_info *vi)
> > >       vi->rx_mode_work_enabled = false;
> > >       spin_unlock_bh(&vi->rx_mode_lock);
> > >
> > > +     virtqueue_wake_up(vi->cvq);
> > >       flush_work(&vi->rx_mode_work);
> > >  }
> > >
> > > @@ -1497,6 +1498,11 @@ static bool try_fill_recv(struct virtnet_info *vi, struct receive_queue *rq,
> > >       return !oom;
> > >  }
> > >
> > > +static void virtnet_cvq_done(struct virtqueue *cvq)
> > > +{
> > > +     virtqueue_wake_up(cvq);
> > > +}
> > > +
> > >  static void skb_recv_done(struct virtqueue *rvq)
> > >  {
> > >       struct virtnet_info *vi = rvq->vdev->priv;
> > > @@ -1984,6 +1990,8 @@ static int virtnet_tx_resize(struct virtnet_info *vi,
> > >       return err;
> > >  }
> > >
> > > +static int virtnet_close(struct net_device *dev);
> > > +
> > >  /*
> > >   * Send command via the control virtqueue and check status.  Commands
> > >   * supported by the hypervisor, as indicated by feature bits, should
> > > @@ -2026,14 +2034,14 @@ 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();
> > > +     if (virtqueue_wait_for_used(vi->cvq)) {
> > > +             virtqueue_get_buf(vi->cvq, &tmp);
> > > +             return vi->ctrl->status == VIRTIO_NET_OK;
> > > +     }
> > >
> > > -     return vi->ctrl->status == VIRTIO_NET_OK;
> > > +     netdev_err(vi->dev, "CVQ command timeout, break the virtio device.");
> > > +     virtio_break_device(vi->vdev);
> > > +     return VIRTIO_NET_ERR;
> > >  }
> > >
> > >  static int virtnet_set_mac_address(struct net_device *dev, void *p)
> > > @@ -3526,7 +3534,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";
> > >       }
> > >
> > > --
> > > 2.25.1
> > >
> > > _______________________________________________
> > > Virtualization mailing list
> > > Virtualization@lists.linux-foundation.org
> > > https://lists.linuxfoundation.org/mailman/listinfo/virtualization
> >
>
Jason Wang Dec. 28, 2022, 11:41 a.m. UTC | #9
On Wed, Dec 28, 2022 at 4:34 PM Xuan Zhuo <xuanzhuo@linux.alibaba.com> wrote:
>
> On Tue, 27 Dec 2022 01:58:22 -0500, "Michael S. Tsirkin" <mst@redhat.com> wrote:
> > On Tue, Dec 27, 2022 at 12:33:53PM +0800, Jason Wang wrote:
> > > On Tue, Dec 27, 2022 at 10:25 AM Xuan Zhuo <xuanzhuo@linux.alibaba.com> wrote:
> > > >
> > > > On Mon, 26 Dec 2022 15:49:08 +0800, Jason Wang <jasowang@redhat.com> wrote:
> > > > > We used to busy waiting on the cvq command this tends to be
> > > > > problematic since:
> > > > >
> > > > > 1) CPU could wait for ever on a buggy/malicous device
> > > > > 2) There's no wait to terminate the process that triggers the cvq
> > > > >    command
> > > > >
> > > > > So this patch switch to use virtqueue_wait_for_used() to sleep with a
> > > > > timeout (1s) instead of busy polling for the cvq command forever. This
> > > >
> > > > I don't think that a fixed 1S is a good choice.
> > >
> > > Well, it could be tweaked to be a little bit longer.
> > >
> > > One way, as discussed, is to let the device advertise a timeout then
> > > the driver can validate if it's valid and use that timeout. But it
> > > needs extension to the spec.
> >
> > Controlling timeout from device is a good idea, e.g. hardware devices
> > would benefit from a shorter timeout, hypervisor devices from a longer
> > timeout or no timeout.
>
> Yes. That is good.
>
> Before introducing this feature, I personally like to use "wait", rather than
> define a timeout.

Note that the driver still needs to validate what device advertises to
avoid infinite wait.

Thanks

>
> Thanks.
>
>
> >
> > >
> > > > Some of the DPUs are very
> > > > lazy for cvq handle.
> > >
> > > Such design needs to be revisited, cvq (control path) should have a
> > > better priority or QOS than datapath.
> >
> > Spec says nothing about this, so driver can't assume this either.
> >
> > > > In particular, we will also directly break the device.
> > >
> > > It's kind of hardening for malicious devices.
> >
> > ATM no amount of hardening can prevent a malicious hypervisor from
> > blocking the guest. Recovering when a hardware device is broken would be
> > nice but I think if we do bother then we should try harder to recover,
> > such as by driving device reset.
> >
> >
> > Also, does your patch break surprise removal? There's no callback
> > in this case ATM.
> >
> > > >
> > > > I think it is necessary to add a Virtio-Net parameter to allow users to define
> > > > this timeout by themselves. Although I don't think this is a good way.
> > >
> > > Very hard and unfriendly to the end users.
> > >
> > > Thanks
> > >
> > > >
> > > > Thanks.
> > > >
> > > >
> > > > > gives the scheduler a breath and can let the process can respond to
> > > > > asignal. If the device doesn't respond in the timeout, break the
> > > > > device.
> > > > >
> > > > > Signed-off-by: Jason Wang <jasowang@redhat.com>
> > > > > ---
> > > > > Changes since V1:
> > > > > - break the device when timeout
> > > > > - get buffer manually since the virtio core check more_used() instead
> > > > > ---
> > > > >  drivers/net/virtio_net.c | 24 ++++++++++++++++--------
> > > > >  1 file changed, 16 insertions(+), 8 deletions(-)
> > > > >
> > > > > diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
> > > > > index efd9dd55828b..6a2ea64cfcb5 100644
> > > > > --- a/drivers/net/virtio_net.c
> > > > > +++ b/drivers/net/virtio_net.c
> > > > > @@ -405,6 +405,7 @@ static void disable_rx_mode_work(struct virtnet_info *vi)
> > > > >       vi->rx_mode_work_enabled = false;
> > > > >       spin_unlock_bh(&vi->rx_mode_lock);
> > > > >
> > > > > +     virtqueue_wake_up(vi->cvq);
> > > > >       flush_work(&vi->rx_mode_work);
> > > > >  }
> > > > >
> > > > > @@ -1497,6 +1498,11 @@ static bool try_fill_recv(struct virtnet_info *vi, struct receive_queue *rq,
> > > > >       return !oom;
> > > > >  }
> > > > >
> > > > > +static void virtnet_cvq_done(struct virtqueue *cvq)
> > > > > +{
> > > > > +     virtqueue_wake_up(cvq);
> > > > > +}
> > > > > +
> > > > >  static void skb_recv_done(struct virtqueue *rvq)
> > > > >  {
> > > > >       struct virtnet_info *vi = rvq->vdev->priv;
> > > > > @@ -1984,6 +1990,8 @@ static int virtnet_tx_resize(struct virtnet_info *vi,
> > > > >       return err;
> > > > >  }
> > > > >
> > > > > +static int virtnet_close(struct net_device *dev);
> > > > > +
> > > > >  /*
> > > > >   * Send command via the control virtqueue and check status.  Commands
> > > > >   * supported by the hypervisor, as indicated by feature bits, should
> > > > > @@ -2026,14 +2034,14 @@ 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();
> > > > > +     if (virtqueue_wait_for_used(vi->cvq)) {
> > > > > +             virtqueue_get_buf(vi->cvq, &tmp);
> > > > > +             return vi->ctrl->status == VIRTIO_NET_OK;
> > > > > +     }
> > > > >
> > > > > -     return vi->ctrl->status == VIRTIO_NET_OK;
> > > > > +     netdev_err(vi->dev, "CVQ command timeout, break the virtio device.");
> > > > > +     virtio_break_device(vi->vdev);
> > > > > +     return VIRTIO_NET_ERR;
> > > > >  }
> > > > >
> > > > >  static int virtnet_set_mac_address(struct net_device *dev, void *p)
> > > > > @@ -3526,7 +3534,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";
> > > > >       }
> > > > >
> > > > > --
> > > > > 2.25.1
> > > > >
> > > > > _______________________________________________
> > > > > Virtualization mailing list
> > > > > Virtualization@lists.linux-foundation.org
> > > > > https://lists.linuxfoundation.org/mailman/listinfo/virtualization
> > > >
> >
>
Jason Wang Dec. 28, 2022, 11:43 a.m. UTC | #10
On Wed, Dec 28, 2022 at 4:40 PM Xuan Zhuo <xuanzhuo@linux.alibaba.com> wrote:
>
> On Tue, 27 Dec 2022 12:33:53 +0800, Jason Wang <jasowang@redhat.com> wrote:
> > On Tue, Dec 27, 2022 at 10:25 AM Xuan Zhuo <xuanzhuo@linux.alibaba.com> wrote:
> > >
> > > On Mon, 26 Dec 2022 15:49:08 +0800, Jason Wang <jasowang@redhat.com> wrote:
> > > > We used to busy waiting on the cvq command this tends to be
> > > > problematic since:
> > > >
> > > > 1) CPU could wait for ever on a buggy/malicous device
> > > > 2) There's no wait to terminate the process that triggers the cvq
> > > >    command
> > > >
> > > > So this patch switch to use virtqueue_wait_for_used() to sleep with a
> > > > timeout (1s) instead of busy polling for the cvq command forever. This
> > >
> > > I don't think that a fixed 1S is a good choice.
> >
> > Well, it could be tweaked to be a little bit longer.
> >
> > One way, as discussed, is to let the device advertise a timeout then
> > the driver can validate if it's valid and use that timeout. But it
> > needs extension to the spec.
> >
> > > Some of the DPUs are very
> > > lazy for cvq handle.
> >
> > Such design needs to be revisited, cvq (control path) should have a
> > better priority or QOS than datapath.
> >
> > > In particular, we will also directly break the device.
> >
> > It's kind of hardening for malicious devices.
>
> Just based on timeout, it is judged that it is a malicious device. I think it is
> too arbitrary.

Drivers have very little information to make the decision. So it's
really a balance.

We can start with a very long timeout like 10 minutes. Otherwise a
buggy/malicious device will block a lot of important things (reboot,
modprobe) even if the scheduler is still functional.

Thanks

>
> Thanks.
>
>
> >
> > >
> > > I think it is necessary to add a Virtio-Net parameter to allow users to define
> > > this timeout by themselves. Although I don't think this is a good way.
> >
> > Very hard and unfriendly to the end users.
> >
> > Thanks
> >
> > >
> > > Thanks.
> > >
> > >
> > > > gives the scheduler a breath and can let the process can respond to
> > > > asignal. If the device doesn't respond in the timeout, break the
> > > > device.
> > > >
> > > > Signed-off-by: Jason Wang <jasowang@redhat.com>
> > > > ---
> > > > Changes since V1:
> > > > - break the device when timeout
> > > > - get buffer manually since the virtio core check more_used() instead
> > > > ---
> > > >  drivers/net/virtio_net.c | 24 ++++++++++++++++--------
> > > >  1 file changed, 16 insertions(+), 8 deletions(-)
> > > >
> > > > diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
> > > > index efd9dd55828b..6a2ea64cfcb5 100644
> > > > --- a/drivers/net/virtio_net.c
> > > > +++ b/drivers/net/virtio_net.c
> > > > @@ -405,6 +405,7 @@ static void disable_rx_mode_work(struct virtnet_info *vi)
> > > >       vi->rx_mode_work_enabled = false;
> > > >       spin_unlock_bh(&vi->rx_mode_lock);
> > > >
> > > > +     virtqueue_wake_up(vi->cvq);
> > > >       flush_work(&vi->rx_mode_work);
> > > >  }
> > > >
> > > > @@ -1497,6 +1498,11 @@ static bool try_fill_recv(struct virtnet_info *vi, struct receive_queue *rq,
> > > >       return !oom;
> > > >  }
> > > >
> > > > +static void virtnet_cvq_done(struct virtqueue *cvq)
> > > > +{
> > > > +     virtqueue_wake_up(cvq);
> > > > +}
> > > > +
> > > >  static void skb_recv_done(struct virtqueue *rvq)
> > > >  {
> > > >       struct virtnet_info *vi = rvq->vdev->priv;
> > > > @@ -1984,6 +1990,8 @@ static int virtnet_tx_resize(struct virtnet_info *vi,
> > > >       return err;
> > > >  }
> > > >
> > > > +static int virtnet_close(struct net_device *dev);
> > > > +
> > > >  /*
> > > >   * Send command via the control virtqueue and check status.  Commands
> > > >   * supported by the hypervisor, as indicated by feature bits, should
> > > > @@ -2026,14 +2034,14 @@ 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();
> > > > +     if (virtqueue_wait_for_used(vi->cvq)) {
> > > > +             virtqueue_get_buf(vi->cvq, &tmp);
> > > > +             return vi->ctrl->status == VIRTIO_NET_OK;
> > > > +     }
> > > >
> > > > -     return vi->ctrl->status == VIRTIO_NET_OK;
> > > > +     netdev_err(vi->dev, "CVQ command timeout, break the virtio device.");
> > > > +     virtio_break_device(vi->vdev);
> > > > +     return VIRTIO_NET_ERR;
> > > >  }
> > > >
> > > >  static int virtnet_set_mac_address(struct net_device *dev, void *p)
> > > > @@ -3526,7 +3534,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";
> > > >       }
> > > >
> > > > --
> > > > 2.25.1
> > > >
> > > > _______________________________________________
> > > > Virtualization mailing list
> > > > Virtualization@lists.linux-foundation.org
> > > > https://lists.linuxfoundation.org/mailman/listinfo/virtualization
> > >
> >
>
Xuan Zhuo Dec. 29, 2022, 2:01 a.m. UTC | #11
On Wed, 28 Dec 2022 19:43:56 +0800, Jason Wang <jasowang@redhat.com> wrote:
> On Wed, Dec 28, 2022 at 4:40 PM Xuan Zhuo <xuanzhuo@linux.alibaba.com> wrote:
> >
> > On Tue, 27 Dec 2022 12:33:53 +0800, Jason Wang <jasowang@redhat.com> wrote:
> > > On Tue, Dec 27, 2022 at 10:25 AM Xuan Zhuo <xuanzhuo@linux.alibaba.com> wrote:
> > > >
> > > > On Mon, 26 Dec 2022 15:49:08 +0800, Jason Wang <jasowang@redhat.com> wrote:
> > > > > We used to busy waiting on the cvq command this tends to be
> > > > > problematic since:
> > > > >
> > > > > 1) CPU could wait for ever on a buggy/malicous device
> > > > > 2) There's no wait to terminate the process that triggers the cvq
> > > > >    command
> > > > >
> > > > > So this patch switch to use virtqueue_wait_for_used() to sleep with a
> > > > > timeout (1s) instead of busy polling for the cvq command forever. This
> > > >
> > > > I don't think that a fixed 1S is a good choice.
> > >
> > > Well, it could be tweaked to be a little bit longer.
> > >
> > > One way, as discussed, is to let the device advertise a timeout then
> > > the driver can validate if it's valid and use that timeout. But it
> > > needs extension to the spec.
> > >
> > > > Some of the DPUs are very
> > > > lazy for cvq handle.
> > >
> > > Such design needs to be revisited, cvq (control path) should have a
> > > better priority or QOS than datapath.
> > >
> > > > In particular, we will also directly break the device.
> > >
> > > It's kind of hardening for malicious devices.
> >
> > Just based on timeout, it is judged that it is a malicious device. I think it is
> > too arbitrary.
>
> Drivers have very little information to make the decision. So it's
> really a balance.
>
> We can start with a very long timeout like 10 minutes. Otherwise a
> buggy/malicious device will block a lot of important things (reboot,
> modprobe) even if the scheduler is still functional.

Relatively speaking, starting from a 1min+ timeout, I think it is safe.

Thanks.



>
> Thanks
>
> >
> > Thanks.
> >
> >
> > >
> > > >
> > > > I think it is necessary to add a Virtio-Net parameter to allow users to define
> > > > this timeout by themselves. Although I don't think this is a good way.
> > >
> > > Very hard and unfriendly to the end users.
> > >
> > > Thanks
> > >
> > > >
> > > > Thanks.
> > > >
> > > >
> > > > > gives the scheduler a breath and can let the process can respond to
> > > > > asignal. If the device doesn't respond in the timeout, break the
> > > > > device.
> > > > >
> > > > > Signed-off-by: Jason Wang <jasowang@redhat.com>
> > > > > ---
> > > > > Changes since V1:
> > > > > - break the device when timeout
> > > > > - get buffer manually since the virtio core check more_used() instead
> > > > > ---
> > > > >  drivers/net/virtio_net.c | 24 ++++++++++++++++--------
> > > > >  1 file changed, 16 insertions(+), 8 deletions(-)
> > > > >
> > > > > diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
> > > > > index efd9dd55828b..6a2ea64cfcb5 100644
> > > > > --- a/drivers/net/virtio_net.c
> > > > > +++ b/drivers/net/virtio_net.c
> > > > > @@ -405,6 +405,7 @@ static void disable_rx_mode_work(struct virtnet_info *vi)
> > > > >       vi->rx_mode_work_enabled = false;
> > > > >       spin_unlock_bh(&vi->rx_mode_lock);
> > > > >
> > > > > +     virtqueue_wake_up(vi->cvq);
> > > > >       flush_work(&vi->rx_mode_work);
> > > > >  }
> > > > >
> > > > > @@ -1497,6 +1498,11 @@ static bool try_fill_recv(struct virtnet_info *vi, struct receive_queue *rq,
> > > > >       return !oom;
> > > > >  }
> > > > >
> > > > > +static void virtnet_cvq_done(struct virtqueue *cvq)
> > > > > +{
> > > > > +     virtqueue_wake_up(cvq);
> > > > > +}
> > > > > +
> > > > >  static void skb_recv_done(struct virtqueue *rvq)
> > > > >  {
> > > > >       struct virtnet_info *vi = rvq->vdev->priv;
> > > > > @@ -1984,6 +1990,8 @@ static int virtnet_tx_resize(struct virtnet_info *vi,
> > > > >       return err;
> > > > >  }
> > > > >
> > > > > +static int virtnet_close(struct net_device *dev);
> > > > > +
> > > > >  /*
> > > > >   * Send command via the control virtqueue and check status.  Commands
> > > > >   * supported by the hypervisor, as indicated by feature bits, should
> > > > > @@ -2026,14 +2034,14 @@ 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();
> > > > > +     if (virtqueue_wait_for_used(vi->cvq)) {
> > > > > +             virtqueue_get_buf(vi->cvq, &tmp);
> > > > > +             return vi->ctrl->status == VIRTIO_NET_OK;
> > > > > +     }
> > > > >
> > > > > -     return vi->ctrl->status == VIRTIO_NET_OK;
> > > > > +     netdev_err(vi->dev, "CVQ command timeout, break the virtio device.");
> > > > > +     virtio_break_device(vi->vdev);
> > > > > +     return VIRTIO_NET_ERR;
> > > > >  }
> > > > >
> > > > >  static int virtnet_set_mac_address(struct net_device *dev, void *p)
> > > > > @@ -3526,7 +3534,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";
> > > > >       }
> > > > >
> > > > > --
> > > > > 2.25.1
> > > > >
> > > > > _______________________________________________
> > > > > Virtualization mailing list
> > > > > Virtualization@lists.linux-foundation.org
> > > > > https://lists.linuxfoundation.org/mailman/listinfo/virtualization
> > > >
> > >
> >
>
Xuan Zhuo Dec. 29, 2022, 2:09 a.m. UTC | #12
On Wed, 28 Dec 2022 19:41:13 +0800, Jason Wang <jasowang@redhat.com> wrote:
> On Wed, Dec 28, 2022 at 4:34 PM Xuan Zhuo <xuanzhuo@linux.alibaba.com> wrote:
> >
> > On Tue, 27 Dec 2022 01:58:22 -0500, "Michael S. Tsirkin" <mst@redhat.com> wrote:
> > > On Tue, Dec 27, 2022 at 12:33:53PM +0800, Jason Wang wrote:
> > > > On Tue, Dec 27, 2022 at 10:25 AM Xuan Zhuo <xuanzhuo@linux.alibaba.com> wrote:
> > > > >
> > > > > On Mon, 26 Dec 2022 15:49:08 +0800, Jason Wang <jasowang@redhat.com> wrote:
> > > > > > We used to busy waiting on the cvq command this tends to be
> > > > > > problematic since:
> > > > > >
> > > > > > 1) CPU could wait for ever on a buggy/malicous device
> > > > > > 2) There's no wait to terminate the process that triggers the cvq
> > > > > >    command
> > > > > >
> > > > > > So this patch switch to use virtqueue_wait_for_used() to sleep with a
> > > > > > timeout (1s) instead of busy polling for the cvq command forever. This
> > > > >
> > > > > I don't think that a fixed 1S is a good choice.
> > > >
> > > > Well, it could be tweaked to be a little bit longer.
> > > >
> > > > One way, as discussed, is to let the device advertise a timeout then
> > > > the driver can validate if it's valid and use that timeout. But it
> > > > needs extension to the spec.
> > >
> > > Controlling timeout from device is a good idea, e.g. hardware devices
> > > would benefit from a shorter timeout, hypervisor devices from a longer
> > > timeout or no timeout.
> >
> > Yes. That is good.
> >
> > Before introducing this feature, I personally like to use "wait", rather than
> > define a timeout.
>
> Note that the driver still needs to validate what device advertises to
> avoid infinite wait.

Sorry, I didn't understand what you mean.

Thanks.

>
> Thanks
>
> >
> > Thanks.
> >
> >
> > >
> > > >
> > > > > Some of the DPUs are very
> > > > > lazy for cvq handle.
> > > >
> > > > Such design needs to be revisited, cvq (control path) should have a
> > > > better priority or QOS than datapath.
> > >
> > > Spec says nothing about this, so driver can't assume this either.
> > >
> > > > > In particular, we will also directly break the device.
> > > >
> > > > It's kind of hardening for malicious devices.
> > >
> > > ATM no amount of hardening can prevent a malicious hypervisor from
> > > blocking the guest. Recovering when a hardware device is broken would be
> > > nice but I think if we do bother then we should try harder to recover,
> > > such as by driving device reset.
> > >
> > >
> > > Also, does your patch break surprise removal? There's no callback
> > > in this case ATM.
> > >
> > > > >
> > > > > I think it is necessary to add a Virtio-Net parameter to allow users to define
> > > > > this timeout by themselves. Although I don't think this is a good way.
> > > >
> > > > Very hard and unfriendly to the end users.
> > > >
> > > > Thanks
> > > >
> > > > >
> > > > > Thanks.
> > > > >
> > > > >
> > > > > > gives the scheduler a breath and can let the process can respond to
> > > > > > asignal. If the device doesn't respond in the timeout, break the
> > > > > > device.
> > > > > >
> > > > > > Signed-off-by: Jason Wang <jasowang@redhat.com>
> > > > > > ---
> > > > > > Changes since V1:
> > > > > > - break the device when timeout
> > > > > > - get buffer manually since the virtio core check more_used() instead
> > > > > > ---
> > > > > >  drivers/net/virtio_net.c | 24 ++++++++++++++++--------
> > > > > >  1 file changed, 16 insertions(+), 8 deletions(-)
> > > > > >
> > > > > > diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
> > > > > > index efd9dd55828b..6a2ea64cfcb5 100644
> > > > > > --- a/drivers/net/virtio_net.c
> > > > > > +++ b/drivers/net/virtio_net.c
> > > > > > @@ -405,6 +405,7 @@ static void disable_rx_mode_work(struct virtnet_info *vi)
> > > > > >       vi->rx_mode_work_enabled = false;
> > > > > >       spin_unlock_bh(&vi->rx_mode_lock);
> > > > > >
> > > > > > +     virtqueue_wake_up(vi->cvq);
> > > > > >       flush_work(&vi->rx_mode_work);
> > > > > >  }
> > > > > >
> > > > > > @@ -1497,6 +1498,11 @@ static bool try_fill_recv(struct virtnet_info *vi, struct receive_queue *rq,
> > > > > >       return !oom;
> > > > > >  }
> > > > > >
> > > > > > +static void virtnet_cvq_done(struct virtqueue *cvq)
> > > > > > +{
> > > > > > +     virtqueue_wake_up(cvq);
> > > > > > +}
> > > > > > +
> > > > > >  static void skb_recv_done(struct virtqueue *rvq)
> > > > > >  {
> > > > > >       struct virtnet_info *vi = rvq->vdev->priv;
> > > > > > @@ -1984,6 +1990,8 @@ static int virtnet_tx_resize(struct virtnet_info *vi,
> > > > > >       return err;
> > > > > >  }
> > > > > >
> > > > > > +static int virtnet_close(struct net_device *dev);
> > > > > > +
> > > > > >  /*
> > > > > >   * Send command via the control virtqueue and check status.  Commands
> > > > > >   * supported by the hypervisor, as indicated by feature bits, should
> > > > > > @@ -2026,14 +2034,14 @@ 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();
> > > > > > +     if (virtqueue_wait_for_used(vi->cvq)) {
> > > > > > +             virtqueue_get_buf(vi->cvq, &tmp);
> > > > > > +             return vi->ctrl->status == VIRTIO_NET_OK;
> > > > > > +     }
> > > > > >
> > > > > > -     return vi->ctrl->status == VIRTIO_NET_OK;
> > > > > > +     netdev_err(vi->dev, "CVQ command timeout, break the virtio device.");
> > > > > > +     virtio_break_device(vi->vdev);
> > > > > > +     return VIRTIO_NET_ERR;
> > > > > >  }
> > > > > >
> > > > > >  static int virtnet_set_mac_address(struct net_device *dev, void *p)
> > > > > > @@ -3526,7 +3534,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";
> > > > > >       }
> > > > > >
> > > > > > --
> > > > > > 2.25.1
> > > > > >
> > > > > > _______________________________________________
> > > > > > Virtualization mailing list
> > > > > > Virtualization@lists.linux-foundation.org
> > > > > > https://lists.linuxfoundation.org/mailman/listinfo/virtualization
> > > > >
> > >
> >
>
Jason Wang Dec. 29, 2022, 3:22 a.m. UTC | #13
On Thu, Dec 29, 2022 at 10:10 AM Xuan Zhuo <xuanzhuo@linux.alibaba.com> wrote:
>
> On Wed, 28 Dec 2022 19:41:13 +0800, Jason Wang <jasowang@redhat.com> wrote:
> > On Wed, Dec 28, 2022 at 4:34 PM Xuan Zhuo <xuanzhuo@linux.alibaba.com> wrote:
> > >
> > > On Tue, 27 Dec 2022 01:58:22 -0500, "Michael S. Tsirkin" <mst@redhat.com> wrote:
> > > > On Tue, Dec 27, 2022 at 12:33:53PM +0800, Jason Wang wrote:
> > > > > On Tue, Dec 27, 2022 at 10:25 AM Xuan Zhuo <xuanzhuo@linux.alibaba.com> wrote:
> > > > > >
> > > > > > On Mon, 26 Dec 2022 15:49:08 +0800, Jason Wang <jasowang@redhat.com> wrote:
> > > > > > > We used to busy waiting on the cvq command this tends to be
> > > > > > > problematic since:
> > > > > > >
> > > > > > > 1) CPU could wait for ever on a buggy/malicous device
> > > > > > > 2) There's no wait to terminate the process that triggers the cvq
> > > > > > >    command
> > > > > > >
> > > > > > > So this patch switch to use virtqueue_wait_for_used() to sleep with a
> > > > > > > timeout (1s) instead of busy polling for the cvq command forever. This
> > > > > >
> > > > > > I don't think that a fixed 1S is a good choice.
> > > > >
> > > > > Well, it could be tweaked to be a little bit longer.
> > > > >
> > > > > One way, as discussed, is to let the device advertise a timeout then
> > > > > the driver can validate if it's valid and use that timeout. But it
> > > > > needs extension to the spec.
> > > >
> > > > Controlling timeout from device is a good idea, e.g. hardware devices
> > > > would benefit from a shorter timeout, hypervisor devices from a longer
> > > > timeout or no timeout.
> > >
> > > Yes. That is good.
> > >
> > > Before introducing this feature, I personally like to use "wait", rather than
> > > define a timeout.
> >
> > Note that the driver still needs to validate what device advertises to
> > avoid infinite wait.
>
> Sorry, I didn't understand what you mean.

I meant the interface needs to carefully designed to

1) avoid device to advertise a infinite (or very long) timeout
2) driver need to have its own max timeout regardless what device advertises

Thanks

>
> Thanks.
>
> >
> > Thanks
> >
> > >
> > > Thanks.
> > >
> > >
> > > >
> > > > >
> > > > > > Some of the DPUs are very
> > > > > > lazy for cvq handle.
> > > > >
> > > > > Such design needs to be revisited, cvq (control path) should have a
> > > > > better priority or QOS than datapath.
> > > >
> > > > Spec says nothing about this, so driver can't assume this either.
> > > >
> > > > > > In particular, we will also directly break the device.
> > > > >
> > > > > It's kind of hardening for malicious devices.
> > > >
> > > > ATM no amount of hardening can prevent a malicious hypervisor from
> > > > blocking the guest. Recovering when a hardware device is broken would be
> > > > nice but I think if we do bother then we should try harder to recover,
> > > > such as by driving device reset.
> > > >
> > > >
> > > > Also, does your patch break surprise removal? There's no callback
> > > > in this case ATM.
> > > >
> > > > > >
> > > > > > I think it is necessary to add a Virtio-Net parameter to allow users to define
> > > > > > this timeout by themselves. Although I don't think this is a good way.
> > > > >
> > > > > Very hard and unfriendly to the end users.
> > > > >
> > > > > Thanks
> > > > >
> > > > > >
> > > > > > Thanks.
> > > > > >
> > > > > >
> > > > > > > gives the scheduler a breath and can let the process can respond to
> > > > > > > asignal. If the device doesn't respond in the timeout, break the
> > > > > > > device.
> > > > > > >
> > > > > > > Signed-off-by: Jason Wang <jasowang@redhat.com>
> > > > > > > ---
> > > > > > > Changes since V1:
> > > > > > > - break the device when timeout
> > > > > > > - get buffer manually since the virtio core check more_used() instead
> > > > > > > ---
> > > > > > >  drivers/net/virtio_net.c | 24 ++++++++++++++++--------
> > > > > > >  1 file changed, 16 insertions(+), 8 deletions(-)
> > > > > > >
> > > > > > > diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
> > > > > > > index efd9dd55828b..6a2ea64cfcb5 100644
> > > > > > > --- a/drivers/net/virtio_net.c
> > > > > > > +++ b/drivers/net/virtio_net.c
> > > > > > > @@ -405,6 +405,7 @@ static void disable_rx_mode_work(struct virtnet_info *vi)
> > > > > > >       vi->rx_mode_work_enabled = false;
> > > > > > >       spin_unlock_bh(&vi->rx_mode_lock);
> > > > > > >
> > > > > > > +     virtqueue_wake_up(vi->cvq);
> > > > > > >       flush_work(&vi->rx_mode_work);
> > > > > > >  }
> > > > > > >
> > > > > > > @@ -1497,6 +1498,11 @@ static bool try_fill_recv(struct virtnet_info *vi, struct receive_queue *rq,
> > > > > > >       return !oom;
> > > > > > >  }
> > > > > > >
> > > > > > > +static void virtnet_cvq_done(struct virtqueue *cvq)
> > > > > > > +{
> > > > > > > +     virtqueue_wake_up(cvq);
> > > > > > > +}
> > > > > > > +
> > > > > > >  static void skb_recv_done(struct virtqueue *rvq)
> > > > > > >  {
> > > > > > >       struct virtnet_info *vi = rvq->vdev->priv;
> > > > > > > @@ -1984,6 +1990,8 @@ static int virtnet_tx_resize(struct virtnet_info *vi,
> > > > > > >       return err;
> > > > > > >  }
> > > > > > >
> > > > > > > +static int virtnet_close(struct net_device *dev);
> > > > > > > +
> > > > > > >  /*
> > > > > > >   * Send command via the control virtqueue and check status.  Commands
> > > > > > >   * supported by the hypervisor, as indicated by feature bits, should
> > > > > > > @@ -2026,14 +2034,14 @@ 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();
> > > > > > > +     if (virtqueue_wait_for_used(vi->cvq)) {
> > > > > > > +             virtqueue_get_buf(vi->cvq, &tmp);
> > > > > > > +             return vi->ctrl->status == VIRTIO_NET_OK;
> > > > > > > +     }
> > > > > > >
> > > > > > > -     return vi->ctrl->status == VIRTIO_NET_OK;
> > > > > > > +     netdev_err(vi->dev, "CVQ command timeout, break the virtio device.");
> > > > > > > +     virtio_break_device(vi->vdev);
> > > > > > > +     return VIRTIO_NET_ERR;
> > > > > > >  }
> > > > > > >
> > > > > > >  static int virtnet_set_mac_address(struct net_device *dev, void *p)
> > > > > > > @@ -3526,7 +3534,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";
> > > > > > >       }
> > > > > > >
> > > > > > > --
> > > > > > > 2.25.1
> > > > > > >
> > > > > > > _______________________________________________
> > > > > > > Virtualization mailing list
> > > > > > > Virtualization@lists.linux-foundation.org
> > > > > > > https://lists.linuxfoundation.org/mailman/listinfo/virtualization
> > > > > >
> > > >
> > >
> >
>
Xuan Zhuo Dec. 29, 2022, 3:41 a.m. UTC | #14
On Thu, 29 Dec 2022 11:22:13 +0800, Jason Wang <jasowang@redhat.com> wrote:
> On Thu, Dec 29, 2022 at 10:10 AM Xuan Zhuo <xuanzhuo@linux.alibaba.com> wrote:
> >
> > On Wed, 28 Dec 2022 19:41:13 +0800, Jason Wang <jasowang@redhat.com> wrote:
> > > On Wed, Dec 28, 2022 at 4:34 PM Xuan Zhuo <xuanzhuo@linux.alibaba.com> wrote:
> > > >
> > > > On Tue, 27 Dec 2022 01:58:22 -0500, "Michael S. Tsirkin" <mst@redhat.com> wrote:
> > > > > On Tue, Dec 27, 2022 at 12:33:53PM +0800, Jason Wang wrote:
> > > > > > On Tue, Dec 27, 2022 at 10:25 AM Xuan Zhuo <xuanzhuo@linux.alibaba.com> wrote:
> > > > > > >
> > > > > > > On Mon, 26 Dec 2022 15:49:08 +0800, Jason Wang <jasowang@redhat.com> wrote:
> > > > > > > > We used to busy waiting on the cvq command this tends to be
> > > > > > > > problematic since:
> > > > > > > >
> > > > > > > > 1) CPU could wait for ever on a buggy/malicous device
> > > > > > > > 2) There's no wait to terminate the process that triggers the cvq
> > > > > > > >    command
> > > > > > > >
> > > > > > > > So this patch switch to use virtqueue_wait_for_used() to sleep with a
> > > > > > > > timeout (1s) instead of busy polling for the cvq command forever. This
> > > > > > >
> > > > > > > I don't think that a fixed 1S is a good choice.
> > > > > >
> > > > > > Well, it could be tweaked to be a little bit longer.
> > > > > >
> > > > > > One way, as discussed, is to let the device advertise a timeout then
> > > > > > the driver can validate if it's valid and use that timeout. But it
> > > > > > needs extension to the spec.
> > > > >
> > > > > Controlling timeout from device is a good idea, e.g. hardware devices
> > > > > would benefit from a shorter timeout, hypervisor devices from a longer
> > > > > timeout or no timeout.
> > > >
> > > > Yes. That is good.
> > > >
> > > > Before introducing this feature, I personally like to use "wait", rather than
> > > > define a timeout.
> > >
> > > Note that the driver still needs to validate what device advertises to
> > > avoid infinite wait.
> >
> > Sorry, I didn't understand what you mean.
>
> I meant the interface needs to carefully designed to
>
> 1) avoid device to advertise a infinite (or very long) timeout
> 2) driver need to have its own max timeout regardless what device advertises


I see.

As far as I know, different operations will take different time.
For example, the queues are initialized one by one when performing
VIRTIO_NET_CTRL_MQ_VQ_PAIRS_SET commands. If the number of queues is large, then
this time will be very long.

So we should set different timeouts for different commands.

Thanks.

>
> Thanks
>
> >
> > Thanks.
> >
> > >
> > > Thanks
> > >
> > > >
> > > > Thanks.
> > > >
> > > >
> > > > >
> > > > > >
> > > > > > > Some of the DPUs are very
> > > > > > > lazy for cvq handle.
> > > > > >
> > > > > > Such design needs to be revisited, cvq (control path) should have a
> > > > > > better priority or QOS than datapath.
> > > > >
> > > > > Spec says nothing about this, so driver can't assume this either.
> > > > >
> > > > > > > In particular, we will also directly break the device.
> > > > > >
> > > > > > It's kind of hardening for malicious devices.
> > > > >
> > > > > ATM no amount of hardening can prevent a malicious hypervisor from
> > > > > blocking the guest. Recovering when a hardware device is broken would be
> > > > > nice but I think if we do bother then we should try harder to recover,
> > > > > such as by driving device reset.
> > > > >
> > > > >
> > > > > Also, does your patch break surprise removal? There's no callback
> > > > > in this case ATM.
> > > > >
> > > > > > >
> > > > > > > I think it is necessary to add a Virtio-Net parameter to allow users to define
> > > > > > > this timeout by themselves. Although I don't think this is a good way.
> > > > > >
> > > > > > Very hard and unfriendly to the end users.
> > > > > >
> > > > > > Thanks
> > > > > >
> > > > > > >
> > > > > > > Thanks.
> > > > > > >
> > > > > > >
> > > > > > > > gives the scheduler a breath and can let the process can respond to
> > > > > > > > asignal. If the device doesn't respond in the timeout, break the
> > > > > > > > device.
> > > > > > > >
> > > > > > > > Signed-off-by: Jason Wang <jasowang@redhat.com>
> > > > > > > > ---
> > > > > > > > Changes since V1:
> > > > > > > > - break the device when timeout
> > > > > > > > - get buffer manually since the virtio core check more_used() instead
> > > > > > > > ---
> > > > > > > >  drivers/net/virtio_net.c | 24 ++++++++++++++++--------
> > > > > > > >  1 file changed, 16 insertions(+), 8 deletions(-)
> > > > > > > >
> > > > > > > > diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
> > > > > > > > index efd9dd55828b..6a2ea64cfcb5 100644
> > > > > > > > --- a/drivers/net/virtio_net.c
> > > > > > > > +++ b/drivers/net/virtio_net.c
> > > > > > > > @@ -405,6 +405,7 @@ static void disable_rx_mode_work(struct virtnet_info *vi)
> > > > > > > >       vi->rx_mode_work_enabled = false;
> > > > > > > >       spin_unlock_bh(&vi->rx_mode_lock);
> > > > > > > >
> > > > > > > > +     virtqueue_wake_up(vi->cvq);
> > > > > > > >       flush_work(&vi->rx_mode_work);
> > > > > > > >  }
> > > > > > > >
> > > > > > > > @@ -1497,6 +1498,11 @@ static bool try_fill_recv(struct virtnet_info *vi, struct receive_queue *rq,
> > > > > > > >       return !oom;
> > > > > > > >  }
> > > > > > > >
> > > > > > > > +static void virtnet_cvq_done(struct virtqueue *cvq)
> > > > > > > > +{
> > > > > > > > +     virtqueue_wake_up(cvq);
> > > > > > > > +}
> > > > > > > > +
> > > > > > > >  static void skb_recv_done(struct virtqueue *rvq)
> > > > > > > >  {
> > > > > > > >       struct virtnet_info *vi = rvq->vdev->priv;
> > > > > > > > @@ -1984,6 +1990,8 @@ static int virtnet_tx_resize(struct virtnet_info *vi,
> > > > > > > >       return err;
> > > > > > > >  }
> > > > > > > >
> > > > > > > > +static int virtnet_close(struct net_device *dev);
> > > > > > > > +
> > > > > > > >  /*
> > > > > > > >   * Send command via the control virtqueue and check status.  Commands
> > > > > > > >   * supported by the hypervisor, as indicated by feature bits, should
> > > > > > > > @@ -2026,14 +2034,14 @@ 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();
> > > > > > > > +     if (virtqueue_wait_for_used(vi->cvq)) {
> > > > > > > > +             virtqueue_get_buf(vi->cvq, &tmp);
> > > > > > > > +             return vi->ctrl->status == VIRTIO_NET_OK;
> > > > > > > > +     }
> > > > > > > >
> > > > > > > > -     return vi->ctrl->status == VIRTIO_NET_OK;
> > > > > > > > +     netdev_err(vi->dev, "CVQ command timeout, break the virtio device.");
> > > > > > > > +     virtio_break_device(vi->vdev);
> > > > > > > > +     return VIRTIO_NET_ERR;
> > > > > > > >  }
> > > > > > > >
> > > > > > > >  static int virtnet_set_mac_address(struct net_device *dev, void *p)
> > > > > > > > @@ -3526,7 +3534,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";
> > > > > > > >       }
> > > > > > > >
> > > > > > > > --
> > > > > > > > 2.25.1
> > > > > > > >
> > > > > > > > _______________________________________________
> > > > > > > > Virtualization mailing list
> > > > > > > > Virtualization@lists.linux-foundation.org
> > > > > > > > https://lists.linuxfoundation.org/mailman/listinfo/virtualization
> > > > > > >
> > > > >
> > > >
> > >
> >
>
Jason Wang Dec. 29, 2022, 4:08 a.m. UTC | #15
On Thu, Dec 29, 2022 at 11:49 AM Xuan Zhuo <xuanzhuo@linux.alibaba.com> wrote:
>
> On Thu, 29 Dec 2022 11:22:13 +0800, Jason Wang <jasowang@redhat.com> wrote:
> > On Thu, Dec 29, 2022 at 10:10 AM Xuan Zhuo <xuanzhuo@linux.alibaba.com> wrote:
> > >
> > > On Wed, 28 Dec 2022 19:41:13 +0800, Jason Wang <jasowang@redhat.com> wrote:
> > > > On Wed, Dec 28, 2022 at 4:34 PM Xuan Zhuo <xuanzhuo@linux.alibaba.com> wrote:
> > > > >
> > > > > On Tue, 27 Dec 2022 01:58:22 -0500, "Michael S. Tsirkin" <mst@redhat.com> wrote:
> > > > > > On Tue, Dec 27, 2022 at 12:33:53PM +0800, Jason Wang wrote:
> > > > > > > On Tue, Dec 27, 2022 at 10:25 AM Xuan Zhuo <xuanzhuo@linux.alibaba.com> wrote:
> > > > > > > >
> > > > > > > > On Mon, 26 Dec 2022 15:49:08 +0800, Jason Wang <jasowang@redhat.com> wrote:
> > > > > > > > > We used to busy waiting on the cvq command this tends to be
> > > > > > > > > problematic since:
> > > > > > > > >
> > > > > > > > > 1) CPU could wait for ever on a buggy/malicous device
> > > > > > > > > 2) There's no wait to terminate the process that triggers the cvq
> > > > > > > > >    command
> > > > > > > > >
> > > > > > > > > So this patch switch to use virtqueue_wait_for_used() to sleep with a
> > > > > > > > > timeout (1s) instead of busy polling for the cvq command forever. This
> > > > > > > >
> > > > > > > > I don't think that a fixed 1S is a good choice.
> > > > > > >
> > > > > > > Well, it could be tweaked to be a little bit longer.
> > > > > > >
> > > > > > > One way, as discussed, is to let the device advertise a timeout then
> > > > > > > the driver can validate if it's valid and use that timeout. But it
> > > > > > > needs extension to the spec.
> > > > > >
> > > > > > Controlling timeout from device is a good idea, e.g. hardware devices
> > > > > > would benefit from a shorter timeout, hypervisor devices from a longer
> > > > > > timeout or no timeout.
> > > > >
> > > > > Yes. That is good.
> > > > >
> > > > > Before introducing this feature, I personally like to use "wait", rather than
> > > > > define a timeout.
> > > >
> > > > Note that the driver still needs to validate what device advertises to
> > > > avoid infinite wait.
> > >
> > > Sorry, I didn't understand what you mean.
> >
> > I meant the interface needs to carefully designed to
> >
> > 1) avoid device to advertise a infinite (or very long) timeout
> > 2) driver need to have its own max timeout regardless what device advertises
>
>
> I see.
>
> As far as I know, different operations will take different time.
> For example, the queues are initialized one by one when performing
> VIRTIO_NET_CTRL_MQ_VQ_PAIRS_SET commands. If the number of queues is large, then
> this time will be very long.

I see. This is the case even for the software backends.

>
> So we should set different timeouts for different commands.

Probably but it would result in a very complex interface, the device
can just choose to advertise the maximum timeout of all the commands
in this case. As discussed, I think we can start a very long timeout.
Is 1 minutes sufficient in this case?

Thanks

>
> Thanks.
>
> >
> > Thanks
> >
> > >
> > > Thanks.
> > >
> > > >
> > > > Thanks
> > > >
> > > > >
> > > > > Thanks.
> > > > >
> > > > >
> > > > > >
> > > > > > >
> > > > > > > > Some of the DPUs are very
> > > > > > > > lazy for cvq handle.
> > > > > > >
> > > > > > > Such design needs to be revisited, cvq (control path) should have a
> > > > > > > better priority or QOS than datapath.
> > > > > >
> > > > > > Spec says nothing about this, so driver can't assume this either.
> > > > > >
> > > > > > > > In particular, we will also directly break the device.
> > > > > > >
> > > > > > > It's kind of hardening for malicious devices.
> > > > > >
> > > > > > ATM no amount of hardening can prevent a malicious hypervisor from
> > > > > > blocking the guest. Recovering when a hardware device is broken would be
> > > > > > nice but I think if we do bother then we should try harder to recover,
> > > > > > such as by driving device reset.
> > > > > >
> > > > > >
> > > > > > Also, does your patch break surprise removal? There's no callback
> > > > > > in this case ATM.
> > > > > >
> > > > > > > >
> > > > > > > > I think it is necessary to add a Virtio-Net parameter to allow users to define
> > > > > > > > this timeout by themselves. Although I don't think this is a good way.
> > > > > > >
> > > > > > > Very hard and unfriendly to the end users.
> > > > > > >
> > > > > > > Thanks
> > > > > > >
> > > > > > > >
> > > > > > > > Thanks.
> > > > > > > >
> > > > > > > >
> > > > > > > > > gives the scheduler a breath and can let the process can respond to
> > > > > > > > > asignal. If the device doesn't respond in the timeout, break the
> > > > > > > > > device.
> > > > > > > > >
> > > > > > > > > Signed-off-by: Jason Wang <jasowang@redhat.com>
> > > > > > > > > ---
> > > > > > > > > Changes since V1:
> > > > > > > > > - break the device when timeout
> > > > > > > > > - get buffer manually since the virtio core check more_used() instead
> > > > > > > > > ---
> > > > > > > > >  drivers/net/virtio_net.c | 24 ++++++++++++++++--------
> > > > > > > > >  1 file changed, 16 insertions(+), 8 deletions(-)
> > > > > > > > >
> > > > > > > > > diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
> > > > > > > > > index efd9dd55828b..6a2ea64cfcb5 100644
> > > > > > > > > --- a/drivers/net/virtio_net.c
> > > > > > > > > +++ b/drivers/net/virtio_net.c
> > > > > > > > > @@ -405,6 +405,7 @@ static void disable_rx_mode_work(struct virtnet_info *vi)
> > > > > > > > >       vi->rx_mode_work_enabled = false;
> > > > > > > > >       spin_unlock_bh(&vi->rx_mode_lock);
> > > > > > > > >
> > > > > > > > > +     virtqueue_wake_up(vi->cvq);
> > > > > > > > >       flush_work(&vi->rx_mode_work);
> > > > > > > > >  }
> > > > > > > > >
> > > > > > > > > @@ -1497,6 +1498,11 @@ static bool try_fill_recv(struct virtnet_info *vi, struct receive_queue *rq,
> > > > > > > > >       return !oom;
> > > > > > > > >  }
> > > > > > > > >
> > > > > > > > > +static void virtnet_cvq_done(struct virtqueue *cvq)
> > > > > > > > > +{
> > > > > > > > > +     virtqueue_wake_up(cvq);
> > > > > > > > > +}
> > > > > > > > > +
> > > > > > > > >  static void skb_recv_done(struct virtqueue *rvq)
> > > > > > > > >  {
> > > > > > > > >       struct virtnet_info *vi = rvq->vdev->priv;
> > > > > > > > > @@ -1984,6 +1990,8 @@ static int virtnet_tx_resize(struct virtnet_info *vi,
> > > > > > > > >       return err;
> > > > > > > > >  }
> > > > > > > > >
> > > > > > > > > +static int virtnet_close(struct net_device *dev);
> > > > > > > > > +
> > > > > > > > >  /*
> > > > > > > > >   * Send command via the control virtqueue and check status.  Commands
> > > > > > > > >   * supported by the hypervisor, as indicated by feature bits, should
> > > > > > > > > @@ -2026,14 +2034,14 @@ 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();
> > > > > > > > > +     if (virtqueue_wait_for_used(vi->cvq)) {
> > > > > > > > > +             virtqueue_get_buf(vi->cvq, &tmp);
> > > > > > > > > +             return vi->ctrl->status == VIRTIO_NET_OK;
> > > > > > > > > +     }
> > > > > > > > >
> > > > > > > > > -     return vi->ctrl->status == VIRTIO_NET_OK;
> > > > > > > > > +     netdev_err(vi->dev, "CVQ command timeout, break the virtio device.");
> > > > > > > > > +     virtio_break_device(vi->vdev);
> > > > > > > > > +     return VIRTIO_NET_ERR;
> > > > > > > > >  }
> > > > > > > > >
> > > > > > > > >  static int virtnet_set_mac_address(struct net_device *dev, void *p)
> > > > > > > > > @@ -3526,7 +3534,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";
> > > > > > > > >       }
> > > > > > > > >
> > > > > > > > > --
> > > > > > > > > 2.25.1
> > > > > > > > >
> > > > > > > > > _______________________________________________
> > > > > > > > > Virtualization mailing list
> > > > > > > > > Virtualization@lists.linux-foundation.org
> > > > > > > > > https://lists.linuxfoundation.org/mailman/listinfo/virtualization
> > > > > > > >
> > > > > >
> > > > >
> > > >
> > >
> >
>
Xuan Zhuo Dec. 29, 2022, 6:13 a.m. UTC | #16
On Thu, 29 Dec 2022 12:08:23 +0800, Jason Wang <jasowang@redhat.com> wrote:
> On Thu, Dec 29, 2022 at 11:49 AM Xuan Zhuo <xuanzhuo@linux.alibaba.com> wrote:
> >
> > On Thu, 29 Dec 2022 11:22:13 +0800, Jason Wang <jasowang@redhat.com> wrote:
> > > On Thu, Dec 29, 2022 at 10:10 AM Xuan Zhuo <xuanzhuo@linux.alibaba.com> wrote:
> > > >
> > > > On Wed, 28 Dec 2022 19:41:13 +0800, Jason Wang <jasowang@redhat.com> wrote:
> > > > > On Wed, Dec 28, 2022 at 4:34 PM Xuan Zhuo <xuanzhuo@linux.alibaba.com> wrote:
> > > > > >
> > > > > > On Tue, 27 Dec 2022 01:58:22 -0500, "Michael S. Tsirkin" <mst@redhat.com> wrote:
> > > > > > > On Tue, Dec 27, 2022 at 12:33:53PM +0800, Jason Wang wrote:
> > > > > > > > On Tue, Dec 27, 2022 at 10:25 AM Xuan Zhuo <xuanzhuo@linux.alibaba.com> wrote:
> > > > > > > > >
> > > > > > > > > On Mon, 26 Dec 2022 15:49:08 +0800, Jason Wang <jasowang@redhat.com> wrote:
> > > > > > > > > > We used to busy waiting on the cvq command this tends to be
> > > > > > > > > > problematic since:
> > > > > > > > > >
> > > > > > > > > > 1) CPU could wait for ever on a buggy/malicous device
> > > > > > > > > > 2) There's no wait to terminate the process that triggers the cvq
> > > > > > > > > >    command
> > > > > > > > > >
> > > > > > > > > > So this patch switch to use virtqueue_wait_for_used() to sleep with a
> > > > > > > > > > timeout (1s) instead of busy polling for the cvq command forever. This
> > > > > > > > >
> > > > > > > > > I don't think that a fixed 1S is a good choice.
> > > > > > > >
> > > > > > > > Well, it could be tweaked to be a little bit longer.
> > > > > > > >
> > > > > > > > One way, as discussed, is to let the device advertise a timeout then
> > > > > > > > the driver can validate if it's valid and use that timeout. But it
> > > > > > > > needs extension to the spec.
> > > > > > >
> > > > > > > Controlling timeout from device is a good idea, e.g. hardware devices
> > > > > > > would benefit from a shorter timeout, hypervisor devices from a longer
> > > > > > > timeout or no timeout.
> > > > > >
> > > > > > Yes. That is good.
> > > > > >
> > > > > > Before introducing this feature, I personally like to use "wait", rather than
> > > > > > define a timeout.
> > > > >
> > > > > Note that the driver still needs to validate what device advertises to
> > > > > avoid infinite wait.
> > > >
> > > > Sorry, I didn't understand what you mean.
> > >
> > > I meant the interface needs to carefully designed to
> > >
> > > 1) avoid device to advertise a infinite (or very long) timeout
> > > 2) driver need to have its own max timeout regardless what device advertises
> >
> >
> > I see.
> >
> > As far as I know, different operations will take different time.
> > For example, the queues are initialized one by one when performing
> > VIRTIO_NET_CTRL_MQ_VQ_PAIRS_SET commands. If the number of queues is large, then
> > this time will be very long.
>
> I see. This is the case even for the software backends.
>
> >
> > So we should set different timeouts for different commands.
>
> Probably but it would result in a very complex interface, the device
> can just choose to advertise the maximum timeout of all the commands
> in this case. As discussed, I think we can start a very long timeout.
> Is 1 minutes sufficient in this case?


For now, 1 minutes are safe.

Thanks.

>
> Thanks
>
> >
> > Thanks.
> >
> > >
> > > Thanks
> > >
> > > >
> > > > Thanks.
> > > >
> > > > >
> > > > > Thanks
> > > > >
> > > > > >
> > > > > > Thanks.
> > > > > >
> > > > > >
> > > > > > >
> > > > > > > >
> > > > > > > > > Some of the DPUs are very
> > > > > > > > > lazy for cvq handle.
> > > > > > > >
> > > > > > > > Such design needs to be revisited, cvq (control path) should have a
> > > > > > > > better priority or QOS than datapath.
> > > > > > >
> > > > > > > Spec says nothing about this, so driver can't assume this either.
> > > > > > >
> > > > > > > > > In particular, we will also directly break the device.
> > > > > > > >
> > > > > > > > It's kind of hardening for malicious devices.
> > > > > > >
> > > > > > > ATM no amount of hardening can prevent a malicious hypervisor from
> > > > > > > blocking the guest. Recovering when a hardware device is broken would be
> > > > > > > nice but I think if we do bother then we should try harder to recover,
> > > > > > > such as by driving device reset.
> > > > > > >
> > > > > > >
> > > > > > > Also, does your patch break surprise removal? There's no callback
> > > > > > > in this case ATM.
> > > > > > >
> > > > > > > > >
> > > > > > > > > I think it is necessary to add a Virtio-Net parameter to allow users to define
> > > > > > > > > this timeout by themselves. Although I don't think this is a good way.
> > > > > > > >
> > > > > > > > Very hard and unfriendly to the end users.
> > > > > > > >
> > > > > > > > Thanks
> > > > > > > >
> > > > > > > > >
> > > > > > > > > Thanks.
> > > > > > > > >
> > > > > > > > >
> > > > > > > > > > gives the scheduler a breath and can let the process can respond to
> > > > > > > > > > asignal. If the device doesn't respond in the timeout, break the
> > > > > > > > > > device.
> > > > > > > > > >
> > > > > > > > > > Signed-off-by: Jason Wang <jasowang@redhat.com>
> > > > > > > > > > ---
> > > > > > > > > > Changes since V1:
> > > > > > > > > > - break the device when timeout
> > > > > > > > > > - get buffer manually since the virtio core check more_used() instead
> > > > > > > > > > ---
> > > > > > > > > >  drivers/net/virtio_net.c | 24 ++++++++++++++++--------
> > > > > > > > > >  1 file changed, 16 insertions(+), 8 deletions(-)
> > > > > > > > > >
> > > > > > > > > > diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
> > > > > > > > > > index efd9dd55828b..6a2ea64cfcb5 100644
> > > > > > > > > > --- a/drivers/net/virtio_net.c
> > > > > > > > > > +++ b/drivers/net/virtio_net.c
> > > > > > > > > > @@ -405,6 +405,7 @@ static void disable_rx_mode_work(struct virtnet_info *vi)
> > > > > > > > > >       vi->rx_mode_work_enabled = false;
> > > > > > > > > >       spin_unlock_bh(&vi->rx_mode_lock);
> > > > > > > > > >
> > > > > > > > > > +     virtqueue_wake_up(vi->cvq);
> > > > > > > > > >       flush_work(&vi->rx_mode_work);
> > > > > > > > > >  }
> > > > > > > > > >
> > > > > > > > > > @@ -1497,6 +1498,11 @@ static bool try_fill_recv(struct virtnet_info *vi, struct receive_queue *rq,
> > > > > > > > > >       return !oom;
> > > > > > > > > >  }
> > > > > > > > > >
> > > > > > > > > > +static void virtnet_cvq_done(struct virtqueue *cvq)
> > > > > > > > > > +{
> > > > > > > > > > +     virtqueue_wake_up(cvq);
> > > > > > > > > > +}
> > > > > > > > > > +
> > > > > > > > > >  static void skb_recv_done(struct virtqueue *rvq)
> > > > > > > > > >  {
> > > > > > > > > >       struct virtnet_info *vi = rvq->vdev->priv;
> > > > > > > > > > @@ -1984,6 +1990,8 @@ static int virtnet_tx_resize(struct virtnet_info *vi,
> > > > > > > > > >       return err;
> > > > > > > > > >  }
> > > > > > > > > >
> > > > > > > > > > +static int virtnet_close(struct net_device *dev);
> > > > > > > > > > +
> > > > > > > > > >  /*
> > > > > > > > > >   * Send command via the control virtqueue and check status.  Commands
> > > > > > > > > >   * supported by the hypervisor, as indicated by feature bits, should
> > > > > > > > > > @@ -2026,14 +2034,14 @@ 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();
> > > > > > > > > > +     if (virtqueue_wait_for_used(vi->cvq)) {
> > > > > > > > > > +             virtqueue_get_buf(vi->cvq, &tmp);
> > > > > > > > > > +             return vi->ctrl->status == VIRTIO_NET_OK;
> > > > > > > > > > +     }
> > > > > > > > > >
> > > > > > > > > > -     return vi->ctrl->status == VIRTIO_NET_OK;
> > > > > > > > > > +     netdev_err(vi->dev, "CVQ command timeout, break the virtio device.");
> > > > > > > > > > +     virtio_break_device(vi->vdev);
> > > > > > > > > > +     return VIRTIO_NET_ERR;
> > > > > > > > > >  }
> > > > > > > > > >
> > > > > > > > > >  static int virtnet_set_mac_address(struct net_device *dev, void *p)
> > > > > > > > > > @@ -3526,7 +3534,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";
> > > > > > > > > >       }
> > > > > > > > > >
> > > > > > > > > > --
> > > > > > > > > > 2.25.1
> > > > > > > > > >
> > > > > > > > > > _______________________________________________
> > > > > > > > > > Virtualization mailing list
> > > > > > > > > > Virtualization@lists.linux-foundation.org
> > > > > > > > > > https://lists.linuxfoundation.org/mailman/listinfo/virtualization
> > > > > > > > >
> > > > > > >
> > > > > >
> > > > >
> > > >
> > >
> >
>
diff mbox series

Patch

diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
index efd9dd55828b..6a2ea64cfcb5 100644
--- a/drivers/net/virtio_net.c
+++ b/drivers/net/virtio_net.c
@@ -405,6 +405,7 @@  static void disable_rx_mode_work(struct virtnet_info *vi)
 	vi->rx_mode_work_enabled = false;
 	spin_unlock_bh(&vi->rx_mode_lock);
 
+	virtqueue_wake_up(vi->cvq);
 	flush_work(&vi->rx_mode_work);
 }
 
@@ -1497,6 +1498,11 @@  static bool try_fill_recv(struct virtnet_info *vi, struct receive_queue *rq,
 	return !oom;
 }
 
+static void virtnet_cvq_done(struct virtqueue *cvq)
+{
+	virtqueue_wake_up(cvq);
+}
+
 static void skb_recv_done(struct virtqueue *rvq)
 {
 	struct virtnet_info *vi = rvq->vdev->priv;
@@ -1984,6 +1990,8 @@  static int virtnet_tx_resize(struct virtnet_info *vi,
 	return err;
 }
 
+static int virtnet_close(struct net_device *dev);
+
 /*
  * Send command via the control virtqueue and check status.  Commands
  * supported by the hypervisor, as indicated by feature bits, should
@@ -2026,14 +2034,14 @@  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();
+	if (virtqueue_wait_for_used(vi->cvq)) {
+		virtqueue_get_buf(vi->cvq, &tmp);
+		return vi->ctrl->status == VIRTIO_NET_OK;
+	}
 
-	return vi->ctrl->status == VIRTIO_NET_OK;
+	netdev_err(vi->dev, "CVQ command timeout, break the virtio device.");
+	virtio_break_device(vi->vdev);
+	return VIRTIO_NET_ERR;
 }
 
 static int virtnet_set_mac_address(struct net_device *dev, void *p)
@@ -3526,7 +3534,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";
 	}