diff mbox series

[RFC,v4,2/4] vfio-ccw: Check workqueue before doing START

Message ID 20210413182410.1396170-3-farman@linux.ibm.com (mailing list archive)
State New
Headers show
Series vfio-ccw: Fix interrupt handling for HALT/CLEAR | expand

Commit Message

Eric Farman April 13, 2021, 6:24 p.m. UTC
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(+)

Comments

Cornelia Huck April 15, 2021, 10:51 a.m. UTC | #1
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 :/]
Eric Farman April 15, 2021, 1:48 p.m. UTC | #2
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
Cornelia Huck April 15, 2021, 4:19 p.m. UTC | #3
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.
Eric Farman April 15, 2021, 6:42 p.m. UTC | #4
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. :(
Cornelia Huck April 16, 2021, 2:41 p.m. UTC | #5
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 mbox series

Patch

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;