Re: Fix quilt merge error in acpi-cpufreq.c

Re: Fix quilt merge error in acpi-cpufreq.c

[Ingo Molnar]

* Linux Kernel Mailing List <linux-kernel@vger.kernel.org> wrote:

> Gitweb:     http://git.kernel.org/linus/1c98aa7424ff163637d8321674ec58dee28152d4
> Commit:     1c98aa7424ff163637d8321674ec58dee28152d4
> Parent:     2e1c63b7ed36532b68f0eddd6a184d7ba1013b89
> Author:     Linus Torvalds <torvalds@linux-foundation.org>
> AuthorDate: Mon Apr 13 18:09:20 2009 -0700
> Committer:  Linus Torvalds <torvalds@linux-foundation.org>
> CommitDate: Mon Apr 13 18:09:20 2009 -0700
> 
>     Fix quilt merge error in acpi-cpufreq.c
>     
>     We ended up incorrectly using '&cur' instead of '&readin' in the
>     work_on_cpu() -> smp_call_function_single() transformation in commit
>     01599fca6758d2cd133e78f87426fc851c9ea725 ("cpufreq: use
>     smp_call_function_[single|many]() in acpi-cpufreq.c").
>     
>     Andrew explains:
>      "OK, the acpi tree went and had conflicting changes merged into it after
>       I'd written the patch and it appears that I incorrectly reverted part
>       of 18b2646fe3babeb40b34a0c1751e0bf5adfdc64c while fixing the resulting
>       rejects.
>     
>       Switching it to `readin' looks correct."
>     
>     Acked-by: Andrew Morton <akpm@linux-foundation.org>
>     Signed-off-by: Linus Torvalds <torvalds@linux-foundation.org>
> ---
>  arch/x86/kernel/cpu/cpufreq/acpi-cpufreq.c |    2 +-
>  1 files changed, 1 insertions(+), 1 deletions(-)
> 
> diff --git a/arch/x86/kernel/cpu/cpufreq/acpi-cpufreq.c b/arch/x86/kernel/cpu/cpufreq/acpi-cpufreq.c
> index 3e3cd3d..837c2c4 100644
> --- a/arch/x86/kernel/cpu/cpufreq/acpi-cpufreq.c
> +++ b/arch/x86/kernel/cpu/cpufreq/acpi-cpufreq.c
> @@ -277,7 +277,7 @@ static unsigned int get_measured_perf(struct cpufreq_policy *policy,
>  	unsigned int perf_percent;
>  	unsigned int retval;
>  
> -	if (smp_call_function_single(cpu, read_measured_perf_ctrs, &cur, 1))
> +	if (smp_call_function_single(cpu, read_measured_perf_ctrs, &readin, 1))
>  		return 0;

Ah, this might explain a few weird smp_processor_id() runtime 
warnings i got a few hours ago in that area of code (but didnt track 
it down at that time) when i updated to at around ~80a04d3.

(Never noticed the build warning - there's still too many of them.)

	Ingo

Re: Fix quilt merge error in acpi-cpufreq.c

[Ingo Molnar]

* Ingo Molnar <mingo@elte.hu> wrote:

> * Linux Kernel Mailing List <linux-kernel@vger.kernel.org> wrote:
> 
> > Gitweb:     http://git.kernel.org/linus/1c98aa7424ff163637d8321674ec58dee28152d4
> > Commit:     1c98aa7424ff163637d8321674ec58dee28152d4
> > Parent:     2e1c63b7ed36532b68f0eddd6a184d7ba1013b89
> > Author:     Linus Torvalds <torvalds@linux-foundation.org>
> > AuthorDate: Mon Apr 13 18:09:20 2009 -0700
> > Committer:  Linus Torvalds <torvalds@linux-foundation.org>
> > CommitDate: Mon Apr 13 18:09:20 2009 -0700
> > 
> >     Fix quilt merge error in acpi-cpufreq.c
> >     
> >     We ended up incorrectly using '&cur' instead of '&readin' in the
> >     work_on_cpu() -> smp_call_function_single() transformation in commit
> >     01599fca6758d2cd133e78f87426fc851c9ea725 ("cpufreq: use
> >     smp_call_function_[single|many]() in acpi-cpufreq.c").
> >     
> >     Andrew explains:
> >      "OK, the acpi tree went and had conflicting changes merged into it after
> >       I'd written the patch and it appears that I incorrectly reverted part
> >       of 18b2646fe3babeb40b34a0c1751e0bf5adfdc64c while fixing the resulting
> >       rejects.
> >     
> >       Switching it to `readin' looks correct."
> >     
> >     Acked-by: Andrew Morton <akpm@linux-foundation.org>
> >     Signed-off-by: Linus Torvalds <torvalds@linux-foundation.org>
> > ---
> >  arch/x86/kernel/cpu/cpufreq/acpi-cpufreq.c |    2 +-
> >  1 files changed, 1 insertions(+), 1 deletions(-)
> > 
> > diff --git a/arch/x86/kernel/cpu/cpufreq/acpi-cpufreq.c b/arch/x86/kernel/cpu/cpufreq/acpi-cpufreq.c
> > index 3e3cd3d..837c2c4 100644
> > --- a/arch/x86/kernel/cpu/cpufreq/acpi-cpufreq.c
> > +++ b/arch/x86/kernel/cpu/cpufreq/acpi-cpufreq.c
> > @@ -277,7 +277,7 @@ static unsigned int get_measured_perf(struct cpufreq_policy *policy,
> >  	unsigned int perf_percent;
> >  	unsigned int retval;
> >  
> > -	if (smp_call_function_single(cpu, read_measured_perf_ctrs, &cur, 1))
> > +	if (smp_call_function_single(cpu, read_measured_perf_ctrs, &readin, 1))
> >  		return 0;
> 
> Ah, this might explain a few weird smp_processor_id() runtime 
> warnings i got a few hours ago in that area of code (but didnt 
> track it down at that time) when i updated to at around ~80a04d3.
> 
> (Never noticed the build warning - there's still too many of 
> them.)

No, that warning is back and triggered in overnight testing:

[   54.888193] BUG: using smp_processor_id() in preemptible [00000000] code: S99local/7753
[   54.888267] caller is smp_call_function_many+0x29/0x210
[   54.888309] Pid: 7753, comm: S99local Not tainted 2.6.30-rc1-tip #1750
[   54.888352] Call Trace:
[   54.888389]  [<c054d06d>] debug_smp_processor_id+0xcd/0xd0
[   54.888432]  [<c016e989>] smp_call_function_many+0x29/0x210
[   54.888477]  [<c0115860>] ? do_drv_write+0x0/0x70
[   54.888519]  [<c0115851>] drv_write+0x21/0x30
[   54.888559]  [<c0115e06>] acpi_cpufreq_target+0x146/0x310

fuller log below. I think this is because smp_call_function_many() 
was essentially unused before - an IPI function should not trigger 
this warning, it will naturally be called in preemptible context. 

Rusty?

	Ingo

----------------->
[   40.227336] Adding 4096564k swap on /dev/sda2.  Priority:-1 extents:1 across:4096564k 
[   43.958724] eth0: no IPv6 routers present
[   54.827389] CPUFREQ: ondemand sampling_rate_max sysfs file is deprecated - used by: cat
[   54.888193] BUG: using smp_processor_id() in preemptible [00000000] code: S99local/7753
[   54.888267] caller is smp_call_function_many+0x29/0x210
[   54.888309] Pid: 7753, comm: S99local Not tainted 2.6.30-rc1-tip #1750
[   54.888352] Call Trace:
[   54.888389]  [<c054d06d>] debug_smp_processor_id+0xcd/0xd0
[   54.888432]  [<c016e989>] smp_call_function_many+0x29/0x210
[   54.888477]  [<c0115860>] ? do_drv_write+0x0/0x70
[   54.888519]  [<c0115851>] drv_write+0x21/0x30
[   54.888559]  [<c0115e06>] acpi_cpufreq_target+0x146/0x310
[   54.888603]  [<c0166aab>] ? trace_hardirqs_on_caller+0x1b/0x1b0
[   54.888647]  [<c0166c4b>] ? trace_hardirqs_on+0xb/0x10
[   54.888690]  [<c022fd78>] ? sysfs_remove_group+0x68/0xd0
[   54.888735]  [<c0f17e02>] ? __mutex_unlock_slowpath+0x172/0x180
[   54.888781]  [<c0115cc0>] ? acpi_cpufreq_target+0x0/0x310
[   54.888828]  [<c0c8141e>] __cpufreq_driver_target+0x5e/0xa0
[   54.888873]  [<c0c827a3>] cpufreq_governor_performance+0x23/0x30
[   54.888918]  [<c0c804cb>] __cpufreq_governor+0x2b/0x60
[   54.888961]  [<c015af6f>] ? blocking_notifier_call_chain+0x1f/0x30
[   54.889006]  [<c0c8076a>] __cpufreq_set_policy+0xfa/0x140
[   54.889048]  [<c0c80f9a>] store_scaling_governor+0x8a/0xb0
[   54.889091]  [<c0c81880>] ? handle_update+0x0/0x20
[   54.889133]  [<c0c80ea4>] ? cpufreq_cpu_get+0x74/0x90
[   54.889174]  [<c0c80f10>] ? store_scaling_governor+0x0/0xb0
[   54.889217]  [<c0c8195d>] store+0x4d/0x70
[   54.889257]  [<c022cc50>] flush_write_buffer+0x50/0x70
[   54.889298]  [<c022cd4c>] sysfs_write_file+0x4c/0x80
[   54.889340]  [<c01df6df>] vfs_write+0x8f/0xd0
[   54.889379]  [<c022cd00>] ? sysfs_write_file+0x0/0x80
[   54.889421]  [<c01df762>] sys_write+0x42/0x70
[   54.889461]  [<c0102fab>] sysenter_do_call+0x12/0x36
[   54.889823] BUG: using smp_processor_id() in preemptible [00000000] code: S99local/7753
[   54.889890] caller is smp_call_function_many+0x29/0x210
[   54.889931] Pid: 7753, comm: S99local Not tainted 2.6.30-rc1-tip #1750
[   54.889974] Call Trace:
[   54.890008]  [<c054d06d>] debug_smp_processor_id+0xcd/0xd0
[   54.890051]  [<c016e989>] smp_call_function_many+0x29/0x210
[   54.890093]  [<c0115860>] ? do_drv_write+0x0/0x70
[   54.890133]  [<c0115851>] drv_write+0x21/0x30
[   54.890172]  [<c0115e06>] acpi_cpufreq_target+0x146/0x310
[   54.890216]  [<c0166aab>] ? trace_hardirqs_on_caller+0x1b/0x1b0
[   54.890261]  [<c0166c4b>] ? trace_hardirqs_on+0xb/0x10
[   54.890304]  [<c022fd78>] ? sysfs_remove_group+0x68/0xd0
[   54.890349]  [<c0f17e02>] ? __mutex_unlock_slowpath+0x172/0x180
[   54.890393]  [<c0115cc0>] ? acpi_cpufreq_target+0x0/0x310
[   54.890437]  [<c0c8141e>] __cpufreq_driver_target+0x5e/0xa0
[   54.890480]  [<c0c827a3>] cpufreq_governor_performance+0x23/0x30
[   54.890526]  [<c0c804cb>] __cpufreq_governor+0x2b/0x60
[   54.890569]  [<c015af6f>] ? blocking_notifier_call_chain+0x1f/0x30
[   54.890614]  [<c0c8076a>] __cpufreq_set_policy+0xfa/0x140
[   54.890658]  [<c0c80f9a>] store_scaling_governor+0x8a/0xb0
[   54.890701]  [<c0c81880>] ? handle_update+0x0/0x20
[   54.890743]  [<c0c80ea4>] ? cpufreq_cpu_get+0x74/0x90
[   54.890783]  [<c0c80f10>] ? store_scaling_governor+0x0/0xb0
[   54.890826]  [<c0c8195d>] store+0x4d/0x70
[   54.890864]  [<c022cc50>] flush_write_buffer+0x50/0x70
[   54.890994]  [<c022cd4c>] sysfs_write_file+0x4c/0x80
[   54.891035]  [<c01df6df>] vfs_write+0x8f/0xd0
[   54.891075]  [<c022cd00>] ? sysfs_write_file+0x0/0x80
[   54.891115]  [<c01df762>] sys_write+0x42/0x70
[   54.891154]  [<c0102fab>] sysenter_do_call+0x12/0x36
[   56.475977] device: 'vcs4': device_add

Re: Fix quilt merge error in acpi-cpufreq.c

[Rusty Russell]

On Wed, 15 Apr 2009 03:14:17 pm Ingo Molnar wrote:
> No, that warning is back and triggered in overnight testing:
> 
> [   54.888193] BUG: using smp_processor_id() in preemptible [00000000] code: S99local/7753
> [   54.888267] caller is smp_call_function_many+0x29/0x210
> [   54.888309] Pid: 7753, comm: S99local Not tainted 2.6.30-rc1-tip #1750
> [   54.888352] Call Trace:
> [   54.888389]  [<c054d06d>] debug_smp_processor_id+0xcd/0xd0
> [   54.888432]  [<c016e989>] smp_call_function_many+0x29/0x210
> [   54.888477]  [<c0115860>] ? do_drv_write+0x0/0x70
> [   54.888519]  [<c0115851>] drv_write+0x21/0x30
> [   54.888559]  [<c0115e06>] acpi_cpufreq_target+0x146/0x310
> 
> fuller log below. I think this is because smp_call_function_many() 
> was essentially unused before - an IPI function should not trigger 
> this warning, it will naturally be called in preemptible context. 
> 
> Rusty?

Hi Ingo,

   Thanks for the ping, but this code hasn't changed from the original
smp_call_function_mask (I just checked).  Andrew's patch is incorrect.

   The API is screwy.  It excludes the current CPU from the mask,
unconditionally.  It's a tlb flush helper masquerading as a general function.

(smp_call_function has the same issue).

Something like this?

Subject: smp_call_function_many: add explicit exclude_self flag

Impact: clarify and extend confusing API

It's not clear that smp_call_function_many (like smp_call_function)
will exclude the current CPU.  Make it explicit and at the same time
make it generically useful.

Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
Cc: Andrew Morton <akpm@linux-foundation.org>
---
 arch/powerpc/mm/tlb_nohash.c |    4 ++--
 arch/sparc/kernel/smp_64.c   |    2 +-
 arch/x86/xen/mmu.c           |    2 +-
 include/linux/smp.h          |    9 +++++----
 kernel/smp.c                 |   41 ++++++++++++++++++++++++++++++-----------
 virt/kvm/kvm_main.c          |    4 ++--
 6 files changed, 41 insertions(+), 21 deletions(-)

diff --git a/arch/powerpc/mm/tlb_nohash.c b/arch/powerpc/mm/tlb_nohash.c
--- a/arch/powerpc/mm/tlb_nohash.c
+++ b/arch/powerpc/mm/tlb_nohash.c
@@ -136,7 +136,7 @@ void flush_tlb_mm(struct mm_struct *mm)
 		struct tlb_flush_param p = { .pid = pid };
 		/* Ignores smp_processor_id() even if set. */
 		smp_call_function_many(mm_cpumask(mm),
-				       do_flush_tlb_mm_ipi, &p, 1);
+				       do_flush_tlb_mm_ipi, &p, 1, 1);
 	}
 	_tlbil_pid(pid);
  no_context:
@@ -168,7 +168,7 @@ void flush_tlb_page(struct vm_area_struc
 			struct tlb_flush_param p = { .pid = pid, .addr = vmaddr };
 			/* Ignores smp_processor_id() even if set in cpu_mask */
 			smp_call_function_many(cpu_mask,
-					       do_flush_tlb_page_ipi, &p, 1);
+					       do_flush_tlb_page_ipi, &p, 1, 1);
 		}
 	}
 	_tlbil_va(vmaddr, pid);
diff --git a/arch/sparc/kernel/smp_64.c b/arch/sparc/kernel/smp_64.c
--- a/arch/sparc/kernel/smp_64.c
+++ b/arch/sparc/kernel/smp_64.c
@@ -850,7 +850,7 @@ static void tsb_sync(void *info)
 
 void smp_tsb_sync(struct mm_struct *mm)
 {
-	smp_call_function_many(mm_cpumask(mm), tsb_sync, mm, 1);
+	smp_call_function_many(mm_cpumask(mm), tsb_sync, mm, 1, 1);
 }
 
 extern unsigned long xcall_flush_tlb_mm;
diff --git a/arch/x86/xen/mmu.c b/arch/x86/xen/mmu.c
--- a/arch/x86/xen/mmu.c
+++ b/arch/x86/xen/mmu.c
@@ -1161,7 +1161,7 @@ static void xen_drop_mm_ref(struct mm_st
 	}
 
 	if (!cpumask_empty(mask))
-		smp_call_function_many(mask, drop_other_mm_ref, mm, 1);
+		smp_call_function_many(mask, drop_other_mm_ref, mm, 1, 1);
 	free_cpumask_var(mask);
 }
 #else
diff --git a/include/linux/smp.h b/include/linux/smp.h
--- a/include/linux/smp.h
+++ b/include/linux/smp.h
@@ -71,14 +71,15 @@ extern void smp_cpus_done(unsigned int m
  */
 int smp_call_function(void(*func)(void *info), void *info, int wait);
 void smp_call_function_many(const struct cpumask *mask,
-			    void (*func)(void *info), void *info, bool wait);
+			    void (*func)(void *info), void *info, bool wait,
+			    bool exclude_self);
 
 /* Deprecated: Use smp_call_function_many which takes a pointer to the mask. */
 static inline int
 smp_call_function_mask(cpumask_t mask, void(*func)(void *info), void *info,
 		       int wait)
 {
-	smp_call_function_many(&mask, func, info, wait);
+	smp_call_function_many(&mask, func, info, wait, 1);
 	return 0;
 }
 
@@ -144,9 +145,9 @@ static inline int up_smp_call_function(v
 static inline void smp_send_reschedule(int cpu) { }
 #define num_booting_cpus()			1
 #define smp_prepare_boot_cpu()			do {} while (0)
-#define smp_call_function_mask(mask, func, info, wait) \
+#define smp_call_function_mask(mask, func, info, wait)	\
 			(up_smp_call_function(func, info))
-#define smp_call_function_many(mask, func, info, wait) \
+#define smp_call_function_many(mask, func, info, wait, exclude_self) \
 			(up_smp_call_function(func, info))
 static inline void init_call_single_data(void)
 {
diff --git a/kernel/smp.c b/kernel/smp.c
--- a/kernel/smp.c
+++ b/kernel/smp.c
@@ -11,6 +11,7 @@
 #include <linux/init.h>
 #include <linux/smp.h>
 #include <linux/cpu.h>
+#include <linux/hardirq.h>
 
 static DEFINE_PER_CPU(struct call_single_queue, call_single_queue);
 
@@ -349,6 +350,8 @@ void __smp_call_function_single(int cpu,
  * @info: An arbitrary pointer to pass to the function.
  * @wait: If true, wait (atomically) until function has completed
  *        on other CPUs.
+ * @exclude_self: If true, don't call the function on this cpu, even if
+ *        it is set.  This implies preemption is disabled.
  *
  * If @wait is true, then returns once @func has returned. Note that @wait
  * will be implicitly turned on in case of allocation failures, since
@@ -356,30 +359,39 @@ void __smp_call_function_single(int cpu,
  *
  * You must not call this function with disabled interrupts or from a
  * hardware interrupt handler or from a bottom half handler. Preemption
- * must be disabled when calling this function.
+ * must be disabled when calling this function with @exclude_self set.
  */
 void smp_call_function_many(const struct cpumask *mask,
-			    void (*func)(void *), void *info, bool wait)
+			    void (*func)(void *), void *info,
+			    bool wait, bool exclude_self)
 {
 	struct call_function_data *data;
 	unsigned long flags;
-	int cpu, next_cpu, this_cpu = smp_processor_id();
+	int cpu, next_cpu, this_cpu;
 
-	/* Can deadlock when called with interrupts disabled */
-	WARN_ON_ONCE(irqs_disabled() && !oops_in_progress);
+	if (!oops_in_progress) {
+		/* Can deadlock when called with interrupts disabled */
+		WARN_ON_ONCE(irqs_disabled());
 
-	/* So, what's a CPU they want? Ignoring this one. */
+		/* Why exclude the current cpu if you don't know what it is? */
+		WARN_ON_ONCE(exclude_self && !in_atomic());
+	}
+
+	/* Disable preemption if it hasn't been already. */
+	this_cpu = get_cpu();
+
+	/* So, what's a CPU they want?  Possibly ignoring this one. */
 	cpu = cpumask_first_and(mask, cpu_online_mask);
-	if (cpu == this_cpu)
+	if (exclude_self && cpu == this_cpu)
 		cpu = cpumask_next_and(cpu, mask, cpu_online_mask);
 
 	/* No online cpus?  We're done. */
 	if (cpu >= nr_cpu_ids)
 		return;
 
-	/* Do we have another CPU which isn't us? */
+	/* Do we have another CPU?  (Which isn't us) */
 	next_cpu = cpumask_next_and(cpu, mask, cpu_online_mask);
-	if (next_cpu == this_cpu)
+	if (exclude_self && next_cpu == this_cpu)
 		next_cpu = cpumask_next_and(next_cpu, mask, cpu_online_mask);
 
 	/* Fastpath: do that cpu by itself. */
@@ -416,12 +428,19 @@ void smp_call_function_many(const struct
 	 */
 	smp_mb();
 
-	/* Send a message to all CPUs in the map */
+	if (!exclude_self && cpumask_test_cpu(this_cpu, data->cpumask)) {
+		local_irq_save(flags);
+		func(info);
+		local_irq_restore(flags);
+	}
+
+	/* Send a message to all CPUs in the map (excludes ourselves) */
 	arch_send_call_function_ipi_mask(data->cpumask);
 
 	/* Optionally wait for the CPUs to complete */
 	if (wait)
 		csd_lock_wait(&data->csd);
+	put_cpu();
 }
 EXPORT_SYMBOL(smp_call_function_many);
 
@@ -444,7 +463,7 @@ EXPORT_SYMBOL(smp_call_function_many);
 int smp_call_function(void (*func)(void *), void *info, int wait)
 {
 	preempt_disable();
-	smp_call_function_many(cpu_online_mask, func, info, wait);
+	smp_call_function_many(cpu_online_mask, func, info, wait, true);
 	preempt_enable();
 
 	return 0;
diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c
--- a/virt/kvm/kvm_main.c
+++ b/virt/kvm/kvm_main.c
@@ -592,9 +592,9 @@ static bool make_all_cpus_request(struct
 			cpumask_set_cpu(cpu, cpus);
 	}
 	if (unlikely(cpus == NULL))
-		smp_call_function_many(cpu_online_mask, ack_flush, NULL, 1);
+		smp_call_function_many(cpu_online_mask, ack_flush, NULL, 1, 1);
 	else if (!cpumask_empty(cpus))
-		smp_call_function_many(cpus, ack_flush, NULL, 1);
+		smp_call_function_many(cpus, ack_flush, NULL, 1, 1);
 	else
 		called = false;
 	put_cpu();

Re: Fix quilt merge error in acpi-cpufreq.c

[Linus Torvalds]

On Wed, 15 Apr 2009, Ingo Molnar wrote:
> 
> fuller log below. I think this is because smp_call_function_many() 
> was essentially unused before - an IPI function should not trigger 
> this warning, it will naturally be called in preemptible context. 

Yeah, that thing is buggy. It just does "this_cpu = smp_processor_id()". 

But I have to admit that the breakage is documented. Both the "other 
CPU's" part _and_ the "preemption must be disabled when calling".

So it's not a bug, it's a "feature". 

Which is obviously not to say that the thing isn't complete crap.

This patch should fix it - not by fixing smp_call_function_many(), but by 
just living with the breakage. Andrew already sent out a patch that just 
avoided the function entirely, but at least some systems are likely to be 
able to do one single broadcast IPI with this, so it's at least in theory 
still better to use that smp_call_function_many() function, even though it 
has braindamaged semantics.

		Linus

---
 arch/x86/kernel/cpu/cpufreq/acpi-cpufreq.c |    6 ++++++
 1 files changed, 6 insertions(+), 0 deletions(-)

diff --git a/arch/x86/kernel/cpu/cpufreq/acpi-cpufreq.c b/arch/x86/kernel/cpu/cpufreq/acpi-cpufreq.c
index 837c2c4..ecdb682 100644
--- a/arch/x86/kernel/cpu/cpufreq/acpi-cpufreq.c
+++ b/arch/x86/kernel/cpu/cpufreq/acpi-cpufreq.c
@@ -204,7 +204,13 @@ static void drv_read(struct drv_cmd *cmd)
 
 static void drv_write(struct drv_cmd *cmd)
 {
+	int this_cpu;
+
+	this_cpu = get_cpu();
+	if (cpumask_test_cpu(this_cpu, cmd->mask))
+		do_drv_write(cmd);
 	smp_call_function_many(cmd->mask, do_drv_write, cmd, 1);
+	put_cpu();
 }
 
 static u32 get_cur_val(const struct cpumask *mask)

Re: Fix quilt merge error in acpi-cpufreq.c

[Ali Gholami Rudi]

Linus Torvalds <torvalds@linux-foundation.org> wrote:
> diff --git a/arch/x86/kernel/cpu/cpufreq/acpi-cpufreq.c b/arch/x86/kernel/cpu/cpufreq/acpi-cpufreq.c
> index 837c2c4..ecdb682 100644
> --- a/arch/x86/kernel/cpu/cpufreq/acpi-cpufreq.c
> +++ b/arch/x86/kernel/cpu/cpufreq/acpi-cpufreq.c
> @@ -204,7 +204,13 @@ static void drv_read(struct drv_cmd *cmd)
>  
>  static void drv_write(struct drv_cmd *cmd)
>  {
> +	int this_cpu;
> +
> +	this_cpu = get_cpu();
> +	if (cpumask_test_cpu(this_cpu, cmd->mask))
> +		do_drv_write(cmd);
>  	smp_call_function_many(cmd->mask, do_drv_write, cmd, 1);
> +	put_cpu();
>  }
>  
>  static u32 get_cur_val(const struct cpumask *mask)

Tested it and works.

Regards,
Ali

Re: Fix quilt merge error in acpi-cpufreq.c

[Linus Torvalds]

On Wed, 15 Apr 2009, Rusty Russell wrote:
> 
>    The API is screwy.  It excludes the current CPU from the mask,
> unconditionally.  It's a tlb flush helper masquerading as a general function.
> 
> (smp_call_function has the same issue).
> 
> Something like this?
> 
> Subject: smp_call_function_many: add explicit exclude_self flag

No. This just makes the API even screwier. It fixes the 
"smp_processor_id()" thing, but it is 

 (a) horribly buggy

     See the 'return' without any put_cpu()

 (b) horribly buggy

     Those 

	if (exclude_self && cpu == this_cpu)
		cpu = cpumask_next_and(cpu, mask, cpu_online_mask);

     things are wrong - we need to do that "jump over our own CPU" thing 
     regardless of whether 'exclude_self' is set or not, since we're going 
     to special-case our own CPU anyway.

 (c) Wrong, even if it wasn't (horribly buggy)^2

     Adding "flags" to an interface doesn't make it better. Quite the 
     reverse. It makes it worse. It also makes it even MORE different from 
     all the other smp_call_function's, which just do the 'self' cpu 
     without any stupid conditionals and flags.

IOW, it would make things worse even if it worked. Which it doesn't.

> Impact: clarify and extend confusing API

And what the hell is up with these bogus "Impact:" things? Who started 
doing that, and why? If your single-line explanation at the top is not 
good enough, and your multi-line explanation isn't clear enough, then you 
should fix the OTHER parts, not add that _idiotic_ "Impact" statement.

The thing has spread like wildfire, and it's STUPID.

		Linus

Re: Fix quilt merge error in acpi-cpufreq.c

[Ingo Molnar]

* Linus Torvalds <torvalds@linux-foundation.org> wrote:

> > Impact: clarify and extend confusing API
> 
> And what the hell is up with these bogus "Impact:" things? Who 
> started doing that, and why? If your single-line explanation at 
> the top is not good enough, and your multi-line explanation isn't 
> clear enough, then you should fix the OTHER parts, not add that 
> _idiotic_ "Impact" statement.

I got Rusty to use it so i'm to blame for this one. A number of 
developers/maintainers use it now and they find it useful in a 
number of circumstances, when used judiciously.

We are using impact lines to judge "practical impact of a commit". 
The shorter (while still correct and expressive), the better. We are 
trying to use it in well-defined cases - but not always.

Here it helped expose the bogosity of a patch more clearly: the 
intended impact of "clarifying a confusing API" was not met, and the 
commit became easier to flame.

There's 6 different classes of uses of impact lines right now:

1)

They force smaller patch submissions: it is hard to write a correct 
impact line for an overly complex, multi-purpose patch. Just try it 
in practice and you'll see. It is _much_ easier to write a correct 
impact line for a properly split up patch series.

So instead of bitching with developers again and again, we asked 
frequent sinners to write proper impact lines. Voila, the patches 
became smaller: one patch, one main line of (intended) impact. It is 
a very nicely self-regulating process.

2)

One other purpose of them is to have a ... managerial 
risk-at-a-glance view of a larger set of commits, post facto.

For example:

      $ git log v2.6.29..v2.6.30-rc1 arch/x86/kernel/apic/ | \
                  grep Impact: | sort | uniq -c | sort -n

      1     Impact: build fix
      1     Impact: build fix, cleanup
      1     Impact: cleanup, paranoia
      1     Impact: cleanup, reduce memory usage for CONFIG_CPUMASK_OFFSTACK=y
      1     Impact: cleanup, remove cpumask from stack
      1     Impact: fix bug with irq-descriptor moving when logical flat
      1     Impact: fix incorrect error message
      1     Impact: fix possible race
      1     Impact: fix spurious IRQs
      1     Impact: get correct smp_affinity as user requested
      1     Impact: interface augmentation (not yet used)
      1     Impact: make kexec work with x2apic
      1     Impact: optimize APIC IPI related barriers
      1     Impact: simplification
     10     Impact: cleanup

Shows (at a glance) that we had 5-6 runtime problems (mostly 
misbehavior, not crashes) in the APIC code during the last 
development window.

Or:

      $ git log v2.6.29..v2.6.30-rc1 kernel/sched.c | \
                   grep Impact: | sort | uniq -c | sort -n

      1     Impact: cleanup, micro-optimization
      1     Impact: cleanup, new schedstat ABI
      1     Impact: fix boot crash
      1     Impact: fix circular locking
      1     Impact: fix function graph trace hang / drop pointless softirq on UP
      1     Impact: fix to preempt trace triggering lockdep check_flag failure
      1     Impact: more precise avg_overlap metric - better load-balancing
      1     Impact: struct rq size optimization
      2     Impact: micro-optimization
     12     Impact: cleanup

Shows that we had ~4 runtime problems (crashes or lockdep asserts) 
in the scheduler during the last development window.

3)

"Risk judgement at a glance" cannot be done via other attributes of 
the commit:

The subject lines are too important to be burdened with structured 
risk information, and they are also too specific and spread too 
much. But if you think we can add [fix crash] and [pure cleanup] 
tags to primary subject lines that would be even better ...

We thought that such artificial risk attribute structure was an 
obvious non-starter, because it would depart from existing commit 
practices so much.

The subject line also tends to mimic the patch-submission subject 
lines (so that it aligns with lkml discussions/submissions) and 
tends to be more detailed and differently structured - so it's a lot 
harder to extract only risk/impact information from it, at a glance.

4)

We are also using "Impact: cleanup" as a special tag for commits 
that are supposed to have _zero_ side-effects. "cleanup" otherwise 
can be ambigious: it often covers restructuring / refactoring of 
code and it covers changes that have side-effects. We had several 
cases in the past when an "Impact: cleanup" patch was bisected to - 
and the bisector / bug reporter already knew it straight away that 
there was a misunderstanding about the impact of the commit, without 
having to fully read the patch. This is very useful when someone not 
versed in that particular code meets such a commit down the line.

5)

Impact lines are also very useful during maintenance, when adding it 
to a patch that got submitted by others: if i mis-interpret the 
impact of a patch and add the wrong impact line, people point it 
out. This happened several times in the past, and it can be 
embarrasing - but it forces maintainer attention and honesty.

6)

It forces developer honesty/disclosure as well: sometimes a commit 
log is too wishy-washy and instead of forcing people to rewrite full 
commit logs (often made difficult by language barriers - the 
intersection of people who can write good code an good 
documentation/commits in English is very small - and we shouldnt 
exclude good coders who are not able to write good 
documentation/commits) we ask them (or show them) impact lines.

It also forces developers to _think_ about the full impact of 
patches. We often got bad patches because people clearly did not 
think through what they were trying to do. With an impact line it's 
much easier to argue whether a developer was fully aware of a given 
risk of a commit or not.


All in one, the impact line standardizes risk/impact info in a 
compact form. I do think that 'risk' is an essential attribute of a 
commit.

Is this scheme perfect? Clearly not - and we are still experimenting 
with exactly how to shape it better (you can see several small 
variants).

But it's already pretty useful in the history too - not just while 
writing changes and queueing up commits.

I guess your flame means we must stop using it. Oh well. The party 
was nice while it lasted ;-)

	Ingo

Re: Fix quilt merge error in acpi-cpufreq.c

[Ingo Molnar]

* Linus Torvalds <torvalds@linux-foundation.org> wrote:

> On Wed, 15 Apr 2009, Ingo Molnar wrote:
> > 
> > fuller log below. I think this is because 
> > smp_call_function_many() was essentially unused before - an IPI 
> > function should not trigger this warning, it will naturally be 
> > called in preemptible context.
> 
> Yeah, that thing is buggy. It just does "this_cpu = 
> smp_processor_id()".
> 
> But I have to admit that the breakage is documented. Both the 
> "other CPU's" part _and_ the "preemption must be disabled when 
> calling".
> 
> So it's not a bug, it's a "feature".
> 
> Which is obviously not to say that the thing isn't complete crap.
> 
> This patch should fix it - not by fixing smp_call_function_many(), 
> but by just living with the breakage. Andrew already sent out a 
> patch that just avoided the function entirely, but at least some 
> systems are likely to be able to do one single broadcast IPI with 
> this, so it's at least in theory still better to use that 
> smp_call_function_many() function, even though it has braindamaged 
> semantics.

I have no good way to trigger the bug right now: it triggered only 
once (maybe twice) in the past 2 days, and was not repeatable. But 
your patch looks like it should work around the API problem. Should 
Dave or me apply it, or will you?

But the whole code there had a pretty suspicious affinity handling 
to begin with and the cpumask changes just tried to keep the status 
quo (and even had trouble at keeping that ;-) and didnt improve it.

	Ingo

Re: Fix quilt merge error in acpi-cpufreq.c

[H. Peter Anvin]

Ingo Molnar wrote:
> 
> I got Rusty to use it so i'm to blame for this one. A number of 
> developers/maintainers use it now and they find it useful in a 
> number of circumstances, when used judiciously.
> 
> We are using impact lines to judge "practical impact of a commit". 
> The shorter (while still correct and expressive), the better. We are 
> trying to use it in well-defined cases - but not always.
> 
> Here it helped expose the bogosity of a patch more clearly: the 
> intended impact of "clarifying a confusing API" was not met, and the 
> commit became easier to flame.
> 

It's more than that.

Impact: clarify and extend confusing API

... isn't really an impact as much of an action.  This is part of why
it's kind of bogus in this case.  I'm not particularly blaming Rusty on
that, I personally find it's actually really hard to write Impact:
lines, exactly because it is hard to think about the consequences of a
patch -- and it's even harder when the patch is someone else's.

I generally find I spend about three times longer reviewing the same
amount of code from someone who has written Impact: lines on their
patches, mostly because it means they have thought about it.  It does
get reflected in the rest of the patch.  Also, if they aren't there, I
do end up writing them, and quite often find issues in the process.

Of course, in isolation -- and when wrong -- the Impact: lines look
rather stupid.  However, as part of a bigger workflow we've found that
they actually work, mostly because they do work as a forcing function on
both the maintainer and the submitter.

Nothing is perfect, sadly.  It just does seem to be a net win.

	-hpa

-- 
H. Peter Anvin, Intel Open Source Technology Center
I work for Intel.  I don't speak on their behalf.

Re: Fix quilt merge error in acpi-cpufreq.c

[H. Peter Anvin]

Impact: fix meaning reversal

H. Peter Anvin wrote:
> 
> I generally find I spend about three times longer reviewing the same
> amount of code from someone who has written Impact: lines on their
> patches, mostly because it means they have thought about it.  It does
> get reflected in the rest of the patch.  Also, if they aren't there, I
> do end up writing them, and quite often find issues in the process.
> 

That should, of course, have been who has *not* written Impact: lines... ;)

	-hpa

-- 
H. Peter Anvin, Intel Open Source Technology Center
I work for Intel.  I don't speak on their behalf.

Re: Fix quilt merge error in acpi-cpufreq.c

[Linus Torvalds]

On Wed, 15 Apr 2009, Ingo Molnar wrote:
> 
> We are using impact lines to judge "practical impact of a commit". 
> The shorter (while still correct and expressive), the better. We are 
> trying to use it in well-defined cases - but not always.

The thing is, I've seen totally bogus "Impact:" statements.

It just makes things look more official, without actually guaranteeing 
that it's any more correct. For example, I've seen impact statements to 
the effect of "cleanup", when (a) it wasn't and (b) it did other things 
too.

At that point, it's just distraction and wrong.

And in fact, "cleanup" seems to be the most common one. Along with other 
totally useless ones like "fix build" or just "Fix" .

Gaah. 

Doing a

	git log -p --grep=Impact:.*lean 

shows several "cleanuips" that are anything but. Those "cleanups" fix 
build errors or actually change code. The "Impact:" statement is actively 
misleading, adds absolutely _nothing_, and detracts from the real issue 
and the real message.

			Linus

Re: Fix quilt merge error in acpi-cpufreq.c

[H. Peter Anvin]

Linus Torvalds wrote:
> 
> The thing is, I've seen totally bogus "Impact:" statements.
> 
> It just makes things look more official, without actually guaranteeing 
> that it's any more correct. For example, I've seen impact statements to 
> the effect of "cleanup", when (a) it wasn't and (b) it did other things 
> too.
> 
> At that point, it's just distraction and wrong.
> 

It's more than that: it's a red flag; an indication that the committer
didn't pay attention.  We hopefully notice and reject such patches (or
perhaps more frequently, fix their impact lines).  An error is indeed
problematic, but it can (and *should*) be recognized after the fact as
well and can be used to improve maintenance practices.

> And in fact, "cleanup" seems to be the most common one. Along with other 
> totally useless ones like "fix build" or just "Fix" .

"cleanup" is indeed the most common, as it is intended to signify a
trivial but nonzero code change.  Whether or not it's *correct* is
another matter.  "build fix" is valid and proper use: it tells that it
fixes a compilation error, which succinctly communicates both the
priority of the fix and how it needs to be validated.

> shows several "cleanuips" that are anything but. Those "cleanups" fix 
> build errors or actually change code. The "Impact:" statement is actively 
> misleading, adds absolutely _nothing_, and detracts from the real issue 
> and the real message.

One of the problems we do have is the lack of standardization; the
reason for that is that we started this out as an experiment, and
weren't really ready to crack down too hard on the exact usage.  This
of course means, too, that the meaning is slightly different for
different people, and although we can fix that for patches we commit, we
can't do that for other trees.

In short, I think we've gotten to the point that we need a spec in the
Documentation/ directory for this to continue to be useful.

	-hpa

-- 
H. Peter Anvin, Intel Open Source Technology Center
I work for Intel.  I don't speak on their behalf.

Re: Fix quilt merge error in acpi-cpufreq.c

[Linus Torvalds]

On Wed, 15 Apr 2009, H. Peter Anvin wrote:
> 
> "cleanup" is indeed the most common, as it is intended to signify a
> trivial but nonzero code change.  Whether or not it's *correct* is
> another matter.  "build fix" is valid and proper use: it tells that it
> fixes a compilation error, which succinctly communicates both the
> priority of the fix and how it needs to be validated.

Why would that be "proper use"?

Dammit, if the "build fix" is not obvious from the rest of the commit 
message, there's something wrong.

And if it _is_ obvious, then the mechanical "Impact:" thing is pointless.

In other words - in neither case does it actually help anything at all. 
It's only distracting noise.

			Linus

Re: Fix quilt merge error in acpi-cpufreq.c

[Ingo Molnar]

* Linus Torvalds <torvalds@linux-foundation.org> wrote:

> On Wed, 15 Apr 2009, H. Peter Anvin wrote:
> > 
> > "cleanup" is indeed the most common, as it is intended to signify a
> > trivial but nonzero code change.  Whether or not it's *correct* is
> > another matter.  "build fix" is valid and proper use: it tells that it
> > fixes a compilation error, which succinctly communicates both the
> > priority of the fix and how it needs to be validated.
> 
> Why would that be "proper use"?
> 
> Dammit, if the "build fix" is not obvious from the rest of the 
> commit message, there's something wrong.
> 
> And if it _is_ obvious, then the mechanical "Impact:" thing is 
> pointless.
> 
> In other words - in neither case does it actually help anything at 
> all. It's only distracting noise.

I often skip "Impact: build fix" - when it's obvious from the 
subject line or the first sentence of the commit - or if it can be 
made obvious by changing the subject line or by changing the first 
sentence of the commit.

I add it occasionally, when some other, higher priority principle 
makes the changing of the subject line undesired.

For example, yesterday i did this commit:

| commit 27b19565fe4ca5b0e9d2ae98ce4b81ca728bf445
| Author: Ingo Molnar <mingo@elte.hu>
| Date:   Tue Apr 14 11:03:12 2009 +0200
|
|     lockdep: warn about lockdep disabling after kernel taint, fix
|    
|     Impact: build fix for Sparc and s390
|    
|     Stephen Rothwell reported that the Sparc build broke:

I added that 'build fix' impact line for two reasons:

Firstly, because the subject line was inherited from the buggy 
commit and the new subject line got a ", fix" postfix. (This 
convention seems rather useful at times in shortlogs, see below.)

Secondly, i also added the impact line because i wanted to specify 
the architectures affected: Sparc and s390 - this fact was not 
obvious from the bug report context which i wanted to preserve to 
credit the bug reporter prominently (Stephen found the build error 
on Sparc only).

Another option would have been to use this primary subject line 
instead:

   fix build error on Sparc and s390

But IMHO that's a worse subject line. It's more important to keep 
the flow of the original change intact. The subject lines cluster up 
better in shortlogs or in git logs:

 $ gll include/linux/debug_locks.h 
 27b1956: lockdep: warn about lockdep disabling after kernel taint, fix
 9eeba61: lockdep: warn about lockdep disabling after kernel taint

The connection between the two commits is plain obvious, at a 
glance.

I could have concatenated the first subject line with the impact 
information:

 27b1956: lockdep: warn about lockdep disabling after kernel taint, fix build error on Sparc and s390

... but this is clearly over-long and dillutes the subject line with 
'effect' information.

	Ingo

Re: Fix quilt merge error in acpi-cpufreq.c

[Andrew Morton]

On Wed, 15 Apr 2009 12:43:02 -0700 (PDT)
Linus Torvalds <torvalds@linux-foundation.org> wrote:

> 
> 
> On Wed, 15 Apr 2009, H. Peter Anvin wrote:
> > 
> > "cleanup" is indeed the most common, as it is intended to signify a
> > trivial but nonzero code change.  Whether or not it's *correct* is
> > another matter.  "build fix" is valid and proper use: it tells that it
> > fixes a compilation error, which succinctly communicates both the
> > priority of the fix and how it needs to be validated.
> 
> Why would that be "proper use"?
> 
> Dammit, if the "build fix" is not obvious from the rest of the commit 
> message, there's something wrong.
> 
> And if it _is_ obvious, then the mechanical "Impact:" thing is pointless.
> 
> In other words - in neither case does it actually help anything at all. 
> It's only distracting noise.
> 

I'm getting quite a few Impact:s now and I must say that the Impact:
line is always duplicative of the Subject:.  Except in a few cases, and
that's because the Subject: sucked.

But I leave the Impact: lines in there because I'm nice.

Re: Fix quilt merge error in acpi-cpufreq.c

[Ingo Molnar]

* Andrew Morton <akpm@linux-foundation.org> wrote:

> On Wed, 15 Apr 2009 12:43:02 -0700 (PDT)
> Linus Torvalds <torvalds@linux-foundation.org> wrote:
> 
> > 
> > 
> > On Wed, 15 Apr 2009, H. Peter Anvin wrote:
> > > 
> > > "cleanup" is indeed the most common, as it is intended to signify a
> > > trivial but nonzero code change.  Whether or not it's *correct* is
> > > another matter.  "build fix" is valid and proper use: it tells that it
> > > fixes a compilation error, which succinctly communicates both the
> > > priority of the fix and how it needs to be validated.
> > 
> > Why would that be "proper use"?
> > 
> > Dammit, if the "build fix" is not obvious from the rest of the commit 
> > message, there's something wrong.
> > 
> > And if it _is_ obvious, then the mechanical "Impact:" thing is pointless.
> > 
> > In other words - in neither case does it actually help anything at all. 
> > It's only distracting noise.
> > 
> 
> I'm getting quite a few Impact:s now and I must say that the 
> Impact: line is always duplicative of the Subject:.  Except in a 
> few cases, and that's because the Subject: sucked.
> 
> But I leave the Impact: lines in there because I'm nice.

I looked at the current .30 cycle up to latest -git and i found only 
five commits out of 584 that had your signoff (most went upstream 
via you) which also had Impact lines:

| commit 3d26dcf7679c5cc6c9f3b95ffdb2152fba2b7fae
| Author: Andi Kleen <andi@firstfloor.org>
| Date:   Mon Apr 13 14:40:08 2009 -0700
|
|    kernel/sys.c: clean up sys_shutdown exit path
|    
|    Impact: cleanup, fix
|
| Clean up sys_shutdown() exit path.  Factor out common code.  Return
| correct error code instead of always 0 on failure.

Impact line exposes wrong patch structure: cleanup should never be 
mixed with fix.

Impact line is not duplicative of subject line - it correctly 
mentions that there are side-effects. (sys_shutdown() changes its 
return code)

The subject line is thus wrong.

| commit 0769c2981495c3d05429840d6fc7a1b5e26accaa
| Author: Andi Kleen <andi@firstfloor.org>
| Date:   Mon Apr 13 14:39:56 2009 -0700
|
|     asm-generic/siginfo.h: update NSIGTRAP definition
|    
|     Impact: (nearly) trivial

impact line somewhat atypical but correct - the patch is a cleanup 
but might affect user-space.

Impact line is not duplicative of subject line.

| commit 6b44003e5ca66a3fffeb5bc90f40ada2c4340896
| Author: Andrew Morton <akpm@linux-foundation.org>
| Date:   Thu Apr 9 09:50:37 2009 -0600
|
|     work_on_cpu(): rewrite it to create a kernel thread on demand
|    
|     Impact: circular locking bugfix

Impact line is correct.

You wrote a fix - Rusty was nice in keeping your subject line and 
enhanced the commit with a non-trivial impact line.

