From: Jason Gunthorpe <jgg@nvidia.com> To: Cornelia Huck <cohuck@redhat.com> Cc: David Airlie <airlied@linux.ie>, Tony Krowiak <akrowiak@linux.ibm.com>, Alex Williamson <alex.williamson@redhat.com>, Christian Borntraeger <borntraeger@de.ibm.com>, Daniel Vetter <daniel@ffwll.ch>, dri-devel@lists.freedesktop.org, Eric Farman <farman@linux.ibm.com>, Harald Freudenberger <freude@linux.ibm.com>, Vasily Gorbik <gor@linux.ibm.com>, Heiko Carstens <hca@linux.ibm.com>, intel-gfx@lists.freedesktop.org, intel-gvt-dev@lists.freedesktop.org, Jani Nikula <jani.nikula@linux.intel.com>, Jason Herne <jjherne@linux.ibm.com>, Joonas Lahtinen <joonas.lahtinen@linux.intel.com>, kvm@vger.kernel.org, Kirti Wankhede <kwankhede@nvidia.com>, linux-s390@vger.kernel.org, Matthew Rosato <mjrosato@linux.ibm.com>, Peter Oberparleiter <oberpar@linux.ibm.com>, Halil Pasic <pasic@linux.ibm.com>, Rodrigo Vivi <rodrigo.vivi@intel.com>, Vineeth Vijayan <vneethv@linux.ibm.com>, Zhenyu Wang <zhenyuw@linux.intel.com>, Zhi Wang <zhi.a.wang@intel.com>, Christoph Hellwig <hch@lst.de> Subject: Re: [PATCH v2 4/9] vfio/ccw: Make the FSM complete and synchronize it to the mdev Date: Mon, 20 Sep 2021 09:30:18 -0300 [thread overview] Message-ID: <20210920123018.GF327412@nvidia.com> (raw) In-Reply-To: <87zgs7fni1.fsf@redhat.com> On Mon, Sep 20, 2021 at 02:19:18PM +0200, Cornelia Huck wrote: > On Thu, Sep 09 2021, Jason Gunthorpe <jgg@nvidia.com> wrote: > > > The subchannel should be left in a quiescent state unless the VFIO device > > FD is opened. When the FD is opened bring the chanel to active and allow > > the VFIO device to operate. When the device FD is closed then quiesce the > > channel. > > > > To make this work the FSM needs to handle the transitions to/from open and > > closed so everything is sequenced. Rename state NOT_OPER to BROKEN and use > > it wheneven the driver has malfunctioned. STANDBY becomes CLOSED. The > > normal case FSM looks like: > > CLOSED -> IDLE -> PROCESS/PENDING* -> IDLE -> CLOSED > > > > With a possible branch off to BROKEN from any state. Once the device is in > > BROKEN it cannot be recovered other than be reloading the driver. > > Hm, not sure whether it is a good idea to conflate "something went > wrong" and "device is not operational". Yes, but that is exactly what this FSM is currently doing, NO_OPER is a dumping ground for all kinds of wonky stuff, and what exactly it is supposed to mean or do is unclear. > while the former case could mean all kind of > things, but the subchannel will likely stay around. I think NOT_OPER > was always meant to be a transitional state. Then these sorts of failures should recover the device and FSM back to an appropriate operational state and keep going - but I'm not going to attempt to guess when each of the conditions are recoverable or not. > > Delete the triply redundant calls to > > vfio_ccw_sch_quiesce(). vfio_ccw_mdev_close_device() always leaves the > > subchannel quiescent. vfio_ccw_mdev_remove() cannot return until > > vfio_ccw_mdev_close_device() completes and vfio_ccw_sch_remove() cannot > > return until vfio_ccw_mdev_remove() completes. Have the FSM code take care > > of calling cp_free() when appropriate. > > I remember some serialization issues wrt cp_free() etc. coming up every > now and than; that might need extra care (I'm taking a look.) I'm not too surprised, things like NOT_OPER just exiting the usual FSM logic mean stuff couldn't be properly sequenced. The new logic puts a cp_free in each of arcs entering the terminal states broken/closed and all the flows that would get us to vfio_ccw_mdev_remove() will enter one of those states. It is quite possible this patch needs someone who actually understand this HW to polish it up - the point was to show how ccw should be cleanly structured. I'd like to go ahead with the other patches and leave this for the ccw maintainers if it is needs significant work. The other patches are what are blocking the core code cleanups. Jason
WARNING: multiple messages have this Message-ID (diff)
From: Jason Gunthorpe <jgg@nvidia.com> To: Cornelia Huck <cohuck@redhat.com> Cc: David Airlie <airlied@linux.ie>, Tony Krowiak <akrowiak@linux.ibm.com>, Alex Williamson <alex.williamson@redhat.com>, Christian Borntraeger <borntraeger@de.ibm.com>, Daniel Vetter <daniel@ffwll.ch>, dri-devel@lists.freedesktop.org, Eric Farman <farman@linux.ibm.com>, Harald Freudenberger <freude@linux.ibm.com>, Vasily Gorbik <gor@linux.ibm.com>, Heiko Carstens <hca@linux.ibm.com>, intel-gfx@lists.freedesktop.org, intel-gvt-dev@lists.freedesktop.org, Jani Nikula <jani.nikula@linux.intel.com>, Jason Herne <jjherne@linux.ibm.com>, Joonas Lahtinen <joonas.lahtinen@linux.intel.com>, kvm@vger.kernel.org, Kirti Wankhede <kwankhede@nvidia.com>, linux-s390@vger.kernel.org, Matthew Rosato <mjrosato@linux.ibm.com>, Peter Oberparleiter <oberpar@linux.ibm.com>, Halil Pasic <pasic@linux.ibm.com>, Rodrigo Vivi <rodrigo.vivi@intel.com>, Vineeth Vijayan <vneethv@linux.ibm.com>, Zhenyu Wang <zhenyuw@linux.intel.com>, Zhi Wang <zhi.a.wang@intel.com>, Christoph Hellwig <hch@lst.de> Subject: Re: [Intel-gfx] [PATCH v2 4/9] vfio/ccw: Make the FSM complete and synchronize it to the mdev Date: Mon, 20 Sep 2021 09:30:18 -0300 [thread overview] Message-ID: <20210920123018.GF327412@nvidia.com> (raw) In-Reply-To: <87zgs7fni1.fsf@redhat.com> On Mon, Sep 20, 2021 at 02:19:18PM +0200, Cornelia Huck wrote: > On Thu, Sep 09 2021, Jason Gunthorpe <jgg@nvidia.com> wrote: > > > The subchannel should be left in a quiescent state unless the VFIO device > > FD is opened. When the FD is opened bring the chanel to active and allow > > the VFIO device to operate. When the device FD is closed then quiesce the > > channel. > > > > To make this work the FSM needs to handle the transitions to/from open and > > closed so everything is sequenced. Rename state NOT_OPER to BROKEN and use > > it wheneven the driver has malfunctioned. STANDBY becomes CLOSED. The > > normal case FSM looks like: > > CLOSED -> IDLE -> PROCESS/PENDING* -> IDLE -> CLOSED > > > > With a possible branch off to BROKEN from any state. Once the device is in > > BROKEN it cannot be recovered other than be reloading the driver. > > Hm, not sure whether it is a good idea to conflate "something went > wrong" and "device is not operational". Yes, but that is exactly what this FSM is currently doing, NO_OPER is a dumping ground for all kinds of wonky stuff, and what exactly it is supposed to mean or do is unclear. > while the former case could mean all kind of > things, but the subchannel will likely stay around. I think NOT_OPER > was always meant to be a transitional state. Then these sorts of failures should recover the device and FSM back to an appropriate operational state and keep going - but I'm not going to attempt to guess when each of the conditions are recoverable or not. > > Delete the triply redundant calls to > > vfio_ccw_sch_quiesce(). vfio_ccw_mdev_close_device() always leaves the > > subchannel quiescent. vfio_ccw_mdev_remove() cannot return until > > vfio_ccw_mdev_close_device() completes and vfio_ccw_sch_remove() cannot > > return until vfio_ccw_mdev_remove() completes. Have the FSM code take care > > of calling cp_free() when appropriate. > > I remember some serialization issues wrt cp_free() etc. coming up every > now and than; that might need extra care (I'm taking a look.) I'm not too surprised, things like NOT_OPER just exiting the usual FSM logic mean stuff couldn't be properly sequenced. The new logic puts a cp_free in each of arcs entering the terminal states broken/closed and all the flows that would get us to vfio_ccw_mdev_remove() will enter one of those states. It is quite possible this patch needs someone who actually understand this HW to polish it up - the point was to show how ccw should be cleanly structured. I'd like to go ahead with the other patches and leave this for the ccw maintainers if it is needs significant work. The other patches are what are blocking the core code cleanups. Jason
next prev parent reply other threads:[~2021-09-20 12:30 UTC|newest] Thread overview: 80+ messages / expand[flat|nested] mbox.gz Atom feed top 2021-09-09 19:38 [PATCH v2 0/9] Move vfio_ccw to the new mdev API Jason Gunthorpe 2021-09-09 19:38 ` [Intel-gfx] " Jason Gunthorpe 2021-09-09 19:38 ` [PATCH v2 1/9] vfio/ccw: Use functions for alloc/free of the vfio_ccw_private Jason Gunthorpe 2021-09-09 19:38 ` [Intel-gfx] " Jason Gunthorpe 2021-09-10 11:27 ` Christoph Hellwig 2021-09-10 11:27 ` Christoph Hellwig 2021-09-14 15:50 ` Cornelia Huck 2021-09-14 15:50 ` [Intel-gfx] " Cornelia Huck 2021-09-14 18:03 ` Jason Gunthorpe 2021-09-14 18:03 ` [Intel-gfx] " Jason Gunthorpe 2021-09-24 2:53 ` Eric Farman 2021-09-24 2:53 ` [Intel-gfx] " Eric Farman 2021-09-09 19:38 ` [PATCH v2 2/9] vfio/ccw: Pass vfio_ccw_private not mdev_device to various functions Jason Gunthorpe 2021-09-09 19:38 ` [Intel-gfx] " Jason Gunthorpe 2021-09-10 11:32 ` Christoph Hellwig 2021-09-10 11:32 ` Christoph Hellwig 2021-09-20 11:12 ` Cornelia Huck 2021-09-20 11:12 ` [Intel-gfx] " Cornelia Huck 2021-09-24 2:53 ` Eric Farman 2021-09-24 2:53 ` [Intel-gfx] " Eric Farman 2021-09-09 19:38 ` [PATCH v2 3/9] vfio/ccw: Convert to use vfio_register_group_dev() Jason Gunthorpe 2021-09-09 19:38 ` [Intel-gfx] " Jason Gunthorpe 2021-09-24 20:37 ` Eric Farman 2021-09-24 20:37 ` [Intel-gfx] " Eric Farman 2021-09-27 12:17 ` Jason Gunthorpe 2021-09-27 12:17 ` [Intel-gfx] " Jason Gunthorpe 2021-09-09 19:38 ` [PATCH v2 4/9] vfio/ccw: Make the FSM complete and synchronize it to the mdev Jason Gunthorpe 2021-09-09 19:38 ` [Intel-gfx] " Jason Gunthorpe 2021-09-20 12:19 ` Cornelia Huck 2021-09-20 12:19 ` [Intel-gfx] " Cornelia Huck 2021-09-20 12:30 ` Jason Gunthorpe [this message] 2021-09-20 12:30 ` Jason Gunthorpe 2021-09-09 19:38 ` [PATCH v2 5/9] vfio/mdev: Consolidate all the device_api sysfs into the core code Jason Gunthorpe 2021-09-09 19:38 ` [Intel-gfx] " Jason Gunthorpe 2021-09-10 12:10 ` Christoph Hellwig 2021-09-10 12:10 ` [Intel-gfx] " Christoph Hellwig 2021-09-10 13:38 ` Jason Gunthorpe 2021-09-10 13:38 ` [Intel-gfx] " Jason Gunthorpe 2021-09-10 16:09 ` Alex Williamson 2021-09-10 16:09 ` [Intel-gfx] " Alex Williamson 2021-09-09 19:38 ` [PATCH v2 6/9] vfio/mdev: Add mdev available instance checking to the core Jason Gunthorpe 2021-09-09 19:38 ` [Intel-gfx] " Jason Gunthorpe 2021-09-10 12:25 ` Christoph Hellwig 2021-09-10 12:25 ` [Intel-gfx] " Christoph Hellwig 2021-09-20 18:02 ` Cornelia Huck 2021-09-20 18:02 ` [Intel-gfx] " Cornelia Huck 2021-09-21 13:19 ` Jason Gunthorpe 2021-09-21 13:19 ` [Intel-gfx] " Jason Gunthorpe 2021-09-24 2:54 ` Eric Farman 2021-09-24 2:54 ` [Intel-gfx] " Eric Farman 2021-09-09 19:38 ` [PATCH v2 7/9] vfio/ccw: Remove private->mdev Jason Gunthorpe 2021-09-09 19:38 ` [Intel-gfx] " Jason Gunthorpe 2021-09-24 20:45 ` Eric Farman 2021-09-24 20:45 ` [Intel-gfx] " Eric Farman 2021-09-27 12:32 ` Jason Gunthorpe 2021-09-27 12:32 ` [Intel-gfx] " Jason Gunthorpe 2021-09-27 20:45 ` Eric Farman 2021-09-27 20:45 ` [Intel-gfx] " Eric Farman 2021-09-09 19:38 ` [PATCH v2 8/9] vfio: Export vfio_device_try_get() Jason Gunthorpe 2021-09-09 19:38 ` [Intel-gfx] " Jason Gunthorpe 2021-09-09 19:38 ` [PATCH v2 9/9] vfio/ccw: Move the lifecycle of the struct vfio_ccw_private to the mdev Jason Gunthorpe 2021-09-09 19:38 ` [Intel-gfx] " Jason Gunthorpe 2021-09-09 19:54 ` [Intel-gfx] ✗ Fi.CI.BUILD: failure for Move vfio_ccw to the new mdev API Patchwork 2021-09-13 17:40 ` [PATCH v2 0/9] " Eric Farman 2021-09-13 17:40 ` [Intel-gfx] " Eric Farman 2021-09-13 19:24 ` Jason Gunthorpe 2021-09-13 19:24 ` [Intel-gfx] " Jason Gunthorpe 2021-09-13 20:31 ` Eric Farman 2021-09-13 20:31 ` [Intel-gfx] " Eric Farman 2021-09-14 13:36 ` Jason Gunthorpe 2021-09-14 13:36 ` [Intel-gfx] " Jason Gunthorpe 2021-09-17 11:59 ` Cornelia Huck 2021-09-17 11:59 ` [Intel-gfx] " Cornelia Huck 2021-09-17 12:51 ` Jason Gunthorpe 2021-09-17 12:51 ` [Intel-gfx] " Jason Gunthorpe 2021-09-17 14:37 ` Cornelia Huck 2021-09-17 14:37 ` [Intel-gfx] " Cornelia Huck 2021-09-14 13:46 ` [Intel-gfx] ✗ Fi.CI.BUILD: failure for Move vfio_ccw to the new mdev API (rev2) Patchwork 2021-09-14 18:19 ` [Intel-gfx] ✗ Fi.CI.BUILD: failure for Move vfio_ccw to the new mdev API (rev3) Patchwork 2021-09-21 15:11 ` [Intel-gfx] ✗ Fi.CI.BUILD: failure for Move vfio_ccw to the new mdev API (rev4) Patchwork
Reply instructions: You may reply publicly to this message via plain-text email using any one of the following methods: * Save the following mbox file, import it into your mail client, and reply-to-all from there: mbox Avoid top-posting and favor interleaved quoting: https://en.wikipedia.org/wiki/Posting_style#Interleaved_style * Reply using the --to, --cc, and --in-reply-to switches of git-send-email(1): git send-email \ --in-reply-to=20210920123018.GF327412@nvidia.com \ --to=jgg@nvidia.com \ --cc=airlied@linux.ie \ --cc=akrowiak@linux.ibm.com \ --cc=alex.williamson@redhat.com \ --cc=borntraeger@de.ibm.com \ --cc=cohuck@redhat.com \ --cc=daniel@ffwll.ch \ --cc=dri-devel@lists.freedesktop.org \ --cc=farman@linux.ibm.com \ --cc=freude@linux.ibm.com \ --cc=gor@linux.ibm.com \ --cc=hca@linux.ibm.com \ --cc=hch@lst.de \ --cc=intel-gfx@lists.freedesktop.org \ --cc=intel-gvt-dev@lists.freedesktop.org \ --cc=jani.nikula@linux.intel.com \ --cc=jjherne@linux.ibm.com \ --cc=joonas.lahtinen@linux.intel.com \ --cc=kvm@vger.kernel.org \ --cc=kwankhede@nvidia.com \ --cc=linux-s390@vger.kernel.org \ --cc=mjrosato@linux.ibm.com \ --cc=oberpar@linux.ibm.com \ --cc=pasic@linux.ibm.com \ --cc=rodrigo.vivi@intel.com \ --cc=vneethv@linux.ibm.com \ --cc=zhenyuw@linux.intel.com \ --cc=zhi.a.wang@intel.com \ /path/to/YOUR_REPLY https://kernel.org/pub/software/scm/git/docs/git-send-email.html * If your mail client supports setting the In-Reply-To header via mailto: links, try the mailto: linkBe sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.