[PATCH v2 1/2] mm: Export nr_swap_pages

[PATCH v2 1/2] mm: Export nr_swap_pages

[Chris Wilson]

Some modules, like i915.ko, use swappable objects and may try to swap
them out under memory pressure (via the shrinker). Before doing so, they
want to check using get_nr_swap_pages() to see if any swap space is
available as otherwise they will waste time purging the object from the
device without recovering any memory for the system. This requires the
nr_swap_pages counter to be exported to the modules.

Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
Cc: "Goel, Akash" <akash.goel@intel.com>
Cc: Johannes Weiner <hannes@cmpxchg.org>
Cc: linux-mm@kvack.org
---
 mm/swapfile.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/mm/swapfile.c b/mm/swapfile.c
index 58877312cf6b..2d259fdb2347 100644
--- a/mm/swapfile.c
+++ b/mm/swapfile.c
@@ -48,6 +48,12 @@ static sector_t map_swap_entry(swp_entry_t, struct block_device**);
 DEFINE_SPINLOCK(swap_lock);
 static unsigned int nr_swapfiles;
 atomic_long_t nr_swap_pages;
+/*
+ * Some modules use swappable objects and may try to swap them out under
+ * memory pressure (via the shrinker). Before doing so, they may wish to
+ * check to see if any swap space is available.
+ */
+EXPORT_SYMBOL_GPL(nr_swap_pages);
 /* protected with swap_lock. reading in vm_swap_full() doesn't need lock */
 long total_swap_pages;
 static int least_priority;
-- 
2.6.2

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

[PATCH v2 2/2] drm/i915: Disable shrinker for non-swapped backed objects

[Chris Wilson]

If the system has no available swap pages, we cannot make forward
progress in the shrinker by releasing active pages, only by releasing
purgeable pages which are immediately reaped. Take total_swap_pages into
account when counting up available objects to be shrunk and subsequently
shrinking them. By doing so, we avoid unbinding objects that cannot be
shrunk and so wasting CPU cycles flushing those objects from the GPU to
the system and then immediately back again (as they will more than
likely be reused shortly after).

Based on a patch by Akash Goel.

v2: frontswap registers extra swap pages available for the system, so it
is already include in the count of available swap pages.

v3: Use get_nr_swap_pages() to query the currently available amount of
swap space. This should also stop us from shrinking the GPU buffers if
we ever run out of swap space. Though at that point, we would expect the
oom-notifier to be running and failing miserably...

Reported-by: Akash Goel <akash.goel@intel.com>
Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
Cc: linux-mm@kvack.org
Cc: Akash Goel <akash.goel@intel.com>
Cc: sourab.gupta@intel.com
---
 drivers/gpu/drm/i915/i915_gem_shrinker.c | 60 +++++++++++++++++++++++---------
 1 file changed, 44 insertions(+), 16 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_gem_shrinker.c b/drivers/gpu/drm/i915/i915_gem_shrinker.c
index f7df54a8ee2b..16da9c1422cc 100644
--- a/drivers/gpu/drm/i915/i915_gem_shrinker.c
+++ b/drivers/gpu/drm/i915/i915_gem_shrinker.c
@@ -47,6 +47,46 @@ static bool mutex_is_locked_by(struct mutex *mutex, struct task_struct *task)
 #endif
 }
 
+static int num_vma_bound(struct drm_i915_gem_object *obj)
+{
+	struct i915_vma *vma;
+	int count = 0;
+
+	list_for_each_entry(vma, &obj->vma_list, vma_link) {
+		if (drm_mm_node_allocated(&vma->node))
+			count++;
+		if (vma->pin_count)
+			count++;
+	}
+
+	return count;
+}
+
+static bool swap_available(void)
+{
+	return get_nr_swap_pages() > 0;
+}
+
+static bool can_release_pages(struct drm_i915_gem_object *obj)
+{
+	/* Only report true if by unbinding the object and putting its pages
+	 * we can actually make forward progress towards freeing physical
+	 * pages.
+	 *
+	 * If the pages are pinned for any other reason than being bound
+	 * to the GPU, simply unbinding from the GPU is not going to succeed
+	 * in releasing our pin count on the pages themselves.
+	 */
+	if (obj->pages_pin_count != num_vma_bound(obj))
+		return false;
+
+	/* We can only return physical pages to the system if we can either
+	 * discard the contents (because the user has marked them as being
+	 * purgeable) or if we can move their contents out to swap.
+	 */
+	return swap_available() || obj->madv == I915_MADV_DONTNEED;
+}
+
 /**
  * i915_gem_shrink - Shrink buffer object caches
  * @dev_priv: i915 device
@@ -129,6 +169,9 @@ i915_gem_shrink(struct drm_i915_private *dev_priv,
 			if ((flags & I915_SHRINK_ACTIVE) == 0 && obj->active)
 				continue;
 
+			if (!can_release_pages(obj))
+				continue;
+
 			drm_gem_object_reference(&obj->base);
 
 			/* For the unbound phase, this should be a no-op! */
@@ -188,21 +231,6 @@ static bool i915_gem_shrinker_lock(struct drm_device *dev, bool *unlock)
 	return true;
 }
 
