linux-kernel - Re: [PATCH] Choose CPU based on allocated IRQs

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Mon, 29 Oct 2018 22:29:05 +0100 (CET)
From:   Thomas Gleixner <tglx@...utronix.de>
To:     Long Li <longli@...rosoft.com>
cc:     LKML <linux-kernel@...r.kernel.org>,
        Michael Kelley <mikelley@...rosoft.com>
Subject: Re: [PATCH] Choose CPU based on allocated IRQs

Long,

On Tue, 23 Oct 2018, Long Li wrote:

thanks for this patch.

A trivial formal thing ahead. The subject line

   [PATCH] Choose CPU based on allocated IRQs

is lacking a proper subsystem prefix. In most cases you can figure the
prefix out by running 'git log path/to/file' which in this case will show
you that most commits touching this file use the prefix 'genirq/matrix:'.

So the proper subject would be:

   [PATCH] genirq/matrix: Choose CPU based on allocated IRQs

Subsystem prefixes are important to see where a patch belongs to right from
the subject. Without that it could belong to any random part of the kernel
and needs further inspection of the patch itself. This applies to both
email and to git shortlog listings.

> From: Long Li <longli@...rosoft.com>
> 
> In irq_matrix, "available" is set when IRQs are allocated earlier in the IRQ
> assigning process.
> 
> Later, when IRQs are activated those values are not good indicators of what
> CPU to choose to assign to this IRQ.

Can you please explain why you think that available is the wrong indicator
and which problem you are trying to solve?

The WHY is really the most important part of a changelog.

> Change to choose CPU for an IRQ based on how many IRQs are already allocated
> on this CPU.

Looking deeper. The initial values are:

    available = alloc_size - (managed + systembits)
    allocated = 0

There are two distinct functionalities which modify 'available' and
'allocated' (omitting the reverse operations for simplicity):

1) managed interrupts

   reserve_managed()
	managed++;
	available--;

   alloc_managed()
        allocated++;

2) regular interrupts

   alloc()
	allocated++;
	available--;

So 'available' can be lower than 'allocated' depending on the number of
reserved managed interrupts, which have not yet been activated.

So for all regular interrupts we really want to look at the number of
'available' vectors because the reserved managed ones are already accounted
there and they need to be taken into account.

For the spreading of managed interrupts in alloc_managed() that's indeed a
different story and 'allocated' is more correct. But even that is not
completely accurate and can lead to the wrong result. The accurate solution
would be to account the managed _and_ allocated vectors separately and do
the spreading for managed interrupts based on that.

Thanks,

	tglx