[net-next-2.6 PATCH 1/3] irq: Export irq_set_affinity() for drivers

[net-next-2.6 PATCH 1/3] irq: Export irq_set_affinity() for drivers

[Jeff Kirsher]

From: Peter P Waskiewicz Jr <peter.p.waskiewicz.jr@intel.com>

This patch allows drivers to specify an IRQ affinity mask for
their respective interrupt sources.  This is very useful on
network adapters using MSI-X, where aligning network flows
linearly to CPUs greatly improves efficiency of the network
stack.

Today, users must either hand-set affinity through /proc, or
use a script through the same interface.  This patch will allow
a driver to come completely pre-canned with an optimal
configuration.

Signed-off-by: Peter P Waskiewicz Jr <peter.p.waskiewicz.jr@intel.com>
Signed-off-by: Jeff Kirsher <jeffrey.t.kirsher@intel.com>
---

 kernel/irq/manage.c |    1 +
 1 files changed, 1 insertions(+), 0 deletions(-)

diff --git a/kernel/irq/manage.c b/kernel/irq/manage.c
index bde4c66..185eb26 100644
--- a/kernel/irq/manage.c
+++ b/kernel/irq/manage.c
@@ -137,6 +137,7 @@ int irq_set_affinity(unsigned int irq, const struct cpumask *cpumask)
 	spin_unlock_irqrestore(&desc->lock, flags);
 	return 0;
 }
+EXPORT_SYMBOL(irq_set_affinity);
 
 #ifndef CONFIG_AUTO_IRQ_AFFINITY
 /*

[net-next-2.6 PATCH 2/3] ixgbe: Set MSI-X vectors to NOBALANCING and set affinity

[Jeff Kirsher]

From: Peter P Waskiewicz Jr <peter.p.waskiewicz.jr@intel.com>

This patch will set each MSI-X vector to IRQF_NOBALANCING to
prevent autobalance of the interrupts, then applies a CPU
affinity.  This will only be done when Flow Director is enabled,
which needs interrupts to be processed on the same CPUs where the
applications are running.

Signed-off-by: Peter P Waskiewicz Jr <peter.p.waskiewicz.jr@intel.com>
Signed-off-by: Jeff Kirsher <jeffrey.t.kirsher@intel.com>
---

 drivers/net/ixgbe/ixgbe_main.c |   34 +++++++++++++++++++++++++++++-----
 1 files changed, 29 insertions(+), 5 deletions(-)

diff --git a/drivers/net/ixgbe/ixgbe_main.c b/drivers/net/ixgbe/ixgbe_main.c
index 4c8a449..d2280c3 100644
--- a/drivers/net/ixgbe/ixgbe_main.c
+++ b/drivers/net/ixgbe/ixgbe_main.c
@@ -1565,8 +1565,10 @@ static int ixgbe_request_msix_irqs(struct ixgbe_adapter *adapter)
 {
 	struct net_device *netdev = adapter->netdev;
 	irqreturn_t (*handler)(int, void *);
-	int i, vector, q_vectors, err;
+	int i, vector, q_vectors, cpu, err;
 	int ri=0, ti=0;
+	u32 intr_flags = 0;
+	u32 num_cpus = num_online_cpus();
 
 	/* Decrement for Other and TCP Timer vectors */
 	q_vectors = adapter->num_msix_vectors - NON_Q_VECTORS;
@@ -1576,17 +1578,22 @@ static int ixgbe_request_msix_irqs(struct ixgbe_adapter *adapter)
 	if (err)
 		goto out;
 
+	/* If Flow Director is enabled, we want to affinitize vectors */
+	if ((adapter->flags & IXGBE_FLAG_FDIR_HASH_CAPABLE) ||
+	    (adapter->flags & IXGBE_FLAG_FDIR_PERFECT_CAPABLE))
+		intr_flags = IRQF_NOBALANCING;
+
 #define SET_HANDLER(_v) ((!(_v)->rxr_count) ? &ixgbe_msix_clean_tx : \
                          (!(_v)->txr_count) ? &ixgbe_msix_clean_rx : \
                          &ixgbe_msix_clean_many)
