[Spice-devel] [PATCH 08/12] qxl-wddm-dod: Prepare for failure to allocate memory

Frediano Ziglio fziglio at redhat.com
Tue Mar 28 13:04:51 UTC 2017


> 
> Preparation for further scenarios when memory allocation failure
> can happen and we'll need to use fallback when possible.
> Memory allocation can fail in following cases:
> - when non-forced memory allocation used and the attempt to allocate
>   the memory must be as fast as possible, without waits
> - when forced memory allocation used, but the driver is in stop flow
>   and it is possible that video subsystem already executes switch
>   to VGA mode, so QEMU will not return previously allocated objects
> In case of allocation failure we may need to free previously allocated
> object (when more than one allocation required for operation), this
> must be protected by m_MemLock mutex.
> In case of forced memory allocation the allocation routine waits
> unpredictable time until the request is satisfied. Now we do not acquire
> m_MemLock mutex for all this time, but release it when enter long wait
> to allow another caller to enter and try to allocate memory.
> 
> Signed-off-by: Yuri Benditovich <yuri.benditovich at daynix.com>
> ---
>  qxldod/QxlDod.cpp | 107
>  ++++++++++++++++++++++++++++++++++++++++++++++++------
>  qxldod/QxlDod.h   |   4 +-
>  2 files changed, 99 insertions(+), 12 deletions(-)
> 
> diff --git a/qxldod/QxlDod.cpp b/qxldod/QxlDod.cpp
> index bb9b227..9a3803a 100755
> --- a/qxldod/QxlDod.cpp
> +++ b/qxldod/QxlDod.cpp
> @@ -3064,6 +3064,7 @@ QxlDevice::QxlDevice(_In_ QxlDod* pQxlDod)
>      m_FreeOutputs = 0;
>      m_Pending = 0;
>      m_PresentThread = NULL;
> +    m_bActive = FALSE;

What does it mean that the device is not active?

>  }
>  
>  QxlDevice::~QxlDevice(void)
> @@ -3494,14 +3495,20 @@ NTSTATUS QxlDevice::QxlInit(DXGK_DISPLAY_INFORMATION*
> pDispInfo)
>      Status = AcquireDisplayInfo(*(pDispInfo));
>      if (NT_SUCCESS(Status))
>      {
> +        m_bActive = TRUE;
>          Status = StartPresentThread();
>      }
> +    if (!NT_SUCCESS(Status))
> +    {
> +        m_bActive = FALSE;

So this means that if the thread cannot be created the device
will work in a an inactive state?

> +    }
>      return Status;
>  }
>  
>  void QxlDevice::QxlClose()
>  {
>      PAGED_CODE();
> +    m_bActive = FALSE;
>      StopPresentThread();
>      DestroyMemSlots();
>  }
> @@ -3982,14 +3989,18 @@ void QxlDevice::WaitForReleaseRing(void)
>  {
>      PAGED_CODE();
>      int wait;
> +    BOOLEAN locked;
>  
>      DbgPrint(TRACE_LEVEL_VERBOSE, ("--->%s\n", __FUNCTION__));
>  
> +    locked = WaitForObject(&m_MemLock, NULL);
>      for (;;) {
>          LARGE_INTEGER timeout;
>  
>          if (SPICE_RING_IS_EMPTY(m_ReleaseRing)) {
> +            ReleaseMutex(&m_MemLock, locked);
>              QXL_SLEEP(10);
> +            locked = WaitForObject(&m_MemLock, NULL);
>              if (!SPICE_RING_IS_EMPTY(m_ReleaseRing)) {
>                  break;
>              }
> @@ -3997,17 +4008,20 @@ void QxlDevice::WaitForReleaseRing(void)
>          }
>          SPICE_RING_CONS_WAIT(m_ReleaseRing, wait);
>  
> -        if (!wait) {
> +        if (!wait || !m_bActive) {
>              break;
>          }
>  
> +        ReleaseMutex(&m_MemLock, locked);
>          timeout.QuadPart = -30 * 1000 * 10; //30ms
>          WaitForObject(&m_DisplayEvent, &timeout);
> +        locked = WaitForObject(&m_MemLock, NULL);
>  
>          if (SPICE_RING_IS_EMPTY(m_ReleaseRing)) {
>              SyncIo(QXL_IO_NOTIFY_OOM, 0);
>          }
>      }
> +    ReleaseMutex(&m_MemLock, locked);
>      DbgPrint(TRACE_LEVEL_VERBOSE, ("%s: <---\n", __FUNCTION__));
>  }
>  
> @@ -4074,11 +4088,18 @@ UINT64 QxlDevice::ReleaseOutput(UINT64 output_id)
>      return next;
>  }
>  
> +void  QxlDevice::ReleaseOutputUnderLock(UINT64 output_id)
> +{
> +    WaitForObject(&m_MemLock, NULL);
> +    ReleaseOutput(output_id);
> +    ReleaseMutex(&m_MemLock, TRUE);
> +}
> +

