multiqueue changes

Say I have such non multiqueue device :

03:00.0 Ethernet controller: Broadcom Corporation NetXtreme II BCM5708S Gigabit Ethernet (rev 12)

Driver bnx2

This drivers does an alloc_etherdev_mq(sizeof(*bp), TX_MAX_RINGS), regardless of real capabilities of the NIC.

Then, a bit later it does :

bp->dev->real_num_tx_queues = bp->num_tx_rings;

(one in my non multiqueue case)

Now I have :

# tc -s -d qdisc show dev eth0
qdisc mq 0: root
 Sent 117693091 bytes 1542188 pkt (dropped 0, overlimits 0 requeues 0)
 backlog 0b 0p requeues 0
# tc -s -d class show dev eth0
class mq :1 root
 Sent 113935509 bytes 1492598 pkt (dropped 0, overlimits 0 requeues 0)
 backlog 0b 0p requeues 0
class mq :2 root
 Sent 0 bytes 0 pkt (dropped 0, overlimits 0 requeues 0)
 backlog 0b 0p requeues 0
class mq :3 root
 Sent 0 bytes 0 pkt (dropped 0, overlimits 0 requeues 0)
 backlog 0b 0p requeues 0
class mq :4 root
 Sent 0 bytes 0 pkt (dropped 0, overlimits 0 requeues 0)
 backlog 0b 0p requeues 0
class mq :5 root
 Sent 0 bytes 0 pkt (dropped 0, overlimits 0 requeues 0)
 backlog 0b 0p requeues 0
class mq :6 root
 Sent 0 bytes 0 pkt (dropped 0, overlimits 0 requeues 0)
 backlog 0b 0p requeues 0
class mq :7 root
 Sent 0 bytes 0 pkt (dropped 0, overlimits 0 requeues 0)
 backlog 0b 0p requeues 0
class mq :8 root
 Sent 0 bytes 0 pkt (dropped 0, overlimits 0 requeues 0)
 backlog 0b 0p requeues 0

While in previous kernels I had :

# tc -s -d qdisc show dev eth0
qdisc pfifo_fast 0: root bands 3 priomap  1 2 2 2 1 2 0 0 1 1 1 1 1 1 1 1
 Sent 26292963818 bytes 347139141 pkts (dropped 0, overlimits 0 requeues 0)
# tc -s -d class show dev eth0

So I lost the default pfifo_fast classification.

Just wondering if it could hurt some people.

Anyway, should we change bnx2/tg3 drivers so that single queue devices have same default qdisc/class than before ?

eg :

drivers/net/bnx2.c +0 -0

index 08cddb6..7cac205 100644

num_tx_queues. But it seems we should rather use more often real_num_tx_queue in schedulers code like dumps and maybe more (like e.g. sch_multiq does).

On Thu, Oct 08, 2009 at 09:03:44AM +0000, Jarek Poplawski wrote:
...

Quoted 3 lines

num_tx_queues. But it seems we should rather use more often real_num_tx_queue in schedulers code like dumps and maybe more (like e.g. sch_multiq does).

...i.e. probably everywhere between dev_activate and dev_deactivate all qdisc operations could use real_num_tx_queues (including a test
like: netif_is_real_multique), unless I miss something.

Quoted 6 lines · Jarek Poplawski

On Thu, Oct 08, 2009 at 09:03:44AM +0000, Jarek Poplawski wrote:
...

Quoted 3 lines

num_tx_queues. But it seems we should rather use more often real_num_tx_queue in schedulers code like dumps and maybe more (like e.g. sch_multiq does).

Quoted 3 lines

...i.e. probably everywhere between dev_activate and dev_deactivate all qdisc operations could use real_num_tx_queues (including a test
like: netif_is_real_multique), unless I miss something.

I am not sure David intent was being able to dynamically adjust real_num_tx_queue between 1 and num_tx_queue.

I am not sure David intent was being able to dynamically adjust real_num_tx_queue between 1 and num_tx_queue.

On Thu, Oct 08, 2009 at 09:03:44AM +0000, Jarek Poplawski wrote:
...

Quoted 3 lines

num_tx_queues. But it seems we should rather use more often real_num_tx_queue in schedulers code like dumps and maybe more (like e.g. sch_multiq does).

...i.e. probably everywhere between dev_activate and dev_deactivate all qdisc operations could use real_num_tx_queues (including a test
like: netif_is_real_multique), unless I miss something.

Quoted 9 lines · Jarek Poplawski

On Thu, Oct 08, 2009 at 09:03:44AM +0000, Jarek Poplawski wrote:
...

Quoted 3 lines

num_tx_queues. But it seems we should rather use more often real_num_tx_queue in schedulers code like dumps and maybe more (like e.g. sch_multiq does).

...i.e. probably everywhere between dev_activate and dev_deactivate all qdisc operations could use real_num_tx_queues (including a test
like: netif_is_real_multique), unless I miss something.

