mm/vmalloc: remove preempt_disable/enable when do preloading
Subsystems (2)
Uladzislau Rezki (Sony)
maintainerPATCH v2 1/1 mm/vmalloc: remove preempt_disable/enable when do preloading
Get rid of preempt_disable() and preempt_enable() when the preload is done for splitting purpose. The reason is that calling spin_lock() with disabled preemtion is forbidden in CONFIG_PREEMPT_RT kernel.
Therefore, we do not guarantee that a CPU is preloaded, instead we minimize the case when it is not with this change.
For example i run the special test case that follows the preload pattern and path. 20 "unbind" threads run it and each does 1000000 allocations. Only 3.5 times among 1000000 a CPU was not preloaded. So it can happen but the number is negligible.
V1 -> V2:
- move __this_cpu_cmpxchg check when spin_lock is taken, as proposed by Andrew Morton
- add more explanation in regard of preloading
- adjust and move some comments
--- mm/vmalloc.c | 50 +++++++++++++++++++++++++++++++++----------------- 1 file changed, 33 insertions(+), 17 deletions(-)
mm/vmalloc.c +33 -17
@@ -969,6 +969,19 @@ adjust_va_to_fit_type(struct vmap_area *va,* There are a few exceptions though, as an example it is * a first allocation (early boot up) when we have "one" * big free space that has to be split. +* +* Also we can hit this path in case of regular "vmap" +* allocations, if "this" current CPU was not preloaded. +* See the comment in alloc_vmap_area() why. If so, then +* GFP_NOWAIT is used instead to get an extra object for +* split purpose. That is rare and most time does not +* occur. +* +* What happens if an allocation gets failed. Basically, +* an "overflow" path is triggered to purge lazily freed +* areas to free some memory, then, the "retry" path is +* triggered to repeat one more time. See more details +* in alloc_vmap_area() function. */ lva =kmem_cache_alloc (vmap_area_cachep,GFP_NOWAIT );if (!lva)
@@ -1078,31 +1091,34 @@ static struct vmap_area *alloc_vmap_area(unsigned long size, retry:/* -* Preload this CPU with one extra vmap_area object to ensure -* that we have it available when fit type of free area is -* NE_FIT_TYPE. +* Preload this CPU with one extra vmap_area object. It is used +* when fit type of free area is NE_FIT_TYPE. Please note, it +* does not guarantee that an allocation occurs on a CPU that +* is preloaded, instead we minimize the case when it is not. +* It can happen because of migration, because there is a race +* until the below spinlock is taken. * * The preload is done in non-atomic context, thus it allows us * to use more permissive allocation masks to be more stable under -* low memory condition and high memory pressure. +* low memory condition and high memory pressure. In rare case, +* if not preloaded, GFP_NOWAIT is used. * -* Even if it fails we do not really care about that. Just proceed -* as it is. "overflow" path will refill the cache we allocate from. +* Set "pva" to NULL here, because of "retry" path. */ -preempt_disable (); -if (!__this_cpu_read (ne_fit_preload_node)) { -preempt_enable (); - pva =kmem_cache_alloc_node (vmap_area_cachep,GFP_KERNEL , node); -preempt_disable (); + pva =NULL ; -if (__this_cpu_cmpxchg (ne_fit_preload_node,NULL , pva)) { -if (pva) -kmem_cache_free (vmap_area_cachep, pva); - } - } +if (!this_cpu_read (ne_fit_preload_node)) +/* +* Even if it fails we do not really care about that. +* Just proceed as it is. If needed "overflow" path +* will refill the cache we allocate from. +*/ + pva =kmem_cache_alloc_node (vmap_area_cachep,GFP_KERNEL , node);spin_lock (&vmap_area_lock); -preempt_enable (); + +if (pva &&__this_cpu_cmpxchg (ne_fit_preload_node,NULL , pva)) +kmem_cache_free (vmap_area_cachep, pva);/* * If an allocation fails, the "vend" address is
Signature · 1 line
2.20.1
Quoted 19 lines · Uladzislau Rezki (Sony)
Get rid of
preempt_disable()andpreempt_enable()when the preload is done for splitting purpose. The reason is that callingspin_lock()with disabled preemtion is forbidden inCONFIG_PREEMPT_RTkernel.Therefore, we do not guarantee that a CPU is preloaded, instead we minimize the case when it is not with this change.
For example i run the special test case that follows the preload pattern and path. 20 "unbind" threads run it and each does 1000000 allocations. Only 3.5 times among 1000000 a CPU was not preloaded. So it can happen but the number is negligible.
V1 -> V2:
- move__this_cpu_cmpxchgcheck whenspin_lockis taken, as proposed by Andrew Morton
- add more explanation in regard of preloading
- adjust and move some comments
Quoted 3 lines
Fixes 82dd23e84be3 ("mm/vmalloc.c: preload a CPU with one object for split purpose")Reviewed-by Steven Rostedt (VMware) <[email protected]>Signed-off-by Uladzislau Rezki (Sony) <[email protected]>
Thank you.
Sebastian
Daniel Wagner
Acked-byQuoted 20 lines · Sebastian Andrzej Siewior
Quoted 19 lines · Uladzislau Rezki (Sony)
Get rid of
preempt_disable()andpreempt_enable()when the preload is done for splitting purpose. The reason is that callingspin_lock()with disabled preemtion is forbidden inCONFIG_PREEMPT_RTkernel.Therefore, we do not guarantee that a CPU is preloaded, instead we minimize the case when it is not with this change.
For example i run the special test case that follows the preload pattern and path. 20 "unbind" threads run it and each does 1000000 allocations. Only 3.5 times among 1000000 a CPU was not preloaded. So it can happen but the number is negligible.
V1 -> V2:
- move__this_cpu_cmpxchgcheck whenspin_lockis taken, as proposed by Andrew Morton
- add more explanation in regard of preloading
- adjust and move some comments
Quoted 5 lines
Quoted 3 lines
Fixes 82dd23e84be3 ("mm/vmalloc.c: preload a CPU with one object for split purpose")Reviewed-by Steven Rostedt (VMware) <[email protected]>Signed-off-by Uladzislau Rezki (Sony) <[email protected]>
Quoted 4 lines · Uladzislau Rezki (Sony)
Get rid of
preempt_disable()andpreempt_enable()when the preload is done for splitting purpose. The reason is that callingspin_lock()with disabled preemtion is forbidden inCONFIG_PREEMPT_RTkernel.
I think it would be really helpful to describe why the preemption was disabled in that path. Some of that is explained in the comment but the changelog should mention that explicitly.
Quoted 34 lines
Therefore, we do not guarantee that a CPU is preloaded, instead we minimize the case when it is not with this change.
For example i run the special test case that follows the preload pattern and path. 20 "unbind" threads run it and each does 1000000 allocations. Only 3.5 times among 1000000 a CPU was not preloaded. So it can happen but the number is negligible.
V1 -> V2:
- move__this_cpu_cmpxchgcheck whenspin_lockis taken, as proposed by Andrew Morton
- add more explanation in regard of preloading
- adjust and move some commentsFixes 82dd23e84be3 ("mm/vmalloc.c: preload a CPU with one object for split purpose")Reviewed-by Steven Rostedt (VMware) <[email protected]>Signed-off-by Uladzislau Rezki (Sony) <[email protected]>--- mm/vmalloc.c | 50 +++++++++++++++++++++++++++++++++----------------- 1 file changed, 33 insertions(+), 17 deletions(-)mm/vmalloc.c +5
-0@@ -969,6 +969,19 @@ adjust_va_to_fit_type(struct vmap_area *va,* There are a few exceptions though, as an example it is * a first allocation (early boot up) when we have "one" * big free space that has to be split. +* +* Also we can hit this path in case of regular "vmap" +* allocations, if "this" current CPU was not preloaded. +* See the comment in alloc_vmap_area() why. If so, then +* GFP_NOWAIT is used instead to get an extra object for
mm/vmalloc.c adjust_va_to_fit_type(struct vmap_area *va, · line 986
Quoted 10 lines
mm/vmalloc.c +8
-0+* split purpose. That is rare and most time does not +* occur. +* +* What happens if an allocation gets failed. Basically, +* an "overflow" path is triggered to purge lazily freed +* areas to free some memory, then, the "retry" path is +* triggered to repeat one more time. See more details +* in alloc_vmap_area() function. */ lva =kmem_cache_alloc (vmap_area_cachep,GFP_NOWAIT );
This doesn't seem to have anything to do with the patch. Have you considered to make it a patch on its own? Btw. I find this comment very useful!
Quoted 4 lines
mm/vmalloc.c +0
-0if (!lva)@@ -1078,31 +1091,34 @@ static struct vmap_area *alloc_vmap_area(unsigned long size, retry:
mm/vmalloc.c static struct vmap_area *alloc_vmap_area(unsigned long size, · line 1099
Quoted 10 lines
mm/vmalloc.c +6
-3/* -* Preload this CPU with one extra vmap_area object to ensure -* that we have it available when fit type of free area is -* NE_FIT_TYPE. +* Preload this CPU with one extra vmap_area object. It is used +* when fit type of free area is NE_FIT_TYPE. Please note, it +* does not guarantee that an allocation occurs on a CPU that +* is preloaded, instead we minimize the case when it is not. +* It can happen because of migration, because there is a race +* until the below spinlock is taken.
s@migration@cpu migration@ because migration without on its own is quite ambiguous, especially in the MM code where it usually refers to memory.
Quoted 41 lines
mm/vmalloc.c +14
-14* * The preload is done in non-atomic context, thus it allows us * to use more permissive allocation masks to be more stable under -* low memory condition and high memory pressure. +* low memory condition and high memory pressure. In rare case, +* if not preloaded, GFP_NOWAIT is used. * -* Even if it fails we do not really care about that. Just proceed -* as it is. "overflow" path will refill the cache we allocate from. +* Set "pva" to NULL here, because of "retry" path. */ -preempt_disable (); -if (!__this_cpu_read (ne_fit_preload_node)) { -preempt_enable (); - pva =kmem_cache_alloc_node (vmap_area_cachep,GFP_KERNEL , node); -preempt_disable (); + pva =NULL ; -if (__this_cpu_cmpxchg (ne_fit_preload_node,NULL , pva)) { -if (pva) -kmem_cache_free (vmap_area_cachep, pva); - } - } +if (!this_cpu_read (ne_fit_preload_node)) +/* +* Even if it fails we do not really care about that. +* Just proceed as it is. If needed "overflow" path +* will refill the cache we allocate from. +*/ + pva =kmem_cache_alloc_node (vmap_area_cachep,GFP_KERNEL , node);spin_lock (&vmap_area_lock); -preempt_enable (); + +if (pva &&__this_cpu_cmpxchg (ne_fit_preload_node,NULL , pva)) +kmem_cache_free (vmap_area_cachep, pva);/* * If an allocation fails, the "vend" address is Signature · 1 line
2.20.1
Signature · 2 lines
Michal Hocko SUSE Labs
Uladzislau Rezki
maintainerQuoted 10 lines · Michal Hocko
Quoted 4 lines · Uladzislau Rezki (Sony)
Get rid of
preempt_disable()andpreempt_enable()when the preload is done for splitting purpose. The reason is that callingspin_lock()with disabled preemtion is forbidden inCONFIG_PREEMPT_RTkernel.I think it would be really helpful to describe why the preemption was disabled in that path. Some of that is explained in the comment but the changelog should mention that explicitly.
Will do that, makes sense.
Quoted 40 lines
Quoted 40 lines
Therefore, we do not guarantee that a CPU is preloaded, instead we minimize the case when it is not with this change.
For example i run the special test case that follows the preload pattern and path. 20 "unbind" threads run it and each does 1000000 allocations. Only 3.5 times among 1000000 a CPU was not preloaded. So it can happen but the number is negligible.
V1 -> V2:
- move__this_cpu_cmpxchgcheck whenspin_lockis taken, as proposed by Andrew Morton
- add more explanation in regard of preloading
- adjust and move some commentsFixes 82dd23e84be3 ("mm/vmalloc.c: preload a CPU with one object for split purpose")Reviewed-by Steven Rostedt (VMware) <[email protected]>Signed-off-by Uladzislau Rezki (Sony) <[email protected]>--- mm/vmalloc.c | 50 +++++++++++++++++++++++++++++++++----------------- 1 file changed, 33 insertions(+), 17 deletions(-)mm/vmalloc.c +11
-0@@ -969,6 +969,19 @@ adjust_va_to_fit_type(struct vmap_area *va,* There are a few exceptions though, as an example it is * a first allocation (early boot up) when we have "one" * big free space that has to be split. +* +* Also we can hit this path in case of regular "vmap" +* allocations, if "this" current CPU was not preloaded. +* See the comment in alloc_vmap_area() why. If so, then +* GFP_NOWAIT is used instead to get an extra object for +* split purpose. That is rare and most time does not +* occur. +* +* What happens if an allocation gets failed. Basically, +* an "overflow" path is triggered to purge lazily freed +* areas to free some memory, then, the "retry" path is
Quoted 9 lines
mm/vmalloc.c adjust_va_to_fit_type(struct vmap_area *va, · line 986
Quoted 4 lines
mm/vmalloc.c +2
-0+* triggered to repeat one more time. See more details +* in alloc_vmap_area() function. */ lva =kmem_cache_alloc (vmap_area_cachep,GFP_NOWAIT );This doesn't seem to have anything to do with the patch. Have you considered to make it a patch on its own? Btw. I find this comment very useful!
Makes sense, will make it as separate patch.
Quoted 18 lines
Quoted 4 lines
mm/vmalloc.c +0
-0if (!lva)@@ -1078,31 +1091,34 @@ static struct vmap_area *alloc_vmap_area(unsigned long size, retry:mm/vmalloc.c static struct vmap_area *alloc_vmap_area(unsigned long size, · line 1099
Quoted 10 lines
mm/vmalloc.c +6
-3/* -* Preload this CPU with one extra vmap_area object to ensure -* that we have it available when fit type of free area is -* NE_FIT_TYPE. +* Preload this CPU with one extra vmap_area object. It is used +* when fit type of free area is NE_FIT_TYPE. Please note, it +* does not guarantee that an allocation occurs on a CPU that +* is preloaded, instead we minimize the case when it is not. +* It can happen because of migration, because there is a race +* until the below spinlock is taken. s@migration@cpu migration@ because migration without on its own is quite ambiguous, especially in the MM code where it usually refers to memory.
Thanks, will update it.
Thank you for the comments!
--
Vlad Rezki