| Message ID | 166002010021.381133.11357879752637949308.stgit@omen (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | mm: re-allow pinning of zero pfns (again) | expand |
On 09.08.22 06:42, Alex Williamson wrote: > The below referenced commit makes the same error as 1c563432588d ("mm: fix > is_pinnable_page against a cma page"), re-interpreting the logic to exclude > pinning of the zero page, which breaks device assignment with vfio. > > Link: https://lore.kernel.org/all/165490039431.944052.12458624139225785964.stgit@omen > Fixes: f25cbb7a95a2 ("mm: add zone device coherent type memory support") > Signed-off-by: Alex Williamson <alex.williamson@redhat.com> > --- > include/linux/mm.h | 5 ++--- > 1 file changed, 2 insertions(+), 3 deletions(-) > > diff --git a/include/linux/mm.h b/include/linux/mm.h > index 18e01474cf6b..772279ed7010 100644 > --- a/include/linux/mm.h > +++ b/include/linux/mm.h > @@ -1544,9 +1544,8 @@ static inline bool is_longterm_pinnable_page(struct page *page) > if (mt == MIGRATE_CMA || mt == MIGRATE_ISOLATE) > return false; > #endif > - return !(is_device_coherent_page(page) || > - is_zone_movable_page(page) || > - is_zero_pfn(page_to_pfn(page))); > + return !(is_device_coherent_page(page) || is_zone_movable_page(page)) || > + is_zero_pfn(page_to_pfn(page)); > } > #else > static inline bool is_longterm_pinnable_page(struct page *page) :/ I guess the code was moved just at the time the old code was still in place, and when rebasing, the diff in the code was ignored. Reviewed-by: David Hildenbrand <david@redhat.com> I have patches in the works that will properly break COW here to get anon pages instead of pinning the shared zeropage, which is questionable in COW mappings.
On Mon, Aug 08, 2022 at 10:42:24PM -0600, Alex Williamson wrote: > The below referenced commit makes the same error as 1c563432588d ("mm: fix > is_pinnable_page against a cma page"), re-interpreting the logic to exclude > pinning of the zero page, which breaks device assignment with vfio. Perhaps we need to admit we're not as good at boolean logic as we think we are. if (is_device_coherent_page(page)) return false; if (is_zone_movable_page(page)) return false; return is_zero_pfn(page_to_pfn(page)); (or whatever the right logic is ... I just woke up and I'm having trouble parsing it). > Link: https://lore.kernel.org/all/165490039431.944052.12458624139225785964.stgit@omen > Fixes: f25cbb7a95a2 ("mm: add zone device coherent type memory support") > Signed-off-by: Alex Williamson <alex.williamson@redhat.com> > --- > include/linux/mm.h | 5 ++--- > 1 file changed, 2 insertions(+), 3 deletions(-) > > diff --git a/include/linux/mm.h b/include/linux/mm.h > index 18e01474cf6b..772279ed7010 100644 > --- a/include/linux/mm.h > +++ b/include/linux/mm.h > @@ -1544,9 +1544,8 @@ static inline bool is_longterm_pinnable_page(struct page *page) > if (mt == MIGRATE_CMA || mt == MIGRATE_ISOLATE) > return false; > #endif > - return !(is_device_coherent_page(page) || > - is_zone_movable_page(page) || > - is_zero_pfn(page_to_pfn(page))); > + return !(is_device_coherent_page(page) || is_zone_movable_page(page)) || > + is_zero_pfn(page_to_pfn(page)); > } > #else > static inline bool is_longterm_pinnable_page(struct page *page) > > >
Am 2022-08-09 um 08:31 schrieb Matthew Wilcox: > On Mon, Aug 08, 2022 at 10:42:24PM -0600, Alex Williamson wrote: >> The below referenced commit makes the same error as 1c563432588d ("mm: fix >> is_pinnable_page against a cma page"), re-interpreting the logic to exclude >> pinning of the zero page, which breaks device assignment with vfio. > Perhaps we need to admit we're not as good at boolean logic as we think > we are. > > if (is_device_coherent_page(page)) > return false; > if (is_zone_movable_page(page)) > return false; > return is_zero_pfn(page_to_pfn(page)); > > (or whatever the right logic is ... I just woke up and I'm having > trouble parsing it). This implies an assumption that zero-page is never device-coherent or moveable, which is probably true, but not part of the original condition. A more formally correct rewrite would be: if (is_zero_pfn(page_to_pfn(page))) return true; if (is_device_coherent_page(page)) return false; return !is_zone_moveable_page(page); Regards, Felix > >> Link: https://lore.kernel.org/all/165490039431.944052.12458624139225785964.stgit@omen >> Fixes: f25cbb7a95a2 ("mm: add zone device coherent type memory support") >> Signed-off-by: Alex Williamson <alex.williamson@redhat.com> >> --- >> include/linux/mm.h | 5 ++--- >> 1 file changed, 2 insertions(+), 3 deletions(-) >> >> diff --git a/include/linux/mm.h b/include/linux/mm.h >> index 18e01474cf6b..772279ed7010 100644 >> --- a/include/linux/mm.h >> +++ b/include/linux/mm.h >> @@ -1544,9 +1544,8 @@ static inline bool is_longterm_pinnable_page(struct page *page) >> if (mt == MIGRATE_CMA || mt == MIGRATE_ISOLATE) >> return false; >> #endif >> - return !(is_device_coherent_page(page) || >> - is_zone_movable_page(page) || >> - is_zero_pfn(page_to_pfn(page))); >> + return !(is_device_coherent_page(page) || is_zone_movable_page(page)) || >> + is_zero_pfn(page_to_pfn(page)); >> } >> #else >> static inline bool is_longterm_pinnable_page(struct page *page) >> >> >>
On Tue, Aug 09, 2022 at 10:14:12AM -0400, Felix Kuehling wrote: > Am 2022-08-09 um 08:31 schrieb Matthew Wilcox: > > On Mon, Aug 08, 2022 at 10:42:24PM -0600, Alex Williamson wrote: > > > The below referenced commit makes the same error as 1c563432588d ("mm: fix > > > is_pinnable_page against a cma page"), re-interpreting the logic to exclude > > > pinning of the zero page, which breaks device assignment with vfio. > > Perhaps we need to admit we're not as good at boolean logic as we think > > we are. > > > > if (is_device_coherent_page(page)) > > return false; > > if (is_zone_movable_page(page)) > > return false; > > return is_zero_pfn(page_to_pfn(page)); > > > > (or whatever the right logic is ... I just woke up and I'm having > > trouble parsing it). > > This implies an assumption that zero-page is never device-coherent or > moveable, which is probably true, but not part of the original condition. A > more formally correct rewrite would be: > > if (is_zero_pfn(page_to_pfn(page))) > return true; > if (is_device_coherent_page(page)) > return false; > return !is_zone_moveable_page(page); It's definitely true that the zero page is never device-coherent, nor movable. Moreover, we want to avoid calling page_to_pfn() if we can. So it should be the last condition that we check.
On 09.08.22 16:43, Matthew Wilcox wrote: > On Tue, Aug 09, 2022 at 10:14:12AM -0400, Felix Kuehling wrote: >> Am 2022-08-09 um 08:31 schrieb Matthew Wilcox: >>> On Mon, Aug 08, 2022 at 10:42:24PM -0600, Alex Williamson wrote: >>>> The below referenced commit makes the same error as 1c563432588d ("mm: fix >>>> is_pinnable_page against a cma page"), re-interpreting the logic to exclude >>>> pinning of the zero page, which breaks device assignment with vfio. >>> Perhaps we need to admit we're not as good at boolean logic as we think >>> we are. >>> >>> if (is_device_coherent_page(page)) >>> return false; >>> if (is_zone_movable_page(page)) >>> return false; >>> return is_zero_pfn(page_to_pfn(page)); >>> >>> (or whatever the right logic is ... I just woke up and I'm having >>> trouble parsing it). >> >> This implies an assumption that zero-page is never device-coherent or >> moveable, which is probably true, but not part of the original condition. A >> more formally correct rewrite would be: >> >> if (is_zero_pfn(page_to_pfn(page))) >> return true; >> if (is_device_coherent_page(page)) >> return false; >> return !is_zone_moveable_page(page); > > It's definitely true that the zero page is never device-coherent, nor > movable. Moreover, we want to avoid calling page_to_pfn() if we can. > So it should be the last condition that we check. IIRC, with "kernelcore" and/or "movablecore", the zero page could eventually end up in the movable zone (whereby we can have boottime allocations being placed into the movable zone). IIRC that's why we have to special-case on the zero-page here at all. So taking out the zero page first is correct.
On Tue, 9 Aug 2022 10:14:12 -0400 Felix Kuehling <felix.kuehling@amd.com> wrote: > Am 2022-08-09 um 08:31 schrieb Matthew Wilcox: > > On Mon, Aug 08, 2022 at 10:42:24PM -0600, Alex Williamson wrote: > >> The below referenced commit makes the same error as 1c563432588d ("mm: fix > >> is_pinnable_page against a cma page"), re-interpreting the logic to exclude > >> pinning of the zero page, which breaks device assignment with vfio. If two people made the same error then surely that's a sign that we need a comment which explains things to the next visitor. > > Perhaps we need to admit we're not as good at boolean logic as we think > > we are. > > > > if (is_device_coherent_page(page)) > > return false; > > if (is_zone_movable_page(page)) > > return false; > > return is_zero_pfn(page_to_pfn(page)); > > > > (or whatever the right logic is ... I just woke up and I'm having > > trouble parsing it). > > This implies an assumption that zero-page is never device-coherent or > moveable, which is probably true, but not part of the original > condition. A more formally correct rewrite would be: > > if (is_zero_pfn(page_to_pfn(page))) > return true; > if (is_device_coherent_page(page)) > return false; > return !is_zone_moveable_page(page); > Yes please, vastly better. And a nice thing about this layout is that it leaves places where we can add a nice little comment against each clause of the test, to explain why we're performing each one.
diff --git a/include/linux/mm.h b/include/linux/mm.h index 18e01474cf6b..772279ed7010 100644 --- a/include/linux/mm.h +++ b/include/linux/mm.h @@ -1544,9 +1544,8 @@ static inline bool is_longterm_pinnable_page(struct page *page) if (mt == MIGRATE_CMA || mt == MIGRATE_ISOLATE) return false; #endif - return !(is_device_coherent_page(page) || - is_zone_movable_page(page) || - is_zero_pfn(page_to_pfn(page))); + return !(is_device_coherent_page(page) || is_zone_movable_page(page)) || + is_zero_pfn(page_to_pfn(page)); } #else static inline bool is_longterm_pinnable_page(struct page *page)
The below referenced commit makes the same error as 1c563432588d ("mm: fix is_pinnable_page against a cma page"), re-interpreting the logic to exclude pinning of the zero page, which breaks device assignment with vfio. Link: https://lore.kernel.org/all/165490039431.944052.12458624139225785964.stgit@omen Fixes: f25cbb7a95a2 ("mm: add zone device coherent type memory support") Signed-off-by: Alex Williamson <alex.williamson@redhat.com> --- include/linux/mm.h | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-)