Messages in this thread | | | From | "Liu, Yi L" <> | Subject | RE: [PATCH v2 1/3] iommufd: Add devices_users to track the hw_pagetable usage by device | Date | Tue, 7 Feb 2023 04:27:00 +0000 |
| |
> From: Nicolin Chen <nicolinc@nvidia.com> > Sent: Thursday, February 2, 2023 3:25 AM > > On Wed, Feb 01, 2023 at 02:37:42PM -0400, Jason Gunthorpe wrote: > > On Wed, Feb 01, 2023 at 09:46:23AM -0800, Nicolin Chen wrote: > > > > So the issue is with replace you need to have the domain populated > > > > before we can call replace but you can't populate the domain until it > > > > is bound because of the above issue? That seems unsovlable without > > > > fixing up the driver. > > > > > > Not really. A REPLACE ioctl is just an ATTACH, if the device just > > > gets BIND-ed. So the SMMU driver will initialize ("finalise") the > > > domain during the replace() call, then iopt_table_add_domain() can > > > be done. > > > > > > So, not a blocker here. > > > > Well, yes, there sort of is because the whole flow becomes nonsensical > > - we are supposed to have the iommu_domain populated by the time we > do > > replace. Otherwise replace is extra-pointless.. > > The "finalise" is one of the very first lines of the attach_dev() > callback function in SMMU driver, though it might still undesirably > fail the replace(). > > https://github.com/nicolinc/iommufd/commit/5ae54f360495aae35b5967d1 > eb00149912145639 > Btw, this is a draft that I made to move iopt_table_add_domain(). I > think we can have this with the nesting series. > > Later, once we pass in the dev pointer to the ->domain_alloc op > using Robin's change, all the iopt_table_add_domain() can be done > within the hwpt_alloc(), prior to an attach()/replace(). > > > > > Is there another issue? > > > > > > Oh. I think we mixed the topics here. These three patches were > > > not to unblock but to clean up a way for the replace series and > > > the nesting series, for the device locking issue: > > > > > > if (cur_hwpt != hwpt) > > > mutex_lock(&cur_hwpt->device_lock); > > > mutex_lock(&hwpt->device_lock); > > > ... > > > if (iommufd_hw_pagetabe_has_group()) { // touching device > list > > > ... > > > iommu_group_replace_domain(); > > > ... > > > } > > > if (cur_hwpt && hwpt) > > > list_del(&idev->devices_item); > > > list_add(&idev->devices_item, &cur_hwpt->devices); > > > ... > > > mutex_unlock(&hwpt->device_lock); > > > if (cur_hwpt != hwpt) > > > mutex_unlock(&cur_hwpt->device_lock); > > > > What is the issue? That isn't quite right, but the basic bit is fine > > > > If you want to do replace then you have to hold both devices_lock and > > you write that super ugly thing like this > > > > lock_both: > > if (hwpt_a < hwpt_b) { > > mutex_lock(&hwpt_a->devices_lock); > > mutex_lock_nested(&hwpt_b->devices_lock); > > } else if (hwpt_a > hwpt_b) { > > mutex_lock(&hwpt_b->devices_lock); > > mutex_lock_nested(&hwpt_a->devices_lock); > > } else > > mutex_lock(&hwpt_a->devices_lock); > > > > And then it is trivial, yes? > > Yea. That's your previous remark. > > > Using the group_lock in the iommu core is the right way to fix > > this.. Maybe someday we can do that. > > > > (also document that replace causes all the devices in the group to > > change iommu_domains at once) > > Yes. There's a discussion in PATCH-3 of this series. I drafted a > patch changing iommu_attach/detach_dev(): > https://github.com/nicolinc/iommufd/commit/124f7804ef38d50490b606fd5 > 6c1e27ce551a839 > > Baolu had a similar patch series a year ago. So we might continue > that effort in parallel, and eventually drop the device list/lock. > > > > I just gave another thought about it. Since we have the patch-2 > > > from this series moving the ioas->mutex, it already serializes > > > attach/detach routines. And I see that all the places touching > > > idev->device_item and hwpt->devices are protected by ioas->mutex. > > > So, perhaps we can simply remove the device_lock? > > > > The two hwpts are not required to have the same ioas, so this doesn't > > really help.. > > Hmm...in that case, we should hold two ioas->mutex locks in > addition to two device locks?
Aha, seems so. You can replace a s1 hwpt with another s1 hwpt which Has a different ioas. 😊 maybe this is something incremental to nesting series. In nesting series, between ioas and s1 hwpt, they can share the device_lock. Isn't it?
Regards, Yi Liu
| |