Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1724779 > unrolled thread

[PATCH 4/4][RFC v2] x86/apic: Spread the vectors by choosing the idlest CPU

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  [PATCH 4/4][RFC v2] x86/apic: Spread the vectors by choosing the idlest CPU Chen Yu <yu.c.chen@intel.com> - 2017-09-01 07:10 +0200
    Re: [PATCH 4/4][RFC v2] x86/apic: Spread the vectors by choosing  the idlest CPU Thomas Gleixner <tglx@linutronix.de> - 2017-09-03 20:20 +0200
      RFD: x86: Sanitize the vector allocator Thomas Gleixner <tglx@linutronix.de> - 2017-09-03 21:20 +0200
      Re: [PATCH 4/4][RFC v2] x86/apic: Spread the vectors by choosing  the idlest CPU Thomas Gleixner <tglx@linutronix.de> - 2017-09-06 01:00 +0200
        Re: [PATCH 4/4][RFC v2] x86/apic: Spread the vectors by choosing the  idlest CPU Yu Chen <yu.c.chen@intel.com> - 2017-09-06 06:40 +0200
          Re: [PATCH 4/4][RFC v2] x86/apic: Spread the vectors by choosing  the idlest CPU Thomas Gleixner <tglx@linutronix.de> - 2017-09-06 10:10 +0200
            Re: [PATCH 4/4][RFC v2] x86/apic: Spread the vectors by choosing  the idlest CPU Thomas Gleixner <tglx@linutronix.de> - 2017-09-07 08:00 +0200
              Re: [PATCH 4/4][RFC v2] x86/apic: Spread the vectors by choosing the  idlest CPU Yu Chen <yu.c.chen@intel.com> - 2017-09-07 10:40 +0200
                Re: [PATCH 4/4][RFC v2] x86/apic: Spread the vectors by choosing  the idlest CPU Thomas Gleixner <tglx@linutronix.de> - 2017-09-07 11:50 +0200
      Re: [PATCH 4/4][RFC v2] x86/apic: Spread the vectors by choosing the  idlest CPU Yu Chen <yu.c.chen@intel.com> - 2017-09-06 06:20 +0200
        Re: [PATCH 4/4][RFC v2] x86/apic: Spread the vectors by choosing         the idlest CPU Christoph Hellwig <hch@lst.de> - 2017-09-06 08:20 +0200
          Re: [PATCH 4/4][RFC v2] x86/apic: Spread the vectors by choosing the  idlest CPU Dan Williams <dan.j.williams@intel.com> - 2017-09-06 19:50 +0200
            Re: [PATCH 4/4][RFC v2] x86/apic: Spread the vectors by choosing the  idlest CPU Yu Chen <yu.c.chen@intel.com> - 2017-09-07 05:00 +0200
            Re: [PATCH 4/4][RFC v2] x86/apic: Spread the vectors by choosing  the idlest CPU Thomas Gleixner <tglx@linutronix.de> - 2017-09-07 08:00 +0200
              Re: [PATCH 4/4][RFC v2] x86/apic: Spread the vectors by choosing the  idlest CPU Dan Williams <dan.j.williams@intel.com> - 2017-09-07 08:30 +0200
                Re: [PATCH 4/4][RFC v2] x86/apic: Spread the vectors by choosing  the idlest CPU Thomas Gleixner <tglx@linutronix.de> - 2017-09-07 09:00 +0200

#1724779 — [PATCH 4/4][RFC v2] x86/apic: Spread the vectors by choosing the idlest CPU

This is the major logic to spread the vectors on different CPUs.
The main idea is to choose the 'idlest' CPU which has assigned
the least number of vectors as the candidate/hint for the vector
allocation domain, in the hope that the vector allocation domain
could leverage this hint to generate corresponding cpumask.

One of the requirements to do this vector spreading work comes from the
following hibernation problem found on a 16 cores server:

CPU 31 disable failed: CPU has 62 vectors assigned and there
are only 0 available.

2 issues were found after investigation:

1. The network driver has declared many vector resources via
   pci_enable_msix_range(), say, this driver might likely want
   to reserve 6 per logical CPU, then there would be 192 of them.
2. Besides, most of the vectors reserved by this driver are assigned
   on CPU0 due to the current code strategy, so there would be
   insufficient slots left on CPU0 to receive any migrated IRQs
   during CPU offine.

In theory after the commit c5cb83bb337c ("genirq/cpuhotplug: Handle
managed IRQs on CPU hotplug") the issue 1 has been solved with the
managed interrupt mechanism for multi queue devices, which no longer
migrate these interrupts. It simply shuts them down and restarts
them when the CPU comes back online.

However, according to the implementation of this network driver,
the commit mentioned above does not fully fix the issue 1.
Here's the framework of the network driver:

step 1. Reserved enough irq vectors and corresponding IRQs.
step 2. If the network is activated, invoke request_irq() to
        register the handler.
step 3. Invoke set_affinity() to spread the IRQs onto different
        CPUs, thus to spread the vectors too.

The problem is, if the network cable is not connected, step 2
and 3 will not get invoked, thus the IRQ vectors will not spread
on different CPUs and will still be allocated on CPU0. As a result
the CPUs will still get the offline failure.

Previously there were some discussion in the thread [1] about the
vector spread, and here are the suggestion from Thomas:

Q1:
    Rewrite that allocation code from scratch, use per cpu bitmaps,
    so we can do fast search over masks and keep track of
    the vectors which are used/free per cpu.
A1:
    per cpu bitmap was onced considered but this might not be
    as fast as the solution proposed here. That is, if we introduce the
    per cpu bitmap, we need to count the number of vectors assigned on
    each CPUs by cpumask_weight() and sort them in order to get the
    CPUs who have the least number of vectors assigned. By contrast,
    if we introduce the bitmaps for each vector number, we can get the
    'idle' CPU at the cost of nearly O(1).

    One scenario of this patch can be illustrated below:

    a) After initialization, vector_mask[0] is set with {CPU0...CPU31},
       other vector_mask[i] remain zero, which means that, CPU0...CPU31
       have zero vectors assigned.
    b) During driver probe, CPU0 is chosen to be assigned with one vector.
       Thus vector_mask[0] becomes {CPU1...CPU31}, and vector_mask[1] becomes
       {CPU0}.
    c) The next time the system tries to assign another vector, it searches from
       vector[0], because it is non-empty, CPU1 is chosen to place
       the vector. Thus vector_mask[0] becomes {CPU2...CPU31}, and vector_mask[1]
       becomes {CPU0, CPU1}.
    d) and so on, the vectors assignment will spread on different CPUs.

Q2:
    "respect the allocation domains"
A2:
    This solution provides a CPU as the param for the vector allocation domains,
    in the hope that the domains can leverage this hint to generate the cpumask
    with less vectors assigned. But yes, the CPU we select upfront gets fed into
    apic->vector_allocation_domain() might just say no because the CPU
    does not belong to it, but anyway this should somehow spread the vectors,
    although I can not find out a graceful solution to this.
    BTW, the vector allocation domain is default_vector_allocation_domain()
    in our case, thus it works.

Q3:
    "set a preference for the node on which the interrupt was allocated"
A3:
    This can be achieved by further optimization in the following way:
    a) Also record the number of vectors used per node.
    b) Each time invoking __assign_irq_vector, adjust the input mask
       by cpumask_and(mask, mask, node_mask_with_least_vectors())

[1] https://marc.info/?l=linux-kernel&m=149867663002202
or
[1] https://patchwork.kernel.org/patch/9725227/

Thanks for your time.

Cc: Thomas Gleixner <tglx@linutronix.de>
Cc: Ingo Molnar <mingo@redhat.com>
Cc: "H. Peter Anvin" <hpa@zytor.com>
Cc: "Rafael J. Wysocki" <rjw@rjwysocki.net>
Cc: Len Brown <lenb@kernel.org>
Cc: Dan Williams <dan.j.williams@intel.com>
Signed-off-by: Chen Yu <yu.c.chen@intel.com>
---
 arch/x86/kernel/apic/vector.c | 26 ++++++++++++++++++++++++--
 1 file changed, 24 insertions(+), 2 deletions(-)

