Netdev List
 help / color / mirror / Atom feed
* Re: [PATCH net-next-2.6 v2] pkt_sched: gen_estimator: kill est_lock rwlock
From: Jarek Poplawski @ 2010-06-08 19:29 UTC (permalink / raw)
  To: Eric Dumazet
  Cc: Changli Gao, David Miller, netdev, Stephen Hemminger,
	Patrick McHardy
In-Reply-To: <20100608124052.GB9392@ff.dom.local>

Jarek Poplawski wrote, On 06/08/2010 02:40 PM:

> On Tue, Jun 08, 2010 at 02:27:32PM +0200, Eric Dumazet wrote:
>> Le mardi 08 juin 2010 ?? 12:15 +0000, Jarek Poplawski a écrit :
>>
>>> Actually, I guess, Changli meant the bug introduced by your previous
>>> patch by removing the est_lock. With this lock (and your commit 5d944)
>>> bstats (and API) seem "fundamentaly" safe.
>>>
>> Sorry, I have no idea of what you want to say, I cant find commit 5d944.
>>
> 
> Sorry, I meant the commit mentioned in your changelog which was quoted.
> 
>> This was partially addressed in commit 5d944c640b4 (gen_estimator:
>> deadlock fix), but same problem exist for all gen_kill_estimator()
>> users.

In case it's still unclear, I wanted to say that IMHO this patch's
title and changelog are misleading because most of its content are
est_lock to RCU changes - not fixes.

>> [PATCH net-next-2.6] pkt_sched: gen_kill_estimator() rcu fixes
>> 
>> gen_kill_estimator() API is fundamentaly wrong, since caller should make
>> sure an RCU grace period is respected before freeing bstats or lock.

Freeing bstats doesn't currently require RCU protection.

Jarek P.

^ permalink raw reply

* Re: [PATCH net-next-2.6 v2] pkt_sched: gen_estimator: kill est_lock rwlock
From: Eric Dumazet @ 2010-06-08 19:45 UTC (permalink / raw)
  To: Jarek Poplawski
  Cc: Changli Gao, David Miller, netdev, Stephen Hemminger,
	Patrick McHardy
In-Reply-To: <4C0E9A2E.9080109@gmail.com>

Le mardi 08 juin 2010 à 21:29 +0200, Jarek Poplawski a écrit :
> Jarek Poplawski wrote, On 06/08/2010 02:40 PM:
> 
> > On Tue, Jun 08, 2010 at 02:27:32PM +0200, Eric Dumazet wrote:
> >> Le mardi 08 juin 2010 ?? 12:15 +0000, Jarek Poplawski a écrit :
> >>
> >>> Actually, I guess, Changli meant the bug introduced by your previous
> >>> patch by removing the est_lock. With this lock (and your commit 5d944)
> >>> bstats (and API) seem "fundamentaly" safe.
> >>>
> >> Sorry, I have no idea of what you want to say, I cant find commit 5d944.
> >>
> > 
> > Sorry, I meant the commit mentioned in your changelog which was quoted.
> > 
> >> This was partially addressed in commit 5d944c640b4 (gen_estimator:
> >> deadlock fix), but same problem exist for all gen_kill_estimator()
> >> users.
> 
> In case it's still unclear, I wanted to say that IMHO this patch's
> title and changelog are misleading because most of its content are
> est_lock to RCU changes - not fixes.
> 

...

> >> [PATCH net-next-2.6] pkt_sched: gen_kill_estimator() rcu fixes
> >> 
> >> gen_kill_estimator() API is fundamentaly wrong, since caller should make
> >> sure an RCU grace period is respected before freeing bstats or lock.
> 
> Freeing bstats doesn't currently require RCU protection.
> 
> Jarek P.

So what ? No changes needed ?

I am really lost by your comments Jarek.

Maybe you could provide an alternative patch, so that we can make some
progress ?




^ permalink raw reply

* Re: [PATCH net-next-2.6 v2] pkt_sched: gen_estimator: kill est_lock rwlock
From: Jarek Poplawski @ 2010-06-08 20:24 UTC (permalink / raw)
  To: Eric Dumazet
  Cc: Changli Gao, David Miller, netdev, Stephen Hemminger,
	Patrick McHardy
In-Reply-To: <1276026329.2439.2.camel@edumazet-laptop>

On Tue, Jun 08, 2010 at 09:45:29PM +0200, Eric Dumazet wrote:
> Le mardi 08 juin 2010 ?? 21:29 +0200, Jarek Poplawski a écrit :
> > Jarek Poplawski wrote, On 06/08/2010 02:40 PM:
> > 
> > > On Tue, Jun 08, 2010 at 02:27:32PM +0200, Eric Dumazet wrote:
> > >> Le mardi 08 juin 2010 ?? 12:15 +0000, Jarek Poplawski a écrit :
> > >>
> > >>> Actually, I guess, Changli meant the bug introduced by your previous
> > >>> patch by removing the est_lock. With this lock (and your commit 5d944)
> > >>> bstats (and API) seem "fundamentaly" safe.
> > >>>
> > >> Sorry, I have no idea of what you want to say, I cant find commit 5d944.
> > >>
> > > 
> > > Sorry, I meant the commit mentioned in your changelog which was quoted.
> > > 
> > >> This was partially addressed in commit 5d944c640b4 (gen_estimator:
> > >> deadlock fix), but same problem exist for all gen_kill_estimator()
> > >> users.
> > 
> > In case it's still unclear, I wanted to say that IMHO this patch's
> > title and changelog are misleading because most of its content are
> > est_lock to RCU changes - not fixes.
> > 
> 
> ...
> 
> > >> [PATCH net-next-2.6] pkt_sched: gen_kill_estimator() rcu fixes
> > >> 
> > >> gen_kill_estimator() API is fundamentaly wrong, since caller should make
> > >> sure an RCU grace period is respected before freeing bstats or lock.
> > 
> > Freeing bstats doesn't currently require RCU protection.
> > 
> > Jarek P.
> 
> So what ? No changes needed ?

Such a changelog is a documentation for future, just like this one,
crucial for this suject:
http://git.kernel.org/?p=linux/kernel/git/torvalds/linux-2.6.git;a=commit;h=0929c2dd83317813425b937fbc0041013b8685ff

There is no reason to make people (our?) life harder with describing
unexistent bugs.

> 
> I am really lost by your comments Jarek.
> 
> Maybe you could provide an alternative patch, so that we can make some
> progress ?
> 

No, I only meant fixing the title and changelog if you have any problem
with separating it for two parts.

Jarek P.

^ permalink raw reply

* Re: [PATCH net-next-2.6 v2] pkt_sched: gen_estimator: kill est_lock rwlock
From: Eric Dumazet @ 2010-06-08 20:52 UTC (permalink / raw)
  To: Jarek Poplawski
  Cc: Changli Gao, David Miller, netdev, Stephen Hemminger,
	Patrick McHardy
In-Reply-To: <20100608202405.GA3496@del.dom.local>


> Such a changelog is a documentation for future, just like this one,
> crucial for this suject:
> http://git.kernel.org/?p=linux/kernel/git/torvalds/linux-2.6.git;a=commit;h=0929c2dd83317813425b937fbc0041013b8685ff
> 
> There is no reason to make people (our?) life harder with describing
> unexistent bugs.
> 

Now you are telling me there is no bug, and I am lying ?

> > 
> > I am really lost by your comments Jarek.
> > 
> > Maybe you could provide an alternative patch, so that we can make some
> > progress ?
> > 
> 
> No, I only meant fixing the title and changelog if you have any problem
> with separating it for two parts.

I am suggesting you rewrite this Changelog, as being part of the patch,
since I still miss your point, sorry, I must be very dumb tonight.

Then, add your Signed-off-by.

This is normal iterative process.



^ permalink raw reply

* Re: [PATCH net-next-2.6 v2] pkt_sched: gen_estimator: kill est_lock rwlock
From: Jarek Poplawski @ 2010-06-08 21:18 UTC (permalink / raw)
  To: Eric Dumazet
  Cc: Changli Gao, David Miller, netdev, Stephen Hemminger,
	Patrick McHardy
In-Reply-To: <1276030354.2439.8.camel@edumazet-laptop>

On Tue, Jun 08, 2010 at 10:52:34PM +0200, Eric Dumazet wrote:
> 
> > Such a changelog is a documentation for future, just like this one,
> > crucial for this suject:
> > http://git.kernel.org/?p=linux/kernel/git/torvalds/linux-2.6.git;a=commit;h=0929c2dd83317813425b937fbc0041013b8685ff
> > 
> > There is no reason to make people (our?) life harder with describing
> > unexistent bugs.
> > 
> 
> Now you are telling me there is no bug, and I am lying ?
> 

I didn't question freeing stats_lock (great catch BTW!).

> > > 
> > > I am really lost by your comments Jarek.
> > > 
> > > Maybe you could provide an alternative patch, so that we can make some
> > > progress ?
> > > 
> > 
> > No, I only meant fixing the title and changelog if you have any problem
> > with separating it for two parts.
> 
> I am suggesting you rewrite this Changelog, as being part of the patch,
> since I still miss your point, sorry, I must be very dumb tonight.
> 
> Then, add your Signed-off-by.
> 
> This is normal iterative process.
> 

No understado - no Englisch ;-)

Sorry,
Jarek P.

^ permalink raw reply

* 2.6.35-rc2, CONFIG_RPS is filling the dmesg log
From: Tim Gardner @ 2010-06-08 20:55 UTC (permalink / raw)
  To: netdev

With 2.6.35-rc2 my dmesg log is being flooded with messages like this:

br0 received packet on queue 4, but number of RX queues is 1

This machine is bridged for KVM and has 2 igb network adapters.

The root cause appears to be CONFIG_RPS=y and the fact that none of the 
drivers that call skb_record_rx_queue() perform their net device 
allocation using alloc_netdev_mq(), thereby initializing num_rx_queues 
to a maximum of 1.

Given that this is early RPS days, is the warning in get_rps_cpu() 
really necessary? It would appear that _all_ of the multi-receive queue 
devices that call skb_record_rx_queue() will cause this log noise.

By the way, how do you turn off CONFIG_RPS? The only way I could get it 
disabled was to change the default in net/Kconfig to 'n'.

rtg
-- 
Tim Gardner tim.gardner@canonical.com

^ permalink raw reply

* Re: [PATCH -next] enic: fix pci_alloc_consistent argument
From: Scott Feldman @ 2010-06-08 21:55 UTC (permalink / raw)
  To: Randy Dunlap, netdev; +Cc: davem, Vasanthy Kolluri, Roopa Prabhu
In-Reply-To: <20100608100020.7d79e932.randy.dunlap@oracle.com>

On 6/8/10 10:00 AM, "Randy Dunlap" <randy.dunlap@oracle.com> wrote:

> From: Randy Dunlap <randy.dunlap@oracle.com>
> 
> Fix build warning on i386 (32-bit) with 32-bit dma_addr_t:
> 
> drivers/net/enic/vnic_dev.c: In function 'vnic_dev_init_prov':
> drivers/net/enic/vnic_dev.c:716: warning: passing argument 3 of
> 'pci_alloc_consistent' from incompatible pointer type
> include/asm-generic/pci-dma-compat.h:16: note: expected 'dma_addr_t *' but
> argument is of type 'u64 *'
> 
> Now builds without warnings on i386 and on x86_64.
> 
> Signed-off-by: Randy Dunlap <randy.dunlap@oracle.com>
> Cc: Scott Feldman <scofeldm@cisco.com>
> Cc: Vasanthy Kolluri <vkolluri@cisco.com>
> Cc: Roopa Prabhu <roprabhu@cisco.com>
> ---
>  drivers/net/enic/vnic_dev.c |    2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)

Acked-by: Scott Feldman <scofeldm@cisco.com>


^ permalink raw reply

* 2.6.35-rc2-git2: Reported regressions from 2.6.34
From: Rafael J. Wysocki @ 2010-06-08 22:06 UTC (permalink / raw)
  To: Linux Kernel Mailing List
  Cc: Maciej Rutecki, Andrew Morton, Linus Torvalds,
	Kernel Testers List, Network Development, Linux ACPI,
	Linux PM List, Linux SCSI List, Linux Wireless List, DRI

