[PATCH 0/9] per-device idle counters

[Peter Hutterer]

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)

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