Re: [LTP] [PATCH v2] kernel/irq: Add irqbalance01

[Richard Palethorpe <rpalethorpe@suse.de>]

Hello Cyril,

Cyril Hrubis <chrubis@suse.cz> writes:

> Hi!
>> +	/* Read the CPU affinity masks for each IRQ. The first CPU is in the
>> +	 * right most (least significant) bit. See bitmap_string() in the kernel
>> +	 * (%*pb)
>> +	 */
>> +	for (row = 0; row < nr_irqs; row++) {
>> +		sprintf(path, "/proc/irq/%u/smp_affinity", irq_ids[row]);
>> +		buf = read_proc_file(path);
>> +		c = buf;
>> +		col = 0;
>> +
>> +		while (*c != '\0')
>> +			c++;
>
> A minor nit, we alredy know the lenght, but we do not return it from the
> read_proc_file() we may as well change this so taht the read_proc_file
> returns both buffer pointer and the size. E.g. add size_t *len parameter
> to the read_proc_file() and set it if it's not NULL.

+1

>
> Other than that the parsing looks good now.
>
>> +static void evidence_of_change(void)
>> +{
>> +	size_t row, col, changed = 0;
>> +
>> +	for (row = 0; row < nr_irqs; row++) {
>> +		for (col = 0; col < nr_cpus; col++) {
>> +			if (!irq_stats[row * nr_cpus + col])
>> +				continue;
>> +
>> +			if (irq_affinity[row * nr_cpus + col] == ALLOW)
>> +				continue;
>> +
>> +			changed++;
>> +		}
>> +	}
>> +
>> +	tst_res(changed ? TPASS : TFAIL,
>> +		"Heuristic: Detected %zu irq-cpu pairs have been dissallowed",
>> +		changed);
>> +}
>
> I'm still unusure about this part though.
>
> The test fails on my workstation where the IRQs are nicely distributed
> by the hardware/BIOS.

You don't need irqbalance then :-p

The original bug this is intened to avoid was caused by the irqbalance
deamon silently failing.

>
> Maybe we should check if IRQs are distributed somehow evenly and PASS
> the test if that is the case as well.

My plan was to have a second heuristic based on the IRQ
distribution. However the intention was to make detection more sensitve
to bad setups. I think what you are suggesting is the opposite. So that
users can blindly run the test and let it figure out if it is
appropriate for the hardware.

>
> In my case there is 27 IRQs, 18 of them have non-zero counters and there
> is 12 CPUs. For active IRQs that gives 18/12 = 1.5 IRQ per CPU and there
> are 7 CPUs that handle 2 IRQs, 4 CPUs that handle 1 IRQ and 1 CPU that
> doesn't handle anything. I wonder if we can figure out some kind of
> heuristic, but I guess that in the end it wouldn't be worth the
> effort.

I general I think it is best to write tests which figure out if they are
in the right environment. This makes all our lives easier. The problem
with this test in particular is the number of assumptions we have to
make about what a good IRQ distirbution looks like.

IRQs are (usually) evenly distributed for "performance". However not all
IRQs are equal, you might get better performance through an uneven
distribution. Especially if you only care about the performance some
tasks and not others. Also there are some reasons to isolate some CPUs
from IRQs regardless of performance.

Having said that we can probably find a heuristic for what a very bad
setup looks like i.e. (almost) all NIC IRQs on one CPU when there are
30+ CPUs and multiple NICs. That is one CPU is totally saturated with
IRQs from different NICs. This is a bit speculative so I'd like some
feedback from testing/community before putting effort into that.

-- 
Thank you,
Richard.

-- 
Mailing list info: https://lists.linux.it/listinfo/ltp