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

[Yonit Halperin]

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)?
- 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.
- can you compare the performance before and after changing the 
deallocation strategy?



> @@ -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.

Cheers,
Yonit.