Pages on CMA area could have MIGRATE_ISOLATE as well as MIGRATE_CMA
so current is_pinnable_page could miss CMA pages which has MIGRATE_
ISOLATE. It ends up putting CMA pages longterm pinning possible on
pin_user_pages APIs so CMA allocation fails.
The CMA allocation path protects the migration type change race
using zone->lock but what GUP path need to know is just whether the
page is on CMA area or not rather than exact type. Thus, we don't
need zone->lock but just checks the migratype in either of
(MIGRATE_ISOLATE and MIGRATE_CMA).
Signed-off-by: Minchan Kim <[email protected]>
---
include/linux/mm.h | 6 ++++--
1 file changed, 4 insertions(+), 2 deletions(-)
diff --git a/include/linux/mm.h b/include/linux/mm.h
index 6acca5cecbc5..f59bbe3296e3 100644
--- a/include/linux/mm.h
+++ b/include/linux/mm.h
@@ -1625,8 +1625,10 @@ static inline bool page_needs_cow_for_dma(struct vm_area_struct *vma,
#ifdef CONFIG_MIGRATION
static inline bool is_pinnable_page(struct page *page)
{
- return !(is_zone_movable_page(page) || is_migrate_cma_page(page)) ||
- is_zero_pfn(page_to_pfn(page));
+ int mt = get_pageblock_migratetype(page);
+
+ return !(is_zone_movable_page(page) || mt == MIGRATE_CMA ||
+ mt == MIGRATE_ISOLATE || is_zero_pfn(page_to_pfn(page)));
}
#else
static inline bool is_pinnable_page(struct page *page)
--
2.36.0.464.gb9c8b46e94-goog
On 02.05.22 19:35, Minchan Kim wrote:
> Pages on CMA area could have MIGRATE_ISOLATE as well as MIGRATE_CMA
> so current is_pinnable_page could miss CMA pages which has MIGRATE_
> ISOLATE. It ends up putting CMA pages longterm pinning possible on
> pin_user_pages APIs so CMA allocation fails.
>
> The CMA allocation path protects the migration type change race
> using zone->lock but what GUP path need to know is just whether the
> page is on CMA area or not rather than exact type. Thus, we don't
> need zone->lock but just checks the migratype in either of
> (MIGRATE_ISOLATE and MIGRATE_CMA).
>
> Signed-off-by: Minchan Kim <[email protected]>
> ---
> include/linux/mm.h | 6 ++++--
> 1 file changed, 4 insertions(+), 2 deletions(-)
>
> diff --git a/include/linux/mm.h b/include/linux/mm.h
> index 6acca5cecbc5..f59bbe3296e3 100644
> --- a/include/linux/mm.h
> +++ b/include/linux/mm.h
> @@ -1625,8 +1625,10 @@ static inline bool page_needs_cow_for_dma(struct vm_area_struct *vma,
> #ifdef CONFIG_MIGRATION
> static inline bool is_pinnable_page(struct page *page)
> {
> - return !(is_zone_movable_page(page) || is_migrate_cma_page(page)) ||
> - is_zero_pfn(page_to_pfn(page));
> + int mt = get_pageblock_migratetype(page);
> +
> + return !(is_zone_movable_page(page) || mt == MIGRATE_CMA ||
> + mt == MIGRATE_ISOLATE || is_zero_pfn(page_to_pfn(page)));
> }
> #else
> static inline bool is_pinnable_page(struct page *page)
That implies that other memory ranges that are currently isolated
(memory offlining, alloc_contig_range()) cannot be pinned. That might
not be a bad thing, however, I think we could end up failing to pin
something that's temporarily unmovable (due to temporary references).
However, I assume we have the same issue right now already with
ZONE_MOVABLE and MIGRATE_CMA when trying to pin a page residing on these
there are temporarily unmovable and we fail to migrate. But it would now
apply even without ZONE_MOVABLE or MIGRATE_CMA. Hm...
--
Thanks,
David / dhildenb
Hi Minchan,
I love your patch! Yet something to improve:
[auto build test ERROR on hnaz-mm/master]
url: https://github.com/intel-lab-lkp/linux/commits/Minchan-Kim/mm-fix-is_pinnable_page-against-on-cma-page/20220503-013733
base: https://github.com/hnaz/linux-mm master
config: nios2-defconfig (https://download.01.org/0day-ci/archive/20220503/[email protected]/config)
compiler: nios2-linux-gcc (GCC) 11.3.0
reproduce (this is a W=1 build):
wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
chmod +x ~/bin/make.cross
# https://github.com/intel-lab-lkp/linux/commit/1b8636710c31d44310f1d344e337c207562b851d
git remote add linux-review https://github.com/intel-lab-lkp/linux
git fetch --no-tags linux-review Minchan-Kim/mm-fix-is_pinnable_page-against-on-cma-page/20220503-013733
git checkout 1b8636710c31d44310f1d344e337c207562b851d
# save the config file
mkdir build_dir && cp config build_dir/.config
COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-11.3.0 make.cross W=1 O=build_dir ARCH=nios2 prepare
If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <[email protected]>
All errors (new ones prefixed by >>):
In file included from include/linux/pid_namespace.h:7,
from include/linux/ptrace.h:10,
from arch/nios2/kernel/asm-offsets.c:9:
include/linux/mm.h: In function 'is_pinnable_page':
>> include/linux/mm.h:1630:54: error: 'MIGRATE_CMA' undeclared (first use in this function); did you mean 'MIGRATE_SYNC'?
1630 | return !(is_zone_movable_page(page) || mt == MIGRATE_CMA ||
| ^~~~~~~~~~~
| MIGRATE_SYNC
include/linux/mm.h:1630:54: note: each undeclared identifier is reported only once for each function it appears in
>> include/linux/mm.h:1631:23: error: 'MIGRATE_ISOLATE' undeclared (first use in this function); did you mean 'MIGRATE_MOVABLE'?
1631 | mt == MIGRATE_ISOLATE || is_zero_pfn(page_to_pfn(page)));
| ^~~~~~~~~~~~~~~
| MIGRATE_MOVABLE
make[2]: *** [scripts/Makefile.build:122: arch/nios2/kernel/asm-offsets.s] Error 1
make[2]: Target '__build' not remade because of errors.
make[1]: *** [Makefile:1283: prepare0] Error 2
make[1]: Target 'prepare' not remade because of errors.
make: *** [Makefile:226: __sub-make] Error 2
make: Target 'prepare' not remade because of errors.
vim +1630 include/linux/mm.h
1623
1624 /* MIGRATE_CMA and ZONE_MOVABLE do not allow pin pages */
1625 #ifdef CONFIG_MIGRATION
1626 static inline bool is_pinnable_page(struct page *page)
1627 {
1628 int mt = get_pageblock_migratetype(page);
1629
> 1630 return !(is_zone_movable_page(page) || mt == MIGRATE_CMA ||
> 1631 mt == MIGRATE_ISOLATE || is_zero_pfn(page_to_pfn(page)));
1632 }
1633 #else
1634 static inline bool is_pinnable_page(struct page *page)
1635 {
1636 return true;
1637 }
1638 #endif
1639
--
0-DAY CI Kernel Test Service
https://01.org/lkp
Hi Minchan,
I love your patch! Perhaps something to improve:
[auto build test WARNING on hnaz-mm/master]
url: https://github.com/intel-lab-lkp/linux/commits/Minchan-Kim/mm-fix-is_pinnable_page-against-on-cma-page/20220503-013733
base: https://github.com/hnaz/linux-mm master
config: hexagon-randconfig-r034-20220501 (https://download.01.org/0day-ci/archive/20220503/[email protected]/config)
compiler: clang version 15.0.0 (https://github.com/llvm/llvm-project 363b3a645a1e30011cc8da624f13dac5fd915628)
reproduce (this is a W=1 build):
wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
chmod +x ~/bin/make.cross
# https://github.com/intel-lab-lkp/linux/commit/1b8636710c31d44310f1d344e337c207562b851d
git remote add linux-review https://github.com/intel-lab-lkp/linux
git fetch --no-tags linux-review Minchan-Kim/mm-fix-is_pinnable_page-against-on-cma-page/20220503-013733
git checkout 1b8636710c31d44310f1d344e337c207562b851d
# save the config file
mkdir build_dir && cp config build_dir/.config
COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross W=1 O=build_dir ARCH=hexagon SHELL=/bin/bash
If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <[email protected]>
All warnings (new ones prefixed by >>):
In file included from fs/statfs.c:2:
In file included from include/linux/syscalls.h:88:
In file included from include/trace/syscall.h:7:
In file included from include/linux/trace_events.h:6:
In file included from include/linux/ring_buffer.h:5:
include/linux/mm.h:1630:47: error: use of undeclared identifier 'MIGRATE_CMA'; did you mean 'MIGRATE_SYNC'?
return !(is_zone_movable_page(page) || mt == MIGRATE_CMA ||
^~~~~~~~~~~
MIGRATE_SYNC
include/linux/migrate_mode.h:18:2: note: 'MIGRATE_SYNC' declared here
MIGRATE_SYNC,
^
In file included from fs/statfs.c:2:
In file included from include/linux/syscalls.h:88:
In file included from include/trace/syscall.h:7:
In file included from include/linux/trace_events.h:6:
In file included from include/linux/ring_buffer.h:5:
include/linux/mm.h:1631:9: error: use of undeclared identifier 'MIGRATE_ISOLATE'; did you mean 'MIGRATE_MOVABLE'?
mt == MIGRATE_ISOLATE || is_zero_pfn(page_to_pfn(page)));
^~~~~~~~~~~~~~~
MIGRATE_MOVABLE
include/linux/mmzone.h:44:2: note: 'MIGRATE_MOVABLE' declared here
MIGRATE_MOVABLE,
^
>> fs/statfs.c:131:3: warning: 'memcpy' will always overflow; destination buffer has size 64, but size argument is 88 [-Wfortify-source]
memcpy(&buf, st, sizeof(*st));
^
1 warning and 2 errors generated.
vim +/memcpy +131 fs/statfs.c
c8b91accfa1059 Al Viro 2011-03-12 125
c8b91accfa1059 Al Viro 2011-03-12 126 static int do_statfs_native(struct kstatfs *st, struct statfs __user *p)
c8b91accfa1059 Al Viro 2011-03-12 127 {
c8b91accfa1059 Al Viro 2011-03-12 128 struct statfs buf;
7ed1ee6118ae77 Al Viro 2010-03-23 129
c8b91accfa1059 Al Viro 2011-03-12 130 if (sizeof(buf) == sizeof(*st))
c8b91accfa1059 Al Viro 2011-03-12 @131 memcpy(&buf, st, sizeof(*st));
7ed1ee6118ae77 Al Viro 2010-03-23 132 else {
c8b91accfa1059 Al Viro 2011-03-12 133 if (sizeof buf.f_blocks == 4) {
c8b91accfa1059 Al Viro 2011-03-12 134 if ((st->f_blocks | st->f_bfree | st->f_bavail |
c8b91accfa1059 Al Viro 2011-03-12 135 st->f_bsize | st->f_frsize) &
7ed1ee6118ae77 Al Viro 2010-03-23 136 0xffffffff00000000ULL)
7ed1ee6118ae77 Al Viro 2010-03-23 137 return -EOVERFLOW;
7ed1ee6118ae77 Al Viro 2010-03-23 138 /*
7ed1ee6118ae77 Al Viro 2010-03-23 139 * f_files and f_ffree may be -1; it's okay to stuff
7ed1ee6118ae77 Al Viro 2010-03-23 140 * that into 32 bits
7ed1ee6118ae77 Al Viro 2010-03-23 141 */
c8b91accfa1059 Al Viro 2011-03-12 142 if (st->f_files != -1 &&
c8b91accfa1059 Al Viro 2011-03-12 143 (st->f_files & 0xffffffff00000000ULL))
7ed1ee6118ae77 Al Viro 2010-03-23 144 return -EOVERFLOW;
c8b91accfa1059 Al Viro 2011-03-12 145 if (st->f_ffree != -1 &&
c8b91accfa1059 Al Viro 2011-03-12 146 (st->f_ffree & 0xffffffff00000000ULL))
7ed1ee6118ae77 Al Viro 2010-03-23 147 return -EOVERFLOW;
7ed1ee6118ae77 Al Viro 2010-03-23 148 }
7ed1ee6118ae77 Al Viro 2010-03-23 149
c8b91accfa1059 Al Viro 2011-03-12 150 buf.f_type = st->f_type;
c8b91accfa1059 Al Viro 2011-03-12 151 buf.f_bsize = st->f_bsize;
c8b91accfa1059 Al Viro 2011-03-12 152 buf.f_blocks = st->f_blocks;
c8b91accfa1059 Al Viro 2011-03-12 153 buf.f_bfree = st->f_bfree;
c8b91accfa1059 Al Viro 2011-03-12 154 buf.f_bavail = st->f_bavail;
c8b91accfa1059 Al Viro 2011-03-12 155 buf.f_files = st->f_files;
c8b91accfa1059 Al Viro 2011-03-12 156 buf.f_ffree = st->f_ffree;
c8b91accfa1059 Al Viro 2011-03-12 157 buf.f_fsid = st->f_fsid;
c8b91accfa1059 Al Viro 2011-03-12 158 buf.f_namelen = st->f_namelen;
c8b91accfa1059 Al Viro 2011-03-12 159 buf.f_frsize = st->f_frsize;
c8b91accfa1059 Al Viro 2011-03-12 160 buf.f_flags = st->f_flags;
c8b91accfa1059 Al Viro 2011-03-12 161 memset(buf.f_spare, 0, sizeof(buf.f_spare));
c8b91accfa1059 Al Viro 2011-03-12 162 }
c8b91accfa1059 Al Viro 2011-03-12 163 if (copy_to_user(p, &buf, sizeof(buf)))
c8b91accfa1059 Al Viro 2011-03-12 164 return -EFAULT;
7ed1ee6118ae77 Al Viro 2010-03-23 165 return 0;
7ed1ee6118ae77 Al Viro 2010-03-23 166 }
7ed1ee6118ae77 Al Viro 2010-03-23 167
--
0-DAY CI Kernel Test Service
https://01.org/lkp
On 03.05.22 17:26, Minchan Kim wrote:
> On Tue, May 03, 2022 at 03:15:24AM +0200, David Hildenbrand wrote:
>>
>>>>>> However, I assume we have the same issue right now already with
>>>> ZONE_MOVABLE and MIGRATE_CMA when trying to pin a page residing on these
>>>
>>> ZONE_MOVALBE is also changed dynamically?
>>>
>>
>> Sorry, with "same issue" I meant failing to pin if having to migrate and
>> the page is temporarily unmovable.
>>
>>>> there are temporarily unmovable and we fail to migrate. But it would now
>>>> apply even without ZONE_MOVABLE or MIGRATE_CMA. Hm...
>>>
>>> Didn't parse your last mention.
>>
>> On a system that neither uses ZONE_MOVABLE nor MIGRATE_CMA we might have
>> to migrate now when pinning.
>
> I don't understand your point. My problem is pin_user_pages with
> FOLL_LONGTERM. It shouldn't pin a page from ZONE_MOVABLE and cma area
> without migrating page out of movable zone or CMA area.
> That's why try_grab_folio checks whether target page stays in those
> movable areas. However, to check CMA area, is_migrate_cma_page is
> racy so the FOLL_LONGTERM flag semantic is broken right now.
>
> Do you see any problem of the fix?
My point is that you might decide to migrate a page because you stumble
over MIGRATE_ISOLATE, although there is no need to reject long-term
pinning and to trigger page migration.
Assume a system without ZONE_MOVABLE and without MIGRATE_CMA. Assume
someone reserves gigantic pages (alloc_contig_range()) and you have
concurrent long-term pinning on a page that is no MIGRATE_ISOLATE.
GUP would see MIGRATE_ISOLATE and would reject pinning. The page has to
be migrated, which can fail if the page is temporarily unmovable.
See my point? We will try migrating in cases where we don't have to
migrate. I think what we would want to do is always reject pinning a CMA
page, independent of the isolation status. but we don't have that
information available.
I raised in the past that we should look into preserving the migration
type and turning MIGRATE_ISOLATE essentially into an additional flag.
So I guess this patch is the right thing to do for now, but I wanted to
spell out the implications.
>
> A thing to get some attention is whether we need READ_ONCE or not
> for the local variable mt.
>
Hmm good point. Staring at __get_pfnblock_flags_mask(), I don't think
there is anything stopping the compiler from re-reading the value. But
we don't care if we're reading MIGRATE_CMA or MIGRATE_ISOLATE, not
something in between.
--
Thanks,
David / dhildenb
>> GUP would see MIGRATE_ISOLATE and would reject pinning. The page has to
>> be migrated, which can fail if the page is temporarily unmovable.
>
> Why is the page temporarily unmovable? The GUP didn't increase the
> refcount in the case. If it's not migrabtable, that's not a fault
> from the GUP but someone is already holding the temporal refcount.
> It's not the scope this patchset would try to solve it.
You can have other references on the page that turn it temporarily
unmovable, for example, via FOLL_GET, short-term FOLL_PIN.
>
>>
>> See my point? We will try migrating in cases where we don't have to
>
> Still not clear for me what you are concerning.
>
>> migrate. I think what we would want to do is always reject pinning a CMA
>> page, independent of the isolation status. but we don't have that
>
> Always reject pinning a CMA page if it is *FOLL_LONGTERM*
Yes.
>
>> information available.
>
> page && (MIGRATE_CMA | MIGRATE_ISOLATE) && gup_flags is not enough
> for it?
>
>>
>> I raised in the past that we should look into preserving the migration
>> type and turning MIGRATE_ISOLATE essentially into an additional flag.
>>
>>
>> So I guess this patch is the right thing to do for now, but I wanted to
>> spell out the implications.
>
> I want but still don't understand what you want to write further
> about the implication parts. If you make more clear, I am happy to
> include it.
What I am essentially saying is that when rejecting to long-term
FOLL_PIN something that is MIGRATE_ISOLATE now, we might now end up
having to migrate pages that are actually fine to get pinned, because
they are not actual CMA pages. And any such migration might fail when
pages are temporarily unmovable.
>
>>
>>>
>>> A thing to get some attention is whether we need READ_ONCE or not
>>> for the local variable mt.
>>>
>>
>> Hmm good point. Staring at __get_pfnblock_flags_mask(), I don't think
>> there is anything stopping the compiler from re-reading the value. But
>> we don't care if we're reading MIGRATE_CMA or MIGRATE_ISOLATE, not
>> something in between.
>
> How about this?
>
> CPU A CPU B
>
> is_pinnable_page
> ..
> .. set_pageblock_migratetype(MIGRATE_ISOLATE)
> mt == MIGRATE_CMA
> get_pageblock_miratetype(page)
> returns MIGRATE_ISOLATE
> mt == MIGRATE_ISOLATE set_pageblock_migratetype(MIGRATE_CMA)
> get_pageblock_miratetype(page)
> returns MIGRATE_CMA
>
> So both conditions fails to detect it.
I think you're right. That's nasty.
--
Thanks,
David / dhildenb
On Tue, May 03, 2022 at 07:27:06PM +0200, David Hildenbrand wrote:
> >> GUP would see MIGRATE_ISOLATE and would reject pinning. The page has to
> >> be migrated, which can fail if the page is temporarily unmovable.
> >
> > Why is the page temporarily unmovable? The GUP didn't increase the
> > refcount in the case. If it's not migrabtable, that's not a fault
> > from the GUP but someone is already holding the temporal refcount.
> > It's not the scope this patchset would try to solve it.
>
> You can have other references on the page that turn it temporarily
> unmovable, for example, via FOLL_GET, short-term FOLL_PIN.
Sure. However, user didn't passed the FOLL_LONGTERM. In that case,
the temporal page migration failure was expected.
What we want to guarantee for successful page migration is only
FOLL_LONGTERM.
If you are talking about the general problem(any GUP API without
FOLL_LONGTERM flag which is supposed to be short-term could turn
into long-term pinning by several reasons - I had struggled with
those issues - FOLL_LONGTERM is misnormer to me), yeah, I agree
we need to fix it but it's orthgonal issue.
>
> >
> >>
> >> See my point? We will try migrating in cases where we don't have to
> >
> > Still not clear for me what you are concerning.
> >
> >> migrate. I think what we would want to do is always reject pinning a CMA
> >> page, independent of the isolation status. but we don't have that
> >
> > Always reject pinning a CMA page if it is *FOLL_LONGTERM*
>
> Yes.
>
> >
> >> information available.
> >
> > page && (MIGRATE_CMA | MIGRATE_ISOLATE) && gup_flags is not enough
> > for it?
> >
> >>
> >> I raised in the past that we should look into preserving the migration
> >> type and turning MIGRATE_ISOLATE essentially into an additional flag.
> >>
> >>
> >> So I guess this patch is the right thing to do for now, but I wanted to
> >> spell out the implications.
> >
> > I want but still don't understand what you want to write further
> > about the implication parts. If you make more clear, I am happy to
> > include it.
>
> What I am essentially saying is that when rejecting to long-term
> FOLL_PIN something that is MIGRATE_ISOLATE now, we might now end up
> having to migrate pages that are actually fine to get pinned, because
> they are not actual CMA pages. And any such migration might fail when
> pages are temporarily unmovable.
Now I understand concern. Then how about introducing cma areas list
and use it instead of migrate type in is_pinnable_page
struct cma {
..
..
list_head list
};
bool is_cma_page(unsigned long pfn) {
for cma in cma_list
if (pfn >= cma->base_pfn && pfn < cma->base_pfn + count
return true;
return false;
}
Do you want to fix it at this moment or just write down the
possibility in the description and then we could fix once it
really happens later?
>
>
> >
> >>
> >>>
> >>> A thing to get some attention is whether we need READ_ONCE or not
> >>> for the local variable mt.
> >>>
> >>
> >> Hmm good point. Staring at __get_pfnblock_flags_mask(), I don't think
> >> there is anything stopping the compiler from re-reading the value. But
> >> we don't care if we're reading MIGRATE_CMA or MIGRATE_ISOLATE, not
> >> something in between.
> >
> > How about this?
> >
> > CPU A CPU B
> >
> > is_pinnable_page
> > ..
> > .. set_pageblock_migratetype(MIGRATE_ISOLATE)
> > mt == MIGRATE_CMA
> > get_pageblock_miratetype(page)
> > returns MIGRATE_ISOLATE
> > mt == MIGRATE_ISOLATE set_pageblock_migratetype(MIGRATE_CMA)
> > get_pageblock_miratetype(page)
> > returns MIGRATE_CMA
> >
> > So both conditions fails to detect it.
>
> I think you're right. That's nasty.
Ccing Paul to borrow expertise. :)
int mt = get_pageblock_migratetype(page);
if (mt == MIGRATE_CMA)
return true;
if (mt == MIGRATE_ISOLATE)
return true;
I'd like to keep use the local variable mt's value in folloing
conditions checks instead of refetching the value from
get_pageblock_migratetype.
What's the right way to achieve it?
Thanks in advance!
On Tue, May 03, 2022 at 06:02:33PM +0200, David Hildenbrand wrote:
> On 03.05.22 17:26, Minchan Kim wrote:
> > On Tue, May 03, 2022 at 03:15:24AM +0200, David Hildenbrand wrote:
> >>
> >>>>>> However, I assume we have the same issue right now already with
> >>>> ZONE_MOVABLE and MIGRATE_CMA when trying to pin a page residing on these
> >>>
> >>> ZONE_MOVALBE is also changed dynamically?
> >>>
> >>
> >> Sorry, with "same issue" I meant failing to pin if having to migrate and
> >> the page is temporarily unmovable.
> >>
> >>>> there are temporarily unmovable and we fail to migrate. But it would now
> >>>> apply even without ZONE_MOVABLE or MIGRATE_CMA. Hm...
> >>>
> >>> Didn't parse your last mention.
> >>
> >> On a system that neither uses ZONE_MOVABLE nor MIGRATE_CMA we might have
> >> to migrate now when pinning.
> >
> > I don't understand your point. My problem is pin_user_pages with
> > FOLL_LONGTERM. It shouldn't pin a page from ZONE_MOVABLE and cma area
> > without migrating page out of movable zone or CMA area.
> > That's why try_grab_folio checks whether target page stays in those
> > movable areas. However, to check CMA area, is_migrate_cma_page is
> > racy so the FOLL_LONGTERM flag semantic is broken right now.
> >
> > Do you see any problem of the fix?
>
> My point is that you might decide to migrate a page because you stumble
> over MIGRATE_ISOLATE, although there is no need to reject long-term
> pinning and to trigger page migration.
>
> Assume a system without ZONE_MOVABLE and without MIGRATE_CMA. Assume
> someone reserves gigantic pages (alloc_contig_range()) and you have
> concurrent long-term pinning on a page that is no MIGRATE_ISOLATE.
>
> GUP would see MIGRATE_ISOLATE and would reject pinning. The page has to
> be migrated, which can fail if the page is temporarily unmovable.
Why is the page temporarily unmovable? The GUP didn't increase the
refcount in the case. If it's not migrabtable, that's not a fault
from the GUP but someone is already holding the temporal refcount.
It's not the scope this patchset would try to solve it.
>
> See my point? We will try migrating in cases where we don't have to
Still not clear for me what you are concerning.
> migrate. I think what we would want to do is always reject pinning a CMA
> page, independent of the isolation status. but we don't have that
Always reject pinning a CMA page if it is *FOLL_LONGTERM*
> information available.
page && (MIGRATE_CMA | MIGRATE_ISOLATE) && gup_flags is not enough
for it?
>
> I raised in the past that we should look into preserving the migration
> type and turning MIGRATE_ISOLATE essentially into an additional flag.
>
>
> So I guess this patch is the right thing to do for now, but I wanted to
> spell out the implications.
I want but still don't understand what you want to write further
about the implication parts. If you make more clear, I am happy to
include it.
>
> >
> > A thing to get some attention is whether we need READ_ONCE or not
> > for the local variable mt.
> >
>
> Hmm good point. Staring at __get_pfnblock_flags_mask(), I don't think
> there is anything stopping the compiler from re-reading the value. But
> we don't care if we're reading MIGRATE_CMA or MIGRATE_ISOLATE, not
> something in between.
How about this?
CPU A CPU B
is_pinnable_page
..
.. set_pageblock_migratetype(MIGRATE_ISOLATE)
mt == MIGRATE_CMA
get_pageblock_miratetype(page)
returns MIGRATE_ISOLATE
mt == MIGRATE_ISOLATE set_pageblock_migratetype(MIGRATE_CMA)
get_pageblock_miratetype(page)
returns MIGRATE_CMA
So both conditions fails to detect it.
On Tue, May 03, 2022 at 11:08:25AM -0700, Minchan Kim wrote:
< snip >
Ccing Paul really this time.
Attach original code for Paul.
diff --git a/include/linux/mm.h b/include/linux/mm.h
index 6acca5cecbc5..f59bbe3296e3 100644
--- a/include/linux/mm.h
+++ b/include/linux/mm.h
@@ -1625,8 +1625,10 @@ static inline bool page_needs_cow_for_dma(struct vm_area_struct *vma,
#ifdef CONFIG_MIGRATION
static inline bool is_pinnable_page(struct page *page)
{
- return !(is_zone_movable_page(page) || is_migrate_cma_page(page)) ||
- is_zero_pfn(page_to_pfn(page));
+ int mt = get_pageblock_migratetype(page);
+
+ return !(is_zone_movable_page(page) || mt == MIGRATE_CMA ||
+ mt == MIGRATE_ISOLATE || is_zero_pfn(page_to_pfn(page)));
}
#else
static inline bool is_pinnable_page(struct page *page)
--
2.36.0.464.gb9c8b46e94-goog
> > >>>
> > >>> A thing to get some attention is whether we need READ_ONCE or not
> > >>> for the local variable mt.
> > >>>
> > >>
> > >> Hmm good point. Staring at __get_pfnblock_flags_mask(), I don't think
> > >> there is anything stopping the compiler from re-reading the value. But
> > >> we don't care if we're reading MIGRATE_CMA or MIGRATE_ISOLATE, not
> > >> something in between.
> > >
> > > How about this?
> > >
> > > CPU A CPU B
> > >
> > > is_pinnable_page
> > > ..
> > > .. set_pageblock_migratetype(MIGRATE_ISOLATE)
> > > mt == MIGRATE_CMA
> > > get_pageblock_miratetype(page)
> > > returns MIGRATE_ISOLATE
> > > mt == MIGRATE_ISOLATE set_pageblock_migratetype(MIGRATE_CMA)
> > > get_pageblock_miratetype(page)
> > > returns MIGRATE_CMA
> > >
> > > So both conditions fails to detect it.
> >
> > I think you're right. That's nasty.
>
> Ccing Paul to borrow expertise. :)
>
> int mt = get_pageblock_migratetype(page);
>
> if (mt == MIGRATE_CMA)
> return true;
>
> if (mt == MIGRATE_ISOLATE)
> return true;
>
> I'd like to keep use the local variable mt's value in folloing
> conditions checks instead of refetching the value from
> get_pageblock_migratetype.
>
> What's the right way to achieve it?
>
> Thanks in advance!
Paul, could you give any hint?
On Wed, May 04, 2022 at 03:48:54PM -0700, Minchan Kim wrote:
> On Tue, May 03, 2022 at 06:02:33PM +0200, David Hildenbrand wrote:
> > On 03.05.22 17:26, Minchan Kim wrote:
> > > On Tue, May 03, 2022 at 03:15:24AM +0200, David Hildenbrand wrote:
> > >>
> > >>>>>> However, I assume we have the same issue right now already with
> > >>>> ZONE_MOVABLE and MIGRATE_CMA when trying to pin a page residing on these
> > >>>
> > >>> ZONE_MOVALBE is also changed dynamically?
> > >>>
> > >>
> > >> Sorry, with "same issue" I meant failing to pin if having to migrate and
> > >> the page is temporarily unmovable.
> > >>
> > >>>> there are temporarily unmovable and we fail to migrate. But it would now
> > >>>> apply even without ZONE_MOVABLE or MIGRATE_CMA. Hm...
> > >>>
> > >>> Didn't parse your last mention.
> > >>
> > >> On a system that neither uses ZONE_MOVABLE nor MIGRATE_CMA we might have
> > >> to migrate now when pinning.
> > >
> > > I don't understand your point. My problem is pin_user_pages with
> > > FOLL_LONGTERM. It shouldn't pin a page from ZONE_MOVABLE and cma area
> > > without migrating page out of movable zone or CMA area.
> > > That's why try_grab_folio checks whether target page stays in those
> > > movable areas. However, to check CMA area, is_migrate_cma_page is
> > > racy so the FOLL_LONGTERM flag semantic is broken right now.
> > >
> > > Do you see any problem of the fix?
> >
> > My point is that you might decide to migrate a page because you stumble
> > over MIGRATE_ISOLATE, although there is no need to reject long-term
> > pinning and to trigger page migration.
> >
> > Assume a system without ZONE_MOVABLE and without MIGRATE_CMA. Assume
> > someone reserves gigantic pages (alloc_contig_range()) and you have
> > concurrent long-term pinning on a page that is no MIGRATE_ISOLATE.
> >
> > GUP would see MIGRATE_ISOLATE and would reject pinning. The page has to
> > be migrated, which can fail if the page is temporarily unmovable.
>
> A dump question since I'm not familiar with hugetlb.
>
> Is above reasonable scenario?
>
> The gigantic page is about to be created using alloc_contig_range so
> they has MIGRATE_ISOLATE as temporal state. It means no one uses the
> page yet so I guess the page is not mapped at userspace but other is
> trying to access the page using pin_user_pages?
>
Too dump question. Never mind.
Posted v2 - https://lore.kernel.org/all/[email protected]/T/#u
Thanks.
On Thu, May 05, 2022 at 10:00:07AM -0700, Mike Kravetz wrote:
> On 5/4/22 23:48, Minchan Kim wrote:
> > On Wed, May 04, 2022 at 03:48:54PM -0700, Minchan Kim wrote:
> >> On Tue, May 03, 2022 at 06:02:33PM +0200, David Hildenbrand wrote:
> >>> On 03.05.22 17:26, Minchan Kim wrote:
> >>>> On Tue, May 03, 2022 at 03:15:24AM +0200, David Hildenbrand wrote:
> >>>>>
> >>>>>>>>> However, I assume we have the same issue right now already with
> >>>>>>> ZONE_MOVABLE and MIGRATE_CMA when trying to pin a page residing on these
> >>>>>>
> >>>>>> ZONE_MOVALBE is also changed dynamically?
> >>>>>>
> >>>>>
> >>>>> Sorry, with "same issue" I meant failing to pin if having to migrate and
> >>>>> the page is temporarily unmovable.
> >>>>>
> >>>>>>> there are temporarily unmovable and we fail to migrate. But it would now
> >>>>>>> apply even without ZONE_MOVABLE or MIGRATE_CMA. Hm...
> >>>>>>
> >>>>>> Didn't parse your last mention.
> >>>>>
> >>>>> On a system that neither uses ZONE_MOVABLE nor MIGRATE_CMA we might have
> >>>>> to migrate now when pinning.
> >>>>
> >>>> I don't understand your point. My problem is pin_user_pages with
> >>>> FOLL_LONGTERM. It shouldn't pin a page from ZONE_MOVABLE and cma area
> >>>> without migrating page out of movable zone or CMA area.
> >>>> That's why try_grab_folio checks whether target page stays in those
> >>>> movable areas. However, to check CMA area, is_migrate_cma_page is
> >>>> racy so the FOLL_LONGTERM flag semantic is broken right now.
> >>>>
> >>>> Do you see any problem of the fix?
> >>>
> >>> My point is that you might decide to migrate a page because you stumble
> >>> over MIGRATE_ISOLATE, although there is no need to reject long-term
> >>> pinning and to trigger page migration.
> >>>
> >>> Assume a system without ZONE_MOVABLE and without MIGRATE_CMA. Assume
> >>> someone reserves gigantic pages (alloc_contig_range()) and you have
> >>> concurrent long-term pinning on a page that is no MIGRATE_ISOLATE.
> >>>
> >>> GUP would see MIGRATE_ISOLATE and would reject pinning. The page has to
> >>> be migrated, which can fail if the page is temporarily unmovable.
> >>
> >> A dump question since I'm not familiar with hugetlb.
> >>
> >> Is above reasonable scenario?
> >>
> >> The gigantic page is about to be created using alloc_contig_range so
> >> they has MIGRATE_ISOLATE as temporal state. It means no one uses the
> >> page yet so I guess the page is not mapped at userspace but other is
> >> trying to access the page using pin_user_pages?
> >>
> >
> > Too dump question. Never mind.
> > Posted v2 - https://lore.kernel.org/all/[email protected]/T/#u
> >
>
> Well your question mentioned hugetlb so my mail filters caught it :)
>
> Your question caused me to think of the following. No need for any immediate
> change: I think. Just wanted to share.
>
> Suppose someone has reserved CMA for gigantic hugetlb allocations. And,
> suppose FOLL_LONGTERM is attempted on such a page (it would be in use). The
> desired action would be to migrate the page out of CMA. Correct?
>
> Gigantic pages can only be migrated IF there is another (already allocated)
> gigantic page available. The routine to try and allocate a page 'on the fly'
> for migration will fail if passed a gigantic size. There 'might' be a free
> pre-allocated gigantic page. However, if the user set up CMA reserves for
> gigantic page allocations it is likely the free gigantic page is also in CMA.
> Therefore, it can not be used for this migration. So, unless my reasoning
> is wrong, FOLL_LONGTERM would almost always fail for gigantic pages in CMA.
FOLL_LONGTERM on CMA-backed gigantic page would already fail, Thanks for sharing!
Anyway, David's concern was non-CMA-backed gigantic page. The alloc_contig_range
with MIGRATE_ISOLATE runs with concurrent FOLL_LONGTERM pinning, which could
trigger page migration we didn't have before so it might increase FOLL_LONGTERM
GUP failure rate.
On Thu, May 05, 2022 at 10:00:07AM -0700, Mike Kravetz wrote:
> Gigantic pages can only be migrated IF there is another (already allocated)
> gigantic page available. The routine to try and allocate a page 'on the fly'
> for migration will fail if passed a gigantic size. There 'might' be a free
> pre-allocated gigantic page. However, if the user set up CMA reserves for
> gigantic page allocations it is likely the free gigantic page is also in CMA.
> Therefore, it can not be used for this migration. So, unless my reasoning
> is wrong, FOLL_LONGTERM would almost always fail for gigantic pages in CMA.
I'm probably not familiar enough with CMA, but.. I just noticed that if CMA
is destined to not be able to be pinned then maybe it'll lose quite a few
scenarios where pinning is a possible use case. It doesn't even need to be
the major use case, but as long as it's possible (e.g. hypervisors hosting
virtual machines with device assignment) it'll be a hard no to CMA, which
seems to be a pity.
--
Peter Xu
On 05.05.22 19:25, Peter Xu wrote:
> On Thu, May 05, 2022 at 10:00:07AM -0700, Mike Kravetz wrote:
>> Gigantic pages can only be migrated IF there is another (already allocated)
>> gigantic page available. The routine to try and allocate a page 'on the fly'
>> for migration will fail if passed a gigantic size. There 'might' be a free
>> pre-allocated gigantic page. However, if the user set up CMA reserves for
>> gigantic page allocations it is likely the free gigantic page is also in CMA.
>> Therefore, it can not be used for this migration. So, unless my reasoning
>> is wrong, FOLL_LONGTERM would almost always fail for gigantic pages in CMA.
>
> I'm probably not familiar enough with CMA, but.. I just noticed that if CMA
> is destined to not be able to be pinned then maybe it'll lose quite a few
> scenarios where pinning is a possible use case. It doesn't even need to be
> the major use case, but as long as it's possible (e.g. hypervisors hosting
> virtual machines with device assignment) it'll be a hard no to CMA, which
> seems to be a pity.
>
Well, the same applies to ZONE_MOVABLE as well, unfortunately.
Eventually, we might want to disable placing hugetlb pages on CMA areas
if it turns out to be a problem. In case of ZONE_MOVABLE we can already
fail "gracefully" when trying offlining (although that's really far from
beautiful).
--
Thanks,
David / dhildenb
On 5/4/22 23:48, Minchan Kim wrote:
> On Wed, May 04, 2022 at 03:48:54PM -0700, Minchan Kim wrote:
>> On Tue, May 03, 2022 at 06:02:33PM +0200, David Hildenbrand wrote:
>>> On 03.05.22 17:26, Minchan Kim wrote:
>>>> On Tue, May 03, 2022 at 03:15:24AM +0200, David Hildenbrand wrote:
>>>>>
>>>>>>>>> However, I assume we have the same issue right now already with
>>>>>>> ZONE_MOVABLE and MIGRATE_CMA when trying to pin a page residing on these
>>>>>>
>>>>>> ZONE_MOVALBE is also changed dynamically?
>>>>>>
>>>>>
>>>>> Sorry, with "same issue" I meant failing to pin if having to migrate and
>>>>> the page is temporarily unmovable.
>>>>>
>>>>>>> there are temporarily unmovable and we fail to migrate. But it would now
>>>>>>> apply even without ZONE_MOVABLE or MIGRATE_CMA. Hm...
>>>>>>
>>>>>> Didn't parse your last mention.
>>>>>
>>>>> On a system that neither uses ZONE_MOVABLE nor MIGRATE_CMA we might have
>>>>> to migrate now when pinning.
>>>>
>>>> I don't understand your point. My problem is pin_user_pages with
>>>> FOLL_LONGTERM. It shouldn't pin a page from ZONE_MOVABLE and cma area
>>>> without migrating page out of movable zone or CMA area.
>>>> That's why try_grab_folio checks whether target page stays in those
>>>> movable areas. However, to check CMA area, is_migrate_cma_page is
>>>> racy so the FOLL_LONGTERM flag semantic is broken right now.
>>>>
>>>> Do you see any problem of the fix?
>>>
>>> My point is that you might decide to migrate a page because you stumble
>>> over MIGRATE_ISOLATE, although there is no need to reject long-term
>>> pinning and to trigger page migration.
>>>
>>> Assume a system without ZONE_MOVABLE and without MIGRATE_CMA. Assume
>>> someone reserves gigantic pages (alloc_contig_range()) and you have
>>> concurrent long-term pinning on a page that is no MIGRATE_ISOLATE.
>>>
>>> GUP would see MIGRATE_ISOLATE and would reject pinning. The page has to
>>> be migrated, which can fail if the page is temporarily unmovable.
>>
>> A dump question since I'm not familiar with hugetlb.
>>
>> Is above reasonable scenario?
>>
>> The gigantic page is about to be created using alloc_contig_range so
>> they has MIGRATE_ISOLATE as temporal state. It means no one uses the
>> page yet so I guess the page is not mapped at userspace but other is
>> trying to access the page using pin_user_pages?
>>
>
> Too dump question. Never mind.
> Posted v2 - https://lore.kernel.org/all/[email protected]/T/#u
>
Well your question mentioned hugetlb so my mail filters caught it :)
Your question caused me to think of the following. No need for any immediate
change: I think. Just wanted to share.
Suppose someone has reserved CMA for gigantic hugetlb allocations. And,
suppose FOLL_LONGTERM is attempted on such a page (it would be in use). The
desired action would be to migrate the page out of CMA. Correct?
Gigantic pages can only be migrated IF there is another (already allocated)
gigantic page available. The routine to try and allocate a page 'on the fly'
for migration will fail if passed a gigantic size. There 'might' be a free
pre-allocated gigantic page. However, if the user set up CMA reserves for
gigantic page allocations it is likely the free gigantic page is also in CMA.
Therefore, it can not be used for this migration. So, unless my reasoning
is wrong, FOLL_LONGTERM would almost always fail for gigantic pages in CMA.
--
Mike Kravetz
On 05.05.22 08:48, Minchan Kim wrote:
> On Wed, May 04, 2022 at 03:48:54PM -0700, Minchan Kim wrote:
>> On Tue, May 03, 2022 at 06:02:33PM +0200, David Hildenbrand wrote:
>>> On 03.05.22 17:26, Minchan Kim wrote:
>>>> On Tue, May 03, 2022 at 03:15:24AM +0200, David Hildenbrand wrote:
>>>>>
>>>>>>>>> However, I assume we have the same issue right now already with
>>>>>>> ZONE_MOVABLE and MIGRATE_CMA when trying to pin a page residing on these
>>>>>>
>>>>>> ZONE_MOVALBE is also changed dynamically?
>>>>>>
>>>>>
>>>>> Sorry, with "same issue" I meant failing to pin if having to migrate and
>>>>> the page is temporarily unmovable.
>>>>>
>>>>>>> there are temporarily unmovable and we fail to migrate. But it would now
>>>>>>> apply even without ZONE_MOVABLE or MIGRATE_CMA. Hm...
>>>>>>
>>>>>> Didn't parse your last mention.
>>>>>
>>>>> On a system that neither uses ZONE_MOVABLE nor MIGRATE_CMA we might have
>>>>> to migrate now when pinning.
>>>>
>>>> I don't understand your point. My problem is pin_user_pages with
>>>> FOLL_LONGTERM. It shouldn't pin a page from ZONE_MOVABLE and cma area
>>>> without migrating page out of movable zone or CMA area.
>>>> That's why try_grab_folio checks whether target page stays in those
>>>> movable areas. However, to check CMA area, is_migrate_cma_page is
>>>> racy so the FOLL_LONGTERM flag semantic is broken right now.
>>>>
>>>> Do you see any problem of the fix?
>>>
>>> My point is that you might decide to migrate a page because you stumble
>>> over MIGRATE_ISOLATE, although there is no need to reject long-term
>>> pinning and to trigger page migration.
>>>
>>> Assume a system without ZONE_MOVABLE and without MIGRATE_CMA. Assume
>>> someone reserves gigantic pages (alloc_contig_range()) and you have
>>> concurrent long-term pinning on a page that is no MIGRATE_ISOLATE.
>>>
>>> GUP would see MIGRATE_ISOLATE and would reject pinning. The page has to
>>> be migrated, which can fail if the page is temporarily unmovable.
>>
>> A dump question since I'm not familiar with hugetlb.
>>
>> Is above reasonable scenario?
>>
>> The gigantic page is about to be created using alloc_contig_range so
>> they has MIGRATE_ISOLATE as temporal state. It means no one uses the
>> page yet so I guess the page is not mapped at userspace but other is
>> trying to access the page using pin_user_pages?
>>
>
> Too dump question. Never mind.
> Posted v2 - https://lore.kernel.org/all/[email protected]/T/#u
Sorry for the late reply, still traveling :)
Just so we're on the same page: MIGRATE_ISOLATE would be set on
pageblocks that contain either free or movable pages. In case of movable
pages, they are in uese.
Regarding your is_cma_page() proposal, I think we might want to consider
that if it really turns out to be a problem. For now, I'm fine with just
documenting it.
Thanks!
--
Thanks,
David / dhildenb
On Tue, May 03, 2022 at 06:02:33PM +0200, David Hildenbrand wrote:
> On 03.05.22 17:26, Minchan Kim wrote:
> > On Tue, May 03, 2022 at 03:15:24AM +0200, David Hildenbrand wrote:
> >>
> >>>>>> However, I assume we have the same issue right now already with
> >>>> ZONE_MOVABLE and MIGRATE_CMA when trying to pin a page residing on these
> >>>
> >>> ZONE_MOVALBE is also changed dynamically?
> >>>
> >>
> >> Sorry, with "same issue" I meant failing to pin if having to migrate and
> >> the page is temporarily unmovable.
> >>
> >>>> there are temporarily unmovable and we fail to migrate. But it would now
> >>>> apply even without ZONE_MOVABLE or MIGRATE_CMA. Hm...
> >>>
> >>> Didn't parse your last mention.
> >>
> >> On a system that neither uses ZONE_MOVABLE nor MIGRATE_CMA we might have
> >> to migrate now when pinning.
> >
> > I don't understand your point. My problem is pin_user_pages with
> > FOLL_LONGTERM. It shouldn't pin a page from ZONE_MOVABLE and cma area
> > without migrating page out of movable zone or CMA area.
> > That's why try_grab_folio checks whether target page stays in those
> > movable areas. However, to check CMA area, is_migrate_cma_page is
> > racy so the FOLL_LONGTERM flag semantic is broken right now.
> >
> > Do you see any problem of the fix?
>
> My point is that you might decide to migrate a page because you stumble
> over MIGRATE_ISOLATE, although there is no need to reject long-term
> pinning and to trigger page migration.
>
> Assume a system without ZONE_MOVABLE and without MIGRATE_CMA. Assume
> someone reserves gigantic pages (alloc_contig_range()) and you have
> concurrent long-term pinning on a page that is no MIGRATE_ISOLATE.
>
> GUP would see MIGRATE_ISOLATE and would reject pinning. The page has to
> be migrated, which can fail if the page is temporarily unmovable.
A dump question since I'm not familiar with hugetlb.
Is above reasonable scenario?
The gigantic page is about to be created using alloc_contig_range so
they has MIGRATE_ISOLATE as temporal state. It means no one uses the
page yet so I guess the page is not mapped at userspace but other is
trying to access the page using pin_user_pages?
>
> See my point? We will try migrating in cases where we don't have to
> migrate. I think what we would want to do is always reject pinning a CMA
> page, independent of the isolation status. but we don't have that
> information available.
>
> I raised in the past that we should look into preserving the migration
> type and turning MIGRATE_ISOLATE essentially into an additional flag.
>
>
> So I guess this patch is the right thing to do for now, but I wanted to
> spell out the implications.