-static int num_vma_bound(struct drm_i915_gem_object *obj)
-{
-	struct i915_vma *vma;
-	int count = 0;
-
-	list_for_each_entry(vma, &obj->vma_list, vma_link) {
-		if (drm_mm_node_allocated(&vma->node))
-			count++;
-		if (vma->pin_count)
-			count++;
-	}
-
-	return count;
-}
-
 static unsigned long
 i915_gem_shrinker_count(struct shrinker *shrinker, struct shrink_control *sc)
 {
@@ -222,7 +250,7 @@ i915_gem_shrinker_count(struct shrinker *shrinker, struct shrink_control *sc)
 			count += obj->base.size >> PAGE_SHIFT;
 
 	list_for_each_entry(obj, &dev_priv->mm.bound_list, global_list) {
-		if (!obj->active && obj->pages_pin_count == num_vma_bound(obj))
+		if (!obj->active && can_release_pages(obj))
 			count += obj->base.size >> PAGE_SHIFT;
 	}
 
-- 
2.6.2

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

[PATCH v2 2/2] drm/i915: Disable shrinker for non-swapped backed objects

[Chris Wilson]

If the system has no available swap pages, we cannot make forward
progress in the shrinker by releasing active pages, only by releasing
purgeable pages which are immediately reaped. Take total_swap_pages into
account when counting up available objects to be shrunk and subsequently
shrinking them. By doing so, we avoid unbinding objects that cannot be
shrunk and so wasting CPU cycles flushing those objects from the GPU to
the system and then immediately back again (as they will more than
likely be reused shortly after).

Based on a patch by Akash Goel.

v2: frontswap registers extra swap pages available for the system, so it
is already include in the count of available swap pages.

v3: Use get_nr_swap_pages() to query the currently available amount of
swap space. This should also stop us from shrinking the GPU buffers if
we ever run out of swap space. Though at that point, we would expect the
oom-notifier to be running and failing miserably...

Reported-by: Akash Goel <akash.goel@intel.com>
Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
Cc: linux-mm@kvack.org
Cc: Akash Goel <akash.goel@intel.com>
Cc: sourab.gupta@intel.com
---
 drivers/gpu/drm/i915/i915_gem_shrinker.c | 60 +++++++++++++++++++++++---------
 1 file changed, 44 insertions(+), 16 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_gem_shrinker.c b/drivers/gpu/drm/i915/i915_gem_shrinker.c
index f7df54a8ee2b..16da9c1422cc 100644
--- a/drivers/gpu/drm/i915/i915_gem_shrinker.c
+++ b/drivers/gpu/drm/i915/i915_gem_shrinker.c
@@ -47,6 +47,46 @@ static bool mutex_is_locked_by(struct mutex *mutex, struct task_struct *task)
 #endif
 }
 
+static int num_vma_bound(struct drm_i915_gem_object *obj)
+{
+	struct i915_vma *vma;
+	int count = 0;
+
+	list_for_each_entry(vma, &obj->vma_list, vma_link) {
+		if (drm_mm_node_allocated(&vma->node))
+			count++;
+		if (vma->pin_count)
+			count++;
+	}
+
+	return count;
+}
+
+static bool swap_available(void)
+{
+	return get_nr_swap_pages() > 0;
+}
+
+static bool can_release_pages(struct drm_i915_gem_object *obj)
+{
+	/* Only report true if by unbinding the object and putting its pages
+	 * we can actually make forward progress towards freeing physical
+	 * pages.
+	 *
+	 * If the pages are pinned for any other reason than being bound
+	 * to the GPU, simply unbinding from the GPU is not going to succeed
+	 * in releasing our pin count on the pages themselves.
+	 */
+	if (obj->pages_pin_count != num_vma_bound(obj))
+		return false;
+
+	/* We can only return physical pages to the system if we can either
+	 * discard the contents (because the user has marked them as being
+	 * purgeable) or if we can move their contents out to swap.
+	 */
+	return swap_available() || obj->madv == I915_MADV_DONTNEED;
+}
+
 /**
  * i915_gem_shrink - Shrink buffer object caches
  * @dev_priv: i915 device
@@ -129,6 +169,9 @@ i915_gem_shrink(struct drm_i915_private *dev_priv,
 			if ((flags & I915_SHRINK_ACTIVE) == 0 && obj->active)
 				continue;
 
+			if (!can_release_pages(obj))
+				continue;
+
 			drm_gem_object_reference(&obj->base);
 
 			/* For the unbound phase, this should be a no-op! */
@@ -188,21 +231,6 @@ static bool i915_gem_shrinker_lock(struct drm_device *dev, bool *unlock)
 	return true;
 }
 
-static int num_vma_bound(struct drm_i915_gem_object *obj)
-{
-	struct i915_vma *vma;
-	int count = 0;
-
-	list_for_each_entry(vma, &obj->vma_list, vma_link) {
-		if (drm_mm_node_allocated(&vma->node))
-			count++;
-		if (vma->pin_count)
-			count++;
-	}
-
-	return count;
-}
-
 static unsigned long
 i915_gem_shrinker_count(struct shrinker *shrinker, struct shrink_control *sc)
 {
@@ -222,7 +250,7 @@ i915_gem_shrinker_count(struct shrinker *shrinker, struct shrink_control *sc)
 			count += obj->base.size >> PAGE_SHIFT;
 
 	list_for_each_entry(obj, &dev_priv->mm.bound_list, global_list) {
-		if (!obj->active && obj->pages_pin_count == num_vma_bound(obj))
+		if (!obj->active && can_release_pages(obj))
 			count += obj->base.size >> PAGE_SHIFT;
 	}
 
-- 
2.6.2

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH v2 1/2] mm: Export nr_swap_pages

[Johannes Weiner]

On Fri, Dec 04, 2015 at 03:58:53PM +0000, Chris Wilson wrote:
> Some modules, like i915.ko, use swappable objects and may try to swap
> them out under memory pressure (via the shrinker). Before doing so, they
> want to check using get_nr_swap_pages() to see if any swap space is
> available as otherwise they will waste time purging the object from the
> device without recovering any memory for the system. This requires the
> nr_swap_pages counter to be exported to the modules.
> 
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> Cc: "Goel, Akash" <akash.goel@intel.com>
> Cc: Johannes Weiner <hannes@cmpxchg.org>
> Cc: linux-mm@kvack.org

Acked-by: Johannes Weiner <hannes@cmpxchg.org>

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [PATCH v2 1/2] mm: Export nr_swap_pages

[Johannes Weiner]

On Fri, Dec 04, 2015 at 03:58:53PM +0000, Chris Wilson wrote:
> Some modules, like i915.ko, use swappable objects and may try to swap
> them out under memory pressure (via the shrinker). Before doing so, they
> want to check using get_nr_swap_pages() to see if any swap space is
> available as otherwise they will waste time purging the object from the
> device without recovering any memory for the system. This requires the
> nr_swap_pages counter to be exported to the modules.
> 
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> Cc: "Goel, Akash" <akash.goel@intel.com>
> Cc: Johannes Weiner <hannes@cmpxchg.org>
> Cc: linux-mm@kvack.org

Acked-by: Johannes Weiner <hannes@cmpxchg.org>
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH v2 2/2] drm/i915: Disable shrinker for non-swapped backed objects

[Johannes Weiner]

