[PATCH] EXA: Defragment offscreen memory.

[Michel Dänzer]

On Wed, 2009-03-04 at 11:06 -0500, Adam Jackson wrote:
> On Tue, 2009-02-24 at 10:41 +0100, Michel Dänzer wrote:
> 
> > That said, I would have expected it to get called every 100ms when the server
> > is idle, but that isn't the case, so maybe I'm missing something and was just
> > lucky it didn't blow up for me. I'd like to get confirmation from Adam that the
> > BlockHandler/WakeupHandler/Timer idea makes sense.
> 
> I think it's just bugs:
> 
> > +static CARD32 ExaDefragTimerCallback(OsTimerPtr pTimer, CARD32 time,
> > +				     pointer arg)
> > +{
> > +    /* Try and keep the offscreen memory area tidy every now and then when the
> > +     * server has been idle for at least 100ms.
> > +     */
> > +    ExaOffscreenDefragment(arg);
> > +
> > +    return 0;
> > +}
> 
> The return value from the timer callback is the interval (in millis)
> until it fires again.  Since you return 0 here, you effectively make
> this a one-shot;

Yes, that's the intention.

> it'll fire once at 100ms of idle, and then never again (until the next
> loop through Wakeup/Block).

I'm trying to re-arm the timer from BlockHandler, but now I think this
doesn't work because WaitForSomething() calculates the select timeout
before calling BlockHandler(). Is this a bug that should be fixed, or is
TimerSet in the BlockHandler just no-no?


> > +static void
> > +ExaWakeupHandler(int screenNum, pointer wakeupData, unsigned long result,
> > +		 pointer pReadmask)
> > +{
> > +    ScreenPtr pScreen = screenInfo.screens[screenNum];
> > +    ExaScreenPriv(pScreen);
> > +
> > +    /* Prevent the timer callback from being called if Select() didn't time out.
> > +     */
> > +    if (result != 0)
> > +	TimerCancel(pExaScr->defragTimer);
> 
> You don't need to do this, I don't think.  DoTimer() will skip you if
> your timer hasn't fired yet; and the TimerSet in BlockHandler will reset
> the expiry to another 100ms in the future.

The idea was to prevent the callback from getting called if the server
was busy for 100ms. Can that never happen?


> > +_X_HIDDEN ExaOffscreenArea*
> > +ExaOffscreenDefragment (ScreenPtr pScreen)
> > +{
> > +    ExaScreenPriv (pScreen);
> > +    ExaOffscreenArea *area, *largest_available = NULL;
> > +    int largest_size = 0;
> > +    PixmapPtr pDstPix;
> > +    ExaPixmapPrivPtr pExaDstPix;
> > +    CARD32 now = GetTimeInMillis();
> > +
> > +    /* Don't defragment more than once per second, to avoid adding more overhead
> > +     * than we're trying to prevent
> > +     */
> > +    if (abs((INT32) (now - pExaScr->lastDefragment)) < 1000)
> > +	return NULL;
> 
> This is sort of unpleasant.  We'll wake up 10 times a second because the
> timer will fire, but we'll only do work once a second, so that's wasting
> power.

FWIW the test above is also to prevent exaOffscreenAlloc() from
triggering more than one defragmentation pass per second.

> But defrag as you've got it is fairly expensive, since it's a full
> walk through the whole arena.  Might be better to split this into
> ExaOffscreenDefragmentIncremental to move just one pixmap, call that
> from the timer, and leave the full-force version for when allocation
> would otherwise fail.

The partial defragment would still need to walk (almost) everything in
the worst case. It shouldn't be that expensive anyway.


> Only other comment is that you don't have any way of marking the screen
> as completely defragmented, which means you'll _always_ wake up 10 times
> a second.  Or, you would, if you didn't return 0 from the timer
> callback.  Should probably record whether defrag did work, and avoid
> arming the timer when you're defragged.  (You'll also need to catch
> pixmap destruction when defragged, and use that to re-arm the timer.)

Good point, I'll add something for that.


-- 
Earthling Michel Dänzer           |                http://www.vmware.com
Libre software enthusiast         |          Debian, X and DRI developer