Glad to see we are finally coming to the end of the rabbit hole. Really good work! Patch 4/4 needs some rework but the rest look pretty good. 3/4 has a minor nit. Comments inline.
On Wed, 2010-01-20 at 17:53 +0100, Jan Friesse wrote: > Attached is previous patch, split to 4 parts and better tested. > > Regarding Chrissie comments: > - about ABI compatibility. I'm pretty sure, that in initialization, is > OK to have item anywhere. Definition is of course augmented on the end. yup this is ok > - schedwrk_create_lock - This was really bad name. I hope > schedwrk_internal_create is better. > > Regards, > Honza > plain text document attachment > (0001-poll_dispatch_delete-ability-to-return-0.patch) > From e9cb38c27919ed6c8d02286bfa4cca9becf290e4 Mon Sep 17 00:00:00 2001 > From: Jan Friesse <[email protected]> > Date: Wed, 20 Jan 2010 14:53:54 +0100 > Subject: [PATCH 1/4] poll_dispatch_delete - ability to return 0 > > Patch fixes poll_dispatch_delete, so it is able to return 0 > (success), when requested FD was found. > --- > trunk/exec/coropoll.c | 2 ++ > 1 files changed, 2 insertions(+), 0 deletions(-) > > diff --git a/trunk/exec/coropoll.c b/trunk/exec/coropoll.c > index 3c8df8d..8f18f0f 100644 > --- a/trunk/exec/coropoll.c > +++ b/trunk/exec/coropoll.c > @@ -260,6 +260,8 @@ int poll_dispatch_delete ( > poll_instance->ufds[i].fd = -1; > poll_instance->poll_entries[i].ufd.fd = -1; > poll_instance->poll_entries[i].ufd.revents = 0; > + > + res = 0; > break; > } > } patch 1/4 looks good for commit > plain text document attachment > (0002-Pass-correct-poll-handle-to-poll_stop-on-exit.patch) > From 9237249db7a01f773821675ff65c4fe645d0dcd4 Mon Sep 17 00:00:00 2001 > From: Jan Friesse <[email protected]> > Date: Wed, 20 Jan 2010 15:02:26 +0100 > Subject: [PATCH 2/4] Pass correct poll handle to poll_stop on exit > > --- > trunk/exec/main.c | 2 +- > 1 files changed, 1 insertions(+), 1 deletions(-) > > diff --git a/trunk/exec/main.c b/trunk/exec/main.c > index b2b84d2..0aab772 100644 > --- a/trunk/exec/main.c > +++ b/trunk/exec/main.c > @@ -156,7 +156,7 @@ void corosync_state_dump (void) > > static void unlink_all_completed (void) > { > - poll_stop (0); > + poll_stop (corosync_poll_handle); > coroipcs_ipc_exit (); > totempg_finalize (); patch 2/4 looks good for commit > > plain text document attachment > (0003-Add-schedwrk_create_nolock-function.patch) > From 5f375059417f1c5eae47b2ebf37e510dcda17fa1 Mon Sep 17 00:00:00 2001 > From: Jan Friesse <[email protected]> > Date: Wed, 20 Jan 2010 15:18:00 +0100 > Subject: [PATCH 3/4] Add schedwrk_create_nolock function > > This patch adds schedwrk_create_nolock, which will not call > serialize_lock before execution of callback. > --- > trunk/exec/apidef.c | 1 + > trunk/exec/schedwrk.c | 31 > +++++++++++++++++++++++++++---- > trunk/exec/schedwrk.h | 5 +++++ > trunk/include/corosync/engine/coroapi.h | 5 +++++ > 4 files changed, 38 insertions(+), 4 deletions(-) > > diff --git a/trunk/exec/apidef.c b/trunk/exec/apidef.c > index 5e1f178..adb2c88 100644 > --- a/trunk/exec/apidef.c > +++ b/trunk/exec/apidef.c > @@ -129,6 +129,7 @@ static struct corosync_api_v1 apidef_corosync_api_v1 = { > .tpg_groups_reserve = NULL, > .tpg_groups_release = NULL, > .schedwrk_create = schedwrk_create, > + .schedwrk_create_nolock = schedwrk_create_nolock, > .schedwrk_destroy = schedwrk_destroy, > .sync_request = NULL, //sync_request, > .quorum_is_quorate = corosync_quorum_is_quorate, > diff --git a/trunk/exec/schedwrk.c b/trunk/exec/schedwrk.c > index 0c330e9..0591063 100644 > --- a/trunk/exec/schedwrk.c > +++ b/trunk/exec/schedwrk.c > @@ -46,6 +46,7 @@ struct schedwrk_instance { > int (*schedwrk_fn) (const void *); > const void *context; > void *callback_handle; > + int lock; > }; > > union u { > @@ -70,9 +71,13 @@ static int schedwrk_do (enum totem_callback_token_type > type, const void *context > goto error_exit; > } > > - serialize_lock (); > + if (instance->lock) > + serialize_lock (); > + > res = instance->schedwrk_fn (instance->context); > - serialize_unlock (); > + > + if (instance->lock) > + serialize_unlock (); > > if (res == 0) { > hdb_handle_destroy (&schedwrk_instance_database, > hdb_nocheck_convert (handle)); > @@ -93,10 +98,11 @@ void schedwrk_init ( > serialize_unlock = serialize_unlock_fn; > } > > -int schedwrk_create ( > +static int schedwrk_internal_create ( > hdb_handle_t *handle, > int (schedwrk_fn) (const void *), > - const void *context) > + const void *context, > + int lock) > { > struct schedwrk_instance *instance; > int res; > @@ -121,6 +127,7 @@ int schedwrk_create ( > > instance->schedwrk_fn = schedwrk_fn; > instance->context = context; > + instance->lock = lock; > > hdb_handle_put (&schedwrk_instance_database, *handle); > > @@ -133,6 +140,22 @@ error_exit: > return (-1); > } > > +int schedwrk_create ( > + hdb_handle_t *handle, > + int (schedwrk_fn) (const void *), > + const void *context) > +{ > + return schedwrk_internal_create (handle, schedwrk_fn, context, 1); > +} > + > +int schedwrk_create_nolock ( > + hdb_handle_t *handle, > + int (schedwrk_fn) (const void *), > + const void *context) > +{ > + return schedwrk_internal_create (handle, schedwrk_fn, context, 0); > +} > + > void schedwrk_destroy (hdb_handle_t handle) > { > hdb_handle_destroy (&schedwrk_instance_database, handle); > diff --git a/trunk/exec/schedwrk.h b/trunk/exec/schedwrk.h > index e67215b..29d6409 100644 > --- a/trunk/exec/schedwrk.h > +++ b/trunk/exec/schedwrk.h > @@ -43,6 +43,11 @@ int schedwrk_create ( > int (schedwrk_fn) (const void *), > const void *context); > > +int schedwrk_create_nolock ( > + hdb_handle_t *handle, > + int (schedwrk_fn) (const void *), > + const void *context); > + The above definition should be "extern". > void schedwrk_destroy (hdb_handle_t handle); > > #endif /* SCHEDWRK_H_DEFINED */ > diff --git a/trunk/include/corosync/engine/coroapi.h > b/trunk/include/corosync/engine/coroapi.h > index 0c487ca..556a568 100644 > --- a/trunk/include/corosync/engine/coroapi.h > +++ b/trunk/include/corosync/engine/coroapi.h > @@ -627,6 +627,11 @@ struct corosync_api_v1 { > objdb_value_types_t *type); > > void *(*totem_get_stats)(void); > + > + int (*schedwrk_create_nolock) ( > + hdb_handle_t *handle, > + int (schedwrk_fn) (const void *), > + const void *context); > }; > > #define SERVICE_ID_MAKE(a,b) ( ((a)<<16) | (b) ) With the exception of minor nitpick above patch is good for commit > plain text document attachment > (0004-Fix-segfaults-of-corosync-on-exit.patch) > From 08ddd5d5aa5dd447491347b4493d6d237ad9b230 Mon Sep 17 00:00:00 2001 > From: Jan Friesse <[email protected]> > Date: Wed, 20 Jan 2010 17:46:35 +0100 > Subject: [PATCH 4/4] Fix segfaults of corosync on exit > > --- > trunk/exec/coroipcs.c | 36 ++++--------- > trunk/exec/main.c | 3 +- > trunk/exec/service.c | 108 > ++++++++++++++++++++++++++++++------- > trunk/exec/service.h | 5 +- > trunk/include/corosync/coroipcs.h | 2 +- > 5 files changed, 104 insertions(+), 50 deletions(-) > > diff --git a/trunk/exec/coroipcs.c b/trunk/exec/coroipcs.c > index a0ef133..9f63c69 100644 > --- a/trunk/exec/coroipcs.c > +++ b/trunk/exec/coroipcs.c > @@ -75,6 +75,7 @@ > #include <corosync/hdb.h> > #include <corosync/coroipcs.h> > #include <corosync/coroipc_ipc.h> > +#include <corosync/totem/coropoll.h> > > #define LOGSYS_UTILS_ONLY 1 > #include <corosync/engine/logsys.h> > @@ -1043,41 +1044,26 @@ static void _corosync_ipc_init(void) > api->poll_accept_add (server_fd); > } Passing poll_handle into this function is not a suitable solution. Remember coroipcs should be generic. What your looking for instead is api->poll_dispatch_destroy(). The appropriate call needs to be added into main.c around line 1090 since currently api->poll_dispatch_destroy is undefined. > > -void coroipcs_ipc_exit (void) > +void coroipcs_ipc_service_exit (unsigned int service, hdb_handle_t > poll_handle) > { > - struct list_head *list; > + struct list_head *list, *list_next; > struct conn_info *conn_info; > - unsigned int res; > + //unsigned int res; > > for (list = conn_info_list_head.next; list != &conn_info_list_head; > - list = list->next) { > + list = list_next) { > + > + list_next = list->next; > > conn_info = list_entry (list, struct conn_info, list); > > - if (conn_info->state != CONN_STATE_THREAD_ACTIVE) > + if (conn_info->service != service || conn_info->state != > CONN_STATE_THREAD_ACTIVE) > continue; > > ipc_disconnect (conn_info); > - > -#if _POSIX_THREAD_PROCESS_SHARED > 0 > - sem_destroy (&conn_info->control_buffer->sem0); > - sem_destroy (&conn_info->control_buffer->sem1); > - sem_destroy (&conn_info->control_buffer->sem2); > -#else > - semctl (conn_info->semid, 0, IPC_RMID); > -#endif > - > - /* > - * Unmap memory segments > - */ > - res = munmap ((void *)conn_info->control_buffer, > - conn_info->control_size); > - res = munmap ((void *)conn_info->request_buffer, > - conn_info->request_size); > - res = munmap ((void *)conn_info->response_buffer, > - conn_info->response_size); > - res = circular_memory_unmap (conn_info->dispatch_buffer, > - conn_info->dispatch_size); > + poll_dispatch_delete (poll_handle, conn_info->fd); > + while (conn_info_destroy (conn_info) != -1) > + ; > } > } > > diff --git a/trunk/exec/main.c b/trunk/exec/main.c > index 0aab772..0944b00 100644 > --- a/trunk/exec/main.c > +++ b/trunk/exec/main.c > @@ -157,7 +157,6 @@ void corosync_state_dump (void) > static void unlink_all_completed (void) > { > poll_stop (corosync_poll_handle); > - coroipcs_ipc_exit (); > totempg_finalize (); > > corosync_exit_error (AIS_DONE_EXIT); > @@ -180,7 +179,7 @@ static void *corosync_exit_thread_handler (void *arg) > { > sem_wait (&corosync_exit_sem); > > - corosync_service_unlink_all (api, unlink_all_completed); > + corosync_service_unlink_all (api, unlink_all_completed, > corosync_poll_handle); > You shouldn't need corosync_poll_handle in this parameter if you use the coroipcs api properly > return arg; > } > diff --git a/trunk/exec/service.c b/trunk/exec/service.c > index d99e7bf..eb352da 100644 > --- a/trunk/exec/service.c > +++ b/trunk/exec/service.c > @@ -55,6 +55,8 @@ > #include <corosync/engine/coroapi.h> > #include "service.h" > > +#include <corosync/coroipcs.h> > + > LOGSYS_DECLARE_SUBSYS ("SERV"); > > struct default_service { > @@ -89,6 +91,16 @@ static struct default_service default_services[] = { > } > }; > > +/* > + * service exit and unlink handler data structure > + */ > +struct seus_handler_data { > + hdb_handle_t service_handle; > + int service_engine; > + hdb_handle_t coropoll_handle; > + struct corosync_api_v1 *api; > +}; > + what does seus mean? > struct corosync_service_engine *ais_service[SERVICE_HANDLER_MAXIMUM_COUNT]; > > hdb_handle_t service_stats_handle[SERVICE_HANDLER_MAXIMUM_COUNT][64]; > @@ -99,8 +111,8 @@ static hdb_handle_t object_stats_services_handle; > > static void (*service_unlink_all_complete) (void) = NULL; > > -static hdb_handle_t unlink_all_handle; > - > +static hdb_handle_t swrk_service_exit_handle; > +static hdb_handle_t swrk_service_unlink_handle; > > static unsigned int default_services_requested (struct corosync_api_v1 > *corosync_api) > { > @@ -272,7 +284,8 @@ corosync_service_unlink_priority ( > struct corosync_api_v1 *corosync_api, > int lowest_priority, > int *current_priority, > - int *current_service_engine) > + int *current_service_engine, > + hdb_handle_t *current_service_handle) > { > unsigned short *service_id; > hdb_handle_t object_service_handle; > @@ -319,11 +332,6 @@ corosync_service_unlink_priority ( > return (-1); > } > } > - log_printf(LOGSYS_LEVEL_NOTICE, > - "Service engine unloaded: %s\n", > - > ais_service[*current_service_engine]->name); > - > - ais_service[*current_service_engine] = > NULL; > > res = corosync_api->object_key_get ( > object_service_handle, > @@ -331,17 +339,24 @@ corosync_service_unlink_priority ( > (void *)&found_service_handle, > NULL); > > - lcr_ifact_release > (*found_service_handle); > + *current_service_handle = > *found_service_handle; > + > + corosync_api->object_find_destroy > (object_find_handle); > > - corosync_api->object_destroy > (object_service_handle); > - break; > + /* > + * Call should call this function again > + */ > + return (1); > } > } > > corosync_api->object_find_destroy (object_find_handle); > } > } > - return 0; > + /* > + * We finish unlink of all services -> no need to call this function > again > + */ > + return (0); > } > > static unsigned int service_unlink_and_exit ( > @@ -538,12 +553,45 @@ unsigned int corosync_service_defaults_link_and_init > (struct corosync_api_v1 *co > return (0); > } > > -static int unlink_all_schedwrk_handler (const void *data) { > +/* > + * Declaration of exit_schedwrk_handler, because of cycle > + * (service_exit_schedwrk_handler calls service_unlink_schedwrk_handler, and > vice-versa) > + */ > +static int service_exit_schedwrk_handler (const void *data); > + > +static int service_unlink_schedwrk_handler (const void *data) { > + struct seus_handler_data *cb_data = (struct seus_handler_data *)data; > + struct corosync_api_v1 *api = (struct corosync_api_v1 *)cb_data->api; > + > + /* > + * Exit all ipc connections dependand on this service > + */ spelling error - dependent. > + coroipcs_ipc_service_exit (cb_data->service_engine, > cb_data->coropoll_handle); > + we really shouldn't need coropoll_handle > + log_printf(LOGSYS_LEVEL_NOTICE, > + "Service engine unloaded: %s\n", > + ais_service[cb_data->service_engine]->name); > + > + ais_service[cb_data->service_engine] = NULL; > + > + lcr_ifact_release (cb_data->service_handle); > + > + api->schedwrk_create ( > + &swrk_service_exit_handle, > + &service_exit_schedwrk_handler, > + data); > + > + return 0; > +} > + > +static int service_exit_schedwrk_handler (const void *data) { > int res; > static int current_priority = 0; > static int current_service_engine = 0; > static int called = 0; > - struct corosync_api_v1 *api = (struct corosync_api_v1 *)data; > + struct seus_handler_data *cb_data = (struct seus_handler_data *)data; > + struct corosync_api_v1 *api = (struct corosync_api_v1 *)cb_data->api; > + hdb_handle_t service_handle; > > if (called == 0) { > log_printf(LOGSYS_LEVEL_NOTICE, > @@ -556,18 +604,35 @@ static int unlink_all_schedwrk_handler (const void > *data) { > api, > 0, > ¤t_priority, > - ¤t_service_engine); > + ¤t_service_engine, > + &service_handle); > if (res == 0) { > service_unlink_all_complete(); > + return (res); > + } > + > + if (res == 1) { > + cb_data->service_engine = current_service_engine; > + cb_data->service_handle = service_handle; > + > + api->schedwrk_create_nolock ( > + &swrk_service_unlink_handle, > + &service_unlink_schedwrk_handler, > + data); > + > + return (0); > } > + > return (res); > } > > void corosync_service_unlink_all ( > struct corosync_api_v1 *api, > - void (*unlink_all_complete) (void)) > + void (*unlink_all_complete) (void), > + hdb_handle_t poll_handle) shouldn't need poll_handle. > { > static int called = 0; > + static struct seus_handler_data cb_data; > > assert (api); > > @@ -579,11 +644,14 @@ void corosync_service_unlink_all ( > if (called == 0) { > called = 1; > } > - > + > + cb_data.api = api; > + cb_data.coropoll_handle = poll_handle; > + > api->schedwrk_create ( > - &unlink_all_handle, > - &unlink_all_schedwrk_handler, > - api); > + &swrk_service_exit_handle, > + &service_exit_schedwrk_handler, > + &cb_data); > } > > struct service_unlink_and_exit_data { > diff --git a/trunk/exec/service.h b/trunk/exec/service.h > index efb3e44..f94889c 100644 > --- a/trunk/exec/service.h > +++ b/trunk/exec/service.h > @@ -58,8 +58,9 @@ extern unsigned int corosync_service_unlink_and_exit ( > * Unlink and exit all corosync services > */ > extern void corosync_service_unlink_all ( > - struct corosync_api_v1 *api, > - void (*unlink_all_complete) (void)); > + struct corosync_api_v1 *api, > + void (*unlink_all_complete) (void), > + hdb_handle_t poll_handle); > > /* > * Load all of the default services > diff --git a/trunk/include/corosync/coroipcs.h > b/trunk/include/corosync/coroipcs.h > index 3838af8..614037e 100644 > --- a/trunk/include/corosync/coroipcs.h > +++ b/trunk/include/corosync/coroipcs.h > @@ -150,7 +150,7 @@ extern void coroipcs_refcount_inc (void *conn); > > extern void coroipcs_refcount_dec (void *conn); > > -extern void coroipcs_ipc_exit (void); > +extern void coroipcs_ipc_service_exit (unsigned int service, hdb_handle_t > poll_handle); > > extern int coroipcs_handler_accept ( > int fd, _______________________________________________ Openais mailing list [email protected] https://lists.linux-foundation.org/mailman/listinfo/openais