On Fri, Dec 04, 2015 at 03:58:54PM +0000, Chris Wilson wrote:
> If the system has no available swap pages, we cannot make forward
> progress in the shrinker by releasing active pages, only by releasing
> purgeable pages which are immediately reaped. Take total_swap_pages into
> account when counting up available objects to be shrunk and subsequently
> shrinking them. By doing so, we avoid unbinding objects that cannot be
> shrunk and so wasting CPU cycles flushing those objects from the GPU to
> the system and then immediately back again (as they will more than
> likely be reused shortly after).
> 
> Based on a patch by Akash Goel.
> 
> v2: frontswap registers extra swap pages available for the system, so it
> is already include in the count of available swap pages.
> 
> v3: Use get_nr_swap_pages() to query the currently available amount of
> swap space. This should also stop us from shrinking the GPU buffers if
> we ever run out of swap space. Though at that point, we would expect the
> oom-notifier to be running and failing miserably...
> 
> Reported-by: Akash Goel <akash.goel@intel.com>
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> Cc: linux-mm@kvack.org
> Cc: Akash Goel <akash.goel@intel.com>
> Cc: sourab.gupta@intel.com

Acked-by: Johannes Weiner <hannes@cmpxchg.org>

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [PATCH v2 2/2] drm/i915: Disable shrinker for non-swapped backed objects

[Johannes Weiner]

On Fri, Dec 04, 2015 at 03:58:54PM +0000, Chris Wilson wrote:
> If the system has no available swap pages, we cannot make forward
> progress in the shrinker by releasing active pages, only by releasing
> purgeable pages which are immediately reaped. Take total_swap_pages into
> account when counting up available objects to be shrunk and subsequently
> shrinking them. By doing so, we avoid unbinding objects that cannot be
> shrunk and so wasting CPU cycles flushing those objects from the GPU to
> the system and then immediately back again (as they will more than
> likely be reused shortly after).
> 
> Based on a patch by Akash Goel.
> 
> v2: frontswap registers extra swap pages available for the system, so it
> is already include in the count of available swap pages.
> 
> v3: Use get_nr_swap_pages() to query the currently available amount of
> swap space. This should also stop us from shrinking the GPU buffers if
> we ever run out of swap space. Though at that point, we would expect the
> oom-notifier to be running and failing miserably...
> 
> Reported-by: Akash Goel <akash.goel@intel.com>
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> Cc: linux-mm@kvack.org
> Cc: Akash Goel <akash.goel@intel.com>
> Cc: sourab.gupta@intel.com

Acked-by: Johannes Weiner <hannes@cmpxchg.org>
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH v2 1/2] mm: Export nr_swap_pages

[Michal Hocko]

On Fri 04-12-15 15:58:53, Chris Wilson wrote:
> Some modules, like i915.ko, use swappable objects and may try to swap
> them out under memory pressure (via the shrinker). Before doing so, they
> want to check using get_nr_swap_pages() to see if any swap space is
> available as otherwise they will waste time purging the object from the
> device without recovering any memory for the system. This requires the
> nr_swap_pages counter to be exported to the modules.

I guess it should be sufficient to change get_nr_swap_pages into a real
function and export it rather than giving the access to the counter
directly?
 
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> Cc: "Goel, Akash" <akash.goel@intel.com>
> Cc: Johannes Weiner <hannes@cmpxchg.org>
> Cc: linux-mm@kvack.org
> ---
>  mm/swapfile.c | 6 ++++++
>  1 file changed, 6 insertions(+)
> 
> diff --git a/mm/swapfile.c b/mm/swapfile.c
> index 58877312cf6b..2d259fdb2347 100644
> --- a/mm/swapfile.c
> +++ b/mm/swapfile.c
> @@ -48,6 +48,12 @@ static sector_t map_swap_entry(swp_entry_t, struct block_device**);
>  DEFINE_SPINLOCK(swap_lock);
>  static unsigned int nr_swapfiles;
>  atomic_long_t nr_swap_pages;
> +/*
> + * Some modules use swappable objects and may try to swap them out under
> + * memory pressure (via the shrinker). Before doing so, they may wish to
> + * check to see if any swap space is available.
> + */
> +EXPORT_SYMBOL_GPL(nr_swap_pages);
>  /* protected with swap_lock. reading in vm_swap_full() doesn't need lock */
>  long total_swap_pages;
>  static int least_priority;
> -- 
> 2.6.2
> 
> --
> To unsubscribe, send a message with 'unsubscribe linux-mm' in
> the body to majordomo@kvack.org.  For more info on Linux MM,
> see: http://www.linux-mm.org/ .
> Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

-- 
Michal Hocko
SUSE Labs

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [PATCH v2 1/2] mm: Export nr_swap_pages

[Johannes Weiner]

On Mon, Dec 07, 2015 at 02:48:12PM +0100, Michal Hocko wrote:
> On Fri 04-12-15 15:58:53, Chris Wilson wrote:
> > Some modules, like i915.ko, use swappable objects and may try to swap
> > them out under memory pressure (via the shrinker). Before doing so, they
> > want to check using get_nr_swap_pages() to see if any swap space is
> > available as otherwise they will waste time purging the object from the
> > device without recovering any memory for the system. This requires the
> > nr_swap_pages counter to be exported to the modules.
> 
> I guess it should be sufficient to change get_nr_swap_pages into a real
> function and export it rather than giving the access to the counter
> directly?

What do you mean by "sufficient"? That is actually more work.

It should be sufficient to just export the counter.

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [PATCH v2 1/2] mm: Export nr_swap_pages

[Johannes Weiner]

On Mon, Dec 07, 2015 at 02:48:12PM +0100, Michal Hocko wrote:
> On Fri 04-12-15 15:58:53, Chris Wilson wrote:
> > Some modules, like i915.ko, use swappable objects and may try to swap
> > them out under memory pressure (via the shrinker). Before doing so, they
> > want to check using get_nr_swap_pages() to see if any swap space is
> > available as otherwise they will waste time purging the object from the
> > device without recovering any memory for the system. This requires the
> > nr_swap_pages counter to be exported to the modules.
> 
> I guess it should be sufficient to change get_nr_swap_pages into a real
> function and export it rather than giving the access to the counter
> directly?

What do you mean by "sufficient"? That is actually more work.

It should be sufficient to just export the counter.
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH v2 1/2] mm: Export nr_swap_pages

[Michal Hocko]

On Mon 07-12-15 11:48:31, Johannes Weiner wrote:
> On Mon, Dec 07, 2015 at 02:48:12PM +0100, Michal Hocko wrote:
> > On Fri 04-12-15 15:58:53, Chris Wilson wrote:
> > > Some modules, like i915.ko, use swappable objects and may try to swap
> > > them out under memory pressure (via the shrinker). Before doing so, they
> > > want to check using get_nr_swap_pages() to see if any swap space is
> > > available as otherwise they will waste time purging the object from the
> > > device without recovering any memory for the system. This requires the
> > > nr_swap_pages counter to be exported to the modules.
> > 
> > I guess it should be sufficient to change get_nr_swap_pages into a real
> > function and export it rather than giving the access to the counter
> > directly?
> 
> What do you mean by "sufficient"?

