[Spice-devel] leaking drawables

[Gerd Hoffmann]

On 09/08/10 21:24, Alexander Larsson wrote:
> On Tue, 2010-09-07 at 17:08 +0200, Gerd Hoffmann wrote:
>> Hi,
>>
>>> This happens for several consecutive resource releases. Looking at what
>>> actually gets released by the driver we see that the resources are freed
>>> in the same order as they were release, its just that a chunk of
>>> resources are missing here and there.
>>
>> Try the attached patch.  I'm pretty sure qxl just overruns the release
>> ring, making complete release lists disappear now and then.
>
> This doesn't seem right. The ring doesn't wrap cons and prod, as they
> are incremented. Instead they are % num_items when used as indexes.

Yes.

> This
> means that its ok to have the ring be completely full (and indeed, the
> IS_FULL macro just checks prod - cons == num_items).

Yes.

There is one special thing in the release ring usage though.  As you 
know qxl builds up a linked lists there, where the heads are in the 
ring.  qxl does also store the head of the list which it currently 
builds into the ring, but doesn't bump prod (yet) so the guest will not 
grab it.  Which needs one extra ring entry.  Because of that the IS_FULL 
macro simply doesn't work for the release ring and the check is open coded.

So, the old code checks whenever prod - cons + 1 != num_items.  Lets 
assume the guest didn't take out any entries yet, so cons is 0.  When 
prod is 7 the check fails and qxl will stop pushing stuff to the ring 
due to being full.  Lets have a look at the prod == 6 case:

   * The extra entry (ring[7%8]) holds the head for the list qxl
     is building).
   * qxl pushes that ring item to the guest (SPICE_RING_PUSH),
     prod is 7 now.
   * qxl gets a pointer to the next entry (SPICE_RING_PROD_ITEM),
     which is ring[8%8].
   * qxl zero-initializes the ring entry as new list head, thereby
     overriding ring[0], which isn't yet consumed by the guest.

Correct?

> And, testing with this patch I still see the leak.

Hmm.  More bugs?

cheers,
   Gerd