Re: [PATCH 3/8] execmem: rework execmem_cache_free()

[Mike Rapoport <rppt@kernel.org>]

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.