Sufficient for the functionality which is required.

> That is actually more work.

Yes marginally more work.

> It should be sufficient to just export the counter.

Yes but the counter is an internal thing to the MM and the outside code
should have no business in manipulating it directly.

-- 
Michal Hocko
SUSE Labs

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [PATCH v2 1/2] mm: Export nr_swap_pages

[Johannes Weiner]

On Mon, Dec 07, 2015 at 06:04:35PM +0100, Michal Hocko wrote:
> Yes but the counter is an internal thing to the MM and the outside code
> should have no business in manipulating it directly.

The counter has been global scope forever. If you want to encapsulate
it, send a patch yourself and make your case in the changelog.  There
is no reason to make people with reasonable working code jump through
your personal preference hoops that have little to do with what their
patch is pursuing.

That being said, I'd NAK any patch that would turn a trivial accessor
like this into a full-blown function. Our interfaces encapsulate for
convenience, not out of distrust and bad faith.

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [PATCH v2 1/2] mm: Export nr_swap_pages

[Johannes Weiner]

On Mon, Dec 07, 2015 at 06:04:35PM +0100, Michal Hocko wrote:
> Yes but the counter is an internal thing to the MM and the outside code
> should have no business in manipulating it directly.

The counter has been global scope forever. If you want to encapsulate
it, send a patch yourself and make your case in the changelog.  There
is no reason to make people with reasonable working code jump through
your personal preference hoops that have little to do with what their
patch is pursuing.

That being said, I'd NAK any patch that would turn a trivial accessor
like this into a full-blown function. Our interfaces encapsulate for
convenience, not out of distrust and bad faith.
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH v2 1/2] mm: Export nr_swap_pages

[Dave Gordon]

On 07/12/15 16:48, Johannes Weiner wrote:
> On Mon, Dec 07, 2015 at 02:48:12PM +0100, Michal Hocko wrote:
>> On Fri 04-12-15 15:58:53, Chris Wilson wrote:
>>> Some modules, like i915.ko, use swappable objects and may try to swap
>>> them out under memory pressure (via the shrinker). Before doing so, they
>>> want to check using get_nr_swap_pages() to see if any swap space is
>>> available as otherwise they will waste time purging the object from the
>>> device without recovering any memory for the system. This requires the
>>> nr_swap_pages counter to be exported to the modules.
>>
>> I guess it should be sufficient to change get_nr_swap_pages into a real
>> function and export it rather than giving the access to the counter
>> directly?
>
> What do you mean by "sufficient"? That is actually more work.
>
> It should be sufficient to just export the counter.
> _______________________________________________

Exporting random uncontrolled variables from the kernel to loaded 
modules is not really considered best practice. It would be preferable 
to provide an accessor function - which is just what the declaration 
says we have; the implementation as a static inline (and/or macro) is 
what causes the problem here.

.Dave.

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [Intel-gfx] [PATCH v2 1/2] mm: Export nr_swap_pages

[Johannes Weiner]

On Mon, Dec 07, 2015 at 06:10:00PM +0000, Dave Gordon wrote:
> Exporting random uncontrolled variables from the kernel to loaded modules is
> not really considered best practice. It would be preferable to provide an
> accessor function - which is just what the declaration says we have; the
> implementation as a static inline (and/or macro) is what causes the problem
> here.

No, what causes the problem is thinking we can't trust in-kernel code.
If somebody screws up, we can fix it easily enough. Sure, we shouldn't
be laying traps and create easy-to-misuse interfaces, but that's not
what's happening here. There is no reason to add function overhead to
what should be a single 'mov' instruction.

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [Intel-gfx] [PATCH v2 1/2] mm: Export nr_swap_pages

[Dave Gordon]

On 07/12/15 19:13, Johannes Weiner wrote:
> On Mon, Dec 07, 2015 at 06:10:00PM +0000, Dave Gordon wrote:
>> Exporting random uncontrolled variables from the kernel to loaded modules is
>> not really considered best practice. It would be preferable to provide an
>> accessor function - which is just what the declaration says we have; the
>> implementation as a static inline (and/or macro) is what causes the problem
>> here.
>
> No, what causes the problem is thinking we can't trust in-kernel code.

We 'trust' kernel code not to be malicious, but not to be designed or 
implemented without mistakes. Keeping the impact of the mistakes as 
small and local as possible increases overall system reliability and 
makes debugging easier, which leads to the general principle of only 
exporting the minimum necessary interfaces. If no other module should 
write this data, then let's not export it as a read-write variable.

> If somebody screws up, we can fix it easily enough. Sure, we shouldn't
> be laying traps and create easy-to-misuse interfaces, but that's not
> what's happening here. There is no reason to add function overhead to
> what should be a single 'mov' instruction.

It could still be a macro or local inline within the mm code, but 
provide a read-only function-call interface for external use. That gives 
you maximum efficiency within the owning module, and makes it clear just 
what sort of access is allowed outside that code.

.Dave.

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [PATCH v2 1/2] mm: Export nr_swap_pages

[Dave Gordon]

On 07/12/15 19:13, Johannes Weiner wrote:
> On Mon, Dec 07, 2015 at 06:10:00PM +0000, Dave Gordon wrote:
>> Exporting random uncontrolled variables from the kernel to loaded modules is
>> not really considered best practice. It would be preferable to provide an
>> accessor function - which is just what the declaration says we have; the
>> implementation as a static inline (and/or macro) is what causes the problem
>> here.
>
> No, what causes the problem is thinking we can't trust in-kernel code.

We 'trust' kernel code not to be malicious, but not to be designed or 
implemented without mistakes. Keeping the impact of the mistakes as 
small and local as possible increases overall system reliability and 
makes debugging easier, which leads to the general principle of only 
exporting the minimum necessary interfaces. If no other module should 
write this data, then let's not export it as a read-write variable.

> If somebody screws up, we can fix it easily enough. Sure, we shouldn't
> be laying traps and create easy-to-misuse interfaces, but that's not
> what's happening here. There is no reason to add function overhead to
> what should be a single 'mov' instruction.

It could still be a macro or local inline within the mm code, but 
provide a read-only function-call interface for external use. That gives 
you maximum efficiency within the owning module, and makes it clear just 
what sort of access is allowed outside that code.

.Dave.
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH v2 1/2] mm: Export nr_swap_pages