Is not really clear why we need additional locking here.
We are allocating resources, failing to allocate some and cleaning up.
The FreeMem calls already use a MemLock to make mspace serialized.
Why we need additional serialization?

>  void *QxlDevice::AllocMem(UINT32 mspace_type, size_t size, BOOL force)
>  {
>      PAGED_CODE();
>      PVOID ptr;
> -    BOOLEAN locked = FALSE;
> +    BOOLEAN locked = FALSE, bInfiniteWait = !!force;
>  
>      ASSERT(m_MSInfo[mspace_type]._mspace);
>      DbgPrint(TRACE_LEVEL_VERBOSE, ("--->%s: %p(%d) size %u\n", __FUNCTION__,
> @@ -4089,7 +4110,19 @@ void *QxlDevice::AllocMem(UINT32 mspace_type, size_t
> size, BOOL force)
>       mspace_malloc_stats(m_MSInfo[mspace_type]._mspace);
>  #endif
>  
> -    locked = WaitForObject(&m_MemLock, NULL);
> +     if (bInfiniteWait)

Why don't you use force here instead?
I don't see the reason for another variable.
Also note that this code is aligned with 5 spaces instead of 4.

> +        locked = WaitForObject(&m_MemLock, NULL);
> +     else
> +     {
> +         LARGE_INTEGER doNotWait;
> +         doNotWait.QuadPart = 0;
> +         locked = WaitForObject(&m_MemLock, &doNotWait);
> +         if (!locked)
> +         {
> +             ptr = NULL;
> +             goto skip_allocation;

I would prefer a return NULL here without the goto.

> +         }
> +     }
>  
>      while (1) {
>          /* Release lots of queued resources, before allocating, as we
> @@ -4107,17 +4140,21 @@ void *QxlDevice::AllocMem(UINT32 mspace_type, size_t
> size, BOOL force)
>              continue;
>          }
>  
> -        if (force) {
> +        if (force && m_bActive) {
>              /* Ask spice to free some stuff */
> +            ReleaseMutex(&m_MemLock, locked);
>              WaitForReleaseRing();
> +            locked = WaitForObject(&m_MemLock, NULL);
>          } else {
>              /* Fail */
>              break;
>          }
>      }
> +
> +skip_allocation:
>      ReleaseMutex(&m_MemLock, locked);
>  
> -    ASSERT((!ptr && !force) || (ptr >= m_MSInfo[mspace_type].mspace_start &&
> +    ASSERT((!ptr && (!force || !m_bActive)) || (ptr >=
> m_MSInfo[mspace_type].mspace_start &&
>                                        ptr <
>                                        m_MSInfo[mspace_type].mspace_end));
>      DbgPrint(TRACE_LEVEL_VERBOSE, ("<---%s: ptr 0x%x\n", __FUNCTION__,
>      ptr));
>      return ptr;
> @@ -4150,6 +4187,10 @@ QXLDrawable *QxlDevice::GetDrawable()
>      QXLOutput *output;
>  
>      output = (QXLOutput *)AllocMem(MSPACE_TYPE_VRAM, sizeof(QXLOutput) +
>      sizeof(QXLDrawable), TRUE);
> +    if (!output)
> +    {
> +        return NULL;
> +    }
>      output->num_res = 0;
>      RESOURCE_TYPE(output, RESOURCE_TYPE_DRAWABLE);
>      ((QXLDrawable *)output->data)->release_info.id = (UINT64)output;
> @@ -4165,6 +4206,9 @@ QXLCursorCmd *QxlDevice::CursorCmd()
>  
>      DbgPrint(TRACE_LEVEL_VERBOSE, ("---> %s\n", __FUNCTION__));
>      output = (QXLOutput *)AllocMem(MSPACE_TYPE_VRAM, sizeof(QXLOutput) +
>      sizeof(QXLCursorCmd), TRUE);
> +    if (!output) {
> +        return NULL;
> +    }

We really should define a style!
Is it

   if (cond) code;

or

   if (cond) {
       code;
   }

or

   if (cond)
       code;

or

   if (cond)
   {
       code;
   }

only this patch contains at least 3 styles of if indentation!

>      output->num_res = 0;
>      RESOURCE_TYPE(output, RESOURCE_TYPE_CURSOR);
>      cursor_cmd = (QXLCursorCmd *)output->data;
> @@ -4314,6 +4358,10 @@ QXLDrawable *QxlDevice::Drawable(UINT8 type, CONST
> RECT *area, CONST RECT *clip,
>      DbgPrint(TRACE_LEVEL_VERBOSE, ("---> %s\n", __FUNCTION__));
>  
>      drawable = GetDrawable();
> +    if (!drawable)
> +    {
> +        return NULL;
> +    }
>      drawable->surface_id = surface_id;
>      drawable->type = type;
>      drawable->effect = QXL_EFFECT_OPAQUE;
> @@ -4326,7 +4374,7 @@ QXLDrawable *QxlDevice::Drawable(UINT8 type, CONST RECT
> *area, CONST RECT *clip,
>  
>      if (!SetClip(clip, drawable)) {
>          DbgPrint(TRACE_LEVEL_VERBOSE, ("%s: set clip failed\n",
>          __FUNCTION__));
> -        ReleaseOutput(drawable->release_info.id);
> +        ReleaseOutputUnderLock(drawable->release_info.id);
>          drawable = NULL;
>      }
>      DbgPrint(TRACE_LEVEL_VERBOSE, ("<--- %s\n", __FUNCTION__));
> @@ -4457,6 +4505,12 @@ QXLDrawable *QxlDevice::BltBits (
>      alloc_size = BITMAP_ALLOC_BASE + BITS_BUF_MAX - BITS_BUF_MAX %
>      line_size;
>      alloc_size = MIN(BITMAP_ALLOC_BASE + height * line_size, alloc_size);
>      image_res = (Resource*)AllocMem(MSPACE_TYPE_VRAM, alloc_size, TRUE);
> +    if (!image_res)
> +    {
> +        ReleaseOutputUnderLock(drawable->release_info.id);
> +        DbgPrint(TRACE_LEVEL_ERROR, ("Cannot allocate bitmap for
> drawable\n"));
> +        return NULL;
> +    }
>  
>      image_res->refs = 1;
>      image_res->free = FreeBitmapImageEx;
> @@ -4487,8 +4541,13 @@ QXLDrawable *QxlDevice::BltBits (
>      alloc_size = height * line_size;
>  
>      for (; src != src_end; src -= pSrc->Pitch, alloc_size -= line_size) {
> -        PutBytesAlign(&chunk, &dest, &dest_end, src,
> -                      line_size, alloc_size, line_size);
> +        if (!PutBytesAlign(&chunk, &dest, &dest_end, src,
> +            line_size, alloc_size, line_size))
> +        {
> +            ReleaseOutputUnderLock(drawable->release_info.id);
> +            DbgPrint(TRACE_LEVEL_ERROR, ("Cannot allocate additional bitmap
> for drawable\n"));
> +            return NULL;
> +        }
>      }
>  
>      internal->image.bitmap.palette = 0;
> @@ -4509,7 +4568,7 @@ QXLDrawable *QxlDevice::BltBits (
>      return drawable;
>  }
>  
> -VOID QxlDevice::PutBytesAlign(QXLDataChunk **chunk_ptr, UINT8 **now_ptr,
> +BOOLEAN QxlDevice::PutBytesAlign(QXLDataChunk **chunk_ptr, UINT8 **now_ptr,
>                              UINT8 **end_ptr, UINT8 *src, int size,
>                              size_t alloc_size, uint32_t alignment)
>  {
> @@ -4527,6 +4586,10 @@ VOID QxlDevice::PutBytesAlign(QXLDataChunk
> **chunk_ptr, UINT8 **now_ptr,
>              aligned_size -=  aligned_size % alignment;
>  
>              void *ptr = AllocMem(MSPACE_TYPE_VRAM, size +
>              sizeof(QXLDataChunk), TRUE);
> +            if (!ptr)
> +            {
> +                return FALSE;
> +            }
>              chunk->next_chunk = PA(ptr, m_SurfaceMemSlot);
>              ((QXLDataChunk *)ptr)->prev_chunk = PA(chunk, m_SurfaceMemSlot);
>              chunk = (QXLDataChunk *)ptr;
> @@ -4547,6 +4610,7 @@ VOID QxlDevice::PutBytesAlign(QXLDataChunk **chunk_ptr,
> UINT8 **now_ptr,
>      *now_ptr = now;
>      *end_ptr = end;
>      DbgPrint(TRACE_LEVEL_VERBOSE, ("<--- %s\n", __FUNCTION__));
> +    return TRUE;
>  }
>  
>  VOID QxlDevice::BlackOutScreen(CURRENT_BDD_MODE* pCurrentBddMod)
> @@ -4610,6 +4674,11 @@ NTSTATUS  QxlDevice::SetPointerShape(_In_ CONST
> DXGKARG_SETPOINTERSHAPE* pSetPoi
>      int num_images = 1;
>  
>      cursor_cmd = CursorCmd();
> +    if (!cursor_cmd) {
> +        DbgPrint(TRACE_LEVEL_ERROR, ("%s: Failed to allocate cursor
> command\n", __FUNCTION__));
> +        return STATUS_INSUFFICIENT_RESOURCES;
> +    }
> +
>      cursor_cmd->type = QXL_CURSOR_SET;
>  
>      cursor_cmd->u.set.visible = TRUE;
> @@ -4617,6 +4686,12 @@ NTSTATUS  QxlDevice::SetPointerShape(_In_ CONST
> DXGKARG_SETPOINTERSHAPE* pSetPoi
>      cursor_cmd->u.set.position.y = 0;
>  
>      res = (Resource *)AllocMem(MSPACE_TYPE_VRAM, CURSOR_ALLOC_SIZE, TRUE);
> +    if (!res) {
> +        DbgPrint(TRACE_LEVEL_ERROR, ("%s: Failed to allocate cursor data\n",
> __FUNCTION__));
> +        ReleaseOutputUnderLock(cursor_cmd->release_info.id);
> +        return STATUS_INSUFFICIENT_RESOURCES;
> +    }
> +
>      res->refs = 1;
>      res->free = FreeCursorEx;
>      res->ptr = this;
> @@ -4653,8 +4728,13 @@ NTSTATUS  QxlDevice::SetPointerShape(_In_ CONST
> DXGKARG_SETPOINTERSHAPE* pSetPoi
>      end = (UINT8 *)res + CURSOR_ALLOC_SIZE;
>      src_end = src + (pSetPointerShape->Pitch * pSetPointerShape->Height *
>      num_images);
>      for (; src != src_end; src += pSetPointerShape->Pitch) {
> -        PutBytesAlign(&chunk, &now, &end, src, line_size,
> -                 PAGE_SIZE, 1);
> +        if (!PutBytesAlign(&chunk, &now, &end, src, line_size,
> +                 PAGE_SIZE, 1))
> +        {
> +            DbgPrint(TRACE_LEVEL_ERROR, ("%s: Failed to allocate cursor
> bitmap\n", __FUNCTION__));
> +            ReleaseOutputUnderLock(cursor_cmd->release_info.id);
> +            return STATUS_INSUFFICIENT_RESOURCES;
> +        }
>      }
>      CursorCmdAddRes(cursor_cmd, res);
>      RELEASE_RES(res);
> @@ -4675,6 +4755,11 @@ NTSTATUS QxlDevice::SetPointerPosition(_In_ CONST
> DXGKARG_SETPOINTERPOSITION* pS
>                                   pSetPointerPosition->X,
>                                   pSetPointerPosition->Y));
>      QXLCursorCmd *cursor_cmd = CursorCmd();
> +    if (!cursor_cmd) {
> +        DbgPrint(TRACE_LEVEL_ERROR, ("%s: Failed to allocate cursor
> command\n", __FUNCTION__));
> +        return STATUS_INSUFFICIENT_RESOURCES;
> +    }
> +
>      if (pSetPointerPosition->X < 0 || !pSetPointerPosition->Flags.Visible) {
>          cursor_cmd->type = QXL_CURSOR_HIDE;
>      } else {
> diff --git a/qxldod/QxlDod.h b/qxldod/QxlDod.h
> index 7bb5de5..3ee16ae 100755
> --- a/qxldod/QxlDod.h
> +++ b/qxldod/QxlDod.h
> @@ -585,6 +585,7 @@ private:
>      void FlushReleaseRing();
>      void FreeMem(UINT32 mspace_type, void *ptr);
>      UINT64 ReleaseOutput(UINT64 output_id);
> +    void ReleaseOutputUnderLock(UINT64 output_id);
>      void WaitForReleaseRing(void);
>      void EmptyReleaseRing(void);
>      BOOL SetClip(const RECT *clip, QXLDrawable *drawable);
> @@ -601,7 +602,7 @@ private:
>      void PushCmd(void);
>      void WaitForCursorRing(void);
>      void PushCursor(void);
> -    void PutBytesAlign(QXLDataChunk **chunk_ptr, UINT8 **now_ptr,
> +    BOOLEAN PutBytesAlign(QXLDataChunk **chunk_ptr, UINT8 **now_ptr,
>                              UINT8 **end_ptr, UINT8 *src, int size,
>                              size_t alloc_size, uint32_t alignment);
>      void AsyncIo(UCHAR  Port, UCHAR Value);
> @@ -671,6 +672,7 @@ private:
>  
>      QXLPresentOnlyRing m_PresentRing[1];
>      HANDLE m_PresentThread;
> +    BOOLEAN m_bActive;
>  };
>  
>  class QxlDod {

Frediano


More information about the Spice-devel mailing list