We don't seem to be supporting changing real_num_tx_queues for registered devices currently (at least I couldn't find it). So I guess it depends on how this would be implemented.

Simply changing the dev->real_num_tx_queues value while the device is down would require qdisc operations to operate on all possible queues since the amount of queues in use could be changed after the qdisc is created/configured, but before the device is set up. This approach has more complications like switching between mq and non-mq root qdiscs, taking care of non-default root qdisc (cloning them to the new queues), etc.

A simpler alternative would be to destroy the existing root qdisc on any change to real_num_tx_queues and have dev_activate() set it up from scratch. In this case, we could (as you suggested) use real_num_tx_queues, which should fix the problem Eric reported.

Quoted 12 lines · Patrick McHardy

We don't seem to be supporting changing real_num_tx_queues for registered devices currently (at least I couldn't find it). So I guess it depends on how this would be implemented.

Simply changing the dev->real_num_tx_queues value while the device is down would require qdisc operations to operate on all possible queues since the amount of queues in use could be changed after the qdisc is created/configured, but before the device is set up. This approach has more complications like switching between mq and non-mq root qdiscs, taking care of non-default root qdisc (cloning them to the new queues), etc.

Quoted 4 lines

A simpler alternative would be to destroy the existing root qdisc on any change to real_num_tx_queues and have dev_activate() set it up from scratch. In this case, we could (as you suggested) use real_num_tx_queues, which should fix the problem Eric reported.

Actually, I changed my mind after Eric's and especially David's explanations. Probably there will be needed some changes in handling the real_num_tx_queues, but there is no reason to misuse them for masking a totally useless num_tx_queues value, like in this case. So, IMHO, its mainly about the driver(s) (and maybe a bit of API change) here.

Well, we do need both values for supporting changes to the actually used numbers of TX queues. If I understood Dave's explanation correctly, this is also what's intended. It also doesn't seem unreasonable what bnx2 is doing.

But getting back to the problem Eric reported - so you're suggesting that bnx2.c should also adjust num_tx_queues in case the hardware doesn't support multiqueue? That seems reasonable as well.

On Thu, Oct 29, 2009 at 05:37:23PM +0100, Patrick McHardy wrote:
...

Quoted 4 lines

Well, we do need both values for supporting changes to the actually used numbers of TX queues. If I understood Dave's explanation correctly, this is also what's intended. It also doesn't seem unreasonable what bnx2 is doing.

Exactly. With a growing number of cores, both available and powered off, these values will be soon treated more carefully than now.

Quoted 3 lines

But getting back to the problem Eric reported - so you're suggesting that bnx2.c should also adjust num_tx_queues in case the hardware doesn't support multiqueue? That seems reasonable as well.

Currently, declaring num_tx_queues with alloc_netdev_mq() looks like too soon for some drivers. It seems they should be able to do it separately later during the .probe.

There is a question if .ndo_open should be considered too.

Quoted 14 lines · Jarek Poplawski

On Thu, Oct 29, 2009 at 05:37:23PM +0100, Patrick McHardy wrote:
...

Quoted 4 lines

Well, we do need both values for supporting changes to the actually used numbers of TX queues. If I understood Dave's explanation correctly, this is also what's intended. It also doesn't seem unreasonable what bnx2 is doing.

Exactly. With a growing number of cores, both available and powered off, these values will be soon treated more carefully than now.

Quoted 3 lines

But getting back to the problem Eric reported - so you're suggesting that bnx2.c should also adjust num_tx_queues in case the hardware doesn't support multiqueue? That seems reasonable as well.

Quoted 3 lines

Currently, declaring num_tx_queues with alloc_netdev_mq() looks like too soon for some drivers. It seems they should be able to do it separately later during the .probe.

The value passed into alloc_netdev_mq() is just used for allocation purposes, from what I can tell there's no downside in reducing it before the dev_activate() call.

Quoted 1 line

There is a question if .ndo_open should be considered too.

I currently can't see any purpose in decreasing num_tx_queues after registration instead of real_num_tx_queues.

Quoted 15 lines · Patrick McHardy

Quoted 14 lines · Jarek Poplawski

On Thu, Oct 29, 2009 at 05:37:23PM +0100, Patrick McHardy wrote:
...

Quoted 4 lines

Well, we do need both values for supporting changes to the actually used numbers of TX queues. If I understood Dave's explanation correctly, this is also what's intended. It also doesn't seem unreasonable what bnx2 is doing.

Exactly. With a growing number of cores, both available and powered off, these values will be soon treated more carefully than now.

Quoted 3 lines

But getting back to the problem Eric reported - so you're suggesting that bnx2.c should also adjust num_tx_queues in case the hardware doesn't support multiqueue? That seems reasonable as well.

Quoted 7 lines

Quoted 3 lines

Currently, declaring num_tx_queues with alloc_netdev_mq() looks like too soon for some drivers. It seems they should be able to do it separately later during the .probe.

The value passed into alloc_netdev_mq() is just used for allocation purposes, from what I can tell there's no downside in reducing it before the dev_activate() call.

Right, but IMHO this reducing (or reallocation) should be done with some API. Simple overwriting of num_tx_queues proposed by Eric, even if no downside now, seems to be asking for problems in the future.

Quoted 4 lines

Quoted 1 line

There is a question if .ndo_open should be considered too.

I currently can't see any purpose in decreasing num_tx_queues after registration instead of real_num_tx_queues.

I agree, but since Eric's example shows some drivers do it (almost) like this, I'd prefer authors/maintainers answer this question.

Quoted 1 line · Jarek Poplawski

On Thu, Oct 29, 2009 at 11:12:39PM +0100, Patrick McHardy wrote:

Quoted 6 lines

Quoted 2 lines

I currently can't see any purpose in decreasing num_tx_queues after registration instead of real_num_tx_queues.

I agree, but since Eric's example shows some drivers do it (almost) like this, I'd prefer authors/maintainers answer this question.

We need the netdev and the private structure at the beginning of ->probe() to store values as we probe the device. Later on in ->probe(), we'll know whether the device supports MSI-X and multiqueue. If we successfully enable MSI-X in ->ndo_open(), tx multiqueue will be used.

So if we can separate the allocation of the netdev and the private structure from the tx queues, we can allocate and set the exact number of num_tx_queues in ->ndo_open().

There is a question if we can predict in ->probe() MSI-X should be successfully enabled in ->ndo_open() for probed hardware. If so, then it could go e.g. like this:

From: Jarek Poplawski <[email protected]>
Date: Sun, 1 Nov 2009 14:20:17 +0100

Quoted 3 lines

There is a question if we can predict in ->probe() MSI-X should be successfully enabled in ->ndo_open() for probed hardware. If so, then it could go e.g. like this:

We never can know this.

Another device driver can eat up all the MSI-X vectors in the PCI domain before we make the request_irq() calls in ->open().

Right, but it's not a 50% chance, I guess? A user most of the time gets consistently multiqueue or non-multiqueue behavior after open, unless I miss something. Then such an exceptional state could be handled by real_num_tx_queues (just like in case of powered of cpus). The main difference is to hold in num_tx_queues something that is really available vs max possible value for all configs.

From: Jarek Poplawski <[email protected]>
Date: Mon, 2 Nov 2009 12:30:29 +0000

Quoted 6 lines

Right, but it's not a 50% chance, I guess? A user most of the time gets consistently multiqueue or non-multiqueue behavior after open, unless I miss something. Then such an exceptional state could be handled by real_num_tx_queues (just like in case of powered of cpus). The main difference is to hold in num_tx_queues something that is really available vs max possible value for all configs.

I see your point, yes this would seem to be a reasonable way to start handling num_tx_queues and real_num_tx_queues.

Quoted 9 lines · David Miller

From: Jarek Poplawski <[email protected]>
Date: Mon, 2 Nov 2009 12:30:29 +0000

Quoted 6 lines

Right, but it's not a 50% chance, I guess? A user most of the time gets consistently multiqueue or non-multiqueue behavior after open, unless I miss something. Then such an exceptional state could be handled by real_num_tx_queues (just like in case of powered of cpus). The main difference is to hold in num_tx_queues something that is really available vs max possible value for all configs.

Quoted 2 lines

I see your point, yes this would seem to be a reasonable way to start handling num_tx_queues and real_num_tx_queues.

Very nice! So, I hope Eric should be satisfied with these requested comments already :-)

Sure, but I prefer a patch from you ;)

From: Jarek Poplawski <[email protected]>
Date: Mon, 2 Nov 2009 12:30:29 +0000

Quoted 6 lines

Right, but it's not a 50% chance, I guess? A user most of the time gets consistently multiqueue or non-multiqueue behavior after open, unless I miss something. Then such an exceptional state could be handled by real_num_tx_queues (just like in case of powered of cpus). The main difference is to hold in num_tx_queues something that is really available vs max possible value for all configs.

I see your point, yes this would seem to be a reasonable way to start handling num_tx_queues and real_num_tx_queues.

From: Jarek Poplawski <[email protected]>
Date: Mon, 2 Nov 2009 12:30:29 +0000

Quoted 6 lines

Right, but it's not a 50% chance, I guess? A user most of the time gets consistently multiqueue or non-multiqueue behavior after open, unless I miss something. Then such an exceptional state could be handled by real_num_tx_queues (just like in case of powered of cpus). The main difference is to hold in num_tx_queues something that is really available vs max possible value for all configs.

I see your point, yes this would seem to be a reasonable way to start handling num_tx_queues and real_num_tx_queues.

Take 2 (with forgotten headers, sorry).

Here is a proposal of netdev api change. I hope Michael finds time to try if this can be really useful for drivers.

Thanks,
Jarek P.
---------------> (take 2)

This patch separates allocation of TX subqueues from alloc_netdev_mq() to realloc_netdev_mq() to allow for resizing like in this example:

some_nic_probe()

{
	...
	dev = alloc_etherdev_mq(sizeof(*bp), 1)
	...
	if (MSI-X_available && device_supports_MSI-X_and_multiqueue)
		realloc_netdev_mq(dev, TX_MAX_RINGS)
	register_netdev(dev)
	...
}

Quoted 1 line · Jarek Poplawski

Take 2 (with forgotten headers, sorry).

Very nice, thanks !

Quoted 11 lines

Here is a proposal of netdev api change. I hope Michael finds time to try if this can be really useful for drivers.

Thanks,
Jarek P.
---------------> (take 2)

This patch separates allocation of TX subqueues from alloc_netdev_mq() to realloc_netdev_mq() to allow for resizing like in this example:

some_nic_probe()

Quoted 10 lines

{
	...
	dev = alloc_etherdev_mq(sizeof(*bp), 1)
	...
	if (MSI-X_available && device_supports_MSI-X_and_multiqueue)
		realloc_netdev_mq(dev, TX_MAX_RINGS)
	register_netdev(dev)
	...
}

Or the reverse :

ie, the device first allocate its max possible tx rings

dev = alloc_etherdev_mq(sizeof(*bp), 128)

then, later, try to reduce it, when knowing exact tx count.

if (realloc_netdev_mq(dev, real_queues))

	dev->real_num_tx_queues = real_queues;

In this case the memory error is not fatal.

Quoted 4 lines · Eric Dumazet

if (realloc_netdev_mq(dev, real_queues))

	dev->real_num_tx_queues = real_queues;

In this case the memory error is not fatal.

Good point! We can consider doing this inside the function too?

Quoted 6 lines · Jarek Poplawski

Quoted 4 lines · Eric Dumazet

if (realloc_netdev_mq(dev, real_queues))

	dev->real_num_tx_queues = real_queues;

In this case the memory error is not fatal.

Good point! We can consider doing this inside the function too?

Hmm... Of course, not exactly this - I mean using min().

Quoted 5 lines · Jarek Poplawski

Quoted 4 lines · Jarek Poplawski

Quoted 3 lines · Eric Dumazet

if (realloc_netdev_mq(dev, real_queues))

	dev->real_num_tx_queues = real_queues;

Quoted 4 lines

Quoted 2 lines

Quoted 1 line

In this case the memory error is not fatal.

Good point! We can consider doing this inside the function too?

Hmm... Of course, not exactly this - I mean using min().

Sure, allowing to reduce the count in case new allocation failed.

And report an error if caller wanted to increase number of queues and allocation failed.

Quoted 5 lines · Jarek Poplawski

Quoted 4 lines · Jarek Poplawski

Quoted 3 lines · Eric Dumazet

if (realloc_netdev_mq(dev, real_queues))

	dev->real_num_tx_queues = real_queues;

Quoted 4 lines

Quoted 2 lines

Quoted 1 line

In this case the memory error is not fatal.

Good point! We can consider doing this inside the function too?

Hmm... Of course, not exactly this - I mean using min().

Sure, allowing to reduce the count in case new allocation failed.

And report an error if caller wanted to increase number of queues and allocation failed.

Quoted 6 lines · Eric Dumazet

Quoted 5 lines · Jarek Poplawski

Quoted 4 lines · Jarek Poplawski

Quoted 3 lines · Eric Dumazet

if (realloc_netdev_mq(dev, real_queues))

	dev->real_num_tx_queues = real_queues;

Quoted 6 lines

Quoted 3 lines

Quoted 2 lines

Quoted 1 line

In this case the memory error is not fatal.

Good point! We can consider doing this inside the function too?

Hmm... Of course, not exactly this - I mean using min().

Sure, allowing to reduce the count in case new allocation failed.

And report an error if caller wanted to increase number of queues and allocation failed.

Hmm... After re-thinking it looks a bit too complex to me. I think, there is no reason to not report this error since in most cases it shouldn't be fatal. That's why I skipped this check in the changelog example. Unless I miss something?

Thanks,
Jarek P.
---------------> (take 4)

This patch separates allocation of TX subqueues from alloc_netdev_mq() to realloc_netdev_mq() to allow for resizing like in this example:

some_nic_probe()

{
	...
	dev = alloc_etherdev_mq(sizeof(*bp), 1)
	...
	if (MSI-X_available && device_supports_MSI-X_and_multiqueue)
		realloc_netdev_mq(dev, TX_MAX_RINGS)
	register_netdev(dev)
	...
}