[Michal Hocko]

On Mon 07-12-15 14:13:46, Johannes Weiner wrote:
> On Mon, Dec 07, 2015 at 06:10:00PM +0000, Dave Gordon wrote:
> > Exporting random uncontrolled variables from the kernel to loaded modules is
> > not really considered best practice. It would be preferable to provide an
> > accessor function - which is just what the declaration says we have; the
> > implementation as a static inline (and/or macro) is what causes the problem
> > here.
> 
> No, what causes the problem is thinking we can't trust in-kernel code.

This is not about the trust. It is about a clear API and separation.

> If somebody screws up, we can fix it easily enough. Sure, we shouldn't
> be laying traps and create easy-to-misuse interfaces, but that's not
> what's happening here. There is no reason to add function overhead to
> what should be a single 'mov' instruction.

The mere fact that the current implementation is a simple atomic_long_read
is a detail and not important for the API. The function is not used
in any hot path where a single function call overhead would be a
performance killer. Exporting implementation details to random users
tends to add maintenance burden in future.

I think it is natural to export symbols which are consumed by modules
and that will be get_nr_swap_pages(). I do not even understand the
resistance against that. Anyway I am not going to argue about it more.
I have raised my review comment and leave the decision to Chris/Andrew.
-- 
Michal Hocko
SUSE Labs

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [PATCH v2 1/2] mm: Export nr_swap_pages

[Michal Hocko]

On Mon 07-12-15 14:13:46, Johannes Weiner wrote:
> On Mon, Dec 07, 2015 at 06:10:00PM +0000, Dave Gordon wrote:
> > Exporting random uncontrolled variables from the kernel to loaded modules is
> > not really considered best practice. It would be preferable to provide an
> > accessor function - which is just what the declaration says we have; the
> > implementation as a static inline (and/or macro) is what causes the problem
> > here.
> 
> No, what causes the problem is thinking we can't trust in-kernel code.

This is not about the trust. It is about a clear API and separation.

> If somebody screws up, we can fix it easily enough. Sure, we shouldn't
> be laying traps and create easy-to-misuse interfaces, but that's not
> what's happening here. There is no reason to add function overhead to
> what should be a single 'mov' instruction.

The mere fact that the current implementation is a simple atomic_long_read
is a detail and not important for the API. The function is not used
in any hot path where a single function call overhead would be a
performance killer. Exporting implementation details to random users
tends to add maintenance burden in future.

I think it is natural to export symbols which are consumed by modules
and that will be get_nr_swap_pages(). I do not even understand the
resistance against that. Anyway I am not going to argue about it more.
I have raised my review comment and leave the decision to Chris/Andrew.
-- 
Michal Hocko
SUSE Labs
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH v2 1/2] mm: Export nr_swap_pages

[Daniel Vetter]

On Fri, Dec 04, 2015 at 11:09:52AM -0500, Johannes Weiner wrote:
> On Fri, Dec 04, 2015 at 03:58:53PM +0000, Chris Wilson wrote:
> > Some modules, like i915.ko, use swappable objects and may try to swap
> > them out under memory pressure (via the shrinker). Before doing so, they
> > want to check using get_nr_swap_pages() to see if any swap space is
> > available as otherwise they will waste time purging the object from the
> > device without recovering any memory for the system. This requires the
> > nr_swap_pages counter to be exported to the modules.
> > 
> > Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> > Cc: "Goel, Akash" <akash.goel@intel.com>
> > Cc: Johannes Weiner <hannes@cmpxchg.org>
> > Cc: linux-mm@kvack.org
> 
> Acked-by: Johannes Weiner <hannes@cmpxchg.org>

Ack for merging this through drm-intel trees for 4.5? I'm a bit unclear
who's ack I need for that for linux-mm topics ...

Thanks, Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [PATCH v2 2/2] drm/i915: Disable shrinker for non-swapped backed objects

[Daniel Vetter]

On Fri, Dec 04, 2015 at 03:58:54PM +0000, Chris Wilson wrote:
> If the system has no available swap pages, we cannot make forward
> progress in the shrinker by releasing active pages, only by releasing
> purgeable pages which are immediately reaped. Take total_swap_pages into
> account when counting up available objects to be shrunk and subsequently
> shrinking them. By doing so, we avoid unbinding objects that cannot be
> shrunk and so wasting CPU cycles flushing those objects from the GPU to
> the system and then immediately back again (as they will more than
> likely be reused shortly after).
> 
> Based on a patch by Akash Goel.
> 
> v2: frontswap registers extra swap pages available for the system, so it
> is already include in the count of available swap pages.
> 
> v3: Use get_nr_swap_pages() to query the currently available amount of
> swap space. This should also stop us from shrinking the GPU buffers if
> we ever run out of swap space. Though at that point, we would expect the
> oom-notifier to be running and failing miserably...
> 
> Reported-by: Akash Goel <akash.goel@intel.com>
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> Cc: linux-mm@kvack.org
> Cc: Akash Goel <akash.goel@intel.com>
> Cc: sourab.gupta@intel.com

Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>

I did wonder whether we shouldn't check this at the top, but this looks
nicer. And if you've run out of memory wasting a bit of cpu won't be a
concern really.
-Daniel

> ---
>  drivers/gpu/drm/i915/i915_gem_shrinker.c | 60 +++++++++++++++++++++++---------
>  1 file changed, 44 insertions(+), 16 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_gem_shrinker.c b/drivers/gpu/drm/i915/i915_gem_shrinker.c
> index f7df54a8ee2b..16da9c1422cc 100644
> --- a/drivers/gpu/drm/i915/i915_gem_shrinker.c
> +++ b/drivers/gpu/drm/i915/i915_gem_shrinker.c
> @@ -47,6 +47,46 @@ static bool mutex_is_locked_by(struct mutex *mutex, struct task_struct *task)
>  #endif
>  }
>  
> +static int num_vma_bound(struct drm_i915_gem_object *obj)
> +{
> +	struct i915_vma *vma;
> +	int count = 0;
> +
> +	list_for_each_entry(vma, &obj->vma_list, vma_link) {
> +		if (drm_mm_node_allocated(&vma->node))
> +			count++;
> +		if (vma->pin_count)
> +			count++;
> +	}
> +
> +	return count;
> +}
> +
> +static bool swap_available(void)
> +{
> +	return get_nr_swap_pages() > 0;
> +}
> +
> +static bool can_release_pages(struct drm_i915_gem_object *obj)
> +{
> +	/* Only report true if by unbinding the object and putting its pages
> +	 * we can actually make forward progress towards freeing physical
> +	 * pages.
> +	 *
> +	 * If the pages are pinned for any other reason than being bound
> +	 * to the GPU, simply unbinding from the GPU is not going to succeed
> +	 * in releasing our pin count on the pages themselves.
> +	 */
> +	if (obj->pages_pin_count != num_vma_bound(obj))
> +		return false;
> +
> +	/* We can only return physical pages to the system if we can either
> +	 * discard the contents (because the user has marked them as being
> +	 * purgeable) or if we can move their contents out to swap.
> +	 */
> +	return swap_available() || obj->madv == I915_MADV_DONTNEED;
> +}
> +
>  /**
>   * i915_gem_shrink - Shrink buffer object caches
>   * @dev_priv: i915 device
> @@ -129,6 +169,9 @@ i915_gem_shrink(struct drm_i915_private *dev_priv,
>  			if ((flags & I915_SHRINK_ACTIVE) == 0 && obj->active)
>  				continue;
>  
> +			if (!can_release_pages(obj))
> +				continue;
> +
>  			drm_gem_object_reference(&obj->base);
>  
>  			/* For the unbound phase, this should be a no-op! */
> @@ -188,21 +231,6 @@ static bool i915_gem_shrinker_lock(struct drm_device *dev, bool *unlock)
>  	return true;
>  }
>  
> -static int num_vma_bound(struct drm_i915_gem_object *obj)
> -{
> -	struct i915_vma *vma;
> -	int count = 0;
> -
> -	list_for_each_entry(vma, &obj->vma_list, vma_link) {
> -		if (drm_mm_node_allocated(&vma->node))
> -			count++;
> -		if (vma->pin_count)
> -			count++;
> -	}
> -
> -	return count;
> -}
> -
>  static unsigned long
>  i915_gem_shrinker_count(struct shrinker *shrinker, struct shrink_control *sc)
>  {
> @@ -222,7 +250,7 @@ i915_gem_shrinker_count(struct shrinker *shrinker, struct shrink_control *sc)
>  			count += obj->base.size >> PAGE_SHIFT;
>  
>  	list_for_each_entry(obj, &dev_priv->mm.bound_list, global_list) {
> -		if (!obj->active && obj->pages_pin_count == num_vma_bound(obj))
> +		if (!obj->active && can_release_pages(obj))
>  			count += obj->base.size >> PAGE_SHIFT;
>  	}
>  
> -- 
> 2.6.2
> 
> --
> To unsubscribe, send a message with 'unsubscribe linux-mm' in
> the body to majordomo@kvack.org.  For more info on Linux MM,
> see: http://www.linux-mm.org/ .
> Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

[PATCH v3] mm: Export {__}get_nr_swap_pages()

[Dave Gordon]

Some modules, like i915.ko, use swappable objects and may try to swap
them out under memory pressure (via the shrinker). Before doing so,
they want to check using get_nr_swap_pages() to see if any swap space
is available as otherwise they will waste time purging the object from
the device without recovering any memory for the system. This requires
the kernel function get_nr_swap_pages() to be exported to the modules.

The current implementation of this function is as a static inline
inside the header file swap.h>; this doesn't work when compiled in
a module, as the necessary global data is not visible. The original
proposed solution was to export the kernel global variable to modules,
but this was considered poor practice as it exposed more than necessary,
and in an uncontrolled fashion. Another idea was to turn it into a real
(non-inline) function; however this was considered to unnecessarily add
overhead for users within the base kernel.

Therefore, to avoid both objections, this patch leaves the base kernel
implementation unchanged, but adds a separate (read-only) functional
interface for callers in loadable kernel modules (LKMs). Which definition
is visible to code depends on the compile-time symbol MODULE, defined
by the Kbuild system when building an LKM.

Signed-off-by: Dave Gordon <david.s.gordon@intel.com>
Cc: Chris Wilson <chris@chris-wilson.co.uk>
Cc: "Goel, Akash" <akash.goel@intel.com>
Cc: Michal Hocko <mhocko@kernel.org>
Cc: Johannes Weiner <hannes@cmpxchg.org>
Cc: linux-mm@kvack.org
Cc: intel-gfx@lists.freedesktop.org
---
 include/linux/swap.h | 12 ++++++++++++
 mm/swapfile.c        |  7 +++++++
 2 files changed, 19 insertions(+)

diff --git a/include/linux/swap.h b/include/linux/swap.h
index 7ba7dcc..7dac1fe 100644
--- a/include/linux/swap.h
+++ b/include/linux/swap.h
@@ -413,6 +413,10 @@ extern struct page *swapin_readahead(swp_entry_t, gfp_t,
 			struct vm_area_struct *vma, unsigned long addr);
 
 /* linux/mm/swapfile.c */
+
+#ifndef	MODULE
+
+/* Inside the base kernel, code can see these variables */
 extern atomic_long_t nr_swap_pages;
 extern long total_swap_pages;
 
@@ -427,6 +431,14 @@ static inline long get_nr_swap_pages(void)
 	return atomic_long_read(&nr_swap_pages);
 }
 
+#else	/* MODULE */
+
+/* Only this read-only interface is available to modules */
+extern long __get_nr_swap_pages(void);
+#define	get_nr_swap_pages	__get_nr_swap_pages
+
+#endif	/* MODULE */
+
 extern void si_swapinfo(struct sysinfo *);
 extern swp_entry_t get_swap_page(void);
 extern swp_entry_t get_swap_page_of_type(int);
diff --git a/mm/swapfile.c b/mm/swapfile.c
index 5887731..9309d6e 100644
--- a/mm/swapfile.c
+++ b/mm/swapfile.c
@@ -2754,6 +2754,13 @@ pgoff_t __page_file_index(struct page *page)
 }
 EXPORT_SYMBOL_GPL(__page_file_index);
 
