* [bug report] mm/gup: migrate device coherent pages when pinning instead of failing
@ 2022-07-19 9:41 Dan Carpenter
2022-07-19 12:04 ` Alistair Popple
0 siblings, 1 reply; 3+ messages in thread
From: Dan Carpenter @ 2022-07-19 9:41 UTC (permalink / raw)
To: apopple; +Cc: linux-mm
Hello Alistair Popple,
The patch b05a79d4377f: "mm/gup: migrate device coherent pages when
pinning instead of failing" from Jul 15, 2022, leads to the following
Smatch static checker warning:
mm/migrate_device.c:842 migrate_device_coherent_page()
warn: duplicate check 'src_pfn & (1 << 1)' (previous on line 832)
mm/migrate_device.c
810 int migrate_device_coherent_page(struct page *page)
811 {
812 unsigned long src_pfn, dst_pfn = 0;
813 struct migrate_vma args;
814 struct page *dpage;
815
816 WARN_ON_ONCE(PageCompound(page));
817
818 lock_page(page);
819 src_pfn = migrate_pfn(page_to_pfn(page)) | MIGRATE_PFN_MIGRATE;
820 args.src = &src_pfn;
821 args.dst = &dst_pfn;
822 args.cpages = 1;
823 args.npages = 1;
824 args.vma = NULL;
825
826 /*
827 * We don't have a VMA and don't need to walk the page tables to find
828 * the source page. So call migrate_vma_unmap() directly to unmap the
829 * page as migrate_vma_setup() will fail if args.vma == NULL.
830 */
831 migrate_vma_unmap(&args);
832 if (!(src_pfn & MIGRATE_PFN_MIGRATE))
833 return -EBUSY;
Return here.
834
835 dpage = alloc_page(GFP_USER | __GFP_NOWARN);
836 if (dpage) {
837 lock_page(dpage);
838 dst_pfn = migrate_pfn(page_to_pfn(dpage));
839 }
840
841 migrate_vma_pages(&args);
--> 842 if (src_pfn & MIGRATE_PFN_MIGRATE)
843 copy_highpage(dpage, page);
No need to check again. Was dst_pfn intended?
844 migrate_vma_finalize(&args);
845
846 if (src_pfn & MIGRATE_PFN_MIGRATE)
847 return 0;
Same check again.
848 return -EBUSY;
849 }
regards,
dan carpenter
^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: [bug report] mm/gup: migrate device coherent pages when pinning instead of failing
2022-07-19 9:41 [bug report] mm/gup: migrate device coherent pages when pinning instead of failing Dan Carpenter
@ 2022-07-19 12:04 ` Alistair Popple
2022-07-19 13:00 ` Dan Carpenter
0 siblings, 1 reply; 3+ messages in thread
From: Alistair Popple @ 2022-07-19 12:04 UTC (permalink / raw)
To: Dan Carpenter; +Cc: linux-mm
Dan Carpenter <dan.carpenter@oracle.com> writes:
> Hello Alistair Popple,
>
> The patch b05a79d4377f: "mm/gup: migrate device coherent pages when
> pinning instead of failing" from Jul 15, 2022, leads to the following
> Smatch static checker warning:
>
> mm/migrate_device.c:842 migrate_device_coherent_page()
> warn: duplicate check 'src_pfn & (1 << 1)' (previous on line 832)
>
> mm/migrate_device.c
> 810 int migrate_device_coherent_page(struct page *page)
> 811 {
> 812 unsigned long src_pfn, dst_pfn = 0;
> 813 struct migrate_vma args;
> 814 struct page *dpage;
> 815
> 816 WARN_ON_ONCE(PageCompound(page));
> 817
> 818 lock_page(page);
> 819 src_pfn = migrate_pfn(page_to_pfn(page)) | MIGRATE_PFN_MIGRATE;
> 820 args.src = &src_pfn;
> 821 args.dst = &dst_pfn;
> 822 args.cpages = 1;
> 823 args.npages = 1;
> 824 args.vma = NULL;
> 825
> 826 /*
> 827 * We don't have a VMA and don't need to walk the page tables to find
> 828 * the source page. So call migrate_vma_unmap() directly to unmap the
> 829 * page as migrate_vma_setup() will fail if args.vma == NULL.
> 830 */
> 831 migrate_vma_unmap(&args);
> 832 if (!(src_pfn & MIGRATE_PFN_MIGRATE))
> 833 return -EBUSY;
>
> Return here.
>
> 834
> 835 dpage = alloc_page(GFP_USER | __GFP_NOWARN);
> 836 if (dpage) {
> 837 lock_page(dpage);
> 838 dst_pfn = migrate_pfn(page_to_pfn(dpage));
> 839 }
> 840
> 841 migrate_vma_pages(&args);
> --> 842 if (src_pfn & MIGRATE_PFN_MIGRATE)
> 843 copy_highpage(dpage, page);
>
> No need to check again. Was dst_pfn intended?
No. The check is necessary and correct. migrate_vma_pages() may clear
MIGRATE_PFN_MIGRATE from src_pfn if the page could not be migrated for
some reason.
> 844 migrate_vma_finalize(&args);
> 845
> 846 if (src_pfn & MIGRATE_PFN_MIGRATE)
> 847 return 0;
>
> Same check again.
I don't see the problem with this?
- Alistair
> 848 return -EBUSY;
> 849 }
>
> regards,
> dan carpenter
^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: [bug report] mm/gup: migrate device coherent pages when pinning instead of failing
2022-07-19 12:04 ` Alistair Popple
@ 2022-07-19 13:00 ` Dan Carpenter
0 siblings, 0 replies; 3+ messages in thread
From: Dan Carpenter @ 2022-07-19 13:00 UTC (permalink / raw)
To: Alistair Popple; +Cc: linux-mm
On Tue, Jul 19, 2022 at 10:04:57PM +1000, Alistair Popple wrote:
>
> Dan Carpenter <dan.carpenter@oracle.com> writes:
>
> > Hello Alistair Popple,
> >
> > The patch b05a79d4377f: "mm/gup: migrate device coherent pages when
> > pinning instead of failing" from Jul 15, 2022, leads to the following
> > Smatch static checker warning:
> >
> > mm/migrate_device.c:842 migrate_device_coherent_page()
> > warn: duplicate check 'src_pfn & (1 << 1)' (previous on line 832)
> >
> > mm/migrate_device.c
> > 810 int migrate_device_coherent_page(struct page *page)
> > 811 {
> > 812 unsigned long src_pfn, dst_pfn = 0;
> > 813 struct migrate_vma args;
> > 814 struct page *dpage;
> > 815
> > 816 WARN_ON_ONCE(PageCompound(page));
> > 817
> > 818 lock_page(page);
> > 819 src_pfn = migrate_pfn(page_to_pfn(page)) | MIGRATE_PFN_MIGRATE;
> > 820 args.src = &src_pfn;
> > 821 args.dst = &dst_pfn;
> > 822 args.cpages = 1;
> > 823 args.npages = 1;
> > 824 args.vma = NULL;
> > 825
> > 826 /*
> > 827 * We don't have a VMA and don't need to walk the page tables to find
> > 828 * the source page. So call migrate_vma_unmap() directly to unmap the
> > 829 * page as migrate_vma_setup() will fail if args.vma == NULL.
> > 830 */
> > 831 migrate_vma_unmap(&args);
> > 832 if (!(src_pfn & MIGRATE_PFN_MIGRATE))
> > 833 return -EBUSY;
> >
> > Return here.
> >
> > 834
> > 835 dpage = alloc_page(GFP_USER | __GFP_NOWARN);
> > 836 if (dpage) {
> > 837 lock_page(dpage);
> > 838 dst_pfn = migrate_pfn(page_to_pfn(dpage));
> > 839 }
> > 840
> > 841 migrate_vma_pages(&args);
> > --> 842 if (src_pfn & MIGRATE_PFN_MIGRATE)
> > 843 copy_highpage(dpage, page);
> >
> > No need to check again. Was dst_pfn intended?
>
> No. The check is necessary and correct. migrate_vma_pages() may clear
> MIGRATE_PFN_MIGRATE from src_pfn if the page could not be migrated for
> some reason.
Ah. Sorry. I missed that we save args.src = &src_pfn at the top. I'll
update the checker for this. It might take a week or two to get around
to it...
regards,
dan carpenter
^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2022-07-19 13:00 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-07-19 9:41 [bug report] mm/gup: migrate device coherent pages when pinning instead of failing Dan Carpenter
2022-07-19 12:04 ` Alistair Popple
2022-07-19 13:00 ` Dan Carpenter
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.