Alternatively, it can be done in reverse order: starting from the highest queue_count and reallocating with a lower one.

The main difference is to hold in num_tx_queues something that is really available, instead of max possible value for all configs, in case of drivers allocating net_device at the beginning of the probe.

The description of alloc_netdev_mq() is fixed btw.

Reported-by Eric Dumazet <[email protected]>

Signed-off-by Jarek Poplawski <[email protected]>

Cc Eric Dumazet <[email protected]>

Nice patch, thanks :)

Acked-by Eric Dumazet <[email protected]>

From: Eric Dumazet <[email protected]>
Date: Thu, 03 Dec 2009 22:29:42 +0100

Quoted 3 lines

Nice patch, thanks :)

Acked-by Eric Dumazet <[email protected]>

I like it too, but please resubmit once we have at least one example user submitted.

Thanks!

From: Eric Dumazet <[email protected]>
Date: Thu, 03 Dec 2009 22:29:42 +0100

Quoted 3 lines

Nice patch, thanks :)

Acked-by Eric Dumazet <[email protected]>

I like it too, but please resubmit once we have at least one example user submitted.

Quoted 5 lines · David Miller

From: Eric Dumazet <[email protected]>
Date: Thu, 03 Dec 2009 22:29:42 +0100

Quoted 2 lines

Nice patch, thanks :)

Quoted 5 lines

Quoted 1 line

Acked-by Eric Dumazet <[email protected]>

I like it too, but please resubmit once we have at least one example user submitted.

I successfully tested following patch.

Thanks !

[PATCH] ixgbe: Use realloc_netdev_mq() helper

Instead of 128 tx queues, we can cap the tx queue number to what driver really uses.

Signed-off-by Eric Dumazet <[email protected]>

---

drivers/net/ixgbe/ixgbe_main.c +1 -1

index 9ba506f..e524d0d 100644
@@ -3306,7 +3306,7 @@ static void ixgbe_set_num_queues(struct ixgbe_adapter *adapter)
 
 done:
 	/* Notify the stack of the (possibly) reduced Tx Queue count. */
-	adapter->netdev->real_num_tx_queues = adapter->num_tx_queues;
+	realloc_netdev_mq(adapter->netdev, adapter->num_tx_queues);
 }
 
 static void ixgbe_acquire_msix_vectors(struct ixgbe_adapter *adapter,

Quoted 10 lines · Eric Dumazet

Quoted 9 lines · David Miller

From: Eric Dumazet <[email protected]>
Date: Thu, 03 Dec 2009 22:29:42 +0100

Quoted 3 lines

Nice patch, thanks :)

Acked-by Eric Dumazet <[email protected]>

I like it too, but please resubmit once we have at least one example user submitted.

Quoted 1 line

I successfully tested following patch.

Great! But, I see, checking if realloc_netdev_mq() use is always legal (before register_netdev()) is not trivial in this driver. (I have some suspicions around ixgbe_resume().) I wonder, if there should be added some debugging for this.

Quoted 11 lines · Jarek Poplawski

Quoted 10 lines · Eric Dumazet

Quoted 9 lines · David Miller

From: Eric Dumazet <[email protected]>
Date: Thu, 03 Dec 2009 22:29:42 +0100

Quoted 3 lines

Nice patch, thanks :)

Acked-by Eric Dumazet <[email protected]>

I like it too, but please resubmit once we have at least one example user submitted.

Quoted 6 lines

Quoted 1 line

I successfully tested following patch.

Great! But, I see, checking if realloc_netdev_mq() use is always legal (before register_netdev()) is not trivial in this driver. (I have some suspicions around ixgbe_resume().) I wonder, if there should be added some debugging for this.

Yes, probably

Or even better, allowing realloc_netdev_mq() to be called even after register_netdev() :)

bnx2 for example could not be patched easily, following patch doesnt work.

drivers/net/bnx2.c +1 -1

index 4cae2a8..8bd5d0d 100644
@@ -6177,6 +6177,7 @@ bnx2_setup_int_mode(struct bnx2 *bp, int dis_msi)
 
 	bp->num_tx_rings = rounddown_pow_of_two(bp->irq_nvecs);
-	bp->dev->real_num_tx_queues = bp->num_tx_rings;
+	realloc_netdev_mq(bp->dev, bp->num_tx_rings);
 
 	bp->num_rx_rings = bp->irq_nvecs;
 }

Quoted 21 lines · Eric Dumazet

Quoted 11 lines · Jarek Poplawski

Quoted 10 lines · Eric Dumazet

Quoted 9 lines · David Miller

From: Eric Dumazet <[email protected]>
Date: Thu, 03 Dec 2009 22:29:42 +0100

Quoted 3 lines

Nice patch, thanks :)

Acked-by Eric Dumazet <[email protected]>

I like it too, but please resubmit once we have at least one example user submitted.

Quoted 6 lines

