[Spice-devel] [PATCH 4/8] Always release free resources when allocating

[Alexander Larsson]

On Sun, 2010-08-22 at 14:19 +0300, Yonit Halperin wrote:
> On 08/20/2010 09:54 PM, alexl at redhat.com wrote:
> > From: Alexander Larsson<alexl at redhat.com>
> >
> > Leaving around potentially freeable resources when you're about
> > to allocate just increases the risk for fragmentation, which is bad,
> > especially for the vram where we do many large allocations in a
> > limited space.
> >
> > This also makes all consumers of the release_ring use the malloc
> > semaphore to make such accesses thread-safe.
> > ---
> >   display/qxldd.h |    1 -
> >   display/res.c   |   72 ++++++++++++++++++++++++++-----------------------------
> >   2 files changed, 34 insertions(+), 39 deletions(-)
> >
> > diff --git a/display/qxldd.h b/display/qxldd.h
> > index 7d22a0d..9f09aa8 100644
> > --- a/display/qxldd.h
> > +++ b/display/qxldd.h
> > @@ -178,7 +178,6 @@ typedef struct DevRes {
> >       MspaceInfo mspaces[NUM_MSPACES];
> >
> >       BOOL need_init;
> > -    UINT64 free_outputs;
> >       UINT32 update_id;
> >
> >       DevResDynamic *dynamic;
> > diff --git a/display/res.c b/display/res.c
> > index 15e14ae..b30951f 100644
> > --- a/display/res.c
> > +++ b/display/res.c
> > @@ -36,8 +36,6 @@
> >   #define WAIT_FOR_EVENT(pdev, event, timeout) EngWaitForSingleObject(event, timeout)
> >   #endif
> >
> > -#define SURFACE_ALLOC_RELEASE_BUNCH_SIZE 3
> > -
> >   static _inline QXLPHYSICAL PA(PDev *pdev, PVOID virt, UINT8 slot_id)
> >   {
> >       PMemSlot *p_slot =&pdev->mem_slots[slot_id];
> > @@ -124,6 +122,13 @@ UINT64 ReleaseOutput(PDev *pdev, UINT64 output_id)
> >       return next;
> >   }
> >
> > +static void ReleaseOutputs(PDev *pdev, UINT64 output_id)
> > +{
> > +    while (output_id != 0) {
> > +        output_id = ReleaseOutput(pdev, output_id);
> > +    }
> > +}
> > +
> >   static void AddRes(PDev *pdev, QXLOutput *output, Resource *res)
> >   {
> >       DEBUG_PRINT((pdev, 9, "%s\n", __FUNCTION__));
> > @@ -285,10 +290,21 @@ static void WaitForReleaseRing(PDev* pdev)
> >       DEBUG_PRINT((pdev, 16, "%s: 0x%lx, done\n", __FUNCTION__, pdev));
> >   }
> >
> > +static void EmptyReleaseRing(PDev *pdev)
> > +{
> > +    UINT64 outputs;
> > +    int notify;
> > +
> > +    while (SPICE_RING_IS_EMPTY(pdev->release_ring)) {
> > +        outputs = *SPICE_RING_CONS_ITEM(pdev->release_ring);
> > +        SPICE_RING_POP(pdev->release_ring, notify);
> > +        ReleaseOutputs(pdev, outputs);
> > +    }
> > +}
> > +

> - Shouldn't it be while !SPICE_RING_IS_EMPTY(pdev->release_ring)?

Ah, yeah. Good catch.

> - I think this loop should be limited, since items can be pushed to the 
> ring by the server during the loop, and then the call that triggered the 
> memory allocation might take too long. I think it would be better if 
> deallocating resources will be spread evenly among allocation calls, 
> then the possibility of choking individual operations.

Having some form of limit might be a good idea I guess. However, I'd
like it to be pretty large. First of all, I don't expect free calls to
be very expensive (certainly not compared to some drawing operations).
Secondly, the frees have to be done anyway, and due to cache reasons its
probably more efficient to free them at the same time (although it makes
malloc times less dependable).

Third, I think with the free-early approach we'd be spreading frees
pretty evenly anyway. With the current system we free nothing until all
memory is in used, then we start freeing a few things one at a time,
retrying the failed mspace_malloc() call after each free. This can
hardly be efficient. Instead in my model we'd continuosly free
everything that the server "naturally" frees, like drawables that were
sent to the client and whatnot. There likely enter the release queue in
an even flow and will get freed in an even flow. Then when we hit an OOM
it will be a real OOM and the spice server will flush caches, pipes and
whatnot, releasing a bunch of memory. I guess this could cause somewhat
of an unexpectedly large stall, with the roundtrip to the server and
whatnot. On the other hand, its exactly when memory is almost full we
get the worst fragmentation from allocations, so its important that we
at least free a large chunk of the returned resources before allocating.

> - can you compare the performance before and after changing the 
> deallocation strategy?

I'm not sure what the best way to measure performance is here.

> 
> > @@ -1328,14 +1326,12 @@ static CacheImage *AllocCacheImage(PDev* pdev)
> >   {
> >       RingItem *item;
> >       while (!(item = RingGetTail(pdev,&pdev->Res.dynamic->cache_image_lru))) {
> > -        int notify;
> > -        if (pdev->Res.free_outputs) {
> > -            pdev->Res.free_outputs = ReleaseOutput(pdev, pdev->Res.free_outputs);
> > -            continue;
> > +        EngAcquireSemaphore(pdev->malloc_sem);
> > +        if (SPICE_RING_IS_EMPTY(pdev->release_ring)) {
> > +            WaitForReleaseRing(pdev);
> >           }
> > -        WaitForReleaseRing(pdev);
> > -        pdev->Res.free_outputs = *SPICE_RING_CONS_ITEM(pdev->release_ring);
> > -        SPICE_RING_POP(pdev->release_ring, notify);
> > +        EmptyReleaseRing(pdev);
> > +        EngReleaseSemaphore(pdev->malloc_sem);
> >       }
> >       RingRemove(pdev, item);
> >       return CONTAINEROF(item, CacheImage, lru_link);
> 
> As you stated in the introductory email, we should acquire more 
> comprehension of concurrency in windows display drivers, in order to 
> understand which data should be protected.

Yeah, this is sort of ad-hoc, but *something* has to protecxt the
release_ring as its used both from AllocCacheImage and Alloc, which may
be parallely run.

-- 
=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=
 Alexander Larsson                                            Red Hat, Inc 
       alexl at redhat.com            alexander.larsson at gmail.com 
He's a scrappy voodoo vampire hunter who must take medication to keep him 
sane. She's a cold-hearted paranoid schoolgirl looking for love in all the 
wrong places. They fight crime!