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

From: Eric Farman <farman@linux.ibm.com>
To: Cornelia Huck <cohuck@redhat.com>
Cc: linux-s390@vger.kernel.org,
	Alex Williamson <alex.williamson@redhat.com>,
	Pierre Morel <pmorel@linux.ibm.com>,
	kvm@vger.kernel.org, Farhan Ali <alifm@linux.ibm.com>,
	qemu-devel@nongnu.org, Halil Pasic <pasic@linux.ibm.com>,
	qemu-s390x@nongnu.org
Subject: Re: [PATCH v2 2/5] vfio-ccw: concurrent I/O handling
Date: Mon, 28 Jan 2019 16:50:43 -0500	[thread overview]
Message-ID: <10b31945-c8d1-865c-3e31-a27bc0245907@linux.ibm.com> (raw)
In-Reply-To: <20190128182424.3ad3d94d.cohuck@redhat.com>

On 01/28/2019 12:24 PM, Cornelia Huck wrote:
> On Fri, 25 Jan 2019 10:57:38 -0500
> Eric Farman <farman@linux.ibm.com> wrote:
> 
>> 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.
> 
> 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
>>>     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.
> 
> 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
>>>     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?)
> 
> 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.

Agreed.

> 
>>
>> 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)
> 
> Nod.
> 
>> 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.  :)