{"thread":{"id":"37712","subject":"[PATCH 1/2] fix compilation with --disable-pthreads","startedAt":"2014-10-11T14:42:07Z","lastAt":"2014-10-14T14:46:47Z","messageCount":5,"participants":["Etienne Buira","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":2},"messages":[{"id":"250508","messageId":"d2df2f619967ea7ba9625868b9c77d79a1190665.1413038338.git.etienne.buira@gmail.com","threadId":"37712","inReplyTo":null,"subject":"[PATCH 1/2] fix compilation with --disable-pthreads","fromName":"Etienne Buira","fromEmail":"etienne.buira@gmail.com","sentAt":"2014-10-11T14:42:07Z","receivedAt":"2014-10-11T14:42:07Z","isPatch":true,"sender":{"key":"etienne.buira@gmail.com","avatar":"https://avatars.githubusercontent.com/u/11487810?v=4"},"body":"Signed-off-by: Etienne Buira \n---\n builtin/index-pack.c | 3 +++\n 1 file changed, 3 insertions(+)\n\ndiff --git a/builtin/index-pack.c b/builtin/index-pack.c\nindex eebf1a8..0f88f4b 100644\n--- a/builtin/index-pack.c\n+++ b/builtin/index-pack.c\n@@ -185,6 +185,9 @@ static void cleanup_thread(void)\n #define deepest_delta_lock()\n #define deepest_delta_unlock()\n \n+#define type_cas_lock()\n+#define type_cas_unlock()\n+\n #endif\n \n \n-- \n1.8.5.5\n"},{"id":"250509","messageId":"8d057560e40fb5edaa3a32f204718c5e561a207a.1413038338.git.etienne.buira@gmail.com","threadId":"37712","inReplyTo":"d2df2f619967ea7ba9625868b9c77d79a1190665.1413038338.git.etienne.buira@gmail.com","subject":"[PATCH 2/2] Remove spurious 'no threads support' warnings","fromName":"Etienne Buira","fromEmail":"etienne.buira@gmail.com","sentAt":"2014-10-11T14:46:27Z","receivedAt":"2014-10-11T14:46:27Z","isPatch":true,"sender":{"key":"etienne.buira@gmail.com","avatar":"https://avatars.githubusercontent.com/u/11487810?v=4"},"body":"Threads count being defaulted to 0 (autodetect), and --disable-pthreads\nbuild checking that thread count==1, there were spurious warnings about\nthreads being ignored, despite not specified on command line/conf.\n\nFixes tests 5521 and 5526 that were broken in --disable-pthreads builds\nbecause of those warnings.\n\nSigned-off-by: Etienne Buira \n---\n builtin/pack-objects.c | 2 +-\n thread-utils.h | 3 +++\n 2 files changed, 4 insertions(+), 1 deletion(-)\n\ndiff --git a/builtin/pack-objects.c b/builtin/pack-objects.c\nindex b59f5d8..7b51453 100644\n--- a/builtin/pack-objects.c\n+++ b/builtin/pack-objects.c\n@@ -51,7 +51,7 @@ static int progress = 1;\n static int window = 10;\n static unsigned long pack_size_limit;\n static int depth = 50;\n-static int delta_search_threads;\n+static int delta_search_threads = THREAD_COMPAT_DEFAULT_THREAD_NBR;\n static int pack_to_stdout;\n static int num_preferred_base;\n static struct progress *progress_state;\ndiff --git a/thread-utils.h b/thread-utils.h\nindex 6fb98c3..4f5e802 100644\n--- a/thread-utils.h\n+++ b/thread-utils.h\n@@ -7,5 +7,8 @@\n extern int online_cpus(void);\n extern int init_recursive_mutex(pthread_mutex_t*);\n \n+#define THREAD_COMPAT_DEFAULT_THREAD_NBR 0\n+#else\n+#define THREAD_COMPAT_DEFAULT_THREAD_NBR 1\n #endif\n #endif /* THREAD_COMPAT_H */\n-- \n1.8.5.5\n"},{"id":"250566","messageId":"xmqqwq833hws.fsf@gitster.dls.corp.google.com","threadId":"37712","inReplyTo":"d2df2f619967ea7ba9625868b9c77d79a1190665.1413038338.git.etienne.buira@gmail.com","subject":"Re: [PATCH 1/2] fix compilation with --disable-pthreads","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2014-10-13T19:10:11Z","receivedAt":"2014-10-13T19:10:11Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Etienne Buira writes:\n\n> Subject: Re: [PATCH 1/2] fix compilation with --disable-pthreads\n\nThat probably is a typo of \"NO_PTHREADS=NoThanks\" or something.\nThanks.\n\nJust out of curiosity, are you porting to some exotic platforms?\n\n> Signed-off-by: Etienne Buira \n> ---\n> builtin/index-pack.c | 3 +++\n> 1 file changed, 3 insertions(+)\n>\n> diff --git a/builtin/index-pack.c b/builtin/index-pack.c\n> index eebf1a8..0f88f4b 100644\n> --- a/builtin/index-pack.c\n> +++ b/builtin/index-pack.c\n> @@ -185,6 +185,9 @@ static void cleanup_thread(void)\n> #define deepest_delta_lock()\n> #define deepest_delta_unlock()\n> \n> +#define type_cas_lock()\n> +#define type_cas_unlock()\n> +\n> #endif\n"},{"id":"250571","messageId":"xmqqbnpf3fub.fsf@gitster.dls.corp.google.com","threadId":"37712","inReplyTo":"8d057560e40fb5edaa3a32f204718c5e561a207a.1413038338.git.etienne.buira@gmail.com","subject":"Re: [PATCH 2/2] Remove spurious 'no threads support' warnings","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2014-10-13T19:54:52Z","receivedAt":"2014-10-13T19:54:52Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Etienne Buira writes:\n\n> Threads count being defaulted to 0 (autodetect), and --disable-pthreads\n> build checking that thread count==1, there were spurious warnings about\n> threads being ignored, despite not specified on command line/conf.\n>\n> Fixes tests 5521 and 5526 that were broken in --disable-pthreads builds\n> because of those warnings.\n>\n> Signed-off-by: Etienne Buira \n> ---\n\nI am not sure if this is the right fix.\n\nShouldn't a --threads=0 from the command line (when there is a\npack.threads configuration hardcoding some number to override it)\ngive a chance to the auto detection codepath to ask online_cpus()\nand receive 1 on NO_PTHREADS build to avoid triggering the same\nwarning you are squelching with this patch?\n\nThat is, something like this instead, perhaps?\n\n-- >8 --\nSubject: [PATCH] pack-objects: set number of threads before checking and warning\n\nUnder NO_PTHREADS build, we warn when delta_search_threads is not\nset to 1, because that is the only sensible value on a single\nthreaded build.\n\nHowever, the auto detection that kicks in when that variable is set\nto 0 (e.g. there is no configuration variable or command line option,\nor an explicit --threads=0 is given from the command line to override\nthe pack.threads configuration to force auto-detection) was not done\nbefore the condition to issue this warning was tested.\n\nMove the auto-detection code and place it at an appropriate spot.\n\nSigned-off-by: Junio C Hamano \n---\n builtin/pack-objects.c | 6 ++++--\n thread-utils.h | 4 ++++\n 2 files changed, 8 insertions(+), 2 deletions(-)\n\ndiff --git a/builtin/pack-objects.c b/builtin/pack-objects.c\nindex d391934..a715237 100644\n--- a/builtin/pack-objects.c\n+++ b/builtin/pack-objects.c\n@@ -1972,8 +1972,6 @@ static void ll_find_deltas(struct object_entry **list, unsigned list_size,\n \n \tinit_threaded_search();\n \n-\tif (!delta_search_threads)\t/* --threads=0 means autodetect */\n-\t\tdelta_search_threads = online_cpus();\n \tif (delta_search_threads <= 1) {\n \t\tfind_deltas(list, &list_size, window, depth, processed);\n \t\tcleanup_threaded_search();\n@@ -2685,6 +2683,10 @@ int cmd_pack_objects(int argc, const char **argv, const char *prefix)\n \t\tpack_compression_level = Z_DEFAULT_COMPRESSION;\n \telse if (pack_compression_level < 0 || pack_compression_level > Z_BEST_COMPRESSION)\n \t\tdie(\"bad pack compression level %d\", pack_compression_level);\n+\n+\tif (!delta_search_threads)\t/* --threads=0 means autodetect */\n+\t\tdelta_search_threads = online_cpus();\n+\n #ifdef NO_PTHREADS\n \tif (delta_search_threads != 1)\n \t\twarning(\"no threads support, ignoring --threads\");\ndiff --git a/thread-utils.h b/thread-utils.h\nindex 6fb98c3..d9a769d 100644\n--- a/thread-utils.h\n+++ b/thread-utils.h\n@@ -7,5 +7,9 @@\n extern int online_cpus(void);\n extern int init_recursive_mutex(pthread_mutex_t*);\n \n+#else\n+\n+#define online_cpus() 1\n+\n #endif\n #endif /* THREAD_COMPAT_H */\n-- \n2.1.2-468-g1a77c5b\n"},{"id":"250611","messageId":"CAMcFKVyGKnGmmzDwKuSn3cQmG-jNiRRW-2rExL2+dBG7eaX_bA@mail.gmail.com","threadId":"37712","inReplyTo":"xmqqbnpf3fub.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH 2/2] Remove spurious 'no threads support' warnings","fromName":"Etienne Buira","fromEmail":"etienne.buira@gmail.com","sentAt":"2014-10-14T14:46:47Z","receivedAt":"2014-10-14T14:46:47Z","isPatch":true,"sender":{"key":"etienne.buira@gmail.com","avatar":"https://avatars.githubusercontent.com/u/11487810?v=4"},"body":"On Mon, Oct 13, 2014 at 9:54 PM, Junio C Hamano wrote:\n> Etienne Buira writes:\n>\n>> Threads count being defaulted to 0 (autodetect), and --disable-pthreads\n>> build checking that thread count==1, there were spurious warnings about\n>> threads being ignored, despite not specified on command line/conf.\n>>\n>> Fixes tests 5521 and 5526 that were broken in --disable-pthreads builds\n>> because of those warnings.\n>>\n>> Signed-off-by: Etienne Buira \n>> ---\n>\n> I am not sure if this is the right fix.\n>\n> Shouldn't a --threads=0 from the command line (when there is a\n> pack.threads configuration hardcoding some number to override it)\n> give a chance to the auto detection codepath to ask online_cpus()\n> and receive 1 on NO_PTHREADS build to avoid triggering the same\n> warning you are squelching with this patch?\n>\n> That is, something like this instead, perhaps?\n\nIndeed, your patch is better.\n\n> -- >8 --\n> Subject: [PATCH] pack-objects: set number of threads before checking and warning\n>\n> Under NO_PTHREADS build, we warn when delta_search_threads is not\n> set to 1, because that is the only sensible value on a single\n> threaded build.\n>\n> However, the auto detection that kicks in when that variable is set\n> to 0 (e.g. there is no configuration variable or command line option,\n> or an explicit --threads=0 is given from the command line to override\n> the pack.threads configuration to force auto-detection) was not done\n> before the condition to issue this warning was tested.\n>\n> Move the auto-detection code and place it at an appropriate spot.\n>\n> Signed-off-by: Junio C Hamano \n> ---\n> builtin/pack-objects.c | 6 ++++--\n> thread-utils.h | 4 ++++\n> 2 files changed, 8 insertions(+), 2 deletions(-)\n>\n> diff --git a/builtin/pack-objects.c b/builtin/pack-objects.c\n> index d391934..a715237 100644\n> --- a/builtin/pack-objects.c\n> +++ b/builtin/pack-objects.c\n> @@ -1972,8 +1972,6 @@ static void ll_find_deltas(struct object_entry **list, unsigned list_size,\n>\n> init_threaded_search();\n>\n> - if (!delta_search_threads) /* --threads=0 means autodetect */\n> - delta_search_threads = online_cpus();\n> if (delta_search_threads <= 1) {\n> find_deltas(list, &list_size, window, depth, processed);\n> cleanup_threaded_search();\n> @@ -2685,6 +2683,10 @@ int cmd_pack_objects(int argc, const char **argv, const char *prefix)\n> pack_compression_level = Z_DEFAULT_COMPRESSION;\n> else if (pack_compression_level < 0 || pack_compression_level > Z_BEST_COMPRESSION)\n> die(\"bad pack compression level %d\", pack_compression_level);\n> +\n> + if (!delta_search_threads) /* --threads=0 means autodetect */\n> + delta_search_threads = online_cpus();\n> +\n> #ifdef NO_PTHREADS\n> if (delta_search_threads != 1)\n> warning(\"no threads support, ignoring --threads\");\n> diff --git a/thread-utils.h b/thread-utils.h\n> index 6fb98c3..d9a769d 100644\n> --- a/thread-utils.h\n> +++ b/thread-utils.h\n> @@ -7,5 +7,9 @@\n> extern int online_cpus(void);\n> extern int init_recursive_mutex(pthread_mutex_t*);\n>\n> +#else\n> +\n> +#define online_cpus() 1\n> +\n> #endif\n> #endif /* THREAD_COMPAT_H */\n> --\n> 2.1.2-468-g1a77c5b\n>\n"}]}