Messages in this thread | | | Date | Thu, 2 Jun 2022 11:54:34 +0200 | Subject | Re: [PATCH v2] xen/gntdev: Avoid blocking in unmap_grant_pages() | From | Juergen Gross <> |
| |
On 30.05.22 19:50, Demi Marie Obenour wrote: > On Mon, May 30, 2022 at 08:41:20AM +0200, Juergen Gross wrote: >> On 25.05.22 20:41, Demi Marie Obenour wrote: >>> unmap_grant_pages() currently waits for the pages to no longer be used. >>> In https://github.com/QubesOS/qubes-issues/issues/7481, this lead to a >>> deadlock against i915: i915 was waiting for gntdev's MMU notifier to >>> finish, while gntdev was waiting for i915 to free its pages. I also >>> believe this is responsible for various deadlocks I have experienced in >>> the past. >>> >>> Avoid these problems by making unmap_grant_pages async. This requires >>> making it return void, as any errors will not be available when the >>> function returns. Fortunately, the only use of the return value is a >>> WARN_ON(), which can be replaced by a WARN_ON when the error is >>> detected. Additionally, a failed call will not prevent further calls >>> from being made, but this is harmless. >>> >>> Because unmap_grant_pages is now async, the grant handle will be sent to >>> INVALID_GRANT_HANDLE too late to prevent multiple unmaps of the same >>> handle. Instead, a separate bool array is allocated for this purpose. >>> This wastes memory, but stuffing this information in padding bytes is >>> too fragile. Furthermore, it is necessary to grab a reference to the >>> map before making the asynchronous call, and release the reference when >>> the call returns. >> >> I think there is even more syncing needed: >> >> - In the error path of gntdev_mmap() unmap_grant_pages() is being called and >> it is assumed, map is available afterwards again. This should be rather easy >> to avoid by adding a counter of active mappings to struct gntdev_grant_map >> (number of pages not being unmapped yet). In case this counter is not zero >> gntdev_mmap() should bail out early. > > Is it possible to just unmap the pages directly here? I don’t think > there can be any other users of these pages yet. Userspace could race > against the unmap and cause a page fault, but that should just cause > userspace to get SIGSEGV or SIGBUS. In any case this code can use the > sync version; if it gets blocked that’s userspace’s problem. > >> - gntdev_put_map() is calling unmap_grant_pages() in case the refcount has >> dropped to zero. This call can set the refcount to 1 again, so there is >> another delay needed before freeing map. I think unmap_grant_pages() should >> return in case the count of mapped pages is zero (see above), thus avoiding >> to increment the refcount of map if nothing is to be done. This would enable >> gntdev_put_map() to just return after the call of unmap_grant_pages() in case >> the refcount has been incremented again. > > I will change this in v3, but I do wonder if gntdev is using the wrong > MMU notifier callback. It seems that the appropriate callback is > actually release(): if I understand correctly, release() is called > precisely when the refcount on the physical page is about to drop to 0, > and that is what we want.
No, I don't think this is correct.
release() is being called when the refcount of the address space is about to drop to 0. It has no page granularity.
Juergen [unhandled content-type:application/pgp-keys][unhandled content-type:application/pgp-signature] | |