Re: [PATCH net-next v4 2/2] virtio-net: add cond_resched() to the command waiting loop
On Fri, Oct 6, 2023 at 3:35 AM Feng Liu wrote:
>
>
>
> On 2023-07-24 a.m.2:46, Michael S. Tsirkin wrote:
> > External email: Use caution opening links or attachments
> >
> >
> > On Fri, Jul 21, 2023 at 10:18:03PM +0200, Maxime Coquelin wrote:
> >>
> >>
> >> On 7/21/23 17:10, Michael S. Tsirkin wrote:
> >>> On Fri, Jul 21, 2023 at 04:58:04PM +0200, Maxime Coquelin wrote:
>
>
> On 7/21/23 16:45, Michael S. Tsirkin wrote:
> > On Fri, Jul 21, 2023 at 04:37:00PM +0200, Maxime Coquelin wrote:
> >>
> >>
> >> On 7/20/23 23:02, Michael S. Tsirkin wrote:
> >>> On Thu, Jul 20, 2023 at 01:26:20PM -0700, Shannon Nelson wrote:
> On 7/20/23 1:38 AM, Jason Wang wrote:
> >
> > Adding cond_resched() to the command waiting loop for a better
> > co-operation with the scheduler. This allows to give CPU a breath to
> > run other task(workqueue) instead of busy looping when preemption is
> > not allowed on a device whose CVQ might be slow.
> >
> > Signed-off-by: Jason Wang
>
> This still leaves hung processes, but at least it doesn't pin the
> CPU any
> more. Thanks.
> Reviewed-by: Shannon Nelson
>
> >>>
> >>> I'd like to see a full solution
> >>> 1- block until interrupt
> >>
> >> Would it make sense to also have a timeout?
> >> And when timeout expires, set FAILED bit in device status?
> >
> > virtio spec does not set any limits on the timing of vq
> > processing.
>
> Indeed, but I thought the driver could decide it is too long for it.
>
> The issue is we keep waiting with rtnl locked, it can quickly make the
> system unusable.
> >>>
> >>> if this is a problem we should find a way not to keep rtnl
> >>> locked indefinitely.
> >>
> >> From the tests I have done, I think it is. With OVS, a reconfiguration is
> >> performed when the VDUSE device is added, and when a MLX5 device is
> >> in the same bridge, it ends up doing an ioctl() that tries to take the
> >> rtnl lock. In this configuration, it is not possible to kill OVS because
> >> it is stuck trying to acquire rtnl lock for mlx5 that is held by virtio-
> >> net.
> >
> > So for sure, we can queue up the work and process it later.
> > The somewhat tricky part is limiting the memory consumption.
> >
> >
>
>
> Hi Jason
>
> Excuse me, is there any plan for when will v5 patch series be sent out?
> Will the v5 patches solve the problem of ctrlvq's infinite poll for
> buggy devices?
We agree to harden VDUSE and,
It would be hard if we try to solve it at the virtio-net level, see
the discussions before. It might require support from various layers
(e.g networking core etc).
We can use workqueue etc as a mitigation. If Michael is fine with
this, I can post v5.
Thanks
>
> Thanks
> Feng
>
> >>>
> >>> 2- still handle surprise removal correctly by waking in that case
> >>>
> >>>
> >>>
> > ---
> > drivers/net/virtio_net.c | 4 +++-
> > 1 file changed, 3 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
> > index 9f3b1d6ac33d..e7533f29b219 100644
> > --- a/drivers/net/virtio_net.c
> > +++ b/drivers/net/virtio_net.c
> > @@ -2314,8 +2314,10 @@ static bool virtnet_send_command(struct
> > virtnet_info *vi, u8 class, u8 cmd,
> > * into the hypervisor, so the request should be
> > handled immediately.
> > */
> > while (!virtqueue_get_buf(vi->cvq, ) &&
> > - !virtqueue_is_broken(vi->cvq))
> > + !virtqueue_is_broken(vi->cvq)) {
> > + cond_resched();
> > cpu_relax();
> > + }
> >
> > return vi->ctrl->status == VIRTIO_NET_OK;
> > }
> > --
> > 2.39.3
> >
> > ___
> > Virtualization mailing list
> > Virtualization@lists.linux-foundation.org
> > https://lists.linuxfoundation.org/mailman/listinfo/virtualization
> >>>
> >
> >>>
> >
> > ___
> > Virtualization mailing list
> > Virtualization@lists.linux-foundation.org
> > https://lists.linuxfoundation.org/mailman/listinfo/virtualization
>
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH net-next v4 2/2] virtio-net: add cond_resched() to the command waiting loop
On 2023-07-24 a.m.2:46, Michael S. Tsirkin wrote:
External email: Use caution opening links or attachments
On Fri, Jul 21, 2023 at 10:18:03PM +0200, Maxime Coquelin wrote:
On 7/21/23 17:10, Michael S. Tsirkin wrote:
On Fri, Jul 21, 2023 at 04:58:04PM +0200, Maxime Coquelin wrote:
On 7/21/23 16:45, Michael S. Tsirkin wrote:
On Fri, Jul 21, 2023 at 04:37:00PM +0200, Maxime Coquelin wrote:
On 7/20/23 23:02, Michael S. Tsirkin wrote:
On Thu, Jul 20, 2023 at 01:26:20PM -0700, Shannon Nelson wrote:
On 7/20/23 1:38 AM, Jason Wang wrote:
Adding cond_resched() to the command waiting loop for a better
co-operation with the scheduler. This allows to give CPU a breath to
run other task(workqueue) instead of busy looping when preemption is
not allowed on a device whose CVQ might be slow.
Signed-off-by: Jason Wang
This still leaves hung processes, but at least it doesn't pin the CPU any
more. Thanks.
Reviewed-by: Shannon Nelson
I'd like to see a full solution
1- block until interrupt
Would it make sense to also have a timeout?
And when timeout expires, set FAILED bit in device status?
virtio spec does not set any limits on the timing of vq
processing.
Indeed, but I thought the driver could decide it is too long for it.
The issue is we keep waiting with rtnl locked, it can quickly make the
system unusable.
if this is a problem we should find a way not to keep rtnl
locked indefinitely.
From the tests I have done, I think it is. With OVS, a reconfiguration is
performed when the VDUSE device is added, and when a MLX5 device is
in the same bridge, it ends up doing an ioctl() that tries to take the
rtnl lock. In this configuration, it is not possible to kill OVS because
it is stuck trying to acquire rtnl lock for mlx5 that is held by virtio-
net.
So for sure, we can queue up the work and process it later.
The somewhat tricky part is limiting the memory consumption.
Hi Jason
Excuse me, is there any plan for when will v5 patch series be sent out?
Will the v5 patches solve the problem of ctrlvq's infinite poll for
buggy devices?
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, ) &&
- !virtqueue_is_broken(vi->cvq))
+ !virtqueue_is_broken(vi->cvq)) {
+ cond_resched();
cpu_relax();
+ }
return vi->ctrl->status == VIRTIO_NET_OK;
}
--
2.39.3
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH net-next v4 2/2] virtio-net: add cond_resched() to the command waiting loop
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 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 > > > 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 > > > > > 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 > > > > > > > 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 > > > > > > > > > wrote: > > > > > > > > > > > > > > > > > > > > On Thu, Jul 27, 2023 at 5:46 PM Michael S. Tsirkin > > > > > > > > > > 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 >
Re: [PATCH net-next v4 2/2] virtio-net: add cond_resched() to the command waiting loop
On Fri, Aug 11, 2023 at 5:51 PM Michael S. Tsirkin 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 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 > > > > 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 > > > > > > 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 > > > > > > > > wrote: > > > > > > > > > > > > > > > > > > On Thu, Jul 27, 2023 at 5:46 PM Michael S. Tsirkin > > > > > > > > > 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 > > > > > > > > > > > > > > > > > > > > > > > >
Re: [PATCH net-next v4 2/2] virtio-net: add cond_resched() to the command waiting loop
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 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 > > > 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 > > > > > 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 > > > > > > > wrote: > > > > > > > > > > > > > > > > On Thu, Jul 27, 2023 at 5:46 PM Michael S. Tsirkin > > > > > > > > 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 > > > > > > > > > > > > > > > > > > > > > ___ Virtualization mailing list Virtualization@lists.linux-foundation.org
Re: [PATCH net-next v4 2/2] virtio-net: add cond_resched() to the command waiting loop
On Fri, Aug 11, 2023 at 5:21 PM Michael S. Tsirkin 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 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 > > > > 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 > > > > > > wrote: > > > > > > > > > > > > > > On Thu, Jul 27, 2023 at 5:46 PM Michael S. Tsirkin > > > > > > > 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 > > > > > > > > > > > > > > > > > ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH net-next v4 2/2] virtio-net: add cond_resched() to the command waiting loop
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 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 > > > 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 > > > > > wrote: > > > > > > > > > > > > On Thu, Jul 27, 2023 at 5:46 PM Michael S. Tsirkin > > > > > > 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 > > > > > > > > > > > > > ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH net-next v4 2/2] virtio-net: add cond_resched() to the command waiting loop
On Fri, Aug 11, 2023 at 1:42 PM Michael S. Tsirkin 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 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 wrote: > > > > > > > > > > On Thu, Jul 27, 2023 at 5:46 PM Michael S. Tsirkin > > > > > 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 > > > > > > > > > > ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH net-next v4 2/2] virtio-net: add cond_resched() to the command waiting loop
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 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 wrote: > > > > > > > > On Thu, Jul 27, 2023 at 5:46 PM Michael S. Tsirkin > > > > 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 > > > > > > > ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH net-next v4 2/2] virtio-net: add cond_resched() to the command waiting loop
On Fri, Aug 11, 2023 at 3:41 AM Michael S. Tsirkin 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 wrote: > > > > > > On Thu, Jul 27, 2023 at 5:46 PM Michael S. Tsirkin > > > 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 > > > > > ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH net-next v4 2/2] virtio-net: add cond_resched() to the command waiting loop
On Tue, Aug 08, 2023 at 10:30:56AM +0800, Jason Wang wrote: > On Mon, Jul 31, 2023 at 2:30 PM Jason Wang wrote: > > > > On Thu, Jul 27, 2023 at 5:46 PM Michael S. Tsirkin 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 > > > ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH net-next v4 2/2] virtio-net: add cond_resched() to the command waiting loop
On Mon, Jul 31, 2023 at 2:30 PM Jason Wang wrote: > > On Thu, Jul 27, 2023 at 5:46 PM Michael S. Tsirkin 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 > > ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH net-next v4 2/2] virtio-net: add cond_resched() to the command waiting loop
On Thu, Jul 27, 2023 at 5:46 PM Michael S. Tsirkin 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 > ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH net-next v4 2/2] virtio-net: add cond_resched() to the command waiting loop
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 ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH net-next v4 2/2] virtio-net: add cond_resched() to the command waiting loop
On Thu, Jul 27, 2023 at 2:10 PM Michael S. Tsirkin 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 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 > > > > 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 > > > > > > 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 > > > > > > > > wrote: > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > On 7/21/23 17:10, Michael S. Tsirkin wrote: > > > > > > > > > > On Fri, Jul 21, 2023 at 04:58:04PM +0200, Maxime Coquelin > > > > > > > > > > wrote: > > > > > > > > > >> > > > > > > > > > >> > > > > > > > > > >> On 7/21/23 16:45, Michael S. Tsirkin wrote: > > > > > > > > > >>> On Fri, Jul 21, 2023 at 04:37:00PM +0200, Maxime Coquelin > > > > > > > > > >>> wrote: > > > > > > > > > > > > > > > > > > > > > > > > > > > On 7/20/23 23:02, Michael S. Tsirkin wrote: > > > > > > > > > > On Thu, Jul 20, 2023 at 01:26:20PM -0700, Shannon > > > > > > > > > > Nelson wrote: > > > > > > > > > >> On 7/20/23 1:38 AM, Jason Wang wrote: > > > > > > > > > >>> > > > > > > > > > >>> Adding cond_resched() to the command waiting loop for > > > > > > > > > >>> a better > > > > > > > > > >>> co-operation with the scheduler. This allows to give > > > > > > > > > >>> CPU a breath to > > > > > > > > > >>> run other task(workqueue) instead of busy looping > > > > > > > > > >>> when preemption is > > > > > > > > > >>> not allowed on a device whose CVQ might be slow. > > > > > > > > > >>> > > > > > > > > > >>> Signed-off-by: Jason Wang > > > > > > > > > >> > > > > > > > > > >> This still leaves hung processes, but at least it > > > > > > > > > >> doesn't pin the CPU any > > > > > > > > > >> more. Thanks. > > > > > > > > > >> Reviewed-by: Shannon Nelson > > > > > > > > > >> > > > > > > > > > > > > > > > > > > > > I'd like to see a full solution > > > > > > > > > > 1- block until interrupt > > > > > > > > > > > > > > > > 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 > > > >
Re: [PATCH net-next v4 2/2] virtio-net: add cond_resched() to the command waiting loop
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 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 > > > 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 > > > > > 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 > > > > > > > wrote: > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > On 7/21/23 17:10, Michael S. Tsirkin wrote: > > > > > > > > > On Fri, Jul 21, 2023 at 04:58:04PM +0200, Maxime Coquelin > > > > > > > > > wrote: > > > > > > > > >> > > > > > > > > >> > > > > > > > > >> On 7/21/23 16:45, Michael S. Tsirkin wrote: > > > > > > > > >>> On Fri, Jul 21, 2023 at 04:37:00PM +0200, Maxime Coquelin > > > > > > > > >>> wrote: > > > > > > > > > > > > > > > > > > > > > > > > On 7/20/23 23:02, Michael S. Tsirkin wrote: > > > > > > > > > On Thu, Jul 20, 2023 at 01:26:20PM -0700, Shannon Nelson > > > > > > > > > wrote: > > > > > > > > >> On 7/20/23 1:38 AM, Jason Wang wrote: > > > > > > > > >>> > > > > > > > > >>> Adding cond_resched() to the command waiting loop for a > > > > > > > > >>> better > > > > > > > > >>> co-operation with the scheduler. This allows to give > > > > > > > > >>> CPU a breath to > > > > > > > > >>> run other task(workqueue) instead of busy looping when > > > > > > > > >>> preemption is > > > > > > > > >>> not allowed on a device whose CVQ might be slow. > > > > > > > > >>> > > > > > > > > >>> Signed-off-by: Jason Wang > > > > > > > > >> > > > > > > > > >> This still leaves hung processes, but at least it > > > > > > > > >> doesn't pin the CPU any > > > > > > > > >> more. Thanks. > > > > > > > > >> Reviewed-by: Shannon Nelson > > > > > > > > >> > > > > > > > > > > > > > > > > > > I'd like to see a full solution > > > > > > > > > 1- block until interrupt > > > > > > > > > > > > > > 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. > >
Re: [PATCH net-next v4 2/2] virtio-net: add cond_resched() to the command waiting loop
On Wed, Jul 26, 2023 at 7:38 PM Michael S. Tsirkin 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 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 > > > > 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 > > > > > > wrote: > > > > > > > > > > > > > > > > > > > > > > > > > > > > On 7/21/23 17:10, Michael S. Tsirkin wrote: > > > > > > > > On Fri, Jul 21, 2023 at 04:58:04PM +0200, Maxime Coquelin wrote: > > > > > > > >> > > > > > > > >> > > > > > > > >> On 7/21/23 16:45, Michael S. Tsirkin wrote: > > > > > > > >>> On Fri, Jul 21, 2023 at 04:37:00PM +0200, Maxime Coquelin > > > > > > > >>> wrote: > > > > > > > > > > > > > > > > > > > > > On 7/20/23 23:02, Michael S. Tsirkin wrote: > > > > > > > > On Thu, Jul 20, 2023 at 01:26:20PM -0700, Shannon Nelson > > > > > > > > wrote: > > > > > > > >> On 7/20/23 1:38 AM, Jason Wang wrote: > > > > > > > >>> > > > > > > > >>> Adding cond_resched() to the command waiting loop for a > > > > > > > >>> better > > > > > > > >>> co-operation with the scheduler. This allows to give CPU > > > > > > > >>> a breath to > > > > > > > >>> run other task(workqueue) instead of busy looping when > > > > > > > >>> preemption is > > > > > > > >>> not allowed on a device whose CVQ might be slow. > > > > > > > >>> > > > > > > > >>> Signed-off-by: Jason Wang > > > > > > > >> > > > > > > > >> This still leaves hung processes, but at least it doesn't > > > > > > > >> pin the CPU any > > > > > > > >> more. Thanks. > > > > > > > >> Reviewed-by: Shannon Nelson > > > > > > > >> > > > > > > > > > > > > > > > > I'd like to see a full solution > > > > > > > > 1- block until interrupt > > > > > > > > > > > > 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 >
Re: [PATCH net-next v4 2/2] virtio-net: add cond_resched() to the command waiting loop
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 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 > > > 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 > > > > > wrote: > > > > > > > > > > > > > > > > > > > > > > > > On 7/21/23 17:10, Michael S. Tsirkin wrote: > > > > > > > On Fri, Jul 21, 2023 at 04:58:04PM +0200, Maxime Coquelin wrote: > > > > > > >> > > > > > > >> > > > > > > >> On 7/21/23 16:45, Michael S. Tsirkin wrote: > > > > > > >>> On Fri, Jul 21, 2023 at 04:37:00PM +0200, Maxime Coquelin wrote: > > > > > > > > > > > > > > > > > > On 7/20/23 23:02, Michael S. Tsirkin wrote: > > > > > > > On Thu, Jul 20, 2023 at 01:26:20PM -0700, Shannon Nelson > > > > > > > wrote: > > > > > > >> On 7/20/23 1:38 AM, Jason Wang wrote: > > > > > > >>> > > > > > > >>> Adding cond_resched() to the command waiting loop for a > > > > > > >>> better > > > > > > >>> co-operation with the scheduler. This allows to give CPU a > > > > > > >>> breath to > > > > > > >>> run other task(workqueue) instead of busy looping when > > > > > > >>> preemption is > > > > > > >>> not allowed on a device whose CVQ might be slow. > > > > > > >>> > > > > > > >>> Signed-off-by: Jason Wang > > > > > > >> > > > > > > >> This still leaves hung processes, but at least it doesn't > > > > > > >> pin the CPU any > > > > > > >> more. Thanks. > > > > > > >> Reviewed-by: Shannon Nelson > > > > > > >> > > > > > > > > > > > > > > I'd like to see a full solution > > > > > > > 1- block until interrupt > > > > > > > > > > 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 > > > >
Re: [PATCH net-next v4 2/2] virtio-net: add cond_resched() to the command waiting loop
On Tue, Jul 25, 2023 at 3:36 PM Michael S. Tsirkin 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 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 > > > > wrote: > > > > > > > > > > > > > > > > > > > > On 7/21/23 17:10, Michael S. Tsirkin wrote: > > > > > > On Fri, Jul 21, 2023 at 04:58:04PM +0200, Maxime Coquelin wrote: > > > > > >> > > > > > >> > > > > > >> On 7/21/23 16:45, Michael S. Tsirkin wrote: > > > > > >>> On Fri, Jul 21, 2023 at 04:37:00PM +0200, Maxime Coquelin wrote: > > > > > > > > > > > > > > > On 7/20/23 23:02, Michael S. Tsirkin wrote: > > > > > > On Thu, Jul 20, 2023 at 01:26:20PM -0700, Shannon Nelson wrote: > > > > > >> On 7/20/23 1:38 AM, Jason Wang wrote: > > > > > >>> > > > > > >>> Adding cond_resched() to the command waiting loop for a better > > > > > >>> co-operation with the scheduler. This allows to give CPU a > > > > > >>> breath to > > > > > >>> run other task(workqueue) instead of busy looping when > > > > > >>> preemption is > > > > > >>> not allowed on a device whose CVQ might be slow. > > > > > >>> > > > > > >>> Signed-off-by: Jason Wang > > > > > >> > > > > > >> This still leaves hung processes, but at least it doesn't pin > > > > > >> the CPU any > > > > > >> more. Thanks. > > > > > >> Reviewed-by: Shannon Nelson > > > > > >> > > > > > > > > > > > > I'd like to see a full solution > > > > > > 1- block until interrupt > > > > > > > > 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/6026e801-6fda-fee9-a69b-d06a80368...@redhat.com/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 > > > > > >>>
Re: [PATCH net-next v4 2/2] virtio-net: add cond_resched() to the command waiting loop
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 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 > > > wrote: > > > > > > > > > > > > > > > > On 7/21/23 17:10, Michael S. Tsirkin wrote: > > > > > On Fri, Jul 21, 2023 at 04:58:04PM +0200, Maxime Coquelin wrote: > > > > >> > > > > >> > > > > >> On 7/21/23 16:45, Michael S. Tsirkin wrote: > > > > >>> On Fri, Jul 21, 2023 at 04:37:00PM +0200, Maxime Coquelin wrote: > > > > > > > > > > > > On 7/20/23 23:02, Michael S. Tsirkin wrote: > > > > > On Thu, Jul 20, 2023 at 01:26:20PM -0700, Shannon Nelson wrote: > > > > >> On 7/20/23 1:38 AM, Jason Wang wrote: > > > > >>> > > > > >>> Adding cond_resched() to the command waiting loop for a better > > > > >>> co-operation with the scheduler. This allows to give CPU a > > > > >>> breath to > > > > >>> run other task(workqueue) instead of busy looping when > > > > >>> preemption is > > > > >>> not allowed on a device whose CVQ might be slow. > > > > >>> > > > > >>> Signed-off-by: Jason Wang > > > > >> > > > > >> This still leaves hung processes, but at least it doesn't pin > > > > >> the CPU any > > > > >> more. Thanks. > > > > >> Reviewed-by: Shannon Nelson > > > > >> > > > > > > > > > > I'd like to see a full solution > > > > > 1- block until interrupt > > > > > > 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/6026e801-6fda-fee9-a69b-d06a80368...@redhat.com/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, ) && > > > > >>>
Re: [PATCH net-next v4 2/2] virtio-net: add cond_resched() to the command waiting loop
On Mon, Jul 24, 2023 at 3:17 PM Michael S. Tsirkin 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
> > wrote:
> > >
> > >
> > >
> > > On 7/21/23 17:10, Michael S. Tsirkin wrote:
> > > > On Fri, Jul 21, 2023 at 04:58:04PM +0200, Maxime Coquelin wrote:
> > > >>
> > > >>
> > > >> On 7/21/23 16:45, Michael S. Tsirkin wrote:
> > > >>> On Fri, Jul 21, 2023 at 04:37:00PM +0200, Maxime Coquelin wrote:
> > >
> > >
> > > On 7/20/23 23:02, Michael S. Tsirkin wrote:
> > > > On Thu, Jul 20, 2023 at 01:26:20PM -0700, Shannon Nelson wrote:
> > > >> On 7/20/23 1:38 AM, Jason Wang wrote:
> > > >>>
> > > >>> Adding cond_resched() to the command waiting loop for a better
> > > >>> co-operation with the scheduler. This allows to give CPU a breath
> > > >>> to
> > > >>> run other task(workqueue) instead of busy looping when preemption
> > > >>> is
> > > >>> not allowed on a device whose CVQ might be slow.
> > > >>>
> > > >>> Signed-off-by: Jason Wang
> > > >>
> > > >> This still leaves hung processes, but at least it doesn't pin the
> > > >> CPU any
> > > >> more. Thanks.
> > > >> Reviewed-by: Shannon Nelson
> > > >>
> > > >
> > > > I'd like to see a full solution
> > > > 1- block until interrupt
> >
> > 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/6026e801-6fda-fee9-a69b-d06a80368...@redhat.com/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, ) &&
> > > >>> - !virtqueue_is_broken(vi->cvq))
> > > >>> + !virtqueue_is_broken(vi->cvq)) {
> > > >>> + cond_resched();
> > > >>> cpu_relax();
> > > >>> + }
> > > >>>
> > > >>> return vi->ctrl->status == VIRTIO_NET_OK;
> > > >>> }
> > > >>> --
> > > >>> 2.39.3
> > > >>>
> > >
Re: [PATCH net-next v4 2/2] virtio-net: add cond_resched() to the command waiting loop
On Mon, Jul 24, 2023 at 3:18 PM Michael S. Tsirkin 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 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
> > > > > > > > > >
> > > > > > > > > > This still leaves hung processes, but at least it doesn't
> > > > > > > > > > pin the CPU any
> > > > > > > > > > more. Thanks.
> > > > > > > > > > Reviewed-by: Shannon Nelson
> > > > > > > > > >
> > > > > > > > >
> > > > > > > > > I'd like to see a full solution
> > > > > > > > > 1- block until interrupt
> > > > > > > >
> > > > > > > > Would it make sense to also have a timeout?
> > > > > > > > And when timeout expires, set FAILED bit in device status?
> > > > > > >
> > > > > > > virtio spec does not set any limits on the timing of vq
> > > > > > > processing.
> > > > > >
> > > > > > Indeed, but I thought the driver could decide it is too long for it.
> > > > > >
> > > > > > The issue is we keep waiting with rtnl locked, it can quickly make
> > > > > > the
> > > > > > system unusable.
> > > > >
> > > > > if this is a problem we should find a way not to keep rtnl
> > > > > locked indefinitely.
> > > >
> > > > From the tests I have done, I think it is. With OVS, a reconfiguration
> > > > is
> > > > performed when the VDUSE device is added, and when a MLX5 device is
> > > > in the same bridge, it ends up doing an ioctl() that tries to take the
> > > > rtnl lock. In this configuration, it is not possible to kill OVS because
> > > > it is stuck trying to acquire rtnl lock for mlx5 that is held by virtio-
> > > > net.
> > >
> > > So for sure, we can queue up the work and process it later.
> > > The somewhat tricky part is limiting the memory consumption.
> >
> > 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, ) &&
> > > > > > > > > > > - !virtqueue_is_broken(vi->cvq))
> > > > > > > > > > > + !virtqueue_is_broken(vi->cvq)) {
> > > > > > > > > > > + cond_resched();
> > > > > > > > > > > cpu_relax();
> > > > > > > > > > > + }
> > > > > > > > > > >
> > > > > > > > > > > return vi->ctrl->status == VIRTIO_NET_OK;
> > > > > > > > > > > }
> > > > > > > > > > > --
> > > > > > > > > > > 2.39.3
> > > > > > > > > > >
> > > > > > > > > > > ___
> > > > > > > > > > > Virtualization mailing list
> > > > > > > > > > > Virtualization@lists.linux-foundation.org
> > > > > > > > > > > https://lists.linuxfoundation.org/mailman/listinfo/virtualization
> > > > > > > > >
> > > > > > >
> > > > >
> > >
>
Re: [PATCH net-next v4 2/2] virtio-net: add cond_resched() to the command waiting loop
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 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
> > > > > > > > >
> > > > > > > > > This still leaves hung processes, but at least it doesn't pin
> > > > > > > > > the CPU any
> > > > > > > > > more. Thanks.
> > > > > > > > > Reviewed-by: Shannon Nelson
> > > > > > > > >
> > > > > > > >
> > > > > > > > I'd like to see a full solution
> > > > > > > > 1- block until interrupt
> > > > > > >
> > > > > > > Would it make sense to also have a timeout?
> > > > > > > And when timeout expires, set FAILED bit in device status?
> > > > > >
> > > > > > virtio spec does not set any limits on the timing of vq
> > > > > > processing.
> > > > >
> > > > > Indeed, but I thought the driver could decide it is too long for it.
> > > > >
> > > > > The issue is we keep waiting with rtnl locked, it can quickly make the
> > > > > system unusable.
> > > >
> > > > if this is a problem we should find a way not to keep rtnl
> > > > locked indefinitely.
> > >
> > > From the tests I have done, I think it is. With OVS, a reconfiguration is
> > > performed when the VDUSE device is added, and when a MLX5 device is
> > > in the same bridge, it ends up doing an ioctl() that tries to take the
> > > rtnl lock. In this configuration, it is not possible to kill OVS because
> > > it is stuck trying to acquire rtnl lock for mlx5 that is held by virtio-
> > > net.
> >
> > So for sure, we can queue up the work and process it later.
> > The somewhat tricky part is limiting the memory consumption.
>
> 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, ) &&
> > > > > > > > > > - !virtqueue_is_broken(vi->cvq))
> > > > > > > > > > + !virtqueue_is_broken(vi->cvq)) {
> > > > > > > > > > + cond_resched();
> > > > > > > > > > cpu_relax();
> > > > > > > > > > + }
> > > > > > > > > >
> > > > > > > > > > return vi->ctrl->status == VIRTIO_NET_OK;
> > > > > > > > > > }
> > > > > > > > > > --
> > > > > > > > > > 2.39.3
> > > > > > > > > >
> > > > > > > > > > ___
> > > > > > > > > > Virtualization mailing list
> > > > > > > > > > Virtualization@lists.linux-foundation.org
> > > > > > > > > > https://lists.linuxfoundation.org/mailman/listinfo/virtualization
> > > > > > > >
> > > > > >
> > > >
> >
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH net-next v4 2/2] virtio-net: add cond_resched() to the command waiting loop
On Mon, Jul 24, 2023 at 02:52:05PM +0800, Jason Wang wrote:
> On Sat, Jul 22, 2023 at 4:18 AM Maxime Coquelin
> wrote:
> >
> >
> >
> > On 7/21/23 17:10, Michael S. Tsirkin wrote:
> > > On Fri, Jul 21, 2023 at 04:58:04PM +0200, Maxime Coquelin wrote:
> > >>
> > >>
> > >> On 7/21/23 16:45, Michael S. Tsirkin wrote:
> > >>> On Fri, Jul 21, 2023 at 04:37:00PM +0200, Maxime Coquelin wrote:
> >
> >
> > On 7/20/23 23:02, Michael S. Tsirkin wrote:
> > > On Thu, Jul 20, 2023 at 01:26:20PM -0700, Shannon Nelson wrote:
> > >> On 7/20/23 1:38 AM, Jason Wang wrote:
> > >>>
> > >>> Adding cond_resched() to the command waiting loop for a better
> > >>> co-operation with the scheduler. This allows to give CPU a breath to
> > >>> run other task(workqueue) instead of busy looping when preemption is
> > >>> not allowed on a device whose CVQ might be slow.
> > >>>
> > >>> Signed-off-by: Jason Wang
> > >>
> > >> This still leaves hung processes, but at least it doesn't pin the
> > >> CPU any
> > >> more. Thanks.
> > >> Reviewed-by: Shannon Nelson
> > >>
> > >
> > > I'd like to see a full solution
> > > 1- block until interrupt
>
> 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/6026e801-6fda-fee9-a69b-d06a80368...@redhat.com/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, ) &&
> > >>> - !virtqueue_is_broken(vi->cvq))
> > >>> + !virtqueue_is_broken(vi->cvq)) {
> > >>> + cond_resched();
> > >>> cpu_relax();
> > >>> + }
> > >>>
> > >>> return vi->ctrl->status == VIRTIO_NET_OK;
> > >>> }
> > >>> --
> > >>> 2.39.3
> > >>>
> > >>> ___
> > >>> Virtualization mailing list
> > >>> Virtualization@lists.linux-foundation.org
> > >>> https://lists.linuxfoundation.org/mailman/listinfo/virtualization
> > >
> > >>>
> > >
> >
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH net-next v4 2/2] virtio-net: add cond_resched() to the command waiting loop
On Mon, Jul 24, 2023 at 2:46 PM Michael S. Tsirkin 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
> > > > > > > >
> > > > > > > > This still leaves hung processes, but at least it doesn't pin
> > > > > > > > the CPU any
> > > > > > > > more. Thanks.
> > > > > > > > Reviewed-by: Shannon Nelson
> > > > > > > >
> > > > > > >
> > > > > > > I'd like to see a full solution
> > > > > > > 1- block until interrupt
> > > > > >
> > > > > > Would it make sense to also have a timeout?
> > > > > > And when timeout expires, set FAILED bit in device status?
> > > > >
> > > > > virtio spec does not set any limits on the timing of vq
> > > > > processing.
> > > >
> > > > Indeed, but I thought the driver could decide it is too long for it.
> > > >
> > > > The issue is we keep waiting with rtnl locked, it can quickly make the
> > > > system unusable.
> > >
> > > if this is a problem we should find a way not to keep rtnl
> > > locked indefinitely.
> >
> > From the tests I have done, I think it is. With OVS, a reconfiguration is
> > performed when the VDUSE device is added, and when a MLX5 device is
> > in the same bridge, it ends up doing an ioctl() that tries to take the
> > rtnl lock. In this configuration, it is not possible to kill OVS because
> > it is stuck trying to acquire rtnl lock for mlx5 that is held by virtio-
> > net.
>
> So for sure, we can queue up the work and process it later.
> The somewhat tricky part is limiting the memory consumption.
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, ) &&
> > > > > > > > > - !virtqueue_is_broken(vi->cvq))
> > > > > > > > > + !virtqueue_is_broken(vi->cvq)) {
> > > > > > > > > + cond_resched();
> > > > > > > > > cpu_relax();
> > > > > > > > > + }
> > > > > > > > >
> > > > > > > > > return vi->ctrl->status == VIRTIO_NET_OK;
> > > > > > > > > }
> > > > > > > > > --
> > > > > > > > > 2.39.3
> > > > > > > > >
> > > > > > > > > ___
> > > > > > > > > Virtualization mailing list
> > > > > > > > > Virtualization@lists.linux-foundation.org
> > > > > > > > > https://lists.linuxfoundation.org/mailman/listinfo/virtualization
> > > > > > >
> > > > >
> > >
>
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH net-next v4 2/2] virtio-net: add cond_resched() to the command waiting loop
On Sat, Jul 22, 2023 at 4:18 AM Maxime Coquelin
wrote:
>
>
>
> On 7/21/23 17:10, Michael S. Tsirkin wrote:
> > On Fri, Jul 21, 2023 at 04:58:04PM +0200, Maxime Coquelin wrote:
> >>
> >>
> >> On 7/21/23 16:45, Michael S. Tsirkin wrote:
> >>> On Fri, Jul 21, 2023 at 04:37:00PM +0200, Maxime Coquelin wrote:
>
>
> On 7/20/23 23:02, Michael S. Tsirkin wrote:
> > On Thu, Jul 20, 2023 at 01:26:20PM -0700, Shannon Nelson wrote:
> >> On 7/20/23 1:38 AM, Jason Wang wrote:
> >>>
> >>> Adding cond_resched() to the command waiting loop for a better
> >>> co-operation with the scheduler. This allows to give CPU a breath to
> >>> run other task(workqueue) instead of busy looping when preemption is
> >>> not allowed on a device whose CVQ might be slow.
> >>>
> >>> Signed-off-by: Jason Wang
> >>
> >> This still leaves hung processes, but at least it doesn't pin the CPU
> >> any
> >> more. Thanks.
> >> Reviewed-by: Shannon Nelson
> >>
> >
> > I'd like to see a full solution
> > 1- block until interrupt
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/6026e801-6fda-fee9-a69b-d06a80368...@redhat.com/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, ) &&
> >>> - !virtqueue_is_broken(vi->cvq))
> >>> + !virtqueue_is_broken(vi->cvq)) {
> >>> + cond_resched();
> >>> cpu_relax();
> >>> + }
> >>>
> >>> return vi->ctrl->status == VIRTIO_NET_OK;
> >>> }
> >>> --
> >>> 2.39.3
> >>>
> >>> ___
> >>> Virtualization mailing list
> >>> Virtualization@lists.linux-foundation.org
> >>> https://lists.linuxfoundation.org/mailman/listinfo/virtualization
> >
> >>>
> >
>
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH net-next v4 2/2] virtio-net: add cond_resched() to the command waiting loop
On Fri, Jul 21, 2023 at 10:18:03PM +0200, Maxime Coquelin wrote:
>
>
> On 7/21/23 17:10, Michael S. Tsirkin wrote:
> > On Fri, Jul 21, 2023 at 04:58:04PM +0200, Maxime Coquelin wrote:
> > >
> > >
> > > On 7/21/23 16:45, Michael S. Tsirkin wrote:
> > > > On Fri, Jul 21, 2023 at 04:37:00PM +0200, Maxime Coquelin wrote:
> > > > >
> > > > >
> > > > > On 7/20/23 23:02, Michael S. Tsirkin wrote:
> > > > > > On Thu, Jul 20, 2023 at 01:26:20PM -0700, Shannon Nelson wrote:
> > > > > > > On 7/20/23 1:38 AM, Jason Wang wrote:
> > > > > > > >
> > > > > > > > Adding cond_resched() to the command waiting loop for a better
> > > > > > > > co-operation with the scheduler. This allows to give CPU a
> > > > > > > > breath to
> > > > > > > > run other task(workqueue) instead of busy looping when
> > > > > > > > preemption is
> > > > > > > > not allowed on a device whose CVQ might be slow.
> > > > > > > >
> > > > > > > > Signed-off-by: Jason Wang
> > > > > > >
> > > > > > > This still leaves hung processes, but at least it doesn't pin the
> > > > > > > CPU any
> > > > > > > more. Thanks.
> > > > > > > Reviewed-by: Shannon Nelson
> > > > > > >
> > > > > >
> > > > > > I'd like to see a full solution
> > > > > > 1- block until interrupt
> > > > >
> > > > > Would it make sense to also have a timeout?
> > > > > And when timeout expires, set FAILED bit in device status?
> > > >
> > > > virtio spec does not set any limits on the timing of vq
> > > > processing.
> > >
> > > Indeed, but I thought the driver could decide it is too long for it.
> > >
> > > The issue is we keep waiting with rtnl locked, it can quickly make the
> > > system unusable.
> >
> > if this is a problem we should find a way not to keep rtnl
> > locked indefinitely.
>
> From the tests I have done, I think it is. With OVS, a reconfiguration is
> performed when the VDUSE device is added, and when a MLX5 device is
> in the same bridge, it ends up doing an ioctl() that tries to take the
> rtnl lock. In this configuration, it is not possible to kill OVS because
> it is stuck trying to acquire rtnl lock for mlx5 that is held by virtio-
> net.
So for sure, we can queue up the work and process it later.
The somewhat tricky part is limiting the memory consumption.
> >
> > > > > > 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, ) &&
> > > > > > > > - !virtqueue_is_broken(vi->cvq))
> > > > > > > > + !virtqueue_is_broken(vi->cvq)) {
> > > > > > > > + cond_resched();
> > > > > > > > cpu_relax();
> > > > > > > > + }
> > > > > > > >
> > > > > > > > return vi->ctrl->status == VIRTIO_NET_OK;
> > > > > > > > }
> > > > > > > > --
> > > > > > > > 2.39.3
> > > > > > > >
> > > > > > > > ___
> > > > > > > > Virtualization mailing list
> > > > > > > > Virtualization@lists.linux-foundation.org
> > > > > > > > https://lists.linuxfoundation.org/mailman/listinfo/virtualization
> > > > > >
> > > >
> >
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH net-next v4 2/2] virtio-net: add cond_resched() to the command waiting loop
On 7/21/23 17:10, Michael S. Tsirkin wrote:
On Fri, Jul 21, 2023 at 04:58:04PM +0200, Maxime Coquelin wrote:
On 7/21/23 16:45, Michael S. Tsirkin wrote:
On Fri, Jul 21, 2023 at 04:37:00PM +0200, Maxime Coquelin wrote:
On 7/20/23 23:02, Michael S. Tsirkin wrote:
On Thu, Jul 20, 2023 at 01:26:20PM -0700, Shannon Nelson wrote:
On 7/20/23 1:38 AM, Jason Wang wrote:
Adding cond_resched() to the command waiting loop for a better
co-operation with the scheduler. This allows to give CPU a breath to
run other task(workqueue) instead of busy looping when preemption is
not allowed on a device whose CVQ might be slow.
Signed-off-by: Jason Wang
This still leaves hung processes, but at least it doesn't pin the CPU any
more. Thanks.
Reviewed-by: Shannon Nelson
I'd like to see a full solution
1- block until interrupt
Would it make sense to also have a timeout?
And when timeout expires, set FAILED bit in device status?
virtio spec does not set any limits on the timing of vq
processing.
Indeed, but I thought the driver could decide it is too long for it.
The issue is we keep waiting with rtnl locked, it can quickly make the
system unusable.
if this is a problem we should find a way not to keep rtnl
locked indefinitely.
From the tests I have done, I think it is. With OVS, a reconfiguration
is performed when the VDUSE device is added, and when a MLX5 device is
in the same bridge, it ends up doing an ioctl() that tries to take the
rtnl lock. In this configuration, it is not possible to kill OVS because
it is stuck trying to acquire rtnl lock for mlx5 that is held by virtio-
net.
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, ) &&
- !virtqueue_is_broken(vi->cvq))
+ !virtqueue_is_broken(vi->cvq)) {
+ cond_resched();
cpu_relax();
+ }
return vi->ctrl->status == VIRTIO_NET_OK;
}
--
2.39.3
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH net-next v4 2/2] virtio-net: add cond_resched() to the command waiting loop
On Fri, Jul 21, 2023 at 04:58:04PM +0200, Maxime Coquelin wrote:
>
>
> On 7/21/23 16:45, Michael S. Tsirkin wrote:
> > On Fri, Jul 21, 2023 at 04:37:00PM +0200, Maxime Coquelin wrote:
> > >
> > >
> > > On 7/20/23 23:02, Michael S. Tsirkin wrote:
> > > > On Thu, Jul 20, 2023 at 01:26:20PM -0700, Shannon Nelson wrote:
> > > > > On 7/20/23 1:38 AM, Jason Wang wrote:
> > > > > >
> > > > > > Adding cond_resched() to the command waiting loop for a better
> > > > > > co-operation with the scheduler. This allows to give CPU a breath to
> > > > > > run other task(workqueue) instead of busy looping when preemption is
> > > > > > not allowed on a device whose CVQ might be slow.
> > > > > >
> > > > > > Signed-off-by: Jason Wang
> > > > >
> > > > > This still leaves hung processes, but at least it doesn't pin the CPU
> > > > > any
> > > > > more. Thanks.
> > > > > Reviewed-by: Shannon Nelson
> > > > >
> > > >
> > > > I'd like to see a full solution
> > > > 1- block until interrupt
> > >
> > > Would it make sense to also have a timeout?
> > > And when timeout expires, set FAILED bit in device status?
> >
> > virtio spec does not set any limits on the timing of vq
> > processing.
>
> Indeed, but I thought the driver could decide it is too long for it.
>
> The issue is we keep waiting with rtnl locked, it can quickly make the
> system unusable.
if this is a problem we should find a way not to keep rtnl
locked indefinitely.
> > > > 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, ) &&
> > > > > > - !virtqueue_is_broken(vi->cvq))
> > > > > > + !virtqueue_is_broken(vi->cvq)) {
> > > > > > + cond_resched();
> > > > > >cpu_relax();
> > > > > > + }
> > > > > >
> > > > > >return vi->ctrl->status == VIRTIO_NET_OK;
> > > > > > }
> > > > > > --
> > > > > > 2.39.3
> > > > > >
> > > > > > ___
> > > > > > Virtualization mailing list
> > > > > > Virtualization@lists.linux-foundation.org
> > > > > > https://lists.linuxfoundation.org/mailman/listinfo/virtualization
> > > >
> >
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH net-next v4 2/2] virtio-net: add cond_resched() to the command waiting loop
On 7/21/23 16:45, Michael S. Tsirkin wrote:
On Fri, Jul 21, 2023 at 04:37:00PM +0200, Maxime Coquelin wrote:
On 7/20/23 23:02, Michael S. Tsirkin wrote:
On Thu, Jul 20, 2023 at 01:26:20PM -0700, Shannon Nelson wrote:
On 7/20/23 1:38 AM, Jason Wang wrote:
Adding cond_resched() to the command waiting loop for a better
co-operation with the scheduler. This allows to give CPU a breath to
run other task(workqueue) instead of busy looping when preemption is
not allowed on a device whose CVQ might be slow.
Signed-off-by: Jason Wang
This still leaves hung processes, but at least it doesn't pin the CPU any
more. Thanks.
Reviewed-by: Shannon Nelson
I'd like to see a full solution
1- block until interrupt
Would it make sense to also have a timeout?
And when timeout expires, set FAILED bit in device status?
virtio spec does not set any limits on the timing of vq
processing.
Indeed, but I thought the driver could decide it is too long for it.
The issue is we keep waiting with rtnl locked, it can quickly make the
system unusable.
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, ) &&
- !virtqueue_is_broken(vi->cvq))
+ !virtqueue_is_broken(vi->cvq)) {
+ cond_resched();
cpu_relax();
+ }
return vi->ctrl->status == VIRTIO_NET_OK;
}
--
2.39.3
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH net-next v4 2/2] virtio-net: add cond_resched() to the command waiting loop
On Fri, Jul 21, 2023 at 04:37:00PM +0200, Maxime Coquelin wrote:
>
>
> On 7/20/23 23:02, Michael S. Tsirkin wrote:
> > On Thu, Jul 20, 2023 at 01:26:20PM -0700, Shannon Nelson wrote:
> > > On 7/20/23 1:38 AM, Jason Wang wrote:
> > > >
> > > > Adding cond_resched() to the command waiting loop for a better
> > > > co-operation with the scheduler. This allows to give CPU a breath to
> > > > run other task(workqueue) instead of busy looping when preemption is
> > > > not allowed on a device whose CVQ might be slow.
> > > >
> > > > Signed-off-by: Jason Wang
> > >
> > > This still leaves hung processes, but at least it doesn't pin the CPU any
> > > more. Thanks.
> > > Reviewed-by: Shannon Nelson
> > >
> >
> > I'd like to see a full solution
> > 1- block until interrupt
>
> Would it make sense to also have a timeout?
> And when timeout expires, set FAILED bit in device status?
virtio spec does not set any limits on the timing of vq
processing.
> > 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, ) &&
> > > > - !virtqueue_is_broken(vi->cvq))
> > > > + !virtqueue_is_broken(vi->cvq)) {
> > > > + cond_resched();
> > > > cpu_relax();
> > > > + }
> > > >
> > > > return vi->ctrl->status == VIRTIO_NET_OK;
> > > >}
> > > > --
> > > > 2.39.3
> > > >
> > > > ___
> > > > Virtualization mailing list
> > > > Virtualization@lists.linux-foundation.org
> > > > https://lists.linuxfoundation.org/mailman/listinfo/virtualization
> >
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH net-next v4 2/2] virtio-net: add cond_resched() to the command waiting loop
On 7/20/23 23:02, Michael S. Tsirkin wrote:
On Thu, Jul 20, 2023 at 01:26:20PM -0700, Shannon Nelson wrote:
On 7/20/23 1:38 AM, Jason Wang wrote:
Adding cond_resched() to the command waiting loop for a better
co-operation with the scheduler. This allows to give CPU a breath to
run other task(workqueue) instead of busy looping when preemption is
not allowed on a device whose CVQ might be slow.
Signed-off-by: Jason Wang
This still leaves hung processes, but at least it doesn't pin the CPU any
more. Thanks.
Reviewed-by: Shannon Nelson
I'd like to see a full solution
1- block until interrupt
Would it make sense to also have a timeout?
And when timeout expires, set FAILED bit in device status?
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, ) &&
- !virtqueue_is_broken(vi->cvq))
+ !virtqueue_is_broken(vi->cvq)) {
+ cond_resched();
cpu_relax();
+ }
return vi->ctrl->status == VIRTIO_NET_OK;
}
--
2.39.3
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH net-next v4 2/2] virtio-net: add cond_resched() to the command waiting loop
On Thu, Jul 20, 2023 at 01:26:20PM -0700, Shannon Nelson wrote:
> On 7/20/23 1:38 AM, Jason Wang wrote:
> >
> > Adding cond_resched() to the command waiting loop for a better
> > co-operation with the scheduler. This allows to give CPU a breath to
> > run other task(workqueue) instead of busy looping when preemption is
> > not allowed on a device whose CVQ might be slow.
> >
> > Signed-off-by: Jason Wang
>
> This still leaves hung processes, but at least it doesn't pin the CPU any
> more. Thanks.
> Reviewed-by: Shannon Nelson
>
I'd like to see a full solution
1- block until interrupt
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, ) &&
> > - !virtqueue_is_broken(vi->cvq))
> > + !virtqueue_is_broken(vi->cvq)) {
> > + cond_resched();
> > cpu_relax();
> > + }
> >
> > return vi->ctrl->status == VIRTIO_NET_OK;
> > }
> > --
> > 2.39.3
> >
> > ___
> > Virtualization mailing list
> > Virtualization@lists.linux-foundation.org
> > https://lists.linuxfoundation.org/mailman/listinfo/virtualization
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH net-next v4 2/2] virtio-net: add cond_resched() to the command waiting loop
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
> > ---
> > 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, ) &&
> > - !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
> > Virtualization@lists.linux-foundation.org
> > https://lists.linuxfoundation.org/mailman/listinfo/virtualization
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH net-next v4 2/2] virtio-net: add cond_resched() to the command waiting loop
On 7/20/23 1:38 AM, Jason Wang wrote:
Adding cond_resched() to the command waiting loop for a better
co-operation with the scheduler. This allows to give CPU a breath to
run other task(workqueue) instead of busy looping when preemption is
not allowed on a device whose CVQ might be slow.
Signed-off-by: Jason Wang
This still leaves hung processes, but at least it doesn't pin the CPU
any more. Thanks.
Reviewed-by: Shannon Nelson
---
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, ) &&
- !virtqueue_is_broken(vi->cvq))
+ !virtqueue_is_broken(vi->cvq)) {
+ cond_resched();
cpu_relax();
+ }
return vi->ctrl->status == VIRTIO_NET_OK;
}
--
2.39.3
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH net-next v4 2/2] virtio-net: add cond_resched() to the command waiting loop
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
---
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, ) &&
- !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
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
[PATCH net-next v4 2/2] virtio-net: add cond_resched() to the command waiting loop
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
---
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, ) &&
- !virtqueue_is_broken(vi->cvq))
+ !virtqueue_is_broken(vi->cvq)) {
+ cond_resched();
cpu_relax();
+ }
return vi->ctrl->status == VIRTIO_NET_OK;
}
--
2.39.3
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