Quoted 1 line

I successfully tested following patch.

Great! But, I see, checking if realloc_netdev_mq() use is always legal (before register_netdev()) is not trivial in this driver. (I have some suspicions around ixgbe_resume().) I wonder, if there should be added some debugging for this.

Yes, probably

Quoted 2 lines

Or even better, allowing realloc_netdev_mq() to be called even after register_netdev() :)

We should try to avoid it because of clashes with qdisc initialization (especially wrt. preserving stats). But mainly it's about knowing the exact reason why this thing (probing the hardware for max mq capabilities) can't be finished before register_netdev(). I'll try to look at this more.

Honestly, there's no reason we can't know how many Tx queues we'll have prior to registering the netdev. All of this should be figured out after ixgbe_alloc_queues() and ixgbe_init_interrupt_scheme() are called.

Once we know what features are enabled, and how many MSI-X vectors the platform gives us, we can make the call for how many queues to allocate.

I like this realloc_netdev_mq() mechanism. I'm going to pull that into my tree for testing, since I unfortunately was unaware it existed (lack of my poking around). This is good stuff.

bnx2 for example could not be patched easily, following patch doesnt work.

drivers/net/bnx2.c +0 -0

index 4cae2a8..8bd5d0d 100644

On Fri, Dec 04, 2009 at 12:04:42AM +0100, Eric Dumazet wrote:
...

Quoted 12 lines

bnx2 for example could not be patched easily, following patch doesnt work.

drivers/net/bnx2.c +1 -1

index 4cae2a8..8bd5d0d 100644
@@ -6177,6 +6177,7 @@ bnx2_setup_int_mode(struct bnx2 *bp, int dis_msi)
 
 	bp->num_tx_rings = rounddown_pow_of_two(bp->irq_nvecs);
-	bp->dev->real_num_tx_queues = bp->num_tx_rings;
+	realloc_netdev_mq(bp->dev, bp->num_tx_rings);
 

drivers/net/bnx2.c bnx2_setup_int_mode(struct bnx2 *bp, int dis_msi) · line 6182

Quoted 2 lines

drivers/net/bnx2.c +0 -0

 	bp->num_rx_rings = bp->irq_nvecs;
 }

Eric, if didn't miss something in the example you gave at the beginning of this thread, it seems a change like below (not compiled) should fix your problem (without even testing MSI exactly)?

Quoted 14 lines · Jarek Poplawski

On Fri, Dec 04, 2009 at 12:04:42AM +0100, Eric Dumazet wrote:
...

Quoted 12 lines

bnx2 for example could not be patched easily, following patch doesnt work.

drivers/net/bnx2.c +1 -1

index 4cae2a8..8bd5d0d 100644
@@ -6177,6 +6177,7 @@ bnx2_setup_int_mode(struct bnx2 *bp, int dis_msi)
 
 	bp->num_tx_rings = rounddown_pow_of_two(bp->irq_nvecs);
-	bp->dev->real_num_tx_queues = bp->num_tx_rings;
+	realloc_netdev_mq(bp->dev, bp->num_tx_rings);
 

Quoted 6 lines

drivers/net/bnx2.c bnx2_setup_int_mode(struct bnx2 *bp, int dis_msi) · line 6182

Quoted 2 lines

drivers/net/bnx2.c +0 -0

 	bp->num_rx_rings = bp->irq_nvecs;
 }

Eric, if didn't miss something in the example you gave at the beginning of this thread, it seems a change like below (not compiled) should fix your problem (without even testing MSI exactly)?

OOPS! Take 2...;-)

Jarek P.
---

 drivers/net/bnx2.c |    5 +++++
 1 files changed, 5 insertions(+), 0 deletions(-)

drivers/net/bnx2.c +5 -0

index 4bfc808..c1ba2b1 100644
@@ -8288,6 +8288,11 @@ bnx2_init_one(struct pci_dev *pdev, const struct pci_device_id *ent)
 		dev->features |= NETIF_F_TSO6;
 		vlan_features_add(dev, NETIF_F_TSO6);
 	}
+
+	if (!((bp->flags & BNX2_FLAG_MSIX_CAP) ||
+	     (bp->flags & BNX2_FLAG_MSI_CAP)) || disable_msi)
+		realloc_netdev_mq(dev, 1);
+
 	if ((rc = register_netdev(dev))) {
 		dev_err(&pdev->dev, "Cannot register net device\n");
 		goto error;

Quoted 5 lines · Jarek Poplawski

Quoted 3 lines · Jarek Poplawski

Eric, if didn't miss something in the example you gave at the beginning of this thread, it seems a change like below (not compiled) should fix your problem (without even testing MSI exactly)?

Bringing back an older thread. Are we still planning to use realloc_netdev_mq()?

The problem with the patch below is that dev->real_num_tx_queues can still be reduced during bnx2_open().

Is it possible to call realloc_netdev_mq() during ->open()?

Quoted 26 lines

OOPS! Take 2...;-)

