Adding cond_resched() to the command waiting loop for a better
co-operation with the scheduler. This allows to give CPU a breath to
run other task(workqueue) instead of busy looping when preemption is
not allowed on a device whose CVQ might be slow.
Signed-off-by: Jason Wang <[email protected]>
---
drivers/net/virtio_net.c | 4 +++-
1 file changed, 3 insertions(+), 1 deletion(-)
diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
index 9f3b1d6ac33d..e7533f29b219 100644
--- a/drivers/net/virtio_net.c
+++ b/drivers/net/virtio_net.c
@@ -2314,8 +2314,10 @@ static bool virtnet_send_command(struct virtnet_info *vi, u8 class, u8 cmd,
* into the hypervisor, so the request should be handled immediately.
*/
while (!virtqueue_get_buf(vi->cvq, &tmp) &&
- !virtqueue_is_broken(vi->cvq))
+ !virtqueue_is_broken(vi->cvq)) {
+ cond_resched();
cpu_relax();
+ }
return vi->ctrl->status == VIRTIO_NET_OK;
}
--
2.39.3
On 7/20/23 1:38 AM, Jason Wang wrote:
>
> Adding cond_resched() to the command waiting loop for a better
> co-operation with the scheduler. This allows to give CPU a breath to
> run other task(workqueue) instead of busy looping when preemption is
> not allowed on a device whose CVQ might be slow.
>
> Signed-off-by: Jason Wang <[email protected]>
> ---
> drivers/net/virtio_net.c | 4 +++-
> 1 file changed, 3 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
> index 9f3b1d6ac33d..e7533f29b219 100644
> --- a/drivers/net/virtio_net.c
> +++ b/drivers/net/virtio_net.c
> @@ -2314,8 +2314,10 @@ static bool virtnet_send_command(struct virtnet_info *vi, u8 class, u8 cmd,
> * into the hypervisor, so the request should be handled immediately.
> */
> while (!virtqueue_get_buf(vi->cvq, &tmp) &&
> - !virtqueue_is_broken(vi->cvq))
> + !virtqueue_is_broken(vi->cvq)) {
> + cond_resched();
> cpu_relax();
> + }
The cover letter suggests that this addresses the infinite poll for
buggy devices, but I don't see how that is resolved here. This should
make it a little nicer to the system, but it still is going to poll
forever on a device that has gone catatonic. Is there a reason that I'm
missing that we don't have a polling limit here?
sln
>
> return vi->ctrl->status == VIRTIO_NET_OK;
> }
> --
> 2.39.3
>
> _______________________________________________
> Virtualization mailing list
> [email protected]
> https://lists.linuxfoundation.org/mailman/listinfo/virtualization
On 7/20/23 1:38 AM, Jason Wang wrote:
>
> Adding cond_resched() to the command waiting loop for a better
> co-operation with the scheduler. This allows to give CPU a breath to
> run other task(workqueue) instead of busy looping when preemption is
> not allowed on a device whose CVQ might be slow.
>
> Signed-off-by: Jason Wang <[email protected]>
This still leaves hung processes, but at least it doesn't pin the CPU
any more. Thanks.
Reviewed-by: Shannon Nelson <[email protected]>
> ---
> drivers/net/virtio_net.c | 4 +++-
> 1 file changed, 3 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
> index 9f3b1d6ac33d..e7533f29b219 100644
> --- a/drivers/net/virtio_net.c
> +++ b/drivers/net/virtio_net.c
> @@ -2314,8 +2314,10 @@ static bool virtnet_send_command(struct virtnet_info *vi, u8 class, u8 cmd,
> * into the hypervisor, so the request should be handled immediately.
> */
> while (!virtqueue_get_buf(vi->cvq, &tmp) &&
> - !virtqueue_is_broken(vi->cvq))
> + !virtqueue_is_broken(vi->cvq)) {
> + cond_resched();
> cpu_relax();
> + }
>
> return vi->ctrl->status == VIRTIO_NET_OK;
> }
> --
> 2.39.3
>
> _______________________________________________
> Virtualization mailing list
> [email protected]
> https://lists.linuxfoundation.org/mailman/listinfo/virtualization
On Thu, Jul 20, 2023 at 08:31:13AM -0700, Shannon Nelson wrote:
> On 7/20/23 1:38 AM, Jason Wang wrote:
> >
> > Adding cond_resched() to the command waiting loop for a better
> > co-operation with the scheduler. This allows to give CPU a breath to
> > run other task(workqueue) instead of busy looping when preemption is
> > not allowed on a device whose CVQ might be slow.
> >
> > Signed-off-by: Jason Wang <[email protected]>
> > ---
> > drivers/net/virtio_net.c | 4 +++-
> > 1 file changed, 3 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
> > index 9f3b1d6ac33d..e7533f29b219 100644
> > --- a/drivers/net/virtio_net.c
> > +++ b/drivers/net/virtio_net.c
> > @@ -2314,8 +2314,10 @@ static bool virtnet_send_command(struct virtnet_info *vi, u8 class, u8 cmd,
> > * into the hypervisor, so the request should be handled immediately.
> > */
> > while (!virtqueue_get_buf(vi->cvq, &tmp) &&
> > - !virtqueue_is_broken(vi->cvq))
> > + !virtqueue_is_broken(vi->cvq)) {
> > + cond_resched();
> > cpu_relax();
> > + }
>
> The cover letter suggests that this addresses the infinite poll for buggy
> devices, but I don't see how that is resolved here. This should make it a
> little nicer to the system, but it still is going to poll forever on a
> device that has gone catatonic. Is there a reason that I'm missing that we
> don't have a polling limit here?
>
> sln
we don't know what the limit would be. but given it's a workqueue
now, why does it still have to poll as opposed to blocking?
> >
> > return vi->ctrl->status == VIRTIO_NET_OK;
> > }
> > --
> > 2.39.3
> >
> > _______________________________________________
> > Virtualization mailing list
> > [email protected]
> > https://lists.linuxfoundation.org/mailman/listinfo/virtualization
On Thu, Jul 20, 2023 at 01:26:20PM -0700, Shannon Nelson wrote:
> On 7/20/23 1:38 AM, Jason Wang wrote:
> >
> > Adding cond_resched() to the command waiting loop for a better
> > co-operation with the scheduler. This allows to give CPU a breath to
> > run other task(workqueue) instead of busy looping when preemption is
> > not allowed on a device whose CVQ might be slow.
> >
> > Signed-off-by: Jason Wang <[email protected]>
>
> This still leaves hung processes, but at least it doesn't pin the CPU any
> more. Thanks.
> Reviewed-by: Shannon Nelson <[email protected]>
>
I'd like to see a full solution
1- block until interrupt
2- still handle surprise removal correctly by waking in that case
> > ---
> > drivers/net/virtio_net.c | 4 +++-
> > 1 file changed, 3 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
> > index 9f3b1d6ac33d..e7533f29b219 100644
> > --- a/drivers/net/virtio_net.c
> > +++ b/drivers/net/virtio_net.c
> > @@ -2314,8 +2314,10 @@ static bool virtnet_send_command(struct virtnet_info *vi, u8 class, u8 cmd,
> > * into the hypervisor, so the request should be handled immediately.
> > */
> > while (!virtqueue_get_buf(vi->cvq, &tmp) &&
> > - !virtqueue_is_broken(vi->cvq))
> > + !virtqueue_is_broken(vi->cvq)) {
> > + cond_resched();
> > cpu_relax();
> > + }
> >
> > return vi->ctrl->status == VIRTIO_NET_OK;
> > }
> > --
> > 2.39.3
> >
> > _______________________________________________
> > Virtualization mailing list
> > [email protected]
> > https://lists.linuxfoundation.org/mailman/listinfo/virtualization
On 7/20/23 23:02, Michael S. Tsirkin wrote:
> On Thu, Jul 20, 2023 at 01:26:20PM -0700, Shannon Nelson wrote:
>> On 7/20/23 1:38 AM, Jason Wang wrote:
>>>
>>> Adding cond_resched() to the command waiting loop for a better
>>> co-operation with the scheduler. This allows to give CPU a breath to
>>> run other task(workqueue) instead of busy looping when preemption is
>>> not allowed on a device whose CVQ might be slow.
>>>
>>> Signed-off-by: Jason Wang <[email protected]>
>>
>> This still leaves hung processes, but at least it doesn't pin the CPU any
>> more. Thanks.
>> Reviewed-by: Shannon Nelson <[email protected]>
>>
>
> I'd like to see a full solution
> 1- block until interrupt
Would it make sense to also have a timeout?
And when timeout expires, set FAILED bit in device status?
> 2- still handle surprise removal correctly by waking in that case
>
>
>
>>> ---
>>> drivers/net/virtio_net.c | 4 +++-
>>> 1 file changed, 3 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
>>> index 9f3b1d6ac33d..e7533f29b219 100644
>>> --- a/drivers/net/virtio_net.c
>>> +++ b/drivers/net/virtio_net.c
>>> @@ -2314,8 +2314,10 @@ static bool virtnet_send_command(struct virtnet_info *vi, u8 class, u8 cmd,
>>> * into the hypervisor, so the request should be handled immediately.
>>> */
>>> while (!virtqueue_get_buf(vi->cvq, &tmp) &&
>>> - !virtqueue_is_broken(vi->cvq))
>>> + !virtqueue_is_broken(vi->cvq)) {
>>> + cond_resched();
>>> cpu_relax();
>>> + }
>>>
>>> return vi->ctrl->status == VIRTIO_NET_OK;
>>> }
>>> --
>>> 2.39.3
>>>
>>> _______________________________________________
>>> Virtualization mailing list
>>> [email protected]
>>> https://lists.linuxfoundation.org/mailman/listinfo/virtualization
>
On Fri, Jul 21, 2023 at 04:37:00PM +0200, Maxime Coquelin wrote:
>
>
> On 7/20/23 23:02, Michael S. Tsirkin wrote:
> > On Thu, Jul 20, 2023 at 01:26:20PM -0700, Shannon Nelson wrote:
> > > On 7/20/23 1:38 AM, Jason Wang wrote:
> > > >
> > > > Adding cond_resched() to the command waiting loop for a better
> > > > co-operation with the scheduler. This allows to give CPU a breath to
> > > > run other task(workqueue) instead of busy looping when preemption is
> > > > not allowed on a device whose CVQ might be slow.
> > > >
> > > > Signed-off-by: Jason Wang <[email protected]>
> > >
> > > This still leaves hung processes, but at least it doesn't pin the CPU any
> > > more. Thanks.
> > > Reviewed-by: Shannon Nelson <[email protected]>
> > >
> >
> > I'd like to see a full solution
> > 1- block until interrupt
>
> Would it make sense to also have a timeout?
> And when timeout expires, set FAILED bit in device status?
virtio spec does not set any limits on the timing of vq
processing.
> > 2- still handle surprise removal correctly by waking in that case
> >
> >
> >
> > > > ---
> > > > drivers/net/virtio_net.c | 4 +++-
> > > > 1 file changed, 3 insertions(+), 1 deletion(-)
> > > >
> > > > diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
> > > > index 9f3b1d6ac33d..e7533f29b219 100644
> > > > --- a/drivers/net/virtio_net.c
> > > > +++ b/drivers/net/virtio_net.c
> > > > @@ -2314,8 +2314,10 @@ static bool virtnet_send_command(struct virtnet_info *vi, u8 class, u8 cmd,
> > > > * into the hypervisor, so the request should be handled immediately.
> > > > */
> > > > while (!virtqueue_get_buf(vi->cvq, &tmp) &&
> > > > - !virtqueue_is_broken(vi->cvq))
> > > > + !virtqueue_is_broken(vi->cvq)) {
> > > > + cond_resched();
> > > > cpu_relax();
> > > > + }
> > > >
> > > > return vi->ctrl->status == VIRTIO_NET_OK;
> > > > }
> > > > --
> > > > 2.39.3
> > > >
> > > > _______________________________________________
> > > > Virtualization mailing list
> > > > [email protected]
> > > > https://lists.linuxfoundation.org/mailman/listinfo/virtualization
> >
On Fri, Jul 21, 2023 at 04:58:04PM +0200, Maxime Coquelin wrote:
>
>
> On 7/21/23 16:45, Michael S. Tsirkin wrote:
> > On Fri, Jul 21, 2023 at 04:37:00PM +0200, Maxime Coquelin wrote:
> > >
> > >
> > > On 7/20/23 23:02, Michael S. Tsirkin wrote:
> > > > On Thu, Jul 20, 2023 at 01:26:20PM -0700, Shannon Nelson wrote:
> > > > > On 7/20/23 1:38 AM, Jason Wang wrote:
> > > > > >
> > > > > > Adding cond_resched() to the command waiting loop for a better
> > > > > > co-operation with the scheduler. This allows to give CPU a breath to
> > > > > > run other task(workqueue) instead of busy looping when preemption is
> > > > > > not allowed on a device whose CVQ might be slow.
> > > > > >
> > > > > > Signed-off-by: Jason Wang <[email protected]>
> > > > >
> > > > > This still leaves hung processes, but at least it doesn't pin the CPU any
> > > > > more. Thanks.
> > > > > Reviewed-by: Shannon Nelson <[email protected]>
> > > > >
> > > >
> > > > I'd like to see a full solution
> > > > 1- block until interrupt
> > >
> > > Would it make sense to also have a timeout?
> > > And when timeout expires, set FAILED bit in device status?
> >
> > virtio spec does not set any limits on the timing of vq
> > processing.
>
> Indeed, but I thought the driver could decide it is too long for it.
>
> The issue is we keep waiting with rtnl locked, it can quickly make the
> system unusable.
if this is a problem we should find a way not to keep rtnl
locked indefinitely.
> > > > 2- still handle surprise removal correctly by waking in that case
> > > >
> > > >
> > > >
> > > > > > ---
> > > > > > drivers/net/virtio_net.c | 4 +++-
> > > > > > 1 file changed, 3 insertions(+), 1 deletion(-)
> > > > > >
> > > > > > diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
> > > > > > index 9f3b1d6ac33d..e7533f29b219 100644
> > > > > > --- a/drivers/net/virtio_net.c
> > > > > > +++ b/drivers/net/virtio_net.c
> > > > > > @@ -2314,8 +2314,10 @@ static bool virtnet_send_command(struct virtnet_info *vi, u8 class, u8 cmd,
> > > > > > * into the hypervisor, so the request should be handled immediately.
> > > > > > */
> > > > > > while (!virtqueue_get_buf(vi->cvq, &tmp) &&
> > > > > > - !virtqueue_is_broken(vi->cvq))
> > > > > > + !virtqueue_is_broken(vi->cvq)) {
> > > > > > + cond_resched();
> > > > > > cpu_relax();
> > > > > > + }
> > > > > >
> > > > > > return vi->ctrl->status == VIRTIO_NET_OK;
> > > > > > }
> > > > > > --
> > > > > > 2.39.3
> > > > > >
> > > > > > _______________________________________________
> > > > > > Virtualization mailing list
> > > > > > [email protected]
> > > > > > https://lists.linuxfoundation.org/mailman/listinfo/virtualization
> > > >
> >
On 7/21/23 16:45, Michael S. Tsirkin wrote:
> On Fri, Jul 21, 2023 at 04:37:00PM +0200, Maxime Coquelin wrote:
>>
>>
>> On 7/20/23 23:02, Michael S. Tsirkin wrote:
>>> On Thu, Jul 20, 2023 at 01:26:20PM -0700, Shannon Nelson wrote:
>>>> On 7/20/23 1:38 AM, Jason Wang wrote:
>>>>>
>>>>> Adding cond_resched() to the command waiting loop for a better
>>>>> co-operation with the scheduler. This allows to give CPU a breath to
>>>>> run other task(workqueue) instead of busy looping when preemption is
>>>>> not allowed on a device whose CVQ might be slow.
>>>>>
>>>>> Signed-off-by: Jason Wang <[email protected]>
>>>>
>>>> This still leaves hung processes, but at least it doesn't pin the CPU any
>>>> more. Thanks.
>>>> Reviewed-by: Shannon Nelson <[email protected]>
>>>>
>>>
>>> I'd like to see a full solution
>>> 1- block until interrupt
>>
>> Would it make sense to also have a timeout?
>> And when timeout expires, set FAILED bit in device status?
>
> virtio spec does not set any limits on the timing of vq
> processing.
Indeed, but I thought the driver could decide it is too long for it.
The issue is we keep waiting with rtnl locked, it can quickly make the
system unusable.
>>> 2- still handle surprise removal correctly by waking in that case
>>>
>>>
>>>
>>>>> ---
>>>>> drivers/net/virtio_net.c | 4 +++-
>>>>> 1 file changed, 3 insertions(+), 1 deletion(-)
>>>>>
>>>>> diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
>>>>> index 9f3b1d6ac33d..e7533f29b219 100644
>>>>> --- a/drivers/net/virtio_net.c
>>>>> +++ b/drivers/net/virtio_net.c
>>>>> @@ -2314,8 +2314,10 @@ static bool virtnet_send_command(struct virtnet_info *vi, u8 class, u8 cmd,
>>>>> * into the hypervisor, so the request should be handled immediately.
>>>>> */
>>>>> while (!virtqueue_get_buf(vi->cvq, &tmp) &&
>>>>> - !virtqueue_is_broken(vi->cvq))
>>>>> + !virtqueue_is_broken(vi->cvq)) {
>>>>> + cond_resched();
>>>>> cpu_relax();
>>>>> + }
>>>>>
>>>>> return vi->ctrl->status == VIRTIO_NET_OK;
>>>>> }
>>>>> --
>>>>> 2.39.3
>>>>>
>>>>> _______________________________________________
>>>>> Virtualization mailing list
>>>>> [email protected]
>>>>> https://lists.linuxfoundation.org/mailman/listinfo/virtualization
>>>
>
On 7/21/23 17:10, Michael S. Tsirkin wrote:
> On Fri, Jul 21, 2023 at 04:58:04PM +0200, Maxime Coquelin wrote:
>>
>>
>> On 7/21/23 16:45, Michael S. Tsirkin wrote:
>>> On Fri, Jul 21, 2023 at 04:37:00PM +0200, Maxime Coquelin wrote:
>>>>
>>>>
>>>> On 7/20/23 23:02, Michael S. Tsirkin wrote:
>>>>> On Thu, Jul 20, 2023 at 01:26:20PM -0700, Shannon Nelson wrote:
>>>>>> On 7/20/23 1:38 AM, Jason Wang wrote:
>>>>>>>
>>>>>>> Adding cond_resched() to the command waiting loop for a better
>>>>>>> co-operation with the scheduler. This allows to give CPU a breath to
>>>>>>> run other task(workqueue) instead of busy looping when preemption is
>>>>>>> not allowed on a device whose CVQ might be slow.
>>>>>>>
>>>>>>> Signed-off-by: Jason Wang <[email protected]>
>>>>>>
>>>>>> This still leaves hung processes, but at least it doesn't pin the CPU any
>>>>>> more. Thanks.
>>>>>> Reviewed-by: Shannon Nelson <[email protected]>
>>>>>>
>>>>>
>>>>> I'd like to see a full solution
>>>>> 1- block until interrupt
>>>>
>>>> Would it make sense to also have a timeout?
>>>> And when timeout expires, set FAILED bit in device status?
>>>
>>> virtio spec does not set any limits on the timing of vq
>>> processing.
>>
>> Indeed, but I thought the driver could decide it is too long for it.
>>
>> The issue is we keep waiting with rtnl locked, it can quickly make the
>> system unusable.
>
> if this is a problem we should find a way not to keep rtnl
> locked indefinitely.
From the tests I have done, I think it is. With OVS, a reconfiguration
is performed when the VDUSE device is added, and when a MLX5 device is
in the same bridge, it ends up doing an ioctl() that tries to take the
rtnl lock. In this configuration, it is not possible to kill OVS because
it is stuck trying to acquire rtnl lock for mlx5 that is held by virtio-
net.
>
>>>>> 2- still handle surprise removal correctly by waking in that case
>>>>>
>>>>>
>>>>>
>>>>>>> ---
>>>>>>> drivers/net/virtio_net.c | 4 +++-
>>>>>>> 1 file changed, 3 insertions(+), 1 deletion(-)
>>>>>>>
>>>>>>> diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
>>>>>>> index 9f3b1d6ac33d..e7533f29b219 100644
>>>>>>> --- a/drivers/net/virtio_net.c
>>>>>>> +++ b/drivers/net/virtio_net.c
>>>>>>> @@ -2314,8 +2314,10 @@ static bool virtnet_send_command(struct virtnet_info *vi, u8 class, u8 cmd,
>>>>>>> * into the hypervisor, so the request should be handled immediately.
>>>>>>> */
>>>>>>> while (!virtqueue_get_buf(vi->cvq, &tmp) &&
>>>>>>> - !virtqueue_is_broken(vi->cvq))
>>>>>>> + !virtqueue_is_broken(vi->cvq)) {
>>>>>>> + cond_resched();
>>>>>>> cpu_relax();
>>>>>>> + }
>>>>>>>
>>>>>>> return vi->ctrl->status == VIRTIO_NET_OK;
>>>>>>> }
>>>>>>> --
>>>>>>> 2.39.3
>>>>>>>
>>>>>>> _______________________________________________
>>>>>>> Virtualization mailing list
>>>>>>> [email protected]
>>>>>>> https://lists.linuxfoundation.org/mailman/listinfo/virtualization
>>>>>
>>>
>
On Sat, Jul 22, 2023 at 4:18 AM Maxime Coquelin
<[email protected]> wrote:
>
>
>
> On 7/21/23 17:10, Michael S. Tsirkin wrote:
> > On Fri, Jul 21, 2023 at 04:58:04PM +0200, Maxime Coquelin wrote:
> >>
> >>
> >> On 7/21/23 16:45, Michael S. Tsirkin wrote:
> >>> On Fri, Jul 21, 2023 at 04:37:00PM +0200, Maxime Coquelin wrote:
> >>>>
> >>>>
> >>>> On 7/20/23 23:02, Michael S. Tsirkin wrote:
> >>>>> On Thu, Jul 20, 2023 at 01:26:20PM -0700, Shannon Nelson wrote:
> >>>>>> On 7/20/23 1:38 AM, Jason Wang wrote:
> >>>>>>>
> >>>>>>> Adding cond_resched() to the command waiting loop for a better
> >>>>>>> co-operation with the scheduler. This allows to give CPU a breath to
> >>>>>>> run other task(workqueue) instead of busy looping when preemption is
> >>>>>>> not allowed on a device whose CVQ might be slow.
> >>>>>>>
> >>>>>>> Signed-off-by: Jason Wang <[email protected]>
> >>>>>>
> >>>>>> This still leaves hung processes, but at least it doesn't pin the CPU any
> >>>>>> more. Thanks.
> >>>>>> Reviewed-by: Shannon Nelson <[email protected]>
> >>>>>>
> >>>>>
> >>>>> I'd like to see a full solution
> >>>>> 1- block until interrupt
I remember in previous versions, you worried about the extra MSI
vector. (Maybe I was wrong).
> >>>>
> >>>> Would it make sense to also have a timeout?
> >>>> And when timeout expires, set FAILED bit in device status?
> >>>
> >>> virtio spec does not set any limits on the timing of vq
> >>> processing.
> >>
> >> Indeed, but I thought the driver could decide it is too long for it.
> >>
> >> The issue is we keep waiting with rtnl locked, it can quickly make the
> >> system unusable.
> >
> > if this is a problem we should find a way not to keep rtnl
> > locked indefinitely.
Any ideas on this direction? Simply dropping rtnl during the busy loop
will result in a lot of races. This seems to require non-trivial
changes in the networking core.
>
> From the tests I have done, I think it is. With OVS, a reconfiguration
> is performed when the VDUSE device is added, and when a MLX5 device is
> in the same bridge, it ends up doing an ioctl() that tries to take the
> rtnl lock. In this configuration, it is not possible to kill OVS because
> it is stuck trying to acquire rtnl lock for mlx5 that is held by virtio-
> net.
Yeah, basically, any RTNL users would be blocked forever.
And the infinite loop has other side effects like it blocks the freezer to work.
To summarize, there are three issues
1) busy polling
2) breaks freezer
3) hold RTNL during the loop
Solving 3 may help somehow for 2 e.g some pm routine e.g wireguard or
even virtnet_restore() itself may try to hold the lock.
>
> >
> >>>>> 2- still handle surprise removal correctly by waking in that case
This is basically what version 1 did?
https://lore.kernel.org/lkml/[email protected]/t/
Thanks
> >>>>>
> >>>>>
> >>>>>
> >>>>>>> ---
> >>>>>>> drivers/net/virtio_net.c | 4 +++-
> >>>>>>> 1 file changed, 3 insertions(+), 1 deletion(-)
> >>>>>>>
> >>>>>>> diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
> >>>>>>> index 9f3b1d6ac33d..e7533f29b219 100644
> >>>>>>> --- a/drivers/net/virtio_net.c
> >>>>>>> +++ b/drivers/net/virtio_net.c
> >>>>>>> @@ -2314,8 +2314,10 @@ static bool virtnet_send_command(struct virtnet_info *vi, u8 class, u8 cmd,
> >>>>>>> * into the hypervisor, so the request should be handled immediately.
> >>>>>>> */
> >>>>>>> while (!virtqueue_get_buf(vi->cvq, &tmp) &&
> >>>>>>> - !virtqueue_is_broken(vi->cvq))
> >>>>>>> + !virtqueue_is_broken(vi->cvq)) {
> >>>>>>> + cond_resched();
> >>>>>>> cpu_relax();
> >>>>>>> + }
> >>>>>>>
> >>>>>>> return vi->ctrl->status == VIRTIO_NET_OK;
> >>>>>>> }
> >>>>>>> --
> >>>>>>> 2.39.3
> >>>>>>>
> >>>>>>> _______________________________________________
> >>>>>>> Virtualization mailing list
> >>>>>>> [email protected]
> >>>>>>> https://lists.linuxfoundation.org/mailman/listinfo/virtualization
> >>>>>
> >>>
> >
>
On Fri, Jul 21, 2023 at 10:18:03PM +0200, Maxime Coquelin wrote:
>
>
> On 7/21/23 17:10, Michael S. Tsirkin wrote:
> > On Fri, Jul 21, 2023 at 04:58:04PM +0200, Maxime Coquelin wrote:
> > >
> > >
> > > On 7/21/23 16:45, Michael S. Tsirkin wrote:
> > > > On Fri, Jul 21, 2023 at 04:37:00PM +0200, Maxime Coquelin wrote:
> > > > >
> > > > >
> > > > > On 7/20/23 23:02, Michael S. Tsirkin wrote:
> > > > > > On Thu, Jul 20, 2023 at 01:26:20PM -0700, Shannon Nelson wrote:
> > > > > > > On 7/20/23 1:38 AM, Jason Wang wrote:
> > > > > > > >
> > > > > > > > Adding cond_resched() to the command waiting loop for a better
> > > > > > > > co-operation with the scheduler. This allows to give CPU a breath to
> > > > > > > > run other task(workqueue) instead of busy looping when preemption is
> > > > > > > > not allowed on a device whose CVQ might be slow.
> > > > > > > >
> > > > > > > > Signed-off-by: Jason Wang <[email protected]>
> > > > > > >
> > > > > > > This still leaves hung processes, but at least it doesn't pin the CPU any
> > > > > > > more. Thanks.
> > > > > > > Reviewed-by: Shannon Nelson <[email protected]>
> > > > > > >
> > > > > >
> > > > > > I'd like to see a full solution
> > > > > > 1- block until interrupt
> > > > >
> > > > > Would it make sense to also have a timeout?
> > > > > And when timeout expires, set FAILED bit in device status?
> > > >
> > > > virtio spec does not set any limits on the timing of vq
> > > > processing.
> > >
> > > Indeed, but I thought the driver could decide it is too long for it.
> > >
> > > The issue is we keep waiting with rtnl locked, it can quickly make the
> > > system unusable.
> >
> > if this is a problem we should find a way not to keep rtnl
> > locked indefinitely.
>
> From the tests I have done, I think it is. With OVS, a reconfiguration is
> performed when the VDUSE device is added, and when a MLX5 device is
> in the same bridge, it ends up doing an ioctl() that tries to take the
> rtnl lock. In this configuration, it is not possible to kill OVS because
> it is stuck trying to acquire rtnl lock for mlx5 that is held by virtio-
> net.
So for sure, we can queue up the work and process it later.
The somewhat tricky part is limiting the memory consumption.
> >
> > > > > > 2- still handle surprise removal correctly by waking in that case
> > > > > >
> > > > > >
> > > > > >
> > > > > > > > ---
> > > > > > > > drivers/net/virtio_net.c | 4 +++-
> > > > > > > > 1 file changed, 3 insertions(+), 1 deletion(-)
> > > > > > > >
> > > > > > > > diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
> > > > > > > > index 9f3b1d6ac33d..e7533f29b219 100644
> > > > > > > > --- a/drivers/net/virtio_net.c
> > > > > > > > +++ b/drivers/net/virtio_net.c
> > > > > > > > @@ -2314,8 +2314,10 @@ static bool virtnet_send_command(struct virtnet_info *vi, u8 class, u8 cmd,
> > > > > > > > * into the hypervisor, so the request should be handled immediately.
> > > > > > > > */
> > > > > > > > while (!virtqueue_get_buf(vi->cvq, &tmp) &&
> > > > > > > > - !virtqueue_is_broken(vi->cvq))
> > > > > > > > + !virtqueue_is_broken(vi->cvq)) {
> > > > > > > > + cond_resched();
> > > > > > > > cpu_relax();
> > > > > > > > + }
> > > > > > > >
> > > > > > > > return vi->ctrl->status == VIRTIO_NET_OK;
> > > > > > > > }
> > > > > > > > --
> > > > > > > > 2.39.3
> > > > > > > >
> > > > > > > > _______________________________________________
> > > > > > > > Virtualization mailing list
> > > > > > > > [email protected]
> > > > > > > > https://lists.linuxfoundation.org/mailman/listinfo/virtualization
> > > > > >
> > > >
> >
On Mon, Jul 24, 2023 at 2:46 PM Michael S. Tsirkin <[email protected]> wrote:
>
> On Fri, Jul 21, 2023 at 10:18:03PM +0200, Maxime Coquelin wrote:
> >
> >
> > On 7/21/23 17:10, Michael S. Tsirkin wrote:
> > > On Fri, Jul 21, 2023 at 04:58:04PM +0200, Maxime Coquelin wrote:
> > > >
> > > >
> > > > On 7/21/23 16:45, Michael S. Tsirkin wrote:
> > > > > On Fri, Jul 21, 2023 at 04:37:00PM +0200, Maxime Coquelin wrote:
> > > > > >
> > > > > >
> > > > > > On 7/20/23 23:02, Michael S. Tsirkin wrote:
> > > > > > > On Thu, Jul 20, 2023 at 01:26:20PM -0700, Shannon Nelson wrote:
> > > > > > > > On 7/20/23 1:38 AM, Jason Wang wrote:
> > > > > > > > >
> > > > > > > > > Adding cond_resched() to the command waiting loop for a better
> > > > > > > > > co-operation with the scheduler. This allows to give CPU a breath to
> > > > > > > > > run other task(workqueue) instead of busy looping when preemption is
> > > > > > > > > not allowed on a device whose CVQ might be slow.
> > > > > > > > >
> > > > > > > > > Signed-off-by: Jason Wang <[email protected]>
> > > > > > > >
> > > > > > > > This still leaves hung processes, but at least it doesn't pin the CPU any
> > > > > > > > more. Thanks.
> > > > > > > > Reviewed-by: Shannon Nelson <[email protected]>
> > > > > > > >
> > > > > > >
> > > > > > > I'd like to see a full solution
> > > > > > > 1- block until interrupt
> > > > > >
> > > > > > Would it make sense to also have a timeout?
> > > > > > And when timeout expires, set FAILED bit in device status?
> > > > >
> > > > > virtio spec does not set any limits on the timing of vq
> > > > > processing.
> > > >
> > > > Indeed, but I thought the driver could decide it is too long for it.
> > > >
> > > > The issue is we keep waiting with rtnl locked, it can quickly make the
> > > > system unusable.
> > >
> > > if this is a problem we should find a way not to keep rtnl
> > > locked indefinitely.
> >
> > From the tests I have done, I think it is. With OVS, a reconfiguration is
> > performed when the VDUSE device is added, and when a MLX5 device is
> > in the same bridge, it ends up doing an ioctl() that tries to take the
> > rtnl lock. In this configuration, it is not possible to kill OVS because
> > it is stuck trying to acquire rtnl lock for mlx5 that is held by virtio-
> > net.
>
> So for sure, we can queue up the work and process it later.
> The somewhat tricky part is limiting the memory consumption.
And it needs to sync with rtnl somehow, e.g device unregistering which
seems not easy.
Thanks
>
>
> > >
> > > > > > > 2- still handle surprise removal correctly by waking in that case
> > > > > > >
> > > > > > >
> > > > > > >
> > > > > > > > > ---
> > > > > > > > > drivers/net/virtio_net.c | 4 +++-
> > > > > > > > > 1 file changed, 3 insertions(+), 1 deletion(-)
> > > > > > > > >
> > > > > > > > > diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
> > > > > > > > > index 9f3b1d6ac33d..e7533f29b219 100644
> > > > > > > > > --- a/drivers/net/virtio_net.c
> > > > > > > > > +++ b/drivers/net/virtio_net.c
> > > > > > > > > @@ -2314,8 +2314,10 @@ static bool virtnet_send_command(struct virtnet_info *vi, u8 class, u8 cmd,
> > > > > > > > > * into the hypervisor, so the request should be handled immediately.
> > > > > > > > > */
> > > > > > > > > while (!virtqueue_get_buf(vi->cvq, &tmp) &&
> > > > > > > > > - !virtqueue_is_broken(vi->cvq))
> > > > > > > > > + !virtqueue_is_broken(vi->cvq)) {
> > > > > > > > > + cond_resched();
> > > > > > > > > cpu_relax();
> > > > > > > > > + }
> > > > > > > > >
> > > > > > > > > return vi->ctrl->status == VIRTIO_NET_OK;
> > > > > > > > > }
> > > > > > > > > --
> > > > > > > > > 2.39.3
> > > > > > > > >
> > > > > > > > > _______________________________________________
> > > > > > > > > Virtualization mailing list
> > > > > > > > > [email protected]
> > > > > > > > > https://lists.linuxfoundation.org/mailman/listinfo/virtualization
> > > > > > >
> > > > >
> > >
>
On Mon, Jul 24, 2023 at 02:52:49PM +0800, Jason Wang wrote:
> On Mon, Jul 24, 2023 at 2:46 PM Michael S. Tsirkin <[email protected]> wrote:
> >
> > On Fri, Jul 21, 2023 at 10:18:03PM +0200, Maxime Coquelin wrote:
> > >
> > >
> > > On 7/21/23 17:10, Michael S. Tsirkin wrote:
> > > > On Fri, Jul 21, 2023 at 04:58:04PM +0200, Maxime Coquelin wrote:
> > > > >
> > > > >
> > > > > On 7/21/23 16:45, Michael S. Tsirkin wrote:
> > > > > > On Fri, Jul 21, 2023 at 04:37:00PM +0200, Maxime Coquelin wrote:
> > > > > > >
> > > > > > >
> > > > > > > On 7/20/23 23:02, Michael S. Tsirkin wrote:
> > > > > > > > On Thu, Jul 20, 2023 at 01:26:20PM -0700, Shannon Nelson wrote:
> > > > > > > > > On 7/20/23 1:38 AM, Jason Wang wrote:
> > > > > > > > > >
> > > > > > > > > > Adding cond_resched() to the command waiting loop for a better
> > > > > > > > > > co-operation with the scheduler. This allows to give CPU a breath to
> > > > > > > > > > run other task(workqueue) instead of busy looping when preemption is
> > > > > > > > > > not allowed on a device whose CVQ might be slow.
> > > > > > > > > >
> > > > > > > > > > Signed-off-by: Jason Wang <[email protected]>
> > > > > > > > >
> > > > > > > > > This still leaves hung processes, but at least it doesn't pin the CPU any
> > > > > > > > > more. Thanks.
> > > > > > > > > Reviewed-by: Shannon Nelson <[email protected]>
> > > > > > > > >
> > > > > > > >
> > > > > > > > I'd like to see a full solution
> > > > > > > > 1- block until interrupt
> > > > > > >
> > > > > > > Would it make sense to also have a timeout?
> > > > > > > And when timeout expires, set FAILED bit in device status?
> > > > > >
> > > > > > virtio spec does not set any limits on the timing of vq
> > > > > > processing.
> > > > >
> > > > > Indeed, but I thought the driver could decide it is too long for it.
> > > > >
> > > > > The issue is we keep waiting with rtnl locked, it can quickly make the
> > > > > system unusable.
> > > >
> > > > if this is a problem we should find a way not to keep rtnl
> > > > locked indefinitely.
> > >
> > > From the tests I have done, I think it is. With OVS, a reconfiguration is
> > > performed when the VDUSE device is added, and when a MLX5 device is
> > > in the same bridge, it ends up doing an ioctl() that tries to take the
> > > rtnl lock. In this configuration, it is not possible to kill OVS because
> > > it is stuck trying to acquire rtnl lock for mlx5 that is held by virtio-
> > > net.
> >
> > So for sure, we can queue up the work and process it later.
> > The somewhat tricky part is limiting the memory consumption.
>
> And it needs to sync with rtnl somehow, e.g device unregistering which
> seems not easy.
>
> Thanks
since when does device unregister need to send cvq commands?
> >
> >
> > > >
> > > > > > > > 2- still handle surprise removal correctly by waking in that case
> > > > > > > >
> > > > > > > >
> > > > > > > >
> > > > > > > > > > ---
> > > > > > > > > > drivers/net/virtio_net.c | 4 +++-
> > > > > > > > > > 1 file changed, 3 insertions(+), 1 deletion(-)
> > > > > > > > > >
> > > > > > > > > > diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
> > > > > > > > > > index 9f3b1d6ac33d..e7533f29b219 100644
> > > > > > > > > > --- a/drivers/net/virtio_net.c
> > > > > > > > > > +++ b/drivers/net/virtio_net.c
> > > > > > > > > > @@ -2314,8 +2314,10 @@ static bool virtnet_send_command(struct virtnet_info *vi, u8 class, u8 cmd,
> > > > > > > > > > * into the hypervisor, so the request should be handled immediately.
> > > > > > > > > > */
> > > > > > > > > > while (!virtqueue_get_buf(vi->cvq, &tmp) &&
> > > > > > > > > > - !virtqueue_is_broken(vi->cvq))
> > > > > > > > > > + !virtqueue_is_broken(vi->cvq)) {
> > > > > > > > > > + cond_resched();
> > > > > > > > > > cpu_relax();
> > > > > > > > > > + }
> > > > > > > > > >
> > > > > > > > > > return vi->ctrl->status == VIRTIO_NET_OK;
> > > > > > > > > > }
> > > > > > > > > > --
> > > > > > > > > > 2.39.3
> > > > > > > > > >
> > > > > > > > > > _______________________________________________
> > > > > > > > > > Virtualization mailing list
> > > > > > > > > > [email protected]
> > > > > > > > > > https://lists.linuxfoundation.org/mailman/listinfo/virtualization
> > > > > > > >
> > > > > >
> > > >
> >
On Mon, Jul 24, 2023 at 02:52:05PM +0800, Jason Wang wrote:
> On Sat, Jul 22, 2023 at 4:18 AM Maxime Coquelin
> <[email protected]> wrote:
> >
> >
> >
> > On 7/21/23 17:10, Michael S. Tsirkin wrote:
> > > On Fri, Jul 21, 2023 at 04:58:04PM +0200, Maxime Coquelin wrote:
> > >>
> > >>
> > >> On 7/21/23 16:45, Michael S. Tsirkin wrote:
> > >>> On Fri, Jul 21, 2023 at 04:37:00PM +0200, Maxime Coquelin wrote:
> > >>>>
> > >>>>
> > >>>> On 7/20/23 23:02, Michael S. Tsirkin wrote:
> > >>>>> On Thu, Jul 20, 2023 at 01:26:20PM -0700, Shannon Nelson wrote:
> > >>>>>> On 7/20/23 1:38 AM, Jason Wang wrote:
> > >>>>>>>
> > >>>>>>> Adding cond_resched() to the command waiting loop for a better
> > >>>>>>> co-operation with the scheduler. This allows to give CPU a breath to
> > >>>>>>> run other task(workqueue) instead of busy looping when preemption is
> > >>>>>>> not allowed on a device whose CVQ might be slow.
> > >>>>>>>
> > >>>>>>> Signed-off-by: Jason Wang <[email protected]>
> > >>>>>>
> > >>>>>> This still leaves hung processes, but at least it doesn't pin the CPU any
> > >>>>>> more. Thanks.
> > >>>>>> Reviewed-by: Shannon Nelson <[email protected]>
> > >>>>>>
> > >>>>>
> > >>>>> I'd like to see a full solution
> > >>>>> 1- block until interrupt
>
> I remember in previous versions, you worried about the extra MSI
> vector. (Maybe I was wrong).
>
> > >>>>
> > >>>> Would it make sense to also have a timeout?
> > >>>> And when timeout expires, set FAILED bit in device status?
> > >>>
> > >>> virtio spec does not set any limits on the timing of vq
> > >>> processing.
> > >>
> > >> Indeed, but I thought the driver could decide it is too long for it.
> > >>
> > >> The issue is we keep waiting with rtnl locked, it can quickly make the
> > >> system unusable.
> > >
> > > if this is a problem we should find a way not to keep rtnl
> > > locked indefinitely.
>
> Any ideas on this direction? Simply dropping rtnl during the busy loop
> will result in a lot of races. This seems to require non-trivial
> changes in the networking core.
>
> >
> > From the tests I have done, I think it is. With OVS, a reconfiguration
> > is performed when the VDUSE device is added, and when a MLX5 device is
> > in the same bridge, it ends up doing an ioctl() that tries to take the
> > rtnl lock. In this configuration, it is not possible to kill OVS because
> > it is stuck trying to acquire rtnl lock for mlx5 that is held by virtio-
> > net.
>
> Yeah, basically, any RTNL users would be blocked forever.
>
> And the infinite loop has other side effects like it blocks the freezer to work.
>
> To summarize, there are three issues
>
> 1) busy polling
> 2) breaks freezer
> 3) hold RTNL during the loop
>
> Solving 3 may help somehow for 2 e.g some pm routine e.g wireguard or
> even virtnet_restore() itself may try to hold the lock.
Yep. So my feeling currently is, the only real fix is to actually
queue up the work in software. It's mostly trivial to limit
memory consumption, vid's is the
only one where it would make sense to have more than
1 command of a given type outstanding. have a tree
or a bitmap with vids to add/remove?
> >
> > >
> > >>>>> 2- still handle surprise removal correctly by waking in that case
>
> This is basically what version 1 did?
>
> https://lore.kernel.org/lkml/[email protected]/t/
>
> Thanks
Yes - except the timeout part.
> > >>>>>
> > >>>>>
> > >>>>>
> > >>>>>>> ---
> > >>>>>>> drivers/net/virtio_net.c | 4 +++-
> > >>>>>>> 1 file changed, 3 insertions(+), 1 deletion(-)
> > >>>>>>>
> > >>>>>>> diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
> > >>>>>>> index 9f3b1d6ac33d..e7533f29b219 100644
> > >>>>>>> --- a/drivers/net/virtio_net.c
> > >>>>>>> +++ b/drivers/net/virtio_net.c
> > >>>>>>> @@ -2314,8 +2314,10 @@ static bool virtnet_send_command(struct virtnet_info *vi, u8 class, u8 cmd,
> > >>>>>>> * into the hypervisor, so the request should be handled immediately.
> > >>>>>>> */
> > >>>>>>> while (!virtqueue_get_buf(vi->cvq, &tmp) &&
> > >>>>>>> - !virtqueue_is_broken(vi->cvq))
> > >>>>>>> + !virtqueue_is_broken(vi->cvq)) {
> > >>>>>>> + cond_resched();
> > >>>>>>> cpu_relax();
> > >>>>>>> + }
> > >>>>>>>
> > >>>>>>> return vi->ctrl->status == VIRTIO_NET_OK;
> > >>>>>>> }
> > >>>>>>> --
> > >>>>>>> 2.39.3
> > >>>>>>>
> > >>>>>>> _______________________________________________
> > >>>>>>> Virtualization mailing list
> > >>>>>>> [email protected]
> > >>>>>>> https://lists.linuxfoundation.org/mailman/listinfo/virtualization
> > >>>>>
> > >>>
> > >
> >
On Mon, Jul 24, 2023 at 3:18 PM Michael S. Tsirkin <[email protected]> wrote:
>
> On Mon, Jul 24, 2023 at 02:52:49PM +0800, Jason Wang wrote:
> > On Mon, Jul 24, 2023 at 2:46 PM Michael S. Tsirkin <[email protected]> wrote:
> > >
> > > On Fri, Jul 21, 2023 at 10:18:03PM +0200, Maxime Coquelin wrote:
> > > >
> > > >
> > > > On 7/21/23 17:10, Michael S. Tsirkin wrote:
> > > > > On Fri, Jul 21, 2023 at 04:58:04PM +0200, Maxime Coquelin wrote:
> > > > > >
> > > > > >
> > > > > > On 7/21/23 16:45, Michael S. Tsirkin wrote:
> > > > > > > On Fri, Jul 21, 2023 at 04:37:00PM +0200, Maxime Coquelin wrote:
> > > > > > > >
> > > > > > > >
> > > > > > > > On 7/20/23 23:02, Michael S. Tsirkin wrote:
> > > > > > > > > On Thu, Jul 20, 2023 at 01:26:20PM -0700, Shannon Nelson wrote:
> > > > > > > > > > On 7/20/23 1:38 AM, Jason Wang wrote:
> > > > > > > > > > >
> > > > > > > > > > > Adding cond_resched() to the command waiting loop for a better
> > > > > > > > > > > co-operation with the scheduler. This allows to give CPU a breath to
> > > > > > > > > > > run other task(workqueue) instead of busy looping when preemption is
> > > > > > > > > > > not allowed on a device whose CVQ might be slow.
> > > > > > > > > > >
> > > > > > > > > > > Signed-off-by: Jason Wang <[email protected]>
> > > > > > > > > >
> > > > > > > > > > This still leaves hung processes, but at least it doesn't pin the CPU any
> > > > > > > > > > more. Thanks.
> > > > > > > > > > Reviewed-by: Shannon Nelson <[email protected]>
> > > > > > > > > >
> > > > > > > > >
> > > > > > > > > I'd like to see a full solution
> > > > > > > > > 1- block until interrupt
> > > > > > > >
> > > > > > > > Would it make sense to also have a timeout?
> > > > > > > > And when timeout expires, set FAILED bit in device status?
> > > > > > >
> > > > > > > virtio spec does not set any limits on the timing of vq
> > > > > > > processing.
> > > > > >
> > > > > > Indeed, but I thought the driver could decide it is too long for it.
> > > > > >
> > > > > > The issue is we keep waiting with rtnl locked, it can quickly make the
> > > > > > system unusable.
> > > > >
> > > > > if this is a problem we should find a way not to keep rtnl
> > > > > locked indefinitely.
> > > >
> > > > From the tests I have done, I think it is. With OVS, a reconfiguration is
> > > > performed when the VDUSE device is added, and when a MLX5 device is
> > > > in the same bridge, it ends up doing an ioctl() that tries to take the
> > > > rtnl lock. In this configuration, it is not possible to kill OVS because
> > > > it is stuck trying to acquire rtnl lock for mlx5 that is held by virtio-
> > > > net.
> > >
> > > So for sure, we can queue up the work and process it later.
> > > The somewhat tricky part is limiting the memory consumption.
> >
> > And it needs to sync with rtnl somehow, e.g device unregistering which
> > seems not easy.
> >
> > Thanks
>
> since when does device unregister need to send cvq commands?
It doesn't do this now. But if we don't process the work under rtnl,
we need to synchronize with device unregistering.
Thanks
>
> > >
> > >
> > > > >
> > > > > > > > > 2- still handle surprise removal correctly by waking in that case
> > > > > > > > >
> > > > > > > > >
> > > > > > > > >
> > > > > > > > > > > ---
> > > > > > > > > > > drivers/net/virtio_net.c | 4 +++-
> > > > > > > > > > > 1 file changed, 3 insertions(+), 1 deletion(-)
> > > > > > > > > > >
> > > > > > > > > > > diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
> > > > > > > > > > > index 9f3b1d6ac33d..e7533f29b219 100644
> > > > > > > > > > > --- a/drivers/net/virtio_net.c
> > > > > > > > > > > +++ b/drivers/net/virtio_net.c
> > > > > > > > > > > @@ -2314,8 +2314,10 @@ static bool virtnet_send_command(struct virtnet_info *vi, u8 class, u8 cmd,
> > > > > > > > > > > * into the hypervisor, so the request should be handled immediately.
> > > > > > > > > > > */
> > > > > > > > > > > while (!virtqueue_get_buf(vi->cvq, &tmp) &&
> > > > > > > > > > > - !virtqueue_is_broken(vi->cvq))
> > > > > > > > > > > + !virtqueue_is_broken(vi->cvq)) {
> > > > > > > > > > > + cond_resched();
> > > > > > > > > > > cpu_relax();
> > > > > > > > > > > + }
> > > > > > > > > > >
> > > > > > > > > > > return vi->ctrl->status == VIRTIO_NET_OK;
> > > > > > > > > > > }
> > > > > > > > > > > --
> > > > > > > > > > > 2.39.3
> > > > > > > > > > >
> > > > > > > > > > > _______________________________________________
> > > > > > > > > > > Virtualization mailing list
> > > > > > > > > > > [email protected]
> > > > > > > > > > > https://lists.linuxfoundation.org/mailman/listinfo/virtualization
> > > > > > > > >
> > > > > > >
> > > > >
> > >
>
On Mon, Jul 24, 2023 at 3:17 PM Michael S. Tsirkin <[email protected]> wrote:
>
> On Mon, Jul 24, 2023 at 02:52:05PM +0800, Jason Wang wrote:
> > On Sat, Jul 22, 2023 at 4:18 AM Maxime Coquelin
> > <[email protected]> wrote:
> > >
> > >
> > >
> > > On 7/21/23 17:10, Michael S. Tsirkin wrote:
> > > > On Fri, Jul 21, 2023 at 04:58:04PM +0200, Maxime Coquelin wrote:
> > > >>
> > > >>
> > > >> On 7/21/23 16:45, Michael S. Tsirkin wrote:
> > > >>> On Fri, Jul 21, 2023 at 04:37:00PM +0200, Maxime Coquelin wrote:
> > > >>>>
> > > >>>>
> > > >>>> On 7/20/23 23:02, Michael S. Tsirkin wrote:
> > > >>>>> On Thu, Jul 20, 2023 at 01:26:20PM -0700, Shannon Nelson wrote:
> > > >>>>>> On 7/20/23 1:38 AM, Jason Wang wrote:
> > > >>>>>>>
> > > >>>>>>> Adding cond_resched() to the command waiting loop for a better
> > > >>>>>>> co-operation with the scheduler. This allows to give CPU a breath to
> > > >>>>>>> run other task(workqueue) instead of busy looping when preemption is
> > > >>>>>>> not allowed on a device whose CVQ might be slow.
> > > >>>>>>>
> > > >>>>>>> Signed-off-by: Jason Wang <[email protected]>
> > > >>>>>>
> > > >>>>>> This still leaves hung processes, but at least it doesn't pin the CPU any
> > > >>>>>> more. Thanks.
> > > >>>>>> Reviewed-by: Shannon Nelson <[email protected]>
> > > >>>>>>
> > > >>>>>
> > > >>>>> I'd like to see a full solution
> > > >>>>> 1- block until interrupt
> >
> > I remember in previous versions, you worried about the extra MSI
> > vector. (Maybe I was wrong).
> >
> > > >>>>
> > > >>>> Would it make sense to also have a timeout?
> > > >>>> And when timeout expires, set FAILED bit in device status?
> > > >>>
> > > >>> virtio spec does not set any limits on the timing of vq
> > > >>> processing.
> > > >>
> > > >> Indeed, but I thought the driver could decide it is too long for it.
> > > >>
> > > >> The issue is we keep waiting with rtnl locked, it can quickly make the
> > > >> system unusable.
> > > >
> > > > if this is a problem we should find a way not to keep rtnl
> > > > locked indefinitely.
> >
> > Any ideas on this direction? Simply dropping rtnl during the busy loop
> > will result in a lot of races. This seems to require non-trivial
> > changes in the networking core.
> >
> > >
> > > From the tests I have done, I think it is. With OVS, a reconfiguration
> > > is performed when the VDUSE device is added, and when a MLX5 device is
> > > in the same bridge, it ends up doing an ioctl() that tries to take the
> > > rtnl lock. In this configuration, it is not possible to kill OVS because
> > > it is stuck trying to acquire rtnl lock for mlx5 that is held by virtio-
> > > net.
> >
> > Yeah, basically, any RTNL users would be blocked forever.
> >
> > And the infinite loop has other side effects like it blocks the freezer to work.
> >
> > To summarize, there are three issues
> >
> > 1) busy polling
> > 2) breaks freezer
> > 3) hold RTNL during the loop
> >
> > Solving 3 may help somehow for 2 e.g some pm routine e.g wireguard or
> > even virtnet_restore() itself may try to hold the lock.
>
> Yep. So my feeling currently is, the only real fix is to actually
> queue up the work in software.
Do you mean something like:
rtnl_lock();
queue up the work
rtnl_unlock();
return success;
?
> It's mostly trivial to limit
> memory consumption, vid's is the
> only one where it would make sense to have more than
> 1 command of a given type outstanding.
And rx mode so this implies we will fail any command if the previous
work is not finished.
> have a tree
> or a bitmap with vids to add/remove?
Probably.
Thanks
>
>
>
> > >
> > > >
> > > >>>>> 2- still handle surprise removal correctly by waking in that case
> >
> > This is basically what version 1 did?
> >
> > https://lore.kernel.org/lkml/[email protected]/t/
> >
> > Thanks
>
> Yes - except the timeout part.
>
>
> > > >>>>>
> > > >>>>>
> > > >>>>>
> > > >>>>>>> ---
> > > >>>>>>> drivers/net/virtio_net.c | 4 +++-
> > > >>>>>>> 1 file changed, 3 insertions(+), 1 deletion(-)
> > > >>>>>>>
> > > >>>>>>> diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
> > > >>>>>>> index 9f3b1d6ac33d..e7533f29b219 100644
> > > >>>>>>> --- a/drivers/net/virtio_net.c
> > > >>>>>>> +++ b/drivers/net/virtio_net.c
> > > >>>>>>> @@ -2314,8 +2314,10 @@ static bool virtnet_send_command(struct virtnet_info *vi, u8 class, u8 cmd,
> > > >>>>>>> * into the hypervisor, so the request should be handled immediately.
> > > >>>>>>> */
> > > >>>>>>> while (!virtqueue_get_buf(vi->cvq, &tmp) &&
> > > >>>>>>> - !virtqueue_is_broken(vi->cvq))
> > > >>>>>>> + !virtqueue_is_broken(vi->cvq)) {
> > > >>>>>>> + cond_resched();
> > > >>>>>>> cpu_relax();
> > > >>>>>>> + }
> > > >>>>>>>
> > > >>>>>>> return vi->ctrl->status == VIRTIO_NET_OK;
> > > >>>>>>> }
> > > >>>>>>> --
> > > >>>>>>> 2.39.3
> > > >>>>>>>
> > > >>>>>>> _______________________________________________
> > > >>>>>>> Virtualization mailing list
> > > >>>>>>> [email protected]
> > > >>>>>>> https://lists.linuxfoundation.org/mailman/listinfo/virtualization
> > > >>>>>
> > > >>>
> > > >
> > >
>
On Tue, Jul 25, 2023 at 11:07:40AM +0800, Jason Wang wrote:
> On Mon, Jul 24, 2023 at 3:17 PM Michael S. Tsirkin <[email protected]> wrote:
> >
> > On Mon, Jul 24, 2023 at 02:52:05PM +0800, Jason Wang wrote:
> > > On Sat, Jul 22, 2023 at 4:18 AM Maxime Coquelin
> > > <[email protected]> wrote:
> > > >
> > > >
> > > >
> > > > On 7/21/23 17:10, Michael S. Tsirkin wrote:
> > > > > On Fri, Jul 21, 2023 at 04:58:04PM +0200, Maxime Coquelin wrote:
> > > > >>
> > > > >>
> > > > >> On 7/21/23 16:45, Michael S. Tsirkin wrote:
> > > > >>> On Fri, Jul 21, 2023 at 04:37:00PM +0200, Maxime Coquelin wrote:
> > > > >>>>
> > > > >>>>
> > > > >>>> On 7/20/23 23:02, Michael S. Tsirkin wrote:
> > > > >>>>> On Thu, Jul 20, 2023 at 01:26:20PM -0700, Shannon Nelson wrote:
> > > > >>>>>> On 7/20/23 1:38 AM, Jason Wang wrote:
> > > > >>>>>>>
> > > > >>>>>>> Adding cond_resched() to the command waiting loop for a better
> > > > >>>>>>> co-operation with the scheduler. This allows to give CPU a breath to
> > > > >>>>>>> run other task(workqueue) instead of busy looping when preemption is
> > > > >>>>>>> not allowed on a device whose CVQ might be slow.
> > > > >>>>>>>
> > > > >>>>>>> Signed-off-by: Jason Wang <[email protected]>
> > > > >>>>>>
> > > > >>>>>> This still leaves hung processes, but at least it doesn't pin the CPU any
> > > > >>>>>> more. Thanks.
> > > > >>>>>> Reviewed-by: Shannon Nelson <[email protected]>
> > > > >>>>>>
> > > > >>>>>
> > > > >>>>> I'd like to see a full solution
> > > > >>>>> 1- block until interrupt
> > >
> > > I remember in previous versions, you worried about the extra MSI
> > > vector. (Maybe I was wrong).
> > >
> > > > >>>>
> > > > >>>> Would it make sense to also have a timeout?
> > > > >>>> And when timeout expires, set FAILED bit in device status?
> > > > >>>
> > > > >>> virtio spec does not set any limits on the timing of vq
> > > > >>> processing.
> > > > >>
> > > > >> Indeed, but I thought the driver could decide it is too long for it.
> > > > >>
> > > > >> The issue is we keep waiting with rtnl locked, it can quickly make the
> > > > >> system unusable.
> > > > >
> > > > > if this is a problem we should find a way not to keep rtnl
> > > > > locked indefinitely.
> > >
> > > Any ideas on this direction? Simply dropping rtnl during the busy loop
> > > will result in a lot of races. This seems to require non-trivial
> > > changes in the networking core.
> > >
> > > >
> > > > From the tests I have done, I think it is. With OVS, a reconfiguration
> > > > is performed when the VDUSE device is added, and when a MLX5 device is
> > > > in the same bridge, it ends up doing an ioctl() that tries to take the
> > > > rtnl lock. In this configuration, it is not possible to kill OVS because
> > > > it is stuck trying to acquire rtnl lock for mlx5 that is held by virtio-
> > > > net.
> > >
> > > Yeah, basically, any RTNL users would be blocked forever.
> > >
> > > And the infinite loop has other side effects like it blocks the freezer to work.
> > >
> > > To summarize, there are three issues
> > >
> > > 1) busy polling
> > > 2) breaks freezer
> > > 3) hold RTNL during the loop
> > >
> > > Solving 3 may help somehow for 2 e.g some pm routine e.g wireguard or
> > > even virtnet_restore() itself may try to hold the lock.
> >
> > Yep. So my feeling currently is, the only real fix is to actually
> > queue up the work in software.
>
> Do you mean something like:
>
> rtnl_lock();
> queue up the work
> rtnl_unlock();
> return success;
>
> ?
yes
> > It's mostly trivial to limit
> > memory consumption, vid's is the
> > only one where it would make sense to have more than
> > 1 command of a given type outstanding.
>
> And rx mode so this implies we will fail any command if the previous
> work is not finished.
don't fail it, store it.
> > have a tree
> > or a bitmap with vids to add/remove?
>
> Probably.
>
> Thanks
>
> >
> >
> >
> > > >
> > > > >
> > > > >>>>> 2- still handle surprise removal correctly by waking in that case
> > >
> > > This is basically what version 1 did?
> > >
> > > https://lore.kernel.org/lkml/[email protected]/t/
> > >
> > > Thanks
> >
> > Yes - except the timeout part.
> >
> >
> > > > >>>>>
> > > > >>>>>
> > > > >>>>>
> > > > >>>>>>> ---
> > > > >>>>>>> drivers/net/virtio_net.c | 4 +++-
> > > > >>>>>>> 1 file changed, 3 insertions(+), 1 deletion(-)
> > > > >>>>>>>
> > > > >>>>>>> diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
> > > > >>>>>>> index 9f3b1d6ac33d..e7533f29b219 100644
> > > > >>>>>>> --- a/drivers/net/virtio_net.c
> > > > >>>>>>> +++ b/drivers/net/virtio_net.c
> > > > >>>>>>> @@ -2314,8 +2314,10 @@ static bool virtnet_send_command(struct virtnet_info *vi, u8 class, u8 cmd,
> > > > >>>>>>> * into the hypervisor, so the request should be handled immediately.
> > > > >>>>>>> */
> > > > >>>>>>> while (!virtqueue_get_buf(vi->cvq, &tmp) &&
> > > > >>>>>>> - !virtqueue_is_broken(vi->cvq))
> > > > >>>>>>> + !virtqueue_is_broken(vi->cvq)) {
> > > > >>>>>>> + cond_resched();
> > > > >>>>>>> cpu_relax();
> > > > >>>>>>> + }
> > > > >>>>>>>
> > > > >>>>>>> return vi->ctrl->status == VIRTIO_NET_OK;
> > > > >>>>>>> }
> > > > >>>>>>> --
> > > > >>>>>>> 2.39.3
> > > > >>>>>>>
> > > > >>>>>>> _______________________________________________
> > > > >>>>>>> Virtualization mailing list
> > > > >>>>>>> [email protected]
> > > > >>>>>>> https://lists.linuxfoundation.org/mailman/listinfo/virtualization
> > > > >>>>>
> > > > >>>
> > > > >
> > > >
> >
On Tue, Jul 25, 2023 at 3:36 PM Michael S. Tsirkin <[email protected]> wrote:
>
> On Tue, Jul 25, 2023 at 11:07:40AM +0800, Jason Wang wrote:
> > On Mon, Jul 24, 2023 at 3:17 PM Michael S. Tsirkin <[email protected]> wrote:
> > >
> > > On Mon, Jul 24, 2023 at 02:52:05PM +0800, Jason Wang wrote:
> > > > On Sat, Jul 22, 2023 at 4:18 AM Maxime Coquelin
> > > > <[email protected]> wrote:
> > > > >
> > > > >
> > > > >
> > > > > On 7/21/23 17:10, Michael S. Tsirkin wrote:
> > > > > > On Fri, Jul 21, 2023 at 04:58:04PM +0200, Maxime Coquelin wrote:
> > > > > >>
> > > > > >>
> > > > > >> On 7/21/23 16:45, Michael S. Tsirkin wrote:
> > > > > >>> On Fri, Jul 21, 2023 at 04:37:00PM +0200, Maxime Coquelin wrote:
> > > > > >>>>
> > > > > >>>>
> > > > > >>>> On 7/20/23 23:02, Michael S. Tsirkin wrote:
> > > > > >>>>> On Thu, Jul 20, 2023 at 01:26:20PM -0700, Shannon Nelson wrote:
> > > > > >>>>>> On 7/20/23 1:38 AM, Jason Wang wrote:
> > > > > >>>>>>>
> > > > > >>>>>>> Adding cond_resched() to the command waiting loop for a better
> > > > > >>>>>>> co-operation with the scheduler. This allows to give CPU a breath to
> > > > > >>>>>>> run other task(workqueue) instead of busy looping when preemption is
> > > > > >>>>>>> not allowed on a device whose CVQ might be slow.
> > > > > >>>>>>>
> > > > > >>>>>>> Signed-off-by: Jason Wang <[email protected]>
> > > > > >>>>>>
> > > > > >>>>>> This still leaves hung processes, but at least it doesn't pin the CPU any
> > > > > >>>>>> more. Thanks.
> > > > > >>>>>> Reviewed-by: Shannon Nelson <[email protected]>
> > > > > >>>>>>
> > > > > >>>>>
> > > > > >>>>> I'd like to see a full solution
> > > > > >>>>> 1- block until interrupt
> > > >
> > > > I remember in previous versions, you worried about the extra MSI
> > > > vector. (Maybe I was wrong).
> > > >
> > > > > >>>>
> > > > > >>>> Would it make sense to also have a timeout?
> > > > > >>>> And when timeout expires, set FAILED bit in device status?
> > > > > >>>
> > > > > >>> virtio spec does not set any limits on the timing of vq
> > > > > >>> processing.
> > > > > >>
> > > > > >> Indeed, but I thought the driver could decide it is too long for it.
> > > > > >>
> > > > > >> The issue is we keep waiting with rtnl locked, it can quickly make the
> > > > > >> system unusable.
> > > > > >
> > > > > > if this is a problem we should find a way not to keep rtnl
> > > > > > locked indefinitely.
> > > >
> > > > Any ideas on this direction? Simply dropping rtnl during the busy loop
> > > > will result in a lot of races. This seems to require non-trivial
> > > > changes in the networking core.
> > > >
> > > > >
> > > > > From the tests I have done, I think it is. With OVS, a reconfiguration
> > > > > is performed when the VDUSE device is added, and when a MLX5 device is
> > > > > in the same bridge, it ends up doing an ioctl() that tries to take the
> > > > > rtnl lock. In this configuration, it is not possible to kill OVS because
> > > > > it is stuck trying to acquire rtnl lock for mlx5 that is held by virtio-
> > > > > net.
> > > >
> > > > Yeah, basically, any RTNL users would be blocked forever.
> > > >
> > > > And the infinite loop has other side effects like it blocks the freezer to work.
> > > >
> > > > To summarize, there are three issues
> > > >
> > > > 1) busy polling
> > > > 2) breaks freezer
> > > > 3) hold RTNL during the loop
> > > >
> > > > Solving 3 may help somehow for 2 e.g some pm routine e.g wireguard or
> > > > even virtnet_restore() itself may try to hold the lock.
> > >
> > > Yep. So my feeling currently is, the only real fix is to actually
> > > queue up the work in software.
> >
> > Do you mean something like:
> >
> > rtnl_lock();
> > queue up the work
> > rtnl_unlock();
> > return success;
> >
> > ?
>
> yes
We will lose the error reporting, is it a real problem or not?
>
>
> > > It's mostly trivial to limit
> > > memory consumption, vid's is the
> > > only one where it would make sense to have more than
> > > 1 command of a given type outstanding.
> >
> > And rx mode so this implies we will fail any command if the previous
> > work is not finished.
>
> don't fail it, store it.
Ok.
Thanks
>
> > > have a tree
> > > or a bitmap with vids to add/remove?
> >
> > Probably.
> >
> > Thanks
> >
> > >
> > >
> > >
> > > > >
> > > > > >
> > > > > >>>>> 2- still handle surprise removal correctly by waking in that case
> > > >
> > > > This is basically what version 1 did?
> > > >
> > > > https://lore.kernel.org/lkml/[email protected]/t/
> > > >
> > > > Thanks
> > >
> > > Yes - except the timeout part.
> > >
> > >
> > > > > >>>>>
> > > > > >>>>>
> > > > > >>>>>
> > > > > >>>>>>> ---
> > > > > >>>>>>> drivers/net/virtio_net.c | 4 +++-
> > > > > >>>>>>> 1 file changed, 3 insertions(+), 1 deletion(-)
> > > > > >>>>>>>
> > > > > >>>>>>> diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
> > > > > >>>>>>> index 9f3b1d6ac33d..e7533f29b219 100644
> > > > > >>>>>>> --- a/drivers/net/virtio_net.c
> > > > > >>>>>>> +++ b/drivers/net/virtio_net.c
> > > > > >>>>>>> @@ -2314,8 +2314,10 @@ static bool virtnet_send_command(struct virtnet_info *vi, u8 class, u8 cmd,
> > > > > >>>>>>> * into the hypervisor, so the request should be handled immediately.
> > > > > >>>>>>> */
> > > > > >>>>>>> while (!virtqueue_get_buf(vi->cvq, &tmp) &&
> > > > > >>>>>>> - !virtqueue_is_broken(vi->cvq))
> > > > > >>>>>>> + !virtqueue_is_broken(vi->cvq)) {
> > > > > >>>>>>> + cond_resched();
> > > > > >>>>>>> cpu_relax();
> > > > > >>>>>>> + }
> > > > > >>>>>>>
> > > > > >>>>>>> return vi->ctrl->status == VIRTIO_NET_OK;
> > > > > >>>>>>> }
> > > > > >>>>>>> --
> > > > > >>>>>>> 2.39.3
> > > > > >>>>>>>
> > > > > >>>>>>> _______________________________________________
> > > > > >>>>>>> Virtualization mailing list
> > > > > >>>>>>> [email protected]
> > > > > >>>>>>> https://lists.linuxfoundation.org/mailman/listinfo/virtualization
> > > > > >>>>>
> > > > > >>>
> > > > > >
> > > > >
> > >
>
On Wed, Jul 26, 2023 at 09:55:37AM +0800, Jason Wang wrote:
> On Tue, Jul 25, 2023 at 3:36 PM Michael S. Tsirkin <[email protected]> wrote:
> >
> > On Tue, Jul 25, 2023 at 11:07:40AM +0800, Jason Wang wrote:
> > > On Mon, Jul 24, 2023 at 3:17 PM Michael S. Tsirkin <[email protected]> wrote:
> > > >
> > > > On Mon, Jul 24, 2023 at 02:52:05PM +0800, Jason Wang wrote:
> > > > > On Sat, Jul 22, 2023 at 4:18 AM Maxime Coquelin
> > > > > <[email protected]> wrote:
> > > > > >
> > > > > >
> > > > > >
> > > > > > On 7/21/23 17:10, Michael S. Tsirkin wrote:
> > > > > > > On Fri, Jul 21, 2023 at 04:58:04PM +0200, Maxime Coquelin wrote:
> > > > > > >>
> > > > > > >>
> > > > > > >> On 7/21/23 16:45, Michael S. Tsirkin wrote:
> > > > > > >>> On Fri, Jul 21, 2023 at 04:37:00PM +0200, Maxime Coquelin wrote:
> > > > > > >>>>
> > > > > > >>>>
> > > > > > >>>> On 7/20/23 23:02, Michael S. Tsirkin wrote:
> > > > > > >>>>> On Thu, Jul 20, 2023 at 01:26:20PM -0700, Shannon Nelson wrote:
> > > > > > >>>>>> On 7/20/23 1:38 AM, Jason Wang wrote:
> > > > > > >>>>>>>
> > > > > > >>>>>>> Adding cond_resched() to the command waiting loop for a better
> > > > > > >>>>>>> co-operation with the scheduler. This allows to give CPU a breath to
> > > > > > >>>>>>> run other task(workqueue) instead of busy looping when preemption is
> > > > > > >>>>>>> not allowed on a device whose CVQ might be slow.
> > > > > > >>>>>>>
> > > > > > >>>>>>> Signed-off-by: Jason Wang <[email protected]>
> > > > > > >>>>>>
> > > > > > >>>>>> This still leaves hung processes, but at least it doesn't pin the CPU any
> > > > > > >>>>>> more. Thanks.
> > > > > > >>>>>> Reviewed-by: Shannon Nelson <[email protected]>
> > > > > > >>>>>>
> > > > > > >>>>>
> > > > > > >>>>> I'd like to see a full solution
> > > > > > >>>>> 1- block until interrupt
> > > > >
> > > > > I remember in previous versions, you worried about the extra MSI
> > > > > vector. (Maybe I was wrong).
> > > > >
> > > > > > >>>>
> > > > > > >>>> Would it make sense to also have a timeout?
> > > > > > >>>> And when timeout expires, set FAILED bit in device status?
> > > > > > >>>
> > > > > > >>> virtio spec does not set any limits on the timing of vq
> > > > > > >>> processing.
> > > > > > >>
> > > > > > >> Indeed, but I thought the driver could decide it is too long for it.
> > > > > > >>
> > > > > > >> The issue is we keep waiting with rtnl locked, it can quickly make the
> > > > > > >> system unusable.
> > > > > > >
> > > > > > > if this is a problem we should find a way not to keep rtnl
> > > > > > > locked indefinitely.
> > > > >
> > > > > Any ideas on this direction? Simply dropping rtnl during the busy loop
> > > > > will result in a lot of races. This seems to require non-trivial
> > > > > changes in the networking core.
> > > > >
> > > > > >
> > > > > > From the tests I have done, I think it is. With OVS, a reconfiguration
> > > > > > is performed when the VDUSE device is added, and when a MLX5 device is
> > > > > > in the same bridge, it ends up doing an ioctl() that tries to take the
> > > > > > rtnl lock. In this configuration, it is not possible to kill OVS because
> > > > > > it is stuck trying to acquire rtnl lock for mlx5 that is held by virtio-
> > > > > > net.
> > > > >
> > > > > Yeah, basically, any RTNL users would be blocked forever.
> > > > >
> > > > > And the infinite loop has other side effects like it blocks the freezer to work.
> > > > >
> > > > > To summarize, there are three issues
> > > > >
> > > > > 1) busy polling
> > > > > 2) breaks freezer
> > > > > 3) hold RTNL during the loop
> > > > >
> > > > > Solving 3 may help somehow for 2 e.g some pm routine e.g wireguard or
> > > > > even virtnet_restore() itself may try to hold the lock.
> > > >
> > > > Yep. So my feeling currently is, the only real fix is to actually
> > > > queue up the work in software.
> > >
> > > Do you mean something like:
> > >
> > > rtnl_lock();
> > > queue up the work
> > > rtnl_unlock();
> > > return success;
> > >
> > > ?
> >
> > yes
>
> We will lose the error reporting, is it a real problem or not?
Fundamental isn't it? Maybe we want a per-device flag for a asynch commands,
and vduse will set it while hardware virtio won't.
this way we only lose error reporting for vduse.
> >
> >
> > > > It's mostly trivial to limit
> > > > memory consumption, vid's is the
> > > > only one where it would make sense to have more than
> > > > 1 command of a given type outstanding.
> > >
> > > And rx mode so this implies we will fail any command if the previous
> > > work is not finished.
> >
> > don't fail it, store it.
>
> Ok.
>
> Thanks
>
> >
> > > > have a tree
> > > > or a bitmap with vids to add/remove?
> > >
> > > Probably.
> > >
> > > Thanks
> > >
> > > >
> > > >
> > > >
> > > > > >
> > > > > > >
> > > > > > >>>>> 2- still handle surprise removal correctly by waking in that case
> > > > >
> > > > > This is basically what version 1 did?
> > > > >
> > > > > https://lore.kernel.org/lkml/[email protected]/t/
> > > > >
> > > > > Thanks
> > > >
> > > > Yes - except the timeout part.
> > > >
> > > >
> > > > > > >>>>>
> > > > > > >>>>>
> > > > > > >>>>>
> > > > > > >>>>>>> ---
> > > > > > >>>>>>> drivers/net/virtio_net.c | 4 +++-
> > > > > > >>>>>>> 1 file changed, 3 insertions(+), 1 deletion(-)
> > > > > > >>>>>>>
> > > > > > >>>>>>> diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
> > > > > > >>>>>>> index 9f3b1d6ac33d..e7533f29b219 100644
> > > > > > >>>>>>> --- a/drivers/net/virtio_net.c
> > > > > > >>>>>>> +++ b/drivers/net/virtio_net.c
> > > > > > >>>>>>> @@ -2314,8 +2314,10 @@ static bool virtnet_send_command(struct virtnet_info *vi, u8 class, u8 cmd,
> > > > > > >>>>>>> * into the hypervisor, so the request should be handled immediately.
> > > > > > >>>>>>> */
> > > > > > >>>>>>> while (!virtqueue_get_buf(vi->cvq, &tmp) &&
> > > > > > >>>>>>> - !virtqueue_is_broken(vi->cvq))
> > > > > > >>>>>>> + !virtqueue_is_broken(vi->cvq)) {
> > > > > > >>>>>>> + cond_resched();
> > > > > > >>>>>>> cpu_relax();
> > > > > > >>>>>>> + }
> > > > > > >>>>>>>
> > > > > > >>>>>>> return vi->ctrl->status == VIRTIO_NET_OK;
> > > > > > >>>>>>> }
> > > > > > >>>>>>> --
> > > > > > >>>>>>> 2.39.3
> > > > > > >>>>>>>
> > > > > > >>>>>>> _______________________________________________
> > > > > > >>>>>>> Virtualization mailing list
> > > > > > >>>>>>> [email protected]
> > > > > > >>>>>>> https://lists.linuxfoundation.org/mailman/listinfo/virtualization
> > > > > > >>>>>
> > > > > > >>>
> > > > > > >
> > > > > >
> > > >
> >
On Wed, Jul 26, 2023 at 7:38 PM Michael S. Tsirkin <[email protected]> wrote:
>
> On Wed, Jul 26, 2023 at 09:55:37AM +0800, Jason Wang wrote:
> > On Tue, Jul 25, 2023 at 3:36 PM Michael S. Tsirkin <[email protected]> wrote:
> > >
> > > On Tue, Jul 25, 2023 at 11:07:40AM +0800, Jason Wang wrote:
> > > > On Mon, Jul 24, 2023 at 3:17 PM Michael S. Tsirkin <[email protected]> wrote:
> > > > >
> > > > > On Mon, Jul 24, 2023 at 02:52:05PM +0800, Jason Wang wrote:
> > > > > > On Sat, Jul 22, 2023 at 4:18 AM Maxime Coquelin
> > > > > > <[email protected]> wrote:
> > > > > > >
> > > > > > >
> > > > > > >
> > > > > > > On 7/21/23 17:10, Michael S. Tsirkin wrote:
> > > > > > > > On Fri, Jul 21, 2023 at 04:58:04PM +0200, Maxime Coquelin wrote:
> > > > > > > >>
> > > > > > > >>
> > > > > > > >> On 7/21/23 16:45, Michael S. Tsirkin wrote:
> > > > > > > >>> On Fri, Jul 21, 2023 at 04:37:00PM +0200, Maxime Coquelin wrote:
> > > > > > > >>>>
> > > > > > > >>>>
> > > > > > > >>>> On 7/20/23 23:02, Michael S. Tsirkin wrote:
> > > > > > > >>>>> On Thu, Jul 20, 2023 at 01:26:20PM -0700, Shannon Nelson wrote:
> > > > > > > >>>>>> On 7/20/23 1:38 AM, Jason Wang wrote:
> > > > > > > >>>>>>>
> > > > > > > >>>>>>> Adding cond_resched() to the command waiting loop for a better
> > > > > > > >>>>>>> co-operation with the scheduler. This allows to give CPU a breath to
> > > > > > > >>>>>>> run other task(workqueue) instead of busy looping when preemption is
> > > > > > > >>>>>>> not allowed on a device whose CVQ might be slow.
> > > > > > > >>>>>>>
> > > > > > > >>>>>>> Signed-off-by: Jason Wang <[email protected]>
> > > > > > > >>>>>>
> > > > > > > >>>>>> This still leaves hung processes, but at least it doesn't pin the CPU any
> > > > > > > >>>>>> more. Thanks.
> > > > > > > >>>>>> Reviewed-by: Shannon Nelson <[email protected]>
> > > > > > > >>>>>>
> > > > > > > >>>>>
> > > > > > > >>>>> I'd like to see a full solution
> > > > > > > >>>>> 1- block until interrupt
> > > > > >
> > > > > > I remember in previous versions, you worried about the extra MSI
> > > > > > vector. (Maybe I was wrong).
> > > > > >
> > > > > > > >>>>
> > > > > > > >>>> Would it make sense to also have a timeout?
> > > > > > > >>>> And when timeout expires, set FAILED bit in device status?
> > > > > > > >>>
> > > > > > > >>> virtio spec does not set any limits on the timing of vq
> > > > > > > >>> processing.
> > > > > > > >>
> > > > > > > >> Indeed, but I thought the driver could decide it is too long for it.
> > > > > > > >>
> > > > > > > >> The issue is we keep waiting with rtnl locked, it can quickly make the
> > > > > > > >> system unusable.
> > > > > > > >
> > > > > > > > if this is a problem we should find a way not to keep rtnl
> > > > > > > > locked indefinitely.
> > > > > >
> > > > > > Any ideas on this direction? Simply dropping rtnl during the busy loop
> > > > > > will result in a lot of races. This seems to require non-trivial
> > > > > > changes in the networking core.
> > > > > >
> > > > > > >
> > > > > > > From the tests I have done, I think it is. With OVS, a reconfiguration
> > > > > > > is performed when the VDUSE device is added, and when a MLX5 device is
> > > > > > > in the same bridge, it ends up doing an ioctl() that tries to take the
> > > > > > > rtnl lock. In this configuration, it is not possible to kill OVS because
> > > > > > > it is stuck trying to acquire rtnl lock for mlx5 that is held by virtio-
> > > > > > > net.
> > > > > >
> > > > > > Yeah, basically, any RTNL users would be blocked forever.
> > > > > >
> > > > > > And the infinite loop has other side effects like it blocks the freezer to work.
> > > > > >
> > > > > > To summarize, there are three issues
> > > > > >
> > > > > > 1) busy polling
> > > > > > 2) breaks freezer
> > > > > > 3) hold RTNL during the loop
> > > > > >
> > > > > > Solving 3 may help somehow for 2 e.g some pm routine e.g wireguard or
> > > > > > even virtnet_restore() itself may try to hold the lock.
> > > > >
> > > > > Yep. So my feeling currently is, the only real fix is to actually
> > > > > queue up the work in software.
> > > >
> > > > Do you mean something like:
> > > >
> > > > rtnl_lock();
> > > > queue up the work
> > > > rtnl_unlock();
> > > > return success;
> > > >
> > > > ?
> > >
> > > yes
> >
> > We will lose the error reporting, is it a real problem or not?
>
> Fundamental isn't it? Maybe we want a per-device flag for a asynch commands,
> and vduse will set it while hardware virtio won't.
> this way we only lose error reporting for vduse.
This problem is not VDUSE specific, DPUs/vDPA may suffer from this as
well. This might require more thoughts.
Thanks
>
> > >
> > >
> > > > > It's mostly trivial to limit
> > > > > memory consumption, vid's is the
> > > > > only one where it would make sense to have more than
> > > > > 1 command of a given type outstanding.
> > > >
> > > > And rx mode so this implies we will fail any command if the previous
> > > > work is not finished.
> > >
> > > don't fail it, store it.
> >
> > Ok.
> >
> > Thanks
> >
> > >
> > > > > have a tree
> > > > > or a bitmap with vids to add/remove?
> > > >
> > > > Probably.
> > > >
> > > > Thanks
> > > >
> > > > >
> > > > >
> > > > >
> > > > > > >
> > > > > > > >
> > > > > > > >>>>> 2- still handle surprise removal correctly by waking in that case
> > > > > >
> > > > > > This is basically what version 1 did?
> > > > > >
> > > > > > https://lore.kernel.org/lkml/[email protected]/t/
> > > > > >
> > > > > > Thanks
> > > > >
> > > > > Yes - except the timeout part.
> > > > >
> > > > >
> > > > > > > >>>>>
> > > > > > > >>>>>
> > > > > > > >>>>>
> > > > > > > >>>>>>> ---
> > > > > > > >>>>>>> drivers/net/virtio_net.c | 4 +++-
> > > > > > > >>>>>>> 1 file changed, 3 insertions(+), 1 deletion(-)
> > > > > > > >>>>>>>
> > > > > > > >>>>>>> diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
> > > > > > > >>>>>>> index 9f3b1d6ac33d..e7533f29b219 100644
> > > > > > > >>>>>>> --- a/drivers/net/virtio_net.c
> > > > > > > >>>>>>> +++ b/drivers/net/virtio_net.c
> > > > > > > >>>>>>> @@ -2314,8 +2314,10 @@ static bool virtnet_send_command(struct virtnet_info *vi, u8 class, u8 cmd,
> > > > > > > >>>>>>> * into the hypervisor, so the request should be handled immediately.
> > > > > > > >>>>>>> */
> > > > > > > >>>>>>> while (!virtqueue_get_buf(vi->cvq, &tmp) &&
> > > > > > > >>>>>>> - !virtqueue_is_broken(vi->cvq))
> > > > > > > >>>>>>> + !virtqueue_is_broken(vi->cvq)) {
> > > > > > > >>>>>>> + cond_resched();
> > > > > > > >>>>>>> cpu_relax();
> > > > > > > >>>>>>> + }
> > > > > > > >>>>>>>
> > > > > > > >>>>>>> return vi->ctrl->status == VIRTIO_NET_OK;
> > > > > > > >>>>>>> }
> > > > > > > >>>>>>> --
> > > > > > > >>>>>>> 2.39.3
> > > > > > > >>>>>>>
> > > > > > > >>>>>>> _______________________________________________
> > > > > > > >>>>>>> Virtualization mailing list
> > > > > > > >>>>>>> [email protected]
> > > > > > > >>>>>>> https://lists.linuxfoundation.org/mailman/listinfo/virtualization
> > > > > > > >>>>>
> > > > > > > >>>
> > > > > > > >
> > > > > > >
> > > > >
> > >
>
On Thu, Jul 27, 2023 at 02:03:59PM +0800, Jason Wang wrote:
> On Wed, Jul 26, 2023 at 7:38 PM Michael S. Tsirkin <[email protected]> wrote:
> >
> > On Wed, Jul 26, 2023 at 09:55:37AM +0800, Jason Wang wrote:
> > > On Tue, Jul 25, 2023 at 3:36 PM Michael S. Tsirkin <[email protected]> wrote:
> > > >
> > > > On Tue, Jul 25, 2023 at 11:07:40AM +0800, Jason Wang wrote:
> > > > > On Mon, Jul 24, 2023 at 3:17 PM Michael S. Tsirkin <[email protected]> wrote:
> > > > > >
> > > > > > On Mon, Jul 24, 2023 at 02:52:05PM +0800, Jason Wang wrote:
> > > > > > > On Sat, Jul 22, 2023 at 4:18 AM Maxime Coquelin
> > > > > > > <[email protected]> wrote:
> > > > > > > >
> > > > > > > >
> > > > > > > >
> > > > > > > > On 7/21/23 17:10, Michael S. Tsirkin wrote:
> > > > > > > > > On Fri, Jul 21, 2023 at 04:58:04PM +0200, Maxime Coquelin wrote:
> > > > > > > > >>
> > > > > > > > >>
> > > > > > > > >> On 7/21/23 16:45, Michael S. Tsirkin wrote:
> > > > > > > > >>> On Fri, Jul 21, 2023 at 04:37:00PM +0200, Maxime Coquelin wrote:
> > > > > > > > >>>>
> > > > > > > > >>>>
> > > > > > > > >>>> On 7/20/23 23:02, Michael S. Tsirkin wrote:
> > > > > > > > >>>>> On Thu, Jul 20, 2023 at 01:26:20PM -0700, Shannon Nelson wrote:
> > > > > > > > >>>>>> On 7/20/23 1:38 AM, Jason Wang wrote:
> > > > > > > > >>>>>>>
> > > > > > > > >>>>>>> Adding cond_resched() to the command waiting loop for a better
> > > > > > > > >>>>>>> co-operation with the scheduler. This allows to give CPU a breath to
> > > > > > > > >>>>>>> run other task(workqueue) instead of busy looping when preemption is
> > > > > > > > >>>>>>> not allowed on a device whose CVQ might be slow.
> > > > > > > > >>>>>>>
> > > > > > > > >>>>>>> Signed-off-by: Jason Wang <[email protected]>
> > > > > > > > >>>>>>
> > > > > > > > >>>>>> This still leaves hung processes, but at least it doesn't pin the CPU any
> > > > > > > > >>>>>> more. Thanks.
> > > > > > > > >>>>>> Reviewed-by: Shannon Nelson <[email protected]>
> > > > > > > > >>>>>>
> > > > > > > > >>>>>
> > > > > > > > >>>>> I'd like to see a full solution
> > > > > > > > >>>>> 1- block until interrupt
> > > > > > >
> > > > > > > I remember in previous versions, you worried about the extra MSI
> > > > > > > vector. (Maybe I was wrong).
> > > > > > >
> > > > > > > > >>>>
> > > > > > > > >>>> Would it make sense to also have a timeout?
> > > > > > > > >>>> And when timeout expires, set FAILED bit in device status?
> > > > > > > > >>>
> > > > > > > > >>> virtio spec does not set any limits on the timing of vq
> > > > > > > > >>> processing.
> > > > > > > > >>
> > > > > > > > >> Indeed, but I thought the driver could decide it is too long for it.
> > > > > > > > >>
> > > > > > > > >> The issue is we keep waiting with rtnl locked, it can quickly make the
> > > > > > > > >> system unusable.
> > > > > > > > >
> > > > > > > > > if this is a problem we should find a way not to keep rtnl
> > > > > > > > > locked indefinitely.
> > > > > > >
> > > > > > > Any ideas on this direction? Simply dropping rtnl during the busy loop
> > > > > > > will result in a lot of races. This seems to require non-trivial
> > > > > > > changes in the networking core.
> > > > > > >
> > > > > > > >
> > > > > > > > From the tests I have done, I think it is. With OVS, a reconfiguration
> > > > > > > > is performed when the VDUSE device is added, and when a MLX5 device is
> > > > > > > > in the same bridge, it ends up doing an ioctl() that tries to take the
> > > > > > > > rtnl lock. In this configuration, it is not possible to kill OVS because
> > > > > > > > it is stuck trying to acquire rtnl lock for mlx5 that is held by virtio-
> > > > > > > > net.
> > > > > > >
> > > > > > > Yeah, basically, any RTNL users would be blocked forever.
> > > > > > >
> > > > > > > And the infinite loop has other side effects like it blocks the freezer to work.
> > > > > > >
> > > > > > > To summarize, there are three issues
> > > > > > >
> > > > > > > 1) busy polling
> > > > > > > 2) breaks freezer
> > > > > > > 3) hold RTNL during the loop
> > > > > > >
> > > > > > > Solving 3 may help somehow for 2 e.g some pm routine e.g wireguard or
> > > > > > > even virtnet_restore() itself may try to hold the lock.
> > > > > >
> > > > > > Yep. So my feeling currently is, the only real fix is to actually
> > > > > > queue up the work in software.
> > > > >
> > > > > Do you mean something like:
> > > > >
> > > > > rtnl_lock();
> > > > > queue up the work
> > > > > rtnl_unlock();
> > > > > return success;
> > > > >
> > > > > ?
> > > >
> > > > yes
> > >
> > > We will lose the error reporting, is it a real problem or not?
> >
> > Fundamental isn't it? Maybe we want a per-device flag for a asynch commands,
> > and vduse will set it while hardware virtio won't.
> > this way we only lose error reporting for vduse.
>
> This problem is not VDUSE specific, DPUs/vDPA may suffer from this as
> well. This might require more thoughts.
>
> Thanks
They really shouldn't - any NIC that takes forever to
program will create issues in the networking stack.
But if they do they can always set this flag too.
> >
> > > >
> > > >
> > > > > > It's mostly trivial to limit
> > > > > > memory consumption, vid's is the
> > > > > > only one where it would make sense to have more than
> > > > > > 1 command of a given type outstanding.
> > > > >
> > > > > And rx mode so this implies we will fail any command if the previous
> > > > > work is not finished.
> > > >
> > > > don't fail it, store it.
> > >
> > > Ok.
> > >
> > > Thanks
> > >
> > > >
> > > > > > have a tree
> > > > > > or a bitmap with vids to add/remove?
> > > > >
> > > > > Probably.
> > > > >
> > > > > Thanks
> > > > >
> > > > > >
> > > > > >
> > > > > >
> > > > > > > >
> > > > > > > > >
> > > > > > > > >>>>> 2- still handle surprise removal correctly by waking in that case
> > > > > > >
> > > > > > > This is basically what version 1 did?
> > > > > > >
> > > > > > > https://lore.kernel.org/lkml/[email protected]/t/
> > > > > > >
> > > > > > > Thanks
> > > > > >
> > > > > > Yes - except the timeout part.
> > > > > >
> > > > > >
> > > > > > > > >>>>>
> > > > > > > > >>>>>
> > > > > > > > >>>>>
> > > > > > > > >>>>>>> ---
> > > > > > > > >>>>>>> drivers/net/virtio_net.c | 4 +++-
> > > > > > > > >>>>>>> 1 file changed, 3 insertions(+), 1 deletion(-)
> > > > > > > > >>>>>>>
> > > > > > > > >>>>>>> diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
> > > > > > > > >>>>>>> index 9f3b1d6ac33d..e7533f29b219 100644
> > > > > > > > >>>>>>> --- a/drivers/net/virtio_net.c
> > > > > > > > >>>>>>> +++ b/drivers/net/virtio_net.c
> > > > > > > > >>>>>>> @@ -2314,8 +2314,10 @@ static bool virtnet_send_command(struct virtnet_info *vi, u8 class, u8 cmd,
> > > > > > > > >>>>>>> * into the hypervisor, so the request should be handled immediately.
> > > > > > > > >>>>>>> */
> > > > > > > > >>>>>>> while (!virtqueue_get_buf(vi->cvq, &tmp) &&
> > > > > > > > >>>>>>> - !virtqueue_is_broken(vi->cvq))
> > > > > > > > >>>>>>> + !virtqueue_is_broken(vi->cvq)) {
> > > > > > > > >>>>>>> + cond_resched();
> > > > > > > > >>>>>>> cpu_relax();
> > > > > > > > >>>>>>> + }
> > > > > > > > >>>>>>>
> > > > > > > > >>>>>>> return vi->ctrl->status == VIRTIO_NET_OK;
> > > > > > > > >>>>>>> }
> > > > > > > > >>>>>>> --
> > > > > > > > >>>>>>> 2.39.3
> > > > > > > > >>>>>>>
> > > > > > > > >>>>>>> _______________________________________________
> > > > > > > > >>>>>>> Virtualization mailing list
> > > > > > > > >>>>>>> [email protected]
> > > > > > > > >>>>>>> https://lists.linuxfoundation.org/mailman/listinfo/virtualization
> > > > > > > > >>>>>
> > > > > > > > >>>
> > > > > > > > >
> > > > > > > >
> > > > > >
> > > >
> >
On Thu, Jul 27, 2023 at 2:10 PM Michael S. Tsirkin <[email protected]> wrote:
>
> On Thu, Jul 27, 2023 at 02:03:59PM +0800, Jason Wang wrote:
> > On Wed, Jul 26, 2023 at 7:38 PM Michael S. Tsirkin <[email protected]> wrote:
> > >
> > > On Wed, Jul 26, 2023 at 09:55:37AM +0800, Jason Wang wrote:
> > > > On Tue, Jul 25, 2023 at 3:36 PM Michael S. Tsirkin <[email protected]> wrote:
> > > > >
> > > > > On Tue, Jul 25, 2023 at 11:07:40AM +0800, Jason Wang wrote:
> > > > > > On Mon, Jul 24, 2023 at 3:17 PM Michael S. Tsirkin <[email protected]> wrote:
> > > > > > >
> > > > > > > On Mon, Jul 24, 2023 at 02:52:05PM +0800, Jason Wang wrote:
> > > > > > > > On Sat, Jul 22, 2023 at 4:18 AM Maxime Coquelin
> > > > > > > > <[email protected]> wrote:
> > > > > > > > >
> > > > > > > > >
> > > > > > > > >
> > > > > > > > > On 7/21/23 17:10, Michael S. Tsirkin wrote:
> > > > > > > > > > On Fri, Jul 21, 2023 at 04:58:04PM +0200, Maxime Coquelin wrote:
> > > > > > > > > >>
> > > > > > > > > >>
> > > > > > > > > >> On 7/21/23 16:45, Michael S. Tsirkin wrote:
> > > > > > > > > >>> On Fri, Jul 21, 2023 at 04:37:00PM +0200, Maxime Coquelin wrote:
> > > > > > > > > >>>>
> > > > > > > > > >>>>
> > > > > > > > > >>>> On 7/20/23 23:02, Michael S. Tsirkin wrote:
> > > > > > > > > >>>>> On Thu, Jul 20, 2023 at 01:26:20PM -0700, Shannon Nelson wrote:
> > > > > > > > > >>>>>> On 7/20/23 1:38 AM, Jason Wang wrote:
> > > > > > > > > >>>>>>>
> > > > > > > > > >>>>>>> Adding cond_resched() to the command waiting loop for a better
> > > > > > > > > >>>>>>> co-operation with the scheduler. This allows to give CPU a breath to
> > > > > > > > > >>>>>>> run other task(workqueue) instead of busy looping when preemption is
> > > > > > > > > >>>>>>> not allowed on a device whose CVQ might be slow.
> > > > > > > > > >>>>>>>
> > > > > > > > > >>>>>>> Signed-off-by: Jason Wang <[email protected]>
> > > > > > > > > >>>>>>
> > > > > > > > > >>>>>> This still leaves hung processes, but at least it doesn't pin the CPU any
> > > > > > > > > >>>>>> more. Thanks.
> > > > > > > > > >>>>>> Reviewed-by: Shannon Nelson <[email protected]>
> > > > > > > > > >>>>>>
> > > > > > > > > >>>>>
> > > > > > > > > >>>>> I'd like to see a full solution
> > > > > > > > > >>>>> 1- block until interrupt
> > > > > > > >
> > > > > > > > I remember in previous versions, you worried about the extra MSI
> > > > > > > > vector. (Maybe I was wrong).
> > > > > > > >
> > > > > > > > > >>>>
> > > > > > > > > >>>> Would it make sense to also have a timeout?
> > > > > > > > > >>>> And when timeout expires, set FAILED bit in device status?
> > > > > > > > > >>>
> > > > > > > > > >>> virtio spec does not set any limits on the timing of vq
> > > > > > > > > >>> processing.
> > > > > > > > > >>
> > > > > > > > > >> Indeed, but I thought the driver could decide it is too long for it.
> > > > > > > > > >>
> > > > > > > > > >> The issue is we keep waiting with rtnl locked, it can quickly make the
> > > > > > > > > >> system unusable.
> > > > > > > > > >
> > > > > > > > > > if this is a problem we should find a way not to keep rtnl
> > > > > > > > > > locked indefinitely.
> > > > > > > >
> > > > > > > > Any ideas on this direction? Simply dropping rtnl during the busy loop
> > > > > > > > will result in a lot of races. This seems to require non-trivial
> > > > > > > > changes in the networking core.
> > > > > > > >
> > > > > > > > >
> > > > > > > > > From the tests I have done, I think it is. With OVS, a reconfiguration
> > > > > > > > > is performed when the VDUSE device is added, and when a MLX5 device is
> > > > > > > > > in the same bridge, it ends up doing an ioctl() that tries to take the
> > > > > > > > > rtnl lock. In this configuration, it is not possible to kill OVS because
> > > > > > > > > it is stuck trying to acquire rtnl lock for mlx5 that is held by virtio-
> > > > > > > > > net.
> > > > > > > >
> > > > > > > > Yeah, basically, any RTNL users would be blocked forever.
> > > > > > > >
> > > > > > > > And the infinite loop has other side effects like it blocks the freezer to work.
> > > > > > > >
> > > > > > > > To summarize, there are three issues
> > > > > > > >
> > > > > > > > 1) busy polling
> > > > > > > > 2) breaks freezer
> > > > > > > > 3) hold RTNL during the loop
> > > > > > > >
> > > > > > > > Solving 3 may help somehow for 2 e.g some pm routine e.g wireguard or
> > > > > > > > even virtnet_restore() itself may try to hold the lock.
> > > > > > >
> > > > > > > Yep. So my feeling currently is, the only real fix is to actually
> > > > > > > queue up the work in software.
> > > > > >
> > > > > > Do you mean something like:
> > > > > >
> > > > > > rtnl_lock();
> > > > > > queue up the work
> > > > > > rtnl_unlock();
> > > > > > return success;
> > > > > >
> > > > > > ?
> > > > >
> > > > > yes
> > > >
> > > > We will lose the error reporting, is it a real problem or not?
> > >
> > > Fundamental isn't it? Maybe we want a per-device flag for a asynch commands,
> > > and vduse will set it while hardware virtio won't.
> > > this way we only lose error reporting for vduse.
> >
> > This problem is not VDUSE specific, DPUs/vDPA may suffer from this as
> > well. This might require more thoughts.
> >
> > Thanks
>
> They really shouldn't - any NIC that takes forever to
> program will create issues in the networking stack.
Unfortunately, it's not rare as the device/cvq could be implemented
via firmware or software.
> But if they do they can always set this flag too.
This may have false negatives and may confuse the management.
Maybe we can extend the networking core to allow some device specific
configurations to be done with device specific lock without rtnl. For
example, split the set_channels to
pre_set_channels
set_channels
post_set_channels
The device specific part could be done in pre and post without a rtnl lock?
Thanks
>
> > >
> > > > >
> > > > >
> > > > > > > It's mostly trivial to limit
> > > > > > > memory consumption, vid's is the
> > > > > > > only one where it would make sense to have more than
> > > > > > > 1 command of a given type outstanding.
> > > > > >
> > > > > > And rx mode so this implies we will fail any command if the previous
> > > > > > work is not finished.
> > > > >
> > > > > don't fail it, store it.
> > > >
> > > > Ok.
> > > >
> > > > Thanks
> > > >
> > > > >
> > > > > > > have a tree
> > > > > > > or a bitmap with vids to add/remove?
> > > > > >
> > > > > > Probably.
> > > > > >
> > > > > > Thanks
> > > > > >
> > > > > > >
> > > > > > >
> > > > > > >
> > > > > > > > >
> > > > > > > > > >
> > > > > > > > > >>>>> 2- still handle surprise removal correctly by waking in that case
> > > > > > > >
> > > > > > > > This is basically what version 1 did?
> > > > > > > >
> > > > > > > > https://lore.kernel.org/lkml/[email protected]/t/
> > > > > > > >
> > > > > > > > Thanks
> > > > > > >
> > > > > > > Yes - except the timeout part.
> > > > > > >
> > > > > > >
> > > > > > > > > >>>>>
> > > > > > > > > >>>>>
> > > > > > > > > >>>>>
> > > > > > > > > >>>>>>> ---
> > > > > > > > > >>>>>>> drivers/net/virtio_net.c | 4 +++-
> > > > > > > > > >>>>>>> 1 file changed, 3 insertions(+), 1 deletion(-)
> > > > > > > > > >>>>>>>
> > > > > > > > > >>>>>>> diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
> > > > > > > > > >>>>>>> index 9f3b1d6ac33d..e7533f29b219 100644
> > > > > > > > > >>>>>>> --- a/drivers/net/virtio_net.c
> > > > > > > > > >>>>>>> +++ b/drivers/net/virtio_net.c
> > > > > > > > > >>>>>>> @@ -2314,8 +2314,10 @@ static bool virtnet_send_command(struct virtnet_info *vi, u8 class, u8 cmd,
> > > > > > > > > >>>>>>> * into the hypervisor, so the request should be handled immediately.
> > > > > > > > > >>>>>>> */
> > > > > > > > > >>>>>>> while (!virtqueue_get_buf(vi->cvq, &tmp) &&
> > > > > > > > > >>>>>>> - !virtqueue_is_broken(vi->cvq))
> > > > > > > > > >>>>>>> + !virtqueue_is_broken(vi->cvq)) {
> > > > > > > > > >>>>>>> + cond_resched();
> > > > > > > > > >>>>>>> cpu_relax();
> > > > > > > > > >>>>>>> + }
> > > > > > > > > >>>>>>>
> > > > > > > > > >>>>>>> return vi->ctrl->status == VIRTIO_NET_OK;
> > > > > > > > > >>>>>>> }
> > > > > > > > > >>>>>>> --
> > > > > > > > > >>>>>>> 2.39.3
> > > > > > > > > >>>>>>>
> > > > > > > > > >>>>>>> _______________________________________________
> > > > > > > > > >>>>>>> Virtualization mailing list
> > > > > > > > > >>>>>>> [email protected]
> > > > > > > > > >>>>>>> https://lists.linuxfoundation.org/mailman/listinfo/virtualization
> > > > > > > > > >>>>>
> > > > > > > > > >>>
> > > > > > > > > >
> > > > > > > > >
> > > > > > >
> > > > >
> > >
>
On Thu, Jul 27, 2023 at 04:59:33PM +0800, Jason Wang wrote:
> > They really shouldn't - any NIC that takes forever to
> > program will create issues in the networking stack.
>
> Unfortunately, it's not rare as the device/cvq could be implemented
> via firmware or software.
Currently that mean one either has sane firmware with a scheduler that
can meet deadlines, or loses ability to report errors back.
> > But if they do they can always set this flag too.
>
> This may have false negatives and may confuse the management.
>
> Maybe we can extend the networking core to allow some device specific
> configurations to be done with device specific lock without rtnl. For
> example, split the set_channels to
>
> pre_set_channels
> set_channels
> post_set_channels
>
> The device specific part could be done in pre and post without a rtnl lock?
>
> Thanks
Would the benefit be that errors can be reported to userspace then?
Then maybe. I think you will have to show how this works for at least
one card besides virtio.
--
MST
On Thu, Jul 27, 2023 at 5:46 PM Michael S. Tsirkin <[email protected]> wrote:
>
> On Thu, Jul 27, 2023 at 04:59:33PM +0800, Jason Wang wrote:
> > > They really shouldn't - any NIC that takes forever to
> > > program will create issues in the networking stack.
> >
> > Unfortunately, it's not rare as the device/cvq could be implemented
> > via firmware or software.
>
> Currently that mean one either has sane firmware with a scheduler that
> can meet deadlines, or loses ability to report errors back.
>
> > > But if they do they can always set this flag too.
> >
> > This may have false negatives and may confuse the management.
> >
> > Maybe we can extend the networking core to allow some device specific
> > configurations to be done with device specific lock without rtnl. For
> > example, split the set_channels to
> >
> > pre_set_channels
> > set_channels
> > post_set_channels
> >
> > The device specific part could be done in pre and post without a rtnl lock?
> >
> > Thanks
>
>
> Would the benefit be that errors can be reported to userspace then?
> Then maybe. I think you will have to show how this works for at least
> one card besides virtio.
Even for virtio, this seems not easy, as e.g the
virtnet_send_command() and netif_set_real_num_tx_queues() need to
appear to be atomic to the networking core.
I wonder if we can re-consider the way of a timeout here and choose a
sane value as a start.
Thanks
>
>
> --
> MST
>
On Mon, Jul 31, 2023 at 2:30 PM Jason Wang <[email protected]> wrote:
>
> On Thu, Jul 27, 2023 at 5:46 PM Michael S. Tsirkin <[email protected]> wrote:
> >
> > On Thu, Jul 27, 2023 at 04:59:33PM +0800, Jason Wang wrote:
> > > > They really shouldn't - any NIC that takes forever to
> > > > program will create issues in the networking stack.
> > >
> > > Unfortunately, it's not rare as the device/cvq could be implemented
> > > via firmware or software.
> >
> > Currently that mean one either has sane firmware with a scheduler that
> > can meet deadlines, or loses ability to report errors back.
> >
> > > > But if they do they can always set this flag too.
> > >
> > > This may have false negatives and may confuse the management.
> > >
> > > Maybe we can extend the networking core to allow some device specific
> > > configurations to be done with device specific lock without rtnl. For
> > > example, split the set_channels to
> > >
> > > pre_set_channels
> > > set_channels
> > > post_set_channels
> > >
> > > The device specific part could be done in pre and post without a rtnl lock?
> > >
> > > Thanks
> >
> >
> > Would the benefit be that errors can be reported to userspace then?
> > Then maybe. I think you will have to show how this works for at least
> > one card besides virtio.
>
> Even for virtio, this seems not easy, as e.g the
> virtnet_send_command() and netif_set_real_num_tx_queues() need to
> appear to be atomic to the networking core.
>
> I wonder if we can re-consider the way of a timeout here and choose a
> sane value as a start.
Michael, any more input on this?
Thanks
>
> Thanks
>
> >
> >
> > --
> > MST
> >
On Tue, Aug 08, 2023 at 10:30:56AM +0800, Jason Wang wrote:
> On Mon, Jul 31, 2023 at 2:30 PM Jason Wang <[email protected]> wrote:
> >
> > On Thu, Jul 27, 2023 at 5:46 PM Michael S. Tsirkin <[email protected]> wrote:
> > >
> > > On Thu, Jul 27, 2023 at 04:59:33PM +0800, Jason Wang wrote:
> > > > > They really shouldn't - any NIC that takes forever to
> > > > > program will create issues in the networking stack.
> > > >
> > > > Unfortunately, it's not rare as the device/cvq could be implemented
> > > > via firmware or software.
> > >
> > > Currently that mean one either has sane firmware with a scheduler that
> > > can meet deadlines, or loses ability to report errors back.
> > >
> > > > > But if they do they can always set this flag too.
> > > >
> > > > This may have false negatives and may confuse the management.
> > > >
> > > > Maybe we can extend the networking core to allow some device specific
> > > > configurations to be done with device specific lock without rtnl. For
> > > > example, split the set_channels to
> > > >
> > > > pre_set_channels
> > > > set_channels
> > > > post_set_channels
> > > >
> > > > The device specific part could be done in pre and post without a rtnl lock?
> > > >
> > > > Thanks
> > >
> > >
> > > Would the benefit be that errors can be reported to userspace then?
> > > Then maybe. I think you will have to show how this works for at least
> > > one card besides virtio.
> >
> > Even for virtio, this seems not easy, as e.g the
> > virtnet_send_command() and netif_set_real_num_tx_queues() need to
> > appear to be atomic to the networking core.
> >
> > I wonder if we can re-consider the way of a timeout here and choose a
> > sane value as a start.
>
> Michael, any more input on this?
>
> Thanks
I think this is just mission creep. We are trying to fix
vduse - let's do that for starters.
Recovering from firmware timeouts is far from trivial and
just assuming that just because it timed out it will not
access memory is just as likely to cause memory corruption
with worse results than an infinite spin.
I propose we fix this for vduse and assume hardware/firmware
is well behaved. Or maybe not well behaved firmware will
set the flag losing error reporting ability.
> >
> > Thanks
> >
> > >
> > >
> > > --
> > > MST
> > >
On Fri, Aug 11, 2023 at 3:41 AM Michael S. Tsirkin <[email protected]> wrote:
>
> On Tue, Aug 08, 2023 at 10:30:56AM +0800, Jason Wang wrote:
> > On Mon, Jul 31, 2023 at 2:30 PM Jason Wang <[email protected]> wrote:
> > >
> > > On Thu, Jul 27, 2023 at 5:46 PM Michael S. Tsirkin <[email protected]> wrote:
> > > >
> > > > On Thu, Jul 27, 2023 at 04:59:33PM +0800, Jason Wang wrote:
> > > > > > They really shouldn't - any NIC that takes forever to
> > > > > > program will create issues in the networking stack.
> > > > >
> > > > > Unfortunately, it's not rare as the device/cvq could be implemented
> > > > > via firmware or software.
> > > >
> > > > Currently that mean one either has sane firmware with a scheduler that
> > > > can meet deadlines, or loses ability to report errors back.
> > > >
> > > > > > But if they do they can always set this flag too.
> > > > >
> > > > > This may have false negatives and may confuse the management.
> > > > >
> > > > > Maybe we can extend the networking core to allow some device specific
> > > > > configurations to be done with device specific lock without rtnl. For
> > > > > example, split the set_channels to
> > > > >
> > > > > pre_set_channels
> > > > > set_channels
> > > > > post_set_channels
> > > > >
> > > > > The device specific part could be done in pre and post without a rtnl lock?
> > > > >
> > > > > Thanks
> > > >
> > > >
> > > > Would the benefit be that errors can be reported to userspace then?
> > > > Then maybe. I think you will have to show how this works for at least
> > > > one card besides virtio.
> > >
> > > Even for virtio, this seems not easy, as e.g the
> > > virtnet_send_command() and netif_set_real_num_tx_queues() need to
> > > appear to be atomic to the networking core.
> > >
> > > I wonder if we can re-consider the way of a timeout here and choose a
> > > sane value as a start.
> >
> > Michael, any more input on this?
> >
> > Thanks
>
> I think this is just mission creep. We are trying to fix
> vduse - let's do that for starters.
>
> Recovering from firmware timeouts is far from trivial and
> just assuming that just because it timed out it will not
> access memory is just as likely to cause memory corruption
> with worse results than an infinite spin.
Yes, this might require support not only in the driver
>
> I propose we fix this for vduse and assume hardware/firmware
> is well behaved.
One major case is the re-connection, in that case it might take
whatever longer that the kernel virito-net driver expects.
So we can have a timeout in VDUSE and trap CVQ then VDUSE can return
and fail early?
> Or maybe not well behaved firmware will
> set the flag losing error reporting ability.
This might be hard since it means not only the set but also the get is
unreliable.
Thanks
>
>
>
> > >
> > > Thanks
> > >
> > > >
> > > >
> > > > --
> > > > MST
> > > >
>
On Fri, Aug 11, 2023 at 10:23:15AM +0800, Jason Wang wrote:
> On Fri, Aug 11, 2023 at 3:41 AM Michael S. Tsirkin <[email protected]> wrote:
> >
> > On Tue, Aug 08, 2023 at 10:30:56AM +0800, Jason Wang wrote:
> > > On Mon, Jul 31, 2023 at 2:30 PM Jason Wang <[email protected]> wrote:
> > > >
> > > > On Thu, Jul 27, 2023 at 5:46 PM Michael S. Tsirkin <[email protected]> wrote:
> > > > >
> > > > > On Thu, Jul 27, 2023 at 04:59:33PM +0800, Jason Wang wrote:
> > > > > > > They really shouldn't - any NIC that takes forever to
> > > > > > > program will create issues in the networking stack.
> > > > > >
> > > > > > Unfortunately, it's not rare as the device/cvq could be implemented
> > > > > > via firmware or software.
> > > > >
> > > > > Currently that mean one either has sane firmware with a scheduler that
> > > > > can meet deadlines, or loses ability to report errors back.
> > > > >
> > > > > > > But if they do they can always set this flag too.
> > > > > >
> > > > > > This may have false negatives and may confuse the management.
> > > > > >
> > > > > > Maybe we can extend the networking core to allow some device specific
> > > > > > configurations to be done with device specific lock without rtnl. For
> > > > > > example, split the set_channels to
> > > > > >
> > > > > > pre_set_channels
> > > > > > set_channels
> > > > > > post_set_channels
> > > > > >
> > > > > > The device specific part could be done in pre and post without a rtnl lock?
> > > > > >
> > > > > > Thanks
> > > > >
> > > > >
> > > > > Would the benefit be that errors can be reported to userspace then?
> > > > > Then maybe. I think you will have to show how this works for at least
> > > > > one card besides virtio.
> > > >
> > > > Even for virtio, this seems not easy, as e.g the
> > > > virtnet_send_command() and netif_set_real_num_tx_queues() need to
> > > > appear to be atomic to the networking core.
> > > >
> > > > I wonder if we can re-consider the way of a timeout here and choose a
> > > > sane value as a start.
> > >
> > > Michael, any more input on this?
> > >
> > > Thanks
> >
> > I think this is just mission creep. We are trying to fix
> > vduse - let's do that for starters.
> >
> > Recovering from firmware timeouts is far from trivial and
> > just assuming that just because it timed out it will not
> > access memory is just as likely to cause memory corruption
> > with worse results than an infinite spin.
>
> Yes, this might require support not only in the driver
>
> >
> > I propose we fix this for vduse and assume hardware/firmware
> > is well behaved.
>
> One major case is the re-connection, in that case it might take
> whatever longer that the kernel virito-net driver expects.
> So we can have a timeout in VDUSE and trap CVQ then VDUSE can return
> and fail early?
Ugh more mission creep. not at all my point. vduse should cache
values in the driver, until someone manages to change
net core to be more friendly to userspace devices.
>
> > Or maybe not well behaved firmware will
> > set the flag losing error reporting ability.
>
> This might be hard since it means not only the set but also the get is
> unreliable.
>
> Thanks
/me shrugs
> >
> >
> >
> > > >
> > > > Thanks
> > > >
> > > > >
> > > > >
> > > > > --
> > > > > MST
> > > > >
> >
On Fri, Aug 11, 2023 at 1:42 PM Michael S. Tsirkin <[email protected]> wrote:
>
> On Fri, Aug 11, 2023 at 10:23:15AM +0800, Jason Wang wrote:
> > On Fri, Aug 11, 2023 at 3:41 AM Michael S. Tsirkin <[email protected]> wrote:
> > >
> > > On Tue, Aug 08, 2023 at 10:30:56AM +0800, Jason Wang wrote:
> > > > On Mon, Jul 31, 2023 at 2:30 PM Jason Wang <[email protected]> wrote:
> > > > >
> > > > > On Thu, Jul 27, 2023 at 5:46 PM Michael S. Tsirkin <[email protected]> wrote:
> > > > > >
> > > > > > On Thu, Jul 27, 2023 at 04:59:33PM +0800, Jason Wang wrote:
> > > > > > > > They really shouldn't - any NIC that takes forever to
> > > > > > > > program will create issues in the networking stack.
> > > > > > >
> > > > > > > Unfortunately, it's not rare as the device/cvq could be implemented
> > > > > > > via firmware or software.
> > > > > >
> > > > > > Currently that mean one either has sane firmware with a scheduler that
> > > > > > can meet deadlines, or loses ability to report errors back.
> > > > > >
> > > > > > > > But if they do they can always set this flag too.
> > > > > > >
> > > > > > > This may have false negatives and may confuse the management.
> > > > > > >
> > > > > > > Maybe we can extend the networking core to allow some device specific
> > > > > > > configurations to be done with device specific lock without rtnl. For
> > > > > > > example, split the set_channels to
> > > > > > >
> > > > > > > pre_set_channels
> > > > > > > set_channels
> > > > > > > post_set_channels
> > > > > > >
> > > > > > > The device specific part could be done in pre and post without a rtnl lock?
> > > > > > >
> > > > > > > Thanks
> > > > > >
> > > > > >
> > > > > > Would the benefit be that errors can be reported to userspace then?
> > > > > > Then maybe. I think you will have to show how this works for at least
> > > > > > one card besides virtio.
> > > > >
> > > > > Even for virtio, this seems not easy, as e.g the
> > > > > virtnet_send_command() and netif_set_real_num_tx_queues() need to
> > > > > appear to be atomic to the networking core.
> > > > >
> > > > > I wonder if we can re-consider the way of a timeout here and choose a
> > > > > sane value as a start.
> > > >
> > > > Michael, any more input on this?
> > > >
> > > > Thanks
> > >
> > > I think this is just mission creep. We are trying to fix
> > > vduse - let's do that for starters.
> > >
> > > Recovering from firmware timeouts is far from trivial and
> > > just assuming that just because it timed out it will not
> > > access memory is just as likely to cause memory corruption
> > > with worse results than an infinite spin.
> >
> > Yes, this might require support not only in the driver
> >
> > >
> > > I propose we fix this for vduse and assume hardware/firmware
> > > is well behaved.
> >
> > One major case is the re-connection, in that case it might take
> > whatever longer that the kernel virito-net driver expects.
> > So we can have a timeout in VDUSE and trap CVQ then VDUSE can return
> > and fail early?
>
> Ugh more mission creep. not at all my point. vduse should cache
> values in the driver,
What do you mean by values here? The cvq command?
Thanks
> until someone manages to change
> net core to be more friendly to userspace devices.
>
> >
> > > Or maybe not well behaved firmware will
> > > set the flag losing error reporting ability.
> >
> > This might be hard since it means not only the set but also the get is
> > unreliable.
> >
> > Thanks
>
> /me shrugs
>
>
>
> > >
> > >
> > >
> > > > >
> > > > > Thanks
> > > > >
> > > > > >
> > > > > >
> > > > > > --
> > > > > > MST
> > > > > >
> > >
>
On Fri, Aug 11, 2023 at 5:51 PM Michael S. Tsirkin <[email protected]> wrote:
>
> On Fri, Aug 11, 2023 at 05:43:25PM +0800, Jason Wang wrote:
> > On Fri, Aug 11, 2023 at 5:21 PM Michael S. Tsirkin <[email protected]> wrote:
> > >
> > > On Fri, Aug 11, 2023 at 05:18:51PM +0800, Jason Wang wrote:
> > > > On Fri, Aug 11, 2023 at 1:42 PM Michael S. Tsirkin <[email protected]> wrote:
> > > > >
> > > > > On Fri, Aug 11, 2023 at 10:23:15AM +0800, Jason Wang wrote:
> > > > > > On Fri, Aug 11, 2023 at 3:41 AM Michael S. Tsirkin <[email protected]> wrote:
> > > > > > >
> > > > > > > On Tue, Aug 08, 2023 at 10:30:56AM +0800, Jason Wang wrote:
> > > > > > > > On Mon, Jul 31, 2023 at 2:30 PM Jason Wang <[email protected]> wrote:
> > > > > > > > >
> > > > > > > > > On Thu, Jul 27, 2023 at 5:46 PM Michael S. Tsirkin <[email protected]> wrote:
> > > > > > > > > >
> > > > > > > > > > On Thu, Jul 27, 2023 at 04:59:33PM +0800, Jason Wang wrote:
> > > > > > > > > > > > They really shouldn't - any NIC that takes forever to
> > > > > > > > > > > > program will create issues in the networking stack.
> > > > > > > > > > >
> > > > > > > > > > > Unfortunately, it's not rare as the device/cvq could be implemented
> > > > > > > > > > > via firmware or software.
> > > > > > > > > >
> > > > > > > > > > Currently that mean one either has sane firmware with a scheduler that
> > > > > > > > > > can meet deadlines, or loses ability to report errors back.
> > > > > > > > > >
> > > > > > > > > > > > But if they do they can always set this flag too.
> > > > > > > > > > >
> > > > > > > > > > > This may have false negatives and may confuse the management.
> > > > > > > > > > >
> > > > > > > > > > > Maybe we can extend the networking core to allow some device specific
> > > > > > > > > > > configurations to be done with device specific lock without rtnl. For
> > > > > > > > > > > example, split the set_channels to
> > > > > > > > > > >
> > > > > > > > > > > pre_set_channels
> > > > > > > > > > > set_channels
> > > > > > > > > > > post_set_channels
> > > > > > > > > > >
> > > > > > > > > > > The device specific part could be done in pre and post without a rtnl lock?
> > > > > > > > > > >
> > > > > > > > > > > Thanks
> > > > > > > > > >
> > > > > > > > > >
> > > > > > > > > > Would the benefit be that errors can be reported to userspace then?
> > > > > > > > > > Then maybe. I think you will have to show how this works for at least
> > > > > > > > > > one card besides virtio.
> > > > > > > > >
> > > > > > > > > Even for virtio, this seems not easy, as e.g the
> > > > > > > > > virtnet_send_command() and netif_set_real_num_tx_queues() need to
> > > > > > > > > appear to be atomic to the networking core.
> > > > > > > > >
> > > > > > > > > I wonder if we can re-consider the way of a timeout here and choose a
> > > > > > > > > sane value as a start.
> > > > > > > >
> > > > > > > > Michael, any more input on this?
> > > > > > > >
> > > > > > > > Thanks
> > > > > > >
> > > > > > > I think this is just mission creep. We are trying to fix
> > > > > > > vduse - let's do that for starters.
> > > > > > >
> > > > > > > Recovering from firmware timeouts is far from trivial and
> > > > > > > just assuming that just because it timed out it will not
> > > > > > > access memory is just as likely to cause memory corruption
> > > > > > > with worse results than an infinite spin.
> > > > > >
> > > > > > Yes, this might require support not only in the driver
> > > > > >
> > > > > > >
> > > > > > > I propose we fix this for vduse and assume hardware/firmware
> > > > > > > is well behaved.
> > > > > >
> > > > > > One major case is the re-connection, in that case it might take
> > > > > > whatever longer that the kernel virito-net driver expects.
> > > > > > So we can have a timeout in VDUSE and trap CVQ then VDUSE can return
> > > > > > and fail early?
> > > > >
> > > > > Ugh more mission creep. not at all my point. vduse should cache
> > > > > values in the driver,
> > > >
> > > > What do you mean by values here? The cvq command?
> > > >
> > > > Thanks
> > >
> > > The card status generally.
> >
> > Just to make sure I understand here. The CVQ needs to be processed by
> > the userspace now. How could we cache the status?
> >
> > Thanks
>
> vduse will have to process it in kernel.
Right, that's my understanding (trap CVQ).
Thanks
>
> > >
> > > > > until someone manages to change
> > > > > net core to be more friendly to userspace devices.
> > > > >
> > > > > >
> > > > > > > Or maybe not well behaved firmware will
> > > > > > > set the flag losing error reporting ability.
> > > > > >
> > > > > > This might be hard since it means not only the set but also the get is
> > > > > > unreliable.
> > > > > >
> > > > > > Thanks
> > > > >
> > > > > /me shrugs
> > > > >
> > > > >
> > > > >
> > > > > > >
> > > > > > >
> > > > > > >
> > > > > > > > >
> > > > > > > > > Thanks
> > > > > > > > >
> > > > > > > > > >
> > > > > > > > > >
> > > > > > > > > > --
> > > > > > > > > > MST
> > > > > > > > > >
> > > > > > >
> > > > >
> > >
>
On Fri, Aug 11, 2023 at 05:18:51PM +0800, Jason Wang wrote:
> On Fri, Aug 11, 2023 at 1:42 PM Michael S. Tsirkin <[email protected]> wrote:
> >
> > On Fri, Aug 11, 2023 at 10:23:15AM +0800, Jason Wang wrote:
> > > On Fri, Aug 11, 2023 at 3:41 AM Michael S. Tsirkin <[email protected]> wrote:
> > > >
> > > > On Tue, Aug 08, 2023 at 10:30:56AM +0800, Jason Wang wrote:
> > > > > On Mon, Jul 31, 2023 at 2:30 PM Jason Wang <[email protected]> wrote:
> > > > > >
> > > > > > On Thu, Jul 27, 2023 at 5:46 PM Michael S. Tsirkin <[email protected]> wrote:
> > > > > > >
> > > > > > > On Thu, Jul 27, 2023 at 04:59:33PM +0800, Jason Wang wrote:
> > > > > > > > > They really shouldn't - any NIC that takes forever to
> > > > > > > > > program will create issues in the networking stack.
> > > > > > > >
> > > > > > > > Unfortunately, it's not rare as the device/cvq could be implemented
> > > > > > > > via firmware or software.
> > > > > > >
> > > > > > > Currently that mean one either has sane firmware with a scheduler that
> > > > > > > can meet deadlines, or loses ability to report errors back.
> > > > > > >
> > > > > > > > > But if they do they can always set this flag too.
> > > > > > > >
> > > > > > > > This may have false negatives and may confuse the management.
> > > > > > > >
> > > > > > > > Maybe we can extend the networking core to allow some device specific
> > > > > > > > configurations to be done with device specific lock without rtnl. For
> > > > > > > > example, split the set_channels to
> > > > > > > >
> > > > > > > > pre_set_channels
> > > > > > > > set_channels
> > > > > > > > post_set_channels
> > > > > > > >
> > > > > > > > The device specific part could be done in pre and post without a rtnl lock?
> > > > > > > >
> > > > > > > > Thanks
> > > > > > >
> > > > > > >
> > > > > > > Would the benefit be that errors can be reported to userspace then?
> > > > > > > Then maybe. I think you will have to show how this works for at least
> > > > > > > one card besides virtio.
> > > > > >
> > > > > > Even for virtio, this seems not easy, as e.g the
> > > > > > virtnet_send_command() and netif_set_real_num_tx_queues() need to
> > > > > > appear to be atomic to the networking core.
> > > > > >
> > > > > > I wonder if we can re-consider the way of a timeout here and choose a
> > > > > > sane value as a start.
> > > > >
> > > > > Michael, any more input on this?
> > > > >
> > > > > Thanks
> > > >
> > > > I think this is just mission creep. We are trying to fix
> > > > vduse - let's do that for starters.
> > > >
> > > > Recovering from firmware timeouts is far from trivial and
> > > > just assuming that just because it timed out it will not
> > > > access memory is just as likely to cause memory corruption
> > > > with worse results than an infinite spin.
> > >
> > > Yes, this might require support not only in the driver
> > >
> > > >
> > > > I propose we fix this for vduse and assume hardware/firmware
> > > > is well behaved.
> > >
> > > One major case is the re-connection, in that case it might take
> > > whatever longer that the kernel virito-net driver expects.
> > > So we can have a timeout in VDUSE and trap CVQ then VDUSE can return
> > > and fail early?
> >
> > Ugh more mission creep. not at all my point. vduse should cache
> > values in the driver,
>
> What do you mean by values here? The cvq command?
>
> Thanks
The card status generally.
> > until someone manages to change
> > net core to be more friendly to userspace devices.
> >
> > >
> > > > Or maybe not well behaved firmware will
> > > > set the flag losing error reporting ability.
> > >
> > > This might be hard since it means not only the set but also the get is
> > > unreliable.
> > >
> > > Thanks
> >
> > /me shrugs
> >
> >
> >
> > > >
> > > >
> > > >
> > > > > >
> > > > > > Thanks
> > > > > >
> > > > > > >
> > > > > > >
> > > > > > > --
> > > > > > > MST
> > > > > > >
> > > >
> >
On Fri, Aug 11, 2023 at 5:21 PM Michael S. Tsirkin <[email protected]> wrote:
>
> On Fri, Aug 11, 2023 at 05:18:51PM +0800, Jason Wang wrote:
> > On Fri, Aug 11, 2023 at 1:42 PM Michael S. Tsirkin <[email protected]> wrote:
> > >
> > > On Fri, Aug 11, 2023 at 10:23:15AM +0800, Jason Wang wrote:
> > > > On Fri, Aug 11, 2023 at 3:41 AM Michael S. Tsirkin <[email protected]> wrote:
> > > > >
> > > > > On Tue, Aug 08, 2023 at 10:30:56AM +0800, Jason Wang wrote:
> > > > > > On Mon, Jul 31, 2023 at 2:30 PM Jason Wang <[email protected]> wrote:
> > > > > > >
> > > > > > > On Thu, Jul 27, 2023 at 5:46 PM Michael S. Tsirkin <[email protected]> wrote:
> > > > > > > >
> > > > > > > > On Thu, Jul 27, 2023 at 04:59:33PM +0800, Jason Wang wrote:
> > > > > > > > > > They really shouldn't - any NIC that takes forever to
> > > > > > > > > > program will create issues in the networking stack.
> > > > > > > > >
> > > > > > > > > Unfortunately, it's not rare as the device/cvq could be implemented
> > > > > > > > > via firmware or software.
> > > > > > > >
> > > > > > > > Currently that mean one either has sane firmware with a scheduler that
> > > > > > > > can meet deadlines, or loses ability to report errors back.
> > > > > > > >
> > > > > > > > > > But if they do they can always set this flag too.
> > > > > > > > >
> > > > > > > > > This may have false negatives and may confuse the management.
> > > > > > > > >
> > > > > > > > > Maybe we can extend the networking core to allow some device specific
> > > > > > > > > configurations to be done with device specific lock without rtnl. For
> > > > > > > > > example, split the set_channels to
> > > > > > > > >
> > > > > > > > > pre_set_channels
> > > > > > > > > set_channels
> > > > > > > > > post_set_channels
> > > > > > > > >
> > > > > > > > > The device specific part could be done in pre and post without a rtnl lock?
> > > > > > > > >
> > > > > > > > > Thanks
> > > > > > > >
> > > > > > > >
> > > > > > > > Would the benefit be that errors can be reported to userspace then?
> > > > > > > > Then maybe. I think you will have to show how this works for at least
> > > > > > > > one card besides virtio.
> > > > > > >
> > > > > > > Even for virtio, this seems not easy, as e.g the
> > > > > > > virtnet_send_command() and netif_set_real_num_tx_queues() need to
> > > > > > > appear to be atomic to the networking core.
> > > > > > >
> > > > > > > I wonder if we can re-consider the way of a timeout here and choose a
> > > > > > > sane value as a start.
> > > > > >
> > > > > > Michael, any more input on this?
> > > > > >
> > > > > > Thanks
> > > > >
> > > > > I think this is just mission creep. We are trying to fix
> > > > > vduse - let's do that for starters.
> > > > >
> > > > > Recovering from firmware timeouts is far from trivial and
> > > > > just assuming that just because it timed out it will not
> > > > > access memory is just as likely to cause memory corruption
> > > > > with worse results than an infinite spin.
> > > >
> > > > Yes, this might require support not only in the driver
> > > >
> > > > >
> > > > > I propose we fix this for vduse and assume hardware/firmware
> > > > > is well behaved.
> > > >
> > > > One major case is the re-connection, in that case it might take
> > > > whatever longer that the kernel virito-net driver expects.
> > > > So we can have a timeout in VDUSE and trap CVQ then VDUSE can return
> > > > and fail early?
> > >
> > > Ugh more mission creep. not at all my point. vduse should cache
> > > values in the driver,
> >
> > What do you mean by values here? The cvq command?
> >
> > Thanks
>
> The card status generally.
Just to make sure I understand here. The CVQ needs to be processed by
the userspace now. How could we cache the status?
Thanks
>
> > > until someone manages to change
> > > net core to be more friendly to userspace devices.
> > >
> > > >
> > > > > Or maybe not well behaved firmware will
> > > > > set the flag losing error reporting ability.
> > > >
> > > > This might be hard since it means not only the set but also the get is
> > > > unreliable.
> > > >
> > > > Thanks
> > >
> > > /me shrugs
> > >
> > >
> > >
> > > > >
> > > > >
> > > > >
> > > > > > >
> > > > > > > Thanks
> > > > > > >
> > > > > > > >
> > > > > > > >
> > > > > > > > --
> > > > > > > > MST
> > > > > > > >
> > > > >
> > >
>
On Fri, Aug 11, 2023 at 05:43:25PM +0800, Jason Wang wrote:
> On Fri, Aug 11, 2023 at 5:21 PM Michael S. Tsirkin <[email protected]> wrote:
> >
> > On Fri, Aug 11, 2023 at 05:18:51PM +0800, Jason Wang wrote:
> > > On Fri, Aug 11, 2023 at 1:42 PM Michael S. Tsirkin <[email protected]> wrote:
> > > >
> > > > On Fri, Aug 11, 2023 at 10:23:15AM +0800, Jason Wang wrote:
> > > > > On Fri, Aug 11, 2023 at 3:41 AM Michael S. Tsirkin <[email protected]> wrote:
> > > > > >
> > > > > > On Tue, Aug 08, 2023 at 10:30:56AM +0800, Jason Wang wrote:
> > > > > > > On Mon, Jul 31, 2023 at 2:30 PM Jason Wang <[email protected]> wrote:
> > > > > > > >
> > > > > > > > On Thu, Jul 27, 2023 at 5:46 PM Michael S. Tsirkin <[email protected]> wrote:
> > > > > > > > >
> > > > > > > > > On Thu, Jul 27, 2023 at 04:59:33PM +0800, Jason Wang wrote:
> > > > > > > > > > > They really shouldn't - any NIC that takes forever to
> > > > > > > > > > > program will create issues in the networking stack.
> > > > > > > > > >
> > > > > > > > > > Unfortunately, it's not rare as the device/cvq could be implemented
> > > > > > > > > > via firmware or software.
> > > > > > > > >
> > > > > > > > > Currently that mean one either has sane firmware with a scheduler that
> > > > > > > > > can meet deadlines, or loses ability to report errors back.
> > > > > > > > >
> > > > > > > > > > > But if they do they can always set this flag too.
> > > > > > > > > >
> > > > > > > > > > This may have false negatives and may confuse the management.
> > > > > > > > > >
> > > > > > > > > > Maybe we can extend the networking core to allow some device specific
> > > > > > > > > > configurations to be done with device specific lock without rtnl. For
> > > > > > > > > > example, split the set_channels to
> > > > > > > > > >
> > > > > > > > > > pre_set_channels
> > > > > > > > > > set_channels
> > > > > > > > > > post_set_channels
> > > > > > > > > >
> > > > > > > > > > The device specific part could be done in pre and post without a rtnl lock?
> > > > > > > > > >
> > > > > > > > > > Thanks
> > > > > > > > >
> > > > > > > > >
> > > > > > > > > Would the benefit be that errors can be reported to userspace then?
> > > > > > > > > Then maybe. I think you will have to show how this works for at least
> > > > > > > > > one card besides virtio.
> > > > > > > >
> > > > > > > > Even for virtio, this seems not easy, as e.g the
> > > > > > > > virtnet_send_command() and netif_set_real_num_tx_queues() need to
> > > > > > > > appear to be atomic to the networking core.
> > > > > > > >
> > > > > > > > I wonder if we can re-consider the way of a timeout here and choose a
> > > > > > > > sane value as a start.
> > > > > > >
> > > > > > > Michael, any more input on this?
> > > > > > >
> > > > > > > Thanks
> > > > > >
> > > > > > I think this is just mission creep. We are trying to fix
> > > > > > vduse - let's do that for starters.
> > > > > >
> > > > > > Recovering from firmware timeouts is far from trivial and
> > > > > > just assuming that just because it timed out it will not
> > > > > > access memory is just as likely to cause memory corruption
> > > > > > with worse results than an infinite spin.
> > > > >
> > > > > Yes, this might require support not only in the driver
> > > > >
> > > > > >
> > > > > > I propose we fix this for vduse and assume hardware/firmware
> > > > > > is well behaved.
> > > > >
> > > > > One major case is the re-connection, in that case it might take
> > > > > whatever longer that the kernel virito-net driver expects.
> > > > > So we can have a timeout in VDUSE and trap CVQ then VDUSE can return
> > > > > and fail early?
> > > >
> > > > Ugh more mission creep. not at all my point. vduse should cache
> > > > values in the driver,
> > >
> > > What do you mean by values here? The cvq command?
> > >
> > > Thanks
> >
> > The card status generally.
>
> Just to make sure I understand here. The CVQ needs to be processed by
> the userspace now. How could we cache the status?
>
> Thanks
vduse will have to process it in kernel.
> >
> > > > until someone manages to change
> > > > net core to be more friendly to userspace devices.
> > > >
> > > > >
> > > > > > Or maybe not well behaved firmware will
> > > > > > set the flag losing error reporting ability.
> > > > >
> > > > > This might be hard since it means not only the set but also the get is
> > > > > unreliable.
> > > > >
> > > > > Thanks
> > > >
> > > > /me shrugs
> > > >
> > > >
> > > >
> > > > > >
> > > > > >
> > > > > >
> > > > > > > >
> > > > > > > > Thanks
> > > > > > > >
> > > > > > > > >
> > > > > > > > >
> > > > > > > > > --
> > > > > > > > > MST
> > > > > > > > >
> > > > > >
> > > >
> >
On Fri, Aug 11, 2023 at 05:54:15PM +0800, Jason Wang wrote:
> On Fri, Aug 11, 2023 at 5:51 PM Michael S. Tsirkin <[email protected]> wrote:
> >
> > On Fri, Aug 11, 2023 at 05:43:25PM +0800, Jason Wang wrote:
> > > On Fri, Aug 11, 2023 at 5:21 PM Michael S. Tsirkin <[email protected]> wrote:
> > > >
> > > > On Fri, Aug 11, 2023 at 05:18:51PM +0800, Jason Wang wrote:
> > > > > On Fri, Aug 11, 2023 at 1:42 PM Michael S. Tsirkin <[email protected]> wrote:
> > > > > >
> > > > > > On Fri, Aug 11, 2023 at 10:23:15AM +0800, Jason Wang wrote:
> > > > > > > On Fri, Aug 11, 2023 at 3:41 AM Michael S. Tsirkin <[email protected]> wrote:
> > > > > > > >
> > > > > > > > On Tue, Aug 08, 2023 at 10:30:56AM +0800, Jason Wang wrote:
> > > > > > > > > On Mon, Jul 31, 2023 at 2:30 PM Jason Wang <[email protected]> wrote:
> > > > > > > > > >
> > > > > > > > > > On Thu, Jul 27, 2023 at 5:46 PM Michael S. Tsirkin <[email protected]> wrote:
> > > > > > > > > > >
> > > > > > > > > > > On Thu, Jul 27, 2023 at 04:59:33PM +0800, Jason Wang wrote:
> > > > > > > > > > > > > They really shouldn't - any NIC that takes forever to
> > > > > > > > > > > > > program will create issues in the networking stack.
> > > > > > > > > > > >
> > > > > > > > > > > > Unfortunately, it's not rare as the device/cvq could be implemented
> > > > > > > > > > > > via firmware or software.
> > > > > > > > > > >
> > > > > > > > > > > Currently that mean one either has sane firmware with a scheduler that
> > > > > > > > > > > can meet deadlines, or loses ability to report errors back.
> > > > > > > > > > >
> > > > > > > > > > > > > But if they do they can always set this flag too.
> > > > > > > > > > > >
> > > > > > > > > > > > This may have false negatives and may confuse the management.
> > > > > > > > > > > >
> > > > > > > > > > > > Maybe we can extend the networking core to allow some device specific
> > > > > > > > > > > > configurations to be done with device specific lock without rtnl. For
> > > > > > > > > > > > example, split the set_channels to
> > > > > > > > > > > >
> > > > > > > > > > > > pre_set_channels
> > > > > > > > > > > > set_channels
> > > > > > > > > > > > post_set_channels
> > > > > > > > > > > >
> > > > > > > > > > > > The device specific part could be done in pre and post without a rtnl lock?
> > > > > > > > > > > >
> > > > > > > > > > > > Thanks
> > > > > > > > > > >
> > > > > > > > > > >
> > > > > > > > > > > Would the benefit be that errors can be reported to userspace then?
> > > > > > > > > > > Then maybe. I think you will have to show how this works for at least
> > > > > > > > > > > one card besides virtio.
> > > > > > > > > >
> > > > > > > > > > Even for virtio, this seems not easy, as e.g the
> > > > > > > > > > virtnet_send_command() and netif_set_real_num_tx_queues() need to
> > > > > > > > > > appear to be atomic to the networking core.
> > > > > > > > > >
> > > > > > > > > > I wonder if we can re-consider the way of a timeout here and choose a
> > > > > > > > > > sane value as a start.
> > > > > > > > >
> > > > > > > > > Michael, any more input on this?
> > > > > > > > >
> > > > > > > > > Thanks
> > > > > > > >
> > > > > > > > I think this is just mission creep. We are trying to fix
> > > > > > > > vduse - let's do that for starters.
> > > > > > > >
> > > > > > > > Recovering from firmware timeouts is far from trivial and
> > > > > > > > just assuming that just because it timed out it will not
> > > > > > > > access memory is just as likely to cause memory corruption
> > > > > > > > with worse results than an infinite spin.
> > > > > > >
> > > > > > > Yes, this might require support not only in the driver
> > > > > > >
> > > > > > > >
> > > > > > > > I propose we fix this for vduse and assume hardware/firmware
> > > > > > > > is well behaved.
> > > > > > >
> > > > > > > One major case is the re-connection, in that case it might take
> > > > > > > whatever longer that the kernel virito-net driver expects.
> > > > > > > So we can have a timeout in VDUSE and trap CVQ then VDUSE can return
> > > > > > > and fail early?
> > > > > >
> > > > > > Ugh more mission creep. not at all my point. vduse should cache
> > > > > > values in the driver,
> > > > >
> > > > > What do you mean by values here? The cvq command?
> > > > >
> > > > > Thanks
> > > >
> > > > The card status generally.
> > >
> > > Just to make sure I understand here. The CVQ needs to be processed by
> > > the userspace now. How could we cache the status?
> > >
> > > Thanks
> >
> > vduse will have to process it in kernel.
>
> Right, that's my understanding (trap CVQ).
>
> Thanks
oh this what you mean by trap. ok. I don't see a new
for a timeout then though.
> >
> > > >
> > > > > > until someone manages to change
> > > > > > net core to be more friendly to userspace devices.
> > > > > >
> > > > > > >
> > > > > > > > Or maybe not well behaved firmware will
> > > > > > > > set the flag losing error reporting ability.
> > > > > > >
> > > > > > > This might be hard since it means not only the set but also the get is
> > > > > > > unreliable.
> > > > > > >
> > > > > > > Thanks
> > > > > >
> > > > > > /me shrugs
> > > > > >
> > > > > >
> > > > > >
> > > > > > > >
> > > > > > > >
> > > > > > > >
> > > > > > > > > >
> > > > > > > > > > Thanks
> > > > > > > > > >
> > > > > > > > > > >
> > > > > > > > > > >
> > > > > > > > > > > --
> > > > > > > > > > > MST
> > > > > > > > > > >
> > > > > > > >
> > > > > >
> > > >
> >
On 2023-07-24 a.m.2:46, Michael S. Tsirkin wrote:
> External email: Use caution opening links or attachments
>
>
> On Fri, Jul 21, 2023 at 10:18:03PM +0200, Maxime Coquelin wrote:
>>
>>
>> On 7/21/23 17:10, Michael S. Tsirkin wrote:
>>> On Fri, Jul 21, 2023 at 04:58:04PM +0200, Maxime Coquelin wrote:
>>>>
>>>>
>>>> On 7/21/23 16:45, Michael S. Tsirkin wrote:
>>>>> On Fri, Jul 21, 2023 at 04:37:00PM +0200, Maxime Coquelin wrote:
>>>>>>
>>>>>>
>>>>>> On 7/20/23 23:02, Michael S. Tsirkin wrote:
>>>>>>> On Thu, Jul 20, 2023 at 01:26:20PM -0700, Shannon Nelson wrote:
>>>>>>>> On 7/20/23 1:38 AM, Jason Wang wrote:
>>>>>>>>>
>>>>>>>>> Adding cond_resched() to the command waiting loop for a better
>>>>>>>>> co-operation with the scheduler. This allows to give CPU a breath to
>>>>>>>>> run other task(workqueue) instead of busy looping when preemption is
>>>>>>>>> not allowed on a device whose CVQ might be slow.
>>>>>>>>>
>>>>>>>>> Signed-off-by: Jason Wang <[email protected]>
>>>>>>>>
>>>>>>>> This still leaves hung processes, but at least it doesn't pin the CPU any
>>>>>>>> more. Thanks.
>>>>>>>> Reviewed-by: Shannon Nelson <[email protected]>
>>>>>>>>
>>>>>>>
>>>>>>> I'd like to see a full solution
>>>>>>> 1- block until interrupt
>>>>>>
>>>>>> Would it make sense to also have a timeout?
>>>>>> And when timeout expires, set FAILED bit in device status?
>>>>>
>>>>> virtio spec does not set any limits on the timing of vq
>>>>> processing.
>>>>
>>>> Indeed, but I thought the driver could decide it is too long for it.
>>>>
>>>> The issue is we keep waiting with rtnl locked, it can quickly make the
>>>> system unusable.
>>>
>>> if this is a problem we should find a way not to keep rtnl
>>> locked indefinitely.
>>
>> From the tests I have done, I think it is. With OVS, a reconfiguration is
>> performed when the VDUSE device is added, and when a MLX5 device is
>> in the same bridge, it ends up doing an ioctl() that tries to take the
>> rtnl lock. In this configuration, it is not possible to kill OVS because
>> it is stuck trying to acquire rtnl lock for mlx5 that is held by virtio-
>> net.
>
> So for sure, we can queue up the work and process it later.
> The somewhat tricky part is limiting the memory consumption.
>
>
Hi Jason
Excuse me, is there any plan for when will v5 patch series be sent out?
Will the v5 patches solve the problem of ctrlvq's infinite poll for
buggy devices?
Thanks
Feng
>>>
>>>>>>> 2- still handle surprise removal correctly by waking in that case
>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>>>> ---
>>>>>>>>> drivers/net/virtio_net.c | 4 +++-
>>>>>>>>> 1 file changed, 3 insertions(+), 1 deletion(-)
>>>>>>>>>
>>>>>>>>> diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
>>>>>>>>> index 9f3b1d6ac33d..e7533f29b219 100644
>>>>>>>>> --- a/drivers/net/virtio_net.c
>>>>>>>>> +++ b/drivers/net/virtio_net.c
>>>>>>>>> @@ -2314,8 +2314,10 @@ static bool virtnet_send_command(struct virtnet_info *vi, u8 class, u8 cmd,
>>>>>>>>> * into the hypervisor, so the request should be handled immediately.
>>>>>>>>> */
>>>>>>>>> while (!virtqueue_get_buf(vi->cvq, &tmp) &&
>>>>>>>>> - !virtqueue_is_broken(vi->cvq))
>>>>>>>>> + !virtqueue_is_broken(vi->cvq)) {
>>>>>>>>> + cond_resched();
>>>>>>>>> cpu_relax();
>>>>>>>>> + }
>>>>>>>>>
>>>>>>>>> return vi->ctrl->status == VIRTIO_NET_OK;
>>>>>>>>> }
>>>>>>>>> --
>>>>>>>>> 2.39.3
>>>>>>>>>
>>>>>>>>> _______________________________________________
>>>>>>>>> Virtualization mailing list
>>>>>>>>> [email protected]
>>>>>>>>> https://lists.linuxfoundation.org/mailman/listinfo/virtualization
>>>>>>>
>>>>>
>>>
>
> _______________________________________________
> Virtualization mailing list
> [email protected]
> https://lists.linuxfoundation.org/mailman/listinfo/virtualization
On Fri, Oct 6, 2023 at 3:35 AM Feng Liu <[email protected]> wrote:
>
>
>
> On 2023-07-24 a.m.2:46, Michael S. Tsirkin wrote:
> > External email: Use caution opening links or attachments
> >
> >
> > On Fri, Jul 21, 2023 at 10:18:03PM +0200, Maxime Coquelin wrote:
> >>
> >>
> >> On 7/21/23 17:10, Michael S. Tsirkin wrote:
> >>> On Fri, Jul 21, 2023 at 04:58:04PM +0200, Maxime Coquelin wrote:
> >>>>
> >>>>
> >>>> On 7/21/23 16:45, Michael S. Tsirkin wrote:
> >>>>> On Fri, Jul 21, 2023 at 04:37:00PM +0200, Maxime Coquelin wrote:
> >>>>>>
> >>>>>>
> >>>>>> On 7/20/23 23:02, Michael S. Tsirkin wrote:
> >>>>>>> On Thu, Jul 20, 2023 at 01:26:20PM -0700, Shannon Nelson wrote:
> >>>>>>>> On 7/20/23 1:38 AM, Jason Wang wrote:
> >>>>>>>>>
> >>>>>>>>> Adding cond_resched() to the command waiting loop for a better
> >>>>>>>>> co-operation with the scheduler. This allows to give CPU a breath to
> >>>>>>>>> run other task(workqueue) instead of busy looping when preemption is
> >>>>>>>>> not allowed on a device whose CVQ might be slow.
> >>>>>>>>>
> >>>>>>>>> Signed-off-by: Jason Wang <[email protected]>
> >>>>>>>>
> >>>>>>>> This still leaves hung processes, but at least it doesn't pin the CPU any
> >>>>>>>> more. Thanks.
> >>>>>>>> Reviewed-by: Shannon Nelson <[email protected]>
> >>>>>>>>
> >>>>>>>
> >>>>>>> I'd like to see a full solution
> >>>>>>> 1- block until interrupt
> >>>>>>
> >>>>>> Would it make sense to also have a timeout?
> >>>>>> And when timeout expires, set FAILED bit in device status?
> >>>>>
> >>>>> virtio spec does not set any limits on the timing of vq
> >>>>> processing.
> >>>>
> >>>> Indeed, but I thought the driver could decide it is too long for it.
> >>>>
> >>>> The issue is we keep waiting with rtnl locked, it can quickly make the
> >>>> system unusable.
> >>>
> >>> if this is a problem we should find a way not to keep rtnl
> >>> locked indefinitely.
> >>
> >> From the tests I have done, I think it is. With OVS, a reconfiguration is
> >> performed when the VDUSE device is added, and when a MLX5 device is
> >> in the same bridge, it ends up doing an ioctl() that tries to take the
> >> rtnl lock. In this configuration, it is not possible to kill OVS because
> >> it is stuck trying to acquire rtnl lock for mlx5 that is held by virtio-
> >> net.
> >
> > So for sure, we can queue up the work and process it later.
> > The somewhat tricky part is limiting the memory consumption.
> >
> >
>
>
> Hi Jason
>
> Excuse me, is there any plan for when will v5 patch series be sent out?
> Will the v5 patches solve the problem of ctrlvq's infinite poll for
> buggy devices?
We agree to harden VDUSE and,
It would be hard if we try to solve it at the virtio-net level, see
the discussions before. It might require support from various layers
(e.g networking core etc).
We can use workqueue etc as a mitigation. If Michael is fine with
this, I can post v5.
Thanks
>
> Thanks
> Feng
>
> >>>
> >>>>>>> 2- still handle surprise removal correctly by waking in that case
> >>>>>>>
> >>>>>>>
> >>>>>>>
> >>>>>>>>> ---
> >>>>>>>>> drivers/net/virtio_net.c | 4 +++-
> >>>>>>>>> 1 file changed, 3 insertions(+), 1 deletion(-)
> >>>>>>>>>
> >>>>>>>>> diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
> >>>>>>>>> index 9f3b1d6ac33d..e7533f29b219 100644
> >>>>>>>>> --- a/drivers/net/virtio_net.c
> >>>>>>>>> +++ b/drivers/net/virtio_net.c
> >>>>>>>>> @@ -2314,8 +2314,10 @@ static bool virtnet_send_command(struct virtnet_info *vi, u8 class, u8 cmd,
> >>>>>>>>> * into the hypervisor, so the request should be handled immediately.
> >>>>>>>>> */
> >>>>>>>>> while (!virtqueue_get_buf(vi->cvq, &tmp) &&
> >>>>>>>>> - !virtqueue_is_broken(vi->cvq))
> >>>>>>>>> + !virtqueue_is_broken(vi->cvq)) {
> >>>>>>>>> + cond_resched();
> >>>>>>>>> cpu_relax();
> >>>>>>>>> + }
> >>>>>>>>>
> >>>>>>>>> return vi->ctrl->status == VIRTIO_NET_OK;
> >>>>>>>>> }
> >>>>>>>>> --
> >>>>>>>>> 2.39.3
> >>>>>>>>>
> >>>>>>>>> _______________________________________________
> >>>>>>>>> Virtualization mailing list
> >>>>>>>>> [email protected]
> >>>>>>>>> https://lists.linuxfoundation.org/mailman/listinfo/virtualization
> >>>>>>>
> >>>>>
> >>>
> >
> > _______________________________________________
> > Virtualization mailing list
> > [email protected]
> > https://lists.linuxfoundation.org/mailman/listinfo/virtualization
>
On Tue, Jul 25, 2023 at 11:03:11AM +0800, Jason Wang wrote:
> On Mon, Jul 24, 2023 at 3:18 PM Michael S. Tsirkin <[email protected]> wrote:
> >
> > On Mon, Jul 24, 2023 at 02:52:49PM +0800, Jason Wang wrote:
> > > On Mon, Jul 24, 2023 at 2:46 PM Michael S. Tsirkin <[email protected]> wrote:
> > > >
> > > > On Fri, Jul 21, 2023 at 10:18:03PM +0200, Maxime Coquelin wrote:
> > > > >
> > > > >
> > > > > On 7/21/23 17:10, Michael S. Tsirkin wrote:
> > > > > > On Fri, Jul 21, 2023 at 04:58:04PM +0200, Maxime Coquelin wrote:
> > > > > > >
> > > > > > >
> > > > > > > On 7/21/23 16:45, Michael S. Tsirkin wrote:
> > > > > > > > On Fri, Jul 21, 2023 at 04:37:00PM +0200, Maxime Coquelin wrote:
> > > > > > > > >
> > > > > > > > >
> > > > > > > > > On 7/20/23 23:02, Michael S. Tsirkin wrote:
> > > > > > > > > > On Thu, Jul 20, 2023 at 01:26:20PM -0700, Shannon Nelson wrote:
> > > > > > > > > > > On 7/20/23 1:38 AM, Jason Wang wrote:
> > > > > > > > > > > >
> > > > > > > > > > > > Adding cond_resched() to the command waiting loop for a better
> > > > > > > > > > > > co-operation with the scheduler. This allows to give CPU a breath to
> > > > > > > > > > > > run other task(workqueue) instead of busy looping when preemption is
> > > > > > > > > > > > not allowed on a device whose CVQ might be slow.
> > > > > > > > > > > >
> > > > > > > > > > > > Signed-off-by: Jason Wang <[email protected]>
> > > > > > > > > > >
> > > > > > > > > > > This still leaves hung processes, but at least it doesn't pin the CPU any
> > > > > > > > > > > more. Thanks.
> > > > > > > > > > > Reviewed-by: Shannon Nelson <[email protected]>
> > > > > > > > > > >
> > > > > > > > > >
> > > > > > > > > > I'd like to see a full solution
> > > > > > > > > > 1- block until interrupt
> > > > > > > > >
> > > > > > > > > Would it make sense to also have a timeout?
> > > > > > > > > And when timeout expires, set FAILED bit in device status?
> > > > > > > >
> > > > > > > > virtio spec does not set any limits on the timing of vq
> > > > > > > > processing.
> > > > > > >
> > > > > > > Indeed, but I thought the driver could decide it is too long for it.
> > > > > > >
> > > > > > > The issue is we keep waiting with rtnl locked, it can quickly make the
> > > > > > > system unusable.
> > > > > >
> > > > > > if this is a problem we should find a way not to keep rtnl
> > > > > > locked indefinitely.
> > > > >
> > > > > From the tests I have done, I think it is. With OVS, a reconfiguration is
> > > > > performed when the VDUSE device is added, and when a MLX5 device is
> > > > > in the same bridge, it ends up doing an ioctl() that tries to take the
> > > > > rtnl lock. In this configuration, it is not possible to kill OVS because
> > > > > it is stuck trying to acquire rtnl lock for mlx5 that is held by virtio-
> > > > > net.
> > > >
> > > > So for sure, we can queue up the work and process it later.
> > > > The somewhat tricky part is limiting the memory consumption.
> > >
> > > And it needs to sync with rtnl somehow, e.g device unregistering which
> > > seems not easy.
> > >
> > > Thanks
> >
> > since when does device unregister need to send cvq commands?
>
> It doesn't do this now. But if we don't process the work under rtnl,
> we need to synchronize with device unregistering.
>
> Thanks
But what's not easy about it?
> >
> > > >
> > > >
> > > > > >
> > > > > > > > > > 2- still handle surprise removal correctly by waking in that case
> > > > > > > > > >
> > > > > > > > > >
> > > > > > > > > >
> > > > > > > > > > > > ---
> > > > > > > > > > > > drivers/net/virtio_net.c | 4 +++-
> > > > > > > > > > > > 1 file changed, 3 insertions(+), 1 deletion(-)
> > > > > > > > > > > >
> > > > > > > > > > > > diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
> > > > > > > > > > > > index 9f3b1d6ac33d..e7533f29b219 100644
> > > > > > > > > > > > --- a/drivers/net/virtio_net.c
> > > > > > > > > > > > +++ b/drivers/net/virtio_net.c
> > > > > > > > > > > > @@ -2314,8 +2314,10 @@ static bool virtnet_send_command(struct virtnet_info *vi, u8 class, u8 cmd,
> > > > > > > > > > > > * into the hypervisor, so the request should be handled immediately.
> > > > > > > > > > > > */
> > > > > > > > > > > > while (!virtqueue_get_buf(vi->cvq, &tmp) &&
> > > > > > > > > > > > - !virtqueue_is_broken(vi->cvq))
> > > > > > > > > > > > + !virtqueue_is_broken(vi->cvq)) {
> > > > > > > > > > > > + cond_resched();
> > > > > > > > > > > > cpu_relax();
> > > > > > > > > > > > + }
> > > > > > > > > > > >
> > > > > > > > > > > > return vi->ctrl->status == VIRTIO_NET_OK;
> > > > > > > > > > > > }
> > > > > > > > > > > > --
> > > > > > > > > > > > 2.39.3
> > > > > > > > > > > >
> > > > > > > > > > > > _______________________________________________
> > > > > > > > > > > > Virtualization mailing list
> > > > > > > > > > > > [email protected]
> > > > > > > > > > > > https://lists.linuxfoundation.org/mailman/listinfo/virtualization
> > > > > > > > > >
> > > > > > > >
> > > > > >
> > > >
> >
On Fri, Feb 23, 2024 at 3:22 AM Michael S. Tsirkin <[email protected]> wrote:
>
> On Tue, Jul 25, 2023 at 11:03:11AM +0800, Jason Wang wrote:
> > On Mon, Jul 24, 2023 at 3:18 PM Michael S. Tsirkin <[email protected]> wrote:
> > >
> > > On Mon, Jul 24, 2023 at 02:52:49PM +0800, Jason Wang wrote:
> > > > On Mon, Jul 24, 2023 at 2:46 PM Michael S. Tsirkin <[email protected]> wrote:
> > > > >
> > > > > On Fri, Jul 21, 2023 at 10:18:03PM +0200, Maxime Coquelin wrote:
> > > > > >
> > > > > >
> > > > > > On 7/21/23 17:10, Michael S. Tsirkin wrote:
> > > > > > > On Fri, Jul 21, 2023 at 04:58:04PM +0200, Maxime Coquelin wrote:
> > > > > > > >
> > > > > > > >
> > > > > > > > On 7/21/23 16:45, Michael S. Tsirkin wrote:
> > > > > > > > > On Fri, Jul 21, 2023 at 04:37:00PM +0200, Maxime Coquelin wrote:
> > > > > > > > > >
> > > > > > > > > >
> > > > > > > > > > On 7/20/23 23:02, Michael S. Tsirkin wrote:
> > > > > > > > > > > On Thu, Jul 20, 2023 at 01:26:20PM -0700, Shannon Nelson wrote:
> > > > > > > > > > > > On 7/20/23 1:38 AM, Jason Wang wrote:
> > > > > > > > > > > > >
> > > > > > > > > > > > > Adding cond_resched() to the command waiting loop for a better
> > > > > > > > > > > > > co-operation with the scheduler. This allows to give CPU a breath to
> > > > > > > > > > > > > run other task(workqueue) instead of busy looping when preemption is
> > > > > > > > > > > > > not allowed on a device whose CVQ might be slow.
> > > > > > > > > > > > >
> > > > > > > > > > > > > Signed-off-by: Jason Wang <[email protected]>
> > > > > > > > > > > >
> > > > > > > > > > > > This still leaves hung processes, but at least it doesn't pin the CPU any
> > > > > > > > > > > > more. Thanks.
> > > > > > > > > > > > Reviewed-by: Shannon Nelson <[email protected]>
> > > > > > > > > > > >
> > > > > > > > > > >
> > > > > > > > > > > I'd like to see a full solution
> > > > > > > > > > > 1- block until interrupt
> > > > > > > > > >
> > > > > > > > > > Would it make sense to also have a timeout?
> > > > > > > > > > And when timeout expires, set FAILED bit in device status?
> > > > > > > > >
> > > > > > > > > virtio spec does not set any limits on the timing of vq
> > > > > > > > > processing.
> > > > > > > >
> > > > > > > > Indeed, but I thought the driver could decide it is too long for it.
> > > > > > > >
> > > > > > > > The issue is we keep waiting with rtnl locked, it can quickly make the
> > > > > > > > system unusable.
> > > > > > >
> > > > > > > if this is a problem we should find a way not to keep rtnl
> > > > > > > locked indefinitely.
> > > > > >
> > > > > > From the tests I have done, I think it is. With OVS, a reconfiguration is
> > > > > > performed when the VDUSE device is added, and when a MLX5 device is
> > > > > > in the same bridge, it ends up doing an ioctl() that tries to take the
> > > > > > rtnl lock. In this configuration, it is not possible to kill OVS because
> > > > > > it is stuck trying to acquire rtnl lock for mlx5 that is held by virtio-
> > > > > > net.
> > > > >
> > > > > So for sure, we can queue up the work and process it later.
> > > > > The somewhat tricky part is limiting the memory consumption.
> > > >
> > > > And it needs to sync with rtnl somehow, e.g device unregistering which
> > > > seems not easy.
> > > >
> > > > Thanks
> > >
> > > since when does device unregister need to send cvq commands?
> >
> > It doesn't do this now. But if we don't process the work under rtnl,
> > we need to synchronize with device unregistering.
> >
> > Thanks
>
> But what's not easy about it?
Yes, so I'm wondering if we can simply have the cond_resched() merged
as the first step.
And leave the indefinite wait for future investigation?
Thanks
>
> > >
> > > > >
> > > > >
> > > > > > >
> > > > > > > > > > > 2- still handle surprise removal correctly by waking in that case
> > > > > > > > > > >
> > > > > > > > > > >
> > > > > > > > > > >
> > > > > > > > > > > > > ---
> > > > > > > > > > > > > drivers/net/virtio_net.c | 4 +++-
> > > > > > > > > > > > > 1 file changed, 3 insertions(+), 1 deletion(-)
> > > > > > > > > > > > >
> > > > > > > > > > > > > diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
> > > > > > > > > > > > > index 9f3b1d6ac33d..e7533f29b219 100644
> > > > > > > > > > > > > --- a/drivers/net/virtio_net.c
> > > > > > > > > > > > > +++ b/drivers/net/virtio_net.c
> > > > > > > > > > > > > @@ -2314,8 +2314,10 @@ static bool virtnet_send_command(struct virtnet_info *vi, u8 class, u8 cmd,
> > > > > > > > > > > > > * into the hypervisor, so the request should be handled immediately.
> > > > > > > > > > > > > */
> > > > > > > > > > > > > while (!virtqueue_get_buf(vi->cvq, &tmp) &&
> > > > > > > > > > > > > - !virtqueue_is_broken(vi->cvq))
> > > > > > > > > > > > > + !virtqueue_is_broken(vi->cvq)) {
> > > > > > > > > > > > > + cond_resched();
> > > > > > > > > > > > > cpu_relax();
> > > > > > > > > > > > > + }
> > > > > > > > > > > > >
> > > > > > > > > > > > > return vi->ctrl->status == VIRTIO_NET_OK;
> > > > > > > > > > > > > }
> > > > > > > > > > > > > --
> > > > > > > > > > > > > 2.39.3
> > > > > > > > > > > > >
> > > > > > > > > > > > > _______________________________________________
> > > > > > > > > > > > > Virtualization mailing list
> > > > > > > > > > > > > [email protected]
> > > > > > > > > > > > > https://lists.linuxfoundation.org/mailman/listinfo/virtualization
> > > > > > > > > > >
> > > > > > > > >
> > > > > > >
> > > > >
> > >
>