[PATCH 0/9] per-device idle counters

[James Jones]

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.

Thanks,
-James

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