Jarek P.
---

 drivers/net/bnx2.c |    5 +++++
 1 files changed, 5 insertions(+), 0 deletions(-)

drivers/net/bnx2.c +5 -0

index 4bfc808..c1ba2b1 100644
@@ -8288,6 +8288,11 @@ bnx2_init_one(struct pci_dev *pdev, const struct pci_device_id *ent)
 		dev->features |= NETIF_F_TSO6;
 		vlan_features_add(dev, NETIF_F_TSO6);
 	}
+
+	if (!((bp->flags & BNX2_FLAG_MSIX_CAP) ||
+	     (bp->flags & BNX2_FLAG_MSI_CAP)) || disable_msi)
+		realloc_netdev_mq(dev, 1);
+
 	if ((rc = register_netdev(dev))) {
 		dev_err(&pdev->dev, "Cannot register net device\n");
 		goto error;

Quoted 3 lines

The problem with the patch below is that dev->real_num_tx_queues can still be reduced during bnx2_open().

I'm not sure I get your point, but there should be no problem with changing dev->real_num_tx_queues during ->open(). The main intention of realloc_netdev_mq() is to give drivers some official way to change dev->num_tx_queues until register_netdev() with the main aim: not to treat obviously non-mq chips as mq according to netif_is_multiqueue(). Additional gain is memory saved in the case fixed by the patch below (which btw. waits for some refinement/verification).

Quoted 2 lines

Is it possible to call realloc_netdev_mq() during ->open()?

No, and no such intention, since there was kind of agreement it's not necessary and collides with preserving qdisc state (stats). But the discussion isn't closed...

Thanks,
Jarek P.

Quoted 6 lines

Quoted 6 lines

OOPS! Take 2...;-)

Jarek P.
---

Quoted 8 lines

drivers/net/bnx2.c bnx2_init_one(struct pci_dev

Quoted 8 lines

 drivers/net/bnx2.c |    5 +++++
 1 files changed, 5 insertions(+), 0 deletions(-)

drivers/net/bnx2.c +0 -0

index 4bfc808..c1ba2b1 100644
@@ -8288,6 +8288,11 @@ bnx2_init_one(struct pci_dev

*pdev, const struct pci_device_id *ent)

Quoted 8 lines

drivers/net/bnx2.c bnx2_init_one(struct pci_dev *pdev, const struct pci_device_id *ent) · line 8295

Quoted 8 lines

drivers/net/bnx2.c +5 -0

          dev->features |= NETIF_F_TSO6;
          vlan_features_add(dev, NETIF_F_TSO6);
  }
+
+ if (!((bp->flags & BNX2_FLAG_MSIX_CAP) ||
+      (bp->flags & BNX2_FLAG_MSI_CAP)) || disable_msi)
+         realloc_netdev_mq(dev, 1);
+

Quoted 6 lines

Quoted 4 lines