-	for (vector = 0; vector < q_vectors; vector++) {
+	for (vector = 0, cpu = 0; vector < q_vectors; vector++) {
 		handler = SET_HANDLER(adapter->q_vector[vector]);
 
-		if(handler == &ixgbe_msix_clean_rx) {
+		if (handler == &ixgbe_msix_clean_rx) {
 			sprintf(adapter->name[vector], "%s-%s-%d",
 				netdev->name, "rx", ri++);
 		}
-		else if(handler == &ixgbe_msix_clean_tx) {
+		else if (handler == &ixgbe_msix_clean_tx) {
 			sprintf(adapter->name[vector], "%s-%s-%d",
 				netdev->name, "tx", ti++);
 		}
@@ -1595,7 +1602,8 @@ static int ixgbe_request_msix_irqs(struct ixgbe_adapter *adapter)
 				netdev->name, "TxRx", vector);
 
 		err = request_irq(adapter->msix_entries[vector].vector,
-		                  handler, 0, adapter->name[vector],
+		                  handler, intr_flags,
+		                  adapter->name[vector],
 		                  adapter->q_vector[vector]);
 		if (err) {
 			DPRINTK(PROBE, ERR,
@@ -1603,9 +1611,25 @@ static int ixgbe_request_msix_irqs(struct ixgbe_adapter *adapter)
 			        "Error: %d\n", err);
 			goto free_queue_irqs;
 		}
+		if (intr_flags) {
+			/*
+			 * We're not balancing the vector, so affinitize it.
+			 * Best default layout is try and assign one vector
+			 * per CPU.  If we have more vectors than online
+			 * CPUs, then try to first affinitize Rx, then lay
+			 * Tx over the same Rx CPU map.  This can always be
+			 * overridden using smp_affinity in /proc
+			 */
+
+			irq_set_affinity(adapter->msix_entries[vector].vector,
+			                 cpumask_of(cpu));
+			if (++cpu >= num_cpus)
+				cpu = 0;
+		}
 	}
 
 	sprintf(adapter->name[vector], "%s:lsc", netdev->name);
+	/* We don't care if this vector is irqbalanced or not */
 	err = request_irq(adapter->msix_entries[vector].vector,
 	                  &ixgbe_msix_lsc, 0, adapter->name[vector], netdev);
 	if (err) {

Re: [net-next-2.6 PATCH 2/3] ixgbe: Set MSI-X vectors to NOBALANCING and set affinity

[David Miller]

From: Jeff Kirsher <jeffrey.t.kirsher@intel.com>
Date: Tue, 20 Oct 2009 19:27:14 -0700

> From: Peter P Waskiewicz Jr <peter.p.waskiewicz.jr@intel.com>
> 
> This patch will set each MSI-X vector to IRQF_NOBALANCING to
> prevent autobalance of the interrupts, then applies a CPU
> affinity.  This will only be done when Flow Director is enabled,
> which needs interrupts to be processed on the same CPUs where the
> applications are running.
> 
> Signed-off-by: Peter P Waskiewicz Jr <peter.p.waskiewicz.jr@intel.com>
> Signed-off-by: Jeff Kirsher <jeffrey.t.kirsher@intel.com>

Just explain to me why irqbalanced in userspace cannot take care
of this issue.

Second, even if we cannot use irqbalanced for some reason, the last
thing I want to see is drivers directly fiddling with interrupt
states and attributes.  Every driver is going to do it every so
slightly differently, and often will get it wrong.

There is also no global policy or policy control available when
drivers do this stuff directly.  And that's how we end up with
situations where every driver behaves differently which results in a
terrible user experience.

Re: [net-next-2.6 PATCH 2/3] ixgbe: Set MSI-X vectors to NOBALANCING and set affinity

[Peter P Waskiewicz Jr]

On Wed, 2009-10-21 at 21:50 -0700, David Miller wrote:
> From: Jeff Kirsher <jeffrey.t.kirsher@intel.com>
> Date: Tue, 20 Oct 2009 19:27:14 -0700
> 
> > From: Peter P Waskiewicz Jr <peter.p.waskiewicz.jr@intel.com>
> > 
> > This patch will set each MSI-X vector to IRQF_NOBALANCING to
> > prevent autobalance of the interrupts, then applies a CPU
> > affinity.  This will only be done when Flow Director is enabled,
> > which needs interrupts to be processed on the same CPUs where the
> > applications are running.
> > 
> > Signed-off-by: Peter P Waskiewicz Jr <peter.p.waskiewicz.jr@intel.com>
> > Signed-off-by: Jeff Kirsher <jeffrey.t.kirsher@intel.com>
> 
> Just explain to me why irqbalanced in userspace cannot take care
> of this issue.

The problem we have is when Flow Director is enabled, we want to try and
balance the applications across all CPUs.  irqbalance is going to fight
with the scheduler to balance things, and our tests show that irqbalance
only utilizes a few of the CPU cores, not all of them.  That fights
directly with Flow Director and what it's trying to do.

> Second, even if we cannot use irqbalanced for some reason, the last
> thing I want to see is drivers directly fiddling with interrupt
> states and attributes.  Every driver is going to do it every so
> slightly differently, and often will get it wrong.

The first thing any performance guide says is to disable irqbalance, and
affinitize the interrupts in /proc/irq/<irq>/smp_affinity.  This will
ensure the best distribution of work.  The major disadvantage in doing
this is disabling irqbalance affects the entire system.  What this
patchset is trying to do is make sure a single driver, trying to
optimize for performance, doesn't need to affect the entire system.
Setting no-balancing on a vector is the best approach for the entire
system.

I completely understand your concern that this opens precedent for other
drivers to potentially start doing crazy things with interrupts, but
with MSI-X, we're only impacting our driver.

> There is also no global policy or policy control available when
> drivers do this stuff directly.  And that's how we end up with
> situations where every driver behaves differently which results in a
> terrible user experience.

Again, I think the overall impact is worse where the normal approach to
performance tuning is to altogether disable irqbalancing.  The same
effect can be attained by a user disabling irqbalance, and assigning
whatever affinity they want, which could be even more devastating.  What
we're trying to do here is have the driver come as best tuned out of the
box as possible.

If there's something about this particular implementation you're not
comfortable with, I'm very willing to take any feedback on it.  We're
trying to do a specific thing, not lead poor design in drivers when
dealing with interrupts.

Regards,
-PJ Waskiewicz

Re: [net-next-2.6 PATCH 2/3] ixgbe: Set MSI-X vectors to NOBALANCING and set affinity

[David Miller]

From: Peter P Waskiewicz Jr <peter.p.waskiewicz.jr@intel.com>
Date: Thu, 22 Oct 2009 01:22:36 -0700

> The first thing any performance guide says is to disable irqbalance

Such guides are wrong, and that's the end of this discussion.

These kinds of guides also say to do all kinds of crazy things with
the socket sysctl settings.  That's wrong too and we absolutely do not
do things to accomodate nor support those guide suggestions.

And we won't do that here.

I'm especially not going to succumb in this case because Arjan has
been more than responsive to making sure irqbalanced in userspace does
the right thing for networking devices, even multiqueue ones.

So we can make it do the right thing when flow director is present.
In fact, the thing you want for flow director makes sense in the
general case too.

Re: [net-next-2.6 PATCH 2/3] ixgbe: Set MSI-X vectors to NOBALANCING and set affinity

[Stephen Hemminger]

On Thu, 22 Oct 2009 03:56:01 -0700 (PDT)
David Miller <davem@davemloft.net> wrote:

> From: Peter P Waskiewicz Jr <peter.p.waskiewicz.jr@intel.com>
> Date: Thu, 22 Oct 2009 01:22:36 -0700
> 
> > The first thing any performance guide says is to disable irqbalance
> 
> Such guides are wrong, and that's the end of this discussion.
> 
> These kinds of guides also say to do all kinds of crazy things with
> the socket sysctl settings.  That's wrong too and we absolutely do not
> do things to accomodate nor support those guide suggestions.
> 
> And we won't do that here.
> 
> I'm especially not going to succumb in this case because Arjan has
> been more than responsive to making sure irqbalanced in userspace does
> the right thing for networking devices, even multiqueue ones.
> 
> So we can make it do the right thing when flow director is present.
> In fact, the thing you want for flow director makes sense in the
> general case too.

irqbalance daemon already has IRQBALANCE_BANNED_INTERRUPTS
to work around this. It also has code to special case devices, if you
think ixgbe needs special treatment, why not do it there.

RE: [net-next-2.6 PATCH 2/3] ixgbe: Set MSI-X vectors to NOBALANCING and set affinity

[Waskiewicz Jr, Peter P]

>From: Jeff Kirsher <jeffrey.t.kirsher@intel.com>
>Date: Tue, 20 Oct 2009 19:27:14 -0700
>
>> From: Peter P Waskiewicz Jr <peter.p.waskiewicz.jr@intel.com>
>>
>> This patch will set each MSI-X vector to IRQF_NOBALANCING to
>> prevent autobalance of the interrupts, then applies a CPU
>> affinity.  This will only be done when Flow Director is enabled,
>> which needs interrupts to be processed on the same CPUs where the
>> applications are running.
>>
>> Signed-off-by: Peter P Waskiewicz Jr <peter.p.waskiewicz.jr@intel.com>
>> Signed-off-by: Jeff Kirsher <jeffrey.t.kirsher@intel.com>
>
>Just explain to me why irqbalanced in userspace cannot take care
>of this issue.
>
>Second, even if we cannot use irqbalanced for some reason, the last
>thing I want to see is drivers directly fiddling with interrupt
>states and attributes.  Every driver is going to do it every so
>slightly differently, and often will get it wrong.

Jesse Brandeburg and I talked with Arjan yesterday regarding these patches.  We all agreed the drivers need some mechanism to help irqbalance make smarter decisions when enforcing its balancing policies.  Arjan did convince me though that enforcing the interrupt's policy from the driver isn't the right thing to do (which echoes your stance too :-) ).

The current proposal is to add a new interface to the interrupt management that drivers can specify an allowable CPU mask.  This way a driver can be specific with where interrupt vectors can be balanced, e.g. specify a CPU mask within a NUMA node that a vector can be balanced.  Then irqbalance can make the right decisions based on cache locality of CPU cores; as long as we land on the correct NUMA node with the interrupt, we should be fine.  This new interface can also be used in the PCI space to assist setting the correct NUMA mask for a driver on load, further streamlining the NUMA affinity of a device.

This change will also require an update to irqbalance to comprehend the new mask.  This should be fairly trivial.

We also discussed the need for irqbalance to distinguish between Rx and Tx queue vectors.  Right now, irqbalance can identify an interrupt belong to an Ethernet device, but it stops there.  It needs to also distinguish the directional vectors, and make sure to balance the right queue vector with its paired queue (i.e. make sure Tx queue 0's vector lines up with Rx queue 0's vector).

I'll be getting patches together for this interface, and will push them separately.  In the meantime, I'll repush the one patch from this patch series to pair the Rx and Tx queues onto a shared vector.  That is an independent patch from this whole series, so it should be fine to apply regardless of the interrupt affinity changes.

Cheers,
-PJ Waskiewicz

Re: [net-next-2.6 PATCH 2/3] ixgbe: Set MSI-X vectors to NOBALANCING and set affinity

[David Miller]

From: "Waskiewicz Jr, Peter P" <peter.p.waskiewicz.jr@intel.com>
Date: Thu, 12 Nov 2009 11:12:22 -0800

> Jesse Brandeburg and I talked with Arjan yesterday regarding these patches.

Thanks for the update.

> We also discussed the need for irqbalance to distinguish between Rx
> and Tx queue vectors.  Right now, irqbalance can identify an
> interrupt belong to an Ethernet device, but it stops there.  It
> needs to also distinguish the directional vectors, and make sure to
> balance the right queue vector with its paired queue (i.e. make sure
> Tx queue 0's vector lines up with Rx queue 0's vector).

Why don't you just simply use the same MSI-X vector for both TX
queue 0 and RX queue 0, can't your hardware do that?

That's what I plan on doing in the NIU driver soon.

Re: [net-next-2.6 PATCH 2/3] ixgbe: Set MSI-X vectors to NOBALANCING and set affinity

[Ben Hutchings]

On Wed, 2009-11-18 at 10:10 -0800, David Miller wrote:
> From: "Waskiewicz Jr, Peter P" <peter.p.waskiewicz.jr@intel.com>
> Date: Thu, 12 Nov 2009 11:12:22 -0800
> 
> > Jesse Brandeburg and I talked with Arjan yesterday regarding these patches.
> 
> Thanks for the update.
> 
> > We also discussed the need for irqbalance to distinguish between Rx
> > and Tx queue vectors.  Right now, irqbalance can identify an
> > interrupt belong to an Ethernet device, but it stops there.  It
> > needs to also distinguish the directional vectors, and make sure to
> > balance the right queue vector with its paired queue (i.e. make sure
> > Tx queue 0's vector lines up with Rx queue 0's vector).
> 
> Why don't you just simply use the same MSI-X vector for both TX
> queue 0 and RX queue 0, can't your hardware do that?
> 
> That's what I plan on doing in the NIU driver soon.

When forwarding between 2 ports it can be beneficial to match the
affinity of each port's TX interrupts with the other port's RX
interrupts.  Obviously this is not the case when the system is acting as
an endpoint, and the situation is presumably more complex when
forwarding between >2 ports.

Ben.

-- 
Ben Hutchings, Senior Software Engineer, Solarflare Communications
Not speaking for my employer; that's the marketing department's job.
They asked us to note that Solarflare product names are trademarked.

Re: [net-next-2.6 PATCH 2/3] ixgbe: Set MSI-X vectors to NOBALANCING and set affinity

[David Miller]

From: Ben Hutchings <bhutchings@solarflare.com>
Date: Wed, 18 Nov 2009 19:46:10 +0000

> When forwarding between 2 ports it can be beneficial to match the
> affinity of each port's TX interrupts with the other port's RX
> interrupts.  Obviously this is not the case when the system is
> acting as an endpoint, and the situation is presumably more complex
> when forwarding between >2 ports.

Yes, but tricks like that won't be necessary with changes that Eric
Dumazet said he'd work on soon, wherein SKB frees always get scheduled
to occur on the cpu where allocation occured.

Re: [net-next-2.6 PATCH 2/3] ixgbe: Set MSI-X vectors to NOBALANCING and set affinity

[Peter P Waskiewicz Jr]

On Wed, 2009-11-18 at 11:50 -0800, David Miller wrote:
> From: Ben Hutchings <bhutchings@solarflare.com>
> Date: Wed, 18 Nov 2009 19:46:10 +0000
> 
> > When forwarding between 2 ports it can be beneficial to match the
> > affinity of each port's TX interrupts with the other port's RX
> > interrupts.  Obviously this is not the case when the system is
> > acting as an endpoint, and the situation is presumably more complex
> > when forwarding between >2 ports.
> 
> Yes, but tricks like that won't be necessary with changes that Eric
> Dumazet said he'd work on soon, wherein SKB frees always get scheduled
> to occur on the cpu where allocation occured.
> 

We actually just submitted a patch that pairs our Rx queue 0 and Tx
queue 0 (up until the number of queues) onto the same MSI-X interrupt.
Arjan had indicated though he'd still like to make irqbalance smart
enough to distinguish Rx and Tx from one another, regardless if the
drivers just move to queue pairs or not.  I don't care either way, since
my queues are now paired.

-PJ

Re: [net-next-2.6 PATCH 2/3] ixgbe: Set MSI-X vectors to NOBALANCING and set affinity

[David Miller]

From: Peter P Waskiewicz Jr <peter.p.waskiewicz.jr@intel.com>
Date: Wed, 18 Nov 2009 17:14:57 -0800

> We actually just submitted a patch that pairs our Rx queue 0 and Tx
> queue 0 (up until the number of queues) onto the same MSI-X
> interrupt.  Arjan had indicated though he'd still like to make
> irqbalance smart enough to distinguish Rx and Tx from one another,
> regardless if the drivers just move to queue pairs or not.  I don't
> care either way, since my queues are now paired.

Ok.

[net-next-2.6 PATCH 3/3] ixgbe: Make queue pairs on single MSI-X interrupts

[Jeff Kirsher]

From: Peter P Waskiewicz Jr <peter.p.waskiewicz.jr@intel.com>

This patch pairs similar-numbered Rx and Tx queues onto a single
MSI-X vector.  For example, Tx queue 0 and Rx queue 0's interrupt
with be ethX-RxTx-0.  This allows for more efficient cleanup, since
fewer interrupts will be firing during device operation.  It also
helps with a cleaner CPU affinity for IRQ affinity.

Signed-off-by: Peter P Waskiewicz Jr <peter.p.waskiewicz.jr@intel.com>
Signed-off-by: Jeff Kirsher <jeffrey.t.kirsher@intel.com>
---

 drivers/net/ixgbe/ixgbe_main.c |    4 ++--
 1 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/net/ixgbe/ixgbe_main.c b/drivers/net/ixgbe/ixgbe_main.c
index d2280c3..00b6829 100644
--- a/drivers/net/ixgbe/ixgbe_main.c
+++ b/drivers/net/ixgbe/ixgbe_main.c
@@ -3579,10 +3579,10 @@ static int ixgbe_set_interrupt_capability(struct ixgbe_adapter *adapter)
 	 * It's easy to be greedy for MSI-X vectors, but it really
 	 * doesn't do us much good if we have a lot more vectors
 	 * than CPU's.  So let's be conservative and only ask for
-	 * (roughly) twice the number of vectors as there are CPU's.
+	 * (roughly) the same number of vectors as there are CPU's.
 	 */
 	v_budget = min(adapter->num_rx_queues + adapter->num_tx_queues,
-	               (int)(num_online_cpus() * 2)) + NON_Q_VECTORS;
+	               (int)num_online_cpus()) + NON_Q_VECTORS;
 
 	/*
 	 * At the same time, hardware can only support a maximum of

Re: [net-next-2.6 PATCH 1/3] irq: Export irq_set_affinity() for drivers

[Ben Hutchings]

On Tue, 2009-10-20 at 19:26 -0700, Jeff Kirsher wrote:
> From: Peter P Waskiewicz Jr <peter.p.waskiewicz.jr@intel.com>
> 
> This patch allows drivers to specify an IRQ affinity mask for
> their respective interrupt sources.  This is very useful on
> network adapters using MSI-X, where aligning network flows
> linearly to CPUs greatly improves efficiency of the network
> stack.
[...]

Since this is not networking-specific, I think it needs to go to
linux-kernel as well.

Ben.

-- 
Ben Hutchings, Senior Software Engineer, Solarflare Communications
Not speaking for my employer; that's the marketing department's job.
They asked us to note that Solarflare product names are trademarked.