[Spice-devel] [PATCH v2 08/12] qxl-wddm-dod: Prepare for failure to allocate memory
Yuri Benditovich
yuri.benditovich at daynix.com
Tue Apr 4 11:16:48 UTC 2017
On Mon, Apr 3, 2017 at 9:12 PM, Frediano Ziglio <fziglio at redhat.com> wrote:
>
> On Mon, Apr 3, 2017 at 1:55 PM, Frediano Ziglio <fziglio at redhat.com>
> wrote:
>
>> >
>> > From: "yuri.benditovich at daynix.com" <yuri.benditovich at daynix.com>
>> >
>> > 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 already received
>> > stop command and waits for thread termination. Note that in case
>> > the VSync control is enabled stop command may happen even after the
>> video
>> > subsystem executes switch to VGA mode. In such case QEMU will not
>> return
>> > previously allocated objects (assuming the QXL driver already disabled
>> > and ignoring callbacks from Spice server in VGA mode).
>> >
>>
>> Just to confirm, by VGA mode you mean when Windows attempt to unload the
>> driver and use VESA/VGA modes, not VgaDevice, right?
>> In this case I assume the memory will be freed when the driver is
>> unloaded by Windows.
>> I would expect the number of commands sent in this case is small.
>> Also why is not possible to just ignore the commands sent by Windows
>> in this case?
>>
>
> When VSync is enabled and the system activates watchdog policy, there are
> 2 possible flows
> related to driver stopping:
> First one is regular PnP stop (for example, on disable), on stop the
> driver waits until thread
> finishes and the thread is able to complete all the operations, in the
> worst case it will take
> some time until thread allocates all the pending drawables and sends them
> down.
> Second one is forced stop due to long processing of presentation callback.
> We do
> our best to avoid this, but this is still possible, at least in theory. In
> this case OS
> may not not wait until the driver completes stop flow and can activate
> fallback
> mode and make switch to VGA mode. If the thread is processing outstanding
> drawables, it needs to allocate memory (forced) and may pend until some
> previously allocated
> objects must be reported by QEMU/spice server to the guest. As the switch
> to VGA
> already happened, QEMU does not report them (assuming the guest does need
> this,
> which is completely correct in case of regular stop). At the moment of VGA
> switch the
> thread can be in the middle of allocation, so the driver shall know to
> abort the allocation if
> stop flow already started.
>
> Wouldn't be helpful to have a flag set on Stop so the thread could start
> deleting drawables directly
> instead of processing them? This should make the close faster.
>
> Yes, this can make the stop faster. I'll prepare such patch over 12/12 as
the driver
anyway shall review the chain of chunks and free the memory it allocated
from OS, in any.
>
> BTW, In earlier notes you mentioned possibility to use VGA memory as
> alternate area for
> objects allocation. I think that in stop flow due to watchdog the VGA area
> may be used by
> fallback video engine as frame buffer and this can corrupt the memory
> arena managed by mspace code.
>
> This should not be an issue. The first part of Bar0 is reserved for the
> frame buffer and excluded
> mspace. VGA should use only this part. But better to test it.
>
>
>> > In case of forced memory allocation the allocation routine waits
>> > unpredictable time until the request is satisfied. In current commit
>> > we do not acquire m_MemLock mutex for all this time, but release it
>> > when entering long wait to allow another caller to try allocating
>> memory.
>> >
>>
>> Looking at final code AllocMem can return NULL in all cases depending
>> on the state of the driver however not all paths check if this function
>> returns NULL. Specifically InitMonitorConfig and SetClip.
>> InitMonitorConfig is called just after initializing memory so is
>> unlikely to fail and SetClip is always called with clip == NULL however
>> I don't like to have unexploded bombs in the code.
>>
>> > Signed-off-by: Yuri Benditovich <yuri.benditovich at daynix.com>
>> > ---
>> > qxldod/QxlDod.cpp | 86
>> > +++++++++++++++++++++++++++++++++++++++++++++++++------
>> > qxldod/QxlDod.h | 3 +-
>> > 2 files changed, 79 insertions(+), 10 deletions(-)
>> >
>> > diff --git a/qxldod/QxlDod.cpp b/qxldod/QxlDod.cpp
>> > index aa70f39..f8440d4 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;
>> > }
>> >
>> > QxlDevice::~QxlDevice(void)
>> > @@ -3490,14 +3491,19 @@ 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;
>> > + }
>> > return Status;
>> > }
>> >
>> > void QxlDevice::QxlClose()
>> > {
>> > PAGED_CODE();
>> > + m_bActive = FALSE;
>> > StopPresentThread();
>> > DestroyMemSlots();
>> > }
>> > @@ -3945,14 +3951,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;
>> > }
>> > @@ -3960,17 +3970,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__));
>> > }
>> >
>> > @@ -4052,7 +4065,16 @@ 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 (force)
>> > + locked = WaitForObject(&m_MemLock, NULL);
>> > + else {
>> > + LARGE_INTEGER doNotWait;
>> > + doNotWait.QuadPart = 0;
>> > + locked = WaitForObject(&m_MemLock, &doNotWait);
>> > + if (!locked) {
>> > + return NULL;
>> > + }
>> > + }
>> >
>> > while (1) {
>> > /* Release lots of queued resources, before allocating, as we
>> > @@ -4070,17 +4092,20 @@ 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;
>> > }
>> > }
>> > +
>> > 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;
>> > @@ -4113,6 +4138,9 @@ 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;
>> > @@ -4128,6 +4156,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;
>> > + }
>> > output->num_res = 0;
>> > RESOURCE_TYPE(output, RESOURCE_TYPE_CURSOR);
>> > cursor_cmd = (QXLCursorCmd *)output->data;
>> > @@ -4277,6 +4308,9 @@ 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;
>> > @@ -4420,6 +4454,11 @@ QXLDrawable *QxlDevice::PrepareBltBits (
>> > 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) {
>> > + ReleaseOutput(drawable->release_info.id);
>> > + DbgPrint(TRACE_LEVEL_ERROR, ("Cannot allocate bitmap for
>> > drawable\n"));
>> > + return NULL;
>> > + }
>> >
>> > image_res->refs = 1;
>> > image_res->free = FreeBitmapImageEx;
>> > @@ -4450,8 +4489,12 @@ QXLDrawable *QxlDevice::PrepareBltBits (
>> > 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)) {
>> > + ReleaseOutput(drawable->release_info.id);
>> > + DbgPrint(TRACE_LEVEL_ERROR, ("Cannot allocate additional
>> bitmap
>> > for drawable\n"));
>> > + return NULL;
>> > + }
>> > }
>> >
>> > internal->image.bitmap.palette = 0;
>> > @@ -4472,7 +4515,7 @@ QXLDrawable *QxlDevice::PrepareBltBits (
>> > 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)
>> > {
>> > @@ -4490,6 +4533,9 @@ 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;
>> > @@ -4510,6 +4556,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)
>> > @@ -4573,6 +4620,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;
>> > @@ -4580,6 +4632,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__));
>> > + ReleaseOutput(cursor_cmd->release_info.id);
>> > + return STATUS_INSUFFICIENT_RESOURCES;
>> > + }
>> > +
>> > res->refs = 1;
>> > res->free = FreeCursorEx;
>> > res->ptr = this;
>> > @@ -4616,8 +4674,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__));
>> > + ReleaseOutput(cursor_cmd->release_info.id);
>> > + return STATUS_INSUFFICIENT_RESOURCES;
>> > + }
>> > }
>> > CursorCmdAddRes(cursor_cmd, res);
>> > RELEASE_RES(res);
>> > @@ -4638,6 +4701,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 a1e9634..cbcd11e 100755
>> > --- a/qxldod/QxlDod.h
>> > +++ b/qxldod/QxlDod.h
>> > @@ -601,7 +601,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 +671,7 @@ private:
>> >
>> > QXLPresentOnlyRing m_PresentRing[1];
>> > HANDLE m_PresentThread;
>> > + BOOLEAN m_bActive;
>> > };
>> >
>> > class QxlDod {
>>
>
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.freedesktop.org/archives/spice-devel/attachments/20170404/9483eef9/attachment-0001.html>
More information about the Spice-devel
mailing list