* [PATCH v2 1/2] mm: Export nr_swap_pages @ 2015-12-04 15:58 Chris Wilson 2015-12-04 15:58 ` Chris Wilson ` (2 more replies) 0 siblings, 3 replies; 29+ messages in thread From: Chris Wilson @ 2015-12-04 15:58 UTC (permalink / raw) To: intel-gfx; +Cc: Chris Wilson, Goel, Akash, Johannes Weiner, linux-mm 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> ^ permalink raw reply related [flat|nested] 29+ messages in thread
* [PATCH v2 2/2] drm/i915: Disable shrinker for non-swapped backed objects 2015-12-04 15:58 [PATCH v2 1/2] mm: Export nr_swap_pages Chris Wilson @ 2015-12-04 15:58 ` Chris Wilson 2015-12-04 16:09 ` Johannes Weiner 2015-12-07 13:48 ` Michal Hocko 2 siblings, 0 replies; 29+ messages in thread From: Chris Wilson @ 2015-12-04 15:58 UTC (permalink / raw) To: intel-gfx; +Cc: Chris Wilson, linux-mm, Akash Goel, sourab.gupta 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> ^ permalink raw reply related [flat|nested] 29+ messages in thread
* [PATCH v2 2/2] drm/i915: Disable shrinker for non-swapped backed objects @ 2015-12-04 15:58 ` Chris Wilson 0 siblings, 0 replies; 29+ messages in thread From: Chris Wilson @ 2015-12-04 15:58 UTC (permalink / raw) To: intel-gfx; +Cc: linux-mm, Akash Goel, sourab.gupta 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 ^ permalink raw reply related [flat|nested] 29+ messages in thread
* Re: [PATCH v2 2/2] drm/i915: Disable shrinker for non-swapped backed objects 2015-12-04 15:58 ` Chris Wilson @ 2015-12-04 16:11 ` Johannes Weiner -1 siblings, 0 replies; 29+ messages in thread From: Johannes Weiner @ 2015-12-04 16:11 UTC (permalink / raw) To: Chris Wilson; +Cc: intel-gfx, linux-mm, Akash Goel, sourab.gupta 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> ^ permalink raw reply [flat|nested] 29+ messages in thread
* Re: [PATCH v2 2/2] drm/i915: Disable shrinker for non-swapped backed objects @ 2015-12-04 16:11 ` Johannes Weiner 0 siblings, 0 replies; 29+ messages in thread From: Johannes Weiner @ 2015-12-04 16:11 UTC (permalink / raw) To: Chris Wilson; +Cc: linux-mm, intel-gfx, sourab.gupta, Akash Goel 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 ^ permalink raw reply [flat|nested] 29+ messages in thread
* Re: [PATCH v2 2/2] drm/i915: Disable shrinker for non-swapped backed objects 2015-12-04 15:58 ` Chris Wilson (?) (?) @ 2015-12-10 9:34 ` Daniel Vetter -1 siblings, 0 replies; 29+ messages in thread From: Daniel Vetter @ 2015-12-10 9:34 UTC (permalink / raw) To: Chris Wilson; +Cc: intel-gfx, linux-mm, Akash Goel, sourab.gupta 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> ^ permalink raw reply [flat|nested] 29+ messages in thread
* Re: [PATCH v2 1/2] mm: Export nr_swap_pages 2015-12-04 15:58 [PATCH v2 1/2] mm: Export nr_swap_pages Chris Wilson @ 2015-12-04 16:09 ` Johannes Weiner 2015-12-04 16:09 ` Johannes Weiner 2015-12-07 13:48 ` Michal Hocko 2 siblings, 0 replies; 29+ messages in thread From: Johannes Weiner @ 2015-12-04 16:09 UTC (permalink / raw) To: Chris Wilson; +Cc: intel-gfx, Goel, Akash, linux-mm 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> ^ permalink raw reply [flat|nested] 29+ messages in thread
* Re: [PATCH v2 1/2] mm: Export nr_swap_pages @ 2015-12-04 16:09 ` Johannes Weiner 0 siblings, 0 replies; 29+ messages in thread From: Johannes Weiner @ 2015-12-04 16:09 UTC (permalink / raw) To: Chris Wilson; +Cc: linux-mm, intel-gfx, Goel, Akash 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 ^ permalink raw reply [flat|nested] 29+ messages in thread
* Re: [Intel-gfx] [PATCH v2 1/2] mm: Export nr_swap_pages 2015-12-04 16:09 ` Johannes Weiner (?) @ 2015-12-10 9:32 ` Daniel Vetter 2015-12-23 22:04 ` Johannes Weiner -1 siblings, 1 reply; 29+ messages in thread From: Daniel Vetter @ 2015-12-10 9:32 UTC (permalink / raw) To: Johannes Weiner; +Cc: Chris Wilson, linux-mm, intel-gfx, Goel, Akash 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> ^ permalink raw reply [flat|nested] 29+ messages in thread
* Re: [Intel-gfx] [PATCH v2 1/2] mm: Export nr_swap_pages 2015-12-10 9:32 ` [Intel-gfx] " Daniel Vetter @ 2015-12-23 22:04 ` Johannes Weiner 0 siblings, 0 replies; 29+ messages in thread From: Johannes Weiner @ 2015-12-23 22:04 UTC (permalink / raw) To: Daniel Vetter Cc: Chris Wilson, linux-mm, intel-gfx, Goel, Akash, Andrew Morton 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> ^ permalink raw reply [flat|nested] 29+ messages in thread
* Re: [PATCH v2 1/2] mm: Export nr_swap_pages @ 2015-12-23 22:04 ` Johannes Weiner 0 siblings, 0 replies; 29+ messages in thread From: Johannes Weiner @ 2015-12-23 22:04 UTC (permalink / raw) To: Daniel Vetter; +Cc: linux-mm, intel-gfx, Goel, Akash, Andrew Morton 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 ^ permalink raw reply [flat|nested] 29+ messages in thread
* Re: [Intel-gfx] [PATCH v2 1/2] mm: Export nr_swap_pages 2015-12-23 22:04 ` Johannes Weiner (?) @ 2015-12-23 22:26 ` Andrew Morton 2016-01-05 10:05 ` Daniel Vetter -1 siblings, 1 reply; 29+ messages in thread From: Andrew Morton @ 2015-12-23 22:26 UTC (permalink / raw) To: Johannes Weiner Cc: Daniel Vetter, Chris Wilson, linux-mm, intel-gfx, Goel, Akash 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> ^ permalink raw reply [flat|nested] 29+ messages in thread
* Re: [Intel-gfx] [PATCH v2 1/2] mm: Export nr_swap_pages 2015-12-23 22:26 ` [Intel-gfx] " Andrew Morton @ 2016-01-05 10:05 ` Daniel Vetter 0 siblings, 0 replies; 29+ messages in thread From: Daniel Vetter @ 2016-01-05 10:05 UTC (permalink / raw) To: Andrew Morton Cc: Johannes Weiner, Daniel Vetter, Chris Wilson, linux-mm, intel-gfx, Goel, Akash 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> ^ permalink raw reply [flat|nested] 29+ messages in thread
* Re: [PATCH v2 1/2] mm: Export nr_swap_pages 2015-12-04 15:58 [PATCH v2 1/2] mm: Export nr_swap_pages Chris Wilson 2015-12-04 15:58 ` Chris Wilson 2015-12-04 16:09 ` Johannes Weiner @ 2015-12-07 13:48 ` Michal Hocko 2015-12-07 16:48 ` Johannes Weiner 2 siblings, 1 reply; 29+ messages in thread From: Michal Hocko @ 2015-12-07 13:48 UTC (permalink / raw) To: Chris Wilson; +Cc: intel-gfx, Goel, Akash, Johannes Weiner, linux-mm 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> ^ permalink raw reply [flat|nested] 29+ messages in thread
* Re: [PATCH v2 1/2] mm: Export nr_swap_pages 2015-12-07 13:48 ` Michal Hocko @ 2015-12-07 16:48 ` Johannes Weiner 0 siblings, 0 replies; 29+ messages in thread From: Johannes Weiner @ 2015-12-07 16:48 UTC (permalink / raw) To: Michal Hocko; +Cc: Chris Wilson, intel-gfx, Goel, Akash, linux-mm 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> ^ permalink raw reply [flat|nested] 29+ messages in thread
* Re: [PATCH v2 1/2] mm: Export nr_swap_pages @ 2015-12-07 16:48 ` Johannes Weiner 0 siblings, 0 replies; 29+ messages in thread From: Johannes Weiner @ 2015-12-07 16:48 UTC (permalink / raw) To: Michal Hocko; +Cc: linux-mm, intel-gfx, Goel, Akash 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 ^ permalink raw reply [flat|nested] 29+ messages in thread
* Re: [PATCH v2 1/2] mm: Export nr_swap_pages 2015-12-07 16:48 ` Johannes Weiner (?) @ 2015-12-07 17:04 ` Michal Hocko 2015-12-07 18:02 ` Johannes Weiner -1 siblings, 1 reply; 29+ messages in thread From: Michal Hocko @ 2015-12-07 17:04 UTC (permalink / raw) To: Johannes Weiner; +Cc: Chris Wilson, intel-gfx, Goel, Akash, linux-mm 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> ^ permalink raw reply [flat|nested] 29+ messages in thread
* Re: [PATCH v2 1/2] mm: Export nr_swap_pages 2015-12-07 17:04 ` Michal Hocko @ 2015-12-07 18:02 ` Johannes Weiner 0 siblings, 0 replies; 29+ messages in thread From: Johannes Weiner @ 2015-12-07 18:02 UTC (permalink / raw) To: Michal Hocko; +Cc: Chris Wilson, intel-gfx, Goel, Akash, linux-mm 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> ^ permalink raw reply [flat|nested] 29+ messages in thread
* Re: [PATCH v2 1/2] mm: Export nr_swap_pages @ 2015-12-07 18:02 ` Johannes Weiner 0 siblings, 0 replies; 29+ messages in thread From: Johannes Weiner @ 2015-12-07 18:02 UTC (permalink / raw) To: Michal Hocko; +Cc: linux-mm, intel-gfx, Goel, Akash 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 ^ permalink raw reply [flat|nested] 29+ messages in thread
* Re: [Intel-gfx] [PATCH v2 1/2] mm: Export nr_swap_pages 2015-12-07 16:48 ` Johannes Weiner (?) (?) @ 2015-12-07 18:10 ` Dave Gordon 2015-12-07 19:13 ` Johannes Weiner -1 siblings, 1 reply; 29+ messages in thread From: Dave Gordon @ 2015-12-07 18:10 UTC (permalink / raw) To: Johannes Weiner, Michal Hocko; +Cc: linux-mm, intel-gfx, Goel, Akash 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> ^ permalink raw reply [flat|nested] 29+ messages in thread
* Re: [Intel-gfx] [PATCH v2 1/2] mm: Export nr_swap_pages 2015-12-07 18:10 ` [Intel-gfx] " Dave Gordon @ 2015-12-07 19:13 ` Johannes Weiner 2015-12-08 11:19 ` Dave Gordon 2015-12-08 11:22 ` Michal Hocko 0 siblings, 2 replies; 29+ messages in thread From: Johannes Weiner @ 2015-12-07 19:13 UTC (permalink / raw) To: Dave Gordon; +Cc: Michal Hocko, linux-mm, intel-gfx, Goel, Akash 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> ^ permalink raw reply [flat|nested] 29+ messages in thread
* Re: [Intel-gfx] [PATCH v2 1/2] mm: Export nr_swap_pages 2015-12-07 19:13 ` Johannes Weiner @ 2015-12-08 11:19 ` Dave Gordon 2015-12-08 11:22 ` Michal Hocko 1 sibling, 0 replies; 29+ messages in thread From: Dave Gordon @ 2015-12-08 11:19 UTC (permalink / raw) To: Johannes Weiner; +Cc: Michal Hocko, linux-mm, intel-gfx, Goel, Akash 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> ^ permalink raw reply [flat|nested] 29+ messages in thread
* Re: [PATCH v2 1/2] mm: Export nr_swap_pages @ 2015-12-08 11:19 ` Dave Gordon 0 siblings, 0 replies; 29+ messages in thread From: Dave Gordon @ 2015-12-08 11:19 UTC (permalink / raw) To: Johannes Weiner; +Cc: linux-mm, intel-gfx, Goel, Akash, Michal Hocko 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 ^ permalink raw reply [flat|nested] 29+ messages in thread
* Re: [Intel-gfx] [PATCH v2 1/2] mm: Export nr_swap_pages 2015-12-07 19:13 ` Johannes Weiner @ 2015-12-08 11:22 ` Michal Hocko 2015-12-08 11:22 ` Michal Hocko 1 sibling, 0 replies; 29+ messages in thread From: Michal Hocko @ 2015-12-08 11:22 UTC (permalink / raw) To: Johannes Weiner; +Cc: Dave Gordon, linux-mm, intel-gfx, Goel, Akash 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> ^ permalink raw reply [flat|nested] 29+ messages in thread
* Re: [PATCH v2 1/2] mm: Export nr_swap_pages @ 2015-12-08 11:22 ` Michal Hocko 0 siblings, 0 replies; 29+ messages in thread From: Michal Hocko @ 2015-12-08 11:22 UTC (permalink / raw) To: Johannes Weiner; +Cc: intel-gfx, Goel, Akash, linux-mm 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 ^ permalink raw reply [flat|nested] 29+ messages in thread
* [PATCH v3] mm: Export {__}get_nr_swap_pages() 2015-12-08 11:22 ` Michal Hocko @ 2015-12-17 18:15 ` Dave Gordon -1 siblings, 0 replies; 29+ messages in thread From: Dave Gordon @ 2015-12-17 18:15 UTC (permalink / raw) To: intel-gfx Cc: Dave Gordon, Chris Wilson, Goel, Akash, Michal Hocko, Johannes Weiner, linux-mm 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> ^ permalink raw reply related [flat|nested] 29+ messages in thread
* [PATCH v3] mm: Export {__}get_nr_swap_pages() @ 2015-12-17 18:15 ` Dave Gordon 0 siblings, 0 replies; 29+ messages in thread From: Dave Gordon @ 2015-12-17 18:15 UTC (permalink / raw) To: intel-gfx; +Cc: Michal Hocko, linux-mm, Goel, Akash, Johannes Weiner 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 ^ permalink raw reply related [flat|nested] 29+ messages in thread
* Re: [PATCH v3] mm: Export {__}get_nr_swap_pages() 2015-12-17 18:15 ` Dave Gordon @ 2015-12-17 19:45 ` Johannes Weiner -1 siblings, 0 replies; 29+ messages in thread From: Johannes Weiner @ 2015-12-17 19:45 UTC (permalink / raw) To: Dave Gordon; +Cc: intel-gfx, Chris Wilson, Goel, Akash, Michal Hocko, linux-mm 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> ^ permalink raw reply [flat|nested] 29+ messages in thread
* Re: [PATCH v3] mm: Export {__}get_nr_swap_pages() @ 2015-12-17 19:45 ` Johannes Weiner 0 siblings, 0 replies; 29+ messages in thread From: Johannes Weiner @ 2015-12-17 19:45 UTC (permalink / raw) To: Dave Gordon; +Cc: linux-mm, intel-gfx, Michal Hocko, Goel, Akash 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 ^ permalink raw reply [flat|nested] 29+ messages in thread
end of thread, other threads:[~2016-01-05 10:05 UTC | newest] Thread overview: 29+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2015-12-04 15:58 [PATCH v2 1/2] mm: Export nr_swap_pages Chris Wilson 2015-12-04 15:58 ` [PATCH v2 2/2] drm/i915: Disable shrinker for non-swapped backed objects Chris Wilson 2015-12-04 15:58 ` Chris Wilson 2015-12-04 16:11 ` Johannes Weiner 2015-12-04 16:11 ` Johannes Weiner 2015-12-10 9:34 ` Daniel Vetter 2015-12-04 16:09 ` [PATCH v2 1/2] mm: Export nr_swap_pages Johannes Weiner 2015-12-04 16:09 ` Johannes Weiner 2015-12-10 9:32 ` [Intel-gfx] " Daniel Vetter 2015-12-23 22:04 ` Johannes Weiner 2015-12-23 22:04 ` Johannes Weiner 2015-12-23 22:26 ` [Intel-gfx] " Andrew Morton 2016-01-05 10:05 ` Daniel Vetter 2015-12-07 13:48 ` Michal Hocko 2015-12-07 16:48 ` Johannes Weiner 2015-12-07 16:48 ` Johannes Weiner 2015-12-07 17:04 ` Michal Hocko 2015-12-07 18:02 ` Johannes Weiner 2015-12-07 18:02 ` Johannes Weiner 2015-12-07 18:10 ` [Intel-gfx] " Dave Gordon 2015-12-07 19:13 ` Johannes Weiner 2015-12-08 11:19 ` Dave Gordon 2015-12-08 11:19 ` Dave Gordon 2015-12-08 11:22 ` [Intel-gfx] " Michal Hocko 2015-12-08 11:22 ` Michal Hocko 2015-12-17 18:15 ` [PATCH v3] mm: Export {__}get_nr_swap_pages() Dave Gordon 2015-12-17 18:15 ` Dave Gordon 2015-12-17 19:45 ` Johannes Weiner 2015-12-17 19:45 ` Johannes Weiner
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.