On Mon, Jul 07, 2025 at 01:11:02PM +0200, Peter Zijlstra wrote:
> On Fri, Jul 04, 2025 at 04:49:38PM +0300, Mike Rapoport wrote:
> > static bool execmem_cache_free(void *ptr)
> > {
> > struct maple_tree *busy_areas = &execmem_cache.busy_areas;
> > unsigned long addr = (unsigned long)ptr;
> > MA_STATE(mas, busy_areas, addr, addr);
> > void *area;
> > + int err;
> > +
> > + guard(mutex)(&execmem_cache.mutex);
> >
> > area = mas_walk(&mas);
> > + if (!area)
> > return false;
> >
> > + err = __execmem_cache_free(&mas, ptr, GFP_KERNEL | __GFP_NORETRY);
> > + if (err)
> > + goto err_slowpath;
> >
> > schedule_work(&execmem_cache_clean_work);
> >
> > return true;
> > +
> > +err_slowpath:
> > + mas_store_gfp(&mas, pending_free_set(ptr), GFP_KERNEL);
> > + execmem_cache.pending_free_cnt++;
> > + schedule_delayed_work(&execmem_cache_free_work, FREE_DELAY);
> > + return true;
> > }
>
> This is a bit if an anti-pattern, using guard() and error goto. Since
Good to know :)
> there is only the one site, its best to write it like so:
>
> static bool execmem_cache_free(void *ptr)
> {
> struct maple_tree *busy_areas = &execmem_cache.busy_areas;
> unsigned long addr = (unsigned long)ptr;
> MA_STATE(mas, busy_areas, addr, addr);
> void *area;
> int err;
>
> guard(mutex)(&execmem_cache.mutex);
>
> area = mas_walk(&mas);
> if (!area)
> return false;
>
> err = __execmem_cache_free(&mas, ptr, GFP_KERNEL | __GFP_NORETRY);
> if (err) {
> mas_store_gfp(&mas, pending_free_set(ptr), GFP_KERNEL);
> execmem_cache.pending_free_cnt++;
> schedule_delayed_work(&execmem_cache_free_work, FREE_DELAY);
> return true;
> }
>
> schedule_work(&execmem_cache_clean_work);
> return true;
> }
>
> And now I have to ask what happens if mas_store_gfp() returns an error?
AFAIU it won't. mas points to exact slot we've got the area from, nothing else
can modify the tree because of the mutex, so that mas_store_gfp()
essentially updates the value at an existing entry.
I'll add a comment about it.
Added @Liam to make sure I'm not saying nonsense :)
--
Sincerely yours,
Mike.
