* Re: [2.6.25-rc2] e100: Trying to free already-free IRQ 11 during suspend ...
From: Kok, Auke @ 2008-02-21 18:33 UTC (permalink / raw)
To: Kok, Auke
Cc: e1000-devel, netdev, Andrew Morton, Andrey Borzenkov,
linux-kernel
In-Reply-To: <47BC725C.3060400@intel.com>
[-- Attachment #1: Type: text/plain, Size: 4383 bytes --]
Kok, Auke wrote:
> Kok, Auke wrote:
>> Andrew Morton wrote:
>>> On Sun, 17 Feb 2008 15:36:50 +0300 Andrey Borzenkov <arvidjaar@mail.ru> wrote:
>>>
>>>> ... and possibly reboot/poweroff (it flows by too fast to be legible).
>>>>
>>>> [ 8803.850634] ACPI: Preparing to enter system sleep state S3
>>>> [ 8803.853141] Suspending console(s)
>>>> [ 8805.287505] serial 00:09: disabled
>>>> [ 8805.291564] Trying to free already-free IRQ 11
>>>> [ 8805.291579] Pid: 6920, comm: pm-suspend Not tainted 2.6.25-rc2-1avb #2
>>>> [ 8805.291628] [<c0152127>] free_irq+0xb7/0x130
>>>> [ 8805.291675] [<c024bd80>] e100_suspend+0xc0/0x100
>>>> [ 8805.291724] [<c01eaa36>] pci_device_suspend+0x26/0x70
>>>> [ 8805.291747] [<c0243674>] suspend_device+0x94/0xd0
>>>> [ 8805.291763] [<c02439a3>] device_suspend+0x153/0x240
>>>> [ 8805.291784] [<c014314f>] suspend_devices_and_enter+0x4f/0xf0
>>>> [ 8805.291808] [<c0143a5f>] ? freeze_processes+0x3f/0x80
>>>> [ 8805.291825] [<c01432fa>] enter_state+0xaa/0x140
>>>> [ 8805.291840] [<c014341f>] state_store+0x8f/0xd0
>>>> [ 8805.291852] [<c0143390>] ? state_store+0x0/0xd0
>>>> [ 8805.291866] [<c01d3404>] kobj_attr_store+0x24/0x30
>>>> [ 8805.291901] [<c01b547b>] sysfs_write_file+0xbb/0x110
>>>> [ 8805.291936] [<c0177d79>] vfs_write+0x99/0x130
>>>> [ 8805.291963] [<c01b53c0>] ? sysfs_write_file+0x0/0x110
>>>> [ 8805.291979] [<c01782fd>] sys_write+0x3d/0x70
>>>> [ 8805.291998] [<c010409a>] sysenter_past_esp+0x5f/0xa5
>>>> [ 8805.292038] =======================
>>>> [ 8805.347640] ACPI: PCI interrupt for device 0000:00:06.0 disabled
>>>> [ 8805.361128] ACPI: PCI interrupt for device 0000:00:02.0 disabled
>>>> [ 8805.376670] hwsleep-0322 [00] enter_sleep_state : Entering sleep state [S3]
>>>> [ 8805.376670] Back to C!
>>>>
>>>> Interface is unused normally (only for netconsole sometimes). dmesg and config
>>>> attached.
>>> Does reverting this:
>>>
>>> commit 8543da6672b0994921f014f2250e27ae81645580
>>> Author: Auke Kok <auke-jan.h.kok@intel.com>
>>> Date: Wed Dec 12 16:30:42 2007 -0800
>>>
>>> e100: free IRQ to remove warningwhenrebooting
>>>
>>> with this patch:
>>>
>>> --- a/drivers/net/e100.c~revert-1
>>> +++ a/drivers/net/e100.c
>>> @@ -2804,9 +2804,8 @@ static int e100_suspend(struct pci_dev *
>>> pci_enable_wake(pdev, PCI_D3cold, 0);
>>> }
>>>
>>> - free_irq(pdev->irq, netdev);
>>> -
>>> pci_disable_device(pdev);
>>> + free_irq(pdev->irq, netdev);
>>> pci_set_power_state(pdev, PCI_D3hot);
>>>
>>> return 0;
>>> @@ -2848,8 +2847,6 @@ static void e100_shutdown(struct pci_dev
>>> pci_enable_wake(pdev, PCI_D3cold, 0);
>>> }
>>>
>>> - free_irq(pdev->irq, netdev);
>>> -
>>> pci_disable_device(pdev);
>>> pci_set_power_state(pdev, PCI_D3hot);
>>> }
>>> _
>>>
>>> fix it?
>>>
>>>> Hmm ... after resume device has disappeared at all ...
>>>>
>>>> {pts/1}% cat /proc/interrupts
>>>> CPU0
>>>> 0: 1290492 XT-PIC-XT timer
>>>> 1: 6675 XT-PIC-XT i8042
>>>> 2: 0 XT-PIC-XT cascade
>>>> 3: 2 XT-PIC-XT
>>>> 4: 2 XT-PIC-XT
>>>> 5: 3 XT-PIC-XT
>>>> 7: 4 XT-PIC-XT irda0
>>>> 8: 0 XT-PIC-XT rtc0
>>>> 9: 583 XT-PIC-XT acpi
>>>> 10: 2 XT-PIC-XT
>>>> 11: 31483 XT-PIC-XT yenta, yenta, yenta, ohci_hcd:usb1, ALI 5451, pcmcia0.0
>>>> 12: 28070 XT-PIC-XT i8042
>>>> 14: 21705 XT-PIC-XT ide0
>>>> 15: 82123 XT-PIC-XT ide1
>>>> NMI: 0 Non-maskable interrupts
>>>> TRM: 0 Thermal event interrupts
>>>> SPU: 0 Spurious interrupts
>>>> ERR: 0
>>> I hope that's not a separate bug...
>> I'll take a look at this as well. thanks for reporting.
>
> ok, I just had a repro - on a regular shutdown even.
>
> this always worked before - I'm not blaming anything yet but something in the pci
> shutdown code must now be freeing our irq for us (I'm not using anything fancy to
> autoconfigure my network here).
>
> I definately do not see this with 2.6.24 either.
can you try this patch please? It rewrites e100 to do suspend/shutdown just like
e1000e does, which is much more tested than anything else. I don't get the IRQ
message anymore - but I haven't gotten to testing suspend/resume just yet.
Auke
[-- Attachment #2: Type: text/plain, Size: 228 bytes --]
-------------------------------------------------------------------------
This SF.net email is sponsored by: Microsoft
Defy all challenges. Microsoft(R) Visual Studio 2008.
http://clk.atdmt.com/MRT/go/vse0120000070mrt/direct/01/
[-- Attachment #3: Type: text/plain, Size: 164 bytes --]
_______________________________________________
E1000-devel mailing list
E1000-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/e1000-devel
^ permalink raw reply
* Re: New sparse warning in net/mac80211/debugfs_sta.c
From: Patrick McHardy @ 2008-02-21 18:15 UTC (permalink / raw)
To: Joe Perches; +Cc: Johannes Berg, David Miller, harvey.harrison, netdev
In-Reply-To: <47BDBC23.5080007@trash.net>
Patrick McHardy wrote:
> Joe Perches wrote:
>>
>> This removes the __pure from print_mac, so reject as appropriate...
>>
>> Add some type safety to print_mac by using struct print_mac_buf *
>> instead of char *.
>
> And adds back the overhead of two completely unnecessary
> function calls to the VLAN fastpath.
BTW, this also affects ATM, with 3 calls in hard_start_xmit,
3 calls in lec_xmit and 1 cakk in lec_atm_send.
> How about just
> stopping this idiocy and reverting the appropriate patches
> to bring back MAC_FMT and use it where appropriate?
^ permalink raw reply
* Re: New sparse warning in net/mac80211/debugfs_sta.c
From: Patrick McHardy @ 2008-02-21 18:00 UTC (permalink / raw)
To: Joe Perches; +Cc: Johannes Berg, David Miller, harvey.harrison, netdev
In-Reply-To: <1203616486.7181.269.camel@localhost>
Joe Perches wrote:
> On Thu, 2008-02-21 at 11:17 +0100, Johannes Berg wrote:
>
>> Yeah, I saw that discussion. I think it's fine, it's just something we
>> need to be aware of. In fact, I Joe had a patch (that seems to have
>> gotten lost?) to make DECLARE_MAC_BUF() declare a structure with the u8
>> pointer in it instead to get type checking for the args, which would
>> make our code there not even compile, and imho rightfully so. I'll send
>> in a patch to fix this (via John) and Joe can resend his patch to get
>> typechecking there.
>>
>
> This removes the __pure from print_mac, so reject as appropriate...
>
> Add some type safety to print_mac by using
> struct print_mac_buf * instead of char *.
And adds back the overhead of two completely unnecessary
function calls to the VLAN fastpath. How about just
stopping this idiocy and reverting the appropriate patches
to bring back MAC_FMT and use it where appropriate?
^ permalink raw reply
* [PATCH] alloc_percpu() fails to allocate percpu data
From: Eric Dumazet @ 2008-02-21 18:00 UTC (permalink / raw)
To: David S. Miller, Andrew Morton
Cc: linux kernel, netdev, Christoph Lameter, Zhang, Yanmin
[-- Attachment #1: Type: text/plain, Size: 1365 bytes --]
Some oprofile results obtained while using tbench on a 2x2 cpu machine
were very surprising.
For example, loopback_xmit() function was using high number of cpu
cycles to perform
the statistic updates, supposed to be real cheap since they use percpu data
pcpu_lstats = netdev_priv(dev);
lb_stats = per_cpu_ptr(pcpu_lstats, smp_processor_id());
lb_stats->packets++; /* HERE : serious contention */
lb_stats->bytes += skb->len;
struct pcpu_lstats is a small structure containing two longs. It appears
that on my 32bits platform,
alloc_percpu(8) allocates a single cache line, instead of giving to
each cpu a separate
cache line.
Using the following patch gave me impressive boost in various benchmarks
( 6 % in tbench)
(all percpu_counters hit this bug too)
Long term fix (ie >= 2.6.26) would be to let each CPU allocate their own
block of memory, so that we
dont need to roudup sizes to L1_CACHE_BYTES, or merging the SGI stuff of
course...
Note : SLUB vs SLAB is important here to *show* the improvement, since
they dont have the same minimum
allocation sizes (8 bytes vs 32 bytes).
This could very well explain regressions some guys reported when they
switched to SLUB.
Signed-off-by: Eric Dumazet <dada1@cosmosbay.com>
mm/allocpercpu.c | 15 ++++++++++++++-
1 files changed, 14 insertions(+), 1 deletion(-)
[-- Attachment #2: percpu_populate.patch --]
[-- Type: text/plain, Size: 1229 bytes --]
diff --git a/mm/allocpercpu.c b/mm/allocpercpu.c
index 7e58322..b0012e2 100644
--- a/mm/allocpercpu.c
+++ b/mm/allocpercpu.c
@@ -6,6 +6,10 @@
#include <linux/mm.h>
#include <linux/module.h>
+#ifndef cache_line_size
+#define cache_line_size() L1_CACHE_BYTES
+#endif
+
/**
* percpu_depopulate - depopulate per-cpu data for given cpu
* @__pdata: per-cpu data to depopulate
@@ -52,6 +56,11 @@ void *percpu_populate(void *__pdata, size_t size, gfp_t gfp, int cpu)
struct percpu_data *pdata = __percpu_disguise(__pdata);
int node = cpu_to_node(cpu);
+ /*
+ * We should make sure each CPU gets private memory.
+ */
+ size = roundup(size, cache_line_size());
+
BUG_ON(pdata->ptrs[cpu]);
if (node_online(node))
pdata->ptrs[cpu] = kmalloc_node(size, gfp|__GFP_ZERO, node);
@@ -98,7 +107,11 @@ EXPORT_SYMBOL_GPL(__percpu_populate_mask);
*/
void *__percpu_alloc_mask(size_t size, gfp_t gfp, cpumask_t *mask)
{
- void *pdata = kzalloc(nr_cpu_ids * sizeof(void *), gfp);
+ /*
+ * We allocate whole cache lines to avoid false sharing
+ */
+ size_t sz = roundup(nr_cpu_ids * sizeof(void *), cache_line_size());
+ void *pdata = kzalloc(sz, gfp);
void *__pdata = __percpu_disguise(pdata);
if (unlikely(!pdata))
^ permalink raw reply related
* Re: New sparse warning in net/mac80211/debugfs_sta.c
From: Joe Perches @ 2008-02-21 17:54 UTC (permalink / raw)
To: Johannes Berg; +Cc: David Miller, harvey.harrison, netdev
In-Reply-To: <1203589042.17534.145.camel@johannes.berg>
On Thu, 2008-02-21 at 11:17 +0100, Johannes Berg wrote:
> Yeah, I saw that discussion. I think it's fine, it's just something we
> need to be aware of. In fact, I Joe had a patch (that seems to have
> gotten lost?) to make DECLARE_MAC_BUF() declare a structure with the u8
> pointer in it instead to get type checking for the args, which would
> make our code there not even compile, and imho rightfully so. I'll send
> in a patch to fix this (via John) and Joe can resend his patch to get
> typechecking there.
This removes the __pure from print_mac, so reject as appropriate...
Add some type safety to print_mac by using
struct print_mac_buf * instead of char *.
Signed-off-by: Joe Perches <joe@perches.com>
diff --git a/include/linux/if_ether.h b/include/linux/if_ether.h
index 42dc6a3..2f8df76 100644
--- a/include/linux/if_ether.h
+++ b/include/linux/if_ether.h
@@ -129,9 +129,16 @@ extern ssize_t sysfs_format_mac(char *buf, const unsigned char *addr, int len);
/*
* Display a 6 byte device address (MAC) in a readable format.
*/
-extern __pure char *print_mac(char *buf, const unsigned char *addr);
-#define MAC_BUF_SIZE 18
-#define DECLARE_MAC_BUF(var) char var[MAC_BUF_SIZE] __maybe_unused
+
+struct print_mac_buf {
+ char formatted_mac_addr[18];
+};
+
+#define DECLARE_MAC_BUF(var) \
+ struct print_mac_buf __maybe_unused _##var; \
+ struct print_mac_buf __maybe_unused *var = &_##var
+
+extern char *print_mac(struct print_mac_buf *buf, const unsigned char *addr);
#endif
diff --git a/net/ethernet/eth.c b/net/ethernet/eth.c
index a7b4175..ce607ab 100644
--- a/net/ethernet/eth.c
+++ b/net/ethernet/eth.c
@@ -384,9 +384,10 @@ ssize_t sysfs_format_mac(char *buf, const unsigned char *addr, int len)
}
EXPORT_SYMBOL(sysfs_format_mac);
-char *print_mac(char *buf, const unsigned char *addr)
+char *print_mac(struct print_mac_buf *buf, const unsigned char *addr)
{
- _format_mac_addr(buf, MAC_BUF_SIZE, addr, ETH_ALEN);
- return buf;
+ _format_mac_addr(buf->formatted_mac_addr,
+ sizeof(buf->formatted_mac_addr), addr, ETH_ALEN);
+ return buf->formatted_mac_addr;
}
EXPORT_SYMBOL(print_mac);
^ permalink raw reply related
* Re: New sparse warning in net/mac80211/debugfs_sta.c
From: Patrick McHardy @ 2008-02-21 17:45 UTC (permalink / raw)
To: David Miller; +Cc: harvey.harrison, joe, netdev
In-Reply-To: <20080221.020554.259219477.davem@davemloft.net>
David Miller wrote:
> From: Harvey Harrison <harvey.harrison@gmail.com>
> Date: Thu, 21 Feb 2008 02:01:19 -0800
>
>> In this case, it's being passed to a debugfs create function, could it
>> instead use sysfs_format_mac?
>
> Just assigning print_mac() to a local variable then passing that to
> debugfs_create_dir() will make the warning go away.
>
>>From another perspective adding that __pure attribute to print_mac()
> might not have been the best idea. But I can't think of another
> way to elimitate the "passing print_mac() args to pr_debug()
> still generates calls to print_mac() even when DEBUG is not
> defined" problem :-/
Frankly, I think the main problem is that MAC_FMT got removed
for no real reason and is forcing us to come up with lots of
workarounds. We have NIPQUAD_FMT, NIP6_FMT, so I don't see
whats wrong with MAC_FMT. In fact I think a simple
printk(MAC_FMT, addr);
is much nicer than all this temporary buffer and function
call stuff.
^ permalink raw reply
* pull request: wireless-2.6.26 2008-02-21
From: John W. Linville @ 2008-02-21 17:25 UTC (permalink / raw)
To: davem-fT/PcQaiUtIeIZ0/mPfg9Q
Cc: netdev-u79uwXL29TY76Z2rM5mHXA,
linux-wireless-u79uwXL29TY76Z2rM5mHXA
In-Reply-To: <20080221021530.GG3825-2XuSBdqkA4R54TAoqtyWWQ@public.gmane.org>
On Wed, Feb 20, 2008 at 09:15:30PM -0500, John W. Linville wrote:
> Here are a slew of developments intended for 2.6.26. The patches are
> too diverse to comment upon in this summary... :-)
David,
This is additive on top of the pull request from last night. Only
Pavel's fix for the brown-paper-bag CONFIG_MAC80211_IBSS_DEBUG build
issue is applied on top of the previous commits.
Please let me know if there are problems!
Thanks,
John
---
The following changes since commit 4d161f9d7f3f3ce069006b53dc8a84ed8a33753f:
Johannes Berg (1):
p54: fix sparse warnings
are available in the git repository at:
git://git.kernel.org/pub/scm/linux/kernel/git/linville/wireless-2.6.git
Pavel Roskin (1):
mac80211: fix incorrect use of CONFIG_MAC80211_IBSS_DEBUG
net/mac80211/ieee80211_sta.c | 4 +++-
1 files changed, 3 insertions(+), 1 deletions(-)
diff --git a/net/mac80211/ieee80211_sta.c b/net/mac80211/ieee80211_sta.c
index 05cc908..453d14a 100644
--- a/net/mac80211/ieee80211_sta.c
+++ b/net/mac80211/ieee80211_sta.c
@@ -2452,10 +2452,12 @@ static void ieee80211_rx_bss_info(struct net_device *dev,
jiffies);
#endif /* CONFIG_MAC80211_IBSS_DEBUG */
if (beacon_timestamp > rx_timestamp) {
- if (CONFIG_MAC80211_IBSS_DEBUG || net_ratelimit())
+#ifdef CONFIG_MAC80211_IBSS_DEBUG
+ if (net_ratelimit())
printk(KERN_DEBUG "%s: beacon TSF higher than "
"local TSF - IBSS merge with BSSID %s\n",
dev->name, print_mac(mac, mgmt->bssid));
+#endif /* CONFIG_MAC80211_IBSS_DEBUG */
ieee80211_sta_join_ibss(dev, &sdata->u.sta, bss);
ieee80211_ibss_add_sta(dev, NULL,
mgmt->bssid, mgmt->sa);
--
John W. Linville
linville-2XuSBdqkA4R54TAoqtyWWQ@public.gmane.org
^ permalink raw reply related
* Re: New sparse warning in net/mac80211/debugfs_sta.c
From: Joe Perches @ 2008-02-21 17:17 UTC (permalink / raw)
To: David Miller; +Cc: harvey.harrison, netdev, LKML
In-Reply-To: <20080221.020554.259219477.davem@davemloft.net>
On Thu, 2008-02-21 at 02:05 -0800, David Miller wrote:
> >From another perspective adding that __pure attribute to print_mac()
> might not have been the best idea. But I can't think of another
> way to eliminate the "passing print_mac() args to pr_debug()
> still generates calls to print_mac() even when DEBUG is not
> defined" problem :-/
Philip Craig made a suggestion:
http://marc.info/?l=linux-wireless&m=120338413108120&w=2
to use this:
If not debugging, guard #define wrapper_printks with if (0).
The gcc optimizer removes the printk but gcc still does
printf argument verification.
There are many other debug printk wrappers
#ifdef DEBUG
#define printk_wrapper
#else
#define printk_wrapper printk
#endif
that should be converted as well.
Signed-off-by: Joe Perches <joe@perches.com>
diff --git a/include/linux/device.h b/include/linux/device.h
index 2258d89..79601b1 100644
--- a/include/linux/device.h
+++ b/include/linux/device.h
@@ -608,21 +608,15 @@ extern const char *dev_driver_string(struct device *dev);
#define dev_dbg(dev, format, arg...) \
dev_printk(KERN_DEBUG , dev , format , ## arg)
#else
-static inline int __attribute__ ((format (printf, 2, 3)))
-dev_dbg(struct device *dev, const char *fmt, ...)
-{
- return 0;
-}
+#define dev_dbg(dev, format, arg...) \
+ do { if (0) dev_printk(KERN_DEBUG , dev , format, ## arg); } while (0)
#endif
#ifdef VERBOSE_DEBUG
#define dev_vdbg dev_dbg
#else
-static inline int __attribute__ ((format (printf, 2, 3)))
-dev_vdbg(struct device *dev, const char *fmt, ...)
-{
- return 0;
-}
+#define dev_vdbg(dev, format, arg...) \
+ do { if (0) dev_printk(KERN_DEBUG , dev , format, ## arg); } while (0)
#endif
/* Create alias, so I can be autoloaded. */
diff --git a/include/linux/kernel.h b/include/linux/kernel.h
index 2df44e7..cd24112 100644
--- a/include/linux/kernel.h
+++ b/include/linux/kernel.h
@@ -293,10 +293,8 @@ extern void print_hex_dump_bytes(const char *prefix_str, int prefix_type,
#define pr_debug(fmt, arg...) \
printk(KERN_DEBUG fmt, ##arg)
#else
-static inline int __attribute__ ((format (printf, 1, 2))) pr_debug(const char * fmt, ...)
-{
- return 0;
-}
+#define pr_debug(fmt, arg...) \
+ do { if (0) printk(KERN_DEBUG fmt, ##arg); } while (0)
#endif
/*
^ permalink raw reply related
* Re: [PATCH][PPPOL2TP]: Fix SMP oops in pppol2tp driver
From: Jarek Poplawski @ 2008-02-21 17:09 UTC (permalink / raw)
To: James Chapman; +Cc: David Miller, Paul Mackerras, netdev
In-Reply-To: <20080221120829.GB12944@ff.dom.local>
Jarek Poplawski wrote, On 02/21/2008 01:08 PM:
...
> Another, probably simpler way would be to move almost all pppol2tp_xmit
...
Actually, the simplest off all seems to be now this old idea to maybe make
sk_dst_lock globally softirq immune. At least I think it's worth of testing,
to check for these other possible lockdep warnings. It should only need to
change all write_ and read_lock(&sk->sk_dst_lock) in very few places:
include/net/sock.h, include/net/ip6_route.h, and net/ipv6/ipv6_sockglue.c.
This could be tested together with you full _bh locking patch (maybe except
these other changes in pppol2tp_xmit).
Jarek P.
^ permalink raw reply
* [PATCH] IPv6 Add more initializations of the new nl_info.nl_net field
From: benjamin.thery @ 2008-02-21 16:51 UTC (permalink / raw)
To: David Miller; +Cc: Linux Netdev List, Benjamin Thery
In-Reply-To: <20080221165149.928863399@theryb.frec.bull.fr>
[-- Attachment #1: ipv6-initialize-new-nl_info.nl_net-field.patch --]
[-- Type: text/plain, Size: 2306 bytes --]
Add more missing initializations of the new nl_info.nl_net field in
IPv6 stack.
This field will be used when network namespaces are fully supported.
Signed-off-by: Benjamin Thery <benjamin.thery@bull.net>
---
net/ipv6/addrconf.c | 9 +++++++++
net/ipv6/route.c | 6 ++++++
2 files changed, 15 insertions(+)
Index: net-2.6.26/net/ipv6/addrconf.c
===================================================================
--- net-2.6.26.orig/net/ipv6/addrconf.c
+++ net-2.6.26/net/ipv6/addrconf.c
@@ -1557,6 +1557,9 @@ addrconf_prefix_route(struct in6_addr *p
.fc_expires = expires,
.fc_dst_len = plen,
.fc_flags = RTF_UP | flags,
+ .fc_nlinfo.pid = 0,
+ .fc_nlinfo.nlh = NULL,
+ .fc_nlinfo.nl_net = &init_net,
};
ipv6_addr_copy(&cfg.fc_dst, pfx);
@@ -1583,6 +1586,9 @@ static void addrconf_add_mroute(struct n
.fc_ifindex = dev->ifindex,
.fc_dst_len = 8,
.fc_flags = RTF_UP,
+ .fc_nlinfo.pid = 0,
+ .fc_nlinfo.nlh = NULL,
+ .fc_nlinfo.nl_net = &init_net,
};
ipv6_addr_set(&cfg.fc_dst, htonl(0xFF000000), 0, 0, 0);
@@ -1599,6 +1605,9 @@ static void sit_route_add(struct net_dev
.fc_ifindex = dev->ifindex,
.fc_dst_len = 96,
.fc_flags = RTF_UP | RTF_NONEXTHOP,
+ .fc_nlinfo.pid = 0,
+ .fc_nlinfo.nlh = NULL,
+ .fc_nlinfo.nl_net = &init_net,
};
/* prefix length - 96 bits "::d.d.d.d" */
Index: net-2.6.26/net/ipv6/route.c
===================================================================
--- net-2.6.26.orig/net/ipv6/route.c
+++ net-2.6.26/net/ipv6/route.c
@@ -604,6 +604,8 @@ static int __ip6_ins_rt(struct rt6_info
int ip6_ins_rt(struct rt6_info *rt)
{
struct nl_info info = {
+ .pid = 0,
+ .nlh = NULL,
.nl_net = &init_net,
};
return __ip6_ins_rt(rt, &info);
@@ -1264,6 +1266,8 @@ static int __ip6_del_rt(struct rt6_info
int ip6_del_rt(struct rt6_info *rt)
{
struct nl_info info = {
+ .pid = 0,
+ .nlh = NULL,
.nl_net = &init_net,
};
return __ip6_del_rt(rt, &info);
@@ -1719,6 +1723,8 @@ static void rtmsg_to_fib6_config(struct
cfg->fc_src_len = rtmsg->rtmsg_src_len;
cfg->fc_flags = rtmsg->rtmsg_flags;
+ cfg->fc_nlinfo.nl_net = &init_net;
+
ipv6_addr_copy(&cfg->fc_dst, &rtmsg->rtmsg_dst);
ipv6_addr_copy(&cfg->fc_src, &rtmsg->rtmsg_src);
ipv6_addr_copy(&cfg->fc_gateway, &rtmsg->rtmsg_gateway);
--
^ permalink raw reply
* Re: [PATCH] macb: Fix speed setting
From: Haavard Skinnemoen @ 2008-02-21 16:39 UTC (permalink / raw)
To: Atsushi Nemoto; +Cc: jeff, linux-kernel, netdev
In-Reply-To: <20080222.000731.18312290.anemo@mba.ocn.ne.jp>
On Fri, 22 Feb 2008 00:07:31 +0900 (JST)
Atsushi Nemoto <anemo@mba.ocn.ne.jp> wrote:
> > I'm willing to take your word for it, but some documentation would be
> > really nice...
>
> Well, simple grepping drivers/net/phy enlighten us ;)
Yeah, the patch certainly looks correct as far as I can tell.
> Anyway, this patch fixes problem with 10Mbps on AT91SAM9260 board.
Thanks. I'll pass it on.
Haavard
^ permalink raw reply
* Re: 2.6.25-rc2-mm1 - several bugs and a crash
From: Patrick McHardy @ 2008-02-21 16:34 UTC (permalink / raw)
To: Stephen Hemminger
Cc: Tilman Schmidt, Andrew Morton, linux-kernel, netdev,
netfilter-devel
In-Reply-To: <20080221083248.5b76b496@extreme>
Stephen Hemminger wrote:
> On Thu, 21 Feb 2008 12:28:50 +0100
> Patrick McHardy <kaber@trash.net> wrote:
>
>
>
>> diff --git a/net/netfilter/nf_conntrack_core.c b/net/netfilter/nf_conntrack_core.c
>> index 327e847..b77eb56 100644
>> --- a/net/netfilter/nf_conntrack_core.c
>> +++ b/net/netfilter/nf_conntrack_core.c
>> @@ -256,13 +256,19 @@ __nf_conntrack_find(const struct nf_conntrack_tuple *tuple)
>> struct hlist_node *n;
>> unsigned int hash = hash_conntrack(tuple);
>>
>> + /* Disable BHs the entire time since we normally need to disable them
>> + * at least once for the stats anyway.
>> + */
>> + local_bh_disable();
>>
>
> Use rcu_read_lock instead. local_bh_disable() won't work with some of the other forms
> of RCU alternatives.
>
The caller already calls rcu_read_lock(). This is for the per-cpu
statistics.
^ permalink raw reply
* Re: 2.6.25-rc2-mm1 - several bugs and a crash
From: Stephen Hemminger @ 2008-02-21 16:32 UTC (permalink / raw)
To: Patrick McHardy
Cc: Tilman Schmidt, Andrew Morton, linux-kernel, netdev,
netfilter-devel
In-Reply-To: <47BD6072.4000407@trash.net>
On Thu, 21 Feb 2008 12:28:50 +0100
Patrick McHardy <kaber@trash.net> wrote:
> Tilman Schmidt wrote:
> > Still, X came up fine, I could log in (Gnome feeling subjectively
> > a bit sluggish), call up a web page from the Internet in Firefox,
> > and start perusing the logs, when the whole system froze: neither
> > mouse nor keyboard would react anymore, and only the Wind^Wreset
> > button would put me back in control. After rebooting into the
> > previous, non-mm kernel I found this in the syslog:
> >
> > Feb 20 17:22:40 xenon kernel: [ 48.180297] BUG: using smp_processor_id() in preemptible [00000000] code: ntpdate/3562
> > Feb 20 17:22:40 xenon kernel: [ 48.180297] caller is __nf_conntrack_find+0x9b/0xeb [nf_conntrack]
> > Feb 20 17:22:40 xenon kernel: [ 48.180297] Pid: 3562, comm: ntpdate Not tainted 2.6.25-rc2-mm1-testing #1
> > Feb 20 17:22:40 xenon kernel: [ 48.180297] [<c02015b9>] debug_smp_processor_id+0x99/0xb0
>
>
> Could you test whether this patch fixes the netfilter
> warnings please?
>
> diff --git a/net/netfilter/nf_conntrack_core.c b/net/netfilter/nf_conntrack_core.c
> index 327e847..b77eb56 100644
> --- a/net/netfilter/nf_conntrack_core.c
> +++ b/net/netfilter/nf_conntrack_core.c
> @@ -256,13 +256,19 @@ __nf_conntrack_find(const struct nf_conntrack_tuple *tuple)
> struct hlist_node *n;
> unsigned int hash = hash_conntrack(tuple);
>
> + /* Disable BHs the entire time since we normally need to disable them
> + * at least once for the stats anyway.
> + */
> + local_bh_disable();
Use rcu_read_lock instead. local_bh_disable() won't work with some of the other forms
of RCU alternatives.
^ permalink raw reply
* Re: pull request: wireless-2.6 2008-02-20
From: John W. Linville @ 2008-02-21 16:13 UTC (permalink / raw)
To: David Miller; +Cc: netdev, linux-wireless
In-Reply-To: <20080220.221510.28377813.davem@davemloft.net>
On Wed, Feb 20, 2008 at 10:15:10PM -0800, David Miller wrote:
> > ssb: Fix the GPIO API
>
> This could have had a much better changelog, it's way too terse.
You're right. I'll try to police those better.
Thanks,
John
--
John W. Linville
linville@tuxdriver.com
^ permalink raw reply
* Re: [RFC PATCH 7/8] [SCTP]: uninline sctp_add_cmd_sf
From: Vlad Yasevich @ 2008-02-21 15:27 UTC (permalink / raw)
To: Ilpo Järvinen; +Cc: Netdev, LKML, David Miller, Arnaldo Carvalho de Melo
In-Reply-To: <Pine.LNX.4.64.0802210020350.17063@kivilampi-30.cs.helsinki.fi>
Ilpo Järvinen wrote:
> On Wed, 20 Feb 2008, Vlad Yasevich wrote:
>
>> Ilpo Järvinen wrote:
>>> I added inline to sctp_add_cmd and appropriate comment there to
>>> avoid adding another call into the call chain. This works at least
>>> with "gcc (GCC) 4.1.2 20070626 (Red Hat 4.1.2-13)". Alternatively,
>>> __sctp_add_cmd could be introduced to .h.
>>>
>> My only concern was performance regressions, but it looks like it
>> doesn't effect anything from the few quick runs I've made.
>
> There was one call made anyway, it's a bit hard to see how it would hurt
> to push that BUG() deeper down (in fact, this is one of the easiest case
> in this respect, many other cases elsewhere that need uninlining don't
> currently make any calls with inlines).
>
>> Since we are putting sctp_add_cmd_sf() on the call stack, we might
>> as well get rid of sctp_add_cmd() and reduce it a bit more.
>
> IMHO it is definately better solution for archiving the size reduction,
> I just didn't know before that the only sctp_add_cmd call could be
> converted.
That one was a really silly one. The chance of not calling BUG() in
that one case was so small, that it didn't really make any sense from
the code robustness side.
>
> [...snip...]
>> diff --git a/net/sctp/command.c b/net/sctp/command.c
>> index bb97733..3a06513 100644
>> --- a/net/sctp/command.c
>> +++ b/net/sctp/command.c
>> @@ -51,19 +51,16 @@ int sctp_init_cmd_seq(sctp_cmd_seq_t *seq)
>>
>> /* Add a command to a sctp_cmd_seq_t.
>> * Return 0 if the command sequence is full.
>> + *
>> + * Inline here is not a mistake, this way sctp_add_cmd_sf doesn't need extra
>> + * calls, size penalty is of insignificant magnitude here
>
> This won't be a necessary note anymore. :-)
>
> [...snip...]
>
Yeah. If you are going to resubmit, feel free to put my Signed-off-by line.
-vlad
^ permalink raw reply
* Re: [PATCH] macb: Fix speed setting
From: Atsushi Nemoto @ 2008-02-21 15:07 UTC (permalink / raw)
To: hskinnemoen; +Cc: jeff, linux-kernel, netdev
In-Reply-To: <20080221151246.6f2044b9@dhcp-252-066.norway.atmel.com>
Excuse me for sending broken mail...
On Thu, 21 Feb 2008 15:12:46 +0100, Haavard Skinnemoen <hskinnemoen@atmel.com> wrote:
> I have to admit that even after looking through include/linux/phy.h and
> include/linux/mii.h, I don't have the faintest idea what values we can
> expect to find in the "speed" field of phydev. The comment above struct
> phy_device says that it is used "like in mii_if_info", but struct
> mii_if_info doesn't have a "speed" field...
>
> I'm willing to take your word for it, but some documentation would be
> really nice...
Well, simple grepping drivers/net/phy enlighten us ;)
Anyway, this patch fixes problem with 10Mbps on AT91SAM9260 board.
---
Atsushi Nemoto
^ permalink raw reply
* Re: [PATCH] macb: Fix speed setting
From: Atsushi Nemoto @ 2008-02-21 15:04 UTC (permalink / raw)
To: hskinnemoen; +Cc: jeff, linux-kernel, netdev
In-Reply-To: <20080221151246.6f2044b9@dhcp-252-066.norway.atmel.com>
On Thu, 21 Feb 2008 15:12:46 +0100, Haavard Skinnemoen <hskinnemoen@atmel.com> wrote:
> I have to admit that even after looking through include/linux/phy.h and
> include/linux/mii.h, I don't have the faintest idea what values we can
> expect to find in the "speed" field of phydev. The comment above struct
> phy_device says that it is used "like in mii_if_info", but struct
> mii_if_info doesn't have a "speed" field...
>
> I'm willing to take your word for it, but some documentation would be
> really nice...
Well, simple grepping drivers/net/phy
^ permalink raw reply
* Re: [PATCH] macb: Fix speed setting
From: Haavard Skinnemoen @ 2008-02-21 14:12 UTC (permalink / raw)
To: Atsushi Nemoto; +Cc: Jeff Garzik, linux-kernel, netdev
In-Reply-To: <20080221.225054.61509660.anemo@mba.ocn.ne.jp>
On Thu, 21 Feb 2008 22:50:54 +0900 (JST)
Atsushi Nemoto <anemo@mba.ocn.ne.jp> wrote:
> Fix NCFGR.SPD setting on 10Mbps. This bug was introduced by
> conversion to generic PHY layer in kernel 2.6.23.
>
> Signed-off-by: Atsushi Nemoto <anemo@mba.ocn.ne.jp>
> ---
> diff --git a/drivers/net/macb.c b/drivers/net/macb.c
> index 81bf005..1d210ed 100644
> --- a/drivers/net/macb.c
> +++ b/drivers/net/macb.c
> @@ -148,7 +148,7 @@ static void macb_handle_link_change(struct net_device *dev)
>
> if (phydev->duplex)
> reg |= MACB_BIT(FD);
> - if (phydev->speed)
> + if (phydev->speed == SPEED_100)
I have to admit that even after looking through include/linux/phy.h and
include/linux/mii.h, I don't have the faintest idea what values we can
expect to find in the "speed" field of phydev. The comment above struct
phy_device says that it is used "like in mii_if_info", but struct
mii_if_info doesn't have a "speed" field...
I'm willing to take your word for it, but some documentation would be
really nice...
Haavard
^ permalink raw reply
* Re: [PATCH] Don't limit the number of tunnels with generic name explicitly.
From: Patrick McHardy @ 2008-02-21 12:45 UTC (permalink / raw)
To: Pavel Emelyanov; +Cc: David Miller, Linux Netdev List, devel
In-Reply-To: <47BD70B8.3030507@openvz.org>
Pavel Emelyanov wrote:
> Patrick McHardy wrote:
>> Pavel Emelyanov wrote:
>>> Patrick McHardy wrote:
>>>
>>>> It would be nicer to replace the entire hand-made name
>>>> allocation to remove the 100 device limit.
>>>>
>>> Actually, I thought the same, but fixing % in names looks like a
>>> BUG-fix for 2.6.25, while removing the hand-made name allocation
>>> looks like an enhancement for 2.6.26. No?
>>
>> Well, its so closely related that I guess it would still look
>> like a bugfix :) But changing this in 2.6.26 is also fine of
>> course, your patch just reminded me since I wanted to change
>> this for a long time and repeatedly forgot about it again.
>
> Ok, point taken ;) Here's the 2nd patch that does so. If David
> decides it can go to 2.6.25, that would be good, otherwise this
> patch will fit the 2.6.26 as well.
>
> Changelog:
>
> Use the added dev_alloc_name() call to create tunnel device name,
> rather than iterate in a hand-made loop with an artificial limit.
>
> Thanks Patrick for noticing this.
>
> Signed-off-by: Pavel Emelyanov <xemul@openvz.org>
Looks good to me, thanks.
^ permalink raw reply
* [PATCH] Don't limit the number of tunnels with generic name explicitly.
From: Pavel Emelyanov @ 2008-02-21 12:38 UTC (permalink / raw)
To: Patrick McHardy, David Miller; +Cc: Linux Netdev List, devel
In-Reply-To: <47BD6CE9.7000908@trash.net>
Patrick McHardy wrote:
> Pavel Emelyanov wrote:
>> Patrick McHardy wrote:
>>
>>> It would be nicer to replace the entire hand-made name
>>> allocation to remove the 100 device limit.
>>>
>> Actually, I thought the same, but fixing % in names looks like a
>> BUG-fix for 2.6.25, while removing the hand-made name allocation
>> looks like an enhancement for 2.6.26. No?
>
>
> Well, its so closely related that I guess it would still look
> like a bugfix :) But changing this in 2.6.26 is also fine of
> course, your patch just reminded me since I wanted to change
> this for a long time and repeatedly forgot about it again.
Ok, point taken ;) Here's the 2nd patch that does so. If David
decides it can go to 2.6.25, that would be good, otherwise this
patch will fit the 2.6.26 as well.
Changelog:
Use the added dev_alloc_name() call to create tunnel device name,
rather than iterate in a hand-made loop with an artificial limit.
Thanks Patrick for noticing this.
Signed-off-by: Pavel Emelyanov <xemul@openvz.org>
---
diff --git a/include/net/ip6_tunnel.h b/include/net/ip6_tunnel.h
index c17fa1f..6512d85 100644
--- a/include/net/ip6_tunnel.h
+++ b/include/net/ip6_tunnel.h
@@ -14,8 +14,6 @@
/* capable of receiving packets */
#define IP6_TNL_F_CAP_RCV 0x20000
-#define IP6_TNL_MAX 128
-
/* IPv6 tunnel */
struct ip6_tnl {
diff --git a/net/ipv4/ip_gre.c b/net/ipv4/ip_gre.c
index 6b9744f..e7821ba 100644
--- a/net/ipv4/ip_gre.c
+++ b/net/ipv4/ip_gre.c
@@ -259,16 +259,8 @@ static struct ip_tunnel * ipgre_tunnel_locate(struct ip_tunnel_parm *parms, int
if (parms->name[0])
strlcpy(name, parms->name, IFNAMSIZ);
- else {
- int i;
- for (i=1; i<100; i++) {
- sprintf(name, "gre%d", i);
- if (__dev_get_by_name(&init_net, name) == NULL)
- break;
- }
- if (i==100)
- goto failed;
- }
+ else
+ sprintf(name, "gre%%d");
dev = alloc_netdev(sizeof(*t), name, ipgre_tunnel_setup);
if (!dev)
@@ -292,7 +284,6 @@ static struct ip_tunnel * ipgre_tunnel_locate(struct ip_tunnel_parm *parms, int
failed_free:
free_netdev(dev);
-failed:
return NULL;
}
diff --git a/net/ipv4/ipip.c b/net/ipv4/ipip.c
index 118e7d9..dbaed69 100644
--- a/net/ipv4/ipip.c
+++ b/net/ipv4/ipip.c
@@ -221,16 +221,8 @@ static struct ip_tunnel * ipip_tunnel_locate(struct ip_tunnel_parm *parms, int c
if (parms->name[0])
strlcpy(name, parms->name, IFNAMSIZ);
- else {
- int i;
- for (i=1; i<100; i++) {
- sprintf(name, "tunl%d", i);
- if (__dev_get_by_name(&init_net, name) == NULL)
- break;
- }
- if (i==100)
- goto failed;
- }
+ else
+ sprintf(name, "tunl%%d");
dev = alloc_netdev(sizeof(*t), name, ipip_tunnel_setup);
if (dev == NULL)
@@ -254,7 +246,6 @@ static struct ip_tunnel * ipip_tunnel_locate(struct ip_tunnel_parm *parms, int c
failed_free:
free_netdev(dev);
-failed:
return NULL;
}
diff --git a/net/ipv6/ip6_tunnel.c b/net/ipv6/ip6_tunnel.c
index fa83d70..78f4388 100644
--- a/net/ipv6/ip6_tunnel.c
+++ b/net/ipv6/ip6_tunnel.c
@@ -229,18 +229,11 @@ static struct ip6_tnl *ip6_tnl_create(struct ip6_tnl_parm *p)
char name[IFNAMSIZ];
int err;
- if (p->name[0]) {
+ if (p->name[0])
strlcpy(name, p->name, IFNAMSIZ);
- } else {
- int i;
- for (i = 1; i < IP6_TNL_MAX; i++) {
- sprintf(name, "ip6tnl%d", i);
- if (__dev_get_by_name(&init_net, name) == NULL)
- break;
- }
- if (i == IP6_TNL_MAX)
- goto failed;
- }
+ else
+ sprintf(name, "ip6tnl%%d");
+
dev = alloc_netdev(sizeof (*t), name, ip6_tnl_dev_setup);
if (dev == NULL)
goto failed;
diff --git a/net/ipv6/sit.c b/net/ipv6/sit.c
index a09a6b0..1656c00 100644
--- a/net/ipv6/sit.c
+++ b/net/ipv6/sit.c
@@ -164,16 +164,8 @@ static struct ip_tunnel * ipip6_tunnel_locate(struct ip_tunnel_parm *parms, int
if (parms->name[0])
strlcpy(name, parms->name, IFNAMSIZ);
- else {
- int i;
- for (i=1; i<100; i++) {
- sprintf(name, "sit%d", i);
- if (__dev_get_by_name(&init_net, name) == NULL)
- break;
- }
- if (i==100)
- goto failed;
- }
+ else
+ sprintf(name, "sit%%d");
dev = alloc_netdev(sizeof(*t), name, ipip6_tunnel_setup);
if (dev == NULL)
^ permalink raw reply related
* Re: [PATCH] Don't create tunnels with '%' in name.
From: Patrick McHardy @ 2008-02-21 12:22 UTC (permalink / raw)
To: Pavel Emelyanov; +Cc: David Miller, Linux Netdev List, devel
In-Reply-To: <47BD6BD7.1060200@openvz.org>
Pavel Emelyanov wrote:
> Patrick McHardy wrote:
>
>> It would be nicer to replace the entire hand-made name
>> allocation to remove the 100 device limit.
>>
>
> Actually, I thought the same, but fixing % in names looks like a
> BUG-fix for 2.6.25, while removing the hand-made name allocation
> looks like an enhancement for 2.6.26. No?
Well, its so closely related that I guess it would still look
like a bugfix :) But changing this in 2.6.26 is also fine of
course, your patch just reminded me since I wanted to change
this for a long time and repeatedly forgot about it again.
^ permalink raw reply
* Re: [NETFILTER]: Introduce nf_inet_address
From: David Woodhouse @ 2008-02-21 12:19 UTC (permalink / raw)
To: Patrick McHardy; +Cc: netdev, Jan Engelhardt, David S. Miller, varekova
In-Reply-To: <47BD67DD.6020404@trash.net>
On Thu, 2008-02-21 at 13:00 +0100, Patrick McHardy wrote:
> I already sent it to Dave for 2.6.25 (I assume that what you meant,
> it was introduced after 2.6.24), its currently sitting in net-2.6
> and should hit Linus' tree next time he pulls from Dave.
I actually meant 2.6.24.x, because I thought the offending patch was in
2.6.24. I didn't realise we were building against post-2.6.24 kernel
headers in Fedora rawhide already. So that'll be fine -- thanks.
--
dwmw2
^ permalink raw reply
* Re: [PATCH] Don't create tunnels with '%' in name.
From: Pavel Emelyanov @ 2008-02-21 12:17 UTC (permalink / raw)
To: Patrick McHardy; +Cc: David Miller, Linux Netdev List, devel
In-Reply-To: <47BD6A28.1030007@trash.net>
Patrick McHardy wrote:
> Pavel Emelyanov wrote:
>> Four tunnel drivers (ip_gre, ipip, ip6_tunnel and sit) can
>> receive a pre-defined name for a device from the userspace.
>> Since these drivers call the register_netdevice() after this
>> (rtnl_lock is held), the device's name may contain a '%'
>> character.
>>
>> Not sure how bad is this to have a device with a '%' in its
>> name, but all the other places either use the register_netdev(),
>> or explicitly call dev_alloc_name() before registering, i.e.
>> do not allow for such names.
>>
>> Signed-off-by: Pavel Emelyanov <xemul@openvz.org>
>>
>> ---
>>
>> diff --git a/net/ipv4/ip_gre.c b/net/ipv4/ip_gre.c
>> index 63f6917..6b9744f 100644
>> --- a/net/ipv4/ip_gre.c
>> +++ b/net/ipv4/ip_gre.c
>> @@ -274,19 +274,24 @@ static struct ip_tunnel * ipgre_tunnel_locate(struct ip_tunnel_parm *parms, int
>> if (!dev)
>> return NULL;
>>
>> + if (strchr(name, '%')) {
>> + if (dev_alloc_name(dev, name) < 0)
>> + goto failed_free;
>> + }
>> +
>
>
> It would be nicer to replace the entire hand-made name
> allocation to remove the 100 device limit.
>
Actually, I thought the same, but fixing % in names looks like a
BUG-fix for 2.6.25, while removing the hand-made name allocation
looks like an enhancement for 2.6.26. No?
Thanks,
Pavel
^ permalink raw reply
* Re: [PATCH] Don't create tunnels with '%' in name.
From: Patrick McHardy @ 2008-02-21 12:10 UTC (permalink / raw)
To: Pavel Emelyanov; +Cc: David Miller, Linux Netdev List, devel
In-Reply-To: <47BD6907.7050303@openvz.org>
Pavel Emelyanov wrote:
> Four tunnel drivers (ip_gre, ipip, ip6_tunnel and sit) can
> receive a pre-defined name for a device from the userspace.
> Since these drivers call the register_netdevice() after this
> (rtnl_lock is held), the device's name may contain a '%'
> character.
>
> Not sure how bad is this to have a device with a '%' in its
> name, but all the other places either use the register_netdev(),
> or explicitly call dev_alloc_name() before registering, i.e.
> do not allow for such names.
>
> Signed-off-by: Pavel Emelyanov <xemul@openvz.org>
>
> ---
>
> diff --git a/net/ipv4/ip_gre.c b/net/ipv4/ip_gre.c
> index 63f6917..6b9744f 100644
> --- a/net/ipv4/ip_gre.c
> +++ b/net/ipv4/ip_gre.c
> @@ -274,19 +274,24 @@ static struct ip_tunnel * ipgre_tunnel_locate(struct ip_tunnel_parm *parms, int
> if (!dev)
> return NULL;
>
> + if (strchr(name, '%')) {
> + if (dev_alloc_name(dev, name) < 0)
> + goto failed_free;
> + }
> +
It would be nicer to replace the entire hand-made name
allocation to remove the 100 device limit.
^ permalink raw reply
* Re: [PATCH][PPPOL2TP]: Fix SMP oops in pppol2tp driver
From: Jarek Poplawski @ 2008-02-21 12:08 UTC (permalink / raw)
To: James Chapman; +Cc: David Miller, Paul Mackerras, netdev
In-Reply-To: <47BD4A34.7070606@katalix.com>
On Thu, Feb 21, 2008 at 09:53:56AM +0000, James Chapman wrote:
> Jarek Poplawski wrote:
...
> The _bh locking fixes in pppol2tp combined with your ppp_generic change
> solved that problem. So I then added data traffic into the mix (since
> this will happen in a real network) and found that lockups still happen.
> But the lockdep trace in this case is different, as you noted.
I'm not sure what do you mean by "solved that problem": lack of lockups
or lack of this kind of lockdep reports. This lockdep report shows a
real danger in this case, probably very little probable, unless a lot
of tries. So if you think just this kind of lockup happend there, this
is would be nice. But there could be something less nice too: we are
"fighting" with lockdep in one place but the lockup happens somewhere
else for some totally different reason...
> Does PPPoE stress the PPP setup code as much as this scenario? I guess
> in theory it could if lots of PPPoE clients connected at the same time,
> but there is no aggregate tunnel like there is with L2TP to cause all
> sessions to connect simultaneously. Perhaps PPTP also suffers from these
> issues? Perhaps not because it tends to be used only in VPN setups where
> there is only 1 session per tunnel.
I don't know this code enough, but it seems it should be easier to
maintain or debug if there are similar solutions where possible.
>>>> 3) I send here another testing patch with this second way to do this:
>>>> on the write side, but it's even more "experimental" and only a
>>>> proof of concept (should be applied on vanilla ppp_generic).
>>> I'll look over it. I think I need to take a step back and look at
>>> what's happening in more detail though.
>>
>> This is something completely new and changes all the picture: the xmit
>> path wasn't expected (at least by me) to be called in softirq context
>> at all, and there were no traces of this on previous reports. But,
>> since lockdep always stops after the first warning, there could be
>> even more surprises like this in the future. I'll check this report.
>
> Doesn't the TX softirq do transmits if they've been queued up?
I've probably too much looked at these reports, and should've expected
this could happen. Probably queueing could be separated, but since
there could be no queue at all and it's done like this, then this
current proof of concept seems to be dead end, and we have to go back
to fixing sk_dst_lock handling and my patches could be dumped...
So if I don't miss something again (and I need more time for this new
report) you should try to fix the problem not reported originally by
lockdep, but forseen by David(!): we need to avoid any path like:
ppp_generic -> pppol2tp -> something, which could take sk_dst_lock
while holding any ppp_generic writing lock: they all are softirq
"safe" (i.e. endangered). David gave some example, but I'm not sure
you did your patch like this (sk_dst_set()). Probably ip_queue_xmit()
can't work with this too.
Another, probably simpler way would be to move almost all pppol2tp_xmit
code to a workqueue: this should let to break most of dependencies with
ppp_generic locks, but I don't know how much it would affect other
things (e.g. performance). So you should really rethink these things.
Jarek P.
^ 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