* Re: [PATCH 2/2] NET: sungem, use spin_trylock_irqsave
From: David Miller @ 2009-07-14 2:12 UTC (permalink / raw)
To: jirislaby; +Cc: netdev, linux-kernel
In-Reply-To: <1247520220-31960-2-git-send-email-jirislaby@gmail.com>
Please DO NOT combine cleanups with real bug fixes into
a patch series.
Bug fixes go to one tree, the rest go to another tree, and that is
especially the case this late in the RC series.
A series of patches are a group of changes that are supposed to be
related and are expected to be applied all to the same tree. That is
not true of the patches you have posted here.
^ permalink raw reply
* Re: [PATCH] imwc3200: move iwmc3200 SDIO ids to sdio_ids.h
From: Marcel Holtmann @ 2009-07-14 2:14 UTC (permalink / raw)
To: Tomas Winkler
Cc: drzeus-list-p3sGCRWkH8CeZLLa646FqQ, netdev-u79uwXL29TY76Z2rM5mHXA,
linux-wireless-u79uwXL29TY76Z2rM5mHXA,
linux-kernel-u79uwXL29TY76Z2rM5mHXA
In-Reply-To: <1247522953-12702-1-git-send-email-tomas.winkler-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>
Hi Tomas,
> 1. add intel's sdio vendor id to sdio_ids.h
> 2. move iwmc3200 sdio devices ids to sdio_ids.h
this needs a better commit message since you are touching three
subsystems right now.
Please explain what you are doing. Sounds maybe too obvious, but it is
important that we have clear commit message. Especially when it is
touching more than one subsystem and people have to resolve merge
conflicts.
> This patch is against 2.6.31-rc2
> 7638d5322bd89d49e013a03fe2afaeb6d214fabd
This should not be part of the commit message.
> Cc:inaky.perez-gonzalez-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org
> Cc:cindy.h.kao-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org
> Cc:samuel.ortiz-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org
> Cc:yi.zhu-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org
> Cc:drzeus-list-p3sGCRWkH8CeZLLa646FqQ@public.gmane.org
> Signed-off-by: Tomas Winkler <tomas.winkler-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>
> ---
> drivers/net/wimax/i2400m/sdio.c | 10 ++++------
> drivers/net/wireless/iwmc3200wifi/sdio.c | 4 +++-
> drivers/net/wireless/iwmc3200wifi/sdio.h | 3 ---
> include/linux/mmc/sdio_ids.h | 5 +++++
> 4 files changed, 12 insertions(+), 10 deletions(-)
>
> diff --git a/drivers/net/wimax/i2400m/sdio.c b/drivers/net/wimax/i2400m/sdio.c
> index 2538825..3dc1990 100644
> --- a/drivers/net/wimax/i2400m/sdio.c
> +++ b/drivers/net/wimax/i2400m/sdio.c
> @@ -58,6 +58,7 @@
> */
>
> #include <linux/debugfs.h>
> +#include <linux/mmc/sdio_ids.h>
> #include <linux/mmc/sdio.h>
> #include <linux/mmc/sdio_func.h>
> #include "i2400m-sdio.h"
> @@ -501,14 +502,11 @@ void i2400ms_remove(struct sdio_func *func)
> d_fnend(3, dev, "SDIO func %p\n", func);
> }
>
> -enum {
> - I2400MS_INTEL_VID = 0x89,
> -};
> -
> static
> const struct sdio_device_id i2400ms_sdio_ids[] = {
> - /* Intel: i2400m WiMAX over SDIO */
> - { SDIO_DEVICE(I2400MS_INTEL_VID, 0x1402) },
> + /* Intel: i2400m WiMAX (iwm3200) over SDIO */
> + { SDIO_DEVICE(SDIO_VENDOR_ID_INTEL,
> + SDIO_DEVICE_ID_INTEL_IWMC3200WIMAX) },
> { }, /* end: all zeroes */
for readability, please indent the SDIO_DEVICE_ID_INTEL...
Otherwise you easily assume you have two SDIO_DEVICE() statement.
> };
> MODULE_DEVICE_TABLE(sdio, i2400ms_sdio_ids);
> diff --git a/drivers/net/wireless/iwmc3200wifi/sdio.c b/drivers/net/wireless/iwmc3200wifi/sdio.c
> index 9166818..995a3f6 100644
> --- a/drivers/net/wireless/iwmc3200wifi/sdio.c
> +++ b/drivers/net/wireless/iwmc3200wifi/sdio.c
> @@ -65,6 +65,7 @@
> #include <linux/kernel.h>
> #include <linux/netdevice.h>
> #include <linux/debugfs.h>
> +#include <linux/mmc/sdio_ids.h>
> #include <linux/mmc/sdio.h>
> #include <linux/mmc/sdio_func.h>
>
> @@ -492,7 +493,8 @@ static void iwm_sdio_remove(struct sdio_func *func)
> }
>
> static const struct sdio_device_id iwm_sdio_ids[] = {
> - { SDIO_DEVICE(SDIO_VENDOR_ID_INTEL, SDIO_DEVICE_ID_IWM) },
> + { SDIO_DEVICE(SDIO_VENDOR_ID_INTEL,
> + SDIO_DEVICE_ID_INTEL_IWMC3200WIFI) },
> { /* end: all zeroes */ },
Same here.
> };
> MODULE_DEVICE_TABLE(sdio, iwm_sdio_ids);
> diff --git a/drivers/net/wireless/iwmc3200wifi/sdio.h b/drivers/net/wireless/iwmc3200wifi/sdio.h
> index b3c156b..aab6b68 100644
> --- a/drivers/net/wireless/iwmc3200wifi/sdio.h
> +++ b/drivers/net/wireless/iwmc3200wifi/sdio.h
> @@ -39,9 +39,6 @@
> #ifndef __IWM_SDIO_H__
> #define __IWM_SDIO_H__
>
> -#define SDIO_VENDOR_ID_INTEL 0x89
> -#define SDIO_DEVICE_ID_IWM 0x1403
> -
> #define IWM_SDIO_DATA_ADDR 0x0
> #define IWM_SDIO_INTR_ENABLE_ADDR 0x14
> #define IWM_SDIO_INTR_STATUS_ADDR 0x13
> diff --git a/include/linux/mmc/sdio_ids.h b/include/linux/mmc/sdio_ids.h
> index 39751c8..b46a57a 100644
> --- a/include/linux/mmc/sdio_ids.h
> +++ b/include/linux/mmc/sdio_ids.h
> @@ -22,6 +22,11 @@
> /*
> * Vendors and devices. Sort key: vendor first, device next.
> */
> +#define SDIO_VENDOR_ID_INTEL 0x0089
> +#define SDIO_DEVICE_ID_INTEL_IWMC3200WIMAX 0x1402
> +#define SDIO_DEVICE_ID_INTEL_IWMC3200WIFI 0x1403
> +#define SDIO_DEVICE_ID_INTEL_IWMC3200TOP 0x1404
> +#define SDIO_DEVICE_ID_INTEL_IWMC3200BT 0x1406
While you are touching files all over, what about GPS?
> #define SDIO_VENDOR_ID_MARVELL 0x02df
> #define SDIO_DEVICE_ID_MARVELL_LIBERTAS 0x9103
Regards
Marcel
--
To unsubscribe from this list: send the line "unsubscribe linux-wireless" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply
* Re: [PATCH] imwc3200: move iwmc3200 SDIO ids to sdio_ids.h
From: Marcel Holtmann @ 2009-07-14 2:17 UTC (permalink / raw)
To: David Miller
Cc: tomas.winkler, drzeus-list, netdev, linux-wireless, linux-kernel
In-Reply-To: <20090713.191001.70978868.davem@davemloft.net>
Hi Dave,
> From: Tomas Winkler <tomas.winkler@intel.com>
> Date: Tue, 14 Jul 2009 01:09:13 +0300
>
> > 1. add intel's sdio vendor id to sdio_ids.h
> > 2. move iwmc3200 sdio devices ids to sdio_ids.h
> >
> > This patch is against 2.6.31-rc2
> > 7638d5322bd89d49e013a03fe2afaeb6d214fabd
> >
> > Cc:inaky.perez-gonzalez@intel.com
> > Cc:cindy.h.kao@intel.com
> > Cc:samuel.ortiz@intel.com
> > Cc:yi.zhu@intel.com
> > Cc:drzeus-list@drzeus.cx
> > Signed-off-by: Tomas Winkler <tomas.winkler@intel.com>
>
> Since wimax stuff goes directly through me and John Linville handles
> wireless things, you're making this overly difficult by combining
> changes for those two subsystems into a single patch.
>
> Please split this up, thanks.
if Pierre is not afraid of a merge conflict during the next merge window
with the SDIO changes, then I think this should go completely via you. I
am also fine if we just split this up and send one patch to Pierre and
one to you. However since speaking from history, your next tree gets
mostly merged before Pierre's we might producing build errors during the
next merge window.
I can ACK the WiMAX and WiFi changes since both Inaky and Samuel are on
vacation.
Regards
Marcel
^ permalink raw reply
* Re: use after free bug in socket code
From: Lothar Waßmann @ 2009-07-14 6:24 UTC (permalink / raw)
To: Oliver Hartkopp; +Cc: Herbert Xu, davem, netdev, urs.thuermann, Urs Thuermann
In-Reply-To: <4A5B730B.8090902@hartkopp.net>
Hi,
Oliver Hartkopp writes:
> unfortunately i did not get any answer from Urs so far, who originally created
> these lines of code.
>
> But from what i was able to get from browsing similar code in the Kernel that
> at least sock_orphan() is called in the appropriate socket release functions,
> which is indeed not done by the mentioned PF_CAN protocols right now.
>
> I assume you already tested this patch (at least with CAN_RAW) successfully,
> right?
>
Yes.
> If so, i would have no objections to add my Acked-by to these changes.
>
> Would you like to prepare a proper patch and post it on netdev?
>
I'll do.
I would also submit a second patch to add an appropriate MODULE_ALIAS
to the protocol drivers, so they can be autoloaded when compiled as
module:
diff -ur linux-2.6.30/net/can/bcm.c linux-2.6.30-karo/net/can/bcm.c
--- linux-2.6.30/net/can/bcm.c 2009-06-10 05:05:27.000000000 +0200
+++ linux-2.6.30-karo/net/can/bcm.c 2009-07-12 20:12:38.000000000 +0200
@@ -75,6 +75,7 @@
MODULE_DESCRIPTION("PF_CAN broadcast manager protocol");
MODULE_LICENSE("Dual BSD/GPL");
MODULE_AUTHOR("Oliver Hartkopp <oliver.hartkopp@volkswagen.de>");
+MODULE_ALIAS("can-proto-2");
/* easy access to can_frame payload */
static inline u64 GET_U64(const struct can_frame *cp)
diff -ur linux-2.6.30/net/can/raw.c linux-2.6.30-karo/net/can/raw.c
--- linux-2.6.30/net/can/raw.c 2009-06-10 05:05:27.000000000 +0200
+++ linux-2.6.30-karo/net/can/raw.c 2009-07-12 20:12:29.000000000 +0200
@@ -62,6 +62,7 @@
MODULE_DESCRIPTION("PF_CAN raw protocol");
MODULE_LICENSE("Dual BSD/GPL");
MODULE_AUTHOR("Urs Thuermann <urs.thuermann@volkswagen.de>");
+MODULE_ALIAS("can-proto-1");
#define MASK_ALL 0
> ps. This code section was stable for more than three years now. Can you tell
> me, how you kicked your system to run into this problem?
>
I was working on a chip driver for the i.MX25 flexcan controller. The
bug was visible due to CONFIG_DEBUG_SLAB=y which makes sure that
memory is poisoned with a special pattern upon being freed.
The situation where this triggers a bug is when the chip driver's
hard_start_xmit function returns a NETDEV_TX_BUSY and subsequently the
can interface is deconfigured.
Maybe you could try this on different hardware?
Lothar Waßmann
--
___________________________________________________________
Ka-Ro electronics GmbH | Pascalstraße 22 | D - 52076 Aachen
Phone: +49 2408 1402-0 | Fax: +49 2408 1402-10
Geschäftsführer: Matthias Kaussen
Handelsregistereintrag: Amtsgericht Aachen, HRB 4996
www.karo-electronics.de | info@karo-electronics.de
___________________________________________________________
^ permalink raw reply
* Re: [PATCH] gre: copy ToS/DiffServ bits to outer IP header
From: Andreas Jaggi @ 2009-07-14 7:14 UTC (permalink / raw)
To: David Miller; +Cc: netdev, kuznet, kaber
In-Reply-To: <20090713.104433.244099298.davem@davemloft.net>
On Mon, Jul 13, 2009 at 10:44:33AM -0700, David Miller wrote:
> Why not just create a new IFLA_GRE_* attribute to pass in
> flags like this that purely are private to the GRE Linux
> tunnel driver and have nothing to do with values defined
> by GRE tunnel headers at all?
OK. What would be an appropriate place to store such a private flag in
the GRE driver?
I thought of extending the ip_tunnel_parms struct with '__u16 priv_flags'
(But this would break compatibility with the iproute2 tools?)
^ permalink raw reply
* Re: [PATCH] x86/PCI: initialize PCI bus node numbers early
From: Jesse Brandeburg @ 2009-07-14 7:41 UTC (permalink / raw)
To: Jesse Barnes; +Cc: Yinghai Lu, linux-kernel, NetDEV list, ak, matthew
In-Reply-To: <20090710140654.32132bcb@jbarnes-g45>
On Fri, Jul 10, 2009 at 2:06 PM, Jesse Barnes<jbarnes@virtuousgeek.org> wrote:
> From 2b51fba93f7b2dabf453a74923a9a217611ebc1a Mon Sep 17 00:00:00 2001
> From: Jesse Barnes <jbarnes@virtuousgeek.org>
> Date: Fri, 10 Jul 2009 14:04:30 -0700
> Subject: [PATCH] x86/PCI: initialize PCI bus node numbers early
>
> The current mp_bus_to_node array is initialized only by AMD specific
> code, since AMD platforms have registers that can be used for
> determining mode numbers. On new Intel platforms it's necessary to
> initialize this array as well though, otherwise all PCI node numbers
> will be 0, when in fact they should be -1 (indicating that I/O isn't
> tied to any particular node).
>
> So move the mp_bus_to_node code into the common PCI code, and
> initialize it early with a default value of -1. This may be overridden
> later by arch code (e.g. the AMD code).
>
> With this change, PCI consistent memory and other node specific
> allocations (e.g. skbuff allocs) should occur on the "current" node.
> If, for performance reasons, applications want to be bound to specific
> nodes, they should open their devices only after being pinned to the
> CPU where they'll run, for maximum locality.
>
> Acked-by: Yinghai Lu <yinghai@kernel.org>
> Tested-by: Jesse Brandeburg <jesse.brandeburg@gmail.com>
> Signed-off-by: Jesse Barnes <jbarnes@virtuousgeek.org>
I can confirm this works, aside from the MSI-X interrupt migration
instability (panics) that I believe are unrelated since they happen
without this patch.
I also see a pretty nice performance boost by running with this change
on a 5520 motherboard, with an 82599 10GbE forwarding packets, esp
with interrupt affinity set correctly.
I'd like to see this applied if at all possible, I think it is really
hampering I/O traffic performance due to limiting all network (among
others) memory allocation to one of the two numa nodes.
^ permalink raw reply
* Re: [patch 1/3] net: serialize hrtimer callback in sched_cbq
From: Patrick McHardy @ 2009-07-14 8:22 UTC (permalink / raw)
To: David Miller; +Cc: tglx, netdev, linux-kernel, peterz
In-Reply-To: <20090712.135555.207096388.davem@davemloft.net>
[-- Attachment #1: Type: text/plain, Size: 1264 bytes --]
David Miller wrote:
> From: Thomas Gleixner <tglx@linutronix.de>
> Date: Thu, 09 Jul 2009 21:59:22 -0000
>
>> The hrtimer callback cbq_undelay() is not serialized against
>> cbq_ovl_delay(). That affects at least q->pmask and q->delay_timer.
>>
>> Lock it proper.
>>
>> Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
>
> The problems here are even much deeper than it appears.
>
> First of all, I am to understand that hrtimers run from hardware
> interrupt context, right? If so, all of these datastructures are
> softirq safe only.
>
> And it is not merely the immediate things you see being modified in
> this hrtimer, such as ->pmask etc., it is also the q->active[]
> pointers, the list state for the classes, just about everything in the
> qdisc state is referenced in this hrtimer code path.
>
> I wonder how many queer unexplainable bugs we see because of this.
>
> What should probably happen is that the hrtimer merely fires off work
> at software interrupt context (perhaps a tasklet or similar), and that
> software interrupt code take the qdisc's root lock throughout it's
> execution.
That's my understanding what HRTIMER_SOFTIRQ is used for. I think
simply grabbing the root lock in cbq_undelay() should be fine.
Compile-tested only.
[-- Attachment #2: 01.diff --]
[-- Type: text/x-patch, Size: 1229 bytes --]
commit a790fb8873f1cbe8b9cb48cb368851e30d3ec172
Author: Patrick McHardy <kaber@trash.net>
Date: Tue Jul 14 10:19:47 2009 +0200
net-sched: sch_cbq: fix locking in cbq_undelay()
The hrtimer callback cbq_undelay() is not serialized against
cbq_ovl_delay(). That affects at least q->pmask and q->delay_timer.
Lock it proper.
Based on patch by Thomas Gleixner <tglx@linutronix.de>
Signed-off-by: Patrick McHardy <kaber@trash.net>
diff --git a/net/sched/sch_cbq.c b/net/sched/sch_cbq.c
index 23a1676..7c659c6 100644
--- a/net/sched/sch_cbq.c
+++ b/net/sched/sch_cbq.c
@@ -593,12 +593,16 @@ static enum hrtimer_restart cbq_undelay(struct hrtimer *timer)
struct cbq_sched_data *q = container_of(timer, struct cbq_sched_data,
delay_timer);
struct Qdisc *sch = q->watchdog.qdisc;
+ spinlock_t *root_lock;
psched_time_t now;
psched_tdiff_t delay = 0;
unsigned pmask;
now = psched_get_time();
+ root_lock = qdisc_lock(qdisc_root(sch));
+ spin_lock(root_lock);
+
pmask = q->pmask;
q->pmask = 0;
@@ -615,6 +619,7 @@ static enum hrtimer_restart cbq_undelay(struct hrtimer *timer)
delay = tmp;
}
}
+ spin_unlock(root_lock);
if (delay) {
ktime_t time;
^ permalink raw reply related
* Re: [patch 1/3] net: serialize hrtimer callback in sched_cbq
From: Peter Zijlstra @ 2009-07-14 8:30 UTC (permalink / raw)
To: Patrick McHardy; +Cc: David Miller, tglx, netdev, linux-kernel
In-Reply-To: <4A5C402A.7090906@trash.net>
On Tue, 2009-07-14 at 10:22 +0200, Patrick McHardy wrote:
>
> That's my understanding what HRTIMER_SOFTIRQ is used for. I think
> simply grabbing the root lock in cbq_undelay() should be fine.
Its not, and its going away soon (again) :-)
The current use of HRTIMER_SOFTIRQ is for when we enqueue a hrtimer with
an expiration time in the past. The current implementation tries to run
the timer instantly, however we cannot do it from the context calling
hrtimer_start(), since that might be holding locks the timer callback
also wants to hold, resulting in deadlocks.
Instead we queue the timer to the softirq, and kick the softirq. Which
leads to another problem in that we cannot always kick the softirq (esp
from within the scheduler).
We're going to change hrtimer_start() to return -ETIME instead of trying
to run the timer in-place, leaving the callers to figure it out.
The basic patch is done, but I still need to audit all the hrtimer users
in the kernel.
^ permalink raw reply
* [PATCH] skbuff.h: Fix comment for NET_IP_ALIGN
From: Tobias Klauser @ 2009-07-14 8:48 UTC (permalink / raw)
To: davem; +Cc: netdev, Tobias Klauser
Use the correct function call for skb_reserve in the comment for
NET_IP_ALIGN.
Signed-off-by: Tobias Klauser <klto@zhaw.ch>
---
include/linux/skbuff.h | 4 ++--
1 files changed, 2 insertions(+), 2 deletions(-)
diff --git a/include/linux/skbuff.h b/include/linux/skbuff.h
index b47b3f0..f2c69a2 100644
--- a/include/linux/skbuff.h
+++ b/include/linux/skbuff.h
@@ -1342,12 +1342,12 @@ static inline int skb_network_offset(const struct sk_buff *skb)
* shifting the start of the packet by 2 bytes. Drivers should do this
* with:
*
- * skb_reserve(NET_IP_ALIGN);
+ * skb_reserve(skb, NET_IP_ALIGN);
*
* The downside to this alignment of the IP header is that the DMA is now
* unaligned. On some architectures the cost of an unaligned DMA is high
* and this cost outweighs the gains made by aligning the IP header.
- *
+ *
* Since this trade off varies between architectures, we allow NET_IP_ALIGN
* to be overridden.
*/
--
1.6.0.4
^ permalink raw reply related
* Re: [patch 1/3] net: serialize hrtimer callback in sched_cbq
From: Thomas Gleixner @ 2009-07-14 8:55 UTC (permalink / raw)
To: David Miller; +Cc: netdev, linux-kernel, kaber, peterz
In-Reply-To: <20090712.135555.207096388.davem@davemloft.net>
David,
On Sun, 12 Jul 2009, David Miller wrote:
> From: Thomas Gleixner <tglx@linutronix.de>
> Date: Thu, 09 Jul 2009 21:59:22 -0000
>
> > The hrtimer callback cbq_undelay() is not serialized against
> > cbq_ovl_delay(). That affects at least q->pmask and q->delay_timer.
> >
> > Lock it proper.
> >
> > Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
>
> The problems here are even much deeper than it appears.
>
> First of all, I am to understand that hrtimers run from hardware
> interrupt context, right? If so, all of these datastructures are
> softirq safe only.
>
> And it is not merely the immediate things you see being modified in
> this hrtimer, such as ->pmask etc., it is also the q->active[]
> pointers, the list state for the classes, just about everything in the
> qdisc state is referenced in this hrtimer code path.
That's what I was worried about.
> I wonder how many queer unexplainable bugs we see because of this.
>
> What should probably happen is that the hrtimer merely fires off work
> at software interrupt context (perhaps a tasklet or similar), and that
> software interrupt code take the qdisc's root lock throughout it's
> execution.
Sigh, I almost expected that the removal of the callback modes will
fire back some day.
Thanks,
tglx
^ permalink raw reply
* Re: [PATCH] iproute2 flush: handle larger tables and deleted entries
From: Patrick McHardy @ 2009-07-14 9:38 UTC (permalink / raw)
To: Gautam Kachroo; +Cc: netdev
In-Reply-To: <4e0db5bc0907130939k48b16256j8f60c786a7e5e44c@mail.gmail.com>
Gautam Kachroo wrote:
> use a new netlink socket when sending flush messages to avoid reading
> any pending data on the existing netlink socket.
>
> read all of the response from the netlink request -- this response can
> be split over multiple recv calls, pretty much one per netlink request
> message. ENOENT errors, which correspond to attempts to delete an
> already deleted entry, are ignored. Other errors are not ignored.
In which case would there be any pending data? From what I can see,
this can only happen when using batching, but in that case the
previous command should continue reading until it has received all
responses (which the netlink functions appear to be doing properly).
^ permalink raw reply
* [PATCH v2] fs_enet/mii-fec.c: fix MII speed calculation
From: Wolfgang Denk @ 2009-07-14 13:42 UTC (permalink / raw)
To: linuxppc-dev; +Cc: netdev, Wolfgang Denk
In-Reply-To: <1244751543-21977-2-git-send-email-wd@denx.de>
The MII speed calculation was based on the CPU clock (ppc_proc_freq),
but for MPC512x we must use the bus clock instead.
This patch makes it use the correct clock and makes sure we don't
clobber reserved bits in the MII_SPEED register.
Signed-off-by: Wolfgang Denk <wd@denx.de>
Cc: Grant Likely <grant.likely@secretlab.ca>
Cc: Kumar Gala <galak@kernel.crashing.org>
Cc: <netdev@vger.kernel.org>
---
Note 1: This patch is a rewrite of a previous posting:
http://patchwork.ozlabs.org/patch/28584/
Note 2: Function mpc5xxx_get_mii_speed() was introduced for re-use
in a later patch to drivers/net/fec_mpc52xx.c and
drivers/net/fec_mpc52xx_phy.c, which also contain code to
calculate the MII speed without taking care to check for
overflow or to write only the bits that belong to the
MII_SPEED field when setting the MII speed.
arch/powerpc/sysdev/mpc5xxx_clocks.c | 37 ++++++++++++++++++++++++++++++++++
drivers/net/fs_enet/mii-fec.c | 13 +++++++++--
2 files changed, 47 insertions(+), 3 deletions(-)
diff --git a/arch/powerpc/sysdev/mpc5xxx_clocks.c b/arch/powerpc/sysdev/mpc5xxx_clocks.c
index 34e12f9..e26d12b 100644
--- a/arch/powerpc/sysdev/mpc5xxx_clocks.c
+++ b/arch/powerpc/sysdev/mpc5xxx_clocks.c
@@ -31,3 +31,40 @@ mpc5xxx_get_bus_frequency(struct device_node *node)
return p_bus_freq ? *p_bus_freq : 0;
}
EXPORT_SYMBOL(mpc5xxx_get_bus_frequency);
+
+/**
+ * mpc5xxx_get_mii_speed - Get the MII_SPEED value
+ * @node: device node
+ *
+ * Returns the MII_SPEED value for MPC512x and MPC52xx systems.
+ * The value gets computed such that the resulting MDC frequency
+ * is 2.5 MHz or lower.
+ */
+
+int
+mpc5xxx_get_mii_speed(struct of_device *ofdev)
+{
+ unsigned int clock, speed;
+
+ clock = mpc5xxx_get_bus_frequency(ofdev->node);
+
+ if (!clock) {
+ dev_err(&ofdev->dev, "could not determine IPS/IPB clock\n");
+ return -ENODEV;
+ }
+
+ /* scale for a MII clock <= 2.5 MHz */
+ speed = (clock + 2499999) / 2500000;
+
+ /* only 6 bits available for MII speed */
+ if (speed > 0x3F) {
+ speed = 0x3F;
+ dev_err(&ofdev->dev,
+ "MII clock (%d MHz) exceeds max (2.5 MHz)\n",
+ clock / speed);
+ }
+
+ /* Field is in bits 25:30 of MII_SPEED register */
+ return speed << 1;
+}
+EXPORT_SYMBOL(mpc5xxx_get_mii_speed);
diff --git a/drivers/net/fs_enet/mii-fec.c b/drivers/net/fs_enet/mii-fec.c
index 75a0999..a28d39f 100644
--- a/drivers/net/fs_enet/mii-fec.c
+++ b/drivers/net/fs_enet/mii-fec.c
@@ -36,6 +36,7 @@
#include <asm/pgtable.h>
#include <asm/irq.h>
#include <asm/uaccess.h>
+#include <asm/mpc5xxx.h>
#include "fs_enet.h"
#include "fec.h"
@@ -103,7 +104,6 @@ static int fs_enet_fec_mii_reset(struct mii_bus *bus)
static int __devinit fs_enet_mdio_probe(struct of_device *ofdev,
const struct of_device_id *match)
{
- struct device_node *np = NULL;
struct resource res;
struct mii_bus *new_bus;
struct fec_info *fec;
@@ -133,13 +133,20 @@ static int __devinit fs_enet_mdio_probe(struct of_device *ofdev,
if (!fec->fecp)
goto out_fec;
- fec->mii_speed = ((ppc_proc_freq + 4999999) / 5000000) << 1;
+ if (of_device_is_compatible(ofdev->node, "fsl,mpc5121-fec-mdio")) {
+ i = mpc5xxx_get_mii_speed(ofdev);
+ if (i < 0)
+ goto out_unmap_regs;
+ fec->mii_speed = i;
+ } else {
+ fec->mii_speed = ((ppc_proc_freq + 4999999) / 5000000) << 1;
+ }
setbits32(&fec->fecp->fec_r_cntrl, FEC_RCNTRL_MII_MODE);
setbits32(&fec->fecp->fec_ecntrl, FEC_ECNTRL_PINMUX |
FEC_ECNTRL_ETHER_EN);
out_be32(&fec->fecp->fec_ievent, FEC_ENET_MII);
- out_be32(&fec->fecp->fec_mii_speed, fec->mii_speed);
+ clrsetbits_be32(&fec->fecp->fec_mii_speed, 0x7E, fec->mii_speed);
new_bus->phy_mask = ~0;
new_bus->irq = kmalloc(sizeof(int) * PHY_MAX_ADDR, GFP_KERNEL);
--
1.6.0.6
^ permalink raw reply related
* Re: [PATCH] gre: copy ToS/DiffServ bits to outer IP header
From: Andreas Jaggi @ 2009-07-14 14:21 UTC (permalink / raw)
To: Wojtek Sawasciuk; +Cc: netdev, kuznet, kaber, davem
In-Reply-To: <20090713.104433.244099298.davem@davemloft.net>
On Tue, Jul 14, 2009 at 01:39:48PM +0100, Wojtek Sawasciuk wrote:
> I think I have similar problems to yours, and I spotted your email on
> the lists when googling :)
> Andreas, sorry but I think your patch is unnecessary, at last in this form.
> From long time there is possibility to create tunnel with tos options,
> you can use "inherit" or any other value to set it to encapsulated
> packets.
>
> ip tun add mode gre name tun1 remote 1.2.3.5 local 10.0.0.234 tos inherit
> or
> ip tun add mode gre name tun2 remote 1.2.3.6 local 10.0.0.234 tos 2
>
> and its working, *almost* ok.
> only problem is when you set tos value with bit 1 set in it, regarding
> this fragment of code from ip_gre.c , in ipgre_tunnel_xmit function:
>
> tos = tiph->tos;
> if (tos&1) {
> if (skb->protocol == htons(ETH_P_IP))
> tos = old_iph->tos;
> tos &= ~1;
> }
>
> (ie. set tos to 2 - it will replicate it to 2, set tos to 3 - it will
> change it to 2).
>
> IHMO it should be replaced to use maybe MBZ bit (look on RFC1349) ,
> instead of bit 1 , to pass on value of tos , from tunnel parameter to IP
> header.
> and this should be sufficient to make it work properly.
You're right, my patch in this form is not necessary.
But as you point out, there is a bug in the handling of the ToS/DiffServ
bits in GRE.
iproute2 and the Linux GRE driver use a tos value of 1 to indicate an
'inherit'/'copy-tos' behavior (and then clear this bit before using the
tos value in the IP header of the encapsulation GRE packet).
This was fine for RFC1349 where this bit is defined as MBZ/must-be-zero.
But with RFC3168, this bit (and the other previously unused one in the tos field)
are used for the ECN bits (ECT + CE). Therefore a tos value of 1 is 'legal' and
conflicts with the 'inherit'/'copy-tos' indication of iproute2 and the
Linux GRE driver.
This creates the problem you described, where IP packets who have a tos
value of 1 (with the ECT bit set to 1) are handled with the 'inherit'/'copy-tos'
mechanism and the resulting GRE IP header has a tos value of 0! (the ECT bit
set to 0 instead of 1)
^ permalink raw reply
* Re: use after free bug in socket code
From: Oliver Hartkopp @ 2009-07-14 14:41 UTC (permalink / raw)
To: Lothar Waßmann
Cc: Herbert Xu, davem, netdev, urs.thuermann, Urs Thuermann
In-Reply-To: <19036.9400.263297.330963@ipc1.ka-ro>
Lothar Waßmann wrote:
> Oliver Hartkopp writes:
>>
>> Would you like to prepare a proper patch and post it on netdev?
>>
> I'll do.
Fine.
> I would also submit a second patch to add an appropriate MODULE_ALIAS
> to the protocol drivers, so they can be autoloaded when compiled as
> module:
> diff -ur linux-2.6.30/net/can/bcm.c linux-2.6.30-karo/net/can/bcm.c
> --- linux-2.6.30/net/can/bcm.c 2009-06-10 05:05:27.000000000 +0200
> +++ linux-2.6.30-karo/net/can/bcm.c 2009-07-12 20:12:38.000000000 +0200
> @@ -75,6 +75,7 @@
> MODULE_DESCRIPTION("PF_CAN broadcast manager protocol");
> MODULE_LICENSE("Dual BSD/GPL");
> MODULE_AUTHOR("Oliver Hartkopp <oliver.hartkopp@volkswagen.de>");
> +MODULE_ALIAS("can-proto-2");
>
> /* easy access to can_frame payload */
> static inline u64 GET_U64(const struct can_frame *cp)
> diff -ur linux-2.6.30/net/can/raw.c linux-2.6.30-karo/net/can/raw.c
> --- linux-2.6.30/net/can/raw.c 2009-06-10 05:05:27.000000000 +0200
> +++ linux-2.6.30-karo/net/can/raw.c 2009-07-12 20:12:29.000000000 +0200
> @@ -62,6 +62,7 @@
> MODULE_DESCRIPTION("PF_CAN raw protocol");
> MODULE_LICENSE("Dual BSD/GPL");
> MODULE_AUTHOR("Urs Thuermann <urs.thuermann@volkswagen.de>");
> +MODULE_ALIAS("can-proto-1");
>
> #define MASK_ALL 0
>
Good idea.
I currently added these aliases somewhere in my /etc/modprobe.d directory. But
if this can be done by the kernel itself, we can reduce the distro-depended
configuation effort.
You can add my
Acked-by: Oliver Hartkopp <oliver@hartkopp.net>
to both discussed patches directly.
>
>> ps. This code section was stable for more than three years now. Can you tell
>> me, how you kicked your system to run into this problem?
>>
> I was working on a chip driver for the i.MX25 flexcan controller. The
> bug was visible due to CONFIG_DEBUG_SLAB=y which makes sure that
> memory is poisoned with a special pattern upon being freed.
Nice hint! I will enable this in my config also.
>
> The situation where this triggers a bug is when the chip driver's
> hard_start_xmit function returns a NETDEV_TX_BUSY and subsequently the
> can interface is deconfigured.
>
> Maybe you could try this on different hardware?
Will do when i'm back at work ;-)
Many Thanks,
Oliver
^ permalink raw reply
* Re: use after free bug in socket code
From: Lothar Waßmann @ 2009-07-14 15:18 UTC (permalink / raw)
To: Oliver Hartkopp; +Cc: Herbert Xu, davem, netdev, urs.thuermann, Urs Thuermann
In-Reply-To: <4A5C990E.3080703@hartkopp.net>
Hi,
Oliver Hartkopp writes:
> > The situation where this triggers a bug is when the chip driver's
> > hard_start_xmit function returns a NETDEV_TX_BUSY and subsequently the
> > can interface is deconfigured.
> >
> > Maybe you could try this on different hardware?
>
> Will do when i'm back at work ;-)
>
Should I wait for the results of your tests before submitting the
patches?
Lothar Waßmann
--
___________________________________________________________
Ka-Ro electronics GmbH | Pascalstraße 22 | D - 52076 Aachen
Phone: +49 2408 1402-0 | Fax: +49 2408 1402-10
Geschäftsführer: Matthias Kaussen
Handelsregistereintrag: Amtsgericht Aachen, HRB 4996
www.karo-electronics.de | info@karo-electronics.de
___________________________________________________________
^ permalink raw reply
* [PATCH] gre: fix ToS/DiffServ inherit bug
From: Andreas Jaggi @ 2009-07-14 15:36 UTC (permalink / raw)
To: Wojtek Sawasciuk; +Cc: netdev, kuznet, kaber, davem
Fixes two bugs:
- ToS/DiffServ inheritance was unintentionally activated when using impair fixed ToS values
- ECN bit was lost during ToS/DiffServ inheritance
Signed-off-by: Andreas Jaggi <aj@open.ch>
--- vanilla-linux-2.6.29.4/net/ipv4/ip_gre.c 2009-05-19 01:52:34.000000000 +0200
+++ gre-dev-2/net/ipv4/ip_gre.c 2009-07-14 17:14:34.000000000 +0200
@@ -677,10 +677,10 @@
}
tos = tiph->tos;
- if (tos&1) {
+ if (tos == 1) {
+ tos = 0;
if (skb->protocol == htons(ETH_P_IP))
tos = old_iph->tos;
- tos &= ~1;
}
{
^ permalink raw reply
* Re: [PATCH] x86/PCI: initialize PCI bus node numbers early
From: Jesse Barnes @ 2009-07-14 15:47 UTC (permalink / raw)
To: Jesse Brandeburg; +Cc: Yinghai Lu, linux-kernel, NetDEV list, ak, matthew
In-Reply-To: <4807377b0907140041y6c9da555lf3e1dba0775cfe7c@mail.gmail.com>
On Tue, 14 Jul 2009 00:41:30 -0700
Jesse Brandeburg <jesse.brandeburg@gmail.com> wrote:
> On Fri, Jul 10, 2009 at 2:06 PM, Jesse
> Barnes<jbarnes@virtuousgeek.org> wrote:
> > From 2b51fba93f7b2dabf453a74923a9a217611ebc1a Mon Sep 17 00:00:00
> > 2001 From: Jesse Barnes <jbarnes@virtuousgeek.org>
> > Date: Fri, 10 Jul 2009 14:04:30 -0700
> > Subject: [PATCH] x86/PCI: initialize PCI bus node numbers early
> >
> > The current mp_bus_to_node array is initialized only by AMD specific
> > code, since AMD platforms have registers that can be used for
> > determining mode numbers. On new Intel platforms it's necessary to
> > initialize this array as well though, otherwise all PCI node numbers
> > will be 0, when in fact they should be -1 (indicating that I/O isn't
> > tied to any particular node).
> >
> > So move the mp_bus_to_node code into the common PCI code, and
> > initialize it early with a default value of -1. This may be
> > overridden later by arch code (e.g. the AMD code).
> >
> > With this change, PCI consistent memory and other node specific
> > allocations (e.g. skbuff allocs) should occur on the "current" node.
> > If, for performance reasons, applications want to be bound to
> > specific nodes, they should open their devices only after being
> > pinned to the CPU where they'll run, for maximum locality.
> >
> > Acked-by: Yinghai Lu <yinghai@kernel.org>
> > Tested-by: Jesse Brandeburg <jesse.brandeburg@gmail.com>
> > Signed-off-by: Jesse Barnes <jbarnes@virtuousgeek.org>
>
> I can confirm this works, aside from the MSI-X interrupt migration
> instability (panics) that I believe are unrelated since they happen
> without this patch.
>
> I also see a pretty nice performance boost by running with this change
> on a 5520 motherboard, with an 82599 10GbE forwarding packets, esp
> with interrupt affinity set correctly.
>
> I'd like to see this applied if at all possible, I think it is really
> hampering I/O traffic performance due to limiting all network (among
> others) memory allocation to one of the two numa nodes.
Ok, thanks for testing. I've pushed it to my linux-next branch.
--
Jesse Barnes, Intel Open Source Technology Center
^ permalink raw reply
* Re: [patch 1/3] net: serialize hrtimer callback in sched_cbq
From: David Miller @ 2009-07-14 16:00 UTC (permalink / raw)
To: tglx; +Cc: netdev, linux-kernel, kaber, peterz
In-Reply-To: <alpine.LFD.2.00.0907141040350.2777@localhost.localdomain>
From: Thomas Gleixner <tglx@linutronix.de>
Date: Tue, 14 Jul 2009 10:55:14 +0200 (CEST)
> David,
>
> On Sun, 12 Jul 2009, David Miller wrote:
>
>> What should probably happen is that the hrtimer merely fires off work
>> at software interrupt context (perhaps a tasklet or similar), and that
>> software interrupt code take the qdisc's root lock throughout it's
>> execution.
>
> Sigh, I almost expected that the removal of the callback modes will
> fire back some day.
Well this makes hrtimers decidedly less useful for networking and we
have a ton of bugs right now, basically in every hrtimer used by the
networking currently.
The only way we can use them, as things currently stand, is as
triggers for softirq work.
Is it really that troublesome to provide this kind of facility
generically, rather than having various subsystems replicate such code
where they want to use hrtimers and are restricted to softirqs?
^ permalink raw reply
* Re: [patch 1/3] net: serialize hrtimer callback in sched_cbq
From: David Miller @ 2009-07-14 16:01 UTC (permalink / raw)
To: kaber; +Cc: tglx, netdev, linux-kernel, peterz
In-Reply-To: <4A5C402A.7090906@trash.net>
From: Patrick McHardy <kaber@trash.net>
Date: Tue, 14 Jul 2009 10:22:02 +0200
> That's my understanding what HRTIMER_SOFTIRQ is used for. I think
> simply grabbing the root lock in cbq_undelay() should be fine.
>
> Compile-tested only.
Unfortunately, as Peter and Thomas explained, this is not the case.
^ permalink raw reply
* Re: sk_lock: inconsistent {RECLAIM_FS-ON-W} -> {IN-RECLAIM_FS-W} usage
From: David Miller @ 2009-07-14 16:04 UTC (permalink / raw)
To: herbert-lOAM2aK0SrRLBo1qDEOMRrpzq4S04n8Q
Cc: fengguang.wu-ral2JQCrhuEAvxtiuMwx3w,
linux-kernel-u79uwXL29TY76Z2rM5mHXA,
linux-nfs-u79uwXL29TY76Z2rM5mHXA, netdev-u79uwXL29TY76Z2rM5mHXA
In-Reply-To: <20090710080247.GA2693-lOAM2aK0SrRLBo1qDEOMRrpzq4S04n8Q@public.gmane.org>
From: Herbert Xu <herbert-lOAM2aK0SrRLBo1qDEOMRrpzq4S04n8Q@public.gmane.org>
Date: Fri, 10 Jul 2009 16:02:47 +0800
> On Fri, Jul 10, 2009 at 04:00:17PM +0800, Wu Fengguang wrote:
>>
>> The (sk_allocation & ~__GFP_WAIT) cases should be rare, but I guess
>> the networking code shall do it anyway, because sk_allocation defaults
>> to GFP_KERNEL. It seems that currently the networking code simply uses
>> a lot of GFP_ATOMIC, do they really mean "I cannot sleep"?
>
> Yep because they're done from softirq context.
Yes, this is the core issue.
All of Wu's talk about how "GFP_ATOMIC will wake up kswapd and
therefore can succeed just as well as GFP_KERNEL" is not relevant,
because GFP_ATOMIC means sleeping is not allowed.
--
To unsubscribe from this list: send the line "unsubscribe linux-nfs" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply
* Re: weird problem
From: Jarek Poplawski @ 2009-07-14 16:24 UTC (permalink / raw)
To: Paweł Staszewski
Cc: Eric Dumazet, Eric Dumazet, Linux Network Development list
In-Reply-To: <4A5BC2B6.9020709@itcare.pl>
On Tue, Jul 14, 2009 at 01:26:46AM +0200, Paweł Staszewski wrote:
> Jarek Poplawski pisze:
>> On Fri, Jul 10, 2009 at 04:47:54PM +0200, Jarek Poplawski wrote:
>>
>>> On Fri, Jul 10, 2009 at 01:59:00AM +0200, Paweł Staszewski wrote:
>>>
>>>> Today i make other tests with change of
>>>> /proc/sys/net/ipv4/rt_cache_rebuild_count and kernel 2.6.30.1
>>>>
>>>> And when rt_cache_rebuild_count is set to "-1" i have always load
>>>> on x86_64 machine approx 40-50% of each cpu where network card is
>>>> binded by irq_aff
>>>>
>>>> when rt_cache_rebuild_count is set to more than "-1" i have 15 to
>>>> 20 sec of 1 to 3% cpu and after 40-50% cpu
>>>>
>>> ...
>>>
>>> Here is one more patch for testing (with caution!). It adds possibility
>>> to turn off cache disabling (so it should even more resemble 2.6.28)
>>> after setting: rt_cache_rebuild_count = 0
>>>
>>> I'd like you to try this patch:
>>> 1) together with the previous patch and "rt_cache_rebuild_count = 0"
>>> to check if there is still the difference wrt. 2.6.28; Btw., let
>>> me know which /proc/sys/net/ipv4/route/* settings do you need to
>>> change and why
>>>
>>> 2) alone (without the previous patch) and "rt_cache_rebuild_count = 0"
>>>
>>> 3) if it's possible to try 2.6.30.1 without these patches, but with
>>> default /proc/sys/net/ipv4/route/* settings, and higher
>>> rt_cache_rebuild_count, e.g. 100; I'm interested if/how long it
>>> takes to trigger higher cpu load and the warning "... rebuilds is
>>> over limit, route caching disabled"; (Btw., I wonder why you didn't
>>> mention about these or maybe also other route caching warnings?)
>>>
>>
>> Here is take 2 to respect setting "rt_cache_rebuild_count = 0" even
>> after cache rebuild counter has been increased earlier. (Btw, don't
>> forget about this setting after going back to vanilla kernel.)
>>
>>
> Applied to 2.6.30.1
> 1) With
>
> rt_cache_rebuild_count = 0
> grep . /proc/sys/net/ipv4/route/*
> /proc/sys/net/ipv4/route/error_burst:1250
> /proc/sys/net/ipv4/route/error_cost:250
> /proc/sys/net/ipv4/route/gc_elasticity:4
> /proc/sys/net/ipv4/route/gc_interval:15
> /proc/sys/net/ipv4/route/gc_min_interval:0
> /proc/sys/net/ipv4/route/gc_min_interval_ms:0
> /proc/sys/net/ipv4/route/gc_thresh:190536
> /proc/sys/net/ipv4/route/gc_timeout:15
> /proc/sys/net/ipv4/route/max_size:1524288
> /proc/sys/net/ipv4/route/min_adv_mss:256
> /proc/sys/net/ipv4/route/min_pmtu:552
> /proc/sys/net/ipv4/route/mtu_expires:600
> /proc/sys/net/ipv4/route/redirect_load:5
> /proc/sys/net/ipv4/route/redirect_number:9
> /proc/sys/net/ipv4/route/redirect_silence:5120
> /proc/sys/net/ipv4/route/secret_interval:3600
>
> I tune this route parameters after looking of traffic/route cache to have not many entries in cache that are not needed anymore
> so gc_timeout = 15
> limit of max entries = 1524288
> And make route cahce a little more "faster" for me after tune
> gc_elasticity
> secret_interval
> gc_interval
> gc_thresh
>
> So with this parameters 15 sec of something like this:
> 00:41:23 CPU %usr %nice %sys %iowait %irq %soft %steal %guest %idle
> 00:41:24 all 0.00 0.00 0.12 0.00 1.49 10.46 0.00 0.00 87.92
> 00:41:24 0 0.00 0.00 0.00 0.00 0.00 0.00 0.00 0.00 100.00
> 00:41:24 1 0.00 0.00 0.00 0.00 4.00 36.00 0.00 0.00 60.00
> 00:41:24 2 0.00 0.00 0.00 0.00 8.91 47.52 0.00 0.00 43.56
> 00:41:24 3 0.00 0.00 0.00 0.00 0.00 0.00 0.00 0.00 100.00
> 00:41:24 4 0.00 0.00 0.00 0.00 0.00 0.00 0.00 0.00 100.00
> 00:41:24 5 0.00 0.00 0.00 0.00 0.00 0.00 0.00 0.00 100.00
> 00:41:24 6 0.00 0.00 0.00 0.00 0.00 0.00 0.00 0.00 100.00
> 00:41:24 7 0.00 0.00 0.00 0.00 0.00 0.00 0.00 0.00 100.00
>
> and 15 sec of something like this:
> 00:41:44 CPU %usr %nice %sys %iowait %irq %soft %steal %guest %idle
> 00:41:45 all 0.00 0.00 0.00 0.00 0.00 0.42 0.00 0.00 99.58
> 00:41:45 0 0.00 0.00 0.00 0.00 0.00 0.00 0.00 0.00 100.00
> 00:41:45 1 0.00 0.00 0.00 0.00 0.00 1.00 0.00 0.00 99.00
> 00:41:45 2 0.00 0.00 0.00 0.00 0.00 2.04 0.00 0.00 97.96
> 00:41:45 3 0.00 0.00 0.00 0.00 0.00 0.00 0.00 0.00 100.00
> 00:41:45 4 0.00 0.00 0.00 0.00 0.00 0.00 0.00 0.00 100.00
> 00:41:45 5 0.00 0.00 0.00 0.00 0.00 0.00 0.00 0.00 100.00
> 00:41:45 6 0.00 0.00 0.00 0.00 0.00 0.00 0.00 0.00 100.00
> 00:41:45 7 0.00 0.00 0.00 0.00 0.00 0.00 0.00 0.00 100.00
>
> So i change /proc/sys/net/ipv4/route/gc_timeout to 1
> with rt_cache_rebuild_count = 0
> And output is like 20 sec of something like this
> 00:48:52 CPU %usr %nice %sys %iowait %irq %soft %steal %guest %idle
> 00:48:53 all 0.00 0.00 0.19 0.00 0.19 0.58 0.00 0.00 99.03
> 00:48:53 0 0.00 0.00 0.00 0.00 0.00 0.00 0.00 0.00 100.00
> 00:48:53 1 0.00 0.00 0.99 0.00 0.99 0.00 0.00 0.00 98.02
> 00:48:53 2 0.00 0.00 0.00 0.00 0.00 2.00 0.00 0.00 98.00
> 00:48:53 3 0.00 0.00 0.00 0.00 0.00 0.00 0.00 0.00 100.00
> 00:48:53 4 0.00 0.00 0.00 0.00 0.00 0.00 0.00 0.00 100.00
> 00:48:53 5 0.00 0.00 0.00 0.00 0.00 0.00 0.00 0.00 100.00
> 00:48:53 6 0.00 0.00 0.00 0.00 0.00 0.00 0.00 0.00 100.00
> 00:48:53 7 0.00 0.00 0.00 0.00 0.00 0.00 0.00 0.00 100.00
>
> and after this two second of something like this:
> 00:48:49 CPU %usr %nice %sys %iowait %irq %soft %steal %guest %idle
> 00:48:50 all 0.00 0.00 0.09 0.00 0.27 2.17 0.00 0.00 97.46
> 00:48:50 0 0.00 0.00 0.00 0.00 0.00 0.00 0.00 0.00 100.00
> 00:48:50 1 0.00 0.00 0.00 0.00 1.96 6.86 0.00 0.00 91.18
> 00:48:50 2 0.00 0.00 0.00 0.00 0.99 16.83 0.00 0.00 82.18
> 00:48:50 3 0.00 0.00 0.00 0.00 0.00 0.00 0.00 0.00 100.00
> 00:48:50 4 0.00 0.00 0.00 0.00 0.00 0.00 0.00 0.00 100.00
> 00:48:50 5 0.00 0.00 0.00 0.00 0.00 0.00 0.00 0.00 100.00
> 00:48:50 6 0.00 0.00 0.00 0.00 0.00 0.00 0.00 0.00 100.00
> 00:48:50 7 0.00 0.00 0.00 0.00 0.00 0.00 0.00 0.00 100.00
>
> 00:48:50 CPU %usr %nice %sys %iowait %irq %soft %steal %guest %idle
> 00:48:51 all 0.00 0.00 0.00 0.00 1.86 10.41 0.00 0.00 87.73
> 00:48:51 0 0.00 0.00 0.00 0.00 0.00 1.00 0.00 0.00 99.00
> 00:48:51 1 0.00 0.00 0.00 0.00 4.85 26.21 0.00 0.00 68.93
> 00:48:51 2 0.00 0.00 1.00 0.00 5.00 29.00 0.00 0.00 65.00
> 00:48:51 3 0.00 0.00 0.00 0.00 0.00 0.00 0.00 0.00 100.00
> 00:48:51 4 0.00 0.00 0.00 0.00 0.00 0.00 0.00 0.00 100.00
> 00:48:51 5 0.00 0.00 0.00 0.00 0.00 0.00 0.00 0.00 100.00
> 00:48:51 6 0.00 0.00 0.00 0.00 0.00 0.00 0.00 0.00 100.00
> 00:48:51 7 0.00 0.00 0.00 0.00 0.00 0.00 0.00 0.00 100.00
>
Could you remind us how it differs from 2.6.28 with the same settings?
>
> Another test:
>
> gc_timeout = 1
> rt_cache_rebuild_count = 100
> 10 to 14 sec of something like this:
> 00:51:36 CPU %usr %nice %sys %iowait %irq %soft %steal %guest %idle
> 00:51:37 all 0.00 0.00 0.00 0.00 0.00 0.27 0.00 0.00 99.73
> 00:51:37 0 0.00 0.00 0.00 0.00 0.00 0.00 0.00 0.00 100.00
> 00:51:37 1 0.00 0.00 0.00 0.00 0.00 2.00 0.00 0.00 98.00
> 00:51:37 2 0.00 0.00 0.00 0.00 0.00 1.00 0.00 0.00 99.00
> 00:51:37 3 0.00 0.00 0.00 0.00 0.00 0.00 0.00 0.00 100.00
> 00:51:37 4 0.00 0.00 0.00 0.00 0.00 0.00 0.00 0.00 100.00
> 00:51:37 5 0.00 0.00 0.00 0.00 0.00 0.00 0.00 0.00 100.00
> 00:51:37 6 0.00 0.00 0.00 0.00 0.00 0.00 0.00 0.00 100.00
> 00:51:37 7 0.00 0.00 0.00 0.00 0.00 0.00 0.00 0.00 100.00
>
> and two seconds of 10 to 30% cpu load more
>
>
> 2).
> Only last patch and almost all the time output like this
> 00:59:49 CPU %usr %nice %sys %iowait %irq %soft %steal %guest %idle
> 00:59:50 all 0.00 0.00 0.13 0.00 1.73 8.00 0.00 0.00 90.13
> 00:59:50 0 0.00 0.00 0.00 0.00 0.00 0.00 0.00 0.00 100.00
> 00:59:50 1 0.00 0.00 0.00 0.00 4.00 24.00 0.00 0.00 72.00
> 00:59:50 2 0.00 0.00 0.00 0.00 8.91 34.65 0.00 0.00 56.44
> 00:59:50 3 0.00 0.00 0.00 0.00 0.00 0.00 0.00 0.00 100.00
> 00:59:50 4 0.00 0.00 0.00 0.00 0.00 0.00 0.00 0.00 100.00
> 00:59:50 5 0.00 0.00 0.00 0.00 0.00 0.00 0.00 0.00 100.00
> 00:59:50 6 0.00 0.00 0.00 0.00 0.00 0.00 0.00 0.00 100.00
> 00:59:50 7 0.00 0.00 0.00 0.00 0.00 0.00 0.00 0.00 100.00
>
> sometimes after 15 to 30 sec i have 1 to 2% cpu load
And how long do you have this 1 to 2% load? Is it with:
rt_cache_rebuild_count = 0
gc_timeout = 1?
Maybe you could describe the main difference with or without the first
patch?
>
> 3).
>
> with default settings and without this patch i have almost all the time output like this:
You mean without these two patches, right? So, there is no breaks with
less load like above?
> 01:21:40 CPU %usr %nice %sys %iowait %irq %soft %steal %guest %idle
> 01:21:41 all 0.00 0.00 0.00 0.00 2.14 10.97 0.00 0.00 86.89
> 01:21:41 0 0.00 0.00 0.00 0.00 0.00 0.00 0.00 0.00 100.00
> 01:21:41 1 0.00 0.00 0.00 0.00 6.93 34.65 0.00 0.00 58.42
> 01:21:41 2 0.00 0.00 0.00 0.00 7.07 42.42 0.00 0.00 50.51
> 01:21:41 3 0.00 0.00 0.00 0.00 0.00 0.00 0.00 0.00 100.00
> 01:21:41 4 0.00 0.00 0.00 0.00 0.00 0.00 0.00 0.00 100.00
> 01:21:41 5 0.00 0.00 0.00 0.00 0.00 0.00 0.00 0.00 100.00
> 01:21:41 6 0.00 0.00 0.00 0.00 0.00 0.00 0.00 0.00 100.00
> 01:21:41 7 0.00 0.00 0.00 0.00 0.00 0.00 0.00 0.00 100.00
>
>
>
> with my settings:
> /proc/sys/net/ipv4/route/error_burst:1250
> /proc/sys/net/ipv4/route/error_cost:250
> /proc/sys/net/ipv4/route/gc_elasticity:4
> /proc/sys/net/ipv4/route/gc_interval:15
> /proc/sys/net/ipv4/route/gc_min_interval:0
> /proc/sys/net/ipv4/route/gc_min_interval_ms:0
> /proc/sys/net/ipv4/route/gc_thresh:190536
> /proc/sys/net/ipv4/route/gc_timeout:15
> /proc/sys/net/ipv4/route/max_size:1524288
> /proc/sys/net/ipv4/route/min_adv_mss:256
> /proc/sys/net/ipv4/route/min_pmtu:552
> /proc/sys/net/ipv4/route/mtu_expires:600
> /proc/sys/net/ipv4/route/redirect_load:5
> /proc/sys/net/ipv4/route/redirect_number:9
> /proc/sys/net/ipv4/route/redirect_silence:5120
> /proc/sys/net/ipv4/route/secret_interval:3600
>
>
> 15 sec of 30 to 50 % cpu and 15 sec 1 to 2 % cpu
>
> with /proc/sys/net/ipv4/route/gc_interval:1
> almost all the time like this
> 01:23:45 CPU %usr %nice %sys %iowait %irq %soft %steal %guest %idle
> 01:23:46 all 0.00 0.00 0.00 0.00 0.00 0.12 0.00 0.00 99.88
> 01:23:46 0 0.00 0.00 0.00 0.00 0.00 0.00 0.00 0.00 100.00
> 01:23:46 1 0.00 0.00 0.00 0.00 1.00 0.00 0.00 0.00 99.00
> 01:23:46 2 0.00 0.00 0.00 0.00 0.00 1.02 0.00 0.00 98.98
> 01:23:46 3 0.00 0.00 0.00 0.00 0.00 0.00 0.00 0.00 100.00
> 01:23:46 4 0.00 0.00 0.00 0.00 0.00 0.00 0.00 0.00 100.00
> 01:23:46 5 0.00 0.00 0.00 0.00 0.00 0.00 0.00 0.00 100.00
> 01:23:46 6 0.00 0.00 0.00 0.00 0.00 0.00 0.00 0.00 100.00
> 01:23:46 7 0.00 0.00 0.00 0.00 0.00 0.00 0.00 0.00 100.00
>
> with max two outputs of 20 to 30% cpu in different times from 12 to 15sec
Didn't you see any: "... rebuilds is over limit, route caching
disabled" warning?
>
>
> And i dont know but i think patch for turning off route cache is not
> working because with this patches and rt_cache_rebuild_count = 0
If you mean the patch #2, it does something opposite: with
rt_cache_rebuild_count = 0 it turns off automatic "cache disabling"
after rt_cache_rebuild_count events signaled with the above-mentionned
warning, which was introduced in 2.6.29. Sorry for not describing this
enough.
Thanks,
Jarek P.
^ permalink raw reply
* Re: [patch 1/3] net: serialize hrtimer callback in sched_cbq
From: Peter Zijlstra @ 2009-07-14 16:28 UTC (permalink / raw)
To: David Miller; +Cc: tglx, netdev, linux-kernel, kaber, Linus Torvalds
In-Reply-To: <20090714.090055.56906831.davem@davemloft.net>
On Tue, 2009-07-14 at 09:00 -0700, David Miller wrote:
> From: Thomas Gleixner <tglx@linutronix.de>
> Date: Tue, 14 Jul 2009 10:55:14 +0200 (CEST)
> > On Sun, 12 Jul 2009, David Miller wrote:
> >
> >> What should probably happen is that the hrtimer merely fires off work
> >> at software interrupt context (perhaps a tasklet or similar), and that
> >> software interrupt code take the qdisc's root lock throughout it's
> >> execution.
> >
> > Sigh, I almost expected that the removal of the callback modes will
> > fire back some day.
>
> Well this makes hrtimers decidedly less useful for networking and we
> have a ton of bugs right now, basically in every hrtimer used by the
> networking currently.
>
> The only way we can use them, as things currently stand, is as
> triggers for softirq work.
>
> Is it really that troublesome to provide this kind of facility
> generically, rather than having various subsystems replicate such code
> where they want to use hrtimers and are restricted to softirqs?
Linus really hated the softirq mode, which is what prompted me to change
that.
Now, it might be he only hated the particular interface and the
resulting code, but I think to remember he simply thought the whole
thing daft.
I can look into adding it back if we can agree on the interface and code
impact, but looking at:
# git grep hrtimer_init net/ | sort -u
net/can/bcm.c: hrtimer_init(&op->thrtimer, CLOCK_MONOTONIC, HRTIMER_MODE_REL);
net/can/bcm.c: hrtimer_init(&op->timer, CLOCK_MONOTONIC, HRTIMER_MODE_REL);
net/sched/sch_api.c: hrtimer_init(&wd->timer, CLOCK_MONOTONIC, HRTIMER_MODE_ABS);
net/sched/sch_cbq.c: hrtimer_init(&q->delay_timer, CLOCK_MONOTONIC, HRTIMER_MODE_ABS);
I wonder if its worth the impact on the core kernel code, or whether its
better for these few timers to kick off a tasklet or the like.
Further, I don't think a lot of subsystems would need this, as the
general trend is away from softirqs/tasklets and towards
threads/workqueues as most people want to schedule. And for those
hardirq hrtimers are good enough as a wakeup source.
^ permalink raw reply
* Re: [PATCH] gre: fix ToS/DiffServ inherit bug
From: David Miller @ 2009-07-14 16:34 UTC (permalink / raw)
To: aj; +Cc: voyo, netdev, kuznet, kaber
In-Reply-To: <20090714153648.GC8461@urbino.open.ch>
From: Andreas Jaggi <aj@open.ch>
Date: Tue, 14 Jul 2009 17:36:48 +0200
> Fixes two bugs:
> - ToS/DiffServ inheritance was unintentionally activated when using impair fixed ToS values
> - ECN bit was lost during ToS/DiffServ inheritance
>
> Signed-off-by: Andreas Jaggi <aj@open.ch>
...
> tos = tiph->tos;
> - if (tos&1) {
> + if (tos == 1) {
> + tos = 0;
> if (skb->protocol == htons(ETH_P_IP))
> tos = old_iph->tos;
> - tos &= ~1;
> }
Note that this only works out because one wouldn't specify ECN
bits in the TOS to use for the tunnel.
But it demonstrates the outright danger of trying to use
"undefined" flag bits in protocol header fields to pass
private control flags into the driver from user interfaces.
^ permalink raw reply
* Re: [PATCH] gre: copy ToS/DiffServ bits to outer IP header
From: David Miller @ 2009-07-14 16:35 UTC (permalink / raw)
To: aj; +Cc: netdev, kuznet, kaber
In-Reply-To: <20090714071410.GA4793@urbino.open.ch>
From: Andreas Jaggi <aj@open.ch>
Date: Tue, 14 Jul 2009 09:14:10 +0200
> On Mon, Jul 13, 2009 at 10:44:33AM -0700, David Miller wrote:
>> Why not just create a new IFLA_GRE_* attribute to pass in
>> flags like this that purely are private to the GRE Linux
>> tunnel driver and have nothing to do with values defined
>> by GRE tunnel headers at all?
>
> OK. What would be an appropriate place to store such a private flag in
> the GRE driver?
>
> I thought of extending the ip_tunnel_parms struct with '__u16 priv_flags'
> (But this would break compatibility with the iproute2 tools?)
You'll need to find a place. Worst case you can make a top-level
new structure, that contains the user interface structure and
then the new __u16 priv_flags thing. And allocate that per-tunnel
instead of what it does now.
^ permalink raw reply
* Re: [patch 1/3] net: serialize hrtimer callback in sched_cbq
From: Linus Torvalds @ 2009-07-14 16:42 UTC (permalink / raw)
To: Peter Zijlstra; +Cc: David Miller, tglx, netdev, linux-kernel, kaber
In-Reply-To: <1247588890.7500.186.camel@twins>
On Tue, 14 Jul 2009, Peter Zijlstra wrote:
>
> Linus really hated the softirq mode, which is what prompted me to change
> that.
>
> Now, it might be he only hated the particular interface and the
> resulting code, but I think to remember he simply thought the whole
> thing daft.
Yes. And I hated the bugs it had.
Don't make something as core as timers any more complicated. Don't take
locks in timers and then complain about deadlocks. If your locking is
broken, don't make the core timers be idiotically broken.
Because it was. The code was a total mess to follow, and had bugs.
Linus
^ permalink raw reply
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox