All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH] drm/gem: remove obsolete BUG_ON
@ 2014-05-21 12:16 ` David Herrmann
  0 siblings, 0 replies; 5+ messages in thread
From: David Herrmann @ 2014-05-21 12:16 UTC (permalink / raw)
  To: dri-devel
  Cc: robdclark, airlied, daniel.vetter, hughd, linux-kernel, David Herrmann

The shmem subsystem supports relocating pages on swap-in in case it was
loaded into the wrong zone. This was implemented 2 years ago in:

    commit bde05d1ccd512696b09db9dd2e5f33ad19152605
    Author: Hugh Dickins <hughd@google.com>
    Date:   Tue May 29 15:06:38 2012 -0700

        shmem: replace page if mapping excludes its zone

If a driver requires pages to be in a specific zone, they _must_ set the
correct GFP flags (like __GFP_DMA32) in mapping_gfp_mask(mapping). shmem
will make sure that any page leaving the swap-cache is located in a
compatible zone.

Signed-off-by: David Herrmann <dh.herrmann@gmail.com>
---
 drivers/gpu/drm/drm_gem.c | 19 -------------------
 1 file changed, 19 deletions(-)

diff --git a/drivers/gpu/drm/drm_gem.c b/drivers/gpu/drm/drm_gem.c
index 9909bef..a6146ab 100644
--- a/drivers/gpu/drm/drm_gem.c
+++ b/drivers/gpu/drm/drm_gem.c
@@ -473,25 +473,6 @@ struct page **drm_gem_get_pages(struct drm_gem_object *obj, gfp_t gfpmask)
 		if (IS_ERR(p))
 			goto fail;
 		pages[i] = p;
-
-		/* There is a hypothetical issue w/ drivers that require
-		 * buffer memory in the low 4GB.. if the pages are un-
-		 * pinned, and swapped out, they can end up swapped back
-		 * in above 4GB.  If pages are already in memory, then
-		 * shmem_read_mapping_page_gfp will ignore the gfpmask,
-		 * even if the already in-memory page disobeys the mask.
-		 *
-		 * It is only a theoretical issue today, because none of
-		 * the devices with this limitation can be populated with
-		 * enough memory to trigger the issue.  But this BUG_ON()
-		 * is here as a reminder in case the problem with
-		 * shmem_read_mapping_page_gfp() isn't solved by the time
-		 * it does become a real issue.
-		 *
-		 * See this thread: http://lkml.org/lkml/2011/7/11/238
-		 */
-		BUG_ON((gfpmask & __GFP_DMA32) &&
-				(page_to_pfn(p) >= 0x00100000UL));
 	}
 
 	return pages;
-- 
1.9.3


^ permalink raw reply related	[flat|nested] 5+ messages in thread
* [PATCH] drm/gem: remove obsolete BUG_ON
@ 2014-05-21 12:16 ` David Herrmann
  0 siblings, 0 replies; 5+ messages in thread
From: David Herrmann @ 2014-05-21 12:16 UTC (permalink / raw)
  To: dri-devel; +Cc: daniel.vetter, hughd, linux-kernel

The shmem subsystem supports relocating pages on swap-in in case it was
loaded into the wrong zone. This was implemented 2 years ago in:

    commit bde05d1ccd512696b09db9dd2e5f33ad19152605
    Author: Hugh Dickins <hughd@google.com>
    Date:   Tue May 29 15:06:38 2012 -0700

        shmem: replace page if mapping excludes its zone

If a driver requires pages to be in a specific zone, they _must_ set the
correct GFP flags (like __GFP_DMA32) in mapping_gfp_mask(mapping). shmem
will make sure that any page leaving the swap-cache is located in a
compatible zone.

Signed-off-by: David Herrmann <dh.herrmann@gmail.com>
---
 drivers/gpu/drm/drm_gem.c | 19 -------------------
 1 file changed, 19 deletions(-)

diff --git a/drivers/gpu/drm/drm_gem.c b/drivers/gpu/drm/drm_gem.c
index 9909bef..a6146ab 100644
--- a/drivers/gpu/drm/drm_gem.c
+++ b/drivers/gpu/drm/drm_gem.c
@@ -473,25 +473,6 @@ struct page **drm_gem_get_pages(struct drm_gem_object *obj, gfp_t gfpmask)
 		if (IS_ERR(p))
 			goto fail;
 		pages[i] = p;
-
-		/* There is a hypothetical issue w/ drivers that require
-		 * buffer memory in the low 4GB.. if the pages are un-
-		 * pinned, and swapped out, they can end up swapped back
-		 * in above 4GB.  If pages are already in memory, then
-		 * shmem_read_mapping_page_gfp will ignore the gfpmask,
-		 * even if the already in-memory page disobeys the mask.
-		 *
-		 * It is only a theoretical issue today, because none of
-		 * the devices with this limitation can be populated with
-		 * enough memory to trigger the issue.  But this BUG_ON()
-		 * is here as a reminder in case the problem with
-		 * shmem_read_mapping_page_gfp() isn't solved by the time
-		 * it does become a real issue.
-		 *
-		 * See this thread: http://lkml.org/lkml/2011/7/11/238
-		 */
-		BUG_ON((gfpmask & __GFP_DMA32) &&
-				(page_to_pfn(p) >= 0x00100000UL));
 	}
 
 	return pages;
-- 
1.9.3

^ permalink raw reply related	[flat|nested] 5+ messages in thread
* Re: [PATCH] drm/gem: remove obsolete BUG_ON
  2014-05-21 12:16 ` David Herrmann
@ 2014-05-21 13:17   ` Daniel Vetter
  -1 siblings, 0 replies; 5+ messages in thread
From: Daniel Vetter @ 2014-05-21 13:17 UTC (permalink / raw)
  To: David Herrmann
  Cc: dri-devel, robdclark, airlied, daniel.vetter, hughd, linux-kernel

On Wed, May 21, 2014 at 02:16:26PM +0200, David Herrmann wrote:
> The shmem subsystem supports relocating pages on swap-in in case it was
> loaded into the wrong zone. This was implemented 2 years ago in:
> 
>     commit bde05d1ccd512696b09db9dd2e5f33ad19152605
>     Author: Hugh Dickins <hughd@google.com>
>     Date:   Tue May 29 15:06:38 2012 -0700
> 
>         shmem: replace page if mapping excludes its zone
> 
> If a driver requires pages to be in a specific zone, they _must_ set the
> correct GFP flags (like __GFP_DMA32) in mapping_gfp_mask(mapping). shmem
> will make sure that any page leaving the swap-cache is located in a
> compatible zone.
> 
> Signed-off-by: David Herrmann <dh.herrmann@gmail.com>

Imo the BUG_ON is nice since we want to make sure this never blows up. We
have a similar one in i915_gem.c since we do have platforms with 32bit
limits that support more than 4G or ram.

Imo better to update the comment and state that this is fixed now, but
better be paranoid.
-Daniel
> ---
>  drivers/gpu/drm/drm_gem.c | 19 -------------------
>  1 file changed, 19 deletions(-)
> 
> diff --git a/drivers/gpu/drm/drm_gem.c b/drivers/gpu/drm/drm_gem.c
> index 9909bef..a6146ab 100644
> --- a/drivers/gpu/drm/drm_gem.c
> +++ b/drivers/gpu/drm/drm_gem.c
> @@ -473,25 +473,6 @@ struct page **drm_gem_get_pages(struct drm_gem_object *obj, gfp_t gfpmask)
>  		if (IS_ERR(p))
>  			goto fail;
>  		pages[i] = p;
> -
> -		/* There is a hypothetical issue w/ drivers that require
> -		 * buffer memory in the low 4GB.. if the pages are un-
> -		 * pinned, and swapped out, they can end up swapped back
> -		 * in above 4GB.  If pages are already in memory, then
> -		 * shmem_read_mapping_page_gfp will ignore the gfpmask,
> -		 * even if the already in-memory page disobeys the mask.
> -		 *
> -		 * It is only a theoretical issue today, because none of
> -		 * the devices with this limitation can be populated with
> -		 * enough memory to trigger the issue.  But this BUG_ON()
> -		 * is here as a reminder in case the problem with
> -		 * shmem_read_mapping_page_gfp() isn't solved by the time
> -		 * it does become a real issue.
> -		 *
> -		 * See this thread: http://lkml.org/lkml/2011/7/11/238
> -		 */
> -		BUG_ON((gfpmask & __GFP_DMA32) &&
> -				(page_to_pfn(p) >= 0x00100000UL));
>  	}
>  
>  	return pages;
> -- 
> 1.9.3
> 

-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch

^ permalink raw reply	[flat|nested] 5+ messages in thread
* Re: [PATCH] drm/gem: remove obsolete BUG_ON
@ 2014-05-21 13:17   ` Daniel Vetter
  0 siblings, 0 replies; 5+ messages in thread
From: Daniel Vetter @ 2014-05-21 13:17 UTC (permalink / raw)
  To: David Herrmann; +Cc: daniel.vetter, hughd, linux-kernel, dri-devel

On Wed, May 21, 2014 at 02:16:26PM +0200, David Herrmann wrote:
> The shmem subsystem supports relocating pages on swap-in in case it was
> loaded into the wrong zone. This was implemented 2 years ago in:
> 
>     commit bde05d1ccd512696b09db9dd2e5f33ad19152605
>     Author: Hugh Dickins <hughd@google.com>
>     Date:   Tue May 29 15:06:38 2012 -0700
> 
>         shmem: replace page if mapping excludes its zone
> 
> If a driver requires pages to be in a specific zone, they _must_ set the
> correct GFP flags (like __GFP_DMA32) in mapping_gfp_mask(mapping). shmem
> will make sure that any page leaving the swap-cache is located in a
> compatible zone.
> 
> Signed-off-by: David Herrmann <dh.herrmann@gmail.com>

Imo the BUG_ON is nice since we want to make sure this never blows up. We
have a similar one in i915_gem.c since we do have platforms with 32bit
limits that support more than 4G or ram.

Imo better to update the comment and state that this is fixed now, but
better be paranoid.
-Daniel
> ---
>  drivers/gpu/drm/drm_gem.c | 19 -------------------
>  1 file changed, 19 deletions(-)
> 
> diff --git a/drivers/gpu/drm/drm_gem.c b/drivers/gpu/drm/drm_gem.c
> index 9909bef..a6146ab 100644
> --- a/drivers/gpu/drm/drm_gem.c
> +++ b/drivers/gpu/drm/drm_gem.c
> @@ -473,25 +473,6 @@ struct page **drm_gem_get_pages(struct drm_gem_object *obj, gfp_t gfpmask)
>  		if (IS_ERR(p))
>  			goto fail;
>  		pages[i] = p;
> -
> -		/* There is a hypothetical issue w/ drivers that require
> -		 * buffer memory in the low 4GB.. if the pages are un-
> -		 * pinned, and swapped out, they can end up swapped back
> -		 * in above 4GB.  If pages are already in memory, then
> -		 * shmem_read_mapping_page_gfp will ignore the gfpmask,
> -		 * even if the already in-memory page disobeys the mask.
> -		 *
> -		 * It is only a theoretical issue today, because none of
> -		 * the devices with this limitation can be populated with
> -		 * enough memory to trigger the issue.  But this BUG_ON()
> -		 * is here as a reminder in case the problem with
> -		 * shmem_read_mapping_page_gfp() isn't solved by the time
> -		 * it does become a real issue.
> -		 *
> -		 * See this thread: http://lkml.org/lkml/2011/7/11/238
> -		 */
> -		BUG_ON((gfpmask & __GFP_DMA32) &&
> -				(page_to_pfn(p) >= 0x00100000UL));
>  	}
>  
>  	return pages;
> -- 
> 1.9.3
> 

-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch

^ permalink raw reply	[flat|nested] 5+ messages in thread
* Re: [PATCH] drm/gem: remove obsolete BUG_ON
  2014-05-21 13:17   ` Daniel Vetter
  (?)
@ 2014-05-25 12:36   ` David Herrmann
  -1 siblings, 0 replies; 5+ messages in thread
From: David Herrmann @ 2014-05-25 12:36 UTC (permalink / raw)
  To: David Herrmann, dri-devel, Rob Clark, Dave Airlie, linux-kernel
  Cc: Daniel Vetter

Hi

On Wed, May 21, 2014 at 3:17 PM, Daniel Vetter <daniel@ffwll.ch> wrote:
> On Wed, May 21, 2014 at 02:16:26PM +0200, David Herrmann wrote:
>> The shmem subsystem supports relocating pages on swap-in in case it was
>> loaded into the wrong zone. This was implemented 2 years ago in:
>>
>>     commit bde05d1ccd512696b09db9dd2e5f33ad19152605
>>     Author: Hugh Dickins <hughd@google.com>
>>     Date:   Tue May 29 15:06:38 2012 -0700
>>
>>         shmem: replace page if mapping excludes its zone
>>
>> If a driver requires pages to be in a specific zone, they _must_ set the
>> correct GFP flags (like __GFP_DMA32) in mapping_gfp_mask(mapping). shmem
>> will make sure that any page leaving the swap-cache is located in a
>> compatible zone.
>>
>> Signed-off-by: David Herrmann <dh.herrmann@gmail.com>
>
> Imo the BUG_ON is nice since we want to make sure this never blows up. We
> have a similar one in i915_gem.c since we do have platforms with 32bit
> limits that support more than 4G or ram.
>
> Imo better to update the comment and state that this is fixed now, but
> better be paranoid.

Ok, I've kept the BUG_ON. I've also appended some more patches that
clean-up the shmem handling. v2 is on the list.

Thanks
David

^ permalink raw reply	[flat|nested] 5+ messages in thread
end of thread, other threads:[~2014-05-25 12:36 UTC | newest]

Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2014-05-21 12:16 [PATCH] drm/gem: remove obsolete BUG_ON David Herrmann
2014-05-21 12:16 ` David Herrmann
2014-05-21 13:17 ` Daniel Vetter
2014-05-21 13:17   ` Daniel Vetter
2014-05-25 12:36   ` David Herrmann

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.