+/* Trivial accessor exported to kernel modules */
+long __get_nr_swap_pages(void)
+{
+	return get_nr_swap_pages();
+}
+EXPORT_SYMBOL_GPL(__get_nr_swap_pages);
+
 /*
  * add_swap_count_continuation - called when a swap count is duplicated
  * beyond SWAP_MAP_MAX, it allocates a new page and links that to the entry's
-- 
1.9.1

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

[PATCH v3] mm: Export {__}get_nr_swap_pages()

[Dave Gordon]

Some modules, like i915.ko, use swappable objects and may try to swap
them out under memory pressure (via the shrinker). Before doing so,
they want to check using get_nr_swap_pages() to see if any swap space
is available as otherwise they will waste time purging the object from
the device without recovering any memory for the system. This requires
the kernel function get_nr_swap_pages() to be exported to the modules.

The current implementation of this function is as a static inline
inside the header file swap.h>; this doesn't work when compiled in
a module, as the necessary global data is not visible. The original
proposed solution was to export the kernel global variable to modules,
but this was considered poor practice as it exposed more than necessary,
and in an uncontrolled fashion. Another idea was to turn it into a real
(non-inline) function; however this was considered to unnecessarily add
overhead for users within the base kernel.

Therefore, to avoid both objections, this patch leaves the base kernel
implementation unchanged, but adds a separate (read-only) functional
interface for callers in loadable kernel modules (LKMs). Which definition
is visible to code depends on the compile-time symbol MODULE, defined
by the Kbuild system when building an LKM.

Signed-off-by: Dave Gordon <david.s.gordon@intel.com>
Cc: Chris Wilson <chris@chris-wilson.co.uk>
Cc: "Goel, Akash" <akash.goel@intel.com>
Cc: Michal Hocko <mhocko@kernel.org>
Cc: Johannes Weiner <hannes@cmpxchg.org>
Cc: linux-mm@kvack.org
Cc: intel-gfx@lists.freedesktop.org
---
 include/linux/swap.h | 12 ++++++++++++
 mm/swapfile.c        |  7 +++++++
 2 files changed, 19 insertions(+)

diff --git a/include/linux/swap.h b/include/linux/swap.h
index 7ba7dcc..7dac1fe 100644
--- a/include/linux/swap.h
+++ b/include/linux/swap.h
@@ -413,6 +413,10 @@ extern struct page *swapin_readahead(swp_entry_t, gfp_t,
 			struct vm_area_struct *vma, unsigned long addr);
 
 /* linux/mm/swapfile.c */
+
+#ifndef	MODULE
+
+/* Inside the base kernel, code can see these variables */
 extern atomic_long_t nr_swap_pages;
 extern long total_swap_pages;
 
@@ -427,6 +431,14 @@ static inline long get_nr_swap_pages(void)
 	return atomic_long_read(&nr_swap_pages);
 }
 
+#else	/* MODULE */
+
+/* Only this read-only interface is available to modules */
+extern long __get_nr_swap_pages(void);
+#define	get_nr_swap_pages	__get_nr_swap_pages
+
+#endif	/* MODULE */
+
 extern void si_swapinfo(struct sysinfo *);
 extern swp_entry_t get_swap_page(void);
 extern swp_entry_t get_swap_page_of_type(int);
diff --git a/mm/swapfile.c b/mm/swapfile.c
index 5887731..9309d6e 100644
--- a/mm/swapfile.c
+++ b/mm/swapfile.c
@@ -2754,6 +2754,13 @@ pgoff_t __page_file_index(struct page *page)
 }
 EXPORT_SYMBOL_GPL(__page_file_index);
 
+/* Trivial accessor exported to kernel modules */
+long __get_nr_swap_pages(void)
+{
+	return get_nr_swap_pages();
+}
+EXPORT_SYMBOL_GPL(__get_nr_swap_pages);
+
 /*
  * add_swap_count_continuation - called when a swap count is duplicated
  * beyond SWAP_MAP_MAX, it allocates a new page and links that to the entry's
-- 
1.9.1

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH v3] mm: Export {__}get_nr_swap_pages()

[Johannes Weiner]

On Thu, Dec 17, 2015 at 06:15:44PM +0000, Dave Gordon wrote:
> Some modules, like i915.ko, use swappable objects and may try to swap
> them out under memory pressure (via the shrinker). Before doing so,
> they want to check using get_nr_swap_pages() to see if any swap space
> is available as otherwise they will waste time purging the object from
> the device without recovering any memory for the system. This requires
> the kernel function get_nr_swap_pages() to be exported to the modules.
> 
> The current implementation of this function is as a static inline
> inside the header file swap.h>; this doesn't work when compiled in
> a module, as the necessary global data is not visible. The original
> proposed solution was to export the kernel global variable to modules,
> but this was considered poor practice as it exposed more than necessary,
> and in an uncontrolled fashion. Another idea was to turn it into a real
> (non-inline) function; however this was considered to unnecessarily add
> overhead for users within the base kernel.
> 
> Therefore, to avoid both objections, this patch leaves the base kernel
> implementation unchanged, but adds a separate (read-only) functional
> interface for callers in loadable kernel modules (LKMs). Which definition
> is visible to code depends on the compile-time symbol MODULE, defined
> by the Kbuild system when building an LKM.

I'm sorry, but this is beyond silly. 19 lines of code to fix a
non-existent problem? This lacks any sort of proportionality.

NAK

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [PATCH v3] mm: Export {__}get_nr_swap_pages()

[Johannes Weiner]

On Thu, Dec 17, 2015 at 06:15:44PM +0000, Dave Gordon wrote:
> Some modules, like i915.ko, use swappable objects and may try to swap
> them out under memory pressure (via the shrinker). Before doing so,
> they want to check using get_nr_swap_pages() to see if any swap space
> is available as otherwise they will waste time purging the object from
> the device without recovering any memory for the system. This requires
> the kernel function get_nr_swap_pages() to be exported to the modules.
> 
> The current implementation of this function is as a static inline
> inside the header file swap.h>; this doesn't work when compiled in
> a module, as the necessary global data is not visible. The original
> proposed solution was to export the kernel global variable to modules,
> but this was considered poor practice as it exposed more than necessary,
> and in an uncontrolled fashion. Another idea was to turn it into a real
> (non-inline) function; however this was considered to unnecessarily add
> overhead for users within the base kernel.
> 
> Therefore, to avoid both objections, this patch leaves the base kernel
> implementation unchanged, but adds a separate (read-only) functional
> interface for callers in loadable kernel modules (LKMs). Which definition
> is visible to code depends on the compile-time symbol MODULE, defined
> by the Kbuild system when building an LKM.

I'm sorry, but this is beyond silly. 19 lines of code to fix a
non-existent problem? This lacks any sort of proportionality.

NAK
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH v2 1/2] mm: Export nr_swap_pages

[Johannes Weiner]

On Thu, Dec 10, 2015 at 10:32:42AM +0100, Daniel Vetter wrote:
> On Fri, Dec 04, 2015 at 11:09:52AM -0500, Johannes Weiner wrote:
> > On Fri, Dec 04, 2015 at 03:58:53PM +0000, Chris Wilson wrote:
> > > Some modules, like i915.ko, use swappable objects and may try to swap
> > > them out under memory pressure (via the shrinker). Before doing so, they
> > > want to check using get_nr_swap_pages() to see if any swap space is
> > > available as otherwise they will waste time purging the object from the
> > > device without recovering any memory for the system. This requires the
> > > nr_swap_pages counter to be exported to the modules.
> > > 
> > > Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> > > Cc: "Goel, Akash" <akash.goel@intel.com>
> > > Cc: Johannes Weiner <hannes@cmpxchg.org>
> > > Cc: linux-mm@kvack.org
> > 
> > Acked-by: Johannes Weiner <hannes@cmpxchg.org>
> 
> Ack for merging this through drm-intel trees for 4.5? I'm a bit unclear
> who's ack I need for that for linux-mm topics ...

Andrew would be the -mm maintainer. CC'd.

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [PATCH v2 1/2] mm: Export nr_swap_pages

[Johannes Weiner]

On Thu, Dec 10, 2015 at 10:32:42AM +0100, Daniel Vetter wrote:
> On Fri, Dec 04, 2015 at 11:09:52AM -0500, Johannes Weiner wrote:
> > On Fri, Dec 04, 2015 at 03:58:53PM +0000, Chris Wilson wrote:
> > > Some modules, like i915.ko, use swappable objects and may try to swap
> > > them out under memory pressure (via the shrinker). Before doing so, they
> > > want to check using get_nr_swap_pages() to see if any swap space is
> > > available as otherwise they will waste time purging the object from the
> > > device without recovering any memory for the system. This requires the
> > > nr_swap_pages counter to be exported to the modules.
> > > 
> > > Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> > > Cc: "Goel, Akash" <akash.goel@intel.com>
> > > Cc: Johannes Weiner <hannes@cmpxchg.org>
> > > Cc: linux-mm@kvack.org
> > 
> > Acked-by: Johannes Weiner <hannes@cmpxchg.org>
> 
> Ack for merging this through drm-intel trees for 4.5? I'm a bit unclear
> who's ack I need for that for linux-mm topics ...

Andrew would be the -mm maintainer. CC'd.
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH v2 1/2] mm: Export nr_swap_pages

[Andrew Morton]

On Wed, 23 Dec 2015 17:04:27 -0500 Johannes Weiner <hannes@cmpxchg.org> wrote:

> On Thu, Dec 10, 2015 at 10:32:42AM +0100, Daniel Vetter wrote:
> > On Fri, Dec 04, 2015 at 11:09:52AM -0500, Johannes Weiner wrote:
> > > On Fri, Dec 04, 2015 at 03:58:53PM +0000, Chris Wilson wrote:
> > > > Some modules, like i915.ko, use swappable objects and may try to swap
> > > > them out under memory pressure (via the shrinker). Before doing so, they
> > > > want to check using get_nr_swap_pages() to see if any swap space is
> > > > available as otherwise they will waste time purging the object from the
> > > > device without recovering any memory for the system. This requires the
> > > > nr_swap_pages counter to be exported to the modules.
> > > > 
> > > > Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> > > > Cc: "Goel, Akash" <akash.goel@intel.com>
> > > > Cc: Johannes Weiner <hannes@cmpxchg.org>
> > > > Cc: linux-mm@kvack.org
> > > 
> > > Acked-by: Johannes Weiner <hannes@cmpxchg.org>
> > 
> > Ack for merging this through drm-intel trees for 4.5? I'm a bit unclear
> > who's ack I need for that for linux-mm topics ...
> 
> Andrew would be the -mm maintainer. CC'd.

yup, please go ahead and merge that via the DRM tree.

nr_swap_pages is a crappy name.  That means "number of pages in swap",
which isn't the case.  Something like "swap_pages_available" would be
better.

And your swap_available() isn't good either ;) It can mean "is any swap
online" or "what is the amount of free swap space (in unknown units!)".
I'd call it "swap_is_full()" and put a ! in the caller.  But it's
hardly important for a wee little static helper.

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [Intel-gfx] [PATCH v2 1/2] mm: Export nr_swap_pages

[Daniel Vetter]

On Wed, Dec 23, 2015 at 02:26:11PM -0800, Andrew Morton wrote:
> On Wed, 23 Dec 2015 17:04:27 -0500 Johannes Weiner <hannes@cmpxchg.org> wrote:
> 
> > On Thu, Dec 10, 2015 at 10:32:42AM +0100, Daniel Vetter wrote:
> > > On Fri, Dec 04, 2015 at 11:09:52AM -0500, Johannes Weiner wrote:
> > > > On Fri, Dec 04, 2015 at 03:58:53PM +0000, Chris Wilson wrote:
> > > > > Some modules, like i915.ko, use swappable objects and may try to swap
> > > > > them out under memory pressure (via the shrinker). Before doing so, they
> > > > > want to check using get_nr_swap_pages() to see if any swap space is
> > > > > available as otherwise they will waste time purging the object from the
> > > > > device without recovering any memory for the system. This requires the
> > > > > nr_swap_pages counter to be exported to the modules.
> > > > > 
> > > > > Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> > > > > Cc: "Goel, Akash" <akash.goel@intel.com>
> > > > > Cc: Johannes Weiner <hannes@cmpxchg.org>
> > > > > Cc: linux-mm@kvack.org
> > > > 
> > > > Acked-by: Johannes Weiner <hannes@cmpxchg.org>
> > > 
> > > Ack for merging this through drm-intel trees for 4.5? I'm a bit unclear
> > > who's ack I need for that for linux-mm topics ...
> > 
> > Andrew would be the -mm maintainer. CC'd.
> 
> yup, please go ahead and merge that via the DRM tree.
> 
> nr_swap_pages is a crappy name.  That means "number of pages in swap",
> which isn't the case.  Something like "swap_pages_available" would be
> better.
> 
> And your swap_available() isn't good either ;) It can mean "is any swap
> online" or "what is the amount of free swap space (in unknown units!)".
> I'd call it "swap_is_full()" and put a ! in the caller.  But it's
> hardly important for a wee little static helper.

Yeah it's not super-pretty, but then the entire core mm/shrinker
abstraction is more than just a bit leaky (at least i915 has plenty of
code to make sure we don't bite our own tail). In case of doubt I prefer
the simplest export and avoid the mistake of fake abstraction in the form
of an inline helper with a pretty name.

Merged to drm-intel.git as-is, but missed the 4.5 train so will only land
in 4.6.

Thanks, Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>