| Message ID | 20201202052330.474592-3-pasha.tatashin@soleen.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | prohibit pinning pages in ZONE_MOVABLE | expand |
On 02.12.20 06:23, Pavel Tatashin wrote: > In order not to fragment CMA the pinned pages are migrated. However, > they are migrated to ZONE_MOVABLE, which also should not have pinned pages. > > Remove __GFP_MOVABLE, so pages can be migrated to zones where pinning > is allowed. > > Signed-off-by: Pavel Tatashin <pasha.tatashin@soleen.com> > --- > mm/gup.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/mm/gup.c b/mm/gup.c > index cdb8b9eeb016..3a76c005a3e2 100644 > --- a/mm/gup.c > +++ b/mm/gup.c > @@ -1610,7 +1610,7 @@ static long check_and_migrate_cma_pages(struct mm_struct *mm, > long ret = nr_pages; > struct migration_target_control mtc = { > .nid = NUMA_NO_NODE, > - .gfp_mask = GFP_USER | __GFP_MOVABLE | __GFP_NOWARN, > + .gfp_mask = GFP_USER | __GFP_NOWARN, > }; > > check_again: > Looks like the right thing to me, thanks! Reviewed-by: David Hildenbrand <david@redhat.com>
> Looks like the right thing to me, thanks! > > Reviewed-by: David Hildenbrand <david@redhat.com> Thank you, Pasha > > -- > Thanks, > > David / dhildenb >
On 12/1/20 9:23 PM, Pavel Tatashin wrote: > In order not to fragment CMA the pinned pages are migrated. However, > they are migrated to ZONE_MOVABLE, which also should not have pinned pages. > > Remove __GFP_MOVABLE, so pages can be migrated to zones where pinning > is allowed. > > Signed-off-by: Pavel Tatashin <pasha.tatashin@soleen.com> > --- > mm/gup.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/mm/gup.c b/mm/gup.c > index cdb8b9eeb016..3a76c005a3e2 100644 > --- a/mm/gup.c > +++ b/mm/gup.c > @@ -1610,7 +1610,7 @@ static long check_and_migrate_cma_pages(struct mm_struct *mm, > long ret = nr_pages; > struct migration_target_control mtc = { > .nid = NUMA_NO_NODE, > - .gfp_mask = GFP_USER | __GFP_MOVABLE | __GFP_NOWARN, > + .gfp_mask = GFP_USER | __GFP_NOWARN, > }; > > check_again: > Reviewed-by: John Hubbard <jhubbard@nvidia.com> ...while I was here, I noticed this, which is outside the scope of your patchset, but I thought I'd just mention it anyway in case anyone cares: static inline bool is_migrate_movable(int mt) { return is_migrate_cma(mt) || mt == MIGRATE_MOVABLE; } ...that really should take an "enum migratetype mt" instead of an int, I think. thanks,
On Wed 02-12-20 00:23:26, Pavel Tatashin wrote: > In order not to fragment CMA the pinned pages are migrated. However, > they are migrated to ZONE_MOVABLE, which also should not have pinned pages. > > Remove __GFP_MOVABLE, so pages can be migrated to zones where pinning > is allowed. I was wondering why we do have __GFP_MOVABLE at all. Took a shovel and... 41b4dc14ee807 says: : We have well defined scope API to exclude CMA region. Use it rather than : manipulating gfp_mask manually. With this change, we can now restore : __GFP_MOVABLE for gfp_mask like as usual migration target allocation. It : would result in that the ZONE_MOVABLE is also searched by page allocator. : For hugetlb, gfp_mask is redefined since it has a regular allocation mask : filter for migration target. __GPF_NOWARN is added to hugetlb gfp_mask : filter since a new user for gfp_mask filter, gup, want to be silent when : allocation fails. This clearly states that the priority was to increase the migration target success rate rather than bother with the pinning aspect of the target page. So I believe we have simply ignored/missed the point of the movable zone guarantees back then and that was a mistake. > Signed-off-by: Pavel Tatashin <pasha.tatashin@soleen.com> I have to admit I am not really sure about the failure path. The code is just too convoluted to follow. I presume the pin will fail in that case. Anyway this wouldn't be anything new in this path. Movable zone exclusion can make the failure slightly more possible in some setups but fundamentally nothing new there. Acked-by: Michal Hocko <mhocko@suse.com> > --- > mm/gup.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/mm/gup.c b/mm/gup.c > index cdb8b9eeb016..3a76c005a3e2 100644 > --- a/mm/gup.c > +++ b/mm/gup.c > @@ -1610,7 +1610,7 @@ static long check_and_migrate_cma_pages(struct mm_struct *mm, > long ret = nr_pages; > struct migration_target_control mtc = { > .nid = NUMA_NO_NODE, > - .gfp_mask = GFP_USER | __GFP_MOVABLE | __GFP_NOWARN, > + .gfp_mask = GFP_USER | __GFP_NOWARN, > }; > > check_again: > -- > 2.25.1 >
On Thu, Dec 3, 2020 at 3:46 AM Michal Hocko <mhocko@suse.com> wrote: > > On Wed 02-12-20 00:23:26, Pavel Tatashin wrote: > > In order not to fragment CMA the pinned pages are migrated. However, > > they are migrated to ZONE_MOVABLE, which also should not have pinned pages. > > > > Remove __GFP_MOVABLE, so pages can be migrated to zones where pinning > > is allowed. > > I was wondering why we do have __GFP_MOVABLE at all. Took a shovel > and... 41b4dc14ee807 says: > : We have well defined scope API to exclude CMA region. Use it rather than > : manipulating gfp_mask manually. With this change, we can now restore > : __GFP_MOVABLE for gfp_mask like as usual migration target allocation. It > : would result in that the ZONE_MOVABLE is also searched by page allocator. > : For hugetlb, gfp_mask is redefined since it has a regular allocation mask > : filter for migration target. __GPF_NOWARN is added to hugetlb gfp_mask > : filter since a new user for gfp_mask filter, gup, want to be silent when > : allocation fails. > > This clearly states that the priority was to increase the migration > target success rate rather than bother with the pinning aspect of the > target page. So I believe we have simply ignored/missed the point of the > movable zone guarantees back then and that was a mistake. > > > Signed-off-by: Pavel Tatashin <pasha.tatashin@soleen.com> > > I have to admit I am not really sure about the failure path. The code is > just too convoluted to follow. I presume the pin will fail in that case. > Anyway this wouldn't be anything new in this path. Movable zone > exclusion can make the failure slightly more possible in some setups but > fundamentally nothing new there. I've been trying to keep this series simple for easier backport, and not to introduce new changes beside increasing the scope of pages which are not allowed to be pinned. This area, however, requires some inspection and fixes, something that Jason also mentioned in another patch. > Acked-by: Michal Hocko <mhocko@suse.com> Thank you, Pasha
diff --git a/mm/gup.c b/mm/gup.c index cdb8b9eeb016..3a76c005a3e2 100644 --- a/mm/gup.c +++ b/mm/gup.c @@ -1610,7 +1610,7 @@ static long check_and_migrate_cma_pages(struct mm_struct *mm, long ret = nr_pages; struct migration_target_control mtc = { .nid = NUMA_NO_NODE, - .gfp_mask = GFP_USER | __GFP_MOVABLE | __GFP_NOWARN, + .gfp_mask = GFP_USER | __GFP_NOWARN, }; check_again:
In order not to fragment CMA the pinned pages are migrated. However, they are migrated to ZONE_MOVABLE, which also should not have pinned pages. Remove __GFP_MOVABLE, so pages can be migrated to zones where pinning is allowed. Signed-off-by: Pavel Tatashin <pasha.tatashin@soleen.com> --- mm/gup.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)