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.hutte...@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.hutte...@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 _______________________________________________ xorg-devel@lists.x.org: X.Org development Archives: http://lists.x.org/archives/xorg-devel Info: http://lists.x.org/mailman/listinfo/xorg-devel