Re: [PATCH 3/4] virtio_ring: introduce a per virtqueue waitqueue
On Thu, Dec 29, 2022 at 4:10 PM Michael S. Tsirkin wrote: > > On Thu, Dec 29, 2022 at 04:04:13PM +0800, Jason Wang wrote: > > On Thu, Dec 29, 2022 at 3:07 PM Michael S. Tsirkin wrote: > > > > > > On Wed, Dec 28, 2022 at 07:53:08PM +0800, Jason Wang wrote: > > > > On Wed, Dec 28, 2022 at 2:34 PM Jason Wang wrote: > > > > > > > > > > > > > > > 在 2022/12/27 17:38, Michael S. Tsirkin 写道: > > > > > > On Tue, Dec 27, 2022 at 05:12:58PM +0800, Jason Wang wrote: > > > > > >> 在 2022/12/27 15:33, Michael S. Tsirkin 写道: > > > > > >>> On Tue, Dec 27, 2022 at 12:30:35PM +0800, Jason Wang wrote: > > > > > > But device is still going and will later use the buffers. > > > > > > > > > > > > Same for timeout really. > > > > > Avoiding infinite wait/poll is one of the goals, another is to > > > > > sleep. > > > > > If we think the timeout is hard, we can start from the wait. > > > > > > > > > > Thanks > > > > > >>> If the goal is to avoid disrupting traffic while CVQ is in use, > > > > > >>> that sounds more reasonable. E.g. someone is turning on promisc, > > > > > >>> a spike in CPU usage might be unwelcome. > > > > > >> > > > > > >> Yes, this would be more obvious is UP is used. > > > > > >> > > > > > >> > > > > > >>> things we should be careful to address then: > > > > > >>> 1- debugging. Currently it's easy to see a warning if CPU is stuck > > > > > >>> in a loop for a while, and we also get a backtrace. > > > > > >>> E.g. with this - how do we know who has the RTNL? > > > > > >>> We need to integrate with kernel/watchdog.c for good results > > > > > >>> and to make sure policy is consistent. > > > > > >> > > > > > >> That's fine, will consider this. > > > > > > > > So after some investigation, it seems the watchdog.c doesn't help. The > > > > only export helper is touch_softlockup_watchdog() which tries to avoid > > > > triggering the lockups warning for the known slow path. > > > > > > I never said you can just use existing exporting APIs. You'll have to > > > write new ones :) > > > > Ok, I thought you wanted to trigger similar warnings as a watchdog. > > > > Btw, I wonder what kind of logic you want here. If we switch to using > > sleep, there won't be soft lockup anymore. A simple wait + timeout + > > warning seems sufficient? > > > > Thanks > > I'd like to avoid need to teach users new APIs. So watchdog setup to apply > to this driver. The warning can be different. Right, so it looks to me the only possible setup is the watchdog_thres. I plan to trigger the warning every watchdog_thres * 2 second (as softlockup did). And I think it would still make sense to fail, we can start with a very long timeout like 1 minutes and break the device. Does this make sense? Thanks > > > > > > > > > And before the patch, we end up with a real infinite loop which could > > > > be caught by RCU stall detector which is not the case of the sleep. > > > > What we can do is probably do a periodic netdev_err(). > > > > > > > > Thanks > > > > > > Only with a bad device. > > > > > > > > >> > > > > > >> > > > > > >>> 2- overhead. In a very common scenario when device is in > > > > > >>> hypervisor, > > > > > >>> programming timers etc has a very high overhead, at bootup > > > > > >>> lots of CVQ commands are run and slowing boot down is not > > > > > >>> nice. > > > > > >>> let's poll for a bit before waiting? > > > > > >> > > > > > >> Then we go back to the question of choosing a good timeout for > > > > > >> poll. And > > > > > >> poll seems problematic in the case of UP, scheduler might not have > > > > > >> the > > > > > >> chance to run. > > > > > > Poll just a bit :) Seriously I don't know, but at least check once > > > > > > after kick. > > > > > > > > > > > > > > > I think it is what the current code did where the condition will be > > > > > check before trying to sleep in the wait_event(). > > > > > > > > > > > > > > > > > > > > > >>> 3- suprise removal. need to wake up thread in some way. what about > > > > > >>> other cases of device breakage - is there a chance this > > > > > >>> introduces new bugs around that? at least enumerate them > > > > > >>> please. > > > > > >> > > > > > >> The current code did: > > > > > >> > > > > > >> 1) check for vq->broken > > > > > >> 2) wakeup during BAD_RING() > > > > > >> > > > > > >> So we won't end up with a never woke up process which should be > > > > > >> fine. > > > > > >> > > > > > >> Thanks > > > > > > > > > > > > BTW BAD_RING on removal will trigger dev_err. Not sure that is a > > > > > > good > > > > > > idea - can cause crashes if kernel panics on error. > > > > > > > > > > > > > > > Yes, it's better to use __virtqueue_break() instead. > > > > > > > > > > But consider we will start from a wait first, I will limit the changes > > > > > in virtio-net without bothering virtio core. > > > > > > > > > > Thanks > > > > > > > > > > > > > > > > > > > > > >>> > > > > __
Re: [PATCH 1/4] virtio-net: convert rx mode setting to use workqueue
On Fri, Dec 30, 2022 at 10:51 AM Jakub Kicinski wrote: > > On Tue, 27 Dec 2022 17:06:10 +0800 Jason Wang wrote: > > > Hmm so user tells us to e.g enable promisc. We report completion > > > but card is still dropping packets. I think this > > > has a chance to break some setups. > > > > I think all those filters are best efforts, am I wrong? > > Are the flags protected by the addr lock which needs BH, tho? > > Taking netif_addr_lock_bh() to look at dev->flags seems a bit > surprising to me. > Yes, RTNL should be sufficient here. Will fix it. Thanks ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [RESEND PATCH 1/3] Add SolidRun vendor id
On Thu, Dec 29, 2022 at 03:29:06PM -0600, Bjorn Helgaas wrote: > On Thu, Dec 29, 2022 at 11:06:02PM +0200, Alvaro Karsz wrote: > > > On Mon, Dec 19, 2022 at 10:35:09AM +0200, Alvaro Karsz wrote: > > > > The vendor id is used in 2 differrent source files, > > > > the SNET vdpa driver and pci quirks. > > > > > > s/id/ID/ # both in subject and commit log > > > s/differrent/different/ > > > s/vdpa/vDPA/ # seems to be the conventional style > > > s/pci/PCI/ > > > > > > Make the commit log say what this patch does. > > > > > > > Signed-off-by: Alvaro Karsz > > > > > > With the above and the sorting fix below: > > > > > > Acked-by: Bjorn Helgaas > > > > > > > --- > > > > include/linux/pci_ids.h | 2 ++ > > > > 1 file changed, 2 insertions(+) > > > > > > > > diff --git a/include/linux/pci_ids.h b/include/linux/pci_ids.h > > > > index b362d90eb9b..33bbe3160b4 100644 > > > > --- a/include/linux/pci_ids.h > > > > +++ b/include/linux/pci_ids.h > > > > @@ -3115,4 +3115,6 @@ > > > > > > > > #define PCI_VENDOR_ID_NCUBE 0x10ff > > > > > > > > +#define PCI_VENDOR_ID_SOLIDRUN 0xd063 > > > > > > Move this to the right spot so the file is sorted by vendor ID. > > > PCI_VENDOR_ID_NCUBE, PCI_VENDOR_ID_OCZ, and PCI_VENDOR_ID_XEN got > > > added in the wrong place. > > > > > > > #endif /* _LINUX_PCI_IDS_H */ > > > > -- > > > > Thanks for your comments. > > > > The patch was taken by another maintainer (CCed) > > https://git.kernel.org/pub/scm/linux/kernel/git/mst/vhost.git/commit/?h=linux-next&id=afc9dcfb846bf35aa7afb160d5370ab5c75e7a70 > > > > So, Michael and Bjorn, > > Do you want me to create a new version, or fix it in a follow up patch? > > > > BTW, the same is true for the next patch in the series, New PCI quirk > > for SolidRun SNET DPU > > https://git.kernel.org/pub/scm/linux/kernel/git/mst/vhost.git/commit/?h=linux-next&id=136dd8d8f3a0ac19f75a875e9b27b83d365a5be3 > > I don't know how Michael runs his tree, so it's up to him, but "New > PCI quirk for SolidRun SNET DPU." is completely different from all the > history and not very informative, so if it were via my tree I would > definitely update both. > > Bjorn New version pls, I rebase so no problem to replace. -- MST ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [RESEND PATCH 1/3] Add SolidRun vendor id
On Thu, Dec 29, 2022 at 11:06:02PM +0200, Alvaro Karsz wrote: > > On Mon, Dec 19, 2022 at 10:35:09AM +0200, Alvaro Karsz wrote: > > > The vendor id is used in 2 differrent source files, > > > the SNET vdpa driver and pci quirks. > > > > s/id/ID/ # both in subject and commit log > > s/differrent/different/ > > s/vdpa/vDPA/ # seems to be the conventional style > > s/pci/PCI/ > > > > Make the commit log say what this patch does. > > > > > Signed-off-by: Alvaro Karsz > > > > With the above and the sorting fix below: > > > > Acked-by: Bjorn Helgaas > > > > > --- > > > include/linux/pci_ids.h | 2 ++ > > > 1 file changed, 2 insertions(+) > > > > > > diff --git a/include/linux/pci_ids.h b/include/linux/pci_ids.h > > > index b362d90eb9b..33bbe3160b4 100644 > > > --- a/include/linux/pci_ids.h > > > +++ b/include/linux/pci_ids.h > > > @@ -3115,4 +3115,6 @@ > > > > > > #define PCI_VENDOR_ID_NCUBE 0x10ff > > > > > > +#define PCI_VENDOR_ID_SOLIDRUN 0xd063 > > > > Move this to the right spot so the file is sorted by vendor ID. > > PCI_VENDOR_ID_NCUBE, PCI_VENDOR_ID_OCZ, and PCI_VENDOR_ID_XEN got > > added in the wrong place. > > > > > #endif /* _LINUX_PCI_IDS_H */ > > > -- > > Thanks for your comments. > > The patch was taken by another maintainer (CCed) > https://git.kernel.org/pub/scm/linux/kernel/git/mst/vhost.git/commit/?h=linux-next&id=afc9dcfb846bf35aa7afb160d5370ab5c75e7a70 > > So, Michael and Bjorn, > Do you want me to create a new version, or fix it in a follow up patch? > > BTW, the same is true for the next patch in the series, New PCI quirk > for SolidRun SNET DPU > https://git.kernel.org/pub/scm/linux/kernel/git/mst/vhost.git/commit/?h=linux-next&id=136dd8d8f3a0ac19f75a875e9b27b83d365a5be3 I don't know how Michael runs his tree, so it's up to him, but "New PCI quirk for SolidRun SNET DPU." is completely different from all the history and not very informative, so if it were via my tree I would definitely update both. Bjorn ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [RESEND PATCH 1/3] Add SolidRun vendor id
Hi Bjorn, > Hi Alvaro, > > On Mon, Dec 19, 2022 at 10:35:09AM +0200, Alvaro Karsz wrote: > > The vendor id is used in 2 differrent source files, > > the SNET vdpa driver and pci quirks. > > s/id/ID/ # both in subject and commit log > s/differrent/different/ > s/vdpa/vDPA/ # seems to be the conventional style > s/pci/PCI/ > > Make the commit log say what this patch does. > > > Signed-off-by: Alvaro Karsz > > With the above and the sorting fix below: > > Acked-by: Bjorn Helgaas > > > --- > > include/linux/pci_ids.h | 2 ++ > > 1 file changed, 2 insertions(+) > > > > diff --git a/include/linux/pci_ids.h b/include/linux/pci_ids.h > > index b362d90eb9b..33bbe3160b4 100644 > > --- a/include/linux/pci_ids.h > > +++ b/include/linux/pci_ids.h > > @@ -3115,4 +3115,6 @@ > > > > #define PCI_VENDOR_ID_NCUBE 0x10ff > > > > +#define PCI_VENDOR_ID_SOLIDRUN 0xd063 > > Move this to the right spot so the file is sorted by vendor ID. > PCI_VENDOR_ID_NCUBE, PCI_VENDOR_ID_OCZ, and PCI_VENDOR_ID_XEN got > added in the wrong place. > > > #endif /* _LINUX_PCI_IDS_H */ > > -- Thanks for your comments. The patch was taken by another maintainer (CCed) https://git.kernel.org/pub/scm/linux/kernel/git/mst/vhost.git/commit/?h=linux-next&id=afc9dcfb846bf35aa7afb160d5370ab5c75e7a70 So, Michael and Bjorn, Do you want me to create a new version, or fix it in a follow up patch? BTW, the same is true for the next patch in the series, New PCI quirk for SolidRun SNET DPU https://git.kernel.org/pub/scm/linux/kernel/git/mst/vhost.git/commit/?h=linux-next&id=136dd8d8f3a0ac19f75a875e9b27b83d365a5be3 Thanks. ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [RESEND PATCH 1/3] Add SolidRun vendor id
Hi Alvaro, On Mon, Dec 19, 2022 at 10:35:09AM +0200, Alvaro Karsz wrote: > The vendor id is used in 2 differrent source files, > the SNET vdpa driver and pci quirks. s/id/ID/ # both in subject and commit log s/differrent/different/ s/vdpa/vDPA/ # seems to be the conventional style s/pci/PCI/ Make the commit log say what this patch does. > Signed-off-by: Alvaro Karsz With the above and the sorting fix below: Acked-by: Bjorn Helgaas > --- > include/linux/pci_ids.h | 2 ++ > 1 file changed, 2 insertions(+) > > diff --git a/include/linux/pci_ids.h b/include/linux/pci_ids.h > index b362d90eb9b..33bbe3160b4 100644 > --- a/include/linux/pci_ids.h > +++ b/include/linux/pci_ids.h > @@ -3115,4 +3115,6 @@ > > #define PCI_VENDOR_ID_NCUBE 0x10ff > > +#define PCI_VENDOR_ID_SOLIDRUN 0xd063 Move this to the right spot so the file is sorted by vendor ID. PCI_VENDOR_ID_NCUBE, PCI_VENDOR_ID_OCZ, and PCI_VENDOR_ID_XEN got added in the wrong place. > #endif /* _LINUX_PCI_IDS_H */ > -- > 2.32.0 > > ___ > 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: [RESEND PATCH 2/3] New PCI quirk for SolidRun SNET DPU.
Hi Alvaro,
Thanks for the patch!
On Mon, Dec 19, 2022 at 10:35:10AM +0200, Alvaro Karsz wrote:
> The DPU advertises FLR, but it may cause the device to hang.
> This only happens with revision 0x1.
Please update the subject line to:
PCI: Avoid FLR for SolidRun SNET DPU rev 1
This makes the subject meaningful by itself and is similar to previous
quirks:
5727043c73fd ("PCI: Avoid FLR for AMD Starship USB 3.0")
0d14f06cd665 ("PCI: Avoid FLR for AMD Matisse HD Audio & USB 3.0")
f65fd1aa4f98 ("PCI: Avoid FLR for Intel 82579 NICs")
Also, update the commit log so it says what this patch does, instead
of simply describing the current situation.
https://chris.beams.io/posts/git-commit/ is a good reference.
With the above changes,
Acked-by: Bjorn Helgaas
> Signed-off-by: Alvaro Karsz
> ---
> drivers/pci/quirks.c | 8
> 1 file changed, 8 insertions(+)
>
> diff --git a/drivers/pci/quirks.c b/drivers/pci/quirks.c
> index 285acc4aacc..809d03272c2 100644
> --- a/drivers/pci/quirks.c
> +++ b/drivers/pci/quirks.c
> @@ -5343,6 +5343,14 @@ DECLARE_PCI_FIXUP_EARLY(PCI_VENDOR_ID_AMD, 0x149c,
> quirk_no_flr);
> DECLARE_PCI_FIXUP_EARLY(PCI_VENDOR_ID_INTEL, 0x1502, quirk_no_flr);
> DECLARE_PCI_FIXUP_EARLY(PCI_VENDOR_ID_INTEL, 0x1503, quirk_no_flr);
>
> +/* FLR may cause the SolidRun SNET DPU (rev 0x1) to hang */
> +static void quirk_no_flr_snet(struct pci_dev *dev)
> +{
> + if (dev->revision == 0x1)
> + quirk_no_flr(dev);
> +}
> +DECLARE_PCI_FIXUP_EARLY(PCI_VENDOR_ID_SOLIDRUN, 0x1000, quirk_no_flr_snet);
> +
> static void quirk_no_ext_tags(struct pci_dev *pdev)
> {
> struct pci_host_bridge *bridge = pci_find_host_bridge(pdev->bus);
> --
> 2.32.0
>
> ___
> 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] virtio: vdpa: explain the dependency of SNET_VDPA on HWMON
> It's ok. I had to push everything out to next merge window > though. Sorry :( Don't worry about it :) ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH] virtio: vdpa: explain the dependency of SNET_VDPA on HWMON
On Thu, Dec 29, 2022 at 02:13:57PM +0200, Alvaro Karsz wrote: > Hi Michael, > > Is this patch ok with you? > Do you have any comments? It's ok. I had to push everything out to next merge window though. Sorry :( -- MST ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH v5] virtio_blk: add VIRTIO_BLK_F_LIFETIME feature support
On Thu, Dec 29, 2022 at 02:08:34PM +0200, Alvaro Karsz wrote: > > So due to my mistake this got pushed out to next linux release. > > Sorry :( > > No problem at all :) > > > I'd like to use this opportunity to write a new better > > interface that actually works with modern backends, > > and add it to the virtio spec. > > > > Do you think you can take up this task? > > Sure, I can do it. > So, the idea is to: > > * Drop this patch > * Implement a new, more general virtio block feature for virtio spec > * Add linux support for the new feature > > right? That's certainly an option. Let's start with point 2, and get ack from people who objected to this feature. -- MST ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH] virtio: vdpa: explain the dependency of SNET_VDPA on HWMON
Hi Michael, Is this patch ok with you? Do you have any comments? ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH v5] virtio_blk: add VIRTIO_BLK_F_LIFETIME feature support
> So due to my mistake this got pushed out to next linux release. > Sorry :( No problem at all :) > I'd like to use this opportunity to write a new better > interface that actually works with modern backends, > and add it to the virtio spec. > > Do you think you can take up this task? Sure, I can do it. So, the idea is to: * Drop this patch * Implement a new, more general virtio block feature for virtio spec * Add linux support for the new feature right? ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH 3/4] virtio_ring: introduce a per virtqueue waitqueue
On Thu, Dec 29, 2022 at 04:04:13PM +0800, Jason Wang wrote: > On Thu, Dec 29, 2022 at 3:07 PM Michael S. Tsirkin wrote: > > > > On Wed, Dec 28, 2022 at 07:53:08PM +0800, Jason Wang wrote: > > > On Wed, Dec 28, 2022 at 2:34 PM Jason Wang wrote: > > > > > > > > > > > > 在 2022/12/27 17:38, Michael S. Tsirkin 写道: > > > > > On Tue, Dec 27, 2022 at 05:12:58PM +0800, Jason Wang wrote: > > > > >> 在 2022/12/27 15:33, Michael S. Tsirkin 写道: > > > > >>> On Tue, Dec 27, 2022 at 12:30:35PM +0800, Jason Wang wrote: > > > > > But device is still going and will later use the buffers. > > > > > > > > > > Same for timeout really. > > > > Avoiding infinite wait/poll is one of the goals, another is to > > > > sleep. > > > > If we think the timeout is hard, we can start from the wait. > > > > > > > > Thanks > > > > >>> If the goal is to avoid disrupting traffic while CVQ is in use, > > > > >>> that sounds more reasonable. E.g. someone is turning on promisc, > > > > >>> a spike in CPU usage might be unwelcome. > > > > >> > > > > >> Yes, this would be more obvious is UP is used. > > > > >> > > > > >> > > > > >>> things we should be careful to address then: > > > > >>> 1- debugging. Currently it's easy to see a warning if CPU is stuck > > > > >>> in a loop for a while, and we also get a backtrace. > > > > >>> E.g. with this - how do we know who has the RTNL? > > > > >>> We need to integrate with kernel/watchdog.c for good results > > > > >>> and to make sure policy is consistent. > > > > >> > > > > >> That's fine, will consider this. > > > > > > So after some investigation, it seems the watchdog.c doesn't help. The > > > only export helper is touch_softlockup_watchdog() which tries to avoid > > > triggering the lockups warning for the known slow path. > > > > I never said you can just use existing exporting APIs. You'll have to > > write new ones :) > > Ok, I thought you wanted to trigger similar warnings as a watchdog. > > Btw, I wonder what kind of logic you want here. If we switch to using > sleep, there won't be soft lockup anymore. A simple wait + timeout + > warning seems sufficient? > > Thanks I'd like to avoid need to teach users new APIs. So watchdog setup to apply to this driver. The warning can be different. > > > > > And before the patch, we end up with a real infinite loop which could > > > be caught by RCU stall detector which is not the case of the sleep. > > > What we can do is probably do a periodic netdev_err(). > > > > > > Thanks > > > > Only with a bad device. > > > > > > >> > > > > >> > > > > >>> 2- overhead. In a very common scenario when device is in hypervisor, > > > > >>> programming timers etc has a very high overhead, at bootup > > > > >>> lots of CVQ commands are run and slowing boot down is not nice. > > > > >>> let's poll for a bit before waiting? > > > > >> > > > > >> Then we go back to the question of choosing a good timeout for poll. > > > > >> And > > > > >> poll seems problematic in the case of UP, scheduler might not have > > > > >> the > > > > >> chance to run. > > > > > Poll just a bit :) Seriously I don't know, but at least check once > > > > > after kick. > > > > > > > > > > > > I think it is what the current code did where the condition will be > > > > check before trying to sleep in the wait_event(). > > > > > > > > > > > > > > > > > >>> 3- suprise removal. need to wake up thread in some way. what about > > > > >>> other cases of device breakage - is there a chance this > > > > >>> introduces new bugs around that? at least enumerate them > > > > >>> please. > > > > >> > > > > >> The current code did: > > > > >> > > > > >> 1) check for vq->broken > > > > >> 2) wakeup during BAD_RING() > > > > >> > > > > >> So we won't end up with a never woke up process which should be fine. > > > > >> > > > > >> Thanks > > > > > > > > > > BTW BAD_RING on removal will trigger dev_err. Not sure that is a good > > > > > idea - can cause crashes if kernel panics on error. > > > > > > > > > > > > Yes, it's better to use __virtqueue_break() instead. > > > > > > > > But consider we will start from a wait first, I will limit the changes > > > > in virtio-net without bothering virtio core. > > > > > > > > Thanks > > > > > > > > > > > > > > > > > >>> > > ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH v5] virtio_blk: add VIRTIO_BLK_F_LIFETIME feature support
On Mon, Dec 19, 2022 at 02:21:55PM +0200, Alvaro Karsz wrote:
> Implement the VIRTIO_BLK_F_LIFETIME feature for Virtio block devices.
>
> This commit introduces a new ioctl command, VIRTIO_BLK_IOCTL_GET_LIFETIME.
>
> VIRTIO_BLK_IOCTL_GET_LIFETIME ioctl asks for the block device to provide
> lifetime information by sending a VIRTIO_BLK_T_GET_LIFETIME command to
> the device.
>
> lifetime information fields:
>
> - pre_eol_info: specifies the percentage of reserved blocks that are
> consumed.
> optional values following virtio spec:
> *) 0 - undefined.
> *) 1 - normal, < 80% of reserved blocks are consumed.
> *) 2 - warning, 80% of reserved blocks are consumed.
> *) 3 - urgent, 90% of reserved blocks are consumed.
>
> - device_lifetime_est_typ_a: this field refers to wear of SLC cells and
>is provided in increments of 10used, and so
>on, thru to 11 meaning estimated lifetime
>exceeded. All values above 11 are reserved.
>
> - device_lifetime_est_typ_b: this field refers to wear of MLC cells and is
>provided with the same semantics as
>device_lifetime_est_typ_a.
>
> The data received from the device will be sent as is to the user.
> No data check/decode is done by virtblk.
>
> Signed-off-by: Alvaro Karsz
> Reviewed-by: Stefan Hajnoczi
So due to my mistake this got pushed out to next linux release.
Sorry :(
I'd like to use this opportunity to write a new better
interface that actually works with modern backends,
and add it to the virtio spec.
Do you think you can take up this task?
> --
> v2:
> - Remove (void *) casting.
> - Fix comments format in virtio_blk.h.
> - Set ioprio value for legacy devices for REQ_OP_DRV_IN
> operations.
>
> v3:
> - Initialize struct virtio_blk_lifetime to 0 instead of memset.
> - Rename ioctl from VBLK_LIFETIME to VBLK_GET_LIFETIME.
> - Return EOPNOTSUPP insted of ENOTTY if ioctl is not supported.
> - Check if process is CAP_SYS_ADMIN capable in lifetime ioctl.
> - Check if vdev is not NULL before accessing it in lifetime ioctl.
>
> v4:
> - Create a dedicated virtio_blk_ioctl.h header for the ioctl command
> and add this file to MAINTAINERS.
> - Rename the ioctl to VIRTIO_BLK_IOCTL_GET_LIFETIME.
> - Document in virtio_blk_ioctl.h which backend device can supply
> this lifetime information.
>
> v5:
> - Rebase patch on top of mst tree.
> - Add a comment explaining the capable(CAP_SYS_ADMIN) part.
> --
> ---
> MAINTAINERS | 1 +
> drivers/block/virtio_blk.c| 102 +-
> include/uapi/linux/virtio_blk.h | 28 +++
> include/uapi/linux/virtio_blk_ioctl.h | 44 +++
> 4 files changed, 173 insertions(+), 2 deletions(-)
> create mode 100644 include/uapi/linux/virtio_blk_ioctl.h
>
> diff --git a/MAINTAINERS b/MAINTAINERS
> index 4aee796cc..3250f7753 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -21724,6 +21724,7 @@ F:drivers/block/virtio_blk.c
> F: drivers/scsi/virtio_scsi.c
> F: drivers/vhost/scsi.c
> F: include/uapi/linux/virtio_blk.h
> +F: include/uapi/linux/virtio_blk_ioctl.h
> F: include/uapi/linux/virtio_scsi.h
>
> VIRTIO CONSOLE DRIVER
> diff --git a/drivers/block/virtio_blk.c b/drivers/block/virtio_blk.c
> index 3c9dae237..1e03bfa9f 100644
> --- a/drivers/block/virtio_blk.c
> +++ b/drivers/block/virtio_blk.c
> @@ -9,6 +9,7 @@
> #include
> #include
> #include
> +#include
> #include
> #include
> #include
> @@ -112,6 +113,18 @@ struct virtblk_req {
> struct scatterlist sg[];
> };
>
> +static inline int virtblk_ioctl_result(struct virtblk_req *vbr)
> +{
> + switch (vbr->status) {
> + case VIRTIO_BLK_S_OK:
> + return 0;
> + case VIRTIO_BLK_S_UNSUPP:
> + return -EOPNOTSUPP;
> + default:
> + return -EIO;
> + }
> +}
> +
> static inline blk_status_t virtblk_result(u8 status)
> {
> switch (status) {
> @@ -840,6 +853,90 @@ static int virtblk_getgeo(struct block_device *bd,
> struct hd_geometry *geo)
> return ret;
> }
>
> +/* Get lifetime information from device */
> +static int virtblk_ioctl_lifetime(struct virtio_blk *vblk, unsigned long arg)
> +{
> + struct request_queue *q = vblk->disk->queue;
> + struct request *req = NULL;
> + struct virtblk_req *vbr;
> + struct virtio_blk_lifetime lifetime = {};
> + int ret;
> +
> + /* It's not clear whether exposing lifetime info to userspace
> + * has any security implications but out of abundance of caution
> + * we limit it to privileged users.
> + */
> + if (!capable(CAP_SYS_ADMIN))
> + return -EPERM;
> +
> + /* The virtio_blk_lifetime struct fields follo
Re: [PATCH 3/4] virtio_ring: introduce a per virtqueue waitqueue
On Thu, Dec 29, 2022 at 3:07 PM Michael S. Tsirkin wrote: > > On Wed, Dec 28, 2022 at 07:53:08PM +0800, Jason Wang wrote: > > On Wed, Dec 28, 2022 at 2:34 PM Jason Wang wrote: > > > > > > > > > 在 2022/12/27 17:38, Michael S. Tsirkin 写道: > > > > On Tue, Dec 27, 2022 at 05:12:58PM +0800, Jason Wang wrote: > > > >> 在 2022/12/27 15:33, Michael S. Tsirkin 写道: > > > >>> On Tue, Dec 27, 2022 at 12:30:35PM +0800, Jason Wang wrote: > > > > But device is still going and will later use the buffers. > > > > > > > > Same for timeout really. > > > Avoiding infinite wait/poll is one of the goals, another is to sleep. > > > If we think the timeout is hard, we can start from the wait. > > > > > > Thanks > > > >>> If the goal is to avoid disrupting traffic while CVQ is in use, > > > >>> that sounds more reasonable. E.g. someone is turning on promisc, > > > >>> a spike in CPU usage might be unwelcome. > > > >> > > > >> Yes, this would be more obvious is UP is used. > > > >> > > > >> > > > >>> things we should be careful to address then: > > > >>> 1- debugging. Currently it's easy to see a warning if CPU is stuck > > > >>> in a loop for a while, and we also get a backtrace. > > > >>> E.g. with this - how do we know who has the RTNL? > > > >>> We need to integrate with kernel/watchdog.c for good results > > > >>> and to make sure policy is consistent. > > > >> > > > >> That's fine, will consider this. > > > > So after some investigation, it seems the watchdog.c doesn't help. The > > only export helper is touch_softlockup_watchdog() which tries to avoid > > triggering the lockups warning for the known slow path. > > I never said you can just use existing exporting APIs. You'll have to > write new ones :) Ok, I thought you wanted to trigger similar warnings as a watchdog. Btw, I wonder what kind of logic you want here. If we switch to using sleep, there won't be soft lockup anymore. A simple wait + timeout + warning seems sufficient? Thanks > > > And before the patch, we end up with a real infinite loop which could > > be caught by RCU stall detector which is not the case of the sleep. > > What we can do is probably do a periodic netdev_err(). > > > > Thanks > > Only with a bad device. > > > > >> > > > >> > > > >>> 2- overhead. In a very common scenario when device is in hypervisor, > > > >>> programming timers etc has a very high overhead, at bootup > > > >>> lots of CVQ commands are run and slowing boot down is not nice. > > > >>> let's poll for a bit before waiting? > > > >> > > > >> Then we go back to the question of choosing a good timeout for poll. > > > >> And > > > >> poll seems problematic in the case of UP, scheduler might not have the > > > >> chance to run. > > > > Poll just a bit :) Seriously I don't know, but at least check once > > > > after kick. > > > > > > > > > I think it is what the current code did where the condition will be > > > check before trying to sleep in the wait_event(). > > > > > > > > > > > > > >>> 3- suprise removal. need to wake up thread in some way. what about > > > >>> other cases of device breakage - is there a chance this > > > >>> introduces new bugs around that? at least enumerate them please. > > > >> > > > >> The current code did: > > > >> > > > >> 1) check for vq->broken > > > >> 2) wakeup during BAD_RING() > > > >> > > > >> So we won't end up with a never woke up process which should be fine. > > > >> > > > >> Thanks > > > > > > > > BTW BAD_RING on removal will trigger dev_err. Not sure that is a good > > > > idea - can cause crashes if kernel panics on error. > > > > > > > > > Yes, it's better to use __virtqueue_break() instead. > > > > > > But consider we will start from a wait first, I will limit the changes > > > in virtio-net without bothering virtio core. > > > > > > Thanks > > > > > > > > > > > > > >>> > ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization
