* Linux Kernel Mailing List <[email protected]> wrote:
> Gitweb: http://git.kernel.org/linus/1c98aa7424ff163637d8321674ec58dee28152d4
> Commit: 1c98aa7424ff163637d8321674ec58dee28152d4
> Parent: 2e1c63b7ed36532b68f0eddd6a184d7ba1013b89
> Author: Linus Torvalds <[email protected]>
> AuthorDate: Mon Apr 13 18:09:20 2009 -0700
> Committer: Linus Torvalds <[email protected]>
> 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 <[email protected]>
> Signed-off-by: Linus Torvalds <[email protected]>
> ---
> 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
* Ingo Molnar <[email protected]> wrote:
> * Linux Kernel Mailing List <[email protected]> wrote:
>
> > Gitweb: http://git.kernel.org/linus/1c98aa7424ff163637d8321674ec58dee28152d4
> > Commit: 1c98aa7424ff163637d8321674ec58dee28152d4
> > Parent: 2e1c63b7ed36532b68f0eddd6a184d7ba1013b89
> > Author: Linus Torvalds <[email protected]>
> > AuthorDate: Mon Apr 13 18:09:20 2009 -0700
> > Committer: Linus Torvalds <[email protected]>
> > 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 <[email protected]>
> > Signed-off-by: Linus Torvalds <[email protected]>
> > ---
> > 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
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 <[email protected]>
Cc: Andrew Morton <[email protected]>
---
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();
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)
Linus Torvalds <[email protected]> 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
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
* Linus Torvalds <[email protected]> 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
* Linus Torvalds <[email protected]> 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
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.
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.
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
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.
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
* Linus Torvalds <[email protected]> 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 <[email protected]>
| 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
On Wed, 15 Apr 2009 12:43:02 -0700 (PDT)
Linus Torvalds <[email protected]> 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.
* Andrew Morton <[email protected]> wrote:
> On Wed, 15 Apr 2009 12:43:02 -0700 (PDT)
> Linus Torvalds <[email protected]> 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 <[email protected]>
| 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 <[email protected]>
| 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 <[email protected]>
| 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 <[email protected]>
| 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 <[email protected]>
| 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
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
On Wed, 15 Apr 2009 23:03:53 +0200
Ingo Molnar <[email protected]> wrote:
> what can we do?
Write better patch titles?
From: Linus Torvalds <[email protected]>
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.
* Linus Torvalds <[email protected]> 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
* David Miller <[email protected]> wrote:
> From: Linus Torvalds <[email protected]>
> 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
* Andrew Morton <[email protected]> wrote:
> On Wed, 15 Apr 2009 23:03:53 +0200
> Ingo Molnar <[email protected]> wrote:
>
> > what can we do?
>
> Write better patch titles?
Lets start with a patch title that you wrote:
| commit 6b44003e5ca66a3fffeb5bc90f40ada2c4340896
| Author: Andrew Morton <[email protected]>
| 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
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
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
From: Linus Torvalds <[email protected]>
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.
* Linus Torvalds <[email protected]> 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
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
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
* Linus Torvalds <[email protected]> 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
* Linus Torvalds <[email protected]> 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
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
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.
* Linus Torvalds <[email protected]> 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
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.
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
On Wed, Apr 15, 2009 at 10:00 PM, Rusty Russell <[email protected]> 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 [email protected]
> More majordomo info at ?http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at ?http://www.tux.org/lkml/
>
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.
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
On Wed, Apr 15, 2009 at 7:00 PM, Rusty Russell <[email protected]> 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.
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 <[email protected]>
----------------------------
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 <[email protected]>
Acked-by: XXXXX XXXXXX <[email protected]>
-----------------------
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 <[email protected]>
----------------------
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 <[email protected]>
-------------------------
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 <[email protected]>
-------------------------
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 <[email protected]>
---------------------
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 <[email protected]>
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.
* H. Peter Anvin <[email protected]> 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
* Linus Torvalds <[email protected]> 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
* Theodore Tso <[email protected]> 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
* Rusty Russell <[email protected]> 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
* Theodore Tso <[email protected]> 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
On Wed, Apr 15, 2009 at 04:49:36PM -0700, David Miller wrote:
> From: Linus Torvalds <[email protected]>
> 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..
On Thu, Apr 16, 2009 at 09:56:51AM +0200, Ingo Molnar wrote:
> * Rusty Russell <[email protected]> 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
On Thu, 16 Apr 2009 11:30:26 +0930
Rusty Russell <[email protected]> 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
On Wed, 15 Apr 2009 14:23:26 PDT, David Miller said:
> From: Linus Torvalds <[email protected]>
> 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. ;)
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.
On Thu, 16 Apr 2009 03:46:42 +0200, Ingo Molnar said:
>
> * Linus Torvalds <[email protected]> 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.
On Thu, 16 Apr 2009 11:25:41 pm Jonathan Corbet wrote:
> On Thu, 16 Apr 2009 11:30:26 +0930
> Rusty Russell <[email protected]> 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.
* Rusty Russell <[email protected]> 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 <[email protected]>
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 <[email protected]>
On Mon, 20 Apr 2009 17:44:21 +0930
Rusty Russell <[email protected]> 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 <[email protected]>
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 <[email protected]>
---
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
On Wed, 22 Apr 2009 05:07:01 am Jonathan Corbet wrote:
> On Mon, 20 Apr 2009 17:44:21 +0930
> Rusty Russell <[email protected]> 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.
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 <[email protected]>
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.