[Top][All Lists]

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [qemu-s390x] [PATCH v2 2/5] vfio-ccw: concurrent I/O handling

From: Eric Farman
Subject: Re: [qemu-s390x] [PATCH v2 2/5] vfio-ccw: concurrent I/O handling
Date: Mon, 28 Jan 2019 16:50:43 -0500
User-agent: Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101 Thunderbird/52.9.1

On 01/28/2019 12:24 PM, Cornelia Huck wrote:
On Fri, 25 Jan 2019 10:57:38 -0500
Eric Farman <address@hidden> wrote:

On 01/25/2019 07:58 AM, Cornelia Huck wrote:
On Fri, 25 Jan 2019 11:24:37 +0100
Cornelia Huck <address@hidden> wrote:
On Thu, 24 Jan 2019 21:37:44 -0500
Eric Farman <address@hidden> wrote:
On 01/24/2019 09:25 PM, Eric Farman wrote:

On 01/21/2019 06:03 AM, Cornelia Huck wrote:
[1] I think these changes are cool.  We end up going into (and staying
in) state=BUSY if we get cc=0 on the SSCH, rather than in/out as we
bumble along.

But why can't these be separated out from this patch?  It does change
the behavior of the state machine, and seem distinct from the addition
of the mutex you otherwise add here?  At the very least, this behavior
change should be documented in the commit since it's otherwise lost in
the mutex/EAGAIN stuff.

That's a very good idea. I'll factor them out into a separate patch.

And now that I've factored it out, I noticed some more problems.

That's good!  Maybe it helps us with the circles we're on :)


What we basically need is the following, I think:

- The code should not be interrupted while we process the channel
    program, do the ssch etc. We want the caller to try again later (i.e.
    return -EAGAIN)
- We currently do not want the user space to submit another channel
    program while the first one is still in flight.

These two seem to contradict one another.  I think you're saying is that
we don't _want_ userspace to issue another channel program, even though
its _allowed_ to as far as vfio-ccw is concerned.

What I'm trying to say is that we want to distinguish two things:
- The code is currently doing translation etc. We probably want to keep
   that atomic, in order not to make things too complicated.
- We have sent the ssch() to the hardware, but have not yet received
   the final interrupt for that request (that's what I meant with "in
   flight"). It's easier for the first shot to disallow a second ssch()
   as that would need handling of more than one cp request, but we may
   want to allow it in the future.
   A hsch()/csch() (which does not generate a new cp) should be fine.

(see also my reply to Halil's mail)

As submitting another
    one is a valid request, however, we should allow this in the future
    (once we have the code to handle that in place).
- With the async interface, we want user space to be able to submit a
    halt/clear while a start request is still in flight, but not while
    we're processing a start request with translation etc. We probably
    want to do -EAGAIN in that case.

My idea would be:

- The BUSY state denotes "I'm busy processing a request right now, try
    again". We hold it while processing the cp and doing the ssch and
    leave it afterwards (i.e., while the start request is processed by
    the hardware). I/O requests and async requests get -EAGAIN in that
- A new state (CP_PENDING?) is entered after ssch returned with cc 0
    (from the BUSY state). We stay in there as long as no final state for
    that request has been received and delivered. (This may be final
    interrupt for that request, a deferred cc, or successful halt/clear.)
    I/O requests get -EBUSY

I liked CP_PENDING, since it corresponds to the subchannel being marked
"start pending" as described in POPS, but this statement suggests that
the BUSY/PENDING state to be swapped, such that state=PENDING returns
-EAGAIN and state=BUSY returns -EBUSY.  Not super-concerned with the
terminology though.

What about s/BUSY/CP_PROCESSING/ ?

So we go IDLE -> CP_PROCESSING -> CP_PENDING -> (IRQ) -> IDLE right? Seems good to me.

, async requests are processed. This state can
    be removed again once we are able to handle more than one outstanding

Does that make sense?

I think so, and I think I like it.  So you want to distinguish between
(I have swapped BUSY/PENDING in this example per my above comment):

A) SSCH issued by userspace (IDLE->PENDING)
B) SSCH issued (successfully) by kernel (PENDING->BUSY)
B') SSCH issued (unsuccessfully) by kernel (PENDING->IDLE?)

I think so.

C) Interrupt received by kernel (no change?)
D) Interrupt given to userspace (BUSY->IDLE)

Only if that is the final interrupt for that cp.


If we receive A and A, the second A gets EAGAIN

If we do A+B and A, the second A gets EBUSY (unless async, which is


Does the boundary of "in flight" in the interrupt side (C and D) need to
be defined, such that we go BUSY->PENDING->IDLE instead of BUSY->IDLE ?

I don't think we can go BUSY->PENDING (in your terminology), at that
would imply a retry of the ssch()?

I didn't think so, but figured it's worth asking while we're already confused. :)

reply via email to

[Prev in Thread] Current Thread [Next in Thread]