[Openais] [PATCH 3/3] Add totem statistics to objdb.

[Steven Dake]

On Mon, 2009-10-12 at 11:24 +1300, angus salkeld wrote:
> On Fri, 2009-10-09 at 10:21 -0700, Steven Dake wrote:
> ...
> > > +
> > > +
> > 
> > Should use api->timer_duration_add if possible.
> > 
> > > +	poll_timer_add (corosync_poll_handle,
> > > +					1500, NULL,
> > > +					corosync_totem_stats_updater,
> > > +					&corosync_stats_timer_handle);
> > > +}
> > > +
> 
> done.
> This timeout could be bigger (5 sec) or even configurable to reduce
> workload.
> 
> > > +static int token_event_stats_collector (enum totem_callback_token_type type, const void *void_instance)
> > > +{
> > > +	struct totemsrp_instance *instance = (struct totemsrp_instance *)void_instance;
> > > +	struct timeval tv;
> > > +	uint32_t time_now;
> > > +
> > 
> > gettimeofday usage in corosync is deprecated for calculating time
> > differences.  take a look at the function timerlist_nano_current_get in
> > tlist.h (included in totemsrp.c).
> > 
> > 
> > > +	if (gettimeofday(&tv, 0)) {
> > > +		return -1;
> > > +	}
> > > +
> > > +	time_now = tv.tv_sec * 1000;
> > > +	time_now += tv.tv_usec / 1000; /* milli seconds */
> > > +
> 
> no problem.
> 
> > >  
> > > +	totemsrp_callback_token_create (instance,
> > > +									&instance->stats->token_recv_event_handle,
> > > +									TOTEM_CALLBACK_TOKEN_RECEIVED,
> > > +									0,
> > > +									token_event_stats_collector,
> > > +									instance);
> > > +	totemsrp_callback_token_create (instance,
> > > +									&instance->stats->token_sent_event_handle,
> > > +									TOTEM_CALLBACK_TOKEN_SENT,
> > > +									0,
> > > +									token_event_stats_collector,
> > > +		
> > This logic is hard to follow and introduces extra locking and
> > performance penalties.  I prefer if you call token_event_stats_collector
> > at start and end of message_handler_orf_token.
> > 
> > 
> Can do, but for interest:
> 1) where is the extra locking?
>    token_callbacks_execute() just goes through a list of callbacks.
>    and token_callbacks_create() is once off at startup.

I was wrong there is no extra penalty associated with using the
mechanism as is.

> 2) I thought you created the mechanism (in part) to cleanup
>    message_handler_orf_token().
>    So there would not be any non-protocol related code littered around.
> 

No the only reason that api exists is to allow new work to be scheduled
once the token has been transmitted or to finish the fragment data. (and
hopefully now there is new empty space in the totem new message queue).

> > can you make all stats 64 bit?
> > 
> done. I have sent a objdb patch to handle it.
> 
> -Angus
> 
> 
> NOTICE: This message contains privileged and confidential
> information intended only for the use of the addressee
> named above. If you are not the intended recipient of
> this message you are hereby notified that you must not
> disseminate, copy or take any action in reliance on it.
> If you have received this message in error please
> notify Allied Telesis Labs Ltd immediately.
> Any views expressed in this message are those of the
> individual sender, except where the sender has the
> authority to issue and specifically states them to
> be the views of Allied Telesis Labs.