diff --git a/arch/x86/kernel/apic/vector.c b/arch/x86/kernel/apic/vector.c
index b60cc66..c7f0e8b 100644
--- a/arch/x86/kernel/apic/vector.c
+++ b/arch/x86/kernel/apic/vector.c
@@ -145,6 +145,28 @@ static void free_apic_chip_data(struct apic_chip_data *data)
 	}
 }
 
+static int pick_next_cpu_hint(const struct cpumask *mask)
+{
+	int i;
+	struct cpumask search, tmp;
+
+	cpumask_and(&search, mask, cpu_online_mask);
+	/*
+	 * Search in the order from vectors_alloc_mask[0] to
+	 * vectors_alloc_mask[255], try to find an intersection
+	 * between the mask and vectors_alloc_mask[],
+	 * return the first CPU in this intersection, thus
+	 * this CPU has the least number of vectors allocated.
+	 */
+	for (i = 0; i < NR_VECTORS; i++) {
+		cpumask_and(&tmp, &search, vectors_alloc_mask[i]);
+		if (!cpumask_empty(&tmp))
+			return cpumask_first(&tmp);
+	}
+	/* This should not happened...*/
+	return cpumask_first_and(mask, cpu_online_mask);
+}
+
 static int __assign_irq_vector(int irq, struct apic_chip_data *d,
 			       const struct cpumask *mask,
 			       struct irq_data *irqdata)
@@ -175,7 +197,7 @@ static int __assign_irq_vector(int irq, struct apic_chip_data *d,
 	/* Only try and allocate irqs on cpus that are present */
 	cpumask_clear(d->old_domain);
 	cpumask_clear(searched_cpumask);
-	cpu = cpumask_first_and(mask, cpu_online_mask);
+	cpu = pick_next_cpu_hint(mask);
 	while (cpu < nr_cpu_ids) {
 		int new_cpu, offset;
 
@@ -247,7 +269,7 @@ static int __assign_irq_vector(int irq, struct apic_chip_data *d,
 		 */
 		cpumask_or(searched_cpumask, searched_cpumask, vector_cpumask);
 		cpumask_andnot(vector_cpumask, mask, searched_cpumask);
-		cpu = cpumask_first_and(vector_cpumask, cpu_online_mask);
+		cpu = pick_next_cpu_hint(vector_cpumask);
 		continue;
 	}
 	return -ENOSPC;
-- 
2.7.4

[toc] | [next] | [standalone]

#1725721 — Re: [PATCH 4/4][RFC v2] x86/apic: Spread the vectors by choosing the idlest CPU

On Fri, 1 Sep 2017, Chen Yu wrote:

> This is the major logic to spread the vectors on different CPUs.
> The main idea is to choose the 'idlest' CPU which has assigned
> the least number of vectors as the candidate/hint for the vector
> allocation domain, in the hope that the vector allocation domain
> could leverage this hint to generate corresponding cpumask.
> 
> One of the requirements to do this vector spreading work comes from the
> following hibernation problem found on a 16 cores server:
> 
> CPU 31 disable failed: CPU has 62 vectors assigned and there
> are only 0 available.
> 
> 2 issues were found after investigation:
> 
> 1. The network driver has declared many vector resources via
>    pci_enable_msix_range(), say, this driver might likely want
>    to reserve 6 per logical CPU, then there would be 192 of them.
> 2. Besides, most of the vectors reserved by this driver are assigned
>    on CPU0 due to the current code strategy, so there would be
>    insufficient slots left on CPU0 to receive any migrated IRQs
>    during CPU offine.
> 
> In theory after the commit c5cb83bb337c ("genirq/cpuhotplug: Handle
> managed IRQs on CPU hotplug") the issue 1 has been solved with the
> managed interrupt mechanism for multi queue devices, which no longer
> migrate these interrupts. It simply shuts them down and restarts
> them when the CPU comes back online.
> 
> However, according to the implementation of this network driver,
> the commit mentioned above does not fully fix the issue 1.
> Here's the framework of the network driver:
> 
> step 1. Reserved enough irq vectors and corresponding IRQs.
> step 2. If the network is activated, invoke request_irq() to
>         register the handler.
> step 3. Invoke set_affinity() to spread the IRQs onto different
>         CPUs, thus to spread the vectors too.
>
> The problem is, if the network cable is not connected, step 2
> and 3 will not get invoked, thus the IRQ vectors will not spread
> on different CPUs and will still be allocated on CPU0. As a result
> the CPUs will still get the offline failure.

So the proper solution for this network driver is to switch to managed
interrupts instead of trying to work around it in some other place. It's
using the wrong mechanism - end of story.

Why are you insisting on implementing a band aid for this particular driver
instead of fixing the underlying problem of that driver which requires to
have 32 queues and interrupts open even if only a single CPU is online?

> Previously there were some discussion in the thread [1] about the
> vector spread, and here are the suggestion from Thomas:
> 
> Q1:
>     Rewrite that allocation code from scratch, use per cpu bitmaps,
>     so we can do fast search over masks and keep track of
>     the vectors which are used/free per cpu.
> A1:
>     per cpu bitmap was onced considered but this might not be
>     as fast as the solution proposed here. That is, if we introduce the
>     per cpu bitmap, we need to count the number of vectors assigned on
>     each CPUs by cpumask_weight() and sort them in order to get the
>     CPUs who have the least number of vectors assigned. By contrast,
>     if we introduce the bitmaps for each vector number, we can get the
>     'idle' CPU at the cost of nearly O(1).

The weight accounting with the cpumasks is an orthogonal issue to the per
cpu vector bitmaps.

Once you selected a CPU the current code still loops in circles instead of
just searching a bitmap.

And if the required affinity mask needs more than one CPU then this is
still not giving you the right answer. You assign perhaps a vector to the
least busy CPU, but the other CPUs which happen to be in the mask are going
to be randomly selected.

I'm not against making the vector allocation better, but certainly not by
adding yet more duct tape to something which is well known as one of the
dumbest search algorithms on the planet with a worst case of

       nvectors * nr_online_cpus * nr_online_cpus_in_affinity_mask

and your mechanism nests another loop of potentially NR_VECTORS into
that. Which is pointless as the actual assignable vector space is
smaller. While x86 has 256 vectors the assignable vector space is way
smaller and non continous:

Vectors   0- 31 are reserved for CPU traps and exceptions
Vectors 239-255 are reserved by the kernel for IPIs, local timer etc.
Vector	     32 can be reserved by the kernel for the reboot interrupt
Vector	     96 can be reserved by the kernel for INT80

So the actually usable array size is between 205 and 207.

Aside of that the code is incorrect vs. the percpu accounting. Remove the
limit in the update function and see what happens. While the limit is
necessary in general, it should at least warn and yell whenever the
accounting goes out of bounds. And if that happens, then the whole thing is
completely useless simply because the numbers are just wrong.

>     One scenario of this patch can be illustrated below:
> 
>     a) After initialization, vector_mask[0] is set with {CPU0...CPU31},
>        other vector_mask[i] remain zero, which means that, CPU0...CPU31
>        have zero vectors assigned.
>     b) During driver probe, CPU0 is chosen to be assigned with one vector.
>        Thus vector_mask[0] becomes {CPU1...CPU31}, and vector_mask[1] becomes
>        {CPU0}.
>     c) The next time the system tries to assign another vector, it searches from
>        vector[0], because it is non-empty, CPU1 is chosen to place
>        the vector. Thus vector_mask[0] becomes {CPU2...CPU31}, and vector_mask[1]
>        becomes {CPU0, CPU1}.
>     d) and so on, the vectors assignment will spread on different CPUs.

So this works for your particular network driver scenario, which is the
wrong example as this driver just needs to be reworked to not expose that
issue.

The reality of affinity requirements is not that simple. It's very much
device dependent and making it mandatory can have negative side effects.

> Q2:
>     "respect the allocation domains"
> A2:
>     This solution provides a CPU as the param for the vector allocation domains,

Solution? This is a crude hack which "solves" one particular problem in the
wrong way.

>     in the hope that the domains can leverage this hint to generate the cpumask

Hope is never a good enginering principle.

