[PATCH 0/9] per-device idle counters

[James Jones]

On 3/14/12 5:20 PM, Peter Hutterer wrote:
> On Wed, Mar 14, 2012 at 05:11:04PM -0700, James Jones wrote:
>> I like the cleanups here.  I prefer the SysCounterGetPrivate() helper over
>> the macro, but don't particularly think the need to allow
>> SyncNumSystemCounters to go negative by decrementing in ResetProc() is a
>> win for clarity.
> 
> sorry, I'm having troubles parsing this sentence. So you'd prefer leaving
> SyncNumSystemCounters out decrement it automatically even after the
> container array has been removed already?
> 
> or force-reset it to 0 and only decrement while the list is still present,
> leaving the counter at 0?  (which is what the first diff below adds)

Sorry for not being clear.  I would prefer not force-resetting it to 0, aka, leaving 1/9 out of the series.  You could check SyncCounterList != NULL instead of SyncNumSystemCounters in the 9/9.  If the inconsistency needs to be fixed, I slightly prefer Jamey's linked-list patch.

The inconsistency between the list and count would even be useful if there was somewhere to assert/BUG_WARN that the number settles to 0.  I suppose you could add a BUG_WARN_MSG(SystemCounterList || (SyncNumSystemCounters == 0)) in SyncCreateSystemCounter() to catch it on server generations.

Thanks,
-James

