Re: [PATCH v6 06/16] libs/call: cache up to 4 pages in hypercall bounce buffers

[Anthony PERARD <anthony.perard@xxxxxxxxxx>]

On Tue, Jul 07, 2026 at 03:47:07PM +0100, Frediano Ziglio wrote:
> On Tue, 7 Jul 2026 at 14:51, Anthony PERARD <anthony.perard@xxxxxxxxxx> wrote:
> > On Fri, Jun 19, 2026 at 02:04:51PM +0100, Frediano Ziglio wrote:
> > > diff --git a/tools/libs/call/buffer.c b/tools/libs/call/buffer.c
> > > index 155e4f9d43..2f0515c273 100644
> > > --- a/tools/libs/call/buffer.c
> > > +++ b/tools/libs/call/buffer.c
> > > @@ -49,6 +49,9 @@ static void *cache_alloc(xencall_handle *xcall, size_t 
> > > nr_pages)
> > >  {
> > >      void *p = NULL;
> > >
> > > +    if ( nr_pages == 0 )
> > > +        return NULL;
> >
> > By doing that check here, we don't update the stat anymore. And it's
> > getting out-of-sync with the updates done in cache_free().
> >
> > Before, we where returning a cache entry for that, and cache_hit++. I
> > think it's ok to return cache_miss++ instead.
> >
> 
> Well... requesting 0 pages is weird by definition, even malloc(0) is
> not well defined.

malloc(0) isn't defined as weird, it is defined as
"implementation-defined" ;-). But the pointer that the cache function
handle isn't from malloc().

> In theory in this case returning NULL would cause cache_free to not be
> called as filtered by xencall_free_buffer_pages.

Yes, for cases where the allocator returned NULL. But I can't find any
guaranty of this. So I would prefer to have both cache_alloc() and
cache_free() behave the same way when faced with nr_pages==0, without
hindsight into the value of the pointer.

> 
> I think the most symmetric think would be adding a similar test in
> cache_free, like
> 
> static int cache_free(xencall_handle *xcall, void *p, size_t nr_pages)
> {
>     int rc = 0;
> 
>     if ( nr_pages == 0 )
>         return 1;
> 
>     cache_lock(xcall);
> 
> 
> (the return 1 is needed to prevent the attempt to munmap the pointer
> which does not make sense).

If we have a pointer that is not NULL, we must free it. Even if you
think it doesn't make sense. Also, there's no way to know, here, whether
munmap() or an other function is going to be used. So, cache_free() must
not say that it cached the pointer, and let the caller free it.

Cheers,


--
Anthony Perard | Vates XCP-ng Developer

XCP-ng & Xen Orchestra - Vates solutions

web: https://vates.tech