On Mon, 24 Jun 2019 11:24:16 -0400 Farhan Ali <alifm@xxxxxxxxxxxxx> wrote: > On 06/24/2019 11:09 AM, Cornelia Huck wrote: > > On Mon, 24 Jun 2019 10:44:17 -0400 > > Farhan Ali <alifm@xxxxxxxxxxxxx> wrote: > >> But even if we don't remove the cp_free from vfio_ccw_sch_io_todo, I am > >> not sure if your suggestion will fix the problem. The problem here is > >> that we can call vfio_ccw_sch_io_todo (for a clear or halt interrupt) at > >> the same time we are handling an ssch request. So depending on the order > >> of the operations we could still end up calling cp_free from both from > >> threads (i refer to the threads I mentioned in response to Eric's > >> earlier email). > > > > What I don't see is why this is a problem with ->initialized; wasn't > > the problem that we misinterpreted an interrupt for csch as one for a > > not-yet-issued ssch? > > > > It's the order in which we do things, which could cause the problem. > Since we queue interrupt handling in the workqueue, we could delay > processing the csch interrupt. During this delay if ssch comes through, > we might have already set ->initialized to true. > > So when we get around to handling the interrupt in io_todo, we would go > ahead and call cp_free. This would cause the problem of freeing the > ccwchain list while we might be adding to it. > > >> > >> Another thing that concerns me is that vfio-ccw can also issue csch/hsch > >> in the quiesce path, independently of what the guest issues. So in that > >> case we could have a similar scenario to processing an ssch request and > >> issuing halt/clear in parallel. But maybe I am being paranoid :) > > > > I think the root problem is really trying to clear a cp while another > > thread is trying to set it up. Should we maybe use something like rcu? > > > > > > Yes, this is the root problem. I am not too familiar with rcu locking, > but what would be the benefit over a traditional mutex? I don't quite remember what I had been envisioning at the time (sorry, the heat seems to make my brain a bit slushy :/), but I think we might have two copies of the cp and use an rcu-ed pointer in the private structure to point to one of the copies. If we make sure we've synchronized on the pointer at interrupt time, we should be able to free the old one in _todo and act on the new on when doing ssch. And yes, I realize that this is awfully vague :)
