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

[Eric Farman <farman@linux.ibm.com>]

On 01/25/2019 07:58 AM, Cornelia Huck wrote:
> On Fri, 25 Jan 2019 11:24:37 +0100
> Cornelia Huck <cohuck@redhat.com> wrote:
> 
>> On Thu, 24 Jan 2019 21:37:44 -0500
>> Eric Farman <farman@linux.ibm.com> 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.

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
>    state.
> - 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.

, async requests are processed. This state can
>    be removed again once we are able to handle more than one outstanding
>    cp.
> 
> 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?)
C) Interrupt received by kernel (no change?)
D) Interrupt given to userspace (BUSY->IDLE)

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 
processed)

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 ?