[Freeipa-devel] [PATCH] 0007 Performace optimization for ldap_parse_configentry

[Adam Tkac]

On 02/23/2012 04:31 PM, Petr Spacek wrote:
> Hello,
>
> this patch is performance optimization of yesterday's fix 
> https://fedorahosted.org/bind-dyndb-ldap/ticket/43 - hold bind and 
> plugin global settings in LDAP.
Thanks for the patch, Petr, please check my comment below and then push 
the patch to master.

Regards, Adam
>
>
> bind-dyndb-ldap-pspacek-0007-Performace-optimization-for-ldap_parse_configentry.patch
>
>
>  From 61c4da9c39d3b42594dab39779da2495970d34f4 Mon Sep 17 00:00:00 2001
> From: Petr Spacek<pspacek at redhat.com>
> Date: Thu, 23 Feb 2012 16:14:13 +0100
> Subject: [PATCH] Performace optimization for ldap_parse_configentry().
>   Signed-off-by: Petr Spacek<pspacek at redhat.com>
>
> ---
>   src/ldap_helper.c |   24 ++++++++++++++----------
>   1 files changed, 14 insertions(+), 10 deletions(-)
>
> diff --git a/src/ldap_helper.c b/src/ldap_helper.c
> index bb1166d..42dbd74 100644
> --- a/src/ldap_helper.c
> +++ b/src/ldap_helper.c
> @@ -966,17 +966,13 @@ ldap_parse_configentry(ldap_entry_t *entry, ldap_instance_t *inst)
>   	isc_result_t result;
>   	ldap_valuelist_t values;
>   	isc_boolean_t unlock_required;
If I read code correctly the "unlock_required" variable is no longer 
needed, is it? Please remove it's declaration.
> +	isc_boolean_t sync_ptr_new;
>   	isc_timer_t *timer_inst;
>   	isc_interval_t timer_interval;
>   	isc_uint32_t interval_sec;
>   	isc_timertype_t timer_type;
>
> -	/* Before changing parameters transit to single-thread operation. */
> -	result = isc_task_beginexclusive(inst->task);
> -	RUNTIME_CHECK(result == ISC_R_SUCCESS ||
> -					result == ISC_R_LOCKBUSY);
> -	if (result == ISC_R_SUCCESS)
> -		unlock_required = ISC_TRUE;
> +	/* BIND functions are thread safe, lock only ldap instance 'inst'. */
>
>   	log_debug(3, "Parsing configuration object");
>
> @@ -993,8 +989,18 @@ ldap_parse_configentry(ldap_entry_t *entry, ldap_instance_t *inst)
>   	result = ldap_entry_getvalues(entry, "idnsAllowSyncPTR",&values);
>   	if (result == ISC_R_SUCCESS) {
>   		log_debug(2, "Setting global AllowSyncPTR = %s", HEAD(values)->value);
> -		inst->sync_ptr = (strcmp(HEAD(values)->value, "TRUE") == 0)
> +		sync_ptr_new = (strcmp(HEAD(values)->value, "TRUE") == 0)
>   						? ISC_TRUE : ISC_FALSE;
> +
> +		if (inst->sync_ptr != sync_ptr_new) { /* lock BIND only if necessary */
> +			result = isc_task_beginexclusive(inst->task);
> +			RUNTIME_CHECK(result == ISC_R_SUCCESS ||
> +							result == ISC_R_LOCKBUSY);
> +			inst->sync_ptr = sync_ptr_new;
> +			if (result == ISC_R_SUCCESS) {
> +				isc_task_endexclusive(inst->task);
> +			}
> +		}
>   	}
>
>   	result = ldap_entry_getvalues(entry, "idnsZoneRefresh",&values);
> @@ -1019,9 +1025,7 @@ ldap_parse_configentry(ldap_entry_t *entry, ldap_instance_t *inst)
>   	}
>
>   cleanup:
> -	if (unlock_required == ISC_TRUE)
> -		isc_task_endexclusive(inst->task);
> -
> +	/* Configuration errors are not fatal. */
>   	return ISC_R_SUCCESS;
>   }
>