[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [PATCH v4 02/11] vpci: cancel pending map/unmap on vpci removal
On 18.11.21 17:41, Jan Beulich wrote: > On 18.11.2021 16:21, Oleksandr Andrushchenko wrote: >> On 18.11.21 17:16, Jan Beulich wrote: >>> On 18.11.2021 16:11, Oleksandr Andrushchenko wrote: >>>> On 18.11.21 16:35, Jan Beulich wrote: >>>>> On 18.11.2021 15:14, Oleksandr Andrushchenko wrote: >>>>>> On 18.11.21 16:04, Roger Pau Monné wrote: >>>>>>> Indeed. In the physdevop failure case this comes from an hypercall >>>>>>> context, so maybe you could do the mapping in place using hypercall >>>>>>> continuations if required. Not sure how complex that would be, >>>>>>> compared to just deferring to guest entry point and then dealing with >>>>>>> the possible cleanup on failure. >>>>>> This will solve one part of the equation: >>>>>> >>>>>> pci_physdev_op >>>>>> pci_add_device >>>>>> init_bars -> modify_bars -> defer_map -> >>>>>> raise_softirq(SCHEDULE_SOFTIRQ) >>>>>> iommu_add_device <- FAILS >>>>>> vpci_remove_device -> xfree(pdev->vpci) >>>>>> >>>>>> But what about the other one, e.g. vpci_process_pending is triggered in >>>>>> parallel with PCI device de-assign for example? >>>>> Well, that's again in hypercall context, so using hypercall continuations >>>>> may again be an option. Of course at the point a de-assign is initiated, >>>>> you "only" need to drain requests (for that device, but that's unlikely >>>>> to be worthwhile optimizing for), while ensuring no new requests can be >>>>> issued. Again, for the device in question, but here this is relevant - >>>>> a flag may want setting to refuse all further requests. Or maybe the >>>>> register handling hooks may want tearing down before draining pending >>>>> BAR mapping requests; without the hooks in place no new such requests >>>>> can possibly appear. >>>> This can be probably even easier to solve as we were talking about >>>> pausing all vCPUs: >>> I have to admit I'm not sure. It might be easier, but it may also be >>> less desirable. >>> >>>> void vpci_cancel_pending(const struct pci_dev *pdev) >>>> { >>>> struct domain *d = pdev->domain; >>>> struct vcpu *v; >>>> int rc; >>>> >>>> while ( (rc = domain_pause_except_self(d)) == -ERESTART ) >>>> cpu_relax(); >>>> >>>> if ( rc ) >>>> printk(XENLOG_G_ERR >>>> "Failed to pause vCPUs while canceling vPCI map/unmap >>>> for %pp %pd: %d\n", >>>> &pdev->sbdf, pdev->domain, rc); >>>> >>>> for_each_vcpu ( d, v ) >>>> { >>>> if ( v->vpci.map_pending && (v->vpci.pdev == pdev) ) >>>> >>>> This will prevent all vCPUs to run, but current, thus making it impossible >>>> to run vpci_process_pending in parallel with any hypercall. >>>> So, even without locking in vpci_process_pending the above should >>>> be enough. >>>> The only concern here is that domain_pause_except_self may return >>>> the error code we somehow need to handle... >>> Not just this. The -ERESTART handling isn't appropriate this way >>> either. >> Are you talking about cpu_relax()? > I'm talking about that spin-waiting loop as a whole. > >>> For the moment I can't help thinking that draining would >>> be preferable over canceling. >> Given that cancellation is going to happen on error path or >> on device de-assign/remove I think this can be acceptable. >> Any reason why not? > It would seem to me that the correctness of a draining approach is > going to be easier to prove than that of a canceling one, where I > expect races to be a bigger risk. Especially something that gets > executed infrequently, if ever (error paths in particular), knowing > things are well from testing isn't typically possible. Could you please then give me a hint how to do that: 1. We have scheduled SOFTIRQ on vCPU0 and it is about to touch pdev->vpci 2. We have de-assign/remove on vCPU1 How do we drain that? Do you mean some atomic variable to be used in vpci_process_pending to flag it is running and de-assign/remove needs to wait and spinning checking that? > > Jan > > Thank you, Oleksandr
|
![]() |
Lists.xenproject.org is hosted with RackSpace, monitoring our |