Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1552882 > unrolled thread

[HMM v15 01/16] mm/free_hot_cold_page: catch ZONE_DEVICE pages

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  [HMM v15 01/16] mm/free_hot_cold_page: catch ZONE_DEVICE pages Jérôme Glisse <jglisse@redhat.com> - 2017-01-06 16:50 +0100
    Re: [HMM v15 01/16] mm/free_hot_cold_page: catch ZONE_DEVICE pages Balbir Singh <bsingharora@gmail.com> - 2017-01-09 10:30 +0100
      Re: [HMM v15 01/16] mm/free_hot_cold_page: catch ZONE_DEVICE pages Dave Hansen <dave.hansen@intel.com> - 2017-01-09 17:30 +0100
        Re: [HMM v15 01/16] mm/free_hot_cold_page: catch ZONE_DEVICE pages Jerome Glisse <jglisse@redhat.com> - 2017-01-09 18:00 +0100
          Re: [HMM v15 01/16] mm/free_hot_cold_page: catch ZONE_DEVICE pages Dave Hansen <dave.hansen@intel.com> - 2017-01-09 18:10 +0100
            Re: [HMM v15 01/16] mm/free_hot_cold_page: catch ZONE_DEVICE pages Jerome Glisse <jglisse@redhat.com> - 2017-01-09 19:00 +0100

#1552882 — [HMM v15 01/16] mm/free_hot_cold_page: catch ZONE_DEVICE pages

Catch page from ZONE_DEVICE in free_hot_cold_page(). This should never
happen as ZONE_DEVICE page must always have an elevated refcount.

This is safety-net to catch any refcounting issues in a sane way for any
ZONE_DEVICE pages.

Signed-off-by: Jérôme Glisse <jglisse@redhat.com>
Cc: Dan Williams <dan.j.williams@intel.com>
Cc: Ross Zwisler <ross.zwisler@linux.intel.com>
---
 mm/page_alloc.c | 10 ++++++++++
 1 file changed, 10 insertions(+)

diff --git a/mm/page_alloc.c b/mm/page_alloc.c
index 1c24112..355beb4 100644
--- a/mm/page_alloc.c
+++ b/mm/page_alloc.c
@@ -2445,6 +2445,16 @@ void free_hot_cold_page(struct page *page, bool cold)
 	unsigned long pfn = page_to_pfn(page);
 	int migratetype;
 
+	/*
+	 * This should never happen ! Page from ZONE_DEVICE always must have an
+	 * active refcount. Complain about it and try to restore the refcount.
+	 */
+	if (is_zone_device_page(page)) {
+		VM_BUG_ON_PAGE(is_zone_device_page(page), page);
+		page_ref_inc(page);
+		return;
+	}
+
 	if (!free_pcp_prepare(page))
 		return;
 
-- 
2.4.3

[toc] | [next] | [standalone]

#1554167

On Fri, Jan 06, 2017 at 11:46:28AM -0500, Jérôme Glisse wrote:
> Catch page from ZONE_DEVICE in free_hot_cold_page(). This should never
> happen as ZONE_DEVICE page must always have an elevated refcount.
> 
> This is safety-net to catch any refcounting issues in a sane way for any
> ZONE_DEVICE pages.
> 
> Signed-off-by: Jérôme Glisse <jglisse@redhat.com>
> Cc: Dan Williams <dan.j.williams@intel.com>
> Cc: Ross Zwisler <ross.zwisler@linux.intel.com>
> ---
>  mm/page_alloc.c | 10 ++++++++++
>  1 file changed, 10 insertions(+)
> 
> diff --git a/mm/page_alloc.c b/mm/page_alloc.c
> index 1c24112..355beb4 100644
> --- a/mm/page_alloc.c
> +++ b/mm/page_alloc.c
> @@ -2445,6 +2445,16 @@ void free_hot_cold_page(struct page *page, bool cold)
>  	unsigned long pfn = page_to_pfn(page);
>  	int migratetype;
>  
> +	/*
> +	 * This should never happen ! Page from ZONE_DEVICE always must have an
> +	 * active refcount. Complain about it and try to restore the refcount.
> +	 */
> +	if (is_zone_device_page(page)) {
> +		VM_BUG_ON_PAGE(is_zone_device_page(page), page);

This can be VM_BUG_ON_PAGE(1, page), hopefully the compiler does the right thing
here. I suspect this should be a BUG_ON, independent of CONFIG_DEBUG_VM

> +		page_ref_inc(page);
> +		return;
> +	}
> +

Balbir Singh.

[toc] | [prev] | [next] | [standalone]

#1554458

On 01/09/2017 01:19 AM, Balbir Singh wrote:
>> +	/*
>> +	 * This should never happen ! Page from ZONE_DEVICE always must have an
>> +	 * active refcount. Complain about it and try to restore the refcount.
>> +	 */
>> +	if (is_zone_device_page(page)) {
>> +		VM_BUG_ON_PAGE(is_zone_device_page(page), page);
> This can be VM_BUG_ON_PAGE(1, page), hopefully the compiler does the right thing
> here. I suspect this should be a BUG_ON, independent of CONFIG_DEBUG_VM

BUG_ON() means "kill the machine dead".  Do we really want a guaranteed
dead machine if someone screws up their refcounting?

[toc] | [prev] | [next] | [standalone]

#1554499

On Mon, Jan 09, 2017 at 08:21:25AM -0800, Dave Hansen wrote:
> On 01/09/2017 01:19 AM, Balbir Singh wrote:
> >> +	/*
> >> +	 * This should never happen ! Page from ZONE_DEVICE always must have an
> >> +	 * active refcount. Complain about it and try to restore the refcount.
> >> +	 */
> >> +	if (is_zone_device_page(page)) {
> >> +		VM_BUG_ON_PAGE(is_zone_device_page(page), page);
> > This can be VM_BUG_ON_PAGE(1, page), hopefully the compiler does the right thing
> > here. I suspect this should be a BUG_ON, independent of CONFIG_DEBUG_VM
> 
> BUG_ON() means "kill the machine dead".  Do we really want a guaranteed
> dead machine if someone screws up their refcounting?

VM_BUG_ON_PAGE ok with you ? It is just a safety net, i can simply drop that
patch if people have too much feeling about it.

Cheers,
Jérôme

[toc] | [prev] | [next] | [standalone]

#1554501

On 01/09/2017 08:57 AM, Jerome Glisse wrote:
> On Mon, Jan 09, 2017 at 08:21:25AM -0800, Dave Hansen wrote:
>> On 01/09/2017 01:19 AM, Balbir Singh wrote:
>>>> +	/*
>>>> +	 * This should never happen ! Page from ZONE_DEVICE always must have an
>>>> +	 * active refcount. Complain about it and try to restore the refcount.
>>>> +	 */
>>>> +	if (is_zone_device_page(page)) {
>>>> +		VM_BUG_ON_PAGE(is_zone_device_page(page), page);
>>> This can be VM_BUG_ON_PAGE(1, page), hopefully the compiler does the right thing
>>> here. I suspect this should be a BUG_ON, independent of CONFIG_DEBUG_VM
>> BUG_ON() means "kill the machine dead".  Do we really want a guaranteed
>> dead machine if someone screws up their refcounting?
> VM_BUG_ON_PAGE ok with you ? It is just a safety net, i can simply drop that
> patch if people have too much feeling about it.

Enough distros turn on DEBUG_VM that there's basically no difference
between VM_BUG_ON() and BUG_ON().

I also think it would be much nicer if you buried the check in the
allocator in a slow path somewhere instead of sticking it in one of the
hottest paths in the whole kernel.

[toc] | [prev] | [next] | [standalone]

#1554558

On Mon, Jan 09, 2017 at 09:00:34AM -0800, Dave Hansen wrote:
> On 01/09/2017 08:57 AM, Jerome Glisse wrote:
> > On Mon, Jan 09, 2017 at 08:21:25AM -0800, Dave Hansen wrote:
> >> On 01/09/2017 01:19 AM, Balbir Singh wrote:
> >>>> +	/*
> >>>> +	 * This should never happen ! Page from ZONE_DEVICE always must have an
> >>>> +	 * active refcount. Complain about it and try to restore the refcount.
> >>>> +	 */
> >>>> +	if (is_zone_device_page(page)) {
> >>>> +		VM_BUG_ON_PAGE(is_zone_device_page(page), page);
> >>> This can be VM_BUG_ON_PAGE(1, page), hopefully the compiler does the right thing
> >>> here. I suspect this should be a BUG_ON, independent of CONFIG_DEBUG_VM
> >> BUG_ON() means "kill the machine dead".  Do we really want a guaranteed
> >> dead machine if someone screws up their refcounting?
> > VM_BUG_ON_PAGE ok with you ? It is just a safety net, i can simply drop that
> > patch if people have too much feeling about it.
> 
> Enough distros turn on DEBUG_VM that there's basically no difference
> between VM_BUG_ON() and BUG_ON().
> 
> I also think it would be much nicer if you buried the check in the
> allocator in a slow path somewhere instead of sticking it in one of the
> hottest paths in the whole kernel.

Well i will just drop that patch then. The point was to catch error
early on before anything happen. This is just a safety net so not
fundamental.

Cheers,
Jérôme

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web