| Message ID | 20211222194828.15320-1-manfred@colorfullife.com |
|---|---|
| State | New |
| Headers | show |
| Series | mm/util.c: Make kvfree() safe for calling while holding spinlocks | expand |
> One codepath in find_alloc_undo() calls kvfree() while holding a spinlock. > Since vfree() can sleep this is a bug. > > Previously, the code path used kfree(), and kfree() is safe to be called > while holding a spinlock. > > Minghao proposed to fix this by updating find_alloc_undo(). > > Alternate proposal to fix this: Instead of changing find_alloc_undo(), > change kvfree() so that the same rules as for kfree() apply: > Having different rules for kfree() and kvfree() just asks for bugs. > > Disadvantage: Releasing vmalloc'ed memory will be delayed a bit. > I guess the issues is with "vmap_purge_lock" mutex? I think it is better to make the vfree() call as non-blocking one, i.e. the current design is is suffering from one drawback. It is related to purging the outstanding lazy areas from caller context. The drain process can be time consuming and if it is done from high-prio or RT contexts it can hog a CPU. Another issue is what you have reported that is about calling the schedule() and holding spinlock. The proposal is to perform a drain in a separate work: <snip> diff --git a/mm/vmalloc.c b/mm/vmalloc.c index d2a00ad4e1dd..7c5d9b148fa4 100644 --- a/mm/vmalloc.c +++ b/mm/vmalloc.c @@ -1717,18 +1717,6 @@ static bool __purge_vmap_area_lazy(unsigned long start, unsigned long end) return true; } -/* - * Kick off a purge of the outstanding lazy areas. Don't bother if somebody - * is already purging. - */ -static void try_purge_vmap_area_lazy(void) -{ - if (mutex_trylock(&vmap_purge_lock)) { - __purge_vmap_area_lazy(ULONG_MAX, 0); - mutex_unlock(&vmap_purge_lock); - } -} - /* * Kick off a purge of the outstanding lazy areas. */ @@ -1740,6 +1728,16 @@ static void purge_vmap_area_lazy(void) mutex_unlock(&vmap_purge_lock); } +static void drain_vmap_area(struct work_struct *work) +{ + if (mutex_trylock(&vmap_purge_lock)) { + __purge_vmap_area_lazy(ULONG_MAX, 0); + mutex_unlock(&vmap_purge_lock); + } +} + +static DECLARE_WORK(drain_vmap_area_work, drain_vmap_area); + /* * Free a vmap area, caller ensuring that the area has been unmapped * and flush_cache_vunmap had been called for the correct range @@ -1766,7 +1764,7 @@ static void free_vmap_area_noflush(struct vmap_area *va) /* After this point, we may free va at any time */ if (unlikely(nr_lazy > lazy_max_pages())) - try_purge_vmap_area_lazy(); + schedule_work(&drain_vmap_area_work); } /* <snip> -- Vlad Rezki
On Sat, Dec 25, 2021 at 07:54:12PM +0100, Uladzislau Rezki wrote: > +static void drain_vmap_area(struct work_struct *work) > +{ > + if (mutex_trylock(&vmap_purge_lock)) { > + __purge_vmap_area_lazy(ULONG_MAX, 0); > + mutex_unlock(&vmap_purge_lock); > + } > +} > + > +static DECLARE_WORK(drain_vmap_area_work, drain_vmap_area); Presuambly if the worker fails to get the mutex, it should reschedule itself? And should it even trylock or just always lock? This kind of ties into something I've been wondering about -- we have a number of places in the kernel which cache 'freed' vmalloc allocations in order to speed up future allocations of the same size. Kind of like slab. Would we be better off trying to cache frequent allocations inside vmalloc instead of always purging them?
On Sat, Dec 25, 2021 at 10:58:29PM +0000, Matthew Wilcox wrote: > On Sat, Dec 25, 2021 at 07:54:12PM +0100, Uladzislau Rezki wrote: > > +static void drain_vmap_area(struct work_struct *work) > > +{ > > + if (mutex_trylock(&vmap_purge_lock)) { > > + __purge_vmap_area_lazy(ULONG_MAX, 0); > > + mutex_unlock(&vmap_purge_lock); > > + } > > +} > > + > > +static DECLARE_WORK(drain_vmap_area_work, drain_vmap_area); > > Presuambly if the worker fails to get the mutex, it should reschedule > itself? And should it even trylock or just always lock? > mutex_trylock() has no sense here. It should just always get the lock. Otherwise we can miss the point to purge. Agree with your opinion. > > This kind of ties into something I've been wondering about -- we have > a number of places in the kernel which cache 'freed' vmalloc allocations > in order to speed up future allocations of the same size. Kind of like > slab. Would we be better off trying to cache frequent allocations > inside vmalloc instead of always purging them? > Hm... Some sort of caching would be good. Though it will require some time to think over all details and design itself. We can cache VAs instead of purging them until some point or threshold. So basically we can keep it in our data structures, associate it with some cache, based on size and reuse it later in the alloc_vmap_area(). All that is related to "vmap_area" caching. Another option is to cache the "vm_struct". It includes "vmap_area" + pages to drive the mapping. It is a higher level of caching and i am not sure if an implementation would be so straightforward. -- Vlad Rezki
On Sun, Dec 26, 2021 at 06:57:16PM +0100, Uladzislau Rezki wrote: > On Sat, Dec 25, 2021 at 10:58:29PM +0000, Matthew Wilcox wrote: > > On Sat, Dec 25, 2021 at 07:54:12PM +0100, Uladzislau Rezki wrote: > > > +static void drain_vmap_area(struct work_struct *work) > > > +{ > > > + if (mutex_trylock(&vmap_purge_lock)) { > > > + __purge_vmap_area_lazy(ULONG_MAX, 0); > > > + mutex_unlock(&vmap_purge_lock); > > > + } > > > +} > > > + > > > +static DECLARE_WORK(drain_vmap_area_work, drain_vmap_area); > > > > Presuambly if the worker fails to get the mutex, it should reschedule > > itself? And should it even trylock or just always lock? > > > mutex_trylock() has no sense here. It should just always get the lock. > Otherwise we can miss the point to purge. Agree with your opinion. > Below the patch that address Matthew's points: <snip> diff --git a/mm/vmalloc.c b/mm/vmalloc.c index d2a00ad4e1dd..b82db44fea60 100644 --- a/mm/vmalloc.c +++ b/mm/vmalloc.c @@ -1717,17 +1717,10 @@ static bool __purge_vmap_area_lazy(unsigned long start, unsigned long end) return true; } -/* - * Kick off a purge of the outstanding lazy areas. Don't bother if somebody - * is already purging. - */ -static void try_purge_vmap_area_lazy(void) -{ - if (mutex_trylock(&vmap_purge_lock)) { - __purge_vmap_area_lazy(ULONG_MAX, 0); - mutex_unlock(&vmap_purge_lock); - } -} +static void purge_vmap_area_lazy(void); +static void drain_vmap_area(struct work_struct *work); +static DECLARE_WORK(drain_vmap_area_work, drain_vmap_area); +static atomic_t drain_vmap_area_work_in_progress; /* * Kick off a purge of the outstanding lazy areas. @@ -1740,6 +1733,22 @@ static void purge_vmap_area_lazy(void) mutex_unlock(&vmap_purge_lock); } +static void drain_vmap_area(struct work_struct *work) +{ + mutex_lock(&vmap_purge_lock); + __purge_vmap_area_lazy(ULONG_MAX, 0); + mutex_unlock(&vmap_purge_lock); + + /* + * Check if rearming is still required. If not, we are + * done and can let a next caller to initiate a new drain. + */ + if (atomic_long_read(&vmap_lazy_nr) > lazy_max_pages()) + schedule_work(&drain_vmap_area_work); + else + atomic_set(&drain_vmap_area_work_in_progress, 0); +} + /* * Free a vmap area, caller ensuring that the area has been unmapped * and flush_cache_vunmap had been called for the correct range @@ -1766,7 +1775,8 @@ static void free_vmap_area_noflush(struct vmap_area *va) /* After this point, we may free va at any time */ if (unlikely(nr_lazy > lazy_max_pages())) - try_purge_vmap_area_lazy(); + if (!atomic_xchg(&drain_vmap_area_work_in_progress, 1)) + schedule_work(&drain_vmap_area_work); } /* <snip> Manfred, could you please have a look and if you have a time test it? I mean if it solves your issue. You can take over this patch and resend it, otherwise i can send it myself later if we all agree with it. -- Vlad Rezki
Hello Vlad, On 12/28/21 20:45, Uladzislau Rezki wrote: > [...] > Manfred, could you please have a look and if you have a time test it? > I mean if it solves your issue. You can take over this patch and resend > it, otherwise i can send it myself later if we all agree with it. I think we mix tasks: We have a bug in ipc/sem.c, thus we need a solution suitable for stable. Fixes: fc37a3b8b438 ("[PATCH] ipc sem: use kvmalloc for sem_undo allocation") Cc: stable@vger.kernel.org I think for stable, there are only two options: - change ipc/sem.c, call kvfree() after dropping the spinlock - change kvfree() to use vfree_atomic(). From my point of view, both approaches are fine. I.e. I'm waiting for feedback from an mm maintainer. As soon as it is agreed, I will retest the chosen solution. Now you propose to redesign vfree(), so that vfree() is safe to be called while holding spinlocks: > <snip> > diff --git a/mm/vmalloc.c b/mm/vmalloc.c > index d2a00ad4e1dd..b82db44fea60 100644 > --- a/mm/vmalloc.c > +++ b/mm/vmalloc.c > @@ -1717,17 +1717,10 @@ static bool __purge_vmap_area_lazy(unsigned long start, unsigned long end) > return true; > } > > -/* > - * Kick off a purge of the outstanding lazy areas. Don't bother if somebody > - * is already purging. > - */ > -static void try_purge_vmap_area_lazy(void) > -{ > - if (mutex_trylock(&vmap_purge_lock)) { > - __purge_vmap_area_lazy(ULONG_MAX, 0); > - mutex_unlock(&vmap_purge_lock); > - } > -} > +static void purge_vmap_area_lazy(void); > +static void drain_vmap_area(struct work_struct *work); > +static DECLARE_WORK(drain_vmap_area_work, drain_vmap_area); > +static atomic_t drain_vmap_area_work_in_progress; > > /* > * Kick off a purge of the outstanding lazy areas. > @@ -1740,6 +1733,22 @@ static void purge_vmap_area_lazy(void) > mutex_unlock(&vmap_purge_lock); > } > > +static void drain_vmap_area(struct work_struct *work) > +{ > + mutex_lock(&vmap_purge_lock); > + __purge_vmap_area_lazy(ULONG_MAX, 0); > + mutex_unlock(&vmap_purge_lock); > + > + /* > + * Check if rearming is still required. If not, we are > + * done and can let a next caller to initiate a new drain. > + */ > + if (atomic_long_read(&vmap_lazy_nr) > lazy_max_pages()) > + schedule_work(&drain_vmap_area_work); > + else > + atomic_set(&drain_vmap_area_work_in_progress, 0); > +} > + > /* > * Free a vmap area, caller ensuring that the area has been unmapped > * and flush_cache_vunmap had been called for the correct range > @@ -1766,7 +1775,8 @@ static void free_vmap_area_noflush(struct vmap_area *va) > > /* After this point, we may free va at any time */ > if (unlikely(nr_lazy > lazy_max_pages())) > - try_purge_vmap_area_lazy(); > + if (!atomic_xchg(&drain_vmap_area_work_in_progress, 1)) > + schedule_work(&drain_vmap_area_work); > } > > /* > <snip> I do now know the mm code well enough to understand the side effects of the change. And doubt that it is suitable for stable, i.e. we need the simple patch first. -- Manfred
> Hello Vlad, > > On 12/28/21 20:45, Uladzislau Rezki wrote: > > [...] > > Manfred, could you please have a look and if you have a time test it? > > I mean if it solves your issue. You can take over this patch and resend > > it, otherwise i can send it myself later if we all agree with it. > > I think we mix tasks: We have a bug in ipc/sem.c, thus we need a solution > suitable for stable. > > Fixes: fc37a3b8b438 ("[PATCH] ipc sem: use kvmalloc for sem_undo > allocation") > Cc: stable@vger.kernel.org > > I think for stable, there are only two options: > > - change ipc/sem.c, call kvfree() after dropping the spinlock > > - change kvfree() to use vfree_atomic(). > > From my point of view, both approaches are fine. > > I.e. I'm waiting for feedback from an mm maintainer. > > As soon as it is agreed, I will retest the chosen solution. > Here for me it anyway looks like a change and it is hard to judge if the second solution is stable or not, because it is a new change and the kvfree() interface is changed internally. > > Now you propose to redesign vfree(), so that vfree() is safe to be called > while holding spinlocks: > > > <snip> > > diff --git a/mm/vmalloc.c b/mm/vmalloc.c > > index d2a00ad4e1dd..b82db44fea60 100644 > > --- a/mm/vmalloc.c > > +++ b/mm/vmalloc.c > > @@ -1717,17 +1717,10 @@ static bool __purge_vmap_area_lazy(unsigned long start, unsigned long end) > > return true; > > } > > -/* > > - * Kick off a purge of the outstanding lazy areas. Don't bother if somebody > > - * is already purging. > > - */ > > -static void try_purge_vmap_area_lazy(void) > > -{ > > - if (mutex_trylock(&vmap_purge_lock)) { > > - __purge_vmap_area_lazy(ULONG_MAX, 0); > > - mutex_unlock(&vmap_purge_lock); > > - } > > -} > > +static void purge_vmap_area_lazy(void); > > +static void drain_vmap_area(struct work_struct *work); > > +static DECLARE_WORK(drain_vmap_area_work, drain_vmap_area); > > +static atomic_t drain_vmap_area_work_in_progress; > > /* > > * Kick off a purge of the outstanding lazy areas. > > @@ -1740,6 +1733,22 @@ static void purge_vmap_area_lazy(void) > > mutex_unlock(&vmap_purge_lock); > > } > > +static void drain_vmap_area(struct work_struct *work) > > +{ > > + mutex_lock(&vmap_purge_lock); > > + __purge_vmap_area_lazy(ULONG_MAX, 0); > > + mutex_unlock(&vmap_purge_lock); > > + > > + /* > > + * Check if rearming is still required. If not, we are > > + * done and can let a next caller to initiate a new drain. > > + */ > > + if (atomic_long_read(&vmap_lazy_nr) > lazy_max_pages()) > > + schedule_work(&drain_vmap_area_work); > > + else > > + atomic_set(&drain_vmap_area_work_in_progress, 0); > > +} > > + > > /* > > * Free a vmap area, caller ensuring that the area has been unmapped > > * and flush_cache_vunmap had been called for the correct range > > @@ -1766,7 +1775,8 @@ static void free_vmap_area_noflush(struct vmap_area *va) > > /* After this point, we may free va at any time */ > > if (unlikely(nr_lazy > lazy_max_pages())) > > - try_purge_vmap_area_lazy(); > > + if (!atomic_xchg(&drain_vmap_area_work_in_progress, 1)) > > + schedule_work(&drain_vmap_area_work); > > } > > /* > > <snip> > I do now know the mm code well enough to understand the side effects of the > change. And doubt that it is suitable for stable, i.e. we need the simple > patch first. > Well, it is as simple as it could be :) -- Vlad Rezki
Hi Andrew, On 1/27/22 03:53, Andrew Morton wrote: > On Wed, 22 Dec 2021 20:48:28 +0100 Manfred Spraul <manfred@colorfullife.com> wrote: > >> One codepath in find_alloc_undo() calls kvfree() while holding a spinlock. >> Since vfree() can sleep this is a bug. >> >> Previously, the code path used kfree(), and kfree() is safe to be called >> while holding a spinlock. >> >> Minghao proposed to fix this by updating find_alloc_undo(). >> >> Alternate proposal to fix this: Instead of changing find_alloc_undo(), >> change kvfree() so that the same rules as for kfree() apply: >> Having different rules for kfree() and kvfree() just asks for bugs. >> >> Disadvantage: Releasing vmalloc'ed memory will be delayed a bit. > I know we've been around this loop a bunch of times and deferring was > considered. But I forget the conclusion. IIRC, mhocko was involved? I do not remember a mail from mhocko. Shakeel proposed to use the approach from Chi. Decision: https://marc.info/?l=linux-kernel&m=164132032717757&w=2 With Reviewed-by: https://marc.info/?l=linux-kernel&m=164132744522325&w=2 >> --- a/mm/util.c >> +++ b/mm/util.c >> @@ -610,12 +610,12 @@ EXPORT_SYMBOL(kvmalloc_node); >> * It is slightly more efficient to use kfree() or vfree() if you are certain >> * that you know which one to use. >> * >> - * Context: Either preemptible task context or not-NMI interrupt. >> + * Context: Any context except NMI interrupt. >> */ >> void kvfree(const void *addr) >> { >> if (is_vmalloc_addr(addr)) >> - vfree(addr); >> + vfree_atomic(addr); >> else >> kfree(addr); >> }
On Thu 27-01-22 06:59:50, Manfred Spraul wrote: > Hi Andrew, > > On 1/27/22 03:53, Andrew Morton wrote: > > On Wed, 22 Dec 2021 20:48:28 +0100 Manfred Spraul <manfred@colorfullife.com> wrote: > > > > > One codepath in find_alloc_undo() calls kvfree() while holding a spinlock. > > > Since vfree() can sleep this is a bug. > > > > > > Previously, the code path used kfree(), and kfree() is safe to be called > > > while holding a spinlock. > > > > > > Minghao proposed to fix this by updating find_alloc_undo(). > > > > > > Alternate proposal to fix this: Instead of changing find_alloc_undo(), > > > change kvfree() so that the same rules as for kfree() apply: > > > Having different rules for kfree() and kvfree() just asks for bugs. > > > > > > Disadvantage: Releasing vmalloc'ed memory will be delayed a bit. > > I know we've been around this loop a bunch of times and deferring was > > considered. But I forget the conclusion. IIRC, mhocko was involved? > > I do not remember a mail from mhocko. I do not remember either. > > Shakeel proposed to use the approach from Chi. > > Decision: https://marc.info/?l=linux-kernel&m=164132032717757&w=2 And I would agree with Shakeel and go with the original change to the ipc code. That is trivial and without any other side effects like this one. I bet nobody has evaluated what the undconditional deferred freeing has. At least changelog doesn't really dive into that more than a very vague statement that this will happen. > With Reviewed-by: > > https://marc.info/?l=linux-kernel&m=164132744522325&w=2 > > > --- a/mm/util.c > > > +++ b/mm/util.c > > > @@ -610,12 +610,12 @@ EXPORT_SYMBOL(kvmalloc_node); > > > * It is slightly more efficient to use kfree() or vfree() if you are certain > > > * that you know which one to use. > > > * > > > - * Context: Either preemptible task context or not-NMI interrupt. > > > + * Context: Any context except NMI interrupt. > > > */ > > > void kvfree(const void *addr) > > > { > > > if (is_vmalloc_addr(addr)) > > > - vfree(addr); > > > + vfree_atomic(addr); > > > else > > > kfree(addr); > > > } >
On Thu, Jan 27, 2022 at 09:25:48AM +0100, Michal Hocko wrote: > On Thu 27-01-22 06:59:50, Manfred Spraul wrote: > > Hi Andrew, > > > > On 1/27/22 03:53, Andrew Morton wrote: > > > On Wed, 22 Dec 2021 20:48:28 +0100 Manfred Spraul <manfred@colorfullife.com> wrote: > > > > > > > One codepath in find_alloc_undo() calls kvfree() while holding a spinlock. > > > > Since vfree() can sleep this is a bug. > > > > > > > > Previously, the code path used kfree(), and kfree() is safe to be called > > > > while holding a spinlock. > > > > > > > > Minghao proposed to fix this by updating find_alloc_undo(). > > > > > > > > Alternate proposal to fix this: Instead of changing find_alloc_undo(), > > > > change kvfree() so that the same rules as for kfree() apply: > > > > Having different rules for kfree() and kvfree() just asks for bugs. > > > > > > > > Disadvantage: Releasing vmalloc'ed memory will be delayed a bit. > > > I know we've been around this loop a bunch of times and deferring was > > > considered. But I forget the conclusion. IIRC, mhocko was involved? > > > > I do not remember a mail from mhocko. > > I do not remember either. > > > > > Shakeel proposed to use the approach from Chi. > > > > Decision: https://marc.info/?l=linux-kernel&m=164132032717757&w=2 > > And I would agree with Shakeel and go with the original change to the > ipc code. That is trivial and without any other side effects like this > one. I bet nobody has evaluated what the undconditional deferred freeing > has. At least changelog doesn't really dive into that more than a very > vague statement that this will happen. > Absolutely agree here. Especially that changing the kvfree() will not look stable. After applying the https://www.spinics.net/lists/linux-mm/msg282264.html we will be able to use vfree() from atomic anyway. -- Vlad Rezki
diff --git a/mm/util.c b/mm/util.c index 741ba32a43ac..7f9181998835 100644 --- a/mm/util.c +++ b/mm/util.c @@ -610,12 +610,12 @@ EXPORT_SYMBOL(kvmalloc_node); * It is slightly more efficient to use kfree() or vfree() if you are certain * that you know which one to use. * - * Context: Either preemptible task context or not-NMI interrupt. + * Context: Any context except NMI interrupt. */ void kvfree(const void *addr) { if (is_vmalloc_addr(addr)) - vfree(addr); + vfree_atomic(addr); else kfree(addr); }
One codepath in find_alloc_undo() calls kvfree() while holding a spinlock. Since vfree() can sleep this is a bug. Previously, the code path used kfree(), and kfree() is safe to be called while holding a spinlock. Minghao proposed to fix this by updating find_alloc_undo(). Alternate proposal to fix this: Instead of changing find_alloc_undo(), change kvfree() so that the same rules as for kfree() apply: Having different rules for kfree() and kvfree() just asks for bugs. Disadvantage: Releasing vmalloc'ed memory will be delayed a bit. Reported-by: Zeal Robot <zealci@zte.com.cn> Reported-by: Minghao Chi <chi.minghao@zte.com.cn> Link: https://lore.kernel.org/all/20211222081026.484058-1-chi.minghao@zte.com.cn/ Fixes: fc37a3b8b438 ("[PATCH] ipc sem: use kvmalloc for sem_undo allocation") Cc: stable@vger.kernel.org Signed-off-by: Manfred Spraul <manfred@colorfullife.com> --- mm/util.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)