> 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@xxxxxxxxxxxxxxx
>
> 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