[ Btw., this commit also had an unintended impact: it caused a 10% 
  mysqld performance drop on certain systems. When this commit was 
  bisected to later on it was immediately obvious from the impact 
  line that this side-effect was not intended when the patch was
  written. ]

Impact line is not duplicative of subject line.

| commit acdd052a285a7b4cc7da4fa7d34ef9fd0a67b2f8
| Author: Hannes Eder <hannes@hanneseder.net>
| Date:   Tue Mar 31 15:23:50 2009 -0700
|
|     init/main.c: fix sparse warnings: context imbalance
|     
|     Impact: Attribute function 'init_post' with __releases(...).

Impact line is incorrect (describes action not effect).

Impact line is not duplicative of subject line.

| commit ee99c71c59f897436ec65debb99372b3146f9985
| Author: KOSAKI Motohiro <kosaki.motohiro@jp.fujitsu.com>
| Date:   Tue Mar 31 15:19:31 2009 -0700
|
|     mm: introduce for_each_populated_zone() macro
|    
|     Impact: cleanup

Impact line is correct and appropriate.

Impact line is not duplicative of subject line.

So, here are my conclusions:

- only 0.85% of the commits you were involved with in this cycle had 
  an impact line.

- out of 5 cases, 4 had correct impact lines, despite _you_ 
  admittedly not liking them and not caring about them. That's a 
  pretty good ratio IMO. Impact lines should be handled by the
  maintainer. You should not pass them through - just erase them
  please - just like you erase or change other parts of changelogs 
  you dont like.

- i one out of the 5 cases, you could have detected a slightly
  suboptimal patch structure just based on the (truthful) impact
  line. But you dont use the impact lines so you missed this detail.

- in one out of the 5 cases, the impact line was used later on 
  during bug analysis. _I_ remember having looked at that impact 
  line and saw that he performance regression was clearly not 
  anticipated. Could i have recovered that information in some other 
  way? Yes, i could have mailed you, or i could have made a guess 
  out of other context data. But the impact line told me for sure.

- i'm not sure on what you base your observation that the impact
  lines you get are "always duplicative of the Subject:.". It wasnt
  duplicative in any of the above cases.

So i'm puzzled really. This stuff is clearly useful to us every day, 
it shows up in only 0.85% of your commits (because you let them 
through - you could erase them) but clearly if both Linus and you 
are against it what can we do? :-(

	Ingo

Re: Fix quilt merge error in acpi-cpufreq.c

[Linus Torvalds]

On Wed, 15 Apr 2009, Ingo Molnar wrote:
> 
> Impact line exposes wrong patch structure: cleanup should never be 
> mixed with fix.
>
> impact line somewhat atypical but correct - the patch is a cleanup 
> but might affect user-space.
>
> Impact line is correct.
> 
> Impact line is not duplicative of subject line.
>
> Impact line is incorrect (describes action not effect).
> 
> Impact line is correct and appropriate.

Bah. 

In _no_ case did the Impact: line actually say anything worth saying, and 
it was there just for self-gratification. 

> - only 0.85% of the commits you were involved with in this cycle had 
>   an impact line.
> 
> - out of 5 cases, 4 had correct impact lines, despite _you_ 
>   admittedly not liking them and not caring about them.

Just about NOBODY cares about "correct".

I care about this "mental masturbation" part, where somebody decided to 
start marking commits with some inane logic that makes no sense.

Instead of havign that IDIOTIC "Impact:" marker, why not just write good 
commit messages?

That's the issue. Those things have no meaning. 

Quite frankly, your argument of using "grep" on those things for 
management is total crap. It would make sense if they were meaningful and 
ubiquotous, but neither of those are actually true.

And your arguments are really so _incredibly_ dishonest that I don't see 
how you can't not see that yourself. Let's quote one:

> |     lockdep: warn about lockdep disabling after kernel taint, fix
> |    
> |     Impact: build fix for Sparc and s390
> |    
> |     Stephen Rothwell reported that the Sparc build broke:
> 
> I added that 'build fix' impact line for two reasons:
> 
> Firstly, because the subject line was inherited from the buggy 
> commit and the new subject line got a ", fix" postfix. (This 
> convention seems rather useful at times in shortlogs, see below.)

THIS counts as an argument for adding an "Impact:" line?

Come on - sure, it's worth mentioning that the patch is a build fix, but
that should obviously have been there regardless of any "Impact:" line. 
So your whole argument is based on the fact that you added a (good) piece 
of information, but you ignore the fact that that good piece of 
information had NOTHING WHAT-SO-EVER to do with the "Impact" line. It 
should have been there regardless.

		Linus

Re: Fix quilt merge error in acpi-cpufreq.c

[Andrew Morton]

On Wed, 15 Apr 2009 23:03:53 +0200
Ingo Molnar <mingo@elte.hu> wrote:

> what can we do?

Write better patch titles?

Re: Fix quilt merge error in acpi-cpufreq.c

[David Miller]

From: Linus Torvalds <torvalds@linux-foundation.org>
Date: Wed, 15 Apr 2009 12:43:02 -0700 (PDT)

> And if it _is_ obvious, then the mechanical "Impact:" thing is pointless.
> 
> In other words - in neither case does it actually help anything at all. 
> It's only distracting noise.

FWIW I find the Impact: blurbs highly annoying too.  Just freakin'
say what the damn patch does in the commit message.

If a person can't be bothered to skim the commit message text,
this Impact: tag only gives them a false sense of understanding
what the change does.

Re: Fix quilt merge error in acpi-cpufreq.c

[Ingo Molnar]

* Linus Torvalds <torvalds@linux-foundation.org> wrote:

> On Wed, 15 Apr 2009, Ingo Molnar wrote:
> > 
> > Impact line exposes wrong patch structure: cleanup should never be 
> > mixed with fix.
> >
> > impact line somewhat atypical but correct - the patch is a cleanup 
> > but might affect user-space.
> >
> > Impact line is correct.
> > 
> > Impact line is not duplicative of subject line.
> >
> > Impact line is incorrect (describes action not effect).
> > 
> > Impact line is correct and appropriate.
> 
> Bah. 
> 
> In _no_ case did the Impact: line actually say anything worth 
> saying, and it was there just for self-gratification.

As i said it in the mail, i actually used the impact line of commit 
6b44003e5ca66 ("work_on_cpu(): rewrite it to create a kernel thread 
on demand") later on, when a regression was caused by that commit.

It is the third entry above.

	Ingo

Re: Fix quilt merge error in acpi-cpufreq.c

[Ingo Molnar]

* David Miller <davem@davemloft.net> wrote:

> From: Linus Torvalds <torvalds@linux-foundation.org>
> Date: Wed, 15 Apr 2009 12:43:02 -0700 (PDT)
> 
> > And if it _is_ obvious, then the mechanical "Impact:" thing is pointless.
> > 
> > In other words - in neither case does it actually help anything at all. 
> > It's only distracting noise.
> 
> FWIW I find the Impact: blurbs highly annoying too.  Just freakin' 
> say what the damn patch does in the commit message.

Just curious: have you tried to use them over a couple of days, just 
to check whether your first read-only impression is correct, that 
they are just annoying blurbs with no other effects? (you might have 
- I dont know.)

> If a person can't be bothered to skim the commit message text, 
> this Impact: tag only gives them a false sense of understanding 
> what the change does.

So do you consider it wrong to summarize impact? Does this argument 
extend to other summaries as well, such as the title itself? Or is 
your argument that there should be only a single kind of summary in 
a commit - the title itself?

Also, would it be wrong for people to be able to skim commit logs 
only for 'interesting looking' commits, by using impact tags? Should 
they be 'punished' by us obfuscating away summaries intentionally?

Also, do you think hpa was not telling the truth when he said that 
it is much faster for him to review patches that have an impact 
line? Do you think i am not telling the truth for reporting the same 
experience? Isnt speed and effiency of review something we should be 
happy to improve?

	Ingo

Re: Fix quilt merge error in acpi-cpufreq.c

[Ingo Molnar]

* Andrew Morton <akpm@linux-foundation.org> wrote:

> On Wed, 15 Apr 2009 23:03:53 +0200
> Ingo Molnar <mingo@elte.hu> wrote:
> 
> > what can we do?
> 
> Write better patch titles?

Lets start with a patch title that you wrote:

| commit 6b44003e5ca66a3fffeb5bc90f40ada2c4340896
| Author: Andrew Morton <akpm@linux-foundation.org>
| Date:   Thu Apr 9 09:50:37 2009 -0600
|
|     work_on_cpu(): rewrite it to create a kernel thread on demand
|
|     Impact: circular locking bugfix

... and to which Rusty added an impact line.

Where is the impact in that title you wrote? How could it have been 
written differently to include the impact? Why didnt you give it a 
better title? How can you expect us to do a better title if you 
yourself dont do titles with impact information?

If the impact cannot be included, can it be included elsewhere in 
the commit perhaps? If it can be included elsewhere in the commit, 
is it allowed to be summarized in a structured way, or must it stay 
mixed into a free-flowing text description?

	Ingo

Re: Fix quilt merge error in acpi-cpufreq.c

[Linus Torvalds]

On Thu, 16 Apr 2009, Ingo Molnar wrote:
>
> As i said it in the mail, i actually used the impact line of commit 
> 6b44003e5ca66 ("work_on_cpu(): rewrite it to create a kernel thread 
> on demand") later on, when a regression was caused by that commit.

Duh. You keep on repeating that idiotic argument.

The "Impact:" part had nothing what-so-ever to do with what you argue for.

I'm not arguing against good commit messages. I'm arguing against the 
"Impact:" part. It's pointless.

ALL of the commit message is (hopefully) about important things. If you 
want to narrow it down to one single line, that's just WRONG.

		Linus

Re: Fix quilt merge error in acpi-cpufreq.c

[Linus Torvalds]

On Thu, 16 Apr 2009, Ingo Molnar wrote:
> 
> So do you consider it wrong to summarize impact?

No.

NOBODY is arguing against talking about what the thing does.

We're arguing against the string "Impact:", which is nonsensical.

> Does this argument extend to other summaries as well, such as the title 
> itself?

Umm. The summary line that doesn't have such a made-up nonsensical prefix?

Ingo, you're missing the _point_.

Summaries and good description of patches are GOOD.

The "Impact:" string is just noise.

Talk about how it was a cleanup all you want, and by all means talk about 
what the intention of it was. Nobody argues against that. What we argue 
against is ugly language. 

Describe the changes in real sentences.

		Linus

Re: Fix quilt merge error in acpi-cpufreq.c

[David Miller]

From: Linus Torvalds <torvalds@linux-foundation.org>
Date: Wed, 15 Apr 2009 14:15:45 -0700 (PDT)

> Instead of havign that IDIOTIC "Impact:" marker, why not just write good 
> commit messages?

Amen.

Re: Fix quilt merge error in acpi-cpufreq.c

[Ingo Molnar]

* Linus Torvalds <torvalds@linux-foundation.org> wrote:

> On Thu, 16 Apr 2009, Ingo Molnar wrote:
> >
> > As i said it in the mail, i actually used the impact line of 
> > commit 6b44003e5ca66 ("work_on_cpu(): rewrite it to create a 
> > kernel thread on demand") later on, when a regression was caused 
> > by that commit.
> 
> Duh. You keep on repeating that idiotic argument.
> 
> The "Impact:" part had nothing what-so-ever to do with what you 
> argue for.
> 
> I'm not arguing against good commit messages. I'm arguing against 
> the "Impact:" part. It's pointless.
> 
> ALL of the commit message is (hopefully) about important things. 
> If you want to narrow it down to one single line, that's just 
> WRONG.

Ok, i see your argument.

Is there any other way for us to get the benefits of the impact 
line, without actually adding one?

They are:

 - Better split-up patches from contributors.

 - Increased maintainer and developer attention on the effects of
   patches.

 - Less time we have to spend on patches we get with impact-lines.
   Both hpa and me reported this.

 - easy regression post-mortems

What i dont understand is how you can dismiss these positive points 
so easily and only concentrate on the negative points - these 
effects are mostly visible to those creating impact lines - not to 
you. You cannot really have seen any of these effects without having 
done impact lines for a few days.

(
  Okay, you are perhaps an exception, you _do_ write fantastic
  commit logs that generally need no 'stinkin impact line.

  There's exceptions though, even with your commits. I wish you had 
  added one to ea34f43a for example, which you committed today.

  It is not clear from that commit log at all what the practical
  relevance of your fix is.

  I suspect it fixes Ali Gholami Rudi's problem of his CPU hitting
  50^C till the fan turns on.

  I'd probably have added the following impact line (although i'd
  have first asked Ali whether this is the precise impact the fix
  had on him - it's not 100% clear from the discussion):

    Impact: fix cpufreq misbehavior causing high CPU temperature

  Does this information matter? I think it does. Does it matter 
  _more_ than the rest of the commit log? I think, to most Linux
  users, it does. That's one of the reasons why we are experimenting 
  with formalized this kind of information. It's important 
  information and it should not be forgotten from commit logs.

  As a developer, while writing up a commit log it is _so_ easy to
  get lost in the details of the 'how' and 'why' - and not emit
  basic information: why do people care? What were the bad
  _practical_ effects of the bug that are fixed here?

  It is basic human nature to get lost in that - even for the best.
)

	Ingo

Re: Fix quilt merge error in acpi-cpufreq.c

[Linus Torvalds]

On Thu, 16 Apr 2009, Ingo Molnar wrote:
> 
> Is there any other way for us to get the benefits of the impact 
> line, without actually adding one?
> 
> They are:
> 
>  - Better split-up patches from contributors.
> 
>  - Increased maintainer and developer attention on the effects of
>    patches.
> 
>  - Less time we have to spend on patches we get with impact-lines.
>    Both hpa and me reported this.
> 
>  - easy regression post-mortems

None of this has anything to do with "Impact:" per se, and everything to 
do with just trying to tach people to talk about their patches better.

By all means, when you see a patch that doesn't describe what it does or 
why it does so, ask people to say so.

Ask people "What's the impact of this patch?"

But don't ask them to then say

	Impact: cleanup.

Teach them to say

	This cleans up the code by doing xyz.

and yes, by all means teach people to write succint and to-the-point 
messages.

Teach people to not write a novel, but instead give them rules like:

 - the subject line has to descibe the over-all patch.

 - Then write a line or two that is about what the patch really does

 - if needed, write a longer detailed explanation with actual error 
   messages that it fixes etc.

Now THAT is somethign that I think we can all get behind. Example of a 
good patch log:

    kprobes: move EXPORT_SYMBOL_GPL just after function definitions
    
    Clean up positions of EXPORT_SYMBOL_GPL in kernel/kprobes.c according to
    checkpatch.pl.

wouldn't you agree? Adding a "Impact: cleanup" wouldn't do anything to it, 
except make it not read as good English any more.

I like seeing explanations that read well. 

			Linus

Re: Fix quilt merge error in acpi-cpufreq.c

[Linus Torvalds]

On Wed, 15 Apr 2009, Linus Torvalds wrote:
> 
> wouldn't you agree? Adding a "Impact: cleanup" wouldn't do anything to it, 
> except make it not read as good English any more.
> 
> I like seeing explanations that read well. 

Btw, I suspect that if the "Impact:" line is at the end, along with the 
"Signed-off-by:" etc lines, it wouldn't be as disturbing and I wouldn't 
react as negatively to it.

At least then it wouldn't break up the narrative, and it would kind of fit 
with all the other "tagged" lines.

			Linus

Re: Fix quilt merge error in acpi-cpufreq.c

[Ingo Molnar]

* Linus Torvalds <torvalds@linux-foundation.org> wrote:

> On Thu, 16 Apr 2009, Ingo Molnar wrote:
> > 
> > So do you consider it wrong to summarize impact?
> 
> No.
> 
> NOBODY is arguing against talking about what the thing does.
> 
> We're arguing against the string "Impact:", which is nonsensical.
> 
> > Does this argument extend to other summaries as well, such as 
> > the title itself?
> 
> Umm. The summary line that doesn't have such a made-up nonsensical 
> prefix?
> 
> Ingo, you're missing the _point_.
> 
> Summaries and good description of patches are GOOD.
> 
> The "Impact:" string is just noise.
> 
> Talk about how it was a cleanup all you want, and by all means 
> talk about what the intention of it was. Nobody argues against 
> that. What we argue against is ugly language.
> 
> Describe the changes in real sentences.

So would a:

   The impact of this change: it is a pure cleanup.

Formalization be acceptable? We tried to make it short and fit into 
a single line - but we can certainly make it longer.

Btw., we have a _lot_ of 'summary' tags in commit logs already, all 
over the place. Added by you, me and everyone who commits changes 
into Linux.

They might not be nearly as intrusive and repulsive to you as the 
now infamous "Impact: " line, but they are all over the place and 
boy are they ugly to any sane person on this planet - you just got 
used to them already.

Starting with the title:

      x86: rename .i assembler includes to .h
      x86: add instrumentation menu

That 'x86:' prefix is a summary and a prefix string. We dont say 
what the English, Finnish, Swedish, Hungarian or German teacher 
taught us to be proper language:

      The x86 architecture code: rename .i assembler includes to .h

      Den x86 arkitekturen koden: ge nytt namn .i assembler omfattar till .h 

Because we found that too long, and because after a few months of 
getting used to everyone parses "x86: " prefixes automatically.

Just like we learned to parse "fuse: " prefixes a few years ago and 
do it sub-consciously now - despite it being arguably bad language 
(what is there to fuse??).

In fact, the .i and .h abbreviations are summarized information 
meaningless to anyone outside of this field. Often we use language 
variants specific to one Linux subsystem. Worklet? Syslet? skb? vma? 
Signed-off-by? Cc:? RIP? ALSA?

We sometimes paste formal descriptions into changelogs, when it 
serves us well:

    // <smpl>
    @disable is_null@
    identifier f;
    expression E;
    identifier fld;
    statement S;
    @@
    
    + if (E == NULL) S
      f(...,E->fld,...);
    - if (E == NULL) S

And abbreviations, summaries and ways of expressions evolve. They 
evolve when people start using them in new ways. Does it happen 
often? Yes, it does. Can it be bogus and harmful? Yes, it can be and 
most often it _is_ bogus, so scrutinizing it is correct.

So the real question is not artificial abbreviations, but the 
_level_, _style_, _efficiency_ and _placement_ of such summarizing, 
and whether you accept an "Impact: " prefix as a meaningful and 
worthwile way to summarize a given category of information. You 
clearly dont, and i dont for a minute dispute that you find it 
genuinely ugly, and i'm not ignoring your view about that.

You just dont seem to understand why i find it useful. You also seem 
to try to deprive us the basic right of creating new, field-specific 
language variants we find useful in our everyday work. And that 
sucks.

q.e.d.

Oops, i should have said, quod erat demonstrandum.

	Ingo

Re: Fix quilt merge error in acpi-cpufreq.c

[Ingo Molnar]

* Linus Torvalds <torvalds@linux-foundation.org> wrote:

> On Wed, 15 Apr 2009, Linus Torvalds wrote:
> > 
> > wouldn't you agree? Adding a "Impact: cleanup" wouldn't do 
> > anything to it, except make it not read as good English any 
> > more.
> > 
> > I like seeing explanations that read well.
> 
> Btw, I suspect that if the "Impact:" line is at the end, along 
> with the "Signed-off-by:" etc lines, it wouldn't be as disturbing 
> and I wouldn't react as negatively to it.
> 
> At least then it wouldn't break up the narrative, and it would 
> kind of fit with all the other "tagged" lines.

Yeah, that makes sense.

Or perhaps make it fit into the narrative but still have a 
recognizable structure to make sure it cannot be forgotten?

Something like:

 The impact of this change is that the build is fixed.
 The impact of this change is that the code gets cleaner.
 The impact of this change is that the CPU does not overheat.

although this sounds extremely artificial, circumvent and childish 
to me :-/

I guess i'm infected with the "Impact: " language already and my 
brain resists lingual inefficiencies ;-/ We've been using it for 
almost a year meanwhile.

	Ingo

Re: Fix quilt merge error in acpi-cpufreq.c

[Linus Torvalds]

On Thu, 16 Apr 2009, Ingo Molnar wrote:
> 
> So would a:
> 
>    The impact of this change: it is a pure cleanup.
> 
> Formalization be acceptable? We tried to make it short and fit into 
> a single line - but we can certainly make it longer.

Language is not a "fit to one string" thing.

It doesn't matter what the string is: if you haev a fixed format, it's not 
human language. It's that easy.

Don't make up stupid fixed-format rules, and most certainly don't make 
them make the real payload harder to tread.

If people write "The impact of this is to fix compilation", then that's 
fine as an occasional thing, although it's damned stilted English, and I 
certainly wouldn't ever recommend it. Why not just say

	This fixes a compile problem in file so-and-so.

instead? Much clearer to everybody.

> You just dont seem to understand why i find it useful. You also seem 
> to try to deprive us the basic right of creating new, field-specific 
> language variants we find useful in our everyday work. And that 
> sucks.

YOU HAVE NEVER GIVEN A COHERENT REASON FOR FINDING IT USEFUL!

Yes, you bring up the same reason every time: namely that you want to know 
what the patch does. But never _once_ have you given a reason fo why that 
fixed-format string helps at all.

That's what i've been trying to tell you: don't use stilted and artificial 
fixed formats where it doesn't make sense.

Instead of

	Impact: cleanup

just write

	Cleanup xyz by doing abc

which is MORE informational, and much easier for everybody to read.

That's all I'm asking. Stop making the commit messages harder to read, 
uglier, and less informative. 

[ And if it's purely for "grep", then you can damn well hide it at the end 
  where nobody sane cares any more, but you seem to not have been able to 
  acknowledge that if it's for 'grep', then you're _missing_ all the 
  changes that don't have that string in the first place, so that's not a 
  very good reason _either_. ]

			Linus

Re: Fix quilt merge error in acpi-cpufreq.c

[Rusty Russell]

On Thu, 16 Apr 2009 12:58:45 am Linus Torvalds wrote:
> 
> On Wed, 15 Apr 2009, Rusty Russell wrote:
> > 
> >    The API is screwy.  It excludes the current CPU from the mask,
> > unconditionally.  It's a tlb flush helper masquerading as a general function.
> > 
> > (smp_call_function has the same issue).
> > 
> > Something like this?
> > 
> > Subject: smp_call_function_many: add explicit exclude_self flag
> 
> No. This just makes the API even screwier. It fixes the 
> "smp_processor_id()" thing, but it is 
> 
>  (a) horribly buggy

Sure.  Did it even compile?

>      Those 
> 
> 	if (exclude_self && cpu == this_cpu)
> 		cpu = cpumask_next_and(cpu, mask, cpu_online_mask);
> 
>      things are wrong - we need to do that "jump over our own CPU" thing 
>      regardless of whether 'exclude_self' is set or not, since we're going 
>      to special-case our own CPU anyway.

I don't think so (smp_call_function_single will DTRT if 
cpu == smp_processor_id).  But I didn't test to be sure.

>  (c) Wrong, even if it wasn't (horribly buggy)^2
> 
>      Adding "flags" to an interface doesn't make it better. Quite the 
>      reverse. It makes it worse.

Uglier.  Worse?  It would have prevented Andrew's mistake.

>      It also makes it even MORE different from 
>      all the other smp_call_function's, which just do the 'self' cpu 
>      without any stupid conditionals and flags.

You've said this twice, but unfortunately that doesn't make it true.

smp_call_function() is the original from which this derives, and it has
always skipped the current cpu.  Hence on_each_cpu().

I'd love to see a fix which isn't ugly and doesn't put a cpumask on the
stack.

> > Impact: clarify and extend confusing API
> 
> And what the hell is up with these bogus "Impact:" things? Who started 
> doing that, and why?

Ingo wants them.  Example:

	lguest: don't expect linear addresses in gdt pvops

	Impact: fix guest crash 'lguest: bad read address 0x4800000 len 256'

What's more important in the subject line?  That it fixes a crash, or what it
does?

Thanks,
Rusty.

Re: Fix quilt merge error in acpi-cpufreq.c

[Ingo Molnar]

* Linus Torvalds <torvalds@linux-foundation.org> wrote:

> > You just dont seem to understand why i find it useful. You also 
> > seem to try to deprive us the basic right of creating new, 
> > field-specific language variants we find useful in our everyday 
> > work. And that sucks.
> 
> YOU HAVE NEVER GIVEN A COHERENT REASON FOR FINDING IT USEFUL!
> 
> Yes, you bring up the same reason every time: namely that you want 
> to know what the patch does. But never _once_ have you given a 
> reason fo why that fixed-format string helps at all.

For similar reasons people have memos, stick-it's and other formal, 
automated, reflex-alike daily routine measures to make sure they 
dont forget to do something they all too easily forget otherwise.

If i apply a patch i always notice the ones without an impact line 
(it's missing from the visual appearance of a commit - just like it 
is _looking ugly_ to you - just inverted), and i dont apply one 
without either:

  1) convincing myself it does not need one
  2) or adding one

It also sticks out like a sore thumb if it's incorrect. For example 
"Impact: fix" is a bad one at a glance.

This kind of formal measure _forces_ the extraction of this very 
specific type of summary on all sides of the contribution chain - 
and it drastically reduced the number of commits i regretted in 
hindsight.

It also speeds up patch processing because seeing an impact line i 
only have to scan for code patterns contradicting that claim - 
instead of doing general scanning figuring out the type of the patch 
- then doing a second pass figuring out whether it truly matches 
that expectation. I can also prioritize and order incoming patches 
much easier if i have a rough expected impact analysis. If a list of 
patches claims to be complex i'll put it in the appropriate section 
of the day.

The number of contributors who can write meaningful changelogs or 
who can be taught to write really good changelogs is very, very low. 
I'd guesstimate somewhere around 5% of all Linux contributors. (The 
guesstimation is probably on the more generous side.)

The central problem, as i see it, is about having people with two 
rare skills:

 - good coding abilities
 - good documentation/expression abilities

Both skills are unlikely to begin with - and it is two unlikely 
factors combined, and to find a person with both skills at once is 
unlikely square two.

In fact they are even less likely to combine in the same person, for 
the following reason: both capabilities reside in the left half of 
the brain, and an over-developed skill in one activity such as 
programming supresses other activities like linguistic abilities.

It happened not once and not twice to me in the past that after a 
night spent coding i was unable to properly order a burger or some 
other meal. I was still seeing code everywere and was thinking in 
code literally - language and talking was far away.

Yes, people combining both skills still exist, and they are often 
maintainers. In fact maintainers dont spent a lot of time _writing_ 
code, so their linguistic abilities tend to be very good. It is not 
that they are not good coders - they still are - but they dont do 
extreme amounts of programming that supresses linguistic skills.

So basically your argument, as i see it, boils down to the following 
end result in practice: that maintainers should write all or most of 
the changelogs. We simplified that down to: 'maintainers write a 
single line impact summary - and try to keep the rest of the commit 
as tidy as possible'. Anything more involved than that just doesnt 
scale.

Show me one person _you_ actually taught to write good changelogs - 
just one person who was not a natural born talker to begin with. 
I'll show you a 100 other people who cannot write good commit logs. 
They'll try and will limp along, but generally they cannot.

They might not even have English as their mother tongue - but still 
can read and understand C fantastically.

I really dont know what the good solution and the good balance here 
is, i only see a lot of conflicting requirements which look 
impossible to meet.

	Ingo

Re: Fix quilt merge error in acpi-cpufreq.c

[Rusty Russell]

On Thu, 16 Apr 2009 04:17:49 am H. Peter Anvin wrote:
> Linus Torvalds wrote:
> "build fix" is valid and proper use: it tells that it
> fixes a compilation error, which succinctly communicates both the
> priority of the fix and how it needs to be validated.

Side note: I really prefer to see the compile error output in this case: great
for googling.  It annoys me when people skip this.

Anyway, Impact: had lead me to think harder about my messages than the
free-form commit style did.  Perhaps it's too rigid, but it helped.

Let's get concrete.  Here's the top 3 non-merge commits in gitk:

    ALSA: hda - Fix the cmd cache keys for amp verbs
    
    Fix the key value generation for get/set amp verbs.  The upper bits of
    the parameter have to be combined with the verb value to be unique for
    each direction/index of amp access.
    
    This fixes the resume problem on some hardwares like Macbook after
    the channel mode is changed.

I have no idea what this patch does.  It seems to be a fix; what are the
symptoms of the problem, and how long has it been there? 

    ALSA: add missing definitions(letters) to HD-Audio.txt
    
    impact: Add missing definitions(letters).

This is actually a pure documentation patch.  "Fix typos" or "Documentation
fixes" would seem sufficient for subject, and no body needed.

    ALSA: sound/pci: use memdup_user()
    
    Remove open-coded memdup_user().

Again, the body seems gratuitous.

Anyone want to try to write a guide on writing good commit messages?
Rusty.

Re: Fix quilt merge error in acpi-cpufreq.c

[Paul Gortmaker]

On Wed, Apr 15, 2009 at 10:00 PM, Rusty Russell <rusty@rustcorp.com.au> wrote:
> On Thu, 16 Apr 2009 04:17:49 am H. Peter Anvin wrote:
>> Linus Torvalds wrote:
>> "build fix" is valid and proper use: it tells that it
>> fixes a compilation error, which succinctly communicates both the
>> priority of the fix and how it needs to be validated.
>
> Side note: I really prefer to see the compile error output in this case: great
> for googling.  It annoys me when people skip this.
>
> Anyway, Impact: had lead me to think harder about my messages than the
> free-form commit style did.  Perhaps it's too rigid, but it helped.
>
> Let's get concrete.  Here's the top 3 non-merge commits in gitk:
>
>    ALSA: hda - Fix the cmd cache keys for amp verbs
>
>    Fix the key value generation for get/set amp verbs.  The upper bits of
>    the parameter have to be combined with the verb value to be unique for
>    each direction/index of amp access.
>
>    This fixes the resume problem on some hardwares like Macbook after
>    the channel mode is changed.
>
> I have no idea what this patch does.  It seems to be a fix; what are the
> symptoms of the problem, and how long has it been there?
>
>    ALSA: add missing definitions(letters) to HD-Audio.txt
>
>    impact: Add missing definitions(letters).
>
> This is actually a pure documentation patch.  "Fix typos" or "Documentation
> fixes" would seem sufficient for subject, and no body needed.
>
>    ALSA: sound/pci: use memdup_user()
>
>    Remove open-coded memdup_user().
>
> Again, the body seems gratuitous.
>
> Anyone want to try to write a guide on writing good commit messages?

I'd put together some notes on making good commit messages for
internal use, trying to guide people into describing the symptoms of a
problem and why what is being submitted is the correct technical fix
-- vs. the semi-useless cop-outs of an English translation of the C
code itself (i.e. "increment foo comparison by one" or similar).

It isn't anything near a big wordy guide in its own right (and
hopefully it doesn't need to be), but I'm willing to use it as a
start, plus bits from this discussion, and once folks agree on the
content I put together being OK, we can stick it in Documentation
SubmittingPatches or wherever seems appropriate.  Assuming people in
general think there is a need for it...

Paul.

> Rusty.
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/
>

Re: Fix quilt merge error in acpi-cpufreq.c

[Linus Torvalds]

On Thu, 16 Apr 2009, Ingo Molnar wrote:
> 
> Show me one person _you_ actually taught to write good changelogs - 
> just one person who was not a natural born talker to begin with. 
> I'll show you a 100 other people who cannot write good commit logs. 
> They'll try and will limp along, but generally they cannot.
> 
> They might not even have English as their mother tongue - but still 
> can read and understand C fantastically.

So?

The fix for that is not to write crap English. The fix for that is to help 
them, and/or just fix their comments for them.

I really don't see the point of your argument. "People don't always write 
good and complete sentences" is _not_ an argument for then making that a 
standard. 

Just fix things up. Edit their emails. I do. Andrew does. Yes, and despite 
that some commits will still have odd grammar or otherwise not be the 
great novel of the century, and that's not the point. But we should 
_improve_ on the language for people who aren't native English speakers, 
not devolve it to something weaker.

			Linus

Re: Fix quilt merge error in acpi-cpufreq.c

[Theodore Tso]

On Thu, Apr 16, 2009 at 10:57:05AM +0930, Rusty Russell wrote:
> Ingo wants them.  Example:
> 
> 	lguest: don't expect linear addresses in gdt pvops
> 
> 	Impact: fix guest crash 'lguest: bad read address 0x4800000 len 256'
> 
> What's more important in the subject line?  That it fixes a crash, or what it
> does?

Well, consider that 2 or 3 months later, when we're trying to find a
potential commit (say, because we're trying to find a potential fix
that needs to be forward ported to a distro kernel, or some such), the
initial summary line is what is going to be visible in gitk or via
"git log --oneline" (or "git log --pretty=oneline --abbrev-commit" for
older git versions).

So when I try to create git log messages, I try to make the first line
useful for folks who might be sorting through potentially thousands of
patches via gitk or git log --oneline.  So I might do something like

lguest: fix crash caused by expecting linear address in gdt pvops

and then making sure the body of the message goes into detail about
the oops message so that someone who is searching "git log" might find
the commit.  I also try to include relevant bugzilla numbers (for
distro's as well as the kernel bugzilla system).[1]

I think it was you who once quoted Will Strunk, "Always write with
deep sympathy for the reader"?  This definitely applies to git commit
messages, both the initial one-line summary as well as the body.

[1] I tend to use "Addresses-Debian-bug: #12345" in e2fsprogs git
repository, but the convention in the kernel commit logs seems to be
to use the URL to the bugzilla entry.  Since it's stuff that's
intended to be grep'ed, I put it at the end of the commit body, just
before the Signed-off-by: messages.

						- Ted

P.S.  One thing wihch I'd like to suggest is for folks to use "fix
lock ordering" instead of "fix lockdep warning".  I've been guilty of
using "lockdep warning" mmyself, and I've realized that when searching
to see if a reported hang might have been fixed, the brain has a
tendency to skip over a summary line that says "warning"; and a commit
that fixes a lockdep warning is more serious, than say, for example,
fixing some whining warning message from gcc.

Re: Fix quilt merge error in acpi-cpufreq.c

[Linus Torvalds]

On Thu, 16 Apr 2009, Rusty Russell wrote:
> 
> Side note: I really prefer to see the compile error output in this case: great
> for googling.  It annoys me when people skip this.

I do agree that that's generally a good idea, although I will tend to edit 
it down when people send in 20 lines of compile errors. Nobody cares at 
that point, and it doesn't add value. Give the first few errors, not a 
whole log-ful of them.

I also ask that people edit away the irrelevant site-specific parts. I do 
that editing myself when I notice, but in case it goes through somebody 
who doesn't, or in case I don't notice, look which one is more readable 
and informative:

    Fix the following warning:
    
    fs/fuse/file.c: In function 'fuse_direct_io':
    fs/fuse/file.c:1002: warning: passing argument 3 of 'fuse_get_user_pages' from incompatible pointer type

or

    Fix staging/rt28x0 printk format warnings:
    
    linux-next-20090209/drivers/staging/rt2860/common/spectrum.c:1599: warning: format '%d' expects type 'int', but argument 3 has type
    linux-next-20090209/drivers/staging/rt2860/rt_linux.c:857: warning: format '%d' expects type 'int', but argument 3 has type 'long u

and realize that the "linux-next-20090209/" part doesn't help (and that's 
a pretty _mild_ example of this particular issue, we have tons of examples 
of absolute pathname prefixes like "/home/jeremy/hg/xen/paravirt/linux/", 
so if you look at the log in even a 100-character wide window, you hardly 
see any of the actual _warning_ at all, it's mostly all just pathnames ;)

> Let's get concrete.  Here's the top 3 non-merge commits in gitk:
> 
>     ALSA: hda - Fix the cmd cache keys for amp verbs
>     
>     Fix the key value generation for get/set amp verbs.  The upper bits of
>     the parameter have to be combined with the verb value to be unique for
>     each direction/index of amp access.
>     
>     This fixes the resume problem on some hardwares like Macbook after
>     the channel mode is changed.
> 
> I have no idea what this patch does.  It seems to be a fix; what are the
> symptoms of the problem, and how long has it been there? 

Oh, I'm absolutely not going to claim that we should not improve on 
changelogs in general. But if you actually look at that commit, I would 
argue that the commit message together with the patch is likely not that 
bad to understand for somebody who understands the hardware well enough 
for the code to make sense in the first place!

So could it be improved? I suspect _every_ commit message can be improved. 
But do you really think that an "Impact" statment would be the big deal? 
Or, in fact, any kind of "fixed message format".

>     ALSA: add missing definitions(letters) to HD-Audio.txt
>     
>     impact: Add missing definitions(letters).

And here, the "impact:" part certainly in no way improves anything. 

>     ALSA: sound/pci: use memdup_user()
>     
>     Remove open-coded memdup_user().
> 
> Again, the body seems gratuitous.

And yes, I agree that in many cases you don't need a body at all. If the 
patch is trivial, and the subject tells it all, then why bother with a 
body? We've got a fair number of those.

That said, I'd rather have a few redundant (but still readable) bodies. 
And as mentioned, I don't think a "perfect" changelog exists. Some people 
will always want more, others will think it's unnecessary. And even if a 
perfect changelog existed, we'll never have the perfect people who write 
them.

But machine-readability should be the _last_ thing we look for.

			Linus

Re: Fix quilt merge error in acpi-cpufreq.c

[Ray Lee]

On Wed, Apr 15, 2009 at 7:00 PM, Rusty Russell <rusty@rustcorp.com.au> wrote:
> Anyone want to try to write a guide on writing good commit messages?

To me at least, this is no different than journalism (a commit message
is reporting on the facts, right?). So one can get some rough guidance
from thinking about who, what, where, when, why, how.


- Who is doing the work (from: lines)
- who will benefit from it. (ie, 'scope')

- What is the intended result? (feature? Bug-fix? Clean-up of the
foundations to prepare for future work?)
- What are you *doing*? (Not how -- which is the patch -- but what: a
human-understandable description of the patch.)

- Where is the feature or bug-fix needed or wanted? (Particular
laptops? hardware that is yet to hit the market? And -stable? -head?)

- When does the patch need to be applied? (This merge window, next
cycle, or as soon as possible for a root hole? After another set of
patches?)

- Why was it done *this* way, instead of another?
- Why does mainline want this? (For features. Bug fixes are self-explanatory.)

- how it was tested or validated (tested-by:, reviewed-by: tags).
- how it was implemented (which is satisfied by the patch itself)


Obviously not every patch needs all that crap. But every patch
*series* should probably have some thought put into all those, and the
ones that aren't obvious from the patch should be mentioned. The
trick, of course, is varying levels of 'obvious.' The measure of
'obvious' for journal articles is "someone who is versed in all the
necessary skills, but unfamiliar with the specifics of the topic."

Don't get me wrong, I don't want changelogs to be mini versions of war
and peace. But thinking about the above can put one in a better
context to then sit down and write a *nice* changelog.

Re: Fix quilt merge error in acpi-cpufreq.c

[Theodore Tso]

On Thu, Apr 16, 2009 at 03:46:42AM +0200, Ingo Molnar wrote:
> 
> For similar reasons people have memos, stick-it's and other formal, 
> automated, reflex-alike daily routine measures to make sure they 
> dont forget to do something they all too easily forget otherwise.
...
> This kind of formal measure _forces_ the extraction of this very 
> specific type of summary on all sides of the contribution chain - 
> and it drastically reduced the number of commits i regretted in 
> hindsight.


The question is whether Impact: _works_ in its current form.  I was
came across a recent set of commits sent to you where it was
pathetically obvious that Impact: doesn't really help.  

The patch submitter in question was a non-English speaker.  I've
blacked out the submitter's name, because I'm not trying to call out
that particular person, but rather the assumption that Impact: is
always helpful.  Here's a very clear case where it is not.

I do feel your pain; there are one or two ext4 contributors where I
always have to rewrite their commit logs and comments, and often I end
up staring at the code trying to figure out what the !@#@? they meant.
I used to try to coach them to make better messages, but I've since
given up and just resigned myself to the fact that it's up to me
rewrite the commit logs (and often, the comments in the code, too...)

If we are going to use the Impact: header, there should only be a few
standardized values that it can have, i.e., "clean up", "fix oops",
"fix lock ordering", "fix performance problem", etc.  Otherwise people
will just put garbage in the Impact field --- what the heck does
"Impact: it is not ready yet.  remove it" mean?  

Or "Impact: fix smp_affinity when moving irq_desc"?  

Or worse yet, "Impact: so use dev_to_node"?!?

At least for this particular submitter, the Impact: header clearly is
adding no value, and in fact, I suspect his git commit logs would be
better without trying to force him into filling out a field in what
appears to be a completely random fashion.

	    	   	     	    		 - Ted


x86/irq: remove NUMA_MIGRATIE_IRQ_DESC

Impact: it is not ready yet.  remove it

it causes crash on system with lots of cards with MSI-X
when irq_balancer enabled...

will have one new patch that create irq_desc according to device numa node.

Signed-off-by: XXXXXXX XX <XXXXXXX@XXXXX.XXX>

----------------------------

irq: correct CPUMASKS_OFFSTACK typo -v3

Impact: fix smp_affinity copying when moving irq_desc

CPUMASKS_OFFSTACK is not defined anywhere. it is a typo
and init_allocate_desc_masks called before it set affinity to all cpus...

split init_alloc_desc_masks() into all_desc_masks() and init_desc_masks()
so in the init_copy_desc_masks could use CPUMASK_OFFSTACK there.
aka copy path will not calling init_desc_masks anymore.

also could use that CPUMASK_OFFSTACK in alloc_desc_masks()

v3: update after "remove numa_migrate irq_desc"

Signed-off-by: XXXXXXX XX <XXXXXXX@XXXXX.XXX>
Acked-by: XXXXX XXXXXX <XXXXX@XXXXXXXX.XXX.XX>

-----------------------

Subject: [PATCH 3/8] irq: make set_affinity to return status -v2

Impact: prepare to use it to keep affinity consistent

according to Ingo, change set_affinity() in irq_chip to return int.

v2: fix two typo

Signed-off-by: XXXXXXX XX <XXXXXXX@XXXXX.XXX>

----------------------

irq: change irq_desc_alloc to take node instead cpu

Impact: prepare to make irq_desc_alloc to numa aware

so could make irq_desc_alloc use node pass down

Signed-off-by: XXXXXXX XX <XXXXXXX@XXXXX.XXX>

-------------------------

irq: make io_apic_set_pci_routing to take device

Impact: so use dev_to_node

and use node in irq_desc_all_node()

Signed-off-by: XXXXXXX XX <XXXXXXX@XXXXX.XXX>

-------------------------

x86/irq: make MSI irq_desc numa aware

Impact: use irq_desc_alloc_node

try to get irq_desc on the node in create_irq_nr

Signed-off-by: XXXXXXX XX <XXXXXXX@XXXXX.XXX>

---------------------

irq: make ht irq_desc numa aware

Impact: use create_irq_nr

try to get irq_desc on the node with create_irq_nr

Signed-off-by: XXXXXXX XX <XXXXXXX@XXXXX.XXX>

Re: Fix quilt merge error in acpi-cpufreq.c

[H. Peter Anvin]

Ingo Molnar wrote:
>>
>> At least then it wouldn't break up the narrative, and it would 
>> kind of fit with all the other "tagged" lines.
> 
> Yeah, that makes sense.
> 

Actually, there is one good thing about this.  One of the things we've
found useful is to have the maintainer add or edit Impact: lines.
Putting them with the tags would make it more clear who did the impact
assessment.

	-hpa

-- 
H. Peter Anvin, Intel Open Source Technology Center
I work for Intel.  I don't speak on their behalf.

Re: Fix quilt merge error in acpi-cpufreq.c

[Ingo Molnar]

* H. Peter Anvin <hpa@zytor.com> wrote:

> Ingo Molnar wrote:
> >>
> >> At least then it wouldn't break up the narrative, and it would 
> >> kind of fit with all the other "tagged" lines.
> > 
> > Yeah, that makes sense.
> 
> Actually, there is one good thing about this.  One of the things 
> we've found useful is to have the maintainer add or edit Impact: 
> lines. Putting them with the tags would make it more clear who did 
> the impact assessment.

Ah, indeed - good point. There two other good things about moving 
the impact line to the signoff section:

 - We can actually add it every time - even if it repeats bits of
   the subject line which would look weird if it was in the second 
   line. Right now with the impact line in a prominent place i often 
   feel reluctant to add an impact line when the subject line is 
   good enough to describe the expected risk/impact of a change.

 - I can update my scripts to warn when i sign off on something with 
   no impact line. I.e. the "dont forget to assess impact" step 
   becomes even harder to flunk.

/me likes.

We then also need a good Documentation/impact-tag.txt description 
about it, and list the principles and a few good and bad examples. 
Would you like to write it up?

	Ingo

Re: Fix quilt merge error in acpi-cpufreq.c

[Ingo Molnar]

* Linus Torvalds <torvalds@linux-foundation.org> wrote:

> On Thu, 16 Apr 2009, Ingo Molnar wrote:
> > 
> > Show me one person _you_ actually taught to write good 
> > changelogs - just one person who was not a natural born talker 
> > to begin with. I'll show you a 100 other people who cannot write 
> > good commit logs. They'll try and will limp along, but generally 
> > they cannot.
> > 
> > They might not even have English as their mother tongue - but 
> > still can read and understand C fantastically.
> 
> So?
> 
> The fix for that is not to write crap English. The fix for that is 
> to help them, and/or just fix their comments for them.
> 
> I really don't see the point of your argument. "People don't 
> always write good and complete sentences" is _not_ an argument for 
> then making that a standard.
> 
> Just fix things up. Edit their emails. I do. Andrew does. Yes, and 
> despite that some commits will still have odd grammar or otherwise 
> not be the great novel of the century, and that's not the point. 
> But we should _improve_ on the language for people who aren't 
> native English speakers, not devolve it to something weaker.

I too end up editing the language and typography non-trivially for 
about 90% of all patches i apply, so there is certainly no lack of 
effort here either.

Moving the impact line to the tags section certainly sounds like a 
good solution to make the main flow of the commit be natural 
language - while still keeping most of the good aspects of the tag 
for those who want to use them.

So if that variant now has the official penguin-pee on it, i'd like 
to move back to maintaining^W editing commits! :)

Note: today is a flag day when i'll flip over to putting the 
impact-tag to the tail section - you'll still see impact lines at 
the head of the commit for already committed bits. They might thus 
show up in the next merge window or even later (if a topic needs 
more work than that).

Thanks,

	Ingo

Re: Fix quilt merge error in acpi-cpufreq.c

[Ingo Molnar]

* Theodore Tso <tytso@mit.edu> wrote:

> On Thu, Apr 16, 2009 at 03:46:42AM +0200, Ingo Molnar wrote:
> > 
> > For similar reasons people have memos, stick-it's and other formal, 
> > automated, reflex-alike daily routine measures to make sure they 
> > dont forget to do something they all too easily forget otherwise.
> ...
> > This kind of formal measure _forces_ the extraction of this very 
> > specific type of summary on all sides of the contribution chain - 
> > and it drastically reduced the number of commits i regretted in 
> > hindsight.
> 
> The question is whether Impact: _works_ in its current form. [...]

For us, it clearly works in most cases. Does it work in _every_ 
case? No. No maintenance measure ever works in every case.

> [...]  I was came across a recent set of commits sent to you where 
> it was pathetically obvious that Impact: doesn't really help.

The thing is - none of the examples you quoted below are commits in 
any of our trees! They are email submissions to lkml, not applied 
yet anywhere. And yes, from the impact line alone it becomes clear 
that the commit log needs serious editing.

And yes, in fact it became _easier_ to me to process patches from 
that particular patch submitter you blacked out, since he started 
using impact-lines a few months ago. These bad impact lines you 
quoted are of course not usable directly in a commit log - but they 
are canaries. Plus the ones where i for example get "Impact: 
cleanup" patches from him are certainly useful.

So even this worst-case example you could possibly can come up with 
is in reality actually a success story to us!

Impact tags allow frequent contributors to pre-tag changes with a 
"this I expected to be an easy one" easily recognizable attribute. 
It is very helpful, _especially_ where there's a language barrier.

Here's an easy challenge: try inserting perfect impact lines for a 
few days into all your commits, really! :-) [ You can erase them 
after you've created them, and you dont have to admit it ever that 
you did this experiment ;-) ]

I noticed that even the best commits win a new dimension of quality 
and awareness as far as the maintainer is concerned. It also helps 
us keep our eyes on the ball all the time: constantly assessing the 
true utility and true risk of our patches.

It's truly useful, and unless you try it seriously and come back to 
us with a "I tried it, all my impact lines were perfect but it didnt 
give me any plus so i stopped doing it" i dont think you can ever 
know what you missed out on.

	Ingo

Re: Fix quilt merge error in acpi-cpufreq.c

[Ingo Molnar]

* Rusty Russell <rusty@rustcorp.com.au> wrote:

> On Thu, 16 Apr 2009 04:17:49 am H. Peter Anvin wrote:
> > Linus Torvalds wrote:
> > "build fix" is valid and proper use: it tells that it
> > fixes a compilation error, which succinctly communicates both the
> > priority of the fix and how it needs to be validated.
> 
> Side note: I really prefer to see the compile error output in this 
> case: great for googling.  It annoys me when people skip this.
> 
> Anyway, Impact: had lead me to think harder about my messages than 
> the free-form commit style did.  Perhaps it's too rigid, but it 
> helped.

btw., and i think this is the crux of the matter, Rusty was quite 
sceptic about impact lines in the beginning, and did not like them 
_at all_. We had discussions (months ago) about it with Rusty and he 
had a similar position to other "read only" participants in this 
thread.

And i can tell it from the other side of the fence: Rusty's trees 
were very nice before, but they became _even_ nicer after he started 
using impact lines. It was very noticeable.

Impact lines are intentionally rigid - but all 'forced' measures 
(like signed-off lines, or a title, or other patch submission 
standards) are rigid in a way and they elicit an initial backlash 
from people who have never adhered to them before.

Impact lines have most of their effects on the people who _write_ 
them: contributors and first-hop maintainers. Their role becomes 
informative as the hops increase - and they might even become 
annoyingly meaningless and verbose as the hop count reaches Linus 
;-)

	Ingo

Re: Fix quilt merge error in acpi-cpufreq.c

[Ingo Molnar]

* Theodore Tso <tytso@mit.edu> wrote:

> On Thu, Apr 16, 2009 at 10:57:05AM +0930, Rusty Russell wrote:
> > Ingo wants them.  Example:
> > 
> > 	lguest: don't expect linear addresses in gdt pvops
> > 
> > 	Impact: fix guest crash 'lguest: bad read address 0x4800000 len 256'
> > 
> > What's more important in the subject line?  That it fixes a crash, or what it
> > does?
> 
> Well, consider that 2 or 3 months later, when we're trying to find 
> a potential commit (say, because we're trying to find a potential 
> fix that needs to be forward ported to a distro kernel, or some 
> such), the initial summary line is what is going to be visible in 
> gitk or via "git log --oneline" (or "git log --pretty=oneline 
> --abbrev-commit" for older git versions).
> 
> So when I try to create git log messages, I try to make the first 
> line useful for folks who might be sorting through potentially 
> thousands of patches via gitk or git log --oneline.  So I might do 
> something like

Hm, have you seen the very simple git log + grep examples i gave, 
about how the impact lines of the APIC code and the scheduler in 
this cycle can be used to summarize a stability track record at a 
glance, out of a much larger body of commits?

Try:

  git log v2.6.29..v2.6.30-rc1 arch/x86/kernel/apic/ | \
                  grep Impact: | sort | uniq -c | sort -n


and:

  git log v2.6.29..v2.6.30-rc1 kernel/sched.c | \
                   grep Impact: | sort | uniq -c | sort -n

on a recent upstream repo.

Thanks,

	Ingo

Re: Fix quilt merge error in acpi-cpufreq.c

[Christoph Hellwig]

On Wed, Apr 15, 2009 at 04:49:36PM -0700, David Miller wrote:
> From: Linus Torvalds <torvalds@linux-foundation.org>
> Date: Wed, 15 Apr 2009 14:15:45 -0700 (PDT)
> 
> > Instead of havign that IDIOTIC "Impact:" marker, why not just write good 
> > commit messages?
> 
> Amen.

Yeah, please write proper commit messages.  Just make sure the first
sentence mentiones what's fixed/cleaned up/added/generalized and then go
into the details later.  Often enough it might be more than one of
those..

Re: Fix quilt merge error in acpi-cpufreq.c

[Theodore Tso]

On Thu, Apr 16, 2009 at 09:56:51AM +0200, Ingo Molnar wrote:
> * Rusty Russell <rusty@rustcorp.com.au> wrote:
> > Anyway, Impact: had lead me to think harder about my messages than 
> > the free-form commit style did.  Perhaps it's too rigid, but it 
> > helped.
> 
> btw., and i think this is the crux of the matter, Rusty was quite 
> sceptic about impact lines in the beginning, and did not like them 
> _at all_. We had discussions (months ago) about it with Rusty and he 
> had a similar position to other "read only" participants in this 
> thread.

Hmm, I guess for me what I consider ideal, and what I consciously try
to do, is to include (at most) 2-3 words that describe the impact in
the patch summary line.  Writing a good patch summary line is _hard_;
in 70-75 characters you need to describe both *why* and *what*; it
needs to be something which is both succinct, but which, several
months later, is enough so that someone scanning the patch summaries
has a fighting chance to pick out the relevant patch amongst a sea of
thousands of other patches.

And at least for me, something mechanical just isn't likely to work.
It reminds me of a story when, over 30 years ago, someone at the MIT
AI Lab wrote a proposal to create "programming for non-programmers";
it proposed creating templates so that people who didn't know how to
do things could use as a starting point, and then have expert systems
that would help fill in the rest.  Shortly after this paper was
circulated, a parody was sent out mocking the first paper, "thinking
for non-thinkers".  It suggested creating template idea for people who
weren't smart enough to create their own original thoughts, etc. 

So I'd really like to encourage try challenging people to try to write
a good patch summary line.  It may be that forcing someone to
constrain themselves to 70 characters (75 if they must) and which must
explain both the impact of the patch as well as the what the patch
does, is enough rigidity or a constraint that it might force people to
think.  Because at the end of the day, that's what we really need
people to do.

              					- Ted

Re: Fix quilt merge error in acpi-cpufreq.c

[Valdis.Kletnieks]

[-- Attachment #1: Type: text/plain, Size: 793 bytes --]

On Wed, 15 Apr 2009 14:23:26 PDT, David Miller said:
> From: Linus Torvalds <torvalds@linux-foundation.org>
> Date: Wed, 15 Apr 2009 12:43:02 -0700 (PDT)
> 
> > And if it _is_ obvious, then the mechanical "Impact:" thing is pointless.
> > 
> > In other words - in neither case does it actually help anything at all. 
> > It's only distracting noise.
> 
> FWIW I find the Impact: blurbs highly annoying too.  Just freakin'
> say what the damn patch does in the commit message.
> 
> If a person can't be bothered to skim the commit message text,
> this Impact: tag only gives them a false sense of understanding
> what the change does.

Maybe we can use it to flag security fixes, to appease the people complaining
they can't find the security fixes without reading the commit message text. ;)


[-- Attachment #2: Type: application/pgp-signature, Size: 226 bytes --]

Re: Fix quilt merge error in acpi-cpufreq.c

[Jonathan Corbet]

On Thu, 16 Apr 2009 11:30:26 +0930
Rusty Russell <rusty@rustcorp.com.au> wrote:

> Anyone want to try to write a guide on writing good commit messages?

I've tried to do that about halfway through
Documentation/development-process/5.Posting.  No doubt it can be
improved...patches (with good commit messages) welcome :)

jon

Re: Fix quilt merge error in acpi-cpufreq.c

[Valdis.Kletnieks]

[-- Attachment #1: Type: text/plain, Size: 682 bytes --]

On Thu, 16 Apr 2009 02:50:25 +0200, Ingo Molnar said:

> Something like:

Avoid the passive voice when feasible.

>  The impact of this change is that the build is fixed.

Fix the build breakage caused by bad #include screwage.

>  The impact of this change is that the code gets cleaner.

Neaten up the spaghetti code.

>  The impact of this change is that the CPU does not overheat.

Prevent the CPU from overheating.

Good clear concise writing doesn't need an 'Impact:' to draw attention to it.

About the *only* use case that I've seen that actually makes *any* sense is
the *one* case where adding "fix on s390 and x86" to the original Subject:
line caused it to be overlong.

[-- Attachment #2: Type: application/pgp-signature, Size: 226 bytes --]

Re: Fix quilt merge error in acpi-cpufreq.c

[Valdis.Kletnieks]

[-- Attachment #1: Type: text/plain, Size: 774 bytes --]

On Thu, 16 Apr 2009 03:46:42 +0200, Ingo Molnar said:
> 
> * Linus Torvalds <torvalds@linux-foundation.org> wrote:
> 
> > > You just dont seem to understand why i find it useful. You also 
> > > seem to try to deprive us the basic right of creating new, 

> The number of contributors who can write meaningful changelogs or 
> who can be taught to write really good changelogs is very, very low. 
> I'd guesstimate somewhere around 5% of all Linux contributors. (The 
> guesstimation is probably on the more generous side.)

"I have made this letter longer than usual because I lack the time to make it
shorter." -- Blaise Pascal

And you want the same people who can't summarize their work in a paragraph
to compress it even further into one line?  This way lies madness.


[-- Attachment #2: Type: application/pgp-signature, Size: 226 bytes --]

Re: Fix quilt merge error in acpi-cpufreq.c

[Rusty Russell]

On Thu, 16 Apr 2009 11:25:41 pm Jonathan Corbet wrote:
> On Thu, 16 Apr 2009 11:30:26 +0930
> Rusty Russell <rusty@rustcorp.com.au> wrote:
> 
> > Anyone want to try to write a guide on writing good commit messages?
> 
> I've tried to do that about halfway through
> Documentation/development-process/5.Posting.  No doubt it can be
> improved...patches (with good commit messages) welcome :)

I think Ted put his finger on it: the definition of a good commit
message is one which is kind to the reader.

There are several readers: someone reviewing the patch, someone looking
for a specific bug, someone backporting, someone dealing with an API change,
someone bisecting.  A reviewer needs to know why the patch is done the way it
is, the bughunter needs to be able to find the commit from the bug symptoms,
the backporter needs to know what the patch fixes, how severe it is, and what
side effects are expected, the what-happened reader needs to know how to port
to the new API (and probably why it changed), and the bisector doesn't need
much at all (since the patch is buggy, the commentry is probably wrong anyway,
but perhaps they need to judge what reverting the patch will do).

Here are my thoughts:

- Actual demonstrated fixes for demonstrated bugs should aim for subject
  "<subsystem>: fix <symptom> <condition>...", eg: "modules: fix crash when out
  of memory" or "modules: fix compile error on arm" or "modules: fix module
  load when CONFIG_FOO=y, CONFIG_BAR=n, moon=full".

  Body should include:
  - how likely (if not obvious from subject).
  - how important.  Root can crash box maybe isn't important, but if
    NetworkManager default configuration tickles the bug, it might be.
  - text of message which results if any.  Oops (trimmed), compile error,
    unexpected output of utility.
  - when the bug was introduced (or exposed).
  - upstream (bugzilla, ml message, or at least reporter) so it can be dug
    further if required.

  I suggest that the keyword "fix" not be used for theoretical or soon-to-be-
  exposed limitations of code.  eg. "virtio: correction to BAD_RING
  macro if arg is not called 'vq'" not "virtio: fix BAD_RING macro when..."
  (this was a real example, but the arg was always called 'vq').

- Neatening-and-documenting commits should aim for "<subsystem>: cleanup...",
  eg. "lguest: cleanup typos in headers" or "lguest: cleanup: rename internal
  function", or "module: cleanup: move function out of header".

  Body should include:
  - larger plan if there is one (eg. removing obsolete function, reordering
    functions because we're going to do something later, exposing functions
    because we're going to need them in successive patch).

- No subject should ever contain the word "trivial".  If it's really trivial,
  you can sum it up in the subject and we'll know it's trivial.  Plus the
  diffstat shows it.  'trivial' is propaganda to sneak a patch into -rc7.

- API changes or deprecation should say why the change is happening, and
  how to port.  The subject line should say what's changed in preference
  to why.  eg. "per_cpu: deprecate per_cpu_ptr in favor of percpu_ptr", or
  "list.h: add list_for_each_reverse_rcu_backflip".

- The change message should always be about the change, not the new code.
  For example, the above commit might say "we need this for the new backflip.c
  debugging code", but the documentation of the function belongs in the code
  itself, not the git log!

Writing good commit messages takes time, practice and feedback.  But
if you're having trouble writing a good commit message, it's often a sign
that your patch needs to be reworked anyway.

Sorry for the too-long post, but this stuff actually matters...
Rusty.

Re: Fix quilt merge error in acpi-cpufreq.c

[Ingo Molnar]

* Rusty Russell <rusty@rustcorp.com.au> wrote:

> Here are my thoughts:
> 
> - Actual demonstrated fixes for demonstrated bugs should aim for subject
>   "<subsystem>: fix <symptom> <condition>...", eg: "modules: fix crash when out
>   of memory" or "modules: fix compile error on arm" or "modules: fix module
>   load when CONFIG_FOO=y, CONFIG_BAR=n, moon=full".

i dont disagree - and this really has been recommended for years and 
is 'obvious' to do - i just dont see it being consistently practiced 
by _anyone_ upstream.

And (at the risk of over-simplifying a good dozen mails) i pointed 
out why it's not being practiced consistently by anyone: without 
easy visual (and/or tooled) reminders that something is structured 
incorrectly, humans tend to slack off. They tend to slack off 
_especially_ when it comes to describing a discomforting aspect of 
an otherwise cool commit: the practical benefit of the patch (which 
might be discomfortingly insignificant) and the risks it carries 
(which might be discomfortingly significant).

This "impact on the kernel" also happens to be an essential (and 
hard to obtain) piece of information that matters most when a tree 
is actualy _used_ by people (not just developed to the moon), and 
this piece of information is missing in most cases.

Developers often _do not create_ this information even in their 
heads: and it's not at all apparent from a commit log whether this 
information has been created. Making them think about this straight 
at patch creation time, with an easy rule and an easy reminder, is 
an obviously good thing to kernel quality in general.

So we saw this general problem and we tried to find a solution.

Instead of ping-ponging with contributors all the time and bitching 
about subject lines (which really is not something most developers 
will even understand the deep importance of, and which is also 
rather difficult to do linguistically) we went for preemptively 
asking them to include this kind of information, in a separate, 
limited-format and limited-purpose impact line that concentrates on 
just creating this information.

And given the very clear rejection of some special sign in the 
summary line (or right next to it), and given the intrusion the 
header-impact-line approach causes to the natural language flow of 
the commit log, a couple of days ago we switched to an impact-footer 
that describes 'impact to the kernel' in an as short way as 
possible.

After a few days of experience with it i can tell you that it's even 
better than the original header approach: it fits better into the 
other tags and it can be done more consistently because it's less 
intrusive to the introductory portion of the commit log (which was 
the main complaint against it). There's also upstream buy-in now 
which is a big plus. So as far as i'm concerned there's a happy 
ending.

Note that this tag is both poorer and richer in information than the 
summary line you describe, so it's really pretty complementary. It 
is poorer in that it does not replicate subsystem information and 
does not replicate implementational details. It is richer in that it 
always tries to describe practical impact of a change.

Look at the example below, it is a commit from today and it has this 
summary line and this impact line:

    tracing/ring-buffer: Add unlock recursion protection on discard

    ...

    [ Impact: fix spurious warning triggering tracing shutdown ]

Both kinds of information are important. To the developer it is more 
important to see that we added recursion protection to trace-record 
discard path.

To the user/tester/distributor/lurker it is more important to see 
that this fix addresses a spurious warning that causes the tracing 
subsystem to self-disable. This is simply a different 'view' of the 
same commit - and a pretty important view at that.

Can you integrate these two into a single summary line? The obvious 
solution would be to append them:

     tracing/ring-buffer: add unlock recursion protection on discard to fix spurious warning triggering tracing shutdown

but 121 characters width is _way_ too long for a summary.

And even if we could squeeze it into the given line length, can you 
make it apparent enough 'at a glance' that this has been done (and 
can you enable tooling that checks whether this has been done) 
versus the many commits where this has not been done?

( The tag scheme Ted proposed addresses those problems but has a 
  number of other disadvantages. )

	Ingo

---------------------------------------------->
commit f3b9aae16219aaeca2dd5a9ca69f7a10faa063df
Author: Frederic Weisbecker <fweisbec@gmail.com>
Date:   Sun Apr 19 23:39:33 2009 +0200

    tracing/ring-buffer: Add unlock recursion protection on discard
    
    The pair of helpers trace_recursive_lock() and trace_recursive_unlock()
    have been introduced recently to provide generic tracing recursion
    protection.
    
    They are used in a symetric way:
    
     - trace_recursive_lock() on buffer reserve
     - trace_recursive_unlock() on buffer commit
    
    However sometimes, we don't commit but discard on entry
    to the buffer, ie: in case of filter checking.
    
    Then we must also unlock the recursion protection on discard time,
    otherwise the tracing gets definitely deactivated and a warning
    is raised spuriously, such as:
    
    111.119821] ------------[ cut here ]------------
    [  111.119829] WARNING: at kernel/trace/ring_buffer.c:1498 ring_buffer_lock_reserve+0x1b7/0x1d0()
    [  111.119835] Hardware name: AMILO Li 2727
    [  111.119839] Modules linked in:
    [  111.119846] Pid: 5731, comm: Xorg Tainted: G        W  2.6.30-rc1 #69
    [  111.119851] Call Trace:
    [  111.119863]  [<ffffffff8025ce68>] warn_slowpath+0xd8/0x130
    [  111.119873]  [<ffffffff8028a30f>] ? __lock_acquire+0x19f/0x1ae0
    [  111.119882]  [<ffffffff8028a30f>] ? __lock_acquire+0x19f/0x1ae0
    [  111.119891]  [<ffffffff802199b0>] ? native_sched_clock+0x20/0x70
    [  111.119899]  [<ffffffff80286dee>] ? put_lock_stats+0xe/0x30
    [  111.119906]  [<ffffffff80286eb8>] ? lock_release_holdtime+0xa8/0x150
    [  111.119913]  [<ffffffff802c8ae7>] ring_buffer_lock_reserve+0x1b7/0x1d0
    [  111.119921]  [<ffffffff802cd110>] trace_buffer_lock_reserve+0x30/0x70
    [  111.119930]  [<ffffffff802ce000>] trace_current_buffer_lock_reserve+0x20/0x30
    [  111.119939]  [<ffffffff802474e8>] ftrace_raw_event_sched_switch+0x58/0x100
    [  111.119948]  [<ffffffff808103b7>] __schedule+0x3a7/0x4cd
    [  111.119957]  [<ffffffff80211b56>] ? ftrace_call+0x5/0x2b
    [  111.119964]  [<ffffffff80211b56>] ? ftrace_call+0x5/0x2b
    [  111.119971]  [<ffffffff80810c08>] schedule+0x18/0x40
    [  111.119977]  [<ffffffff80810e09>] preempt_schedule+0x39/0x60
    [  111.119985]  [<ffffffff80813bd3>] _read_unlock+0x53/0x60
    [  111.119993]  [<ffffffff807259d2>] sock_def_readable+0x72/0x80
    [  111.120002]  [<ffffffff807ad5ed>] unix_stream_sendmsg+0x24d/0x3d0
    [  111.120011]  [<ffffffff807219a3>] sock_aio_write+0x143/0x160
    [  111.120019]  [<ffffffff80211b56>] ? ftrace_call+0x5/0x2b
    [  111.120026]  [<ffffffff80721860>] ? sock_aio_write+0x0/0x160
    [  111.120033]  [<ffffffff80721860>] ? sock_aio_write+0x0/0x160
    [  111.120042]  [<ffffffff8031c283>] do_sync_readv_writev+0xf3/0x140
    [  111.120049]  [<ffffffff80211b56>] ? ftrace_call+0x5/0x2b
    [  111.120057]  [<ffffffff80276ff0>] ? autoremove_wake_function+0x0/0x40
    [  111.120067]  [<ffffffff8045d489>] ? cap_file_permission+0x9/0x10
    [  111.120074]  [<ffffffff8045c1e6>] ? security_file_permission+0x16/0x20
    [  111.120082]  [<ffffffff8031cab4>] do_readv_writev+0xd4/0x1f0
    [  111.120089]  [<ffffffff80211b56>] ? ftrace_call+0x5/0x2b
    [  111.120097]  [<ffffffff80211b56>] ? ftrace_call+0x5/0x2b
    [  111.120105]  [<ffffffff8031cc18>] vfs_writev+0x48/0x70
    [  111.120111]  [<ffffffff8031cd65>] sys_writev+0x55/0xc0
    [  111.120119]  [<ffffffff80211e32>] system_call_fastpath+0x16/0x1b
    [  111.120125] ---[ end trace 15605f4e98d5ccb5 ]---
    
    [ Impact: fix spurious warning triggering tracing shutdown ]
    
    Signed-off-by: Frederic Weisbecker <fweisbec@gmail.com>

Re: Fix quilt merge error in acpi-cpufreq.c

[Jonathan Corbet]

On Mon, 20 Apr 2009 17:44:21 +0930
Rusty Russell <rusty@rustcorp.com.au> wrote:

> There are several readers: someone reviewing the patch, someone looking
> for a specific bug, someone backporting, someone dealing with an API change,
> someone bisecting. 

With this in mind, I've tried to make a relatively concise addition to
the development process document.  How does the following look?

jon

--

>From c67584ac4f8fcad9f577e5ca9f73496ef99df63a Mon Sep 17 00:00:00 2001
From: Jonathan Corbet <corbet@lwn.net>
Date: Tue, 21 Apr 2009 13:33:06 -0600
Subject: [PATCH] docs: Encourage better changelogs in the development process document

Add a couple of paragraphs to the "patch formatting" section on how patches
should be described.  This text is shamelessly cribbed from suggestions
posted by Rusty Russell.

Signed-off-by: Jonathan Corbet <corbet@lwn.net>
---
 Documentation/development-process/5.Posting |   31 ++++++++++++++++++++++++--
 1 files changed, 28 insertions(+), 3 deletions(-)

diff --git a/Documentation/development-process/5.Posting b/Documentation/development-process/5.Posting
index dd48132..f622c1e 100644
--- a/Documentation/development-process/5.Posting
+++ b/Documentation/development-process/5.Posting
@@ -119,7 +119,7 @@ which takes quite a bit of time and thought after the "real work" has been
 done.  When done properly, though, it is time well spent.
 
 
-5.4: PATCH FORMATTING
+5.4: PATCH FORMATTING AND CHANGELOGS
 
 So now you have a perfect series of patches for posting, but the work is
 not done quite yet.  Each patch needs to be formatted into a message which
@@ -146,8 +146,33 @@ that end, each patch will be composed of the following:
  - One or more tag lines, with, at a minimum, one Signed-off-by: line from
    the author of the patch.  Tags will be described in more detail below.
 
-The above three items should, normally, be the text used when committing
-the change to a revision control system.  They are followed by:
+The items above, together, form the changelog for the patch.  Writing good
+changelogs is a crucial but often-neglected art; it's worth spending
+another moment discussing this issue.  When writing a changelog, you should
+bear in mind that a number of different people will be reading your words.
+These include subsystem maintainers and reviewers who need to decide
+whether the patch should be included, distributors and other maintainers
+trying to decide whether a patch should be backported to other kernels, bug
+hunters wondering whether the patch is responsible for a problem they are
+chasing, users who want to know how the kernel has changed, and more.  A
+good changelog conveys the needed information to all of these people in the
+most direct and concise way possible.
+
+To that end, the summary line should describe the effects of and motivation
+for the change as well as possible given the one-line constraint.  The
+detailed description can then amplify on those topics and provide any
+needed additional information.  If the patch fixes a bug, cite the commit
+which introduced the bug if possible.  If a problem is associated with
+specific log or compiler output, include that output to help others
+searching for a solution to the same problem.  If the change is meant to
+support other changes coming in later patch, say so.  If internal APIs are
+changed, detail those changes and how other developers should respond.  In
+general, the more you can put yourself into the shoes of everybody who will
+be reading your changelog, the better that changelog (and the kernel as a
+whole) will be.
+
+Needless to say, the changelog should be the text used when committing the
+change to a revision control system.  It will be followed by:
 
  - The patch itself, in the unified ("-u") patch format.  Using the "-p"
    option to diff will associate function names with changes, making the
-- 
1.6.2.2

Re: Fix quilt merge error in acpi-cpufreq.c

[Rusty Russell]

On Wed, 22 Apr 2009 05:07:01 am Jonathan Corbet wrote:
> On Mon, 20 Apr 2009 17:44:21 +0930
> Rusty Russell <rusty@rustcorp.com.au> wrote:
> 
> > There are several readers: someone reviewing the patch, someone looking
> > for a specific bug, someone backporting, someone dealing with an API change,
> > someone bisecting. 
> 
> With this in mind, I've tried to make a relatively concise addition to
> the development process document.  How does the following look?

I like it, but will it help?

Fresh from an epic battle with printk, grep, debugging race conditions and
subtly confusing code, who among us can resist providing a blow-by-blow
account?  Leave the heroics unsung in a dry summary of the final patch?

Of course, you never see *me* use flowery prose!
Rusty.

Re: Fix quilt merge error in acpi-cpufreq.c

[Rusty Russell]

On Mon, 20 Apr 2009 08:08:05 pm Ingo Molnar wrote:
> Can you integrate these two into a single summary line? The obvious 
> solution would be to append them:
> 
>      tracing/ring-buffer: add unlock recursion protection on discard to fix spurious warning triggering tracing shutdown
> 
> but 121 characters width is _way_ too long for a summary.

No, if I'm reading this commit correctly, the commit message is well written,
just really bad.

    tracing/ring-buffer: Add unlock recursion protection on discard

-- This helps the patch reviewer, but noone else.  And the patch reviewer
   should be reading the entire thing anyway.  This is a fix, but you have
   to read to the bottom to know that.

    The pair of helpers trace_recursive_lock() and trace_recursive_unlock()
    have been introduced recently to provide generic tracing recursion
    protection.

    They are used in a symetric way:

     - trace_recursive_lock() on buffer reserve
     - trace_recursive_unlock() on buffer commit

-- Err, why is this verbiage in this patch at all?

    However sometimes, we don't commit but discard on entry
    to the buffer, ie: in case of filter checking.

    Then we must also unlock the recursion protection on discard time,
    otherwise the tracing gets definitely deactivated and a warning
    is raised spuriously, such as:

-- So, the problem is that tracing gets deactivated permanently?  Also a
   warning is raised (in which case is it really spurious?).   Since I have
   no idea what this code does, is it common?  When was this problem
   introduced?  

    111.119821] ------------[ cut here ]------------
    [  111.119829] WARNING: at kernel/trace/ring_buffer.c:1498 ring_buffer_lock_reserve+0x1b7/0x1d0()
    [  111.119835] Hardware name: AMILO Li 2727
    [  111.119839] Modules linked in:
    [  111.119846] Pid: 5731, comm: Xorg Tainted: G        W  2.6.30-rc1 #69
    [  111.119851] Call Trace:
    [  111.119863]  [<ffffffff8025ce68>] warn_slowpath+0xd8/0x130
    [  111.119873]  [<ffffffff8028a30f>] ? __lock_acquire+0x19f/0x1ae0
    [  111.119882]  [<ffffffff8028a30f>] ? __lock_acquire+0x19f/0x1ae0
    ...

-- Good, a backtrace is nice.

   [ Impact: fix spurious warning triggering tracing shutdown ]

-- Hidden away here, I don't think I like this.  Not an improvement.

    Signed-off-by: Frederic Weisbecker <fweisbec@gmail.com>

The patch itself basically adds trace_recursive_unlock() to event discard.
Seems like it should always have done that, and it's a simple bug that it
didn't.  But I had to work hard to figure that out.

Rusty.

Re: Fix quilt merge error in acpi-cpufreq.c

[Niel Lambrechts]

On 04/16/2009 06:00 AM, Theodore Tso wrote:
> On Thu, Apr 16, 2009 at 03:46:42AM +0200, Ingo Molnar wrote:
>> For similar reasons people have memos, stick-it's and other formal, 
>> automated, reflex-alike daily routine measures to make sure they 
>> dont forget to do something they all too easily forget otherwise.
> ...
>> This kind of formal measure _forces_ the extraction of this very 
>> specific type of summary on all sides of the contribution chain - 
>> and it drastically reduced the number of commits i regretted in 
>> hindsight.
> 
> 
> The question is whether Impact: _works_ in its current form.  I was
> came across a recent set of commits sent to you where it was
> pathetically obvious that Impact: doesn't really help.  
> 
> The patch submitter in question was a non-English speaker.  I've
> blacked out the submitter's name, because I'm not trying to call out
> that particular person, but rather the assumption that Impact: is
> always helpful.  Here's a very clear case where it is not.
> 
> I do feel your pain; there are one or two ext4 contributors where I
> always have to rewrite their commit logs and comments, and often I end
> up staring at the code trying to figure out what the !@#@? they meant.
> I used to try to coach them to make better messages, but I've since
> given up and just resigned myself to the fact that it's up to me
> rewrite the commit logs (and often, the comments in the code, too...)
> 
> If we are going to use the Impact: header, there should only be a few
> standardized values that it can have, i.e., "clean up", "fix oops",
> "fix lock ordering", "fix performance problem", etc.  Otherwise people
> will just put garbage in the Impact field --- what the heck does
> "Impact: it is not ready yet.  remove it" mean?  
> 
> Or "Impact: fix smp_affinity when moving irq_desc"?  
> 
> Or worse yet, "Impact: so use dev_to_node"?!?

Sorry if I'm speaking out of turn, would it not be more meaningful to
have "Tags:" or a "Keywords:" instead of a subjective impact assessment?

As for"Impact" - does the author at time of creation really know all the
possible problem permutations a specific piece of code might cause or
apply to?

Tags could however be helpful to someone that is not the developer or
maintainer.

As a real life example from an issue I just had, I would be inclined to
google for "brightness cannot be controlled by keyboard" rather than
gleaning anything from "i915: Register ACPI video even when not
modesetting" - even though that probably is a really clear commit
message in context of being the developer / maintainer.

It is not quite the same as "Impact", since someone else with XYZ laptop
might have some other issue as a result of the same commit.

The developer already has at least an entire paragraph to describe the
technicalities for his direct audience, but what about making the
problem context easier to understand (searchable) for the interested
outsiders? :)

cheers
Niel