Re: [patch] reduce IPI noise due to /dev/cdrom open/close

[Keith Owens <kaos@sgi.com>]

Jes Sorensen (on 03 Jul 2006 11:33:54 -0400) wrote:
>Anyway, this patch reduces the IPI noise by keeping a cpumask of CPUs
>which have items in the bh lru and only flushing on the relevant
>CPUs. On systems with larger CPU counts it's quite normal that only a
>few CPUs are actively doing block IO, so spewing IPIs everywhere to
>flush this is unnecessary.
>
>Index: linux-2.6/fs/buffer.c
>===================================================================
>--- linux-2.6.orig/fs/buffer.c
>+++ linux-2.6/fs/buffer.c
>@@ -1323,6 +1323,7 @@ struct bh_lru {
> };
> 
> static DEFINE_PER_CPU(struct bh_lru, bh_lrus) = {{ NULL }};
>+static cpumask_t lru_in_use;
> 
> #ifdef CONFIG_SMP
> #define bh_lru_lock()	local_irq_disable()
>@@ -1352,9 +1353,14 @@ static void bh_lru_install(struct buffer
> 	lru = &__get_cpu_var(bh_lrus);
> 	if (lru->bhs[0] != bh) {
> 		struct buffer_head *bhs[BH_LRU_SIZE];
>-		int in;
>-		int out = 0;
>+		int in, out, cpu;
> 
>+		cpu = raw_smp_processor_id();

Why raw_smp_processor_id?  That normally indicates code that wants a
lazy cpu number, but this code requires the exact cpu number, IMHO
using raw_smp_processor_id is confusing.  smp_processor_id can safely
be used here, bh_lru_lock has disabled irq or preempt.

>+		/* Test first to avoid cache lines bouncing around */
>+		if (!cpu_isset(cpu, lru_in_use))
>+			cpu_set(cpu, lru_in_use);
>+
>+		out = 0;
> 		get_bh(bh);
> 		bhs[out++] = bh;
> 		for (in = 0; in < BH_LRU_SIZE; in++) {
>@@ -1500,19 +1506,28 @@ EXPORT_SYMBOL(__bread);
>  */
> static void invalidate_bh_lru(void *arg)
> {
>-	struct bh_lru *b = &get_cpu_var(bh_lrus);
>+	struct bh_lru *b;
> 	int i;
> 
>+	local_irq_disable();
>+	b = &get_cpu_var(bh_lrus);
> 	for (i = 0; i < BH_LRU_SIZE; i++) {
> 		brelse(b->bhs[i]);
> 		b->bhs[i] = NULL;
> 	}
> 	put_cpu_var(bh_lrus);
>+	local_irq_enable();
> }
> 	
> static void invalidate_bh_lrus(void)
> {
>-	on_each_cpu(invalidate_bh_lru, NULL, 1, 1);
>+	/*
>+	 * Need to hand down a copy of the mask or we wouldn't be run
>+	 * anywhere due to the original mask being cleared
>+	 */
>+	cpumask_t mask = lru_in_use;
>+	cpus_clear(lru_in_use);
>+	schedule_on_each_cpu_mask(invalidate_bh_lru, NULL, mask);
> }

Racy?  Start with an empty lru_in_use.

Cpu A                         Cpu B
invalidate_bh_lrus()
mask = lru_in_use;
preempted
                              block I/O
			      bh_lru_install()
			      cpu_set(cpu, lru_in_use);
resume
cpus_clear(lru_in_use);
schedule_on_each_cpu_mask() - does not send IPI to cpu B