* RE: [RFC PATCH 0/8]: uninline & uninline
From: Hua Zhong @ 2008-02-23 19:58 UTC (permalink / raw)
To: 'Andrew Morton', 'Andi Kleen'
Cc: 'Ilpo Järvinen', netdev, linux-kernel,
'David Miller', 'Arnaldo Carvalho de Melo'
In-Reply-To: <20080223105517.4d706511.akpm@linux-foundation.org>
> > Is there any reason they couldn't just be merged to mainline?
> >
> > I think it's a useful facility.
>
> ummm, now why did we made that decision... I think we decided that
> it's the sort of thing which one person can run once per few months
> and that will deliver its full value. I can maintain it in -mm and
> we're happy - no need to add it to mainline. No strong feelings
> either way really.
Apparently nobody has been doing it for a while. :-) Last time I did it it
was around the submission time and I actually patched it into mainline
kernel to do so. Not particularly hard to do, but sitting in mm-only does
make it a bit harder, and there are the vdso problem you just mentioned that
one has to fix for himself if it exists in mainline.
> It does have the downside that the kernel explodes if someone adds
> unlikely or likely to the vdso code and I need to occasionally hunt
> down new additions and revert them in that patch. That makes it a
> bit of a maintenance burden.
Is it possible to catch this automatically, like, by re-defining
likely/unlikely to the raw form in specific file(s)?
Hua
^ permalink raw reply
* Re: [PATCH] [PS3] gelic wireless driver needs MAC80211 support
From: Ivo van Doorn @ 2008-02-23 19:50 UTC (permalink / raw)
To: Sebastian Siewior; +Cc: Masakazu Mokuno, jgarzik, netdev
In-Reply-To: <200802232044.59887.IvDoorn@gmail.com>
On Saturday 23 February 2008, Ivo van Doorn wrote:
> On Saturday 23 February 2008, Sebastian Siewior wrote:
> > so select it.
> > Signed-off-by: Sebastian Siewior <sebastian@breakpoint.cc>
> > ---
> > drivers/net/Kconfig | 1 +
> > 1 files changed, 1 insertions(+), 0 deletions(-)
> >
> > diff --git a/drivers/net/Kconfig b/drivers/net/Kconfig
> > index f337800..a116056 100644
> > --- a/drivers/net/Kconfig
> > +++ b/drivers/net/Kconfig
> > @@ -2363,6 +2363,7 @@ config GELIC_NET
> > config GELIC_WIRELESS
> > bool "PS3 Wireless support"
> > depends on GELIC_NET
> > + select MAC80211
> > help
> > This option adds the support for the wireless feature of PS3.
> > If you have the wireless-less model of PS3 or have no plan to
>
> Is there any particular reason why this driver is in drivers/net instead
> of drivers/net/wireless (along with all other wireless drivers?
>
> Additionally, all other mac80211 drivers depend on mac80211 instead
> of selecting it. So I think for consistency it might be better to also
> make this driver depend on it, rather then selecting.
Additionally, what part of the driver actually uses mac80211?
I just browsed to the code, and it seems to work completely without
using mac80211. Instead it seems to work directly by registering a
net_device structure...
Ivo
^ permalink raw reply
* Re: [PATCH] [PS3] gelic wireless driver needs MAC80211 support
From: Ivo van Doorn @ 2008-02-23 19:44 UTC (permalink / raw)
To: Sebastian Siewior; +Cc: Masakazu Mokuno, jgarzik, netdev
In-Reply-To: <20080223193517.GA6704@Chamillionaire.breakpoint.cc>
On Saturday 23 February 2008, Sebastian Siewior wrote:
> so select it.
> Signed-off-by: Sebastian Siewior <sebastian@breakpoint.cc>
> ---
> drivers/net/Kconfig | 1 +
> 1 files changed, 1 insertions(+), 0 deletions(-)
>
> diff --git a/drivers/net/Kconfig b/drivers/net/Kconfig
> index f337800..a116056 100644
> --- a/drivers/net/Kconfig
> +++ b/drivers/net/Kconfig
> @@ -2363,6 +2363,7 @@ config GELIC_NET
> config GELIC_WIRELESS
> bool "PS3 Wireless support"
> depends on GELIC_NET
> + select MAC80211
> help
> This option adds the support for the wireless feature of PS3.
> If you have the wireless-less model of PS3 or have no plan to
Is there any particular reason why this driver is in drivers/net instead
of drivers/net/wireless (along with all other wireless drivers?
Additionally, all other mac80211 drivers depend on mac80211 instead
of selecting it. So I think for consistency it might be better to also
make this driver depend on it, rather then selecting.
Ivo
^ permalink raw reply
* [PATCH] [PS3] gelic wireless driver needs MAC80211 support
From: Sebastian Siewior @ 2008-02-23 19:35 UTC (permalink / raw)
To: Masakazu Mokuno; +Cc: jgarzik, netdev
so select it.
Signed-off-by: Sebastian Siewior <sebastian@breakpoint.cc>
---
drivers/net/Kconfig | 1 +
1 files changed, 1 insertions(+), 0 deletions(-)
diff --git a/drivers/net/Kconfig b/drivers/net/Kconfig
index f337800..a116056 100644
--- a/drivers/net/Kconfig
+++ b/drivers/net/Kconfig
@@ -2363,6 +2363,7 @@ config GELIC_NET
config GELIC_WIRELESS
bool "PS3 Wireless support"
depends on GELIC_NET
+ select MAC80211
help
This option adds the support for the wireless feature of PS3.
If you have the wireless-less model of PS3 or have no plan to
--
1.5.3.8
^ permalink raw reply related
* Re: [RFC PATCH 0/8]: uninline & uninline
From: Andrew Morton @ 2008-02-23 18:55 UTC (permalink / raw)
To: Andi Kleen
Cc: Ilpo Järvinen, netdev, linux-kernel, David Miller,
Arnaldo Carvalho de Melo
In-Reply-To: <p734pbz95lx.fsf@bingen.suse.de>
On Sat, 23 Feb 2008 14:15:06 +0100 Andi Kleen <andi@firstfloor.org> wrote:
> Andrew Morton <akpm@linux-foundation.org> writes:
>
>
> >> -41525 2066 f, 3370 +, 44895 -, diff: -41525 IS_ERR
> >
> > This is a surprise. I expect that the -mm-only
> > profile-likely-unlikely-macros.patch is the cause of this and mainline
> > doesn't have this problem.
>
> Shouldn't they only have overhead when the respective CONFIG is enabled?
yup.
> > If true, then this likely/unlikely bloat has probably spread into a lot of
> > your other results and it all should be redone against mainline, sorry :(
> >
> > (I'm not aware of anyone having used profile-likely-unlikely-macros.patch
> > in quite some time. That's unfortunate because it has turned up some
> > fairly flagrant code deoptimisations)
>
> Is there any reason they couldn't just be merged to mainline?
>
> I think it's a useful facility.
ummm, now why did we made that decision... I think we decided that it's
the sort of thing which one person can run once per few months and that
will deliver its full value. I can maintain it in -mm and we're happy - no
need to add it to mainline. No strong feelings either way really.
It does have the downside that the kernel explodes if someone adds unlikely
or likely to the vdso code and I need to occasionally hunt down new
additions and revert them in that patch. That makes it a bit of a
maintenance burden.
^ permalink raw reply
* Re: [RFC PATCH 8/8] Jhash in too big for inlining, move under lib/
From: Andrew Morton @ 2008-02-23 18:21 UTC (permalink / raw)
To: "Ilpo Järvinen"
Cc: Netdev, LKML, David Miller, Arnaldo Carvalho de Melo
In-Reply-To: <Pine.LNX.4.64.0802231006360.29504@kivilampi-30.cs.helsinki.fi>
On Sat, 23 Feb 2008 12:05:36 +0200 (EET) "Ilpo Järvinen" <ilpo.jarvinen@helsinki.fi> wrote:
> On Sat, 23 Feb 2008, Andrew Morton wrote:
>
> > On Wed, 20 Feb 2008 15:47:18 +0200 "Ilpo Järvinen" <ilpo.jarvinen@helsinki.fi> wrote:
> >
> > > vmlinux.o:
> > > 62 functions changed, 66 bytes added, 10935 bytes removed, diff: -10869
> > >
> > > ...+ these to lib/jhash.o:
> > > jhash_3words: 112
> > > jhash2: 276
> > > jhash: 475
> > >
> > > select for networking code might need a more fine-grained approach.
> >
> > It should be possible to use a modular jhash.ko. The things which you
> > have identified as clients of the jhash library are usually loaded as modules.
> > But in the case where someone does (say) NFSD=y we do need jhash.o linked into
> > vmlinux also. This is doable in Kconfig but I always forget how.
>
> Ok, even though its not that likely that one lives without e.g.
> net/ipv4/inet_connection_sock.c or net/netlink/af_netlink.c?
Sure, the number of people who will want CONFIG_JHASH=n is very small.
> But maybe
> some guys "really know what they are doing" and can come up with config
> that would be able to build it as module (for other than proof-of-concept
> uses I mean)... :-/
>
> > Adrian, Sam and Randy are the repositories of knowledge here ;)
>
> Thanks, I'll consult them in this. I've never needed to do any Kconfig
> stuff so far so it's no surprise I have very little clue... :-)
Thanks. If it gets messy I'd just put lib/jhash.o in obj-y. I assume it's
pretty small.
> I've one question for you Andrew, how would you like this kind of
> cross-subsys toucher to be merged, through you directly I suppose?
Sure, I can scrounge around for appropriate reviews and acks and can make
sure that nobody's pending work gets disrupted.
^ permalink raw reply
* Re: [RFC PATCH 0/8]: uninline & uninline
From: Ilpo Järvinen @ 2008-02-23 18:06 UTC (permalink / raw)
To: Andi Kleen
Cc: Andrew Morton, Netdev, LKML, David Miller,
Arnaldo Carvalho de Melo
In-Reply-To: <p734pbz95lx.fsf@bingen.suse.de>
On Sat, 23 Feb 2008, Andi Kleen wrote:
> Andrew Morton <akpm@linux-foundation.org> writes:
>
>
> >> -41525 2066 f, 3370 +, 44895 -, diff: -41525 IS_ERR
> >
> > This is a surprise. I expect that the -mm-only
> > profile-likely-unlikely-macros.patch is the cause of this and mainline
> > doesn't have this problem.
>
> Shouldn't they only have overhead when the respective CONFIG is enabled?
I uninlined those with allyesconfig. I'll anyway try later on without a
number of debug related CONFIGs.
--
i.
^ permalink raw reply
* [PATCH 2/2]via-rhine.c: Write-Wake-on-LAN-options-in-_real-time_-not-at-shutdown
From: Laura Garcia @ 2008-02-23 17:56 UTC (permalink / raw)
To: netdev; +Cc: linux-kernel
Write Wake-on-LAN options in _real time_ not at shutdown. It might
be safe in case of power outage.
Signed-off-by: Laura Garcia Liebana <nevola@gmail.com>
modified: drivers/net/via-rhine.c
---
drivers/net/via-rhine.c | 76 +++++++++++++++++++++++++---------------------
1 files changed, 41 insertions(+), 35 deletions(-)
diff --git a/drivers/net/via-rhine.c b/drivers/net/via-rhine.c
index 8c9d6ae..7723ec0 100644
--- a/drivers/net/via-rhine.c
+++ b/drivers/net/via-rhine.c
@@ -418,6 +418,7 @@ static void rhine_tx(struct net_device *dev);
static int rhine_rx(struct net_device *dev, int limit);
static void rhine_error(struct net_device *dev, int intr_status);
static void rhine_set_rx_mode(struct net_device *dev);
+static void rhine_write_wol(struct net_device *dev);
static struct net_device_stats *rhine_get_stats(struct net_device *dev);
static int netdev_ioctl(struct net_device *dev, struct ifreq *rq, int cmd);
static const struct ethtool_ops netdev_ethtool_ops;
@@ -1772,7 +1773,7 @@ static void rhine_get_wol(struct net_device *dev, struct ethtool_wolinfo *wol)
struct rhine_private *rp = netdev_priv(dev);
if (!(rp->quirks & rqWOL))
- return;
+ return; /* Nothing to do for non-WOL adapters */
spin_lock_irq(&rp->lock);
wol->supported = WAKE_PHY | WAKE_MAGIC |
@@ -1788,7 +1789,7 @@ static int rhine_set_wol(struct net_device *dev, struct ethtool_wolinfo *wol)
WAKE_UCAST | WAKE_MCAST | WAKE_BCAST; /* Untested */
if (!(rp->quirks & rqWOL))
- return -EINVAL;
+ return -EINVAL; /* Nothing to do for non-WOL adapters */
if (wol->wolopts & ~support)
return -EINVAL;
@@ -1797,9 +1798,47 @@ static int rhine_set_wol(struct net_device *dev, struct ethtool_wolinfo *wol)
rp->wolopts = wol->wolopts;
spin_unlock_irq(&rp->lock);
+ rhine_write_wol(dev);
+
return 0;
}
+static void rhine_write_wol(struct net_device *dev)
+{
+ struct rhine_private *rp = netdev_priv(dev);
+ void __iomem *ioaddr = rp->base;
+
+ rhine_power_init(dev);
+
+ /* Make sure we use pattern 0, 1 and not 4, 5 */
+ if (rp->quirks & rq6patterns)
+ iowrite8(0x04, ioaddr + WOLcgClr);
+
+ if (rp->wolopts & WAKE_MAGIC) {
+ iowrite8(WOLmagic, ioaddr + WOLcrSet);
+ /*
+ * Turn EEPROM-controlled wake-up back on -- some hardware may
+ * not cooperate otherwise.
+ */
+ iowrite8(ioread8(ioaddr + ConfigA) | 0x03, ioaddr + ConfigA);
+ }
+
+ if (rp->wolopts & (WAKE_BCAST|WAKE_MCAST))
+ iowrite8(WOLbmcast, ioaddr + WOLcgSet);
+
+ if (rp->wolopts & WAKE_PHY)
+ iowrite8(WOLlnkon | WOLlnkoff, ioaddr + WOLcrSet);
+
+ if (rp->wolopts & WAKE_UCAST)
+ iowrite8(WOLucast, ioaddr + WOLcrSet);
+
+ if (rp->wolopts) {
+ /* Enable legacy WOL (for old motherboards) */
+ iowrite8(0x01, ioaddr + PwcfgSet);
+ iowrite8(ioread8(ioaddr + StickyHW) | 0x04, ioaddr + StickyHW);
+ }
+}
+
static const struct ethtool_ops netdev_ethtool_ops = {
.get_drvinfo = netdev_get_drvinfo,
.get_settings = netdev_get_settings,
@@ -1886,39 +1925,6 @@ static void rhine_shutdown (struct pci_dev *pdev)
struct rhine_private *rp = netdev_priv(dev);
void __iomem *ioaddr = rp->base;
- if (!(rp->quirks & rqWOL))
- return; /* Nothing to do for non-WOL adapters */
-
- rhine_power_init(dev);
-
- /* Make sure we use pattern 0, 1 and not 4, 5 */
- if (rp->quirks & rq6patterns)
- iowrite8(0x04, ioaddr + WOLcgClr);
-
- if (rp->wolopts & WAKE_MAGIC) {
- iowrite8(WOLmagic, ioaddr + WOLcrSet);
- /*
- * Turn EEPROM-controlled wake-up back on -- some hardware may
- * not cooperate otherwise.
- */
- iowrite8(ioread8(ioaddr + ConfigA) | 0x03, ioaddr + ConfigA);
- }
-
- if (rp->wolopts & (WAKE_BCAST|WAKE_MCAST))
- iowrite8(WOLbmcast, ioaddr + WOLcgSet);
-
- if (rp->wolopts & WAKE_PHY)
- iowrite8(WOLlnkon | WOLlnkoff, ioaddr + WOLcrSet);
-
- if (rp->wolopts & WAKE_UCAST)
- iowrite8(WOLucast, ioaddr + WOLcrSet);
-
- if (rp->wolopts) {
- /* Enable legacy WOL (for old motherboards) */
- iowrite8(0x01, ioaddr + PwcfgSet);
- iowrite8(ioread8(ioaddr + StickyHW) | 0x04, ioaddr + StickyHW);
- }
-
/* Hit power state D3 (sleep) */
if (!avoid_D3)
iowrite8(ioread8(ioaddr + StickyHW) | 0x03, ioaddr + StickyHW);
--
1.5.3.8
^ permalink raw reply related
* [PATCH 1/2]via-rhine.c: Use-register-offset-definition-for-WOLcgClr
From: Laura Garcia @ 2008-02-23 17:56 UTC (permalink / raw)
To: netdev; +Cc: linux-kernel
Use register offset definition for WOLcgClr. This patch does not
change the driver behaviour.
Signed-off-by: Laura Garcia Liebana <nevola@gmail.com>
modified: drivers/net/via-rhine.c
---
drivers/net/via-rhine.c | 2 +-
1 files changed, 1 insertions(+), 1 deletions(-)
diff --git a/drivers/net/via-rhine.c b/drivers/net/via-rhine.c
index 7c851b1..8c9d6ae 100644
--- a/drivers/net/via-rhine.c
+++ b/drivers/net/via-rhine.c
@@ -1893,7 +1893,7 @@ static void rhine_shutdown (struct pci_dev *pdev)
/* Make sure we use pattern 0, 1 and not 4, 5 */
if (rp->quirks & rq6patterns)
- iowrite8(0x04, ioaddr + 0xA7);
+ iowrite8(0x04, ioaddr + WOLcgClr);
if (rp->wolopts & WAKE_MAGIC) {
iowrite8(WOLmagic, ioaddr + WOLcrSet);
--
1.5.3.8
^ permalink raw reply related
* Re: [RFC PATCH 0/8]: uninline & uninline
From: Andi Kleen @ 2008-02-23 13:15 UTC (permalink / raw)
To: Andrew Morton
Cc: Ilpo Järvinen, netdev, linux-kernel, David Miller,
Arnaldo Carvalho de Melo
In-Reply-To: <20080223000220.e9229c99.akpm@linux-foundation.org>
Andrew Morton <akpm@linux-foundation.org> writes:
>> -41525 2066 f, 3370 +, 44895 -, diff: -41525 IS_ERR
>
> This is a surprise. I expect that the -mm-only
> profile-likely-unlikely-macros.patch is the cause of this and mainline
> doesn't have this problem.
Shouldn't they only have overhead when the respective CONFIG is enabled?
> If true, then this likely/unlikely bloat has probably spread into a lot of
> your other results and it all should be redone against mainline, sorry :(
>
> (I'm not aware of anyone having used profile-likely-unlikely-macros.patch
> in quite some time. That's unfortunate because it has turned up some
> fairly flagrant code deoptimisations)
Is there any reason they couldn't just be merged to mainline?
I think it's a useful facility.
-Andi
^ permalink raw reply
* Re: [RFC PATCH 8/8] Jhash in too big for inlining, move under lib/
From: Andi Kleen @ 2008-02-23 13:06 UTC (permalink / raw)
To: Andrew Morton
Cc: Ilpo Järvinen, netdev, linux-kernel, David Miller,
Arnaldo Carvalho de Melo
In-Reply-To: <20080223000214.81b1390a.akpm@linux-foundation.org>
Andrew Morton <akpm@linux-foundation.org> writes:
>
> It should be possible to use a modular jhash.ko. The things which you
> have identified as clients of the jhash library are usually loaded as modules.
For very small functions like this own modules are quite expensive. First
everything gets rounded up to at least one 4K page (or worse on architectures
with larger pages). That just wastes some memory.
But then since modules live in vmalloc space they also need an own
TLB entry, which are notouriously scarce in the kernel because often user space
wants to monopolize them all. So if you're unlucky and user space
is thrashing the TLB just a single call to this hash function will be an own
TLB miss and quite expensive.
It would be better to just always link it in for this case.
-Andi
^ permalink raw reply
* Re: [RFC PATCH 0/8]: uninline & uninline
From: Ilpo Järvinen @ 2008-02-23 10:11 UTC (permalink / raw)
To: Andrew Morton; +Cc: Netdev, LKML, David Miller, Arnaldo Carvalho de Melo
In-Reply-To: <20080223000220.e9229c99.akpm@linux-foundation.org>
On Sat, 23 Feb 2008, Andrew Morton wrote:
> On Wed, 20 Feb 2008 15:47:10 +0200 "Ilpo J__rvinen" <ilpo.jarvinen@helsinki.fi> wrote:
>
> > -41525 2066 f, 3370 +, 44895 -, diff: -41525 IS_ERR
>
> This is a surprise.
It surprised me as well, there were something like 10 bytes I just
couldn't explain in IS_ERR size (kernel/uninlined.c: IS_ERR | +25). I was
to look into it deeper but didn't have the .o's at hand right away, not so
trivial to store results of 5000+ build results except some carefully
picked textual output :-)... Hmm, I'll add logging for the disassembly of
the uninlined stuff into the next run, that won't cost too much...
> I expect that the -mm-only
> profile-likely-unlikely-macros.patch is the cause of this and mainline
> doesn't have this problem.
Ahaa, this explain it, I suspected that there was something (un)likely
related elsewhere as well, no wonder why I couldn't reproduce the 25 bytes
result in my quick copy-pasted non-kernel test...
> If true, then this likely/unlikely bloat has probably spread into a lot of
> your other results and it all should be redone against mainline, sorry :(
It isn't that troublesome to redo them, it's mainly automatic combined
with impatient waiting from my behalf :-)... The spreading problem is
probably true, to some extent. I did some runs also with carefully
selected CONFIG.*DEBUG.* off under include/net/ earlier, in general it
made very little difference, if something bloats, it usually does that
regardless of .config. There are probably couple of exceptions when the
size is on the boundary where it's very close of being useful to uninline
it.
One interesting thing in there was that the largest offenders are quite
small per call-site but due to vast number of them even a small benefit
buys off a lot in kernel wide results. I suspect the differences due to
(un)likely will be negligle, because the IS_ERR with all its trivialness
is now mostly -15, so anything clearly larger than that will likely still
be a win x n (where n is quite large).
Anyway, I'll see when I get the new set of tests running... :-) I'd prefer
with CONFIG.*DEBUG off to get larger picture about non-debug builds too
(especially the scatterlist.h things interest me a lot), do you think that
would do as well?
Thanks for your comments, I found them very useful.
--
i.
^ permalink raw reply
* Re: [RFC PATCH 8/8] Jhash in too big for inlining, move under lib/
From: Ilpo Järvinen @ 2008-02-23 10:05 UTC (permalink / raw)
To: Andrew Morton; +Cc: Netdev, LKML, David Miller, Arnaldo Carvalho de Melo
In-Reply-To: <20080223000214.81b1390a.akpm@linux-foundation.org>
[-- Attachment #1: Type: TEXT/PLAIN, Size: 1353 bytes --]
On Sat, 23 Feb 2008, Andrew Morton wrote:
> On Wed, 20 Feb 2008 15:47:18 +0200 "Ilpo Järvinen" <ilpo.jarvinen@helsinki.fi> wrote:
>
> > vmlinux.o:
> > 62 functions changed, 66 bytes added, 10935 bytes removed, diff: -10869
> >
> > ...+ these to lib/jhash.o:
> > jhash_3words: 112
> > jhash2: 276
> > jhash: 475
> >
> > select for networking code might need a more fine-grained approach.
>
> It should be possible to use a modular jhash.ko. The things which you
> have identified as clients of the jhash library are usually loaded as modules.
> But in the case where someone does (say) NFSD=y we do need jhash.o linked into
> vmlinux also. This is doable in Kconfig but I always forget how.
Ok, even though its not that likely that one lives without e.g.
net/ipv4/inet_connection_sock.c or net/netlink/af_netlink.c? But maybe
some guys "really know what they are doing" and can come up with config
that would be able to build it as module (for other than proof-of-concept
uses I mean)... :-/
> Adrian, Sam and Randy are the repositories of knowledge here ;)
Thanks, I'll consult them in this. I've never needed to do any Kconfig
stuff so far so it's no surprise I have very little clue... :-)
I've one question for you Andrew, how would you like this kind of
cross-subsys toucher to be merged, through you directly I suppose?
--
i.
^ permalink raw reply
* Re: Error on ip route replace command
From: Francesco Saverio Giudice @ 2008-02-23 9:41 UTC (permalink / raw)
To: Joonwoo Park; +Cc: netdev
In-Reply-To: <b25c3fa70802222244g1d3d08ecme90f4395c1c5505c@mail.gmail.com>
----- Original Message -----
From: "Joonwoo Park" <joonwpark81@gmail.com>
To: "Francesco Saverio Giudice" <info@fsgiudice.com>
Cc: <netdev@vger.kernel.org>
Sent: Saturday, February 23, 2008 7:44 AM
Subject: Re: Error on ip route replace command
>
> Francesco,
> It was fixed by 936f6f8e1bc46834bbb3e3fa3ac13ab44f1e7ba6 and
> c18865f39276435abb9286f9a816cb5b66c99a00.
> Please try with lastest git source
>
> Joonwoo.
>
Joonwoo,
I will give a try.
Thank you
Francesco
^ permalink raw reply
* Re: [PATCH] alloc_percpu() fails to allocate percpu data
From: Nick Piggin @ 2008-02-23 9:23 UTC (permalink / raw)
To: Peter Zijlstra
Cc: Eric Dumazet, David S. Miller, Andrew Morton, linux kernel,
netdev, Christoph Lameter, Zhang, Yanmin
In-Reply-To: <1203632765.6112.20.camel@lappy>
On Friday 22 February 2008 09:26, Peter Zijlstra wrote:
> On Thu, 2008-02-21 at 19:00 +0100, Eric Dumazet wrote:
> > 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.
>
> I've complained about this false sharing as well, so until we get the
> new and improved percpu allocators,
What I don't understand is why the slab allocators have something like
this in it:
if ((flags & SLAB_HWCACHE_ALIGN) &&
size > cache_line_size() / 2)
return max_t(unsigned long, align, cache_line_size());
If you ask for HWCACHE_ALIGN, then you should get it. I don't
understand, why do they think they knows better than the caller?
Things like this are just going to lead to very difficult to track
performance problems. Possibly correctness problems in rare cases.
There could be another flag for "maybe align".
^ permalink raw reply
* Re: [PATCH] alloc_percpu() fails to allocate percpu data
From: Andrew Morton @ 2008-02-23 8:04 UTC (permalink / raw)
To: Eric Dumazet
Cc: David S. Miller, linux kernel, netdev, Christoph Lameter,
Zhang, Yanmin
In-Reply-To: <47BDBC23.10605@cosmosbay.com>
On Thu, 21 Feb 2008 19:00:03 +0100 Eric Dumazet <dada1@cosmosbay.com> wrote:
> +#ifndef cache_line_size
> +#define cache_line_size() L1_CACHE_BYTES
> +#endif
argh, you made me look.
Really cache_line_size() should be implemented in include/linux/cache.h.
Then we tromp the stupid private implementations in slob.c and slub.c.
Then we wonder why x86 uses a custom cache_line_size(), but still uses
L1_CACHE_BYTES for its L1_CACHE_ALIGN().
Once we've answered that, we look at your
+ /*
+ * We should make sure each CPU gets private memory.
+ */
+ size = roundup(size, cache_line_size());
and wonder whether it should have used L1_CACHE_ALIGN().
I think I'd better stop looking.
^ permalink raw reply
* Re: [PATCH 10/28] mm: memory reserve management
From: Andrew Morton @ 2008-02-23 8:06 UTC (permalink / raw)
To: Peter Zijlstra
Cc: Linus Torvalds, linux-kernel, linux-mm, netdev, trond.myklebust,
Alexey Dobriyan
In-Reply-To: <20080220150306.548965000@chello.nl>
On Wed, 20 Feb 2008 15:46:20 +0100 Peter Zijlstra <a.p.zijlstra@chello.nl> wrote:
> Generic reserve management code.
>
> It provides methods to reserve and charge. Upon this, generic alloc/free style
> reserve pools could be build, which could fully replace mempool_t
> functionality.
>
> It should also allow for a Banker's algorithm replacement of __GFP_NOFAIL.
Generally: the comments in this code are a bit straggly and hard to follow.
They'd be worth a revisit.
> +/*
> + * Simple output of the reserve tree in: /proc/reserve_info
> + * Example:
> + *
> + * localhost ~ # cat /proc/reserve_info
> + * total reserve 8156K (0/544817)
> + * total network reserve 8156K (0/544817)
> + * network TX reserve 196K (0/49)
> + * protocol TX pages 196K (0/49)
> + * network RX reserve 7960K (0/544768)
> + * IPv6 route cache 1372K (0/4096)
> + * IPv4 route cache 5468K (0/16384)
> + * SKB data reserve 1120K (0/524288)
> + * IPv6 fragment cache 560K (0/262144)
> + * IPv4 fragment cache 560K (0/262144)
> + */
Well, "Simple" was a freudian typo. Not designed for programmatic parsing,
I see.
> +static __init int mem_reserve_proc_init(void)
> +{
> + struct proc_dir_entry *entry;
> +
> + entry = create_proc_entry("reserve_info", S_IRUSR, NULL);
I think we're supposed to use proc_create(). Blame Alexey.
> + if (entry)
> + entry->proc_fops = &mem_reserve_opterations;
> +
> + return 0;
> +}
> +
> +__initcall(mem_reserve_proc_init);
module_init() is more trendy.
^ permalink raw reply
* Re: [PATCH 15/28] netvm: network reserve infrastructure
From: Andrew Morton @ 2008-02-23 8:06 UTC (permalink / raw)
To: Peter Zijlstra
Cc: Linus Torvalds, linux-kernel, linux-mm, netdev, trond.myklebust
In-Reply-To: <20080220150307.208040000@chello.nl>
On Wed, 20 Feb 2008 15:46:25 +0100 Peter Zijlstra <a.p.zijlstra@chello.nl> wrote:
> Provide the basic infrastructure to reserve and charge/account network memory.
>
> We provide the following reserve tree:
>
> 1) total network reserve
> 2) network TX reserve
> 3) protocol TX pages
> 4) network RX reserve
> 5) SKB data reserve
>
> [1] is used to make all the network reserves a single subtree, for easy
> manipulation.
>
> [2] and [4] are merely for eastetic reasons.
>
> The TX pages reserve [3] is assumed bounded by it being the upper bound of
> memory that can be used for sending pages (not quite true, but good enough)
>
> The SKB reserve [5] is an aggregate reserve, which is used to charge SKB data
> against in the fallback path.
>
> The consumers for these reserves are sockets marked with:
> SOCK_MEMALLOC
>
> Such sockets are to be used to service the VM (iow. to swap over). They
> must be handled kernel side, exposing such a socket to user-space is a BUG.
>
> +/**
> + * sk_adjust_memalloc - adjust the global memalloc reserve for critical RX
> + * @socks: number of new %SOCK_MEMALLOC sockets
> + * @tx_resserve_pages: number of pages to (un)reserve for TX
> + *
> + * This function adjusts the memalloc reserve based on system demand.
> + * The RX reserve is a limit, and only added once, not for each socket.
> + *
> + * NOTE:
> + * @tx_reserve_pages is an upper-bound of memory used for TX hence
> + * we need not account the pages like we do for RX pages.
> + */
> +int sk_adjust_memalloc(int socks, long tx_reserve_pages)
> +{
> + int nr_socks;
> + int err;
> +
> + err = mem_reserve_pages_add(&net_tx_pages, tx_reserve_pages);
> + if (err)
> + return err;
> +
> + nr_socks = atomic_read(&memalloc_socks);
> + if (!nr_socks && socks > 0)
> + err = mem_reserve_connect(&net_reserve, &mem_reserve_root);
This looks like it should have some locking?
> + nr_socks = atomic_add_return(socks, &memalloc_socks);
> + if (!nr_socks && socks)
> + err = mem_reserve_disconnect(&net_reserve);
Or does that try to make up for it? Still looks fishy.
> + if (err)
> + mem_reserve_pages_add(&net_tx_pages, -tx_reserve_pages);
> +
> + return err;
> +}
> +
> +/**
> + * sk_set_memalloc - sets %SOCK_MEMALLOC
> + * @sk: socket to set it on
> + *
> + * Set %SOCK_MEMALLOC on a socket and increase the memalloc reserve
> + * accordingly.
> + */
> +int sk_set_memalloc(struct sock *sk)
> +{
> + int set = sock_flag(sk, SOCK_MEMALLOC);
> +#ifndef CONFIG_NETVM
> + BUG();
> +#endif
?? #error, maybe?
> + if (!set) {
> + int err = sk_adjust_memalloc(1, 0);
> + if (err)
> + return err;
> +
> + sock_set_flag(sk, SOCK_MEMALLOC);
> + sk->sk_allocation |= __GFP_MEMALLOC;
> + }
> + return !set;
> +}
> +EXPORT_SYMBOL_GPL(sk_set_memalloc);
^ permalink raw reply
* Re: [PATCH 17/28] netvm: hook skb allocation to reserves
From: Andrew Morton @ 2008-02-23 8:06 UTC (permalink / raw)
To: Peter Zijlstra
Cc: Linus Torvalds, linux-kernel, linux-mm, netdev, trond.myklebust
In-Reply-To: <20080220150307.507134000@chello.nl>
On Wed, 20 Feb 2008 15:46:27 +0100 Peter Zijlstra <a.p.zijlstra@chello.nl> wrote:
> Change the skb allocation api to indicate RX usage and use this to fall back to
> the reserve when needed. SKBs allocated from the reserve are tagged in
> skb->emergency.
>
> Teach all other skb ops about emergency skbs and the reserve accounting.
>
> Use the (new) packet split API to allocate and track fragment pages from the
> emergency reserve. Do this using an atomic counter in page->index. This is
> needed because the fragments have a different sharing semantic than that
> indicated by skb_shinfo()->dataref.
>
> Note that the decision to distinguish between regular and emergency SKBs allows
> the accounting overhead to be limited to the later kind.
>
> ...
>
> +static inline void skb_get_page(struct sk_buff *skb, struct page *page)
> +{
> + get_page(page);
> + if (skb_emergency(skb))
> + atomic_inc(&page->frag_count);
> +}
> +
> +static inline void skb_put_page(struct sk_buff *skb, struct page *page)
> +{
> + if (skb_emergency(skb) && atomic_dec_and_test(&page->frag_count))
> + rx_emergency_put(PAGE_SIZE);
> + put_page(page);
> +}
I'm thinking we should do `#define slowcall inline' then use that in the future.
> static void skb_release_data(struct sk_buff *skb)
> {
> if (!skb->cloned ||
> !atomic_sub_return(skb->nohdr ? (1 << SKB_DATAREF_SHIFT) + 1 : 1,
> &skb_shinfo(skb)->dataref)) {
> + int size;
> +
> +#ifdef NET_SKBUFF_DATA_USES_OFFSET
> + size = skb->end;
> +#else
> + size = skb->end - skb->head;
> +#endif
The patch adds rather a lot of ifdefs.
^ permalink raw reply
* Re: [PATCH 08/28] mm: system wide ALLOC_NO_WATERMARK
From: Andrew Morton @ 2008-02-23 8:05 UTC (permalink / raw)
To: Peter Zijlstra
Cc: Linus Torvalds, linux-kernel, linux-mm, netdev, trond.myklebust
In-Reply-To: <20080220150306.297640000@chello.nl>
On Wed, 20 Feb 2008 15:46:18 +0100 Peter Zijlstra <a.p.zijlstra@chello.nl> wrote:
> Change ALLOC_NO_WATERMARK page allocation such that the reserves are system
> wide - which they are per setup_per_zone_pages_min(), when we scrape the
> barrel, do it properly.
>
The changelog is fairly incomprehensible.
> mm/page_alloc.c | 6 ++++++
> 1 file changed, 6 insertions(+)
>
> Index: linux-2.6/mm/page_alloc.c
> ===================================================================
> --- linux-2.6.orig/mm/page_alloc.c
> +++ linux-2.6/mm/page_alloc.c
> @@ -1552,6 +1552,12 @@ restart:
> rebalance:
> if (alloc_flags & ALLOC_NO_WATERMARKS) {
> nofail_alloc:
> + /*
> + * break out of mempolicy boundaries
> + */
> + zonelist = NODE_DATA(numa_node_id())->node_zonelists +
> + gfp_zone(gfp_mask);
> +
> /* go through the zonelist yet again, ignoring mins */
> page = get_page_from_freelist(gfp_mask, order, zonelist,
> ALLOC_NO_WATERMARKS);
As is the patch. People who care about mempolicies will want a better
explanation, please, so they can check that we're not busting their stuff.
^ permalink raw reply
* Re: [PATCH 00/28] Swap over NFS -v16
From: Andrew Morton @ 2008-02-23 8:06 UTC (permalink / raw)
To: Peter Zijlstra
Cc: Linus Torvalds, linux-kernel, linux-mm, netdev, trond.myklebust
In-Reply-To: <20080220144610.548202000@chello.nl>
On Wed, 20 Feb 2008 15:46:10 +0100 Peter Zijlstra <a.p.zijlstra@chello.nl> wrote:
> Another posting of the full swap over NFS series.
Well I looked. There's rather a lot of it and I wouldn't pretend to
understand it.
What is the NFS and net people's take on all of this?
^ permalink raw reply
* Re: [PATCH 05/28] mm: allow PF_MEMALLOC from softirq context
From: Andrew Morton @ 2008-02-23 8:05 UTC (permalink / raw)
To: Peter Zijlstra
Cc: Linus Torvalds, linux-kernel, linux-mm, netdev, trond.myklebust
In-Reply-To: <20080220150305.905314000@chello.nl>
On Wed, 20 Feb 2008 15:46:15 +0100 Peter Zijlstra <a.p.zijlstra@chello.nl> wrote:
> Allow PF_MEMALLOC to be set in softirq context. When running softirqs from
> a borrowed context save current->flags, ksoftirqd will have its own
> task_struct.
The second sentence doesn't make sense.
> This is needed to allow network softirq packet processing to make use of
> PF_MEMALLOC.
>
> ...
>
> +#define tsk_restore_flags(p, pflags, mask) \
> + do { (p)->flags &= ~(mask); \
> + (p)->flags |= ((pflags) & (mask)); } while (0)
> +
Does it need to be a macro?
If so, it really should cook up a temporary to avoid referencing p twice -
the children might be watching.
^ permalink raw reply
* Re: [PATCH 04/28] mm: kmem_estimate_pages()
From: Andrew Morton @ 2008-02-23 8:05 UTC (permalink / raw)
To: Peter Zijlstra
Cc: Linus Torvalds, linux-kernel, linux-mm, netdev, trond.myklebust
In-Reply-To: <20080220150305.774294000@chello.nl>
On Wed, 20 Feb 2008 15:46:14 +0100 Peter Zijlstra <a.p.zijlstra@chello.nl> wrote:
> Provide a method to get the upper bound on the pages needed to allocate
> a given number of objects from a given kmem_cache.
>
> This lays the foundation for a generic reserve framework as presented in
> a later patch in this series. This framework needs to convert object demand
> (kmalloc() bytes, kmem_cache_alloc() objects) to pages.
>
> ...
>
> /*
> + * return the max number of pages required to allocated count
> + * objects from the given cache
> + */
> +unsigned kmem_estimate_pages(struct kmem_cache *s, gfp_t flags, int objects)
You might want to have another go at that comment.
> +/*
> + * return the max number of pages required to allocate @bytes from kmalloc
> + * in an unspecified number of allocation of heterogeneous size.
> + */
> +unsigned kestimate(gfp_t flags, size_t bytes)
And its pal.
^ permalink raw reply
* Re: [PATCH 07/28] mm: emergency pool
From: Andrew Morton @ 2008-02-23 8:05 UTC (permalink / raw)
To: Peter Zijlstra
Cc: Linus Torvalds, linux-kernel, linux-mm, netdev, trond.myklebust
In-Reply-To: <20080220150306.165236000@chello.nl>
On Wed, 20 Feb 2008 15:46:17 +0100 Peter Zijlstra <a.p.zijlstra@chello.nl> wrote:
> @@ -213,7 +213,7 @@ enum zone_type {
>
> struct zone {
> /* Fields commonly accessed by the page allocator */
> - unsigned long pages_min, pages_low, pages_high;
> + unsigned long pages_emerg, pages_min, pages_low, pages_high;
It would be nice to make these one-per-line, then document them.
^ permalink raw reply
* Re: [PATCH 09/28] mm: __GFP_MEMALLOC
From: Andrew Morton @ 2008-02-23 8:06 UTC (permalink / raw)
To: Peter Zijlstra
Cc: Linus Torvalds, linux-kernel, linux-mm, netdev, trond.myklebust
In-Reply-To: <20080220150306.424308000@chello.nl>
On Wed, 20 Feb 2008 15:46:19 +0100 Peter Zijlstra <a.p.zijlstra@chello.nl> wrote:
> __GFP_MEMALLOC will allow the allocation to disregard the watermarks,
> much like PF_MEMALLOC.
>
'twould be nice if the changelog had some explanation of the reason
for this change.
^ 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