| Message ID | 20210413182410.1396170-3-farman@linux.ibm.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | vfio-ccw: Fix interrupt handling for HALT/CLEAR | expand |
On Tue, 13 Apr 2021 20:24:08 +0200 Eric Farman <farman@linux.ibm.com> wrote: > When an interrupt is received via the IRQ, the bulk of the work > is stacked on a workqueue for later processing. Which means that > a concurrent START or HALT/CLEAR operation (via the async_region) > will race with this process and require some serialization. > > Once we have all our locks acquired, let's just look to see if we're > in a window where the process has been started from the IRQ, but not > yet picked up by vfio-ccw to clean up an I/O. If there is, mark the > request as BUSY so it can be redriven. > > Signed-off-by: Eric Farman <farman@linux.ibm.com> > --- > drivers/s390/cio/vfio_ccw_fsm.c | 5 +++++ > 1 file changed, 5 insertions(+) > > diff --git a/drivers/s390/cio/vfio_ccw_fsm.c b/drivers/s390/cio/vfio_ccw_fsm.c > index 23e61aa638e4..92d638f10b27 100644 > --- a/drivers/s390/cio/vfio_ccw_fsm.c > +++ b/drivers/s390/cio/vfio_ccw_fsm.c > @@ -28,6 +28,11 @@ static int fsm_io_helper(struct vfio_ccw_private *private) > > spin_lock_irqsave(sch->lock, flags); > > + if (work_pending(&private->io_work)) { > + ret = -EBUSY; > + goto out; > + } > + > orb = cp_get_orb(&private->cp, (u32)(addr_t)sch, sch->lpm); > if (!orb) { > ret = -EIO; I'm wondering what condition we can consider this situation equivalent to. I'd say that the virtual version of the subchannel is basically status pending already, even though userspace may not have retrieved that information yet; so probably cc 1? Following the code path further along, it seems we return -EBUSY both for cc 1 and cc 2 conditions we receive from the device (i.e. they are not distinguishable from userspace). I don't think we can change that, as it is an existing API (QEMU maps -EBUSY to cc 2.) So this change looks fine so far. I'm wondering what we should do for hsch. We probably want to return -EBUSY for a pending condition as well, if I read the PoP correctly... the only problem is that QEMU seems to match everything to 0; but that is arguably not the kernel's problem. For clear, we obviously don't have busy conditions. Should we clean up any pending conditions? [It feels like we have discussed this before, but any information has vanished from my cache :/]
On Thu, 2021-04-15 at 12:51 +0200, Cornelia Huck wrote: > On Tue, 13 Apr 2021 20:24:08 +0200 > Eric Farman <farman@linux.ibm.com> wrote: > > > When an interrupt is received via the IRQ, the bulk of the work > > is stacked on a workqueue for later processing. Which means that > > a concurrent START or HALT/CLEAR operation (via the async_region) > > will race with this process and require some serialization. > > > > Once we have all our locks acquired, let's just look to see if > > we're > > in a window where the process has been started from the IRQ, but > > not > > yet picked up by vfio-ccw to clean up an I/O. If there is, mark the > > request as BUSY so it can be redriven. > > > > Signed-off-by: Eric Farman <farman@linux.ibm.com> > > --- > > drivers/s390/cio/vfio_ccw_fsm.c | 5 +++++ > > 1 file changed, 5 insertions(+) > > > > diff --git a/drivers/s390/cio/vfio_ccw_fsm.c > > b/drivers/s390/cio/vfio_ccw_fsm.c > > index 23e61aa638e4..92d638f10b27 100644 > > --- a/drivers/s390/cio/vfio_ccw_fsm.c > > +++ b/drivers/s390/cio/vfio_ccw_fsm.c > > @@ -28,6 +28,11 @@ static int fsm_io_helper(struct vfio_ccw_private > > *private) > > > > spin_lock_irqsave(sch->lock, flags); > > > > + if (work_pending(&private->io_work)) { > > + ret = -EBUSY; > > + goto out; > > + } > > + > > orb = cp_get_orb(&private->cp, (u32)(addr_t)sch, sch->lpm); > > if (!orb) { > > ret = -EIO; > > I'm wondering what condition we can consider this situation > equivalent > to. I'd say that the virtual version of the subchannel is basically > status pending already, even though userspace may not have retrieved > that information yet; so probably cc 1? Yes, I guess cc1 is a more natural fit, since there is status pending rather than an active start/halt/clear that would expect get the cc2. > > Following the code path further along, it seems we return -EBUSY both > for cc 1 and cc 2 conditions we receive from the device (i.e. they > are > not distinguishable from userspace). Yeah. :/ > I don't think we can change that, > as it is an existing API (QEMU maps -EBUSY to cc 2.) So this change > looks fine so far. > > I'm wondering what we should do for hsch. We probably want to return > -EBUSY for a pending condition as well, if I read the PoP > correctly... Ah, yes... I agree that to maintain parity with ssch and pops, the same cc1/-EBUSY would be applicable here. Will make that change in next version. > the only problem is that QEMU seems to match everything to 0; but > that > is arguably not the kernel's problem. > > For clear, we obviously don't have busy conditions. Should we clean > up > any pending conditions? By doing anything other than issuing the csch to the subchannel? I don't think so, that should be more than enough to get the css and vfio-ccw in sync with each other. > > [It feels like we have discussed this before, but any information has > vanished from my cache :/] > It has vanished from mine too, and looking over the old threads and notes doesn't page anything useful in, so here we are. Sorry. :( Eric
On Thu, 15 Apr 2021 09:48:37 -0400 Eric Farman <farman@linux.ibm.com> wrote: > On Thu, 2021-04-15 at 12:51 +0200, Cornelia Huck wrote: > > I'm wondering what we should do for hsch. We probably want to return > > -EBUSY for a pending condition as well, if I read the PoP > > correctly... > > Ah, yes... I agree that to maintain parity with ssch and pops, the > same cc1/-EBUSY would be applicable here. Will make that change in next > version. Yes, just to handle things in the same fashion consistently. > > > the only problem is that QEMU seems to match everything to 0; but > > that > > is arguably not the kernel's problem. > > > > For clear, we obviously don't have busy conditions. Should we clean > > up > > any pending conditions? > > By doing anything other than issuing the csch to the subchannel? I > don't think so, that should be more than enough to get the css and > vfio-ccw in sync with each other. Hm, doesn't a successful csch clear any status pending? That would mean that invoking our csch backend implies that we won't deliver the status pending that is already pending via the workqueue, which therefore needs to be flushed out in some way? I remember we did some special csch handling, but I don't immediately see where; might have been only in QEMU.
On Thu, 2021-04-15 at 18:19 +0200, Cornelia Huck wrote: > On Thu, 15 Apr 2021 09:48:37 -0400 > Eric Farman <farman@linux.ibm.com> wrote: > > > On Thu, 2021-04-15 at 12:51 +0200, Cornelia Huck wrote: > > > I'm wondering what we should do for hsch. We probably want to > > > return > > > -EBUSY for a pending condition as well, if I read the PoP > > > correctly... > > > > Ah, yes... I agree that to maintain parity with ssch and pops, the > > same cc1/-EBUSY would be applicable here. Will make that change in > > next > > version. > > Yes, just to handle things in the same fashion consistently. > > > > the only problem is that QEMU seems to match everything to 0; but > > > that > > > is arguably not the kernel's problem. > > > > > > For clear, we obviously don't have busy conditions. Should we > > > clean > > > up > > > any pending conditions? > > > > By doing anything other than issuing the csch to the subchannel? I > > don't think so, that should be more than enough to get the css and > > vfio-ccw in sync with each other. > > Hm, doesn't a successful csch clear any status pending? Yep. > That would mean > that invoking our csch backend implies that we won't deliver the > status > pending that is already pending via the workqueue, which therefore > needs to be flushed out in some way? Ah, so I misunderstood the direction you were going... I'm not aware of a way to "purge" items from a workqueue, as the flush_workqueue() routine is documented as picking them off and running them. Perhaps an atomic flag in (private? cp?) that causes vfio_ccw_sch_io_todo() to just exit rather than doing all its stuff? > I remember we did some special > csch handling, but I don't immediately see where; might have been > only > in QEMU. > Maybe. I don't see anything jumping out at me though. :(
On Thu, 15 Apr 2021 14:42:21 -0400 Eric Farman <farman@linux.ibm.com> wrote: > On Thu, 2021-04-15 at 18:19 +0200, Cornelia Huck wrote: > > On Thu, 15 Apr 2021 09:48:37 -0400 > > Eric Farman <farman@linux.ibm.com> wrote: > > > > > On Thu, 2021-04-15 at 12:51 +0200, Cornelia Huck wrote: > > > > I'm wondering what we should do for hsch. We probably want to > > > > return > > > > -EBUSY for a pending condition as well, if I read the PoP > > > > correctly... > > > > > > Ah, yes... I agree that to maintain parity with ssch and pops, the > > > same cc1/-EBUSY would be applicable here. Will make that change in > > > next > > > version. > > > > Yes, just to handle things in the same fashion consistently. > > > > > > the only problem is that QEMU seems to match everything to 0; but > > > > that > > > > is arguably not the kernel's problem. > > > > > > > > For clear, we obviously don't have busy conditions. Should we > > > > clean > > > > up > > > > any pending conditions? > > > > > > By doing anything other than issuing the csch to the subchannel? I > > > don't think so, that should be more than enough to get the css and > > > vfio-ccw in sync with each other. > > > > Hm, doesn't a successful csch clear any status pending? > > Yep. > > > That would mean > > that invoking our csch backend implies that we won't deliver the > > status > > pending that is already pending via the workqueue, which therefore > > needs to be flushed out in some way? > > Ah, so I misunderstood the direction you were going... I'm not aware of > a way to "purge" items from a workqueue, as the flush_workqueue() > routine is documented as picking them off and running them. > > Perhaps an atomic flag in (private? cp?) that causes > vfio_ccw_sch_io_todo() to just exit rather than doing all its stuff? Yes, maybe something like that. Maybe we should do that on top once we have a good idea, if the current series already fixes the problems that are actually happening now and then. > > > I remember we did some special > > csch handling, but I don't immediately see where; might have been > > only > > in QEMU. > > > > Maybe. I don't see anything jumping out at me though. :( I might have misremembered; it only really applies to passthrough, as emulated subchannels are handled synchronously anyway.
diff --git a/drivers/s390/cio/vfio_ccw_fsm.c b/drivers/s390/cio/vfio_ccw_fsm.c index 23e61aa638e4..92d638f10b27 100644 --- a/drivers/s390/cio/vfio_ccw_fsm.c +++ b/drivers/s390/cio/vfio_ccw_fsm.c @@ -28,6 +28,11 @@ static int fsm_io_helper(struct vfio_ccw_private *private) spin_lock_irqsave(sch->lock, flags); + if (work_pending(&private->io_work)) { + ret = -EBUSY; + goto out; + } + orb = cp_get_orb(&private->cp, (u32)(addr_t)sch, sch->lpm); if (!orb) { ret = -EIO;
When an interrupt is received via the IRQ, the bulk of the work is stacked on a workqueue for later processing. Which means that a concurrent START or HALT/CLEAR operation (via the async_region) will race with this process and require some serialization. Once we have all our locks acquired, let's just look to see if we're in a window where the process has been started from the IRQ, but not yet picked up by vfio-ccw to clean up an I/O. If there is, mark the request as BUSY so it can be redriven. Signed-off-by: Eric Farman <farman@linux.ibm.com> --- drivers/s390/cio/vfio_ccw_fsm.c | 5 +++++ 1 file changed, 5 insertions(+)