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/28/2019 02:15 PM, Halil Pasic wrote:
On Mon, 28 Jan 2019 18:09:48 +0100
Cornelia Huck <cohuck@xxxxxxxxxx> wrote:

On Fri, 25 Jan 2019 15:01:01 +0100
Halil Pasic <pasic@xxxxxxxxxxxxx> wrote:

On Fri, 25 Jan 2019 13:58:35 +0100
Cornelia Huck <cohuck@xxxxxxxxxx> wrote:

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

(...)

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

This reads very similar to your first point.

Not quite. ssch() means that we have a cp around; for hsch()/csch() we
don't have such a thing. So we want to protect the process of
translating the cp etc., but we don't need such protection for the
halt/clear processing.


What does this don't 'need such protection' mean in terms of code,
moving the unlock of the io_mutex upward (in
vfio_ccw_async_region_write())?

Here the function in question for reference:

+static ssize_t vfio_ccw_async_region_write(struct vfio_ccw_private
*private,
+					   const char __user *buf,
size_t count,
+					   loff_t *ppos)
+{
+	unsigned int i = VFIO_CCW_OFFSET_TO_INDEX(*ppos) -
VFIO_CCW_NUM_REGIONS;
+	loff_t pos = *ppos & VFIO_CCW_OFFSET_MASK;
+	struct ccw_cmd_region *region;
+	int ret;
+
+	if (pos + count > sizeof(*region))
+		return -EINVAL;
+
+	if (private->state == VFIO_CCW_STATE_NOT_OPER ||
+	    private->state == VFIO_CCW_STATE_STANDBY)
+		return -EACCES;
+	if (!mutex_trylock(&private->io_mutex))
+		return -EAGAIN;
+
+	region = private->region[i].data;
+	if (copy_from_user((void *)region + pos, buf, count)) {
+		ret = -EFAULT;
+		goto out_unlock;
+	}
+
+	vfio_ccw_fsm_event(private, VFIO_CCW_EVENT_ASYNC_REQ);
+
+	ret = region->ret_code ? region->ret_code : count;
+
+out_unlock:
+	mutex_unlock(&private->io_mutex);
+	return ret;
+}

That does not make much sense to me at the moment (so I guess I
misunderstood again).



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, 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?

AFAIU your idea is to split up the busy state into two states: CP_PENDING
and of busy without CP_PENDING called BUSY. I like the idea of having a
separate state for CP_PENDING but I don't like the new semantic of BUSY.

Hm mashing a conceptual state machine and the jumptabe stuff ain't
making reasoning about this simpler either. I'm taking about the
conceptual state machine. It would be nice to have a picture of it and
then think about how to express that in code.

Sorry, I'm having a hard time parsing your comments. Are you looking
for something like the below?

I had more something like this
https://en.wikipedia.org/wiki/UML_state_machine,
in mind but the lists of state transitions are also useful.


I think the picture Connie paints below is just as useful as any formalized UML diagram.


IDLE --- IO_REQ --> BUSY ---> CP_PENDING --- IRQ ---> IDLE (if final

There ain't no trigger/action list  between BUSY and CP_PENDING.

Right, because BUSY means "KVM started processing a SSCH" and CP_PENDING means "KVM finished processing the SSCH and issued it to the hardware, and got cc=0."

I'm also in the  dark about where the issuing of the ssch() happen
here (is it an internal transition within CP_PENDING?).

Connie said...

>>>> - A new state (CP_PENDING?) is entered after ssch returned with cc 0
>>>>    (from the BUSY state).

...and I agree with that.

I guess if
the ssch() returns with non cc == 0 the CP_PENDING ---IRQ---> IDLE
transition
won't take place. And I guess the IRQ is a final one.

Yes this is the one point I hadn't seen explicitly stated. We shouldn't remain in state=BUSY if the ssch got cc!=0, and probably return to IDLE when processing the failure. In Connie's response (Mon, 28 Jan 2019 18:24:24 +0100) to my note, she expressed some agreement to that.


Sorry abstraction is not a concept unknown to me. But this is too much
abstraction for me in this context. The devil is in the details, and
AFAIU we are discussing these details right now.
state for I/O)
(normal ssch)

BUSY --- IO_REQ ---> return -EAGAIN, stay in BUSY
(user space is supposed to retry, as we'll eventually progress from
BUSY)

CP_PENDING --- IO_REQ ---> return -EBUSY, stay in CP_PENDING
(user space is supposed to map this to the appropriate cc for the guest)

 From this it seems you don't intend to issue the second  requested ssch()
any more (and don't want to do any translation). Is that right? (If it
is, that what I was asking for for a while, but then it's a pity for the
retries.)


IDLE --- ASYNC_REQ ---> IDLE
(user space is welcome to do anything else right away)

Your idea is to not issue a requested hsch() if we think we are IDLE
it seems. Do I understand this right? We would end up with a different
semantic for hsch()/and csch() (compared to PoP) in the guest with this
(AFAICT).


BUSY --- ASYNC_REQ ---> return -EAGAIN, stay in BUSY
(user space is supposed to retry, as above)

CP_PENDING --- ASYNC_REQ ---> return success, stay in CP_PENDING
(the interrupt will get us out of CP_PENDING eventually)

Issue (c|h)sch() is an action that is done on this internal
transition (within CP_PENDING).

These three do read like CSCH/HSCH are subject to the same rules as SSCH, when in fact they would be (among other reasons) issued to clean up a lost interrupt from a previous SSCH. So maybe return -EAGAIN on state=BUSY (don't race ourselves with the start), but issue to hardware if CP_PENDING.

If we get an async request when state=IDLE, then maybe just issue it for fun, as if it were an SSCH?


Thank you very much for investing into this description of the state
machine. I'm afraid I'm acting like a not so nice person (self censored)
at the moment. I can't help myself, sorry. Maybe Farhan and Eric can take
this as a starting point and come up with something that we can integrate
into our documentation. Maybe not...

Regards,
Halil





[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