>     with less vectors assigned. But yes, the CPU we select upfront gets fed into
>     apic->vector_allocation_domain() might just say no because the CPU
>     does not belong to it, but anyway this should somehow spread the vectors,
>     although I can not find out a graceful solution to this.
>     BTW, the vector allocation domain is default_vector_allocation_domain()
>     in our case, thus it works.

Now try that with the logical delivery modes which allow multi CPU
assignements and the cluster mode thereof.

> Q3:
>     "set a preference for the node on which the interrupt was allocated"
> A3:
>     This can be achieved by further optimization in the following way:
>     a) Also record the number of vectors used per node.
>     b) Each time invoking __assign_irq_vector, adjust the input mask
>        by cpumask_and(mask, mask, node_mask_with_least_vectors())

I'm not looking forward to that heuristics and the next loop inside the
loops of loops.

So no, this is not going to happen. The current implementation sucks and
needs a rewrite.

I already started to look into that, but started at the low level end of
it. See the IDT cleanup series in tip:x86/apic. There is much more vooddo
in the vector management code to clean up before we can actually talk about
making the assignment algorithm smarter.

I'll send out a separate mail raising a few points to discuss before we are
actually starting to look into magic spreading functionality.

Thanks,

	tglx

[toc] | [prev] | [next] | [standalone]

#1725737 — RFD: x86: Sanitize the vector allocator

[Multipart message — attachments visible in raw view] — view raw

The vector allocator of x86 is a pretty stupid linear search algorithm with
a worst case of

    nr_vectors * nr_online_cpus * nr_cpus_in_affinity mask

It has some other magic properties and really wants to be replaced by
something smarter.

That needs quite some cleanup of the vector management code outside of the
allocator, which I started to work on with the cleanup of the IDT
management which is headed for 4.14. I have some other things in the
pipeline which eliminate quite some duct tape in that area, but I ran into
a couple of interesting things:

1) Multi CPU affinities

   This is only vailable when the APIC is using logical destination
   mode. With physical destination mode there is already a restriction to a
   single CPU target.

   The multi CPU affinity is biased towards the CPU with the lowest APIC ID
   in the destination bitfield. Only if that APIC is busy (ISR not empty)
   then the next APIC gets it.

   A full kernel build on a SKL 4 CPU desktop machine with affinity set to
   CPU0-3 shows that more than 90 percent of the AHCI interrupts end up on
   CPU0.

   Aside of that the same cold build (right after boot) is about 2% faster
   when the AHCI interrupt is only affine to CPU0.

   I did some experiments on all my machines which have logical destination
   mode with various workloads and the results are similiar. The
   distribution of interrupts on the CPUs varies with the workloads, but
   the vast majority always ends up on CPU0

   I've not found a case where the multi CPU affinity is superiour. I might
   have the wrong workloads and the wrong machines, but it would be
   extremly helpful just to get rid of this and use single CPU affinities
   only. That'd simplify the allocator along with the various APIC
   implementations.


2) The 'priority level' spreading magic

   The comment in __asign_irq_vector says:

      * NOTE! The local APIC isn't very good at handling
      * multiple interrupts at the same interrupt level.
      * As the interrupt level is determined by taking the
      * vector number and shifting that right by 4, we
      * want to spread these out a bit so that they don't
      * all fall in the same interrupt level.                         

   After doing some palaeontological research I found the following in the
   PPro Developer Manual Volume 3:

     "7.4.2. Valid Interrupts

     The local and I/O APICs support 240 distinct vectors in the range of 16
     to 255. Interrupt priority is implied by its vector, according to the
     following relationship: priority = vector / 16

     One is the lowest priority and 15 is the highest. Vectors 16 through
     31 are reserved for exclusive use by the processor. The remaining
     vectors are for general use. The processor’s local APIC includes an
     in-service entry and a holding entry for each priority level. To avoid
     losing inter- rupts, software should allocate no more than 2 interrupt
     vectors per priority."

   The current SDM tells nothing about that, instead it states:

     "If more than one interrupt is generated with the same vector number,
      the local APIC can set the bit for the vector both in the IRR and the
      ISR. This means that for the Pentium 4 and Intel Xeon processors, the
      IRR and ISR can queue two interrupts for each interrupt vector: one
      in the IRR and one in the ISR. Any additional interrupts issued for
      the same interrupt vector are collapsed into the single bit in the
      IRR.

      For the P6 family and Pentium processors, the IRR and ISR registers
      can queue no more than two interrupts per interrupt vector and will
      reject other interrupts that are received within the same vector."

   Which means, that on P6/Pentium the APIC will reject a new message and
   tell the sender to retry, which increases the load on the APIC bus and
   nothing more.

   There is no affirmative answer from Intel on that, but I think it's sane
   to remove that:

    1) I've looked through a bunch of other operating systems and none of
       them bothers to implement this or mentiones this at all.

    2) The current allocator has no enforcement for this and especially the
       legacy interrupts, which are the main source of interrupts on these
       P6 and older systmes, are allocated linearly in the same priority
       level and just work.

    3) The current machines have no problem with that at all as I verified
       with some experiments.

    4) AMD at least confirmed that such an issue is unknown.

    5) P6 and older are dinosaurs almost 20 years EOL, so we really should
       not worry about that anymore.

   So this can be eliminated, which makes the allocation mechanism way
   simpler.


Some other issues which are not in the way of cleanups and replacements,
but need to be looked at as well:

1) Automated affinity assignment

    This only helps when the underlying device requests it and has the
    matching queues per CPU. That's what the managed interrupt affinity
    mechanism was made for.

    In other cases the automated assignment can have really bad effects.

    On the same SKL as above I made the AHCI interrupt affine to CPU3 only
    which makes the kernel build slower by whopping 10% than having it
    affine on CPU0. Interestingly enough irqbalanced end up with the wrong
    decision as well.

    So we need to be very careful about that. It depends on the device and
    the driver how good 'random' placement works.

    That means we need hinting from the drivers about their preferred
    allocation scheme. If we don't have that then we should for now default
    to the current scheme which puts the interrupt on the node on which the
    device is.


2) Vector waste

   All 16 legacy interrupt vectors are populated at boot and stay there
   forever whether they are used or not. On most modern machines that's 10+
   vectors wasted for nothing. If the APIC uses logical destination mode
   that means these vectors are per default allocated on up to 8 CPUs or in
   the case of clustered X2APIC on all CPUs in a cluster.

   It'd be worthwhile to allocate these legacy vectors dynamically when
   they are actually used. That might fail, but that's the same on devices
   which use MSI etc. For legacy systems this is a non issue as there are
   plenty of vectors available. On modern machines the 4-5 really used
   legacy vectors are requested early during the boot process and should
   not end up in a fully exhausted vector space.

   Nothing urgent, but worthwhile to fix I think.

Thoughts?

Thanks,

     tglx

[toc] | [prev] | [next] | [standalone]

#1727066 — Re: [PATCH 4/4][RFC v2] x86/apic: Spread the vectors by choosing the idlest CPU

On Sun, 3 Sep 2017, Thomas Gleixner wrote:

> On Fri, 1 Sep 2017, Chen Yu wrote:
> 
> > This is the major logic to spread the vectors on different CPUs.
> > The main idea is to choose the 'idlest' CPU which has assigned
> > the least number of vectors as the candidate/hint for the vector
> > allocation domain, in the hope that the vector allocation domain
> > could leverage this hint to generate corresponding cpumask.
> > 
> > One of the requirements to do this vector spreading work comes from the
> > following hibernation problem found on a 16 cores server:
> > 
> > CPU 31 disable failed: CPU has 62 vectors assigned and there
> > are only 0 available.

Thinking more about this, this makes no sense whatsoever.

The total number of interrupts on a system is the same whether they are
all on CPU 0 or evenly spread over all CPUs.

As this machine is using physcial destination mode, the number of vectors
used is the same as the number of interrupts, except for the case where a
move of an interrupt is in progress and the interrupt which cleans up the
old vector has not yet arrived. Lets ignore that for now.

The available vector space is 204 per CPU on such a system.

    256 - SYSTEM[0-31, 32, 128, 239-255] - LEGACY[50] = 204

