| Message ID | 20230124012210.13963-2-vishal.moola@gmail.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | Convert various mempolicy.c functions | expand |
Hi Vishal,
Thank you for the patch! Yet something to improve:
[auto build test ERROR on akpm-mm/mm-everything]
[also build test ERROR on next-20230123]
[cannot apply to linus/master v6.2-rc5]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]
url: https://github.com/intel-lab-lkp/linux/commits/Vishal-Moola-Oracle/mm-Add-folio_estimated_mapcount/20230124-092349
base: https://git.kernel.org/pub/scm/linux/kernel/git/akpm/mm.git mm-everything
patch link: https://lore.kernel.org/r/20230124012210.13963-2-vishal.moola%40gmail.com
patch subject: [PATCH mm-unstable v2 1/6] mm: Add folio_estimated_mapcount()
config: alpha-allyesconfig (https://download.01.org/0day-ci/archive/20230124/202301241100.GAjve4Wl-lkp@intel.com/config)
compiler: alpha-linux-gcc (GCC) 12.1.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/2ec1fab96da69cd5e71330186987468d7d1a2595
git remote add linux-review https://github.com/intel-lab-lkp/linux
git fetch --no-tags linux-review Vishal-Moola-Oracle/mm-Add-folio_estimated_mapcount/20230124-092349
git checkout 2ec1fab96da69cd5e71330186987468d7d1a2595
# save the config file
mkdir build_dir && cp config build_dir/.config
COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-12.1.0 make.cross W=1 O=build_dir ARCH=alpha olddefconfig
COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-12.1.0 make.cross W=1 O=build_dir ARCH=alpha prepare
If you fix the issue, kindly add following tag where applicable
| Reported-by: kernel test robot <lkp@intel.com>
All errors (new ones prefixed by >>):
In file included from arch/alpha/include/asm/page.h:93,
from include/linux/shm.h:6,
from include/linux/sched.h:16,
from arch/alpha/kernel/asm-offsets.c:10:
include/linux/mm.h: In function 'folio_estimated_mapcount':
>> include/asm-generic/memory_model.h:35:21: error: implicit declaration of function 'page_to_section'; did you mean 'present_section'? [-Werror=implicit-function-declaration]
35 | int __sec = page_to_section(__pg); \
| ^~~~~~~~~~~~~~~
include/asm-generic/memory_model.h:40:32: note: in definition of macro '__pfn_to_page'
40 | ({ unsigned long __pfn = (pfn); \
| ^~~
include/asm-generic/memory_model.h:52:21: note: in expansion of macro '__page_to_pfn'
52 | #define page_to_pfn __page_to_pfn
| ^~~~~~~~~~~~~
include/linux/mm.h:216:38: note: in expansion of macro 'page_to_pfn'
216 | #define nth_page(page,n) pfn_to_page(page_to_pfn((page)) + (n))
| ^~~~~~~~~~~
include/linux/page-flags.h:286:33: note: in expansion of macro 'nth_page'
286 | #define folio_page(folio, n) nth_page(&(folio)->page, n)
| ^~~~~~~~
include/linux/mm.h:918:30: note: in expansion of macro 'folio_page'
918 | return page_mapcount(folio_page(folio, 0));
| ^~~~~~~~~~
In file included from include/linux/pid_namespace.h:7,
from include/linux/ptrace.h:10,
from arch/alpha/kernel/asm-offsets.c:11:
include/linux/mm.h: At top level:
>> include/linux/mm.h:1626:29: error: conflicting types for 'page_to_section'; have 'long unsigned int(const struct page *)'
1626 | static inline unsigned long page_to_section(const struct page *page)
| ^~~~~~~~~~~~~~~
include/asm-generic/memory_model.h:35:21: note: previous implicit declaration of 'page_to_section' with type 'int()'
35 | int __sec = page_to_section(__pg); \
| ^~~~~~~~~~~~~~~
include/asm-generic/memory_model.h:40:32: note: in definition of macro '__pfn_to_page'
40 | ({ unsigned long __pfn = (pfn); \
| ^~~
include/asm-generic/memory_model.h:52:21: note: in expansion of macro '__page_to_pfn'
52 | #define page_to_pfn __page_to_pfn
| ^~~~~~~~~~~~~
include/linux/mm.h:216:38: note: in expansion of macro 'page_to_pfn'
216 | #define nth_page(page,n) pfn_to_page(page_to_pfn((page)) + (n))
| ^~~~~~~~~~~
include/linux/page-flags.h:286:33: note: in expansion of macro 'nth_page'
286 | #define folio_page(folio, n) nth_page(&(folio)->page, n)
| ^~~~~~~~
include/linux/mm.h:918:30: note: in expansion of macro 'folio_page'
918 | return page_mapcount(folio_page(folio, 0));
| ^~~~~~~~~~
arch/alpha/kernel/asm-offsets.c:15:6: warning: no previous prototype for 'foo' [-Wmissing-prototypes]
15 | void foo(void)
| ^~~
cc1: some warnings being treated as errors
make[2]: *** [scripts/Makefile.build:114: arch/alpha/kernel/asm-offsets.s] Error 1
make[2]: Target 'prepare' not remade because of errors.
make[1]: *** [Makefile:1286: prepare0] Error 2
make[1]: Target 'prepare' not remade because of errors.
make: *** [Makefile:242: __sub-make] Error 2
make: Target 'prepare' not remade because of errors.
vim +1626 include/linux/mm.h
bf4e8902ee5080 Daniel Kiper 2011-05-24 1625
aa462abe8aaf21 Ian Campbell 2011-08-17 @1626 static inline unsigned long page_to_section(const struct page *page)
d41dee369bff3b Andy Whitcroft 2005-06-23 1627 {
d41dee369bff3b Andy Whitcroft 2005-06-23 1628 return (page->flags >> SECTIONS_PGSHIFT) & SECTIONS_MASK;
d41dee369bff3b Andy Whitcroft 2005-06-23 1629 }
308c05e35e3517 Christoph Lameter 2008-04-28 1630 #endif
d41dee369bff3b Andy Whitcroft 2005-06-23 1631
On 24.01.23 02:22, Vishal Moola (Oracle) wrote: > folio_estimated_mapcount() takes in a folio and calls page_mapcount() on > the first page of that folio. > > This is necessary for folio conversions where we only care about either the > entire_mapcount of a large folio, or the mapcount of a not large folio. > > This is in contrast to folio_mapcount() which calculates the total > number of the times a folio and its subpages are mapped. > > Signed-off-by: Vishal Moola (Oracle) <vishal.moola@gmail.com> > --- > include/linux/mm.h | 5 +++++ > 1 file changed, 5 insertions(+) > > diff --git a/include/linux/mm.h b/include/linux/mm.h > index c9db257f09b3..543c360f7ecc 100644 > --- a/include/linux/mm.h > +++ b/include/linux/mm.h > @@ -875,6 +875,11 @@ static inline int page_mapcount(struct page *page) > return mapcount; > } > > +static inline int folio_estimated_mapcount(struct folio *folio) > +{ > + return page_mapcount(folio_page(folio, 0)); > +} > + > int folio_total_mapcount(struct folio *folio); > > /** I'm sorry, but "estimated" as absolutely unclear semantics. You could have a THP mapped into 9999 processes using THP and the estimation would be "0". Huh? Absolutely unclear and confusing. No thanks.
On 25.01.23 11:20, David Hildenbrand wrote: > On 24.01.23 02:22, Vishal Moola (Oracle) wrote: >> folio_estimated_mapcount() takes in a folio and calls page_mapcount() on >> the first page of that folio. >> >> This is necessary for folio conversions where we only care about either the >> entire_mapcount of a large folio, or the mapcount of a not large folio. >> >> This is in contrast to folio_mapcount() which calculates the total >> number of the times a folio and its subpages are mapped. >> >> Signed-off-by: Vishal Moola (Oracle) <vishal.moola@gmail.com> >> --- >> include/linux/mm.h | 5 +++++ >> 1 file changed, 5 insertions(+) >> >> diff --git a/include/linux/mm.h b/include/linux/mm.h >> index c9db257f09b3..543c360f7ecc 100644 >> --- a/include/linux/mm.h >> +++ b/include/linux/mm.h >> @@ -875,6 +875,11 @@ static inline int page_mapcount(struct page *page) >> return mapcount; >> } >> >> +static inline int folio_estimated_mapcount(struct folio *folio) >> +{ >> + return page_mapcount(folio_page(folio, 0)); >> +} >> + >> int folio_total_mapcount(struct folio *folio); >> >> /** > > I'm sorry, but "estimated" as absolutely unclear semantics. You could > have a THP mapped into 9999 processes using THP and the estimation would > be "0". ... or would it be 9999 ? What about a PMD-mapped THP? What about a partially unmapped THP? What are we estimating?
On 25.01.23 11:24, David Hildenbrand wrote: > On 25.01.23 11:20, David Hildenbrand wrote: >> On 24.01.23 02:22, Vishal Moola (Oracle) wrote: >>> folio_estimated_mapcount() takes in a folio and calls page_mapcount() on >>> the first page of that folio. >>> >>> This is necessary for folio conversions where we only care about either the >>> entire_mapcount of a large folio, or the mapcount of a not large folio. >>> >>> This is in contrast to folio_mapcount() which calculates the total >>> number of the times a folio and its subpages are mapped. >>> >>> Signed-off-by: Vishal Moola (Oracle) <vishal.moola@gmail.com> >>> --- >>> include/linux/mm.h | 5 +++++ >>> 1 file changed, 5 insertions(+) >>> >>> diff --git a/include/linux/mm.h b/include/linux/mm.h >>> index c9db257f09b3..543c360f7ecc 100644 >>> --- a/include/linux/mm.h >>> +++ b/include/linux/mm.h >>> @@ -875,6 +875,11 @@ static inline int page_mapcount(struct page *page) >>> return mapcount; >>> } >>> >>> +static inline int folio_estimated_mapcount(struct folio *folio) >>> +{ >>> + return page_mapcount(folio_page(folio, 0)); >>> +} >>> + >>> int folio_total_mapcount(struct folio *folio); >>> >>> /** >> >> I'm sorry, but "estimated" as absolutely unclear semantics. You could >> have a THP mapped into 9999 processes using THP and the estimation would >> be "0". > > ... or would it be 9999 ? What about a PMD-mapped THP? What about a > partially unmapped THP? > > What are we estimating? Thinking about mapcounts again, might not have been my smartest moment. What we return here is the precise number of times the first subpage is mapped (via the large folio and directly). That's supposed to be an estimate for the number of times any subpage part of the folio is mapped. I really don't know a better name, but folio_estimated_mapcount() does not feel completely right to me and triggere dmy confusion in the first place ... hm ...
On Wed, Jan 25, 2023 at 1:29 PM David Hildenbrand <david@redhat.com> wrote: > > On 25.01.23 11:24, David Hildenbrand wrote: > > On 25.01.23 11:20, David Hildenbrand wrote: > >> On 24.01.23 02:22, Vishal Moola (Oracle) wrote: > >>> folio_estimated_mapcount() takes in a folio and calls page_mapcount() on > >>> the first page of that folio. > >>> > >>> This is necessary for folio conversions where we only care about either the > >>> entire_mapcount of a large folio, or the mapcount of a not large folio. > >>> > >>> This is in contrast to folio_mapcount() which calculates the total > >>> number of the times a folio and its subpages are mapped. > >>> > >>> Signed-off-by: Vishal Moola (Oracle) <vishal.moola@gmail.com> > >>> --- > >>> include/linux/mm.h | 5 +++++ > >>> 1 file changed, 5 insertions(+) > >>> > >>> diff --git a/include/linux/mm.h b/include/linux/mm.h > >>> index c9db257f09b3..543c360f7ecc 100644 > >>> --- a/include/linux/mm.h > >>> +++ b/include/linux/mm.h > >>> @@ -875,6 +875,11 @@ static inline int page_mapcount(struct page *page) > >>> return mapcount; > >>> } > >>> > >>> +static inline int folio_estimated_mapcount(struct folio *folio) > >>> +{ > >>> + return page_mapcount(folio_page(folio, 0)); > >>> +} > >>> + > >>> int folio_total_mapcount(struct folio *folio); > >>> > >>> /** > >> > >> I'm sorry, but "estimated" as absolutely unclear semantics. You could > >> have a THP mapped into 9999 processes using THP and the estimation would > >> be "0". > > > > ... or would it be 9999 ? What about a PMD-mapped THP? What about a > > partially unmapped THP? > > > > What are we estimating? > > Thinking about mapcounts again, might not have been my smartest moment. > > What we return here is the precise number of times the first subpage is > mapped (via the large folio and directly). That's supposed to be an > estimate for the number of times any subpage part of the folio is mapped. > > I really don't know a better name, but folio_estimated_mapcount() does > not feel completely right to me and triggere dmy confusion in the first > place ... hm ... I can understand the confusion, but I can't think of a better name either myself. I'll go ahead and add a comment to make the purpose of this function more clear. Looks like I'll have to move it to get rid of the build warnings/errors anyway.
On 25.01.23 23:09, Vishal Moola wrote: > On Wed, Jan 25, 2023 at 1:29 PM David Hildenbrand <david@redhat.com> wrote: >> >> On 25.01.23 11:24, David Hildenbrand wrote: >>> On 25.01.23 11:20, David Hildenbrand wrote: >>>> On 24.01.23 02:22, Vishal Moola (Oracle) wrote: >>>>> folio_estimated_mapcount() takes in a folio and calls page_mapcount() on >>>>> the first page of that folio. >>>>> >>>>> This is necessary for folio conversions where we only care about either the >>>>> entire_mapcount of a large folio, or the mapcount of a not large folio. >>>>> >>>>> This is in contrast to folio_mapcount() which calculates the total >>>>> number of the times a folio and its subpages are mapped. >>>>> >>>>> Signed-off-by: Vishal Moola (Oracle) <vishal.moola@gmail.com> >>>>> --- >>>>> include/linux/mm.h | 5 +++++ >>>>> 1 file changed, 5 insertions(+) >>>>> >>>>> diff --git a/include/linux/mm.h b/include/linux/mm.h >>>>> index c9db257f09b3..543c360f7ecc 100644 >>>>> --- a/include/linux/mm.h >>>>> +++ b/include/linux/mm.h >>>>> @@ -875,6 +875,11 @@ static inline int page_mapcount(struct page *page) >>>>> return mapcount; >>>>> } >>>>> >>>>> +static inline int folio_estimated_mapcount(struct folio *folio) >>>>> +{ >>>>> + return page_mapcount(folio_page(folio, 0)); >>>>> +} >>>>> + >>>>> int folio_total_mapcount(struct folio *folio); >>>>> >>>>> /** >>>> >>>> I'm sorry, but "estimated" as absolutely unclear semantics. You could >>>> have a THP mapped into 9999 processes using THP and the estimation would >>>> be "0". >>> >>> ... or would it be 9999 ? What about a PMD-mapped THP? What about a >>> partially unmapped THP? >>> >>> What are we estimating? >> >> Thinking about mapcounts again, might not have been my smartest moment. >> >> What we return here is the precise number of times the first subpage is >> mapped (via the large folio and directly). That's supposed to be an >> estimate for the number of times any subpage part of the folio is mapped. >> >> I really don't know a better name, but folio_estimated_mapcount() does >> not feel completely right to me and triggere dmy confusion in the first >> place ... hm ... > > I can understand the confusion, but I can't think of a better name > either myself. I'll go ahead and add a comment to make the purpose > of this function more clear. Looks like I'll have to move it to get rid > of the build warnings/errors anyway. The issue is that we're not estimating the mapcount of the folio, so the name is very misleading ... I think you really want to avoid the term mapcount completely in that context. We're just using the #mappings of the first subpage to determine something differently. Thinking about it, I guess "folio_estimated_sharers()" might be what we actually want to name it. Then you can comment how we estimate sharers by looking at into how many page tables the first subpage is currently mapped, and assume the same holds true for the other subpages. It's unreliable because other subpages might behave differently, we might not be holding the pagelock to stabilize, and we're not looking at indirect mappings via the swapcache. But it's a comapratively good estimate for most scenarios I guess.
On 1/26/2023 12:37 AM, David Hildenbrand wrote: > On 25.01.23 23:09, Vishal Moola wrote: [..] > > The issue is that we're not estimating the mapcount of the folio, so the > name is very misleading ... I think you really want to avoid the term > mapcount completely in that context. We're just using the #mappings of > the first subpage to determine something differently. > > Thinking about it, I guess "folio_estimated_sharers()" might be what we > actually want to name it. Then you can comment how we estimate sharers > by looking at into how many page tables the first subpage is currently > mapped, and assume the same holds true for the other subpages. > > It's unreliable because other subpages might behave differently, we > might not be holding the pagelock to stabilize, and we're not looking at > indirect mappings via the swapcache. But it's a comapratively good > estimate for most scenarios I guess. > Hmm, how about simply call it folio_hpage_mapcount(), folio_firstpage_mapcount(), or, folio_cover_mapcount() ? It is used to replace page_mapcount() in that sense - https://lore.kernel.org/linux-mm/Y9MDJuPWsk9820xD@x1n/T/#me0531cfb9e82ad5ca88804c727d69cc6b9b33ffa if (flags & (MPOL_MF_MOVE_ALL) || (flags & MPOL_MF_MOVE && folio_estimated_mapcount(folio) == 1 && !hugetlb_pmd_shared(pte))) { if (isolate_hugetlb(folio, qp->pagelist) && thanks, -jane
On 1/26/2023 4:37 PM, David Hildenbrand wrote:
> Thinking about it, I guess "folio_estimated_sharers()" might be what we actually want to name it. Then you can comment how we estimate sharers by looking at into how many page tables the first subpage is currently mapped, and assume the same holds true for the other subpages.
Vote for 'folio_estimated_sharers()'. If better method
other than checking mapcount is found in the future, it's
easy to update the implementation without change the API
name.
Regards
Yin, Fengwei
On 28.01.23 01:48, Jane Chu wrote: > On 1/26/2023 12:37 AM, David Hildenbrand wrote: >> On 25.01.23 23:09, Vishal Moola wrote: > [..] >> >> The issue is that we're not estimating the mapcount of the folio, so the >> name is very misleading ... I think you really want to avoid the term >> mapcount completely in that context. We're just using the #mappings of >> the first subpage to determine something differently. >> >> Thinking about it, I guess "folio_estimated_sharers()" might be what we >> actually want to name it. Then you can comment how we estimate sharers >> by looking at into how many page tables the first subpage is currently >> mapped, and assume the same holds true for the other subpages. >> >> It's unreliable because other subpages might behave differently, we >> might not be holding the pagelock to stabilize, and we're not looking at >> indirect mappings via the swapcache. But it's a comapratively good >> estimate for most scenarios I guess. >> > > Hmm, how about simply call it folio_hpage_mapcount(), > folio_firstpage_mapcount(), or, folio_cover_mapcount() ? All not better IMHO. folio_estimated_subpage_mapcount() is a bit too verbose for my taste and ... > > It is used to replace page_mapcount() in that sense - > https://lore.kernel.org/linux-mm/Y9MDJuPWsk9820xD@x1n/T/#me0531cfb9e82ad5ca88804c727d69cc6b9b33ffa > > if (flags & (MPOL_MF_MOVE_ALL) || > (flags & MPOL_MF_MOVE && folio_estimated_mapcount(folio) == 1 && > !hugetlb_pmd_shared(pte))) { > if (isolate_hugetlb(folio, qp->pagelist) && ... what we want to have here is an estimation on the number of sharers. [actually, we would want it precise, but that's hard to achieve ... ]
diff --git a/include/linux/mm.h b/include/linux/mm.h index c9db257f09b3..543c360f7ecc 100644 --- a/include/linux/mm.h +++ b/include/linux/mm.h @@ -875,6 +875,11 @@ static inline int page_mapcount(struct page *page) return mapcount; } +static inline int folio_estimated_mapcount(struct folio *folio) +{ + return page_mapcount(folio_page(folio, 0)); +} + int folio_total_mapcount(struct folio *folio); /**
folio_estimated_mapcount() takes in a folio and calls page_mapcount() on the first page of that folio. This is necessary for folio conversions where we only care about either the entire_mapcount of a large folio, or the mapcount of a not large folio. This is in contrast to folio_mapcount() which calculates the total number of the times a folio and its subpages are mapped. Signed-off-by: Vishal Moola (Oracle) <vishal.moola@gmail.com> --- include/linux/mm.h | 5 +++++ 1 file changed, 5 insertions(+)