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

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

 





On 01/25/2019 07:58 AM, Cornelia Huck wrote:
On Fri, 25 Jan 2019 11:24:37 +0100
Cornelia Huck <cohuck@xxxxxxxxxx> wrote:

On Thu, 24 Jan 2019 21:37:44 -0500
Eric Farman <farman@xxxxxxxxxxxxx> 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 ?




[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
[Index of Archives]     [Kernel Development]     [Kernel Newbies]     [IDE]     [Security]     [Git]     [Netfilter]     [Bugtraq]     [Yosemite Info]     [MIPS Linux]     [ARM Linux]     [Linux Security]     [Linux RAID]     [Linux ATA RAID]     [Samba]     [Linux Media]     [Device Mapper]

  Powered by Linux