From: Mike Kravetz <mike.kravetz@oracle.com>
To: Vlastimil Babka <vbabka@suse.cz>,
linux-mm@kvack.org, linux-kernel@vger.kernel.org,
linux-api@vger.kernel.org
Cc: Reinette Chatre <reinette.chatre@intel.com>,
Michal Hocko <mhocko@kernel.org>,
Christopher Lameter <cl@linux.com>,
Guy Shattah <sguy@mellanox.com>,
Anshuman Khandual <khandual@linux.vnet.ibm.com>,
Michal Nazarewicz <mina86@mina86.com>,
David Nellans <dnellans@nvidia.com>,
Laura Abbott <labbott@redhat.com>, Pavel Machek <pavel@ucw.cz>,
Dave Hansen <dave.hansen@intel.com>,
Andrew Morton <akpm@linux-foundation.org>,
Joonsoo Kim <iamjoonsoo.kim@lge.com>,
Marek Szyprowski <m.szyprowski@samsung.com>
Subject: Re: [PATCH v2 1/4] mm: change type of free_contig_range(nr_pages) to unsigned long
Date: Fri, 18 May 2018 15:01:20 -0700 [thread overview]
Message-ID: <87943a21-4465-31bd-ed37-69253879ec47@oracle.com> (raw)
In-Reply-To: <87d74ce0-b135-064d-a589-64235e44c388@suse.cz>
On 05/18/2018 02:12 AM, Vlastimil Babka wrote:
> On 05/04/2018 01:29 AM, Mike Kravetz wrote:
>> free_contig_range() is currently defined as:
>> void free_contig_range(unsigned long pfn, unsigned nr_pages);
>> change to,
>> void free_contig_range(unsigned long pfn, unsigned long nr_pages);
>>
>> Some callers are passing a truncated unsigned long today. It is
>> highly unlikely that these values will overflow an unsigned int.
>> However, this should be changed to an unsigned long to be consistent
>> with other page counts.
>>
>> Signed-off-by: Mike Kravetz <mike.kravetz@oracle.com>
>> ---
>> include/linux/gfp.h | 2 +-
>> mm/cma.c | 2 +-
>> mm/hugetlb.c | 2 +-
>> mm/page_alloc.c | 6 +++---
>> 4 files changed, 6 insertions(+), 6 deletions(-)
>>
>> diff --git a/include/linux/gfp.h b/include/linux/gfp.h
>> index 1a4582b44d32..86a0d06463ab 100644
>> --- a/include/linux/gfp.h
>> +++ b/include/linux/gfp.h
>> @@ -572,7 +572,7 @@ static inline bool pm_suspended_storage(void)
>> /* The below functions must be run on a range from a single zone. */
>> extern int alloc_contig_range(unsigned long start, unsigned long end,
>> unsigned migratetype, gfp_t gfp_mask);
>> -extern void free_contig_range(unsigned long pfn, unsigned nr_pages);
>> +extern void free_contig_range(unsigned long pfn, unsigned long nr_pages);
>> #endif
>>
>> #ifdef CONFIG_CMA
>> diff --git a/mm/cma.c b/mm/cma.c
>> index aa40e6c7b042..f473fc2b7cbd 100644
>> --- a/mm/cma.c
>> +++ b/mm/cma.c
>> @@ -563,7 +563,7 @@ bool cma_release(struct cma *cma, const struct page *pages, unsigned int count)
>>
>> VM_BUG_ON(pfn + count > cma->base_pfn + cma->count);
>>
>> - free_contig_range(pfn, count);
>> + free_contig_range(pfn, (unsigned long)count);
>
> I guess this cast from uint to ulong doesn't need to be explicit? But
> instead, cma_release() signature could be also changed to ulong, because
> some of its callers do pass those?
Correct, that cast is not needed.
I like the idea of changing cma_release() to take ulong. Until you mentioned
this, I did not realize that some callers were passing in a truncated ulong.
As noted in my commit message, this truncation is unlikely to be an issue.
But, I think we should 'fix' them if we can.
I'll spin another version with this change.
>
>> cma_clear_bitmap(cma, pfn, count);
>> trace_cma_release(pfn, pages, count);
>>
>> diff --git a/mm/hugetlb.c b/mm/hugetlb.c
>> index 218679138255..c81072ce7510 100644
>> --- a/mm/hugetlb.c
>> +++ b/mm/hugetlb.c
>> @@ -1055,7 +1055,7 @@ static void destroy_compound_gigantic_page(struct page *page,
>>
>> static void free_gigantic_page(struct page *page, unsigned int order)
>> {
>> - free_contig_range(page_to_pfn(page), 1 << order);
>> + free_contig_range(page_to_pfn(page), 1UL << order);
>> }
>>
>> static int __alloc_gigantic_page(unsigned long start_pfn,
>> diff --git a/mm/page_alloc.c b/mm/page_alloc.c
>> index 905db9d7962f..0fd5e8e2456e 100644
>> --- a/mm/page_alloc.c
>> +++ b/mm/page_alloc.c
>> @@ -7937,9 +7937,9 @@ int alloc_contig_range(unsigned long start, unsigned long end,
>> return ret;
>> }
>>
>> -void free_contig_range(unsigned long pfn, unsigned nr_pages)
>> +void free_contig_range(unsigned long pfn, unsigned long nr_pages)
>> {
>> - unsigned int count = 0;
>> + unsigned long count = 0;
>>
>> for (; nr_pages--; pfn++) {
>> struct page *page = pfn_to_page(pfn);
>> @@ -7947,7 +7947,7 @@ void free_contig_range(unsigned long pfn, unsigned nr_pages)
>> count += page_count(page) != 1;
>> __free_page(page);
>> }
>> - WARN(count != 0, "%d pages are still in use!\n", count);
>> + WARN(count != 0, "%ld pages are still in use!\n", count);
>
> Maybe change to %lu while at it?
Yes
> BTW, this warning can theoretically produce false positives, because
> page users have to deal with page_count() being incremented by e.g.
> parallel pfn scanners using get_page_unless_zero(). We also don't detect
> refcount leaks in general. Should we remove it or change it to VM_WARN
> if it's still useful for debugging?
Added Marek on Cc: as he is the one who originally added this message (although
it has been a long time). I do not know what specific issue he was concerned
with. A search found a few bug reports related to this warning. In these
cases, it clearly was not a false positive but some other issue. It 'appears'
the message helped in those cases.
However, I would hate for a support organization to spend a bunch of time
doing investigation for a false positive. At the very least, I think we
should add a message/comment about the possibility of false positives in the
code. I would be inclined to change it to VM_WARN, but it would be good
to get input from others who might find it useful as it.
--
Mike Kravetz
>
>> }
>> #endif
>>
>>
>
next prev parent reply other threads:[~2018-05-18 22:01 UTC|newest]
Thread overview: 19+ messages / expand[flat|nested] mbox.gz Atom feed top
2018-05-03 23:29 [PATCH v2 0/4] Interface for higher order contiguous allocations Mike Kravetz
2018-05-03 23:29 ` [PATCH v2 1/4] mm: change type of free_contig_range(nr_pages) to unsigned long Mike Kravetz
2018-05-18 9:12 ` Vlastimil Babka
2018-05-18 22:01 ` Mike Kravetz [this message]
2018-05-03 23:29 ` [PATCH v2 2/4] mm: check for proper migrate type during isolation Mike Kravetz
2018-05-18 10:32 ` Vlastimil Babka
2018-05-21 23:10 ` Mike Kravetz
2018-05-22 7:07 ` Vlastimil Babka
2018-05-03 23:29 ` [PATCH v2 3/4] mm: add find_alloc_contig_pages() interface Mike Kravetz
2018-05-21 8:54 ` Vlastimil Babka
2018-05-21 23:48 ` Mike Kravetz
2018-05-22 16:41 ` Reinette Chatre
2018-05-22 20:35 ` Mike Kravetz
2018-05-23 11:18 ` Vlastimil Babka
2018-05-23 18:07 ` Reinette Chatre
2018-05-28 13:12 ` Vlastimil Babka
2018-05-03 23:29 ` [PATCH v2 4/4] mm/hugetlb: use find_alloc_contig_pages() to allocate gigantic pages Mike Kravetz
2018-05-21 12:00 ` [PATCH v2 0/4] Interface for higher order contiguous allocations Vlastimil Babka
2018-05-22 0:15 ` Mike Kravetz
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=87943a21-4465-31bd-ed37-69253879ec47@oracle.com \
--to=mike.kravetz@oracle.com \
--cc=akpm@linux-foundation.org \
--cc=cl@linux.com \
--cc=dave.hansen@intel.com \
--cc=dnellans@nvidia.com \
--cc=iamjoonsoo.kim@lge.com \
--cc=khandual@linux.vnet.ibm.com \
--cc=labbott@redhat.com \
--cc=linux-api@vger.kernel.org \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-mm@kvack.org \
--cc=m.szyprowski@samsung.com \
--cc=mhocko@kernel.org \
--cc=mina86@mina86.com \
--cc=pavel@ucw.cz \
--cc=reinette.chatre@intel.com \
--cc=sguy@mellanox.com \
--cc=vbabka@suse.cz \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).