| Message ID | 20220831141040.13231-1-volodymyr_babchuk@epam.com (mailing list archive) |
|---|---|
| Headers | show |
| Series | Rework PCI locking | expand |
On 31.08.2022 16:10, Volodymyr Babchuk wrote: > Hello, > > This is yet another take to a PCI locking rework. This approach > was suggest by Jan Beulich who proposed to use a reference > counter to control lifetime of pci_dev objects. > > When I started added reference counting it quickly became clear > that this approach can provide more granular locking insted of > huge pcidevs_lock() which is used right now. I studied how this > lock used and what it protects. And found the following: > > 0. Comment in pci.h states the following: > > 153 /* > 154 * The pcidevs_lock protect alldevs_list, and the assignment for the > 155 * devices, it also sync the access to the msi capability that is not > 156 * interrupt handling related (the mask bit register). > 157 */ > > But in reality it does much more. Here is what I found: > > 1. Lifetime of pci_dev struct > > 2. Access to pseg->alldevs_list > > 3. Access to domain->pdev_list > > 4. Access to iommu->ats_list > > 5. Access to MSI capability > > 6. Some obsucure stuff in IOMMU drivers: there are places that > are guarded by pcidevs_lock() but it seems that nothing > PCI-related happens there. Right - the lock being held was (ab)used in IOMMU code in a number of places. This likely needs to change in the course of this re-work; patch titles don't suggest this is currently part of the series. > 7. Something that I probably overlooked And this is the main risk here. The huge scope of the original lock means that many things are serialized now but won't be anymore once the lock is gone. But yes - thanks for the work. To be honest I don't expect to be able to look at this series in detail until after the Xen Summit. And even then it may take a while ... Jan
Hi Jan, On 06/09/2022 11:32, Jan Beulich wrote: > On 31.08.2022 16:10, Volodymyr Babchuk wrote: >> Hello, >> >> This is yet another take to a PCI locking rework. This approach >> was suggest by Jan Beulich who proposed to use a reference >> counter to control lifetime of pci_dev objects. >> >> When I started added reference counting it quickly became clear >> that this approach can provide more granular locking insted of >> huge pcidevs_lock() which is used right now. I studied how this >> lock used and what it protects. And found the following: >> >> 0. Comment in pci.h states the following: >> >> 153 /* >> 154 * The pcidevs_lock protect alldevs_list, and the assignment for the >> 155 * devices, it also sync the access to the msi capability that is not >> 156 * interrupt handling related (the mask bit register). >> 157 */ >> >> But in reality it does much more. Here is what I found: >> >> 1. Lifetime of pci_dev struct >> >> 2. Access to pseg->alldevs_list >> >> 3. Access to domain->pdev_list >> >> 4. Access to iommu->ats_list >> >> 5. Access to MSI capability >> >> 6. Some obsucure stuff in IOMMU drivers: there are places that >> are guarded by pcidevs_lock() but it seems that nothing >> PCI-related happens there. > > Right - the lock being held was (ab)used in IOMMU code in a number of > places. This likely needs to change in the course of this re-work; > patch titles don't suggest this is currently part of the series. > >> 7. Something that I probably overlooked > > And this is the main risk here. The huge scope of the original lock > means that many things are serialized now but won't be anymore once > the lock is gone. > > But yes - thanks for the work. To be honest I don't expect to be able > to look at this series in detail until after the Xen Summit. And even > then it may take a while ... I was wondering if this is still in your list to review? Cheers,
On 18.01.2023 19:21, Julien Grall wrote: > On 06/09/2022 11:32, Jan Beulich wrote: >> On 31.08.2022 16:10, Volodymyr Babchuk wrote: >>> Hello, >>> >>> This is yet another take to a PCI locking rework. This approach >>> was suggest by Jan Beulich who proposed to use a reference >>> counter to control lifetime of pci_dev objects. >>> >>> When I started added reference counting it quickly became clear >>> that this approach can provide more granular locking insted of >>> huge pcidevs_lock() which is used right now. I studied how this >>> lock used and what it protects. And found the following: >>> >>> 0. Comment in pci.h states the following: >>> >>> 153 /* >>> 154 * The pcidevs_lock protect alldevs_list, and the assignment for the >>> 155 * devices, it also sync the access to the msi capability that is not >>> 156 * interrupt handling related (the mask bit register). >>> 157 */ >>> >>> But in reality it does much more. Here is what I found: >>> >>> 1. Lifetime of pci_dev struct >>> >>> 2. Access to pseg->alldevs_list >>> >>> 3. Access to domain->pdev_list >>> >>> 4. Access to iommu->ats_list >>> >>> 5. Access to MSI capability >>> >>> 6. Some obsucure stuff in IOMMU drivers: there are places that >>> are guarded by pcidevs_lock() but it seems that nothing >>> PCI-related happens there. >> >> Right - the lock being held was (ab)used in IOMMU code in a number of >> places. This likely needs to change in the course of this re-work; >> patch titles don't suggest this is currently part of the series. >> >>> 7. Something that I probably overlooked >> >> And this is the main risk here. The huge scope of the original lock >> means that many things are serialized now but won't be anymore once >> the lock is gone. >> >> But yes - thanks for the work. To be honest I don't expect to be able >> to look at this series in detail until after the Xen Summit. And even >> then it may take a while ... > > I was wondering if this is still in your list to review? Yes, it certainly is. But as before no predictions when I might get to it. Jan