On Thursday 19 April 2007 14:10:45 David Lee wrote:
> On Thu, 19 Apr 2007, Bernd Schubert wrote:
> > while looking into the source due to our recent problems, I see there
> > are several macros, which could be replaced by static (inline)
> > functions, e.g. in GSource.c.
> >
> > Is there a reason to use macros? Do you mind if I convert this into
> > functions?
>
> My reply here isn't either yes or no. It is simply recollection and
> observation from about a year ago. See:
>
> http://cvs.linux-ha.org/viewcvs/viewcvs.cgi/linux-ha/lib/clplumbing/GSource
>.c?r1=1.79&r2=1.80
>
> There are some "longclock_t" things in there which benefitted from being
> wrapped up as (abstracted into) functions, because of alignment
> requirements on some architectures (e.g. sparc) which were being adversely
> affected by some suboptimal casting that happened before that (beneficial)
> function abstration.
>
> In short, there are places in GSource.c that are sensitive to
> architectural issues for this portable "heartbeat" package. Perhaps you
> could describe a little more your intentions (what functions, a sample
> rewrite (even if only pseudo-code) etc.).
>
Attached is a quick shot, even didn't test if it compiles...
>
>
> Incidentally, when you say "static (inline) functions", what do you mean
> by "inline"? The "static" qualifier, in this context, simply means that
> the function's scope is restricted to that compilation unit and it is not
> linkable from other compilation units. I'm not sure what you mean by
> "inline". Some optimisation levels of some compilers might choose, under
> certain conditions, to put the code of a short function "in line" from the
> places where it is called, rather than separately, thus removing the
> (small) runtime function setup/teardown overhead. But that's an internal
> detail and judgement-call made within an optimiser, not a user-visible
> thing (unless one gets deep in there with "gdb" etc.).
See the patch, I made it "static inline name()", "static name()" would be
fine, too. Depends what the project likes more. Personally I prefer static
inline, since it was a macro previously and inline should give the compiler a
stronger hint to really inline it.
--
Bernd Schubert
Q-Leap Networks GmbH
diff -r cd7a78d5646f -r 7edfdd75b2ee lib/clplumbing/GSource.c
--- a/lib/clplumbing/GSource.c Tue Apr 10 15:09:15 2007 +0200
+++ b/lib/clplumbing/GSource.c Thu Apr 19 14:14:54 2007 +0200
@@ -91,43 +91,45 @@ lc_fetch(char *ptr) {
, __FUNCTION__, (input)->description, ms, mx \
, POINTER_TO_ULONG(input))
-#define CHECK_DISPATCH_DELAY(i) { \
- unsigned long ms; \
- longclock_t dettime; \
- dispstart = time_longclock(); \
- dettime = lc_fetch((i)->detecttime); \
- ms = longclockto_ms(sub_longclock(dispstart,dettime)); \
- if ((i)->maxdispatchdelayms > 0 \
- && ms > (i)->maxdispatchdelayms) { \
- WARN_DELAY(ms, (i)->maxdispatchdelayms, (i)); \
- EXPLAINDELAY(dispstart, dettime); \
- } \
-}
-
-#define CHECK_DISPATCH_TIME(i) { \
- unsigned long ms; \
- longclock_t dispend = time_longclock(); \
- ms = longclockto_ms(sub_longclock(dispend, dispstart)); \
- if ((i)->maxdispatchms > 0 && ms > (i)->maxdispatchms) { \
- WARN_TOOLONG(ms, (i)->maxdispatchms, (i)); \
- } \
- lc_store(((i)->detecttime), zero_longclock); \
+static inline void CHECK_DISPATCH_DELAY(GFDSource *i, longclock_t dispstart)
+{
+ unsigned long ms;
+ longclock_t dettime;
+ dettime = lc_fetch(i->detecttime);
+ ms = longclockto_ms(sub_longclock(dispstart, dettime));
+
+ if (i->maxdispatchdelayms > 0
+ && ms > i->maxdispatchdelayms) {
+ WARN_DELAY(ms, i->maxdispatchdelayms, i);
+ EXPLAINDELAY(dispstart, dettime);
+ }
+}
+
+static inline void CHECK_DISPATCH_TIME(GFDSource *i, longclock_t dispstart)
+{
+ longclock_t dispend = time_longclock();
+ unsigned long ms = longclockto_ms(sub_longclock(dispend, dispstart));
+
+ if (i->maxdispatchms > 0 && ms > i->maxdispatchms) {
+ WARN_TOOLONG(ms, i->maxdispatchms, i);
+ }
+ lc_store(i->detecttime, zero_longclock);
}
#define WARN_TOOMUCH(ms, mx, input) cl_log(LOG_WARNING \
, "%s: working on %s took %ld ms (> %ld ms)" \
, __FUNCTION__, (input)->description, ms, mx);
-#define SAVESTART {funstart = time_longclock();}
-
-#define CHECKEND(input) { \
- longclock_t funend = time_longclock(); \
- long ms; \
- ms = longclockto_ms(sub_longclock(funend, funstart)); \
- if (ms > OTHER_MAXDELAY){ \
- WARN_TOOMUCH(ms, ((long) OTHER_MAXDELAY), input); \
- } \
-} \
+static inline void CHECKEND(GCHSource* input, longclock_t funstart)
+{
+ longclock_t funend = time_longclock();
+ long ms;
+ ms = longclockto_ms(sub_longclock(funend, funstart));
+
+ if (ms > OTHER_MAXDELAY){
+ WARN_TOOMUCH(ms, ((long) OTHER_MAXDELAY), input);
+ }
+}
#ifndef _NSIG
@@ -285,9 +287,9 @@ G_fd_dispatch(GSource* source,
{
GFDSource* fdp = (GFDSource*)source;
- longclock_t dispstart;
+ longclock_t dispstart = time_longclock()
g_assert(IS_FDSOURCE(fdp));
- CHECK_DISPATCH_DELAY(fdp);
+ CHECK_DISPATCH_DELAY(fdp, dispstart);
/*
@@ -304,10 +306,10 @@ G_fd_dispatch(GSource* source,
if(!(fdp->dispatch(fdp->gpfd.fd, fdp->udata))){
g_source_remove_poll(source,&fdp->gpfd);
g_source_unref(source);
- CHECK_DISPATCH_TIME(fdp);
+ CHECK_DISPATCH_TIME(fdp, dispstart);
return FALSE;
}
- CHECK_DISPATCH_TIME(fdp);
+ CHECK_DISPATCH_TIME(fdp, dispstart);
}
return TRUE;
@@ -491,7 +493,7 @@ G_CH_prepare_int(GSource* source,
gint* timeout)
{
GCHSource* chp = (GCHSource*)source;
- longclock_t funstart;
+ longclock_t funstart = time_longclock();
gboolean ret;
g_assert(IS_CHSOURCE(chp));
@@ -538,7 +540,7 @@ G_CH_check_int(GSource* source)
GCHSource* chp = (GCHSource*)source;
gboolean ret;
- longclock_t funstart;
+ longclock_t funstart = time_longclock();
g_assert(IS_CHSOURCE(chp));
SAVESTART;
@@ -569,11 +571,11 @@ G_CH_dispatch_int(GSource * source,
gpointer user_data)
{
GCHSource* chp = (GCHSource*)source;
- longclock_t dispstart;
+ longclock_t dispstart = time_longclock();
longclock_t resume_start = zero_longclock;
g_assert(IS_CHSOURCE(chp));
- CHECK_DISPATCH_DELAY(chp);
+ CHECK_DISPATCH_DELAY(chp, dispstart);
if (chp->dontread){
@@ -619,12 +621,12 @@ G_CH_dispatch_int(GSource * source,
if (!chp->fd_fdx) {
g_source_remove_poll(source, &chp->outfd);
}
- CHECK_DISPATCH_TIME(chp);
+ CHECK_DISPATCH_TIME(chp, dispstart);
g_source_unref(source);
return FALSE;
}
}
- CHECK_DISPATCH_TIME(chp);
+ CHECK_DISPATCH_TIME(chp, dispstart);
if (chp->ch->ch_status == IPC_DISCONNECT){
return FALSE;
@@ -788,10 +790,10 @@ G_WC_dispatch(GSource* source,
IPC_Channel* ch;
gboolean rc = TRUE;
int count = 0;
- longclock_t dispstart;
+ longclock_t dispstart = time_longclock();
g_assert(IS_WCSOURCE(wcp));
- CHECK_DISPATCH_DELAY(wcp);
+ CHECK_DISPATCH_DELAY(wcp, dispstart);
while(1) {
ch = wcp->wch->ops->accept_connection(wcp->wch, wcp->auth_info);
@@ -811,7 +813,7 @@ G_WC_dispatch(GSource* source,
break;
}
}
- CHECK_DISPATCH_TIME(wcp);
+ CHECK_DISPATCH_TIME(wcp, dispstart);
return rc;
}
@@ -1036,10 +1038,10 @@ G_SIG_dispatch(GSource * source,
gpointer user_data)
{
GSIGSource* sig_src = (GSIGSource*)source;
- longclock_t dispstart;
+ longclock_t dispstart = time_longclock();
g_assert(IS_SIGSOURCE(sig_src));
- CHECK_DISPATCH_DELAY(sig_src);
+ CHECK_DISPATCH_DELAY(sig_src, dispstart);
sig_src->sh_detecttime = 0UL;
sig_src->signal_triggered = FALSE;
@@ -1047,11 +1049,11 @@ G_SIG_dispatch(GSource * source,
if(sig_src->dispatch) {
if(!(sig_src->dispatch(sig_src->signal, sig_src->udata))){
G_main_del_SignalHandler(sig_src);
- CHECK_DISPATCH_TIME(sig_src);
+ CHECK_DISPATCH_TIME(sig_src, dispstart);
return FALSE;
}
}
- CHECK_DISPATCH_TIME(sig_src);
+ CHECK_DISPATCH_TIME(sig_src, dispstart);
return TRUE;
}
@@ -1338,20 +1340,20 @@ G_TRIG_dispatch(GSource * source,
gpointer user_data)
{
GTRIGSource* trig_src = (GTRIGSource*)source;
- longclock_t dispstart;
+ longclock_t dispstart = time_longclock();
g_assert(IS_TRIGSOURCE(trig_src));
- CHECK_DISPATCH_DELAY(trig_src);
+ CHECK_DISPATCH_DELAY(trig_src, dispstart);
trig_src->manual_trigger = FALSE;
if(trig_src->dispatch) {
if(!(trig_src->dispatch(trig_src->udata))){
G_main_del_TriggerHandler(trig_src);
- CHECK_DISPATCH_TIME(trig_src);
+ CHECK_DISPATCH_TIME(trig_src, dispstart);
return FALSE;
}
- CHECK_DISPATCH_TIME(trig_src);
+ CHECK_DISPATCH_TIME(trig_src, dispstart);
}
lc_store((trig_src->detecttime), zero_longclock);
@@ -1515,12 +1517,12 @@ Gmain_timeout_dispatch(GSource* src, GSo
Gmain_timeout_dispatch(GSource* src, GSourceFunc func, gpointer user_data)
{
struct GTimeoutAppend* append = GTIMEOUT(src);
- longclock_t dispstart;
+ longclock_t dispstart = time_longclock();
gboolean ret;
g_assert(IS_TIMEOUTSRC(append));
lc_store(append->detecttime, append->nexttime);
- CHECK_DISPATCH_DELAY(append);
+ CHECK_DISPATCH_DELAY(append, dispstart);
/* Schedule our next dispatch */
@@ -1530,7 +1532,7 @@ Gmain_timeout_dispatch(GSource* src, GSo
/* Then call the user function */
ret = func(user_data);
- CHECK_DISPATCH_TIME(append);
+ CHECK_DISPATCH_TIME(append, dispstart);
return ret;
}
void
_______________________________________________________
Linux-HA-Dev: Linux-HA-Dev@lists.linux-ha.org
http://lists.linux-ha.org/mailman/listinfo/linux-ha-dev
Home Page: http://linux-ha.org/
