[Openais] [PATCH 2/3] Add per service-function statistics.

[Steven Dake]

Comments inline

On Thu, 2009-09-24 at 14:02 +1200, angus salkeld wrote:
> Hi
> 
> This is minor change to change the name of the service objects
> from:
> runtime.services.8.fn_0_stats.tx=1
> runtime.services.8.fn_0_stats.rx=1
> to
> runtime.services.cpg.0.tx=1
> runtime.services.cpg.0.rx=1
> 
> -Angus
> 
> Signed-off-by: Angus Salkeld <angus.salkeld at alliedtelesis.co.nz>
> ---
>  exec/main.c    |   17 +++++++++++++++++
>  exec/service.c |   56 +++++++++++++++++++++++++++++++++++++++++++++++++++++++-
>  2 files changed, 72 insertions(+), 1 deletions(-)
> 
> diff --git a/exec/main.c b/exec/main.c
> index 82ab44d..c4ea700 100644
> --- a/exec/main.c
> +++ b/exec/main.c
> @@ -463,6 +463,9 @@ static void deliver_fn (
>  			((void *)msg);
>  	}
>  
> +	objdb->object_key_increment (ais_service[service]->exec_engine[fn_id].stats_handle,
> +								 "rx", strlen("rx"),
> +								 &key_incr_dummy);

I see how you use stats_handle now.  Unfortunately some other method
will have to be used because this one breaks binary compatibility with
existing lcrsos.

I recommend a global array something like
global_stats_handle[64][64] where X = service and Y = fn_id.


>  
>  	ais_service[service]->exec_engine[fn_id].exec_handler_fn
>  		(msg, nodeid);
> @@ -481,6 +484,20 @@ int main_mcast (
>          unsigned int iov_len,
>          unsigned int guarantee)
>  {
> +	const coroipc_request_header_t *req = iovec->iov_base;
> +	int service;
> +	int fn_id;
> +	unsigned int key_incr_dummy;
> +
> +	service = req->id >> 16;
> +	fn_id = req->id & 0xffff;
> +
> +	if (ais_service[service]) {
> +		objdb->object_key_increment (ais_service[service]->exec_engine[fn_id].stats_handle,
> +									 "tx", strlen("tx"),
> +									 &key_incr_dummy);

The tabs here are wacky.  Please use 1 tabstop increase for new lines
for an existing construct.

> +	}
> +
>  	return (totempg_groups_mcast_joined (corosync_group_handle, iovec, iov_len, guarantee));
>  }
>  
> diff --git a/exec/service.c b/exec/service.c
> index b8933de..796a0a2 100644
> --- a/exec/service.c
> +++ b/exec/service.c
> @@ -88,7 +88,7 @@ static struct default_service default_services[] = {
>  struct corosync_service_engine *ais_service[SERVICE_HANDLER_MAXIMUM_COUNT];
>  
>  static hdb_handle_t object_internal_configuration_handle;
> -
> +static hdb_handle_t object_stats_services_handle;
>  
>  static unsigned int default_services_requested (struct corosync_api_v1 *corosync_api)
>  {
> @@ -137,6 +137,11 @@ unsigned int corosync_service_link_and_init (
>  	struct corosync_service_engine *service;
>  	unsigned int res;
>  	hdb_handle_t object_service_handle;
> +	hdb_handle_t object_stats_handle;
> +	int fn;
> +	char object_name[32];
> +	char *name_sufix;
> +	int zero_32 = 0;
>  
>  	/*
>  	 * reference the service interface
> @@ -199,6 +204,39 @@ unsigned int corosync_service_link_and_init (
>  		&service->id,
>  		sizeof (service->id), OBJDB_VALUETYPE_UINT16);
>  
> +	name_sufix = strrchr (service_name, '_');
> +	if (name_sufix)
> +		name_sufix++;
> +	else
> +		name_sufix = (char*)service_name;
> +
> +	corosync_api->object_create (object_stats_services_handle,
> +								 &object_stats_handle,
> +								 name_sufix, strlen (name_sufix));
> +
> +	corosync_api->object_key_create_ext (object_stats_handle,
> +										 "service_id",
> +										 &service->id, sizeof (service->id),
> +										 OBJDB_VALUETYPE_INT16);
> +
> +	for (fn = 0; fn < service->exec_engine_count; fn++) {
> +
> +		snprintf (object_name, 32, "%d", fn);
> +		corosync_api->object_create (object_stats_handle,
> +									 &service->exec_engine[fn].stats_handle,
> +									 object_name, strlen (object_name));
> +
> +		corosync_api->object_key_create_ext (service->exec_engine[fn].stats_handle,
> +											 "tx",
> +											 &zero_32, sizeof (int),
> +											 OBJDB_VALUETYPE_UINT32);
> +
> +		corosync_api->object_key_create_ext (service->exec_engine[fn].stats_handle,
> +											 "rx",
> +											 &zero_32, sizeof (int),
> +											 OBJDB_VALUETYPE_UINT32);
> +	}
> +
>  	log_printf (LOGSYS_LEVEL_NOTICE, "Service initialized '%s'\n", service->name);
>  	return (res);
>  }
> @@ -373,7 +411,23 @@ unsigned int corosync_service_defaults_link_and_init (struct corosync_api_v1 *co
>  	char *found_service_ver;
>  	unsigned int found_service_ver_atoi;
>  	hdb_handle_t object_find_handle;
> +	hdb_handle_t object_find2_handle;
> +	hdb_handle_t object_runtime_handle;
>  
> +	corosync_api->object_find_create (
> +		OBJECT_PARENT_HANDLE,
> +		"runtime",
> +		strlen ("runtime"),
> +		&object_find2_handle);
> +
> +	if (corosync_api->object_find_next (
> +			object_find2_handle,
> +			&object_runtime_handle) == 0) {
> +
> +		corosync_api->object_create (object_runtime_handle,
> +									 &object_stats_services_handle,
> +									 "services", strlen ("services"));
> +	}
>  	corosync_api->object_create (OBJECT_PARENT_HANDLE,
>  		&object_internal_configuration_handle,
>  		"internal_configuration",

Service engine unload isn't handled by this patch.