[NOTES:
 * This by no means is a complete list, but we only put e-mail reports that
   are at least 1 week old into the Bugzilla.
 * Quite a few of the already reported regressions may be related to the bug
   fixed by 386f40c86d6c8d5b717ef20620af1a750d0dacb4 (Revert "tty: fix a little
   bug in scrup, vt.c"), so reporters please retest with this commit applied.]

This message contains a list of some regressions from 2.6.34,
for which there are no fixes in the mainline known to the tracking team.
If any of them have been fixed already, please let us know.

If you know of any other unresolved regressions from 2.6.34, please let us
know either and we'll add them to the list.  Also, please let us know
if any of the entries below are invalid.

Each entry from the list will be sent additionally in an automatic reply
to this message with CCs to the people involved in reporting and handling
the issue.


Listed regressions statistics:

  Date          Total  Pending  Unresolved
  ----------------------------------------
  2010-06-09       15       13          10


Unresolved regressions
----------------------

Bug-Entry	: http://bugzilla.kernel.org/show_bug.cgi?id=16163
Subject		: [2.6.35-rc1 Regression] i915: Commit cfecde causes VGA to stay off
Submitter	: David John <davidjon@xenontk.org>
Date		: 2010-06-02 12:52 (7 days old)
Message-ID	: <4C065423.3000202@xenontk.org>
References	: http://marc.info/?l=linux-kernel&m=127548313828613&w=2


Bug-Entry	: http://bugzilla.kernel.org/show_bug.cgi?id=16161
Subject		: [2.6.35-rc1 regression] sysfs: cannot create duplicate filename ... XVR-600 related?
Submitter	: Mikael Pettersson <mikpe@it.uu.se>
Date		: 2010-06-01 19:57 (8 days old)
Message-ID	: <19461.26166.427857.612983@pilspetsen.it.uu.se>
References	: http://marc.info/?l=linux-kernel&m=127542227511925&w=2


Bug-Entry	: http://bugzilla.kernel.org/show_bug.cgi?id=16160
Subject		: 2.6.35 Radeon KMS power management regression?
Submitter	: Nigel Cunningham <ncunningham@crca.org.au>
Date		: 2010-06-01 6:23 (8 days old)
Message-ID	: <4C04A767.8000209@crca.org.au>
References	: http://marc.info/?l=linux-kernel&m=127537343722290&w=2


Bug-Entry	: http://bugzilla.kernel.org/show_bug.cgi?id=16145
Subject		: Unable to boot after "ACPI: Don't let acpi_pad needlessly mark TSC unstable"
Submitter	: Tom Gundersen <teg@jklm.no>
Date		: 2010-06-07 13:11 (2 days old)
Handled-By	: Venkatesh Pallipadi <venki@google.com>


Bug-Entry	: http://bugzilla.kernel.org/show_bug.cgi?id=16129
Subject		: BUG: using smp_processor_id() in preemptible [00000000] code: jbd2/sda2
Submitter	: Jan Kreuzer <kontrollator@gmx.de>
Date		: 2010-06-05 06:15 (4 days old)


Bug-Entry	: http://bugzilla.kernel.org/show_bug.cgi?id=16122
Subject		: 2.6.35-rc1: WARNING at fs/fs-writeback.c:1142 __mark_inode_dirty+0x103/0x170
Submitter	: Larry Finger <Larry.Finger@lwfinger.net>
Date		: 2010-06-04 13:18 (5 days old)


Bug-Entry	: http://bugzilla.kernel.org/show_bug.cgi?id=16120
Subject		: Oops: 0000 [#1] SMP, unable to handle kernel NULL pointer dereference at (null)
Submitter	: Alex Zhavnerchik <alex.vizor@gmail.com>
Date		: 2010-06-04 09:25 (5 days old)
Handled-By	: Eric Dumazet <eric.dumazet@gmail.com>


Bug-Entry	: http://bugzilla.kernel.org/show_bug.cgi?id=16104
Subject		: Radeon KMS does not start after merge of the new PM-Code
Submitter	: Jan Kreuzer <kontrollator@gmx.de>
Date		: 2010-06-02 07:47 (7 days old)


Bug-Entry	: http://bugzilla.kernel.org/show_bug.cgi?id=16090
Subject		: sysfs: cannot create duplicate filename
Submitter	: Tobias <devnull@plzk.org>
Date		: 2010-06-01 15:59 (8 days old)
Handled-By	: Jesse Barnes <jbarnes@virtuousgeek.org>


Bug-Entry	: http://bugzilla.kernel.org/show_bug.cgi?id=16037
Subject		: NULL Pointer dereference in __ir_input_register/budget_ci_attach
Submitter	: Sean Finney <seanius@debian.org>
Date		: 2010-05-23 19:52 (17 days old)


Regressions with patches
------------------------

Bug-Entry	: http://bugzilla.kernel.org/show_bug.cgi?id=16131
Subject		: kernel BUG at fs/btrfs/extent-tree.c:4363 (btrfs_free_tree_block)
Submitter	: Chow Loong Jin <hyperair@ubuntu.com>
Date		: 2010-06-05 18:53 (4 days old)
Handled-By	: Yan Zheng <zheng.yan@oracle.com>
Patch		: https://patchwork.kernel.org/patch/103235/


Bug-Entry	: http://bugzilla.kernel.org/show_bug.cgi?id=16127
Subject		: Boot freeze on HP Compaq nx6325 (RS482) with Radeon KMS
Submitter	: Jure Repinc <jlp.bugs@gmail.com>
Date		: 2010-06-04 21:14 (5 days old)
Handled-By	: Dave Airlie <airlied@linux.ie>
Patch		: https://bugzilla.kernel.org/attachment.cgi?id=26677


Bug-Entry	: http://bugzilla.kernel.org/show_bug.cgi?id=16092
Subject		: Caught 64-bit read from uninitialized memory in memtype_rb_augment_cb
Submitter	: Christian Casteyde <casteyde.christian@free.fr>
Date		: 2010-06-01 18:08 (8 days old)
Handled-By	: Venki <venki@google.com>
Patch		: https://bugzilla.kernel.org/show_bug.cgi?id=16092#c2


For details, please visit the bug entries and follow the links given in
references.

As you can see, there is a Bugzilla entry for each of the listed regressions.
There also is a Bugzilla entry used for tracking the regressions from 2.6.34,
unresolved as well as resolved, at:

http://bugzilla.kernel.org/show_bug.cgi?id=16055

Please let the tracking team know if there are any Bugzilla entries that
should be added to the list in there.

Thanks!


^ permalink raw reply

* Re: RFS seems to have incompatiblities with bridged vlans
From: Tom Herbert @ 2010-06-08 23:00 UTC (permalink / raw)
  To: Eric Dumazet
  Cc: John Fastabend, Stephen Hemminger, Peter Lieven,
	davem@davemloft.net, netdev@vger.kernel.org
In-Reply-To: <1276006721.2486.141.camel@edumazet-laptop>

How about only checking against dev->num_rx_queues when that value is
greater than one.  Since bonding device is calling alloc_netdev, it is
not going to set the queue mapping, but dev->num_rx_queues will be one
in that case (this handles any intermediate driver that does do
multiple queues).

diff --git a/net/core/dev.c b/net/core/dev.c
index 6f330ce..30ab66d 100644
--- a/net/core/dev.c
+++ b/net/core/dev.c
@@ -2270,7 +2270,7 @@ static int get_rps_cpu(struct net_device *dev,
struct sk_buff *skb,
                u16 v16[2];
        } ports;

-       if (skb_rx_queue_recorded(skb)) {
+       if (skb_rx_queue_recorded(skb) && dev->num_rx_queues > 1) {
                u16 index = skb_get_rx_queue(skb);
                if (unlikely(index >= dev->num_rx_queues)) {
                        if (net_ratelimit()) {


On Tue, Jun 8, 2010 at 7:18 AM, Eric Dumazet <eric.dumazet@gmail.com> wrote:
> Le lundi 07 juin 2010 à 15:30 -0700, John Fastabend a écrit :
>
>> There is always a possibility that the underlying device sets the
>> queue_mapping to be greater then num_cpus.  Also I suspect the same
>> issue exists with bonding devices.  Maybe something like the following
>> is worth while? compile tested only,
>>
>> [PATCH] 8021q: vlan reassigns dev without check queue_mapping
>>
>> recv path reassigns skb->dev without sanity checking the
>> queue_mapping field.  This can result in the queue_mapping
>> field being set incorrectly if the new dev supports less
>> queues then the underlying device.
>>
>> This patch just resets the queue_mapping to 0 which should
>> resolve this issue?  Any thoughts?
>>
>> The same issue could happen on bonding devices as well.
>>
>> Signed-off-by: John Fastabend <john.r.fastabend@intel.com>
>> ---
>>
>>   net/8021q/vlan_core.c |    6 ++++++
>>   1 files changed, 6 insertions(+), 0 deletions(-)
>>
>> diff --git a/net/8021q/vlan_core.c b/net/8021q/vlan_core.c
>> index bd537fc..ad309f8 100644
>> --- a/net/8021q/vlan_core.c
>> +++ b/net/8021q/vlan_core.c
>> @@ -21,6 +21,9 @@ int __vlan_hwaccel_rx(struct sk_buff *skb, struct
>> vlan_group *grp,
>>       if (!skb->dev)
>>               goto drop;
>>
>> +     if (unlikely(skb->queue_mapping >= skb->dev->real_num_tx_queues))
>> +             skb_set_queue_mapping(skb, 0);
>> +
>>       return (polling ? netif_receive_skb(skb) : netif_rx(skb));
>>
>>   drop:
>> @@ -93,6 +96,9 @@ vlan_gro_common(struct napi_struct *napi, struct
>> vlan_group *grp,
>>       if (!skb->dev)
>>               goto drop;
>>
>> +     if (unlikely(skb->queue_mapping >= skb->dev->real_num_tx_queues))
>> +             skb_set_queue_mapping(skb, 0);
>> +
>>       for (p = napi->gro_list; p; p = p->next) {
>>               NAPI_GRO_CB(p)->same_flow =
>>                       p->dev == skb->dev && !compare_ether_header(
>> --
>
> Only a workaround, added in hot path in a otherwise 'good' driver
> (multiqueue enabled and ready)
>
> eth0  -------> bond / bridge ---------> vlan.id
> (nbtxq=8)      (ntxbq=1)        (nbtxq=X)
>
> X is capped to 1 because of bond/bridge, while bond has no "queue"
> (LLTX driver)
>
> Solutions :
>
> 1) queue_mapping could be silently tested in get_rps_cpu()...
>
> diff --git a/net/core/dev.c b/net/core/dev.c
> index 6f330ce..3a3f7f6 100644
> --- a/net/core/dev.c
> +++ b/net/core/dev.c
> @@ -2272,14 +2272,11 @@ static int get_rps_cpu(struct net_device *dev, struct sk_buff *skb,
>
>        if (skb_rx_queue_recorded(skb)) {
>                u16 index = skb_get_rx_queue(skb);
> -               if (unlikely(index >= dev->num_rx_queues)) {
> -                       if (net_ratelimit()) {
> -                               pr_warning("%s received packet on queue "
> -                                       "%u, but number of RX queues is %u\n",
> -                                       dev->name, index, dev->num_rx_queues);
> -                       }
> -                       goto done;
> -               }
> +               if (WARN_ONCE(index >= dev->num_rx_queues,
> +                               KERN_WARNING "%s received packet on queue %u, "
> +                               "but number of RX queues is %u\n",
> +                               dev->name, index, dev->num_rx_queues))
> +                       index %= dev->num_rx_queues;
>                rxqueue = dev->_rx + index;
>        } else
>                rxqueue = dev->_rx;
>
>
>
> 2) bond/bridge should setup more queues, just in case.
>   We probably need to be able to make things more dynamic,
>   (propagate nbtxq between layers) but not for 2.6.35
>
>
>
> diff --git a/drivers/net/bonding/bond_main.c b/drivers/net/bonding/bond_main.c
> index 5e12462..ce813dd 100644
> --- a/drivers/net/bonding/bond_main.c
> +++ b/drivers/net/bonding/bond_main.c
> @@ -5012,8 +5012,8 @@ int bond_create(struct net *net, const char *name)
>
>        rtnl_lock();
>
> -       bond_dev = alloc_netdev(sizeof(struct bonding), name ? name : "",
> -                               bond_setup);
> +       bond_dev = alloc_netdev_mq(sizeof(struct bonding), name ? name : "",
> +                                  bond_setup, max(64, nr_cpu_ids));
>        if (!bond_dev) {
>                pr_err("%s: eek! can't alloc netdev!\n", name);
>                rtnl_unlock();
>
>
> --
> To unsubscribe from this list: send the line "unsubscribe netdev" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>

^ permalink raw reply related

* Re: RFS seems to have incompatiblities with bridged vlans
From: Tim Gardner @ 2010-06-08 23:58 UTC (permalink / raw)
  To: Tom Herbert
  Cc: Eric Dumazet, John Fastabend, Stephen Hemminger, Peter Lieven,
	davem@davemloft.net, netdev@vger.kernel.org
In-Reply-To: <AANLkTiluIibUglhf28JAeSLzBC0E1Zw0STPoqFTbXcev@mail.gmail.com>

[-- Attachment #1: Type: text/plain, Size: 6157 bytes --]

On 06/08/2010 05:00 PM, Tom Herbert wrote:
> How about only checking against dev->num_rx_queues when that value is
> greater than one.  Since bonding device is calling alloc_netdev, it is
> not going to set the queue mapping, but dev->num_rx_queues will be one
> in that case (this handles any intermediate driver that does do
> multiple queues).
>
> diff --git a/net/core/dev.c b/net/core/dev.c
> index 6f330ce..30ab66d 100644
> --- a/net/core/dev.c
> +++ b/net/core/dev.c
> @@ -2270,7 +2270,7 @@ static int get_rps_cpu(struct net_device *dev,
> struct sk_buff *skb,
>                  u16 v16[2];
>          } ports;
>
> -       if (skb_rx_queue_recorded(skb)) {
> +       if (skb_rx_queue_recorded(skb)&&  dev->num_rx_queues>  1) {
>                  u16 index = skb_get_rx_queue(skb);
>                  if (unlikely(index>= dev->num_rx_queues)) {
>                          if (net_ratelimit()) {
>
>
> On Tue, Jun 8, 2010 at 7:18 AM, Eric Dumazet<eric.dumazet@gmail.com>  wrote:
>> Le lundi 07 juin 2010 à 15:30 -0700, John Fastabend a écrit :
>>
>>> There is always a possibility that the underlying device sets the
>>> queue_mapping to be greater then num_cpus.  Also I suspect the same
>>> issue exists with bonding devices.  Maybe something like the following
>>> is worth while? compile tested only,
>>>
>>> [PATCH] 8021q: vlan reassigns dev without check queue_mapping
>>>
>>> recv path reassigns skb->dev without sanity checking the
>>> queue_mapping field.  This can result in the queue_mapping
>>> field being set incorrectly if the new dev supports less
>>> queues then the underlying device.
>>>
>>> This patch just resets the queue_mapping to 0 which should
>>> resolve this issue?  Any thoughts?
>>>
>>> The same issue could happen on bonding devices as well.
>>>
>>> Signed-off-by: John Fastabend<john.r.fastabend@intel.com>
>>> ---
>>>
>>>    net/8021q/vlan_core.c |    6 ++++++
>>>    1 files changed, 6 insertions(+), 0 deletions(-)
>>>
>>> diff --git a/net/8021q/vlan_core.c b/net/8021q/vlan_core.c
>>> index bd537fc..ad309f8 100644
>>> --- a/net/8021q/vlan_core.c
>>> +++ b/net/8021q/vlan_core.c
>>> @@ -21,6 +21,9 @@ int __vlan_hwaccel_rx(struct sk_buff *skb, struct
>>> vlan_group *grp,
>>>        if (!skb->dev)
>>>                goto drop;
>>>
>>> +     if (unlikely(skb->queue_mapping>= skb->dev->real_num_tx_queues))
>>> +             skb_set_queue_mapping(skb, 0);
>>> +
>>>        return (polling ? netif_receive_skb(skb) : netif_rx(skb));
>>>
>>>    drop:
>>> @@ -93,6 +96,9 @@ vlan_gro_common(struct napi_struct *napi, struct
>>> vlan_group *grp,
>>>        if (!skb->dev)
>>>                goto drop;
>>>
>>> +     if (unlikely(skb->queue_mapping>= skb->dev->real_num_tx_queues))
>>> +             skb_set_queue_mapping(skb, 0);
>>> +
>>>        for (p = napi->gro_list; p; p = p->next) {
>>>                NAPI_GRO_CB(p)->same_flow =
>>>                        p->dev == skb->dev&&  !compare_ether_header(
>>> --
>>
>> Only a workaround, added in hot path in a otherwise 'good' driver
>> (multiqueue enabled and ready)
>>
>> eth0  ------->  bond / bridge --------->  vlan.id
>> (nbtxq=8)      (ntxbq=1)        (nbtxq=X)
>>
>> X is capped to 1 because of bond/bridge, while bond has no "queue"
>> (LLTX driver)
>>
>> Solutions :
>>
>> 1) queue_mapping could be silently tested in get_rps_cpu()...
>>
>> diff --git a/net/core/dev.c b/net/core/dev.c
>> index 6f330ce..3a3f7f6 100644
>> --- a/net/core/dev.c
>> +++ b/net/core/dev.c
>> @@ -2272,14 +2272,11 @@ static int get_rps_cpu(struct net_device *dev, struct sk_buff *skb,
>>
>>         if (skb_rx_queue_recorded(skb)) {
>>                 u16 index = skb_get_rx_queue(skb);
>> -               if (unlikely(index>= dev->num_rx_queues)) {
>> -                       if (net_ratelimit()) {
>> -                               pr_warning("%s received packet on queue "
>> -                                       "%u, but number of RX queues is %u\n",
>> -                                       dev->name, index, dev->num_rx_queues);
>> -                       }
>> -                       goto done;
>> -               }
>> +               if (WARN_ONCE(index>= dev->num_rx_queues,
>> +                               KERN_WARNING "%s received packet on queue %u, "
>> +                               "but number of RX queues is %u\n",
>> +                               dev->name, index, dev->num_rx_queues))
>> +                       index %= dev->num_rx_queues;
>>                 rxqueue = dev->_rx + index;
>>         } else
>>                 rxqueue = dev->_rx;
>>
>>
>>
>> 2) bond/bridge should setup more queues, just in case.
>>    We probably need to be able to make things more dynamic,
>>    (propagate nbtxq between layers) but not for 2.6.35
>>
>>
>>
>> diff --git a/drivers/net/bonding/bond_main.c b/drivers/net/bonding/bond_main.c
>> index 5e12462..ce813dd 100644
>> --- a/drivers/net/bonding/bond_main.c
>> +++ b/drivers/net/bonding/bond_main.c
>> @@ -5012,8 +5012,8 @@ int bond_create(struct net *net, const char *name)
>>
>>         rtnl_lock();
>>
>> -       bond_dev = alloc_netdev(sizeof(struct bonding), name ? name : "",
>> -                               bond_setup);
>> +       bond_dev = alloc_netdev_mq(sizeof(struct bonding), name ? name : "",
>> +                                  bond_setup, max(64, nr_cpu_ids));
>>         if (!bond_dev) {
>>                 pr_err("%s: eek! can't alloc netdev!\n", name);
>>                 rtnl_unlock();
>>
>>
>> --
>> To unsubscribe from this list: send the line "unsubscribe netdev" in
>> the body of a message to majordomo@vger.kernel.org
>> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>>
> --
> To unsubscribe from this list: send the line "unsubscribe netdev" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Huh, so you guys are looking at the same issue (only my issue is RPS). 
See http://marc.info/?l=linux-netdev&m=127603240621028&w=2. I'm in favor 
of dropping the warning when no queues have been allocated.

How about this (see attached).

rtg

-- 
Tim Gardner tim.gardner@canonical.com

[-- Attachment #2: 0001-net-Print-num_rx_queues-warning-only-when-there-are-.patch --]
[-- Type: text/x-patch, Size: 1152 bytes --]

>From 391b0140b5c7e410d55258a8a2541bddf84d8d0e Mon Sep 17 00:00:00 2001
From: Tim Gardner <tim.gardner@canonical.com>
Date: Tue, 8 Jun 2010 17:51:27 -0600
Subject: [PATCH] net: Print num_rx_queues warning only when there are allocated queues

Most users of skb_record_rx_queue() do not use alloc_netdev_mq() for
network device initialization, so don't print a warning about num_rx_queues
overflow in get_rps_cpu() unless they have actually been allocated.

Signed-off-by: Tim Gardner <tim.gardner@canonical.com>
---
 net/core/dev.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/net/core/dev.c b/net/core/dev.c
index d03470f..0852608 100644
--- a/net/core/dev.c
+++ b/net/core/dev.c
@@ -2253,7 +2253,7 @@ static int get_rps_cpu(struct net_device *dev, struct sk_buff *skb,
 	if (skb_rx_queue_recorded(skb)) {
 		u16 index = skb_get_rx_queue(skb);
 		if (unlikely(index >= dev->num_rx_queues)) {
-			if (net_ratelimit()) {
+			if (dev->num_rx_queues > 1 && net_ratelimit()) {
 				pr_warning("%s received packet on queue "
 					"%u, but number of RX queues is %u\n",
 					dev->name, index, dev->num_rx_queues);
-- 
1.7.0.4


^ permalink raw reply related

* Re: RFS seems to have incompatiblities with bridged vlans
From: John Fastabend @ 2010-06-09  1:08 UTC (permalink / raw)
  To: tim.gardner@canonical.com
  Cc: Tom Herbert, Eric Dumazet, Stephen Hemminger, Peter Lieven,
	davem@davemloft.net, netdev@vger.kernel.org
In-Reply-To: <4C0ED931.6030402@canonical.com>

Tim Gardner wrote:
> On 06/08/2010 05:00 PM, Tom Herbert wrote:
>> How about only checking against dev->num_rx_queues when that value is
>> greater than one.  Since bonding device is calling alloc_netdev, it is
>> not going to set the queue mapping, but dev->num_rx_queues will be one
>> in that case (this handles any intermediate driver that does do
>> multiple queues).
>>
>> diff --git a/net/core/dev.c b/net/core/dev.c
>> index 6f330ce..30ab66d 100644
>> --- a/net/core/dev.c
>> +++ b/net/core/dev.c
>> @@ -2270,7 +2270,7 @@ static int get_rps_cpu(struct net_device *dev,
>> struct sk_buff *skb,
>>                  u16 v16[2];
>>          } ports;
>>
>> -       if (skb_rx_queue_recorded(skb)) {
>> +       if (skb_rx_queue_recorded(skb)&&  dev->num_rx_queues>  1) {
>>                  u16 index = skb_get_rx_queue(skb);
>>                  if (unlikely(index>= dev->num_rx_queues)) {
>>                          if (net_ratelimit()) {
>>


Problem with this is it doesn't address mis-aligned num_rx_queues.  For example 
with the bonding driver defaulting to 16 queues now. We could end up with a base 
driver with 16+ queues and a bond with 16.  Then we have the same issue again.

  eth0  -------> bond / bridge ---------> vlan.id
(nbrxq=64)      (nbrxq=16)              (nbrxq=X)

>>
>> On Tue, Jun 8, 2010 at 7:18 AM, Eric Dumazet<eric.dumazet@gmail.com>  wrote:
>>> Le lundi 07 juin 2010 à 15:30 -0700, John Fastabend a écrit :
>>>
>>>> There is always a possibility that the underlying device sets the
>>>> queue_mapping to be greater then num_cpus.  Also I suspect the same
>>>> issue exists with bonding devices.  Maybe something like the following
>>>> is worth while? compile tested only,
>>>>
>>>> [PATCH] 8021q: vlan reassigns dev without check queue_mapping
>>>>
>>>> recv path reassigns skb->dev without sanity checking the
>>>> queue_mapping field.  This can result in the queue_mapping
>>>> field being set incorrectly if the new dev supports less
>>>> queues then the underlying device.
>>>>
>>>> This patch just resets the queue_mapping to 0 which should
>>>> resolve this issue?  Any thoughts?
>>>>
>>>> The same issue could happen on bonding devices as well.
>>>>
>>>> Signed-off-by: John Fastabend<john.r.fastabend@intel.com>
>>>> ---
>>>>
>>>>    net/8021q/vlan_core.c |    6 ++++++
>>>>    1 files changed, 6 insertions(+), 0 deletions(-)
>>>>
>>>> diff --git a/net/8021q/vlan_core.c b/net/8021q/vlan_core.c
>>>> index bd537fc..ad309f8 100644
>>>> --- a/net/8021q/vlan_core.c
>>>> +++ b/net/8021q/vlan_core.c
>>>> @@ -21,6 +21,9 @@ int __vlan_hwaccel_rx(struct sk_buff *skb, struct
>>>> vlan_group *grp,
>>>>        if (!skb->dev)
>>>>                goto drop;
>>>>
>>>> +     if (unlikely(skb->queue_mapping>= skb->dev->real_num_tx_queues))
>>>> +             skb_set_queue_mapping(skb, 0);
>>>> +
>>>>        return (polling ? netif_receive_skb(skb) : netif_rx(skb));
>>>>
>>>>    drop:
>>>> @@ -93,6 +96,9 @@ vlan_gro_common(struct napi_struct *napi, struct
>>>> vlan_group *grp,
>>>>        if (!skb->dev)
>>>>                goto drop;
>>>>
>>>> +     if (unlikely(skb->queue_mapping>= skb->dev->real_num_tx_queues))
>>>> +             skb_set_queue_mapping(skb, 0);
>>>> +
>>>>        for (p = napi->gro_list; p; p = p->next) {
>>>>                NAPI_GRO_CB(p)->same_flow =
>>>>                        p->dev == skb->dev&&  !compare_ether_header(
>>>> --
>>> Only a workaround, added in hot path in a otherwise 'good' driver
>>> (multiqueue enabled and ready)

Agreed thanks!

>>>
>>> eth0  ------->  bond / bridge --------->  vlan.id
>>> (nbtxq=8)      (ntxbq=1)        (nbtxq=X)
>>>
>>> X is capped to 1 because of bond/bridge, while bond has no "queue"
>>> (LLTX driver)
>>>
>>> Solutions :
>>>
>>> 1) queue_mapping could be silently tested in get_rps_cpu()...
>>>
>>> diff --git a/net/core/dev.c b/net/core/dev.c
>>> index 6f330ce..3a3f7f6 100644
>>> --- a/net/core/dev.c
>>> +++ b/net/core/dev.c
>>> @@ -2272,14 +2272,11 @@ static int get_rps_cpu(struct net_device *dev, struct sk_buff *skb,
>>>
>>>         if (skb_rx_queue_recorded(skb)) {
>>>                 u16 index = skb_get_rx_queue(skb);
>>> -               if (unlikely(index>= dev->num_rx_queues)) {
>>> -                       if (net_ratelimit()) {
>>> -                               pr_warning("%s received packet on queue "
>>> -                                       "%u, but number of RX queues is %u\n",
>>> -                                       dev->name, index, dev->num_rx_queues);
>>> -                       }
>>> -                       goto done;
>>> -               }
>>> +               if (WARN_ONCE(index>= dev->num_rx_queues,
>>> +                               KERN_WARNING "%s received packet on queue %u, "
>>> +                               "but number of RX queues is %u\n",
>>> +                               dev->name, index, dev->num_rx_queues))
>>> +                       index %= dev->num_rx_queues;
>>>                 rxqueue = dev->_rx + index;
>>>         } else
>>>                 rxqueue = dev->_rx;
>>>
>>>

Looks good to me.

>>>
>>> 2) bond/bridge should setup more queues, just in case.
>>>    We probably need to be able to make things more dynamic,
>>>    (propagate nbtxq between layers) but not for 2.6.35
>>>
>>>

The bonding driver is already multiq per Andy Gospodarek's patch commit bb1d912, 
but unless the bond and bridge devices use the max num_rx_queues of there 
underlying devices this could still go wrong.

The bonding driver would possibly need to increase num_rx_queues and 
num_tx_queues when a device is enslaved or be set to some maximum at init for 
this to work right.

>>>
>>> diff --git a/drivers/net/bonding/bond_main.c b/drivers/net/bonding/bond_main.c
>>> index 5e12462..ce813dd 100644
>>> --- a/drivers/net/bonding/bond_main.c
>>> +++ b/drivers/net/bonding/bond_main.c
>>> @@ -5012,8 +5012,8 @@ int bond_create(struct net *net, const char *name)
>>>
>>>         rtnl_lock();
>>>
>>> -       bond_dev = alloc_netdev(sizeof(struct bonding), name ? name : "",
>>> -                               bond_setup);
>>> +       bond_dev = alloc_netdev_mq(sizeof(struct bonding), name ? name : "",
>>> +                                  bond_setup, max(64, nr_cpu_ids));
>>>         if (!bond_dev) {
>>>                 pr_err("%s: eek! can't alloc netdev!\n", name);
>>>                 rtnl_unlock();
>>>
>>>
>>> --
>>> To unsubscribe from this list: send the line "unsubscribe netdev" in
>>> the body of a message to majordomo@vger.kernel.org
>>> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>>>
>> --
>> To unsubscribe from this list: send the line "unsubscribe netdev" in
>> the body of a message to majordomo@vger.kernel.org
>> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 
> Huh, so you guys are looking at the same issue (only my issue is RPS). 
> See http://marc.info/?l=linux-netdev&m=127603240621028&w=2. I'm in favor 
> of dropping the warning when no queues have been allocated.
> 
> How about this (see attached).

Prefer Eric's patch see first comment.

Thanks,
John

> 
> rtg
> 
> 


^ permalink raw reply

* Re: RFS seems to have incompatiblities with bridged vlans
From: Tom Herbert @ 2010-06-09  1:52 UTC (permalink / raw)
  To: John Fastabend
  Cc: tim.gardner@canonical.com, Eric Dumazet, Stephen Hemminger,
	Peter Lieven, davem@davemloft.net, netdev@vger.kernel.org
In-Reply-To: <4C0EE99B.8030300@intel.com>

>
> Problem with this is it doesn't address mis-aligned num_rx_queues.  For
> example with the bonding driver defaulting to 16 queues now. We could end up
> with a base driver with 16+ queues and a bond with 16.  Then we have the
> same issue again.
>
>  eth0  -------> bond / bridge ---------> vlan.id
> (nbrxq=64)      (nbrxq=16)              (nbrxq=X)
>

I don't think the intent is to solve this alignment problem here.  If
a driver allocates multiple queues, it should set the queue mapping
accordingly.  If this isn't the case, the meaning of queue mapping is
ambiguous (which driver is it relative to?), and using the value for
rps_cpus is probably not going to work well-- hacking the
queue-mapping to be a legal value in get_rps_cpus doesn't seem like
the right answer.

>>>
>>> On Tue, Jun 8, 2010 at 7:18 AM, Eric Dumazet<eric.dumazet@gmail.com>
>>>  wrote:
>>>>
>>>> Le lundi 07 juin 2010 à 15:30 -0700, John Fastabend a écrit :
>>>>
>>>>> There is always a possibility that the underlying device sets the
>>>>> queue_mapping to be greater then num_cpus.  Also I suspect the same
>>>>> issue exists with bonding devices.  Maybe something like the following
>>>>> is worth while? compile tested only,
>>>>>
>>>>> [PATCH] 8021q: vlan reassigns dev without check queue_mapping
>>>>>
>>>>> recv path reassigns skb->dev without sanity checking the
>>>>> queue_mapping field.  This can result in the queue_mapping
>>>>> field being set incorrectly if the new dev supports less
>>>>> queues then the underlying device.
>>>>>
>>>>> This patch just resets the queue_mapping to 0 which should
>>>>> resolve this issue?  Any thoughts?
>>>>>
>>>>> The same issue could happen on bonding devices as well.
>>>>>
>>>>> Signed-off-by: John Fastabend<john.r.fastabend@intel.com>
>>>>> ---
>>>>>
>>>>>   net/8021q/vlan_core.c |    6 ++++++
>>>>>   1 files changed, 6 insertions(+), 0 deletions(-)
>>>>>
>>>>> diff --git a/net/8021q/vlan_core.c b/net/8021q/vlan_core.c
>>>>> index bd537fc..ad309f8 100644
>>>>> --- a/net/8021q/vlan_core.c
>>>>> +++ b/net/8021q/vlan_core.c
>>>>> @@ -21,6 +21,9 @@ int __vlan_hwaccel_rx(struct sk_buff *skb, struct
>>>>> vlan_group *grp,
>>>>>       if (!skb->dev)
>>>>>               goto drop;
>>>>>
>>>>> +     if (unlikely(skb->queue_mapping>= skb->dev->real_num_tx_queues))
>>>>> +             skb_set_queue_mapping(skb, 0);
>>>>> +
>>>>>       return (polling ? netif_receive_skb(skb) : netif_rx(skb));
>>>>>
>>>>>   drop:
>>>>> @@ -93,6 +96,9 @@ vlan_gro_common(struct napi_struct *napi, struct
>>>>> vlan_group *grp,
>>>>>       if (!skb->dev)
>>>>>               goto drop;
>>>>>
>>>>> +     if (unlikely(skb->queue_mapping>= skb->dev->real_num_tx_queues))
>>>>> +             skb_set_queue_mapping(skb, 0);
>>>>> +
>>>>>       for (p = napi->gro_list; p; p = p->next) {
>>>>>               NAPI_GRO_CB(p)->same_flow =
>>>>>                       p->dev == skb->dev&&  !compare_ether_header(
>>>>> --
>>>>
>>>> Only a workaround, added in hot path in a otherwise 'good' driver
>>>> (multiqueue enabled and ready)
>
> Agreed thanks!
>
>>>>
>>>> eth0  ------->  bond / bridge --------->  vlan.id
>>>> (nbtxq=8)      (ntxbq=1)        (nbtxq=X)
>>>>
>>>> X is capped to 1 because of bond/bridge, while bond has no "queue"
>>>> (LLTX driver)
>>>>
>>>> Solutions :
>>>>
>>>> 1) queue_mapping could be silently tested in get_rps_cpu()...
>>>>
>>>> diff --git a/net/core/dev.c b/net/core/dev.c
>>>> index 6f330ce..3a3f7f6 100644
>>>> --- a/net/core/dev.c
>>>> +++ b/net/core/dev.c
>>>> @@ -2272,14 +2272,11 @@ static int get_rps_cpu(struct net_device *dev,
>>>> struct sk_buff *skb,
>>>>
>>>>        if (skb_rx_queue_recorded(skb)) {
>>>>                u16 index = skb_get_rx_queue(skb);
>>>> -               if (unlikely(index>= dev->num_rx_queues)) {
>>>> -                       if (net_ratelimit()) {
>>>> -                               pr_warning("%s received packet on queue
>>>> "
>>>> -                                       "%u, but number of RX queues is
>>>> %u\n",
>>>> -                                       dev->name, index,
>>>> dev->num_rx_queues);
>>>> -                       }
>>>> -                       goto done;
>>>> -               }
>>>> +               if (WARN_ONCE(index>= dev->num_rx_queues,
>>>> +                               KERN_WARNING "%s received packet on
>>>> queue %u, "
>>>> +                               "but number of RX queues is %u\n",
>>>> +                               dev->name, index, dev->num_rx_queues))
>>>> +                       index %= dev->num_rx_queues;
>>>>                rxqueue = dev->_rx + index;
>>>>        } else
>>>>                rxqueue = dev->_rx;
>>>>
>>>>
>
> Looks good to me.
>
>>>>
>>>> 2) bond/bridge should setup more queues, just in case.
>>>>   We probably need to be able to make things more dynamic,
>>>>   (propagate nbtxq between layers) but not for 2.6.35
>>>>
>>>>
>
> The bonding driver is already multiq per Andy Gospodarek's patch commit
> bb1d912, but unless the bond and bridge devices use the max num_rx_queues of
> there underlying devices this could still go wrong.
>
> The bonding driver would possibly need to increase num_rx_queues and
> num_tx_queues when a device is enslaved or be set to some maximum at init
> for this to work right.
>
>>>>
>>>> diff --git a/drivers/net/bonding/bond_main.c
>>>> b/drivers/net/bonding/bond_main.c
>>>> index 5e12462..ce813dd 100644
>>>> --- a/drivers/net/bonding/bond_main.c
>>>> +++ b/drivers/net/bonding/bond_main.c
>>>> @@ -5012,8 +5012,8 @@ int bond_create(struct net *net, const char *name)
>>>>
>>>>        rtnl_lock();
>>>>
>>>> -       bond_dev = alloc_netdev(sizeof(struct bonding), name ? name :
>>>> "",
>>>> -                               bond_setup);
>>>> +       bond_dev = alloc_netdev_mq(sizeof(struct bonding), name ? name :
>>>> "",
>>>> +                                  bond_setup, max(64, nr_cpu_ids));
>>>>        if (!bond_dev) {
>>>>                pr_err("%s: eek! can't alloc netdev!\n", name);
>>>>                rtnl_unlock();
>>>>
>>>>
>>>> --
>>>> To unsubscribe from this list: send the line "unsubscribe netdev" in
>>>> the body of a message to majordomo@vger.kernel.org
>>>> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>>>>
>>> --
>>> To unsubscribe from this list: send the line "unsubscribe netdev" in
>>> the body of a message to majordomo@vger.kernel.org
>>> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>>
>> Huh, so you guys are looking at the same issue (only my issue is RPS). See
>> http://marc.info/?l=linux-netdev&m=127603240621028&w=2. I'm in favor of
>> dropping the warning when no queues have been allocated.
>>
>> How about this (see attached).
>
> Prefer Eric's patch see first comment.
>
> Thanks,
> John
>
>>
>> rtg
>>
>>
>
>

^ permalink raw reply

* Re: 2.6.35-rc2-git2: Reported regressions from 2.6.34
From: Linus Torvalds @ 2010-06-09  1:53 UTC (permalink / raw)
  To: Rafael J. Wysocki, Carl Worth, Eric Anholt, Venkatesh Pallipadi,
	Jens Axboe
  Cc: DRI, Linux SCSI List, Network Development, Linux Wireless List,
	Linux Kernel Mailing List, Linux ACPI, Andrew Morton,
	Kernel Testers List, Linux PM List, Maciej Rutecki
In-Reply-To: <Rlf_Qjov7bG.A.a8F.I8rDMB@chimera>



[ Added lots of cc's to direct specific people to look at the regressions 
  that may or may not be theirs... ]

On Wed, 9 Jun 2010, Rafael J. Wysocki wrote:
>
>  * Quite a few of the already reported regressions may be related to the bug
>    fixed by 386f40c86d6c8d5b717ef20620af1a750d0dacb4 (Revert "tty: fix a little
>    bug in scrup, vt.c"), so reporters please retest with this commit applied.]

>From a quick look, most of them look unrelated to that unfortunate bug. 
It's hard to tell for sure, of course (memory corruption can do pretty 
much anything), but I'm not seeing the 07200720.. pattern at least.

And some of them do seem to be bisected to likely culprits and/or have 
patches that are claimed to have fixed them.

> Bug-Entry	: http://bugzilla.kernel.org/show_bug.cgi?id=16163
> Subject	: [2.6.35-rc1 Regression] i915: Commit cfecde causes VGA to stay off
> Submitter	: David John <davidjon@xenontk.org>
> Date		: 2010-06-02 12:52 (7 days old)
> Message-ID	: <4C065423.3000202@xenontk.org>
> References	: http://marc.info/?l=linux-kernel&m=127548313828613&w=2

That has a "reverting the commit fixes it", and a confirmation from Nick 
Bowler.

Eric, Carl: should I just revert that commit? Or do you have a fix?

> Bug-Entry	: http://bugzilla.kernel.org/show_bug.cgi?id=16145
> Subject	: Unable to boot after "ACPI: Don't let acpi_pad needlessly mark TSC unstable"
> Submitter	: Tom Gundersen <teg@jklm.no>
> Date		: 2010-06-07 13:11 (2 days old)
> Handled-By	: Venkatesh Pallipadi <venki@google.com>

Hmm. This does seem to be a properly bisected commit, but at the same time 
it looks from the bugzilla like it's just pure luck on that machine that 
the acpi_pad driver happened to mark TSC unstable - so while the commit 
bisected is the real one, it's not the "deeper reason" for the problem.

Venki, any updates?

> Bug-Entry	: http://bugzilla.kernel.org/show_bug.cgi?id=16129
> Subject	: BUG: using smp_processor_id() in preemptible [00000000] code: jbd2/sda2
> Submitter	: Jan Kreuzer <kontrollator@gmx.de>
> Date		: 2010-06-05 06:15 (4 days old)

This seems to have been introduced by

	commit 7cbaef9c83e58bbd4bdd534b09052b6c5ec457d5
	Author: Ingo Molnar <mingo@elte.hu>
	Date:   Sat Nov 8 17:05:38 2008 +0100

	    sched: optimize sched_clock() a bit
    
	    sched_clock() uses cycles_2_ns() needlessly - which is an irq-disabling
	    variant of __cycles_2_ns().
    
	    Most of the time sched_clock() is called with irqs disabled already.
	    The few places that call it with irqs enabled need to be updated.
    
	    Signed-off-by: Ingo Molnar <mingo@elte.hu>

and this seems to be one of those calling cases that need to be updated.

Ingo? The call trace is:

	BUG: using smp_processor_id() in preemptible [00000000] code: jbd2/sda2-8/337
	caller is native_sched_clock+0x3c/0x68
	Pid: 337, comm: jbd2/sda2-8 Not tainted 2.6.35-rc1jan+ #4
	Call Trace:
	[<ffffffff812362c5>] debug_smp_processor_id+0xc9/0xe4
	[<ffffffff8101059d>] native_sched_clock+0x3c/0x68
	[<ffffffff8101043d>] sched_clock+0x9/0xd
	[<ffffffff81212d7a>] blk_rq_init+0x97/0xa3
	[<ffffffff81214d71>] get_request+0x1c4/0x2d0
	[<ffffffff81214ea6>] get_request_wait+0x29/0x1a6
	[<ffffffff81215537>] __make_request+0x338/0x45b
	[<ffffffff812147c2>] generic_make_request+0x2bb/0x330
	[<ffffffff81214909>] submit_bio+0xd2/0xef
	[<ffffffff811413cb>] submit_bh+0xf4/0x116
	[<ffffffff81144853>] block_write_full_page_endio+0x89/0x96
	[<ffffffff81144875>] block_write_full_page+0x15/0x17
	[<ffffffff8119b00a>] ext4_writepage+0x356/0x36b
	[<ffffffff810e1f91>] __writepage+0x1a/0x39
	[<ffffffff810e32a6>] write_cache_pages+0x20d/0x346
	[<ffffffff810e3406>] generic_writepages+0x27/0x29
	[<ffffffff811ca279>] journal_submit_data_buffers+0x110/0x17d
	[<ffffffff811ca986>] jbd2_journal_commit_transaction+0x4cb/0x156d
	[<ffffffff811d0cba>] kjournald2+0x147/0x37a

(from the bugzilla thing)

> Bug-Entry	: http://bugzilla.kernel.org/show_bug.cgi?id=16122
> Subject	: 2.6.35-rc1: WARNING at fs/fs-writeback.c:1142 __mark_inode_dirty+0x103/0x170
> Submitter	: Larry Finger <Larry.Finger@lwfinger.net>
> Date		: 2010-06-04 13:18 (5 days old)

Jens?

> Bug-Entry	: http://bugzilla.kernel.org/show_bug.cgi?id=16120
> Subject	: Oops: 0000 [#1] SMP, unable to handle kernel NULL pointer dereference at (null)
> Submitter	: Alex Zhavnerchik <alex.vizor@gmail.com>
> Date		: 2010-06-04 09:25 (5 days old)
> Handled-By	: Eric Dumazet <eric.dumazet@gmail.com>

This one seems to have a patch in bugzilla.

> Bug-Entry	: http://bugzilla.kernel.org/show_bug.cgi?id=16104
> Subject	: Radeon KMS does not start after merge of the new PM-Code
> Submitter	: Jan Kreuzer <kontrollator@gmx.de>
> Date		: 2010-06-02 07:47 (7 days old)

This one also has a patch in Bugzilla, I think Airlie is just waiting to 
calm down his queue and already removed the dependency on the temperature 
code. DaveA?

> Bug-Entry     : http://bugzilla.kernel.org/show_bug.cgi?id=16161
> Subject       : [2.6.35-rc1 regression] sysfs: cannot create duplicate filename ... XVR-600 related?
> Submitter     : Mikael Pettersson <mikpe@it.uu.se>
> Date          : 2010-06-01 19:57 (8 days old)
> Message-ID    : <19461.26166.427857.612983@pilspetsen.it.uu.se>
> References    : http://marc.info/?l=linux-kernel&m=127542227511925&w=2
>
> Bug-Entry	: http://bugzilla.kernel.org/show_bug.cgi?id=16090
> Subject	: sysfs: cannot create duplicate filename
> Submitter	: Tobias <devnull@plzk.org>
> Date		: 2010-06-01 15:59 (8 days old)
> Handled-By	: Jesse Barnes <jbarnes@virtuousgeek.org>

These two look related. Both are related to that "slot" thing in sysfs 
PCI, but only one of them is marked as Jesse's. Jesse?

> Bug-Entry	: http://bugzilla.kernel.org/show_bug.cgi?id=16037
> Subject	: NULL Pointer dereference in __ir_input_register/budget_ci_attach
> Submitter	: Sean Finney <seanius@debian.org>
> Date		: 2010-05-23 19:52 (17 days old)

Perhaps related to commit 13c24497086418010bf4f76378bcae241d7f59cd?

David Härdeman, Mauro Carvalho Chehab added to cc.

				Linus

^ permalink raw reply

* Re: 2.6.35-rc2-git2: Reported regressions from 2.6.34
From: Mauro Carvalho Chehab @ 2010-06-09  2:26 UTC (permalink / raw)
  To: Linus Torvalds
  Cc: Rafael J. Wysocki, Carl Worth, Eric Anholt, Venkatesh Pallipadi,
	Jens Axboe, Dave Airlie, Jesse Barnes, Ingo Molnar,
	David Härdeman, Eric Dumazet, Linux Kernel Mailing List,
	Maciej Rutecki, Andrew Morton, Kernel Testers List,
	Network Development, Linux ACPI, Linux PM List, Linux SCSI List,
	Linux Wireless List, DRI
In-Reply-To: <alpine.LFD.2.00.1006081814240.4506@i5.linux-foundation.org>

Em 08-06-2010 22:53, Linus Torvalds escreveu:

> 
>> Bug-Entry	: http://bugzilla.kernel.org/show_bug.cgi?id=16037
>> Subject	: NULL Pointer dereference in __ir_input_register/budget_ci_attach
>> Submitter	: Sean Finney <seanius@debian.org>
>> Date		: 2010-05-23 19:52 (17 days old)
> 
> Perhaps related to commit 13c24497086418010bf4f76378bcae241d7f59cd?
> 
> David Härdeman, Mauro Carvalho Chehab added to cc.

This patch probably solves the issue:

http://git.kernel.org/?p=linux/kernel/git/torvalds/linux-2.6.git;a=commitdiff;h=84b14f181a36eea6591779156ef356f8d198bbfd

The patch were already applied upstream. I've already asked the reporter to test it, via BZ.

Cheers,
Mauro
--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

^ permalink raw reply

* Re: 2.6.35-rc2-git2: Reported regressions from 2.6.34
From: Carl Worth @ 2010-06-09  2:38 UTC (permalink / raw)
  To: Linus Torvalds, Rafael J. Wysocki, Eric Anholt,
	Venkatesh Pallipadi, Jens Axboe
  Cc: Linux Kernel Mailing List, Maciej Rutecki, Andrew Morton,
	Kernel Testers List, Network Development, Linux ACPI,
	Linux PM List, Linux SCSI List, Linux Wireless List, DRI
In-Reply-To: <alpine.LFD.2.00.1006081814240.4506-GpypE611fyS63QaFMGN2QEqCLAeBNdoH@public.gmane.org>

[-- Attachment #1: Type: text/plain, Size: 1165 bytes --]

On Tue, 8 Jun 2010 18:53:55 -0700 (PDT), Linus Torvalds <torvalds@linux-foundation.org> wrote:
> And some of them do seem to be bisected to likely culprits and/or have 
> patches that are claimed to have fixed them.
> 
> > Bug-Entry	: http://bugzilla.kernel.org/show_bug.cgi?id=16163
> > Subject	: [2.6.35-rc1 Regression] i915: Commit cfecde causes VGA to stay off
> > Submitter	: David John <davidjon-XRr60H37pjdAfugRpC6u6w@public.gmane.org>
> > Date		: 2010-06-02 12:52 (7 days old)
> > Message-ID	: <4C065423.3000202-XRr60H37pjdAfugRpC6u6w@public.gmane.org>
> > References	: http://marc.info/?l=linux-kernel&m=127548313828613&w=2
> 
> That has a "reverting the commit fixes it", and a confirmation from Nick 
> Bowler.
> 
> Eric, Carl: should I just revert that commit? Or do you have a fix?

I'm not aware of any real fix here. That commit isn't supposed to change
much, but it clearly unmasks some broken driver code.

So reverting it for now to hide the broken code from poor users does
seem a good plan to me, (unless Eric has any updates or alternate
suggestions).

-Carl

-- 
carl.d.worth-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org

[-- Attachment #2: Type: application/pgp-signature, Size: 189 bytes --]

^ permalink raw reply

* RE: [PATCH] r8169: fix random mdio_write failures
From: hayeswang @ 2010-06-09  2:47 UTC (permalink / raw)
  To: 'Timo Teräs', 'Francois Romieu'; +Cc: netdev, davem
In-Reply-To: <4C0DDDCC.6010500@iki.fi>

I think the major point is that it needs a delay (20 us) after both read and
write completed indiacation before next command.
It needs almost 100 us for the indication to be completed, so our engineer
suggests us to check the indication per 100 us.
However, it is  ok to reduce the timer from 100 us to 25 us for checking .
 
Best Regards,
Hayes


-----Original Message-----
From: Timo Teräs [mailto:timo.teras@gmail.com] On Behalf Of Timo Teras
Sent: Tuesday, June 08, 2010 2:06 PM
To: Francois Romieu
Cc: Hayeswang; netdev@vger.kernel.org; davem@davemloft.net
Subject: Re: [PATCH] r8169: fix random mdio_write failures

On 06/08/2010 12:51 AM, Francois Romieu wrote:
> hayeswang <hayeswang@realtek.com> :
>> Our hardware engineer suggests that check the completed indication 
>> per 100 micro seconds. And it needs 20 micro seconds delay after the 
>> completed indication for the next command.
> 
> Should we do the same for mdio_read as well (100 us per iteration + an 
> extra 20 us) ?

Well, doing 100us per iteration will increase the latency that the code
notices "write complete" which slows down things. It'll also slightly
decrease bus traffic which is good. But I'd be just fine with 25us per
iteration. It sounds unlikely that polling the status register would slow
down the actual write operation (if that is the case then 100us would be
desirable).

Changing my 25us to 20us would good. The original 25us was just a guess.
The comment should be probably also updated that those delays are from
realtek hw specs then.

Would you like me to send a patch?

- Timo




------Please consider the environment before printing this e-mail. 




^ permalink raw reply

* #Very Important#
From: Mr. Jiang Jianmin @ 2010-06-09 11:36 UTC (permalink / raw)


Good Day,
 
I have a secured business proposal of $28,272,000.00. Only contact me via my private email(jiang_jianmin2011@9.cn) if interested.
 
Mr Jiang Jianmin.

^ permalink raw reply

* Re: [PATCH 1/2] Export firmware assigned labels of network devices to sysfs
From: Matt Domsch @ 2010-06-09  4:17 UTC (permalink / raw)
  To: Greg KH
  Cc: K, Narendra, netdev@vger.kernel.org,
	linux-hotplug@vger.kernel.org, linux-pci@vger.kernel.org,
	Hargrave, Jordan, Rose, Charles, Nijhawan, Vijay
In-Reply-To: <20100529045140.GA22563@mdomsch-pws380.aus.amer.dell.com>

On Fri, May 28, 2010 at 11:51:40PM -0500, Domsch, Matt wrote:
> On Fri, May 28, 2010 at 05:27:45PM -0500, Greg KH wrote:
> > Care to post that ECN publically?  And no, the Linux Foundation does not
> > have a PCI-SIG membership, the PCI-SIG keeps forbidding it.  Other
> > operating systems are allowed to join but not Linux.  Strange but
> > true...
> 
> I'm looking into it, and should know more next week.

I'm advised that I cannot post the ECN publically, due to it being an
in-progress work item of a SIG working group, and therefore falls
under the confidentiality rules that SIG members agree to.  Members of
the PCI SIG have access, which unfortunately is not everyone.

-- 
Matt Domsch
Technology Strategist
Dell | Office of the CTO

^ permalink raw reply

* Re: RFS seems to have incompatiblities with bridged vlans
From: Eric Dumazet @ 2010-06-09  4:42 UTC (permalink / raw)
  To: John Fastabend
  Cc: tim.gardner@canonical.com, Tom Herbert, Stephen Hemminger,
	Peter Lieven, davem@davemloft.net, netdev@vger.kernel.org
In-Reply-To: <4C0EE99B.8030300@intel.com>

Le mardi 08 juin 2010 à 18:08 -0700, John Fastabend a écrit :


> 
> The bonding driver is already multiq per Andy Gospodarek's patch commit bb1d912, 
> but unless the bond and bridge devices use the max num_rx_queues of there 
> underlying devices this could still go wrong.
> 

This patch is in net-next-2.6, it is not scheduled for 2.6.35

We need some fix for 2.6.35

> The bonding driver would possibly need to increase num_rx_queues and 
> num_tx_queues when a device is enslaved or be set to some maximum at init for 
> this to work right.

Thats a bit complex and definitly not 2.6.35 material.




^ permalink raw reply

* [PATCH net-next-2.6] [PPPOE] cleanup: remove pppoe_xmit() declaration.
From: Rami Rosen @ 2010-06-09  5:07 UTC (permalink / raw)
  To: davem, netdev

[-- Attachment #1: Type: text/plain, Size: 204 bytes --]

Hi,
There is no need for pppoe_xmit() forward declaration in
drivers/net/pppoe.c. This patch removes this  pppoe_xmit() declaration.

Regards,
Rami Rosen



Signed-off-by: Rami Rosen <ramirose@gmail.com>

[-- Attachment #2: patch.txt --]
[-- Type: text/plain, Size: 443 bytes --]

diff --git a/drivers/net/pppoe.c b/drivers/net/pppoe.c
index 7ebb8e8..344ef33 100644
--- a/drivers/net/pppoe.c
+++ b/drivers/net/pppoe.c
@@ -89,7 +89,6 @@
 #define PPPOE_HASH_SIZE (1 << PPPOE_HASH_BITS)
 #define PPPOE_HASH_MASK	(PPPOE_HASH_SIZE - 1)
 
-static int pppoe_xmit(struct ppp_channel *chan, struct sk_buff *skb);
 static int __pppoe_xmit(struct sock *sk, struct sk_buff *skb);
 
 static const struct proto_ops pppoe_ops;

^ permalink raw reply related

* [PATCH] r8169: fix mdio_read and update mdio_write according to hw specs
From: Timo Teräs @ 2010-06-09  5:22 UTC (permalink / raw)
  To: netdev; +Cc: Timo Teräs, Francois Romieu, Hayeswang
In-Reply-To: <BD599B881C1D407DA7BC93A9244650B0@realtek.com.tw>

Realtek confirmed that a 20us delay is needed after mdio_read and
mdio_write operations. Reduce the delay in mdio_write, and add it
to mdio_read too. Also add a comment that the 20us is from hw specs.

Signed-off-by: Timo Teräs <timo.teras@iki.fi>
Cc: Francois Romieu <romieu@fr.zoreil.com>
Cc: Hayeswang <hayeswang@realtek.com>
---
 drivers/net/r8169.c |   12 +++++++++---
 1 files changed, 9 insertions(+), 3 deletions(-)

diff --git a/drivers/net/r8169.c b/drivers/net/r8169.c
index 03a8318..96b6cfb 100644
--- a/drivers/net/r8169.c
+++ b/drivers/net/r8169.c
@@ -560,10 +560,10 @@ static void mdio_write(void __iomem *ioaddr, int reg_addr, int value)
 		udelay(25);
 	}
 	/*
-	 * Some configurations require a small delay even after the write
-	 * completed indication or the next write might fail.
+	 * According to hardware specs a 20us delay is required after write
+	 * complete indication, but before sending next command.
 	 */
-	udelay(25);
+	udelay(20);
 }
 
 static int mdio_read(void __iomem *ioaddr, int reg_addr)
@@ -583,6 +583,12 @@ static int mdio_read(void __iomem *ioaddr, int reg_addr)
 		}
 		udelay(25);
 	}
+	/*
+	 * According to hardware specs a 20us delay is required after read
+	 * complete indication, but before sending next command.
+	 */
+	udelay(20);
+
 	return value;
 }
 
-- 
1.7.0.4


^ permalink raw reply related

* Re: 2.6.35-rc2-git2: Reported regressions from 2.6.34
From: Ingo Molnar @ 2010-06-09  5:34 UTC (permalink / raw)
  To: Linus Torvalds, Jens Axboe, Peter Zijlstra
  Cc: Rafael J. Wysocki, Carl Worth, Eric Anholt, Venkatesh Pallipadi,
	Jens Axboe, Dave Airlie, Jesse Barnes, David H?rdeman,
	Mauro Carvalho Chehab, Eric Dumazet, Linux Kernel Mailing List,
	Maciej Rutecki, Andrew Morton, Kernel Testers List,
	Network Development, Linux ACPI, Linux PM List, Linux SCSI List,
	Linux Wireless List, DRI
In-Reply-To: <alpine.LFD.2.00.1006081814240.4506@i5.linux-foundation.org>


* Linus Torvalds <torvalds@linux-foundation.org> wrote:

> > Bug-Entry	: http://bugzilla.kernel.org/show_bug.cgi?id=16129
> > Subject	: BUG: using smp_processor_id() in preemptible [00000000] code: jbd2/sda2
> > Submitter	: Jan Kreuzer <kontrollator@gmx.de>
> > Date		: 2010-06-05 06:15 (4 days old)
> 
> This seems to have been introduced by
> 
> 	commit 7cbaef9c83e58bbd4bdd534b09052b6c5ec457d5
> 	Author: Ingo Molnar <mingo@elte.hu>
> 	Date:   Sat Nov 8 17:05:38 2008 +0100
> 
> 	    sched: optimize sched_clock() a bit
>     
> 	    sched_clock() uses cycles_2_ns() needlessly - which is an irq-disabling
> 	    variant of __cycles_2_ns().
>     
> 	    Most of the time sched_clock() is called with irqs disabled already.
> 	    The few places that call it with irqs enabled need to be updated.
>     
> 	    Signed-off-by: Ingo Molnar <mingo@elte.hu>
> 
> and this seems to be one of those calling cases that need to be updated.

That's a commit from 2008.

> Ingo? The call trace is:
> 
> 	BUG: using smp_processor_id() in preemptible [00000000] code: jbd2/sda2-8/337
> 	caller is native_sched_clock+0x3c/0x68
> 	Pid: 337, comm: jbd2/sda2-8 Not tainted 2.6.35-rc1jan+ #4
> 	Call Trace:
> 	[<ffffffff812362c5>] debug_smp_processor_id+0xc9/0xe4
> 	[<ffffffff8101059d>] native_sched_clock+0x3c/0x68
> 	[<ffffffff8101043d>] sched_clock+0x9/0xd
> 	[<ffffffff81212d7a>] blk_rq_init+0x97/0xa3
> 	[<ffffffff81214d71>] get_request+0x1c4/0x2d0
> 	[<ffffffff81214ea6>] get_request_wait+0x29/0x1a6
> 	[<ffffffff81215537>] __make_request+0x338/0x45b
> 	[<ffffffff812147c2>] generic_make_request+0x2bb/0x330
> 	[<ffffffff81214909>] submit_bio+0xd2/0xef
> 	[<ffffffff811413cb>] submit_bh+0xf4/0x116
> 	[<ffffffff81144853>] block_write_full_page_endio+0x89/0x96
> 	[<ffffffff81144875>] block_write_full_page+0x15/0x17
> 	[<ffffffff8119b00a>] ext4_writepage+0x356/0x36b
> 	[<ffffffff810e1f91>] __writepage+0x1a/0x39
> 	[<ffffffff810e32a6>] write_cache_pages+0x20d/0x346
> 	[<ffffffff810e3406>] generic_writepages+0x27/0x29
> 	[<ffffffff811ca279>] journal_submit_data_buffers+0x110/0x17d
> 	[<ffffffff811ca986>] jbd2_journal_commit_transaction+0x4cb/0x156d
> 	[<ffffffff811d0cba>] kjournald2+0x147/0x37a
> 
> (from the bugzilla thing)

The warning was introduced by this fresh commit (and a followup commit) merged 
in the .35 merge window:

 | commit 9195291e5f05e01d67f9a09c756b8aca8f009089
 | Author: Divyesh Shah <dpshah@google.com>
 | Date:   Thu Apr 1 15:01:41 2010 -0700
 | 
 |     blkio: Increment the blkio cgroup stats for real now

IIRC Jens posted a fix for the regression. Jens, what's the status of that?

As the code there started using a raw sched_clock() call for block statistics 
purposes, which was a poorly thought out (and buggy) approach:

 - it takes timestamps on different cpus and then compares then, but doesnt 
   consider that sched_clock() is not comparable between CPUs without extra 
   care

 - it doesnt consider the possibility for the sched_clock() result going
   backwards on certain platforms (such as x86)

 - it doesnt consider preemptability

(There's work ongoing to add a clock variant that can be used for such 
purposes, but that's .36 fodder.)

Thanks,

	Ingo

^ permalink raw reply

* pkt_sched: gen_estimator: more fuel for Jarek and Changli
From: Eric Dumazet @ 2010-06-09  6:13 UTC (permalink / raw)
  To: Jarek Poplawski
  Cc: Changli Gao, David Miller, netdev, Stephen Hemminger,
	Patrick McHardy
In-Reply-To: <1276030354.2439.8.camel@edumazet-laptop>


With un-modified kernel, I ran following scripts on my machine

taskset 01 sh -c "while :;do iptables  -I INPUT -i eth0 -j RATEEST --rateest-name eth0 --rateest-interval 250ms --rateest-ewmalog 1000ms; done" &
taskset 02 sh -c "while :;do iptables  -F INPUT; done" &
taskset 02 sh -c "while :;do tc qdisc del dev eth0 root 2>/dev/null;done" &
taskset 08 sh -c "while :;do tc qdisc add dev eth0 root handle 1: est 250msec 1sec cbq avpkt 1000 rate 1000Mbit bandwidth 1000Mbit 2>/dev/null;done" &


I got following oops in about 10 seconds, and my machine had to be
rebooted, rtnl being locked forever, so many commands block hard in
rtnl_lock()

root      6016  0.0  0.0  2040  536 pts/0    D    07:14   0:00 tc qdisc del dev eth0 root
root      6021  0.0  0.0  2040  676 pts/0    D    07:14   0:00 tc qdisc add dev eth0 root handle 1: est 250msec 1sec cbq avpkt 1000 rate 1
root     19358  0.0  0.0  1752  252 ?        D    07:45   0:00 ip -o link ls dev eth0

[  753.892107] BUG: unable to handle kernel NULL pointer dereference at (null)
[  753.892132] IP: [<c116b6c8>] rb_insert_color+0xc6/0xd0
[  753.892156] *pdpt = 0000000032827001 *pde = 0000000000000000 
[  753.892177] Oops: 0002 [#1] PREEMPT SMP 
[  753.892196] last sysfs file: /sys/devices/pci0000:00/0000:00:1e.0/0000:01:04.6/class
[  753.892218] Modules linked in: xt_RATEEST iptable_filter ip_tables x_tables ipmi_devintf ipmi_si ipmi_msghandler ipv6 dm_mod button battery ac ehci_hcd uhci_hcd tg3 libphy bnx2x crc32c libcrc32c mdio [last unloaded: x_tables]
[  753.892314] 
[  753.892321] Pid: 5951, comm: tc Not tainted 2.6.35-rc1-00208-g50e3a9a #68 /ProLiant BL460c G6
[  753.892341] EIP: 0060:[<c116b6c8>] EFLAGS: 00010202 CPU: 3
[  753.892356] EIP is at rb_insert_color+0xc6/0xd0
[  753.892368] EAX: 00000000 EBX: f34c1750 ECX: f34c1750 EDX: c1b5a1bc
[  753.892384] ESI: 00000001 EDI: f34c1ae0 EBP: f34a0c0c ESP: f34a0bf8
[  753.892399]  DS: 007b ES: 007b FS: 00d8 GS: 0033 SS: 0068
[  753.892413] Process tc (pid: 5951, ti=f34a0000 task=f43f2ac0 task.ti=f34a0000)
[  753.892430] Stack:
[  753.892465]  c1292899 c1b5a1bc f34c1aa8 f3ae47f4 f36baf78 f34a0c34 c1292a66 f36baf5c
[  753.892524] <0> 00000098 d8d43110 f36baf2c 00000000 f36baf00 f34a0ca0 00000000 f34a0c6c
[  753.892598] <0> c12aa80c d8d4310c c16ba5a0 00000000 f4160000 c1561fa0 f43f2a00 00000000
[  753.892681] Call Trace:
[  753.892707]  [<c1292899>] ? gen_new_estimator+0x55/0x247
[  753.892736]  [<c1292a66>] ? gen_new_estimator+0x222/0x247
[  753.892765]  [<c12aa80c>] ? qdisc_create+0x1e4/0x273
[  753.892793]  [<c12aabd8>] ? tc_modify_qdisc+0x33d/0x3be
[  753.892822]  [<c12aa89b>] ? tc_modify_qdisc+0x0/0x3be
[  753.892850]  [<c12a1c10>] ? rtnetlink_rcv_msg+0x197/0x1a6
[  753.892880]  [<c132d454>] ? mutex_lock_nested+0x26e/0x288
[  753.892909]  [<c12a1a79>] ? rtnetlink_rcv_msg+0x0/0x1a6
[  753.892938]  [<c12c74ec>] ? netlink_rcv_skb+0x32/0x73
[  753.892966]  [<c12a1a00>] ? rtnetlink_rcv+0x1b/0x22
[  753.892993]  [<c12c7045>] ? netlink_unicast+0x1b3/0x214
[  753.893021]  [<c12c72dc>] ? netlink_sendmsg+0x236/0x243
[  753.893050]  [<c1288262>] ? sock_sendmsg+0xc0/0xdb
[  753.893080]  [<c109f15a>] ? might_fault+0x36/0x70
[  753.893107]  [<c109f15a>] ? might_fault+0x36/0x70
[  753.893134]  [<c109f15a>] ? might_fault+0x36/0x70
[  753.893161]  [<c116f330>] ? _copy_from_user+0x39/0x4d
[  753.893189]  [<c1290a91>] ? verify_iovec+0x3e/0x6d
[  753.893217]  [<c1289b89>] ? sys_sendmsg+0x13f/0x18c
[  753.893244]  [<c12882cd>] ? sockfd_lookup_light+0x19/0x4b
[  753.893274]  [<c1094dea>] ? __lru_cache_add+0x64/0x7b
[  753.893302]  [<c102a200>] ? get_parent_ip+0x9/0x31
[  753.893332]  [<c105a62b>] ? lock_release_non_nested+0x88/0x245
[  753.893362]  [<c109f15a>] ? might_fault+0x36/0x70
[  753.893389]  [<c109f15a>] ? might_fault+0x36/0x70
[  753.893415]  [<c109f15a>] ? might_fault+0x36/0x70
[  753.893443]  [<c1289f62>] ? sys_socketcall+0x163/0x1a3
[  753.893472]  [<c116edd0>] ? trace_hardirqs_on_thunk+0xc/0x10
[  753.893501]  [<c100278c>] ? sysenter_do_call+0x12/0x32
[  753.893537] Code: cb 83 0b 01 89 f0 83 26 fe 8b 55 f0 e8 8e fe ff ff 8b 1f 83 e3 fc 74 0e 8b 33 f7 c6 01 00 00 00 0f 84 61 ff ff ff 8b 55 f0 8b 02 <83> 08 01 58 5a 5b 5e 5f 5d c3 55 89 e5 57 56 89 d6 53 89 c3 83 
[  753.893763] EIP: [<c116b6c8>] rb_insert_color+0xc6/0xd0 SS:ESP 0068:f34a0bf8
[  753.893799] CR2: 0000000000000000
[  753.894062] ---[ end trace da6bae989b9be023 ]---



Triggering the other bug is more difficult :

est_timer() should be interrupted 
(by hard irqs for example), right before spin_lock(e->stats_lock);

Then a caller of gen_kill_estimator() might freed stats_lock and 
est_timer() reference a freed spinlock.

This can be simulated with following patch, to inject a 100 ms delay.

diff --git a/net/core/gen_estimator.c b/net/core/gen_estimator.c
index cf8e703..55ba060 100644
--- a/net/core/gen_estimator.c
+++ b/net/core/gen_estimator.c
@@ -120,6 +120,8 @@ static void est_timer(unsigned long arg)
                u32 npackets;
                u32 rate;
 
+               for (rate = 0; rate < 100; rate++)
+                       udelay(1000);
                spin_lock(e->stats_lock);
                read_lock(&est_lock);
                if (e->bstats == NULL)

My machine crash almost instantly in spin_lock(e->stats_lock)

I'll post v3 of the patch, with updated Changelog



^ permalink raw reply related

* Re: [PATCH] r8169: fix mdio_read and update mdio_write according to hw specs
From: Francois Romieu @ 2010-06-09  6:18 UTC (permalink / raw)
  To: Timo Teräs; +Cc: netdev, Hayeswang
In-Reply-To: <1276060930-15697-1-git-send-email-timo.teras@iki.fi>

Timo Teräs <timo.teras@iki.fi> :
> Realtek confirmed that a 20us delay is needed after mdio_read and
> mdio_write operations. Reduce the delay in mdio_write, and add it
> to mdio_read too. Also add a comment that the 20us is from hw specs.
> 
> Signed-off-by: Timo Teräs <timo.teras@iki.fi>

Acked-off-by: Francois Romieu <romieu@fr.zoreil.com>

^ permalink raw reply

* Re: 2.6.35-rc2-git2: Reported regressions from 2.6.34
From: Eric Dumazet @ 2010-06-09  6:36 UTC (permalink / raw)
  To: Linus Torvalds
  Cc: Rafael J. Wysocki, Carl Worth, Eric Anholt, Venkatesh Pallipadi,
	Jens Axboe, Dave Airlie, Jesse Barnes, Ingo Molnar,
	David Härdeman, Mauro Carvalho Chehab,
	Linux Kernel Mailing List, Maciej Rutecki, Andrew Morton,
	Kernel Testers List, Network Development, Linux ACPI,
	Linux PM List, Linux SCSI List, Linux Wireless List, DRI
In-Reply-To: <alpine.LFD.2.00.1006081814240.4506-GpypE611fyS63QaFMGN2QEqCLAeBNdoH@public.gmane.org>

Le mardi 08 juin 2010 à 18:53 -0700, Linus Torvalds a écrit :

> > Bug-Entry	: http://bugzilla.kernel.org/show_bug.cgi?id=16120
> > Subject	: Oops: 0000 [#1] SMP, unable to handle kernel NULL pointer dereference at (null)
> > Submitter	: Alex Zhavnerchik <alex.vizor-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
> > Date		: 2010-06-04 09:25 (5 days old)
> > Handled-By	: Eric Dumazet <eric.dumazet-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
> 
> This one seems to have a patch in bugzilla.

Yep, commit 035320d54758e21227987e3aae0d46e7a04f4ddc in David net-2.6
tree, i'll be included in its next pull request.

http://git.kernel.org/?p=linux/kernel/git/davem/net-2.6.git;a=commit;h=035320d54758e21227987e3aae0d46e7a04f4ddc

Thanks

^ permalink raw reply


This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox