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

["Liam R. Howlett" <Liam.Howlett@oracle.com>]

* Mike Rapoport <rppt@kernel.org> [250707 07:32]:
> 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 :)
> 

Yes, if there is already a node with a value with the same range, there
will be no allocations that will happen, so it'll just change the
pointer for you.  This is a slot store operation.

But, if it's possible to have no entries (an empty tree, or a single
value at 0), you will most likely allocate a node to store it, which is
256B.

I don't think this is a concern in this particular case though as you
are searching for an entry and storing, so it needs to exist.  So
really, the only scenario here is if you store 1 - ULONG_MAX (without
having expanded a root node) or 0 - ULONG_MAX, and that seems invalid.

Thanks,
Liam