> Cheers,
>    Peter
> 
>> On 3/14/12 4:30 PM, Peter Hutterer wrote:
>>> On Wed, Mar 14, 2012 at 09:01:13AM -0700, Jamey Sharp wrote:
>>>> This series seems like a good idea to me! I have a few review comments though:
>>>>
>>>> Patch 1 seems strange. Shouldn't the counters themselves get freed at
>>>> reset? If they are, shouldn't that lead to the number of counters
>>>> reaching 0?
>>>
>>> you're right but the order is a tad weird. The ResetProc is called before
>>> the counters are cleaned up, so we free the container list but the number is
>>> still correct. Later, the number goes down to 0 when the counters are
>>> actually freed. Technically correct, but weird. I think the diff below would
>>> make sense to merge into patch 1/9. This way, we remove all knowledge of
>>> system counters in the ResetProc and don't get any weird effects.
>>>
>>> diff --git a/Xext/sync.c b/Xext/sync.c
>>> index 6c8c564..bf47c93 100644
>>> --- a/Xext/sync.c
>>> +++ b/Xext/sync.c
>>> @@ -1200,8 +1200,8 @@ FreeCounter(void *env, XID id)
>>>                       SysCounterList[i] = SysCounterList[i+1];
>>>                   }
>>>               }
>>> +            SyncNumSystemCounters--;
>>>           }
>>> -       SyncNumSystemCounters--;
>>>        }
>>>        free(pCounter);
>>>        return Success;
>>>
>>>
>>>> I guess the input ABI should get bumped for the patch that stores
>>>> per-device idle times. We decided to bump ABI more aggressively,
>>>> right?
>>>
>>> correct, but I need to go through my lists to check whether I've got
>>> anything else that bumps the ABI in the near future. If so, I prefer
>>> bundling them with one ABI bump.
>>>
>>>> I haven't checked: is it possible to build xserver without SYNC, or to
>>>> disable it at runtime? If so, the final patch needs some conditionals,
>>>> I assume.
>>>
>>> in configure.ac:1312, it's always defined, so no conditionals are necessary.
>>> AC_DEFINE(XSYNC, 1, [Support XSync extension])
>>>
>>>> Here's a request you can ignore if you like: Please consider making
>>>> SYSCOUNTERPRIV a static inline instead of a macro, and removing the
>>>> cast from inside it. That'd be different from the usual idiom in the X
>>>> server, but the usual idiom is dumb. :-) I'd prefer to see all the
>>>> callers assign their "pointer pCounter"s to an appropriately-typed
>>>> local once right at the beginning of the function, which also serves
>>>> as documentation for what type you're supposed to pass in there.
>>>
>>> How does this one look then? I'd squash this in but it's easier to review as
>>> a diff on top.
>>>
>>>   From cc02b58e95822a50658ef7fe13e862c3f569ee22 Mon Sep 17 00:00:00 2001
>>> From: Peter Hutterer<peter.hutterer at who-t.net>
>>> Date: Thu, 15 Mar 2012 09:27:24 +1000
>>> Subject: [PATCH] Xext: replace the macro with a static inline function.
>>>
>>> Signed-off-by: Peter Hutterer<peter.hutterer at who-t.net>
>>> ---
>>>    Xext/sync.c    |   24 ++++++++++++++++++------
>>>    Xext/syncsrv.h |    2 --
>>>    2 files changed, 18 insertions(+), 8 deletions(-)
>>>
>>> diff --git a/Xext/sync.c b/Xext/sync.c
>>> index 6c8c564..5f74dab 100644
>>> --- a/Xext/sync.c
>>> +++ b/Xext/sync.c
>>> @@ -115,6 +115,15 @@ static void SyncInitServerTime(void);
>>>
>>>    static void SyncInitIdleTime(void);
>>>
>>> +static inline void*
>>> +SysCounterGetPrivate(SyncCounter *counter)
>>> +{
>>> +    BUG_WARN(!IsSystemCounter(counter));
>>> +
>>> +    return counter->pSysCounterInfo ? counter->pSysCounterInfo->private : NULL;
>>> +}
>>> +
>>> +
>>>    static Bool
>>>    SyncCheckWarnIsCounter(const SyncObject* pSync, const char *warning)
>>>    {
>>> @@ -2747,7 +2756,8 @@ IdleTimeQueryValue (pointer pCounter, CARD64 *pValue_return)
>>>        CARD32 idle;
>>>
>>>        if (pCounter) {
>>> -        IdleCounterPriv *priv = SYSCOUNTERPRIV(pCounter);
>>> +        SyncCounter *counter = pCounter;
>>> +        IdleCounterPriv *priv = SysCounterGetPrivate(counter);
>>>            deviceid = priv->deviceid;
>>>        } else
>>>            deviceid = XIAllDevices;
>>> @@ -2758,8 +2768,8 @@ IdleTimeQueryValue (pointer pCounter, CARD64 *pValue_return)
>>>    static void
>>>    IdleTimeBlockHandler(pointer pCounter, struct timeval **wt, pointer LastSelectMask)
>>>    {
>>> -    IdleCounterPriv *priv = SYSCOUNTERPRIV(pCounter);
>>>        SyncCounter *counter = pCounter;
>>> +    IdleCounterPriv *priv = SysCounterGetPrivate(counter);
>>>        XSyncValue *less = priv->value_less,
>>>    	       *greater = priv->value_greater;
>>>        XSyncValue idle, old_idle;
>>> @@ -2835,7 +2845,8 @@ IdleTimeBlockHandler(pointer pCounter, struct timeval **wt, pointer LastSelectMa
>>>    static void
>>>    IdleTimeWakeupHandler (pointer pCounter, int rc, pointer LastSelectMask)
>>>    {
>>> -    IdleCounterPriv *priv = SYSCOUNTERPRIV(pCounter);
>>> +    SyncCounter *counter = pCounter;
>>> +    IdleCounterPriv *priv = SysCounterGetPrivate(counter);
>>>        XSyncValue *less = priv->value_less,
>>>    	       *greater = priv->value_greater;
>>>        XSyncValue idle;
>>> @@ -2856,7 +2867,8 @@ static void
>>>    IdleTimeBracketValues (pointer pCounter, CARD64 *pbracket_less,
>>>                           CARD64 *pbracket_greater)
>>>    {
>>> -    IdleCounterPriv *priv = SYSCOUNTERPRIV(pCounter);
>>> +    SyncCounter *counter = pCounter;
>>> +    IdleCounterPriv *priv = SysCounterGetPrivate(counter);
>>>        XSyncValue *less = priv->value_less,
>>>    	       *greater = priv->value_greater;
>>>        Bool registered = (less || greater);
>>> @@ -2897,7 +2909,7 @@ init_system_idle_counter(const char *name, int deviceid)
>>>        priv->deviceid = deviceid;
>>>        priv->value_less = priv->value_greater = NULL;
>>>
>>> -    SYSCOUNTERPRIV(idle_time_counter) = priv;
>>> +    idle_time_counter->pSysCounterInfo->private = priv;
>>>
>>>        return idle_time_counter;
>>>    }
>>> diff --git a/Xext/syncsrv.h b/Xext/syncsrv.h
>>> index 361b68d..6c0bf74 100644
>>> --- a/Xext/syncsrv.h
>>> +++ b/Xext/syncsrv.h
>>> @@ -71,8 +71,6 @@ typedef void (*SyncSystemCounterBracketValues)(pointer counter,
>>>    					       CARD64 *pbracket_less,
>>>    					       CARD64 *pbracket_greater);
>>>
>>> -#define SYSCOUNTERPRIV(counter) (((SyncCounter*)(counter))->pSysCounterInfo->private)
>>> -
>>>    typedef struct _SysCounterInfo {
>>>        char	*name;
>>>        CARD64	resolution;
>>

nvpublic