> > CPU 31 disable failed: CPU has 62 vectors assigned and there
> > are only 0 available.

CPU31 is the last AP going offline (CPU0 is still online).

It wants to move 62 vectors to CPU0, but it can't because CPU0 has 0
available vectors. That means CPU0 has 204 vectors used. I doubt that, but
what I doubt even more is that this interrupt spreading helps in any way.

Assumed that we have a total of 204 + 62 = 266 device interrupt vectors in
use and they are evenly spread over 32 CPUs, so each CPU has either 8 or
nine vectors. Fine.

Now if you unplug all CPUs except CPU0 starting from CPU1 up to CPU31 then
at the point where CPU31 is about to be unplugged, CPU0 holds 133 vectors
and CPU31 holds 133 vectors as well - assumed that the spread is exactly
even.

I have a hard time to figure out how the 133 vectors on CPU31 are now
magically fitting in the empty space on CPU0, which is 204 - 133 = 71. In
my limited understanding of math 133 is greater than 71, but your patch
might make that magically be wrong.

Can you please provide detailed information about how many device
interrupts are actually in use/allocated on that system?

Please enable CONFIG_GENERIC_IRQ_DEBUGFS and provide the output of 

# cat /sys/kernel/debug/irq/domains/*

and

# ls /sys/kernel/debug/irq/irqs

Thanks,

	tglx





   

[toc] | [prev] | [next] | [standalone]

#1727148 — Re: [PATCH 4/4][RFC v2] x86/apic: Spread the vectors by choosing the idlest CPU

On Wed, Sep 06, 2017 at 12:57:41AM +0200, Thomas Gleixner wrote:
> On Sun, 3 Sep 2017, Thomas Gleixner wrote:
> 
> > On Fri, 1 Sep 2017, Chen Yu wrote:
> > 
> > > This is the major logic to spread the vectors on different CPUs.
> > > The main idea is to choose the 'idlest' CPU which has assigned
> > > the least number of vectors as the candidate/hint for the vector
> > > allocation domain, in the hope that the vector allocation domain
> > > could leverage this hint to generate corresponding cpumask.
> > > 
> > > One of the requirements to do this vector spreading work comes from the
> > > following hibernation problem found on a 16 cores server:
> > > 
> > > CPU 31 disable failed: CPU has 62 vectors assigned and there
> > > are only 0 available.
> 
> Thinking more about this, this makes no sense whatsoever.
> 
> The total number of interrupts on a system is the same whether they are
> all on CPU 0 or evenly spread over all CPUs.
> 
> As this machine is using physcial destination mode, the number of vectors
> used is the same as the number of interrupts, except for the case where a
> move of an interrupt is in progress and the interrupt which cleans up the
> old vector has not yet arrived. Lets ignore that for now.
> 
> The available vector space is 204 per CPU on such a system.
> 
>     256 - SYSTEM[0-31, 32, 128, 239-255] - LEGACY[50] = 204
> 
> > > CPU 31 disable failed: CPU has 62 vectors assigned and there
> > > are only 0 available.
> 
> CPU31 is the last AP going offline (CPU0 is still online).
> 
> It wants to move 62 vectors to CPU0, but it can't because CPU0 has 0
> available vectors. That means CPU0 has 204 vectors used. I doubt that, but
> what I doubt even more is that this interrupt spreading helps in any way.
> 
> Assumed that we have a total of 204 + 62 = 266 device interrupt vectors in
> use and they are evenly spread over 32 CPUs, so each CPU has either 8 or
> nine vectors. Fine.
> 
> Now if you unplug all CPUs except CPU0 starting from CPU1 up to CPU31 then
> at the point where CPU31 is about to be unplugged, CPU0 holds 133 vectors
> and CPU31 holds 133 vectors as well - assumed that the spread is exactly
> even.
> 
> I have a hard time to figure out how the 133 vectors on CPU31 are now
> magically fitting in the empty space on CPU0, which is 204 - 133 = 71. In
> my limited understanding of math 133 is greater than 71, but your patch
> might make that magically be wrong.
>
The problem is reproduced when the network cable is not plugged in,
because this driver looks like this:

step 1. Reserved enough irq vectors and corresponding IRQs.
step 2. If the network is activated, invoke request_irq() to
        register the handler.
step 3. Invoke set_affinity() to spread the IRQs onto different
        CPUs, thus to spread the vectors too.

Here's my understanding for why spreading vectors might help for this
special case: 
As step 2 will not get invoked, the IRQs of this driver
has not been enabled, thus in migrate_one_irq() this IRQ
will not be considered because there is a check of
irqd_is_started(d), thus there should only be 8 vectors
allocated by this driver on CPU0, and 8 vectors left on
CPU31, and the 8 vectors on CPU31 will not be migrated
to CPU0 neither, so there is room for other 'valid' vectors
to be migrated to CPU0.
> Can you please provide detailed information about how many device
> interrupts are actually in use/allocated on that system?
> 
> Please enable CONFIG_GENERIC_IRQ_DEBUGFS and provide the output of 
> 
> # cat /sys/kernel/debug/irq/domains/*
>
> and
> 
> # ls /sys/kernel/debug/irq/irqs
>
Ok, here's the information after system bootup on top
of 4.13:

# cat /sys/kernel/debug/irq/domains/*
name:   VECTOR
 size:   0
 mapped: 388
 flags:  0x00000041
name:   IO-APIC-0
 size:   24
 mapped: 16
 flags:  0x00000041
 parent: VECTOR
    name:   VECTOR
     size:   0
     mapped: 388
     flags:  0x00000041
name:   IO-APIC-1
 size:   8
 mapped: 2
 flags:  0x00000041
 parent: VECTOR
    name:   VECTOR
     size:   0
     mapped: 388
     flags:  0x00000041
name:   IO-APIC-2
 size:   8
 mapped: 0
 flags:  0x00000041
 parent: VECTOR
    name:   VECTOR
     size:   0
     mapped: 388
     flags:  0x00000041
name:   IO-APIC-3
 size:   8
 mapped: 0
 flags:  0x00000041
 parent: VECTOR
    name:   VECTOR
     size:   0
     mapped: 388
     flags:  0x00000041
name:   IO-APIC-4
 size:   8
 mapped: 5
 flags:  0x00000041
 parent: VECTOR
    name:   VECTOR
     size:   0
     mapped: 388
     flags:  0x00000041
name:   PCI-HT
 size:   0
 mapped: 0
 flags:  0x00000041
 parent: VECTOR
    name:   VECTOR
     size:   0
     mapped: 388
     flags:  0x00000041
name:   PCI-MSI-2
 size:   0
 mapped: 365
 flags:  0x00000051
 parent: VECTOR
    name:   VECTOR
     size:   0
     mapped: 388
     flags:  0x00000041
name:   VECTOR
 size:   0
 mapped: 388
 flags:  0x00000041


# ls /sys/kernel/debug/irq/irqs
ls /sys/kernel/debug/irq/irqs
0  10   11  13  142  184  217  259  292  31  33   337  339
340  342  344  346  348  350  352  354  356  358  360  362
364  366  368  370  372  374  376  378  380  382  384  386
388  390  392  394  4  6   7  9  1  109  12  14  15   2
24   26   3    32  335  338  34   341  343  345  347  349
351  353  355  357  359  361  363  365  367  369  371  373
375  377  379  381  383  385  387  389  391  393  395  5
67  8

BTW, do we have sysfs to display how much vectors used on each CPUs?

Thanks,
	Yu

> Thanks,
> 
> 	tglx
> 
> 
> 
> 
> 
>    
> 
> 
> 
> 
> 

[toc] | [prev] | [next] | [standalone]

#1727194 — Re: [PATCH 4/4][RFC v2] x86/apic: Spread the vectors by choosing the idlest CPU

