| Message ID | 20200616195053.99253-1-farman@linux.ibm.com (mailing list archive) |
|---|---|
| Headers | show |
| Series | vfio-ccw: Fix interrupt handling for HALT/CLEAR | expand |
On 6/16/20 3:50 PM, Eric Farman wrote: > Let's continue our discussion of the handling of vfio-ccw interrupts. > > The initial fix [1] relied upon the interrupt path's examination of the > FSM state, and freeing all resources if it were CP_PENDING. But the > interface used by HALT/CLEAR SUBCHANNEL doesn't affect the FSM state. > Consider this sequence: > > CPU 1 CPU 2 > CLEAR (state=IDLE/no change) > START [2] > INTERRUPT (set state=IDLE) > INTERRUPT (set state=IDLE) > > This translates to a couple of possible scenarios: > > A) The START gets a cc2 because of the outstanding CLEAR, -EBUSY is > returned, resources are freed, and state remains IDLE > B) The START gets a cc0 because the CLEAR has already presented an > interrupt, and state is set to CP_PENDING > > If the START gets a cc0 before the CLEAR INTERRUPT (stacked onto a > workqueue by the IRQ context) gets a chance to run, then the INTERRUPT > will release the channel program memory prematurely. If the two > operations run concurrently, then the FSM state set to CP_PROCESSING > will prevent the cp_free() from being invoked. But the io_mutex > boundary on that path will pause itself until the START completes, > and then allow the FSM to be reset to IDLE without considering the > outstanding START. Neither scenario would be considered good. > > Having said all of that, in v2 Conny suggested [3] the following: > >> - 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. > > What I have implemented here is basically this, with a few changes: > > - I don't queue cp's. Since there should only be one START in process > at a time, and HALT/CLEAR doesn't build a cp, I didn't see a pressing > need to introduce that complexity. > - Furthermore, while I initially made a separately allocated cp, adding > an alloc for a cp on each I/O AND moving the guest_cp alloc from the > probe path to the I/O path seems excessive. So I implemented a > "started" flag to the cp, set after a cc0 from the START, and examine > that on the interrupt path to determine whether cp_free() is needed. FYI... After a day or two of running, I sprung a kernel debug oops for list corruption in ccwchain_free(). I'm going to blame this piece, since it was the last thing I changed and I hadn't come across any such damage since v2. So either "started" is a bad idea, or a broken one. Or both. :) > - I opted against a "SOMETHING_PENDING" state if START/HALT/CLEAR > got a cc0, and just put the FSM back to IDLE. It becomes too unwieldy > to discern which operation an interrupt is completing, and whether > more interrupts are expected, to be worth the additional state. > - A successful CSCH doesn't do anything special, and cp_free() > is only performed on the interrupt path. Part of me wrestled with > how a HALT fits into that, but mostly it was that a cc0 on any > of the instructions indicated the "channel subsystem is signaled > to asynchronously perform the [START/HALT/CLEAR] function." > This means that an in-flight START could still receive data from the > device/subchannel, so not a good idea to release memory at that point. > > Separate from all that, I added a small check of the io_work queue to > the FSM START path. Part of the problems I've seen was that an interrupt > is presented by a CPU, but not yet processed by vfio-ccw. Some of the > problems seen thus far is because of this gap, and the above changes > don't address that either. Whether this is appropriate or ridiculous > would be a welcome discussion. > > Previous versions: > v2: https://lore.kernel.org/kvm/20200513142934.28788-1-farman@linux.ibm.com/ > v1: https://lore.kernel.org/kvm/20200124145455.51181-1-farman@linux.ibm.com/ > > Footnotes: > [1] https://lore.kernel.org/kvm/62e87bf67b38dc8d5760586e7c96d400db854ebe.1562854091.git.alifm@linux.ibm.com/ > [2] Halil has pointed out that QEMU should prohibit this, based on the > rules set forth by the POPs. This is true, but we should not rely on > it behaving properly without addressing this scenario that is visible > today. Once I get this behaving correctly, I'll spend some time > seeing if QEMU is misbehaving somehow. > [3] https://lore.kernel.org/kvm/20200518180903.7cb21dd8.cohuck@redhat.com/ > [4] https://lore.kernel.org/kvm/a52368d3-8cec-7b99-1587-25e055228b62@linux.ibm.com/ > > Eric Farman (3): > vfio-ccw: Indicate if a channel_program is started > vfio-ccw: Remove the CP_PENDING FSM state > vfio-ccw: Check workqueue before doing START > > drivers/s390/cio/vfio_ccw_cp.c | 2 ++ > drivers/s390/cio/vfio_ccw_cp.h | 1 + > drivers/s390/cio/vfio_ccw_drv.c | 5 +---- > drivers/s390/cio/vfio_ccw_fsm.c | 32 +++++++++++++++++------------ > drivers/s390/cio/vfio_ccw_ops.c | 3 +-- > drivers/s390/cio/vfio_ccw_private.h | 1 - > 6 files changed, 24 insertions(+), 20 deletions(-) >
On Tue, 16 Jun 2020 21:50:50 +0200 Eric Farman <farman@linux.ibm.com> wrote: > Let's continue our discussion of the handling of vfio-ccw interrupts. > > The initial fix [1] relied upon the interrupt path's examination of the > FSM state, and freeing all resources if it were CP_PENDING. But the > interface used by HALT/CLEAR SUBCHANNEL doesn't affect the FSM state. > Consider this sequence: > > CPU 1 CPU 2 > CLEAR (state=IDLE/no change) > START [2] > INTERRUPT (set state=IDLE) > INTERRUPT (set state=IDLE) > > This translates to a couple of possible scenarios: > > A) The START gets a cc2 because of the outstanding CLEAR, -EBUSY is > returned, resources are freed, and state remains IDLE > B) The START gets a cc0 because the CLEAR has already presented an > interrupt, and state is set to CP_PENDING > > If the START gets a cc0 before the CLEAR INTERRUPT (stacked onto a > workqueue by the IRQ context) gets a chance to run, then the INTERRUPT > will release the channel program memory prematurely. If the two > operations run concurrently, then the FSM state set to CP_PROCESSING > will prevent the cp_free() from being invoked. But the io_mutex > boundary on that path will pause itself until the START completes, > and then allow the FSM to be reset to IDLE without considering the > outstanding START. Neither scenario would be considered good. > > Having said all of that, in v2 Conny suggested [3] the following: > > > - 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. > > What I have implemented here is basically this, with a few changes: > > - I don't queue cp's. Since there should only be one START in process > at a time, and HALT/CLEAR doesn't build a cp, I didn't see a pressing > need to introduce that complexity. > - Furthermore, while I initially made a separately allocated cp, adding > an alloc for a cp on each I/O AND moving the guest_cp alloc from the > probe path to the I/O path seems excessive. So I implemented a > "started" flag to the cp, set after a cc0 from the START, and examine > that on the interrupt path to determine whether cp_free() is needed. > - I opted against a "SOMETHING_PENDING" state if START/HALT/CLEAR > got a cc0, and just put the FSM back to IDLE. It becomes too unwieldy > to discern which operation an interrupt is completing, and whether > more interrupts are expected, to be worth the additional state. > - A successful CSCH doesn't do anything special, and cp_free() > is only performed on the interrupt path. Part of me wrestled with > how a HALT fits into that, but mostly it was that a cc0 on any > of the instructions indicated the "channel subsystem is signaled > to asynchronously perform the [START/HALT/CLEAR] function." > This means that an in-flight START could still receive data from the > device/subchannel, so not a good idea to release memory at that point. Hm, csch clears any pending status, so it is indeed special in a way. If we do a csch with cc 0, we already know for sure that we won't get any further status other than an interrupt indicating that clear has been done. This was my reasoning behind csch dequeuing the request. > > Separate from all that, I added a small check of the io_work queue to > the FSM START path. Part of the problems I've seen was that an interrupt > is presented by a CPU, but not yet processed by vfio-ccw. Some of the > problems seen thus far is because of this gap, and the above changes > don't address that either. Whether this is appropriate or ridiculous > would be a welcome discussion. > > Previous versions: > v2: https://lore.kernel.org/kvm/20200513142934.28788-1-farman@linux.ibm.com/ > v1: https://lore.kernel.org/kvm/20200124145455.51181-1-farman@linux.ibm.com/ > > Footnotes: > [1] https://lore.kernel.org/kvm/62e87bf67b38dc8d5760586e7c96d400db854ebe.1562854091.git.alifm@linux.ibm.com/ > [2] Halil has pointed out that QEMU should prohibit this, based on the > rules set forth by the POPs. This is true, but we should not rely on > it behaving properly without addressing this scenario that is visible > today. Once I get this behaving correctly, I'll spend some time > seeing if QEMU is misbehaving somehow. > [3] https://lore.kernel.org/kvm/20200518180903.7cb21dd8.cohuck@redhat.com/ > [4] https://lore.kernel.org/kvm/a52368d3-8cec-7b99-1587-25e055228b62@linux.ibm.com/ > > Eric Farman (3): > vfio-ccw: Indicate if a channel_program is started > vfio-ccw: Remove the CP_PENDING FSM state > vfio-ccw: Check workqueue before doing START > > drivers/s390/cio/vfio_ccw_cp.c | 2 ++ > drivers/s390/cio/vfio_ccw_cp.h | 1 + > drivers/s390/cio/vfio_ccw_drv.c | 5 +---- > drivers/s390/cio/vfio_ccw_fsm.c | 32 +++++++++++++++++------------ > drivers/s390/cio/vfio_ccw_ops.c | 3 +-- > drivers/s390/cio/vfio_ccw_private.h | 1 - > 6 files changed, 24 insertions(+), 20 deletions(-) >
On Wed, 17 Jun 2020 07:24:17 -0400 Eric Farman <farman@linux.ibm.com> wrote: > On 6/16/20 3:50 PM, Eric Farman wrote: > > Let's continue our discussion of the handling of vfio-ccw interrupts. > > > > The initial fix [1] relied upon the interrupt path's examination of the > > FSM state, and freeing all resources if it were CP_PENDING. But the > > interface used by HALT/CLEAR SUBCHANNEL doesn't affect the FSM state. > > Consider this sequence: > > > > CPU 1 CPU 2 > > CLEAR (state=IDLE/no change) > > START [2] > > INTERRUPT (set state=IDLE) > > INTERRUPT (set state=IDLE) > > > > This translates to a couple of possible scenarios: > > > > A) The START gets a cc2 because of the outstanding CLEAR, -EBUSY is > > returned, resources are freed, and state remains IDLE > > B) The START gets a cc0 because the CLEAR has already presented an > > interrupt, and state is set to CP_PENDING > > > > If the START gets a cc0 before the CLEAR INTERRUPT (stacked onto a > > workqueue by the IRQ context) gets a chance to run, then the INTERRUPT > > will release the channel program memory prematurely. If the two > > operations run concurrently, then the FSM state set to CP_PROCESSING > > will prevent the cp_free() from being invoked. But the io_mutex > > boundary on that path will pause itself until the START completes, > > and then allow the FSM to be reset to IDLE without considering the > > outstanding START. Neither scenario would be considered good. > > > > Having said all of that, in v2 Conny suggested [3] the following: > > > >> - 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. > > > > What I have implemented here is basically this, with a few changes: > > > > - I don't queue cp's. Since there should only be one START in process > > at a time, and HALT/CLEAR doesn't build a cp, I didn't see a pressing > > need to introduce that complexity. > > - Furthermore, while I initially made a separately allocated cp, adding > > an alloc for a cp on each I/O AND moving the guest_cp alloc from the > > probe path to the I/O path seems excessive. So I implemented a > > "started" flag to the cp, set after a cc0 from the START, and examine > > that on the interrupt path to determine whether cp_free() is needed. > > FYI... After a day or two of running, I sprung a kernel debug oops for > list corruption in ccwchain_free(). I'm going to blame this piece, since > it was the last thing I changed and I hadn't come across any such damage > since v2. So either "started" is a bad idea, or a broken one. Or both. :) Have you come to any conclusion wrt 'started'? Not wanting to generate stress, just asking :)
On 6/29/20 10:56 AM, Cornelia Huck wrote: > On Wed, 17 Jun 2020 07:24:17 -0400 > Eric Farman <farman@linux.ibm.com> wrote: > >> On 6/16/20 3:50 PM, Eric Farman wrote: >>> Let's continue our discussion of the handling of vfio-ccw interrupts. >>> >>> The initial fix [1] relied upon the interrupt path's examination of the >>> FSM state, and freeing all resources if it were CP_PENDING. But the >>> interface used by HALT/CLEAR SUBCHANNEL doesn't affect the FSM state. >>> Consider this sequence: >>> >>> CPU 1 CPU 2 >>> CLEAR (state=IDLE/no change) >>> START [2] >>> INTERRUPT (set state=IDLE) >>> INTERRUPT (set state=IDLE) >>> >>> This translates to a couple of possible scenarios: >>> >>> A) The START gets a cc2 because of the outstanding CLEAR, -EBUSY is >>> returned, resources are freed, and state remains IDLE >>> B) The START gets a cc0 because the CLEAR has already presented an >>> interrupt, and state is set to CP_PENDING >>> >>> If the START gets a cc0 before the CLEAR INTERRUPT (stacked onto a >>> workqueue by the IRQ context) gets a chance to run, then the INTERRUPT >>> will release the channel program memory prematurely. If the two >>> operations run concurrently, then the FSM state set to CP_PROCESSING >>> will prevent the cp_free() from being invoked. But the io_mutex >>> boundary on that path will pause itself until the START completes, >>> and then allow the FSM to be reset to IDLE without considering the >>> outstanding START. Neither scenario would be considered good. >>> >>> Having said all of that, in v2 Conny suggested [3] the following: >>> >>>> - 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. >>> >>> What I have implemented here is basically this, with a few changes: >>> >>> - I don't queue cp's. Since there should only be one START in process >>> at a time, and HALT/CLEAR doesn't build a cp, I didn't see a pressing >>> need to introduce that complexity. >>> - Furthermore, while I initially made a separately allocated cp, adding >>> an alloc for a cp on each I/O AND moving the guest_cp alloc from the >>> probe path to the I/O path seems excessive. So I implemented a >>> "started" flag to the cp, set after a cc0 from the START, and examine >>> that on the interrupt path to determine whether cp_free() is needed. >> >> FYI... After a day or two of running, I sprung a kernel debug oops for >> list corruption in ccwchain_free(). I'm going to blame this piece, since >> it was the last thing I changed and I hadn't come across any such damage >> since v2. So either "started" is a bad idea, or a broken one. Or both. :) > > Have you come to any conclusion wrt 'started'? Not wanting to generate > stress, just asking :) > I've talked myself out of it, and gone back to your original proposal of a separately allocated cp. (Still no queuing.) Too early to pass judgement though. Yesterday, when running with a cp_free() call after a CSCH, I was getting all sorts of errors very early on, so at the moment I've pulled that back out again. If it looks good in this form, I'll put that as a separate patch and write up some doc for a discussion on that point.
Let's continue our discussion of the handling of vfio-ccw interrupts. The initial fix [1] relied upon the interrupt path's examination of the FSM state, and freeing all resources if it were CP_PENDING. But the interface used by HALT/CLEAR SUBCHANNEL doesn't affect the FSM state. Consider this sequence: CPU 1 CPU 2 CLEAR (state=IDLE/no change) START [2] INTERRUPT (set state=IDLE) INTERRUPT (set state=IDLE) This translates to a couple of possible scenarios: A) The START gets a cc2 because of the outstanding CLEAR, -EBUSY is returned, resources are freed, and state remains IDLE B) The START gets a cc0 because the CLEAR has already presented an interrupt, and state is set to CP_PENDING If the START gets a cc0 before the CLEAR INTERRUPT (stacked onto a workqueue by the IRQ context) gets a chance to run, then the INTERRUPT will release the channel program memory prematurely. If the two operations run concurrently, then the FSM state set to CP_PROCESSING will prevent the cp_free() from being invoked. But the io_mutex boundary on that path will pause itself until the START completes, and then allow the FSM to be reset to IDLE without considering the outstanding START. Neither scenario would be considered good. Having said all of that, in v2 Conny suggested [3] the following: > - 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. What I have implemented here is basically this, with a few changes: - I don't queue cp's. Since there should only be one START in process at a time, and HALT/CLEAR doesn't build a cp, I didn't see a pressing need to introduce that complexity. - Furthermore, while I initially made a separately allocated cp, adding an alloc for a cp on each I/O AND moving the guest_cp alloc from the probe path to the I/O path seems excessive. So I implemented a "started" flag to the cp, set after a cc0 from the START, and examine that on the interrupt path to determine whether cp_free() is needed. - I opted against a "SOMETHING_PENDING" state if START/HALT/CLEAR got a cc0, and just put the FSM back to IDLE. It becomes too unwieldy to discern which operation an interrupt is completing, and whether more interrupts are expected, to be worth the additional state. - A successful CSCH doesn't do anything special, and cp_free() is only performed on the interrupt path. Part of me wrestled with how a HALT fits into that, but mostly it was that a cc0 on any of the instructions indicated the "channel subsystem is signaled to asynchronously perform the [START/HALT/CLEAR] function." This means that an in-flight START could still receive data from the device/subchannel, so not a good idea to release memory at that point. Separate from all that, I added a small check of the io_work queue to the FSM START path. Part of the problems I've seen was that an interrupt is presented by a CPU, but not yet processed by vfio-ccw. Some of the problems seen thus far is because of this gap, and the above changes don't address that either. Whether this is appropriate or ridiculous would be a welcome discussion. Previous versions: v2: https://lore.kernel.org/kvm/20200513142934.28788-1-farman@linux.ibm.com/ v1: https://lore.kernel.org/kvm/20200124145455.51181-1-farman@linux.ibm.com/ Footnotes: [1] https://lore.kernel.org/kvm/62e87bf67b38dc8d5760586e7c96d400db854ebe.1562854091.git.alifm@linux.ibm.com/ [2] Halil has pointed out that QEMU should prohibit this, based on the rules set forth by the POPs. This is true, but we should not rely on it behaving properly without addressing this scenario that is visible today. Once I get this behaving correctly, I'll spend some time seeing if QEMU is misbehaving somehow. [3] https://lore.kernel.org/kvm/20200518180903.7cb21dd8.cohuck@redhat.com/ [4] https://lore.kernel.org/kvm/a52368d3-8cec-7b99-1587-25e055228b62@linux.ibm.com/ Eric Farman (3): vfio-ccw: Indicate if a channel_program is started vfio-ccw: Remove the CP_PENDING FSM state vfio-ccw: Check workqueue before doing START drivers/s390/cio/vfio_ccw_cp.c | 2 ++ drivers/s390/cio/vfio_ccw_cp.h | 1 + drivers/s390/cio/vfio_ccw_drv.c | 5 +---- drivers/s390/cio/vfio_ccw_fsm.c | 32 +++++++++++++++++------------ drivers/s390/cio/vfio_ccw_ops.c | 3 +-- drivers/s390/cio/vfio_ccw_private.h | 1 - 6 files changed, 24 insertions(+), 20 deletions(-)