From: Ken Raeburn Date: Thu, 18 Jun 2009 23:25:25 +0000 (+0000) Subject: reduce some mutex performance problems in profile library X-Git-Tag: krb5-1.8-alpha1~429 X-Git-Url: http://git.tremily.us/?a=commitdiff_plain;h=eaa340d042d0d9439b707d6462cf74c7d9bb5e61;p=krb5.git reduce some mutex performance problems in profile library In profile_node_iterator we unlock a mutex in order to call profile_update_file_data, which wants to lock that mutex itself, and then when it returns we re-lock the mutex. (We don't use recursive mutexes, and I would continue to argue that we shouldn't.) On the Mac, when running multiple threads, it appears that this results in very poor peformance, and much system and user CPU time is spent working with the locks. (Linux doesn't seem to suffer as much.) So: Split profile_update_file_data into a locking wrapper, and an inner routine that does the real work but requires that the lock be held on entry. Call the latter from profile_node_iterator *without* unlocking first, and only unlock if there's an error. This doesn't move any significant amount of work into the locking region; it pretty much just joins locking regions that were disjoint for no good reason. On my tests on an 8-core Mac, in a test program running gss_init_sec_context in a loop in 6 threads, this brought CPU usage per call down by 40%, and improved wall-clock time even more. Single-threaded performance improved very slightly, probably in the noise. Linux showed modest improvement (5% or less) in CPU usage in a 3-thread test on a 4-core system. Similar tests with gss_accept_sec_context showed similar contention around the profile-library mutexes, but I haven't analyzed the performance changes there from this patch. More work is needed, but this will help. ticket: 6515 tags: pullup target_version: 1.7.1 version_reported: 1.7 git-svn-id: svn://anonsvn.mit.edu/krb5/trunk@22418 dc483132-0cff-0310-8789-dd5450dbe970 --- diff --git a/src/util/profile/prof_file.c b/src/util/profile/prof_file.c index fad1b2871..b24fa59f9 100644 --- a/src/util/profile/prof_file.c +++ b/src/util/profile/prof_file.c @@ -303,7 +303,7 @@ errcode_t profile_open_file(const_profile_filespec_t filespec, return 0; } -errcode_t profile_update_file_data(prf_data_t data) +errcode_t profile_update_file_data_locked(prf_data_t data) { errcode_t retval; #ifdef HAVE_STAT @@ -313,20 +313,13 @@ errcode_t profile_update_file_data(prf_data_t data) #endif FILE *f; - retval = k5_mutex_lock(&data->lock); - if (retval) - return retval; - #ifdef HAVE_STAT now = time(0); if (now == data->last_stat && data->root != NULL) { - k5_mutex_unlock(&data->lock); return 0; } if (stat(data->filespec, &st)) { - retval = errno; - k5_mutex_unlock(&data->lock); - return retval; + return errno; } data->last_stat = now; #if defined HAVE_STRUCT_STAT_ST_MTIMENSEC @@ -341,7 +334,6 @@ errcode_t profile_update_file_data(prf_data_t data) if (st.st_mtime == data->timestamp && frac == data->frac_ts && data->root != NULL) { - k5_mutex_unlock(&data->lock); return 0; } if (data->root) { @@ -359,7 +351,6 @@ errcode_t profile_update_file_data(prf_data_t data) * profile file if it changes. */ if (data->root) { - k5_mutex_unlock(&data->lock); return 0; } #endif @@ -367,7 +358,6 @@ errcode_t profile_update_file_data(prf_data_t data) f = fopen(data->filespec, "r"); if (f == NULL) { retval = errno; - k5_mutex_unlock(&data->lock); if (retval == 0) retval = ENOENT; return retval; @@ -378,7 +368,6 @@ errcode_t profile_update_file_data(prf_data_t data) retval = profile_parse_file(f, &data->root); fclose(f); if (retval) { - k5_mutex_unlock(&data->lock); return retval; } assert(data->root != NULL); @@ -386,10 +375,21 @@ errcode_t profile_update_file_data(prf_data_t data) data->timestamp = st.st_mtime; data->frac_ts = frac; #endif - k5_mutex_unlock(&data->lock); return 0; } +errcode_t profile_update_file_data(prf_data_t data) +{ + errcode_t retval, retval2; + + retval = k5_mutex_lock(&data->lock); + if (retval) + return retval; + retval = profile_update_file_data_locked(data); + retval2 = k5_mutex_unlock(&data->lock); + return retval ? retval : retval2; +} + static int make_hard_link(const char *oldpath, const char *newpath) { diff --git a/src/util/profile/prof_int.h b/src/util/profile/prof_int.h index 02e1f21da..216c5986d 100644 --- a/src/util/profile/prof_int.h +++ b/src/util/profile/prof_int.h @@ -203,6 +203,9 @@ errcode_t profile_open_file #define profile_update_file(P) profile_update_file_data((P)->data) errcode_t profile_update_file_data (prf_data_t profile); +#define profile_update_file_locked(P) profile_update_file_data_locked((P)->data) +errcode_t profile_update_file_data_locked + (prf_data_t data); #define profile_flush_file(P) (((P) && (P)->magic == PROF_MAGIC_FILE) ? profile_flush_file_data((P)->data) : PROF_MAGIC_FILE) errcode_t profile_flush_file_data diff --git a/src/util/profile/prof_tree.c b/src/util/profile/prof_tree.c index 2b644a935..d8db45daf 100644 --- a/src/util/profile/prof_tree.c +++ b/src/util/profile/prof_tree.c @@ -497,8 +497,8 @@ get_new_file: *ret_value =0; return 0; } - k5_mutex_unlock(&iter->file->data->lock); - if ((retval = profile_update_file(iter->file))) { + if ((retval = profile_update_file_locked(iter->file))) { + k5_mutex_unlock(&iter->file->data->lock); if (retval == ENOENT || retval == EACCES) { /* XXX memory leak? */ iter->file = iter->file->next; @@ -517,11 +517,6 @@ get_new_file: return retval; } } - retval = k5_mutex_lock(&iter->file->data->lock); - if (retval) { - profile_node_iterator_free(iter_p); - return retval; - } iter->file_serial = iter->file->data->upd_serial; /* * Find the section to list if we are a LIST_SECTION,