On Wed, 6 Sep 2017, Yu Chen wrote:
> On Wed, Sep 06, 2017 at 12:57:41AM +0200, Thomas Gleixner wrote:
> > I have a hard time to figure out how the 133 vectors on CPU31 are now
> > magically fitting in the empty space on CPU0, which is 204 - 133 = 71. In
> > my limited understanding of math 133 is greater than 71, but your patch
> > might make that magically be wrong.
> >
> The problem is reproduced when the network cable is not plugged in,
> because this driver looks like this:
> 
> step 1. Reserved enough irq vectors and corresponding IRQs.
> step 2. If the network is activated, invoke request_irq() to
>         register the handler.
> step 3. Invoke set_affinity() to spread the IRQs onto different
>         CPUs, thus to spread the vectors too.
> 
> Here's my understanding for why spreading vectors might help for this
> special case: 
> As step 2 will not get invoked, the IRQs of this driver
> has not been enabled, thus in migrate_one_irq() this IRQ
> will not be considered because there is a check of
> irqd_is_started(d), thus there should only be 8 vectors
> allocated by this driver on CPU0, and 8 vectors left on
> CPU31, and the 8 vectors on CPU31 will not be migrated
> to CPU0 neither, so there is room for other 'valid' vectors
> to be migrated to CPU0.

Can you please spare me repeating your theories, as long as you don't have
hard facts to back them up? The network cable is changing the symptoms,
but the underlying root cause is definitely something different.

> # cat /sys/kernel/debug/irq/domains/*
> name:   VECTOR
>  size:   0
>  mapped: 388
>  flags:  0x00000041

So we have 388 vectors mapped in total. And those are just device vectors
because system vectors are not accounted there.

> name:   IO-APIC-0
>  size:   24
>  mapped: 16

That's the legacy space

> name:   IO-APIC-1
>  size:   8
>  mapped: 2

> name:   IO-APIC-2
>  size:   8
>  mapped: 0

> name:   IO-APIC-3
>  size:   8
>  mapped: 0

> name:   IO-APIC-4
>  size:   8
>  mapped: 5

And a few GSIs: Total GSIs = 16 + 2 + 5 = 23

> name:   PCI-MSI-2
>  size:   0
>  mapped: 365

Plus 365 PCI-MSI vectors allocated.

>  flags:  0x00000051
>  parent: VECTOR
>     name:   VECTOR
>      size:   0
>      mapped: 388

Which nicely sums up to 388

> # ls /sys/kernel/debug/irq/irqs
> ls /sys/kernel/debug/irq/irqs
> 0  10   11  13  142  184  217  259  292  31  33   337  339
> 340  342  344  346  348  350  352  354  356  358  360  362
> 364  366  368  370  372  374  376  378  380  382  384  386
> 388  390  392  394  4  6   7  9  1  109  12  14  15   2
> 24   26   3    32  335  338  34   341  343  345  347  349
> 351  353  355  357  359  361  363  365  367  369  371  373
> 375  377  379  381  383  385  387  389  391  393  395  5
> 67  8

That are all interrupts which are active. That's a total of 89. Can you
explain where the delta of 299 vectors comes from?

299 allocated, vector mapped, but unused interrupts?

That's where your problem is, not in the vector spreading. You have a
massive leak.

> BTW, do we have sysfs to display how much vectors used on each CPUs?

Not yet.

Can you please apply the debug patch below, boot the machine and right
after login provide the output of

# cat /sys/kernel/debug/tracing/trace

Thanks,

	tglx

8<-------------------
--- a/kernel/irq/msi.c
+++ b/kernel/irq/msi.c
@@ -372,6 +372,9 @@ int msi_domain_alloc_irqs(struct irq_dom
 			return ret;
 		}
 
+		trace_printk("dev: %s nvec %d virq %d\n",
+			     dev_name(dev), desc->nvec_used, virq);
+
 		for (i = 0; i < desc->nvec_used; i++)
 			irq_set_msi_desc_off(virq, i, desc);
 	}
@@ -419,6 +422,8 @@ void msi_domain_free_irqs(struct irq_dom
 		 * entry. If that's the case, don't do anything.
 		 */
 		if (desc->irq) {
+			trace_printk("dev: %s nvec %d virq %d\n",
+				     dev_name(dev), desc->nvec_used, desc->irq);
 			irq_domain_free_irqs(desc->irq, desc->nvec_used);
 			desc->irq = 0;
 		}

[toc] | [prev] | [next] | [standalone]

#1727922 — Re: [PATCH 4/4][RFC v2] x86/apic: Spread the vectors by choosing the idlest CPU

On Thu, 7 Sep 2017, Yu Chen wrote:
> On Wed, Sep 06, 2017 at 10:03:58AM +0200, Thomas Gleixner wrote:
> > Can you please apply the debug patch below, boot the machine and right
> > after login provide the output of
> > 
> > # cat /sys/kernel/debug/tracing/trace
> >
>      kworker/0:2-303   [000] ....     9.135467: msi_domain_alloc_irqs: dev: 0000:bb:00.0 nvec 1 virq 34
>      kworker/0:2-303   [000] ....     9.135476: msi_domain_alloc_irqs: dev: 0000:bb:00.0 nvec 1 virq 35
>      kworker/0:2-303   [000] ....     9.135484: msi_domain_alloc_irqs: dev: 0000:bb:00.0 nvec 1 virq 36

<SNIP>

>      kworker/0:2-303   [000] ....     9.762268: msi_domain_alloc_irqs: dev: 0000:bb:00.3 nvec 1 virq 331
>      kworker/0:2-303   [000] ....     9.762278: msi_domain_alloc_irqs: dev: 0000:bb:00.3 nvec 1 virq 332
>      kworker/0:2-303   [000] ....     9.762288: msi_domain_alloc_irqs: dev: 0000:bb:00.3 nvec 1 virq 333

That's 300 vectors.

>  bb:00.[0-3] Ethernet controller: Intel Corporation Device 37d0 (rev 03)
> 
> -+-[0000:b2]-+-00.0-[b3-bc]----00.0-[b4-bc]--+-00.0-[b5-b6]----00.0
>  |           |                               +-01.0-[b7-b8]----00.0
>  |           |                               +-02.0-[b9-ba]----00.0
>  |           |                               \-03.0-[bb-bc]--+-00.0
>  |           |                                               +-00.1
>  |           |                                               +-00.2
>  |           |                                               \-00.3
> 
> and they are using i40e driver, the vectors should be reserved by:
> i40e_probe() ->
>   i40e_init_interrupt_scheme() ->
>     i40e_init_msix() ->
>       i40e_reserve_msix_vectors() ->
>         pci_enable_msix_range()
> 
> # ls /sys/kernel/debug/irq/irqs
> 0  10   11  13  142  184  217  259  292  31  33
> 337  339  340  342  344  346  348  350  352  354  356
> 358  360  362  364  366  368  370  372  374  376  378
> 380  382  384  386  388  390  392  394  4  6   7  9
> 1  109  12  14  15   2    24   26   3    32  335
> 338  34   341  343  345  347  349  351  353  355  357
> 359  361  363  365  367  369  371  373  375  377  379
> 381  383  385  387  389  391  393  395  5  67  8

Out of these 300 interrupts exactly 8 randomly selected ones are actively
used. And the other 292 interrupts are just there because it might need
them in the future when the 32 CPU machine gets magically upgraded to 4096
cores at runtime?

Can the i40e people @intel please fix this waste of resources and sanitize
their interrupt allocation scheme?

Please switch it over to managed interrupts so the affinity spreading
happens in a sane way and the interrupts are properly managed on CPU
hotplug.

Thanks,

	tglx

[toc] | [prev] | [next] | [standalone]

#1728035 — Re: [PATCH 4/4][RFC v2] x86/apic: Spread the vectors by choosing the idlest CPU

On Thu, Sep 07, 2017 at 07:54:09AM +0200, Thomas Gleixner wrote:
> On Thu, 7 Sep 2017, Yu Chen wrote:
> > On Wed, Sep 06, 2017 at 10:03:58AM +0200, Thomas Gleixner wrote:
> > > Can you please apply the debug patch below, boot the machine and right
> > > after login provide the output of
> > > 
> > > # cat /sys/kernel/debug/tracing/trace
> > >
> >      kworker/0:2-303   [000] ....     9.135467: msi_domain_alloc_irqs: dev: 0000:bb:00.0 nvec 1 virq 34
> >      kworker/0:2-303   [000] ....     9.135476: msi_domain_alloc_irqs: dev: 0000:bb:00.0 nvec 1 virq 35
> >      kworker/0:2-303   [000] ....     9.135484: msi_domain_alloc_irqs: dev: 0000:bb:00.0 nvec 1 virq 36
> 
> <SNIP>
> 
> >      kworker/0:2-303   [000] ....     9.762268: msi_domain_alloc_irqs: dev: 0000:bb:00.3 nvec 1 virq 331
> >      kworker/0:2-303   [000] ....     9.762278: msi_domain_alloc_irqs: dev: 0000:bb:00.3 nvec 1 virq 332
> >      kworker/0:2-303   [000] ....     9.762288: msi_domain_alloc_irqs: dev: 0000:bb:00.3 nvec 1 virq 333
> 
> That's 300 vectors.
> 
> >  bb:00.[0-3] Ethernet controller: Intel Corporation Device 37d0 (rev 03)
> > 
> > -+-[0000:b2]-+-00.0-[b3-bc]----00.0-[b4-bc]--+-00.0-[b5-b6]----00.0
> >  |           |                               +-01.0-[b7-b8]----00.0
> >  |           |                               +-02.0-[b9-ba]----00.0
> >  |           |                               \-03.0-[bb-bc]--+-00.0
> >  |           |                                               +-00.1
> >  |           |                                               +-00.2
> >  |           |                                               \-00.3
> > 
> > and they are using i40e driver, the vectors should be reserved by:
> > i40e_probe() ->
> >   i40e_init_interrupt_scheme() ->
> >     i40e_init_msix() ->
> >       i40e_reserve_msix_vectors() ->
> >         pci_enable_msix_range()
> > 
> > # ls /sys/kernel/debug/irq/irqs
> > 0  10   11  13  142  184  217  259  292  31  33
> > 337  339  340  342  344  346  348  350  352  354  356
> > 358  360  362  364  366  368  370  372  374  376  378
> > 380  382  384  386  388  390  392  394  4  6   7  9
> > 1  109  12  14  15   2    24   26   3    32  335
> > 338  34   341  343  345  347  349  351  353  355  357
> > 359  361  363  365  367  369  371  373  375  377  379
> > 381  383  385  387  389  391  393  395  5  67  8
> 
> Out of these 300 interrupts exactly 8 randomly selected ones are actively
> used. And the other 292 interrupts are just there because it might need
> them in the future when the 32 CPU machine gets magically upgraded to 4096
> cores at runtime?
>
Humm, the 292 vectors remain disabled due to the network devices have
not been enabled(say,ifconfig up does not get invoked), so request_irq()
does not get invoked for these vectors? I have an impression that once
I've borrowed some fiber cables to connect the platform, the active IRQ
from i40e raised a lot, although I don't have these expensive cables
now...
> Can the i40e people @intel please fix this waste of resources and sanitize
> their interrupt allocation scheme?
> 
> Please switch it over to managed interrupts so the affinity spreading
> happens in a sane way and the interrupts are properly managed on CPU
> hotplug.
Ok, I think currently in i40e driver the reservation of vectors
leverages pci_enable_msix_range() and did not provide the affinity
hit to low level IRQ system thus the managed interrupts is not enabled
there(although later in i40e driver we use irq_set_affinity_hint() to
spread the IRQs)

Thanks,
	Yu
> 
> Thanks,
> 
> 	tglx

[toc] | [prev] | [next] | [standalone]

#1728097 — Re: [PATCH 4/4][RFC v2] x86/apic: Spread the vectors by choosing the idlest CPU

On Thu, 7 Sep 2017, Yu Chen wrote:
> On Thu, Sep 07, 2017 at 07:54:09AM +0200, Thomas Gleixner wrote:
> > Please switch it over to managed interrupts so the affinity spreading
> > happens in a sane way and the interrupts are properly managed on CPU
> > hotplug.
> Ok, I think currently in i40e driver the reservation of vectors
> leverages pci_enable_msix_range() and did not provide the affinity
> hit to low level IRQ system thus the managed interrupts is not enabled
> there(although later in i40e driver we use irq_set_affinity_hint() to
> spread the IRQs)

The affinity hint has nothing to do with that. It's a hint which tells user
space irqbalanced what the desired placement of the interrupt should
be. That was never used for spreading the affinity automatically in the
kernel and will never be used to do so. It was a design failure from the
very beginning and should be eliminated ASAP.

The general problem here is the way how the whole MSI(X) machinery works in
the kernel.

pci_enable_msix()

   allocate_interrupts()
     allocate_irqdescs()
       allocate_resources()
         allocate_DMAR_entries()
	   allocate_vectors()
	     initialize_MSI_entries()

The reason for this is historical. Drivers expect, that request_irq()
works, when they allocated the required resources upfront.

Of course this could be changed, but there are issues with that:

  1) The driver must ensure that it does not enable any of the internal
     interrupt delivery mechanisms in the device before request_irq() has
     succeeded.

     That needs auditing drivers all over the place or we just ignore that
     and leave everyone puzzled why things suddenly stop to work.

  2) Reservation accounting

     When no vectors are allocated, we still need to make reservations so
     we can tell a driver that the vector space is exhausted when it
     invokes pci_enable_msix(). But how do we size the reservation space?
     Based on nr_possible_cpus(), nr_online_cpus() or some other
     heuristics?

     Sure, we can just ignore that and resort to overcommitment and fail
     request_irq() when resources are not available, which brings us back
     to #1

But resorting to overcommitment does not make the cpu hotplug problem
magically go away. If queues and interrupts are used, then the non managed
variants are going to break affinities and move stuff to the still online
CPUs, which is going to fail.

Managed irqs just work because the driver stops the queue and the interrupt
(which can even stay requested) is shut down and 'kept' on the outgoing
CPU. If the CPU comes back then the vector is reestablished and the
interrupt started up on the fly. Stuff just works.....

Thanks,

	tglx


     

[toc] | [prev] | [next] | [standalone]

#1727143 — Re: [PATCH 4/4][RFC v2] x86/apic: Spread the vectors by choosing the idlest CPU

Thanks for looking at this,
(please bear my slow response as I need to check related code
before replying.)
On Sun, Sep 03, 2017 at 08:17:04PM +0200, Thomas Gleixner wrote:
> On Fri, 1 Sep 2017, Chen Yu wrote:
> 
> > This is the major logic to spread the vectors on different CPUs.
> > The main idea is to choose the 'idlest' CPU which has assigned
> > the least number of vectors as the candidate/hint for the vector
> > allocation domain, in the hope that the vector allocation domain
> > could leverage this hint to generate corresponding cpumask.
> > 
> > One of the requirements to do this vector spreading work comes from the
> > following hibernation problem found on a 16 cores server:
> > 
> > CPU 31 disable failed: CPU has 62 vectors assigned and there
> > are only 0 available.
> > 
> > 2 issues were found after investigation:
> > 
> > 1. The network driver has declared many vector resources via
> >    pci_enable_msix_range(), say, this driver might likely want
> >    to reserve 6 per logical CPU, then there would be 192 of them.
> > 2. Besides, most of the vectors reserved by this driver are assigned
> >    on CPU0 due to the current code strategy, so there would be
> >    insufficient slots left on CPU0 to receive any migrated IRQs
> >    during CPU offine.
> > 
> > In theory after the commit c5cb83bb337c ("genirq/cpuhotplug: Handle
> > managed IRQs on CPU hotplug") the issue 1 has been solved with the
> > managed interrupt mechanism for multi queue devices, which no longer
> > migrate these interrupts. It simply shuts them down and restarts
> > them when the CPU comes back online.
> > 
> > However, according to the implementation of this network driver,
> > the commit mentioned above does not fully fix the issue 1.
> > Here's the framework of the network driver:
> > 
> > step 1. Reserved enough irq vectors and corresponding IRQs.
> > step 2. If the network is activated, invoke request_irq() to
> >         register the handler.
> > step 3. Invoke set_affinity() to spread the IRQs onto different
> >         CPUs, thus to spread the vectors too.
> >
> > The problem is, if the network cable is not connected, step 2
> > and 3 will not get invoked, thus the IRQ vectors will not spread
> > on different CPUs and will still be allocated on CPU0. As a result
> > the CPUs will still get the offline failure.
> 
> So the proper solution for this network driver is to switch to managed
> interrupts instead of trying to work around it in some other place. It's
> using the wrong mechanism - end of story.
> 
> Why are you insisting on implementing a band aid for this particular driver
> instead of fixing the underlying problem of that driver which requires to
> have 32 queues and interrupts open even if only a single CPU is online?
> 
I agree, the driver could be rewritten, but it might take some time, so
meanwhile I'm looking at also other possible optimization.
> > Previously there were some discussion in the thread [1] about the
> > vector spread, and here are the suggestion from Thomas:
> > 
> > Q1:
> >     Rewrite that allocation code from scratch, use per cpu bitmaps,
> >     so we can do fast search over masks and keep track of
> >     the vectors which are used/free per cpu.
> > A1:
> >     per cpu bitmap was onced considered but this might not be
> >     as fast as the solution proposed here. That is, if we introduce the
> >     per cpu bitmap, we need to count the number of vectors assigned on
> >     each CPUs by cpumask_weight() and sort them in order to get the
> >     CPUs who have the least number of vectors assigned. By contrast,
> >     if we introduce the bitmaps for each vector number, we can get the
> >     'idle' CPU at the cost of nearly O(1).
> 
> The weight accounting with the cpumasks is an orthogonal issue to the per
> cpu vector bitmaps.
> 
> Once you selected a CPU the current code still loops in circles instead of
> just searching a bitmap.
Yes, I agree.
> 
> And if the required affinity mask needs more than one CPU then this is
> still not giving you the right answer. You assign perhaps a vector to the
> least busy CPU, but the other CPUs which happen to be in the mask are going
> to be randomly selected.
Yes, for multi cpumask, it might depends on how vector domain behaves.
> 
> I'm not against making the vector allocation better, but certainly not by
> adding yet more duct tape to something which is well known as one of the
> dumbest search algorithms on the planet with a worst case of
> 
>        nvectors * nr_online_cpus * nr_online_cpus_in_affinity_mask
> 
> and your mechanism nests another loop of potentially NR_VECTORS into
> that. Which is pointless as the actual assignable vector space is
> smaller. While x86 has 256 vectors the assignable vector space is way
> smaller and non continous:
> 
> Vectors   0- 31 are reserved for CPU traps and exceptions
> Vectors 239-255 are reserved by the kernel for IPIs, local timer etc.
> Vector	     32 can be reserved by the kernel for the reboot interrupt
> Vector	     96 can be reserved by the kernel for INT80
> 
> So the actually usable array size is between 205 and 207.
> 
> Aside of that the code is incorrect vs. the percpu accounting. Remove the
> limit in the update function and see what happens. While the limit is
> necessary in general, it should at least warn and yell whenever the
> accounting goes out of bounds. And if that happens, then the whole thing is
> completely useless simply because the numbers are just wrong.
> 
Ok.
> >     One scenario of this patch can be illustrated below:
> > 
> >     a) After initialization, vector_mask[0] is set with {CPU0...CPU31},
> >        other vector_mask[i] remain zero, which means that, CPU0...CPU31
> >        have zero vectors assigned.
> >     b) During driver probe, CPU0 is chosen to be assigned with one vector.
> >        Thus vector_mask[0] becomes {CPU1...CPU31}, and vector_mask[1] becomes
> >        {CPU0}.
> >     c) The next time the system tries to assign another vector, it searches from
> >        vector[0], because it is non-empty, CPU1 is chosen to place
> >        the vector. Thus vector_mask[0] becomes {CPU2...CPU31}, and vector_mask[1]
> >        becomes {CPU0, CPU1}.
> >     d) and so on, the vectors assignment will spread on different CPUs.
> 
> So this works for your particular network driver scenario, which is the
> wrong example as this driver just needs to be reworked to not expose that
> issue.
> 
> The reality of affinity requirements is not that simple. It's very much
> device dependent and making it mandatory can have negative side effects.
> 
Yes, this patch just 'cure' a special case.
> > Q2:
> >     "respect the allocation domains"
> > A2:
> >     This solution provides a CPU as the param for the vector allocation domains,
> 
> Solution? This is a crude hack which "solves" one particular problem in the
> wrong way.
> 
> >     in the hope that the domains can leverage this hint to generate the cpumask
> 
> Hope is never a good enginering principle.
> 
> >     with less vectors assigned. But yes, the CPU we select upfront gets fed into
> >     apic->vector_allocation_domain() might just say no because the CPU
> >     does not belong to it, but anyway this should somehow spread the vectors,
> >     although I can not find out a graceful solution to this.
> >     BTW, the vector allocation domain is default_vector_allocation_domain()
> >     in our case, thus it works.
> 
> Now try that with the logical delivery modes which allow multi CPU
> assignements and the cluster mode thereof.
Yes, I feel a little hard to consider the vector domain as it looks
indepent of the current vector allocation process.
> 
> > Q3:
> >     "set a preference for the node on which the interrupt was allocated"
> > A3:
> >     This can be achieved by further optimization in the following way:
> >     a) Also record the number of vectors used per node.
> >     b) Each time invoking __assign_irq_vector, adjust the input mask
> >        by cpumask_and(mask, mask, node_mask_with_least_vectors())
> 
> I'm not looking forward to that heuristics and the next loop inside the
> loops of loops.
> 
> So no, this is not going to happen. The current implementation sucks and
> needs a rewrite.
> 
> I already started to look into that, but started at the low level end of
> it. See the IDT cleanup series in tip:x86/apic. There is much more vooddo
> in the vector management code to clean up before we can actually talk about
> making the assignment algorithm smarter.
Ok, I'm pulling from tip:x86/apic.
> 
> I'll send out a separate mail raising a few points to discuss before we are
> actually starting to look into magic spreading functionality.
> 
For the vector calculation question you asked I'll reply to that thread.
Thanks,
	Yu
> Thanks,
> 
> 	tglx

[toc] | [prev] | [next] | [standalone]

#1727174 — Re: [PATCH 4/4][RFC v2] x86/apic: Spread the vectors by choosing the idlest CPU

On Wed, Sep 06, 2017 at 12:13:38PM +0800, Yu Chen wrote:
> I agree, the driver could be rewritten, but it might take some time, so
> meanwhile I'm looking at also other possible optimization.

Which driver are we talking about anyway?  Let's start looking at it
and fix the issue there.

[toc] | [prev] | [next] | [standalone]

#1727664 — Re: [PATCH 4/4][RFC v2] x86/apic: Spread the vectors by choosing the idlest CPU

On Tue, Sep 5, 2017 at 11:15 PM, Christoph Hellwig <hch@lst.de> wrote:
> On Wed, Sep 06, 2017 at 12:13:38PM +0800, Yu Chen wrote:
>> I agree, the driver could be rewritten, but it might take some time, so
>> meanwhile I'm looking at also other possible optimization.
>
> Which driver are we talking about anyway?  Let's start looking at it
> and fix the issue there.

As far as I understand, it's already fixed there:

commit 7c9ae7f053e9e896c24fd23595ba369a5fe322e1
Author: Carolyn Wyborny <carolyn.wyborny@intel.com>
Date:   Tue Jun 20 15:16:53 2017 -0700

    i40e: Fix for trace found with S4 state

    This patch fixes a problem found in systems when entering
    S4 state.  This patch fixes the problem by ensuring that
    the misc vector's IRQ is disabled as well.  Without this
    patch a stack trace can be seen upon entering S4 state.

However this seems like something that should be handled generically
in the irq-core especially since commit c5cb83bb337c
"genirq/cpuhotplug: Handle managed IRQs on CPU hotplug" was headed in
that direction. It's otherwise non-obvious when a driver needs to
release and re-acquire interrupts or be reworked to use managed
interrupts.

[toc] | [prev] | [next] | [standalone]

#1727882 — Re: [PATCH 4/4][RFC v2] x86/apic: Spread the vectors by choosing the idlest CPU

On Wed, Sep 06, 2017 at 10:46:17AM -0700, Dan Williams wrote:
> On Tue, Sep 5, 2017 at 11:15 PM, Christoph Hellwig <hch@lst.de> wrote:
> > On Wed, Sep 06, 2017 at 12:13:38PM +0800, Yu Chen wrote:
> >> I agree, the driver could be rewritten, but it might take some time, so
> >> meanwhile I'm looking at also other possible optimization.
> >
> > Which driver are we talking about anyway?  Let's start looking at it
> > and fix the issue there.
> 
> As far as I understand, it's already fixed there:
> 
> commit 7c9ae7f053e9e896c24fd23595ba369a5fe322e1
> Author: Carolyn Wyborny <carolyn.wyborny@intel.com>
> Date:   Tue Jun 20 15:16:53 2017 -0700
> 
>     i40e: Fix for trace found with S4 state
> 
>     This patch fixes a problem found in systems when entering
>     S4 state.  This patch fixes the problem by ensuring that
>     the misc vector's IRQ is disabled as well.  Without this
>     patch a stack trace can be seen upon entering S4 state.
> 
> However this seems like something that should be handled generically
> in the irq-core especially since commit c5cb83bb337c
> "genirq/cpuhotplug: Handle managed IRQs on CPU hotplug" was headed in
> that direction. It's otherwise non-obvious when a driver needs to
> release and re-acquire interrupts or be reworked to use managed
> interrupts.
Yes, thanks for the explaination! I did not notice this patch has
been merged already.
I'm using the normal CPU hotplug to reproduce the issue:
#!/bin/bash

n=1

while [ $n -le 31 ]
do
	echo 0 > /sys/devices/system/cpu/cpu${n}/online
	n=$(( n+1 ))
done

Thanks,
	Yu

[toc] | [prev] | [next] | [standalone]

#1727923 — Re: [PATCH 4/4][RFC v2] x86/apic: Spread the vectors by choosing the idlest CPU

On Wed, 6 Sep 2017, Dan Williams wrote:

> On Tue, Sep 5, 2017 at 11:15 PM, Christoph Hellwig <hch@lst.de> wrote:
> > On Wed, Sep 06, 2017 at 12:13:38PM +0800, Yu Chen wrote:
> >> I agree, the driver could be rewritten, but it might take some time, so
> >> meanwhile I'm looking at also other possible optimization.
> >
> > Which driver are we talking about anyway?  Let's start looking at it
> > and fix the issue there.
> 
> As far as I understand, it's already fixed there:
> 
> commit 7c9ae7f053e9e896c24fd23595ba369a5fe322e1

-ENOSUCHCOMMIT

> Author: Carolyn Wyborny <carolyn.wyborny@intel.com>
> Date:   Tue Jun 20 15:16:53 2017 -0700
> 
>     i40e: Fix for trace found with S4 state
> 
>     This patch fixes a problem found in systems when entering
>     S4 state.  This patch fixes the problem by ensuring that
>     the misc vector's IRQ is disabled as well.  Without this
>     patch a stack trace can be seen upon entering S4 state.
> 
> However this seems like something that should be handled generically
> in the irq-core especially since commit c5cb83bb337c
> "genirq/cpuhotplug: Handle managed IRQs on CPU hotplug" was headed in
> that direction. It's otherwise non-obvious when a driver needs to
> release and re-acquire interrupts or be reworked to use managed
> interrupts.

There are two problems here:

1) The driver allocates 300 interrupts and uses exactly 8 randomly chosen
   ones.

2) It's not using the managed affinity mechanics, so the interrupts cannot
   be sanely handled by the kernel, neither affinity wise nor at hotplug
   time.

Thanks,

	tglx

[toc] | [prev] | [next] | [standalone]

#1727944 — Re: [PATCH 4/4][RFC v2] x86/apic: Spread the vectors by choosing the idlest CPU

On Wed, Sep 6, 2017 at 10:59 PM, Thomas Gleixner <tglx@linutronix.de> wrote:
> On Wed, 6 Sep 2017, Dan Williams wrote:
>
>> On Tue, Sep 5, 2017 at 11:15 PM, Christoph Hellwig <hch@lst.de> wrote:
>> > On Wed, Sep 06, 2017 at 12:13:38PM +0800, Yu Chen wrote:
>> >> I agree, the driver could be rewritten, but it might take some time, so
>> >> meanwhile I'm looking at also other possible optimization.
>> >
>> > Which driver are we talking about anyway?  Let's start looking at it
>> > and fix the issue there.
>>
>> As far as I understand, it's already fixed there:
>>
>> commit 7c9ae7f053e9e896c24fd23595ba369a5fe322e1
>
> -ENOSUCHCOMMIT

Sorry, that's still pending in -next.

>> Author: Carolyn Wyborny <carolyn.wyborny@intel.com>
>> Date:   Tue Jun 20 15:16:53 2017 -0700
>>
>>     i40e: Fix for trace found with S4 state
>>
>>     This patch fixes a problem found in systems when entering
>>     S4 state.  This patch fixes the problem by ensuring that
>>     the misc vector's IRQ is disabled as well.  Without this
>>     patch a stack trace can be seen upon entering S4 state.
>>
>> However this seems like something that should be handled generically
>> in the irq-core especially since commit c5cb83bb337c
>> "genirq/cpuhotplug: Handle managed IRQs on CPU hotplug" was headed in
>> that direction. It's otherwise non-obvious when a driver needs to
>> release and re-acquire interrupts or be reworked to use managed
>> interrupts.
>
> There are two problems here:
>
> 1) The driver allocates 300 interrupts and uses exactly 8 randomly chosen
>    ones.
>
> 2) It's not using the managed affinity mechanics, so the interrupts cannot
>    be sanely handled by the kernel, neither affinity wise nor at hotplug
>    time.

Ok, this driver is an obvious candidate, but is there a general
guideline of when a driver must use affinity management? Should we be
emitting a message when a driver exceeds a certain threshold of
unmanaged interrupts to flag this in the future?

[toc] | [prev] | [next] | [standalone]

#1727954 — Re: [PATCH 4/4][RFC v2] x86/apic: Spread the vectors by choosing the idlest CPU

On Wed, 6 Sep 2017, Dan Williams wrote:
> On Wed, Sep 6, 2017 at 10:59 PM, Thomas Gleixner <tglx@linutronix.de> wrote:
> >> commit 7c9ae7f053e9e896c24fd23595ba369a5fe322e1
> >
> > -ENOSUCHCOMMIT
> 
> Sorry, that's still pending in -next.

Ok.

> >> Author: Carolyn Wyborny <carolyn.wyborny@intel.com>
> >> Date:   Tue Jun 20 15:16:53 2017 -0700
> >>
> >>     i40e: Fix for trace found with S4 state
> >>
> >>     This patch fixes a problem found in systems when entering
> >>     S4 state.  This patch fixes the problem by ensuring that
> >>     the misc vector's IRQ is disabled as well.  Without this
> >>     patch a stack trace can be seen upon entering S4 state.

Btw. This changelog is pretty useless.....

> >> However this seems like something that should be handled generically
> >> in the irq-core especially since commit c5cb83bb337c
> >> "genirq/cpuhotplug: Handle managed IRQs on CPU hotplug" was headed in
> >> that direction. It's otherwise non-obvious when a driver needs to
> >> release and re-acquire interrupts or be reworked to use managed
> >> interrupts.
> >
> > There are two problems here:
> >
> > 1) The driver allocates 300 interrupts and uses exactly 8 randomly chosen
> >    ones.
> >
> > 2) It's not using the managed affinity mechanics, so the interrupts cannot
> >    be sanely handled by the kernel, neither affinity wise nor at hotplug
> >    time.
> 
> Ok, this driver is an obvious candidate, but is there a general
> guideline of when a driver must use affinity management? Should we be
> emitting a message when a driver exceeds a certain threshold of
> unmanaged interrupts to flag this in the future?

I guess everything which uses multi queues and therefor allocates 8+
vectors is something which falls into that category.

Aside of that, drivers should be sane in terms of allocations. Allocating
metric tons of interrupts for nothing is not really a sign of a proper
thought out resource management.

Thanks,

	tglx

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web