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

[Frediano Ziglio <freddy77@xxxxxxxxx>]

On Wed, 8 Jul 2026 at 14:19, Anthony PERARD <anthony.perard@xxxxxxxxxx> wrote:
>
> 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.
>

Changed to return 0, NULL pointer is handled by the caller anyway.

> Cheers,
>
>
> --
> Anthony Perard | Vates XCP-ng Developer
>
> XCP-ng & Xen Orchestra - Vates solutions
>
> web: https://vates.tech