On 5/18/20 12:09 PM, Cornelia Huck wrote: > On Wed, 13 May 2020 16:29:30 +0200 > Eric Farman <farman@xxxxxxxxxxxxx> wrote: > >> Hi Conny, >> >> Back in January, I suggested a small patch [1] to try to clean up >> the handling of HSCH/CSCH interrupts, especially as it relates to >> concurrent SSCH interrupts. Here is a new attempt to address this. >> >> There was some suggestion earlier about locking the FSM, but I'm not >> seeing any problems with that. Rather, what I'm noticing is that the >> flow between a synchronous START and asynchronous HALT/CLEAR have >> different impacts on the FSM state. Consider: >> >> CPU 1 CPU 2 >> >> SSCH (set state=CP_PENDING) >> INTERRUPT (set state=IDLE) >> CSCH (no change in state) >> SSCH (set state=CP_PENDING) > > This is the transition I do not understand. When we get a request via > the I/O area, we go to CP_PROCESSING and start doing translations. > However, we only transition to CP_PENDING if we actually do a SSCH with > cc 0 -- which shouldn't be possible in the flow you outline... unless > it really is something that can be taken care of with locking (state > machine transitioning due to an interrupt without locking, so we go to > IDLE without other parts noticing.) I'm only going by what the (existing and my temporary) tea leaves in s390dbf are telling us. :) > >> INTERRUPT (set state=IDLE) Part of the problem is that this is actually comprised of these elements: if (irb_is_final && state == CP_PENDING) cp_free() lock io_mutex copy irb to io_region unlock io_mutex if (irb_is_final) state = IDLE The CP_PENDING check will protect us if a SSCH is still being built at the time we execute this code. But if we got to CP_PENDING first (between fsm_irq() stacking to the workqueue and us unstacking vfio_ccw_sch_io_todo()), we would free an unrelated operation. (This was the scenario in the first version of my fix back in January.) We can't add a CP_PENDING check after the io_mutex barrier, because if a second SSCH is being processed, we will hang on the lock acquisition and will DEFINITELY be in CP_PENDING state when we come back. But by that point, we will have skipped freeing the (now active) CP but are back in an IDLE state. >> INTERRUPT (set state=IDLE) > > But taking a step back (and ignoring your series and the discussion, > sorry about that): No apologies necessary. > > We need to do something (creating a local translation of the guest's > channel program) that does not have any relation to the process in the > architecture at all, but is only something that needs to be done > because of what vfio-ccw is trying to do (issuing a channel program on > behalf of another entity.) Trying to sort that out by poking at actl > and fctl bits does not seem like the best way; especially as keeping > the bits up-to-date via STSCH is an exercise in futility. I am coming to strongly agree with this sentiment. > > What about the following (and yes, I had suggested something vaguely in > that direction before): > > - Detach the cp from the subchannel (or better, remove the 1:1 > relationship). By that I mean building the cp as a separately > allocated structure (maybe embedding a kref, but that might not be > needed), and appending it to a list after SSCH with cc=0. Discard it > if cc!=0. > - Remove the CP_PENDING state. The state is either IDLE after any > successful SSCH/HSCH/CSCH, or a new state in that case. But no > special state for SSCH. > - A successful CSCH removes the first queued request, if any. > - A final interrupt removes the first queued request, if any. > > Thoughts? > I'm cautiously optimistic, for exactly the reason I mention above. If we always expect to be in IDLE state once an interrupt arrives, we can just rely on determining if the interrupt is in relation to an actual operation we're waiting on. I'll give this a try and report back.
