[PATCH] EXA: Defragment offscreen memory.

[Adam Jackson]

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; it'll fire once at 100ms of idle, and then never again
(until the next loop through Wakeup/Block).

> +static void
> +ExaBlockHandler(int screenNum, pointer blockData, pointer pTimeout,
> +		pointer pReadmask)
> +{
> +    ScreenPtr pScreen = screenInfo.screens[screenNum];
> +    ExaScreenPriv(pScreen);
> +
> +    unwrap(pExaScr, pScreen, BlockHandler);
> +    (*pScreen->BlockHandler) (screenNum, blockData, pTimeout, pReadmask);
> +    wrap(pExaScr, pScreen, BlockHandler, ExaBlockHandler);
> +
> +    pExaScr->defragTimer = TimerSet(pExaScr->defragTimer, 0, 100,
> +				    ExaDefragTimerCallback, pScreen);
> +}
> +
> +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.

> +_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.  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.

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

- ajax
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 197 bytes
Desc: This is a digitally signed message part
Url : http://lists.x.org/archives/xorg-devel/attachments/20090304/a9cf3cc8/attachment.pgp