drivers/net/bnx2.c +0 -0

  if ((rc = register_netdev(dev))) {
          dev_err(&pdev->dev, "Cannot register net device\n");
          goto error;

Jarek Poplawski wrote

Quoted 5 lines · Michael Chan

Quoted 3 lines

The problem with the patch below is that dev->real_num_tx_queues can still be reduced during bnx2_open().

Quoted 7 lines

I'm not sure I get your point, but there should be no problem with changing dev->real_num_tx_queues during ->open(). The main intention of realloc_netdev_mq() is to give drivers some official way to change dev->num_tx_queues until register_netdev() with the main aim: not to treat obviously non-mq chips as mq according to netif_is_multiqueue(). Additional gain is memory saved in the case fixed by the patch below (which btw. waits for some refinement/verification).

Yes, the patch achieves what you describe. dev->num_tx_queues will be set more accurately during ->probe based on chip type. In ->open(), the dev->real_num_tx_queues will still be possibly reduced depending on the number of CPU cores and whether we can enable and allocate the MSI-X vectors.

Quoted 4 lines

Quoted 4 lines

drivers/net/bnx2.c bnx2_init_one(struct pci_dev *pdev, const struct pci_device_id *ent) · line 8295

Quoted 4 lines

+ if (!((bp->flags & BNX2_FLAG_MSIX_CAP) ||

drivers/net/bnx2.c +3 -0

+      (bp->flags & BNX2_FLAG_MSI_CAP)) || disable_msi)
+         realloc_netdev_mq(dev, 1);
+

I think we can refine this a little:

        if (!(bp->flags & BNX2_FLAG_MSIX_CAP) || disable_msi)
                realloc_netdev_mq(dev, 1);

Quoted 14 lines · Michael Chan

Jarek Poplawski wrote

Quoted 5 lines · Michael Chan

Quoted 3 lines

The problem with the patch below is that dev->real_num_tx_queues can still be reduced during bnx2_open().

Quoted 7 lines

I'm not sure I get your point, but there should be no problem with changing dev->real_num_tx_queues during ->open(). The main intention of realloc_netdev_mq() is to give drivers some official way to change dev->num_tx_queues until register_netdev() with the main aim: not to treat obviously non-mq chips as mq according to netif_is_multiqueue(). Additional gain is memory saved in the case fixed by the patch below (which btw. waits for some refinement/verification).

Quoted 5 lines

Yes, the patch achieves what you describe. dev->num_tx_queues will be set more accurately during ->probe based on chip type. In ->open(), the dev->real_num_tx_queues will still be possibly reduced depending on the number of CPU cores and whether we can enable and allocate the MSI-X vectors.

I wondered, and Peter seemed to confirm, some check whether we can use MSI-X could be done during ->probe as well?

...

Quoted 10 lines

Quoted 4 lines

Quoted 4 lines

drivers/net/bnx2.c bnx2_init_one(struct pci_dev *pdev, const struct pci_device_id *ent) · line 8295

Quoted 4 lines

+ if (!((bp->flags & BNX2_FLAG_MSIX_CAP) ||

drivers/net/bnx2.c +3 -0

+      (bp->flags & BNX2_FLAG_MSI_CAP)) || disable_msi)
+         realloc_netdev_mq(dev, 1);
+

I think we can refine this a little:

        if (!(bp->flags & BNX2_FLAG_MSIX_CAP) || disable_msi)
                realloc_netdev_mq(dev, 1);

Hmm... Probably I should use different words. I mentioned above the main intention of realloc_netdev_mq() was to fix mq-ness for non-mq chips, and this patch demonstrates this. But, there is no reason not to fix/optimize more btw. So, I hoped the refinement could also include some preliminary MSI-X test etc. Anyway, since I can't even test it, I would be glad if you could author something based on this stub (unless you think it's enough for now - then I'll send it as is, no problem).

It's an improvement over existing code to unconditionally allocate max tx queues. To make further improvements, we'll have to experiment with calling pci_enable_msix() so early during ->probe(). I will look into it. Thanks.

On Thu, Oct 08, 2009 at 09:18:45AM +0200, Eric Dumazet wrote:

Quoted 17 lines

Just wondering if it could hurt some people.

Anyway, should we change bnx2/tg3 drivers so that single queue devices have same default qdisc/class than before ?

eg :

drivers/net/bnx2.c +1 -0

index 08cddb6..7cac205 100644
@@ -6152,6 +6152,7 @@ bnx2_setup_int_mode(struct bnx2 *bp, int dis_msi)
 
 	bp->num_tx_rings = rounddown_pow_of_two(bp->irq_nvecs);
 	bp->dev->real_num_tx_queues = bp->num_tx_rings;
+	bp->dev->num_tx_queues = bp->dev->real_num_tx_queues;
 

drivers/net/bnx2.c bnx2_setup_int_mode(struct bnx2 *bp, int dis_msi) · line 6158

Quoted 2 lines

drivers/net/bnx2.c +0 -0

 	bp->num_rx_rings = bp->irq_nvecs;
 }

It doesn't look consistent to me wrt. the comment in netdevice.h on num_tx_queues. But it seems we should rather use more often real_num_tx_queue in schedulers code like dumps and maybe more (like e.g. sch_multiq does).

Quoted 1 line · Jarek Poplawski

On Thu, Oct 08, 2009 at 09:18:45AM +0200, Eric Dumazet wrote:

...

Quoted 17 lines

Quoted 17 lines

Just wondering if it could hurt some people.

Anyway, should we change bnx2/tg3 drivers so that single queue devices have same default qdisc/class than before ?

eg :

drivers/net/bnx2.c +1 -0

index 08cddb6..7cac205 100644
@@ -6152,6 +6152,7 @@ bnx2_setup_int_mode(struct bnx2 *bp, int dis_msi)
 
 	bp->num_tx_rings = rounddown_pow_of_two(bp->irq_nvecs);
 	bp->dev->real_num_tx_queues = bp->num_tx_rings;
+	bp->dev->num_tx_queues = bp->dev->real_num_tx_queues;
 

Quoted 7 lines

drivers/net/bnx2.c bnx2_setup_int_mode(struct bnx2 *bp, int dis_msi) · line 6158

Quoted 2 lines

drivers/net/bnx2.c +0 -0

 	bp->num_rx_rings = bp->irq_nvecs;
 }

It doesn't look consistent to me wrt. the comment in netdevice.h on num_tx_queues. But it seems we should rather use more often real_num_tx_queue in schedulers code like dumps and maybe more (like e.g. sch_multiq does).

So, according to my current understanding, we should probably let drivers to reset the tx_queues allocation with a new num_tx_queues in a place like this (ndo_open).