* Re: [patch 2/4] Configure out file locking features
From: Tim Bird @ 2008-07-31 17:32 UTC (permalink / raw)
To: Adrian Bunk
Cc: Thomas Petazzoni, linux-kernel, linux-embedded, michael,
Matt Mackall, matthew, linux-fsdevel, akpm
In-Reply-To: <20080731164918.GE20212@cs181140183.pp.htv.fi>
Adrian Bunk wrote:
> And for embedded systems with which applications is it 100% safe to
> disable this option?
Sony's digital cameras.
This option *is* disabled in the kernel for (most) Sony digital cameras.
Those digital cameras have the kernel, busybox, a custom C library,
and one proprietary application. The application does not use
flock() (or AIO, or ethtool or multi-cast)
These cameras were heavily tested, and are shipping now. I can't
make any guarantees for other developers, but those of us who
are careful about our application development would like the option
to eliminate completely unused features from the kernel. (And
the C library, but that's a different issue.)
> And don't answer "doesn't use flock()", I want a real-life example of a
> device where you could guarantee a developer that disabling this option
> in his product would be safe.
I'm not sure why a guarantee is required that other developers
use this option safely. Maybe this is a point of disconnect between
embedded folks and non-embedded folks. We're accustomed to making
tradeoff decisions that only affect our product, and which
we take full responsibility for testing.
If warnings or support avoidance for the general population using
such config options is the issue, I think that David Woodhouse's
suggestion that such things could taint the kernel is an interesting
idea. Maybe have we could have an "unsafe-config" taint flag?
I should add that I am sympathetic with the larger issue you raise
about nibbling at the bottom with patches that only address a
few KB of the problem, while the size continues to build (to an
even greater degree) with each release. My response is that I agree
with you on the nibbling bit, but probably just at a different
level of KB savings.
That is, I presume you'd be OK with something that saved 100K or
even 20K, but balk at bit at these patches, which save 10k or less.
My threshold is lower (probably down to about 5K, so these are
pretty close to the bubble), but even I wouldn't recommend
applying anything much below that. We've already started
considering to drop some linux-tiny patches that just don't save
enough to warrant continued maintenance.
-- Tim
=============================
Tim Bird
Architecture Group Chair, CE Linux Forum
Senior Staff Engineer, Sony Corporation of America
=============================
^ permalink raw reply
* Re: [patch 0/4] [resend] Add configuration options to disable features not needed on embedded devices
From: Tim Bird @ 2008-07-31 17:20 UTC (permalink / raw)
To: David Miller
Cc: dwmw2, torvalds, thomas.petazzoni, linux-kernel, linux-embedded,
michael
In-Reply-To: <4891EB82.9060703@am.sony.com>
Tim Bird wrote:
> It sounds like you don't want to talk about it any more,
> but could you please give me the 30-second overview
> of why ETHTOOL is required for proper ipv4 operation?
...
> Sorry if I missed an earlier explanation.
Never mind. I hadn't read the whole thread yet.
=============================
Tim Bird
Architecture Group Chair, CE Linux Forum
Senior Staff Engineer, Sony Corporation of America
=============================
^ permalink raw reply
* Re: [patch 2/4] Configure out file locking features
From: David Woodhouse @ 2008-07-31 16:57 UTC (permalink / raw)
To: Adrian Bunk
Cc: Thomas Petazzoni, linux-kernel, linux-embedded, michael,
Matt Mackall, matthew, linux-fsdevel, akpm
In-Reply-To: <20080731164918.GE20212@cs181140183.pp.htv.fi>
On Thu, 2008-07-31 at 19:49 +0300, Adrian Bunk wrote:
> On Thu, Jul 31, 2008 at 06:26:16PM +0200, Thomas Petazzoni wrote:
> > Le Thu, 31 Jul 2008 18:37:57 +0300,
> > Adrian Bunk <bunk@kernel.org> a écrit :
> >
> > > I'm just not a fan of adding config options for each few kB of code -
> > > we have to maintain them and the more complex the configuration
> > > becomes the more often it breaks.
> >
> > I'm not a fan of these too, but are there other solutions ?
>
> There are many things that can be done to reduce the kernel size or try
> to minimize the growth of the kernel.
>
> E.g. working on --combine -fwhole-program (where David once had
> preliminary patches for the per-module approach) might be better.
Yeah, I'm planning to dig that out again and play with it some time
soon. The Kbuild issues were too scary at the time, but I'm less
frightened of it these days...
Segher has also been looking at it, and reported quite a useful win when
he used it to combine arch/$ARCH/mm and mm/, and arch/$ARCH/kernel and
kernel/.
--
David Woodhouse Open Source Technology Centre
David.Woodhouse@intel.com Intel Corporation
^ permalink raw reply
* Re: [patch 2/4] Configure out file locking features
From: Adrian Bunk @ 2008-07-31 16:49 UTC (permalink / raw)
To: Thomas Petazzoni
Cc: linux-kernel, linux-embedded, michael, Matt Mackall, matthew,
linux-fsdevel, akpm
In-Reply-To: <20080731182616.4c20f0db@surf>
On Thu, Jul 31, 2008 at 06:26:16PM +0200, Thomas Petazzoni wrote:
> Le Thu, 31 Jul 2008 18:37:57 +0300,
> Adrian Bunk <bunk@kernel.org> a écrit :
>
> > I'm just not a fan of adding config options for each few kB of code -
> > we have to maintain them and the more complex the configuration
> > becomes the more often it breaks.
>
> I'm not a fan of these too, but are there other solutions ?
There are many things that can be done to reduce the kernel size or try
to minimize the growth of the kernel.
E.g. working on --combine -fwhole-program (where David once had
preliminary patches for the per-module approach) might be better.
> > What became bigger was most likely not related to the patches you
> > sent.
>
> No, it is not.
>
> > Where and why did the kernel become bigger?
>
> It's not up-to-date with 2.6.26 and 2.6.27-rc1, but Bloatwatch
> <http://www.selenic.com/bloatwatch/>, by Matt Mackall, is here to
> answer these questions. I haven't made the analysis for
> 2.6.26->2.6.27-rc1.
It can only give some hints where to start searching.
But it tracks a defconfig, and e.g. the nearly doubled size between
2.6.18 and 2.6.19 is both expected and not a problem for embedded
systems.
The real work is to figure out in which areas that are relevant for
embedded systems the kernel became bigger.
> > Why did CONFIG_FW_LOADER get enabled?
> > Due to alnoconfig disabling CONFIG_EMBEDDED?
>
> I don't know. Haven't made the analysis for now.
>
> > A user will ask:
> > I'm using $applications with $libraries, can I safely disable this
> > option?
>
> Hard to tell in the general case.
>
> > And e.g. according to a quick grep through the sources uClibc's
> > updwtmp() seems to cease working without flock().
>
> Correct. But on many embedded systems, we don't care about logging past
> user logins. We might even not care about logins at all.
And for embedded systems with which applications is it 100% safe to
disable this option?
And don't answer "doesn't use flock()", I want a real-life example of a
device where you could guarantee a developer that disabling this option
in his product would be safe.
> > It costs us maintainance of the option and the #ifdef's and gives
> > users one way more to shoot themselves into the foot in nontrivial to
> > detect ways.
>
> That's correct, and as I said previously, I fully understand the
> maintainance problem of all these new configuration options. I must
> admit that I do not really have more objective technical arguments that
> would help us deciding whether the code size reduction vs. code
> maintainance choice should be made in one direction or the other.
My personal criteron for this patch is still how many real-life systems
can safely disable it.
> Sincerly,
>
> Thomas
cu
Adrian
--
"Is there not promise of rain?" Ling Tan asked suddenly out
of the darkness. There had been need of rain for many days.
"Only a promise," Lao Er said.
Pearl S. Buck - Dragon Seed
--
To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply
* Re: [patch 0/4] [resend] Add configuration options to disable features not needed on embedded devices
From: Tim Bird @ 2008-07-31 16:42 UTC (permalink / raw)
To: David Miller
Cc: dwmw2, torvalds, thomas.petazzoni, linux-kernel, linux-embedded,
michael
In-Reply-To: <20080731.025547.35826309.davem@davemloft.net>
David Miller wrote:
> From: David Woodhouse <dwmw2@infradead.org>
> Date: Thu, 31 Jul 2008 10:51:52 +0100
>
>> But there are a lot of people who really don't need these features
>> and really want the option of leaving them out.
>
> I'll say it one last time.
>
> If you have ipv4 enabled, you need ETHTOOL.
I've been using and administering Linux for 16 years,
and using it successfully in embedded projects for 10.
Until I stumbled upon this patch in Linux-tiny, I had
never heard of ethtool. Sony has shipped hundreds
of thousands of products with ETHTOOL turned off.
It sounds like you don't want to talk about it any more,
but could you please give me the 30-second overview
of why ETHTOOL is required for proper ipv4 operation?
If Sony is shipping network-buggy products I certainly
want to know about it.
Sorry if I missed an earlier explanation.
Thanks,
-- Tim
=============================
Tim Bird
Architecture Group Chair, CE Linux Forum
Senior Staff Engineer, Sony Corporation of America
=============================
^ permalink raw reply
* Re: [patch 3/4] Configure out ethtool support
From: Thomas Petazzoni @ 2008-07-31 16:35 UTC (permalink / raw)
To: Adrian Bunk
Cc: David Woodhouse, David Miller, linux-kernel, linux-embedded,
michael, mpm, jgarzik, netdev, akpm
In-Reply-To: <20080731155820.GC20212@cs181140183.pp.htv.fi>
Le Thu, 31 Jul 2008 18:58:20 +0300,
Adrian Bunk <bunk@kernel.org> a écrit :
> It might have also made more sense to spend some of the energy used
> in this discussion instead on checking where the global 24 kB size
> increase Thomas reported for 2.6.27-rc1 compared to 2.6.26 comes from.
I agree. This is something I'm working on. I hope to have results soon.
But I *fear* that the results will be: the size growth is 10 bytes
here, 22 bytes there, 35 bytes here, spread over hundreds of patches.
Something we can do anything against.
Thomas
--
Thomas Petazzoni, Free Electrons
Kernel, drivers and embedded Linux development,
consulting, training and support.
http://free-electrons.com
^ permalink raw reply
* Re: [patch 2/4] Configure out file locking features
From: Thomas Petazzoni @ 2008-07-31 16:26 UTC (permalink / raw)
To: Adrian Bunk
Cc: linux-kernel, linux-embedded, michael, Matt Mackall, matthew,
linux-fsdevel, akpm
In-Reply-To: <20080731153757.GB20212@cs181140183.pp.htv.fi>
Le Thu, 31 Jul 2008 18:37:57 +0300,
Adrian Bunk <bunk@kernel.org> a écrit :
> I'm just not a fan of adding config options for each few kB of code -
> we have to maintain them and the more complex the configuration
> becomes the more often it breaks.
I'm not a fan of these too, but are there other solutions ?
> What became bigger was most likely not related to the patches you
> sent.
No, it is not.
> Where and why did the kernel become bigger?
It's not up-to-date with 2.6.26 and 2.6.27-rc1, but Bloatwatch
<http://www.selenic.com/bloatwatch/>, by Matt Mackall, is here to
answer these questions. I haven't made the analysis for
2.6.26->2.6.27-rc1.
> Why did CONFIG_FW_LOADER get enabled?
> Due to alnoconfig disabling CONFIG_EMBEDDED?
I don't know. Haven't made the analysis for now.
> A user will ask:
> I'm using $applications with $libraries, can I safely disable this
> option?
Hard to tell in the general case.
> And e.g. according to a quick grep through the sources uClibc's
> updwtmp() seems to cease working without flock().
Correct. But on many embedded systems, we don't care about logging past
user logins. We might even not care about logins at all.
> It costs us maintainance of the option and the #ifdef's and gives
> users one way more to shoot themselves into the foot in nontrivial to
> detect ways.
That's correct, and as I said previously, I fully understand the
maintainance problem of all these new configuration options. I must
admit that I do not really have more objective technical arguments that
would help us deciding whether the code size reduction vs. code
maintainance choice should be made in one direction or the other.
Sincerly,
Thomas
--
Thomas Petazzoni, Free Electrons
Kernel, drivers and embedded Linux development,
consulting, training and support.
http://free-electrons.com
--
To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply
* Re: [patch 3/4] Configure out ethtool support
From: Adrian Bunk @ 2008-07-31 15:58 UTC (permalink / raw)
To: David Woodhouse
Cc: David Miller, thomas.petazzoni, linux-kernel, linux-embedded,
michael, mpm, jgarzik, netdev, akpm
In-Reply-To: <1217504801.3454.170.camel@pmac.infradead.org>
On Thu, Jul 31, 2008 at 12:46:41PM +0100, David Woodhouse wrote:
> On Thu, 2008-07-31 at 04:33 -0700, David Miller wrote:
> > From: David Woodhouse <dwmw2@infradead.org>
> > Date: Thu, 31 Jul 2008 12:29:30 +0100
> >
> > > After an offline discussion, I understand that if we can sort out the
> > > actual technical issues, you'll carry this in the net tree. Thanks.
> >
> > I will, but only because you threatened to bypass me and send them
> > directly to Linus. And frankly fighting someone willing to do things
> > like that is simply not worth my time, so I'll just merge them
> > blindly.
>
> That doesn't make a lot of sense, Dave. Just because I _submit_ them to
> Linus, that doesn't mean he automatically takes them.
>
> I only said I'd submit them directly to Linus because I _think_ he'd
> agree with Andrew and I, and take them despite your objections. And
> because I think that's the right thing for him to do.
>...
I'm sure we can find simpler and less controversial ways to save 6 kB in
the network stack. E.g. Ilpo's past work on making inline functions in
the network stack out-of-line had far bigger effects than the patch in
this discussion. And as a bonus, his work brings benefits to everyone.
It might have also made more sense to spend some of the energy used in
this discussion instead on checking where the global 24 kB size increase
Thomas reported for 2.6.27-rc1 compared to 2.6.26 comes from.
It's kinda silly to spend time on creating and arguing about non-trivial
patches that might save a few kB for very few people while noone seems
to work on reducing the continuous size increase of the kernel that
affects everyone...
> dwmw2
cu
Adrian
--
"Is there not promise of rain?" Ling Tan asked suddenly out
of the darkness. There had been need of rain for many days.
"Only a promise," Lao Er said.
Pearl S. Buck - Dragon Seed
^ permalink raw reply
* Re: [patch 2/4] Configure out file locking features
From: Adrian Bunk @ 2008-07-31 15:37 UTC (permalink / raw)
To: Thomas Petazzoni
Cc: linux-kernel, linux-embedded, michael, Matt Mackall, matthew,
linux-fsdevel, akpm
In-Reply-To: <20080731162007.285938e0@surf>
On Thu, Jul 31, 2008 at 04:20:07PM +0200, Thomas Petazzoni wrote:
> Le Thu, 31 Jul 2008 16:53:19 +0300,
> Adrian Bunk <bunk@kernel.org> a écrit :
>
> > As I've already said in the past I'm personally not a huge fan of
> > these patches, but if it brings advantages in real-life situations
> > it's hard to argue against it.
>
> Yes, I've seen your points about that kind of patches on
> linux-embedded, and I understand them. I agree that adding dozens and
> dozens of configuration items for small features doesn't look like
> something sustainable on the long run. However, the kernel keeps
> growing and this isn't sustainable either on the long run for *some*
> embedded users. So, what should we do ? (That's a real question)
I'm not against making the kernel smaller.
I'm just not a fan of adding config options for each few kB of code -
we have to maintain them and the more complex the configuration becomes
the more often it breaks.
> Some numbers about a bootable x86 allnoconfig kernel with ELF, ext2 and
> IDE support :
>
> text data bss dec hex filename
> 1110389 119468 217088 1446945 161421 vmlinux.2.6.26
> 1134606 118840 212992 1466438 166046 vmlinux.2.6.27-rc1
> 24217 -628 -4096 19493 4C25 +/-
What became bigger was most likely not related to the patches you sent.
Where and why did the kernel become bigger?
> (The only configuration change between the two kernels is
> CONFIG_FW_LOADER n->y, which pulls drivers/base/firmware_class.o, 3k).
Why did CONFIG_FW_LOADER get enabled?
Due to alnoconfig disabling CONFIG_EMBEDDED?
> > In which use cases can users safely disable this option, and on what
> > devices have you verified that CONFIG_FILE_LOCKING=n kernels actually
> > work in practice?
>
> As long as they don't use NFS (realistic in many production
> environments) and that the applications do not rely on advisory locking
> (flock() and fnctl() F_GETLK and F_SETLK), file locking can be
> disabled.
But what does that mean in practice?
A user will ask:
I'm using $applications with $libraries, can I safely disable this option?
And e.g. according to a quick grep through the sources uClibc's
updwtmp() seems to cease working without flock().
It costs us maintainance of the option and the #ifdef's and gives users
one way more to shoot themselves into the foot in nontrivial to detect
ways.
> In practice, I only tested a CONFIG_FILE_LOCKING=n kernel
> with a basic Busybox under Qemu.
>
> Sincerly,
>
> Thomas
cu
Adrian
--
"Is there not promise of rain?" Ling Tan asked suddenly out
of the darkness. There had been need of rain for many days.
"Only a promise," Lao Er said.
Pearl S. Buck - Dragon Seed
^ permalink raw reply
* Re: [patch 2/4] Configure out file locking features
From: Thomas Petazzoni @ 2008-07-31 14:20 UTC (permalink / raw)
To: Adrian Bunk
Cc: linux-kernel, linux-embedded, michael, Matt Mackall, matthew,
linux-fsdevel, akpm
In-Reply-To: <20080731135319.GA20212@cs181140183.pp.htv.fi>
Le Thu, 31 Jul 2008 16:53:19 +0300,
Adrian Bunk <bunk@kernel.org> a écrit :
> As I've already said in the past I'm personally not a huge fan of
> these patches, but if it brings advantages in real-life situations
> it's hard to argue against it.
Yes, I've seen your points about that kind of patches on
linux-embedded, and I understand them. I agree that adding dozens and
dozens of configuration items for small features doesn't look like
something sustainable on the long run. However, the kernel keeps
growing and this isn't sustainable either on the long run for *some*
embedded users. So, what should we do ? (That's a real question)
Some numbers about a bootable x86 allnoconfig kernel with ELF, ext2 and
IDE support :
text data bss dec hex filename
1110389 119468 217088 1446945 161421 vmlinux.2.6.26
1134606 118840 212992 1466438 166046 vmlinux.2.6.27-rc1
24217 -628 -4096 19493 4C25 +/-
(The only configuration change between the two kernels is
CONFIG_FW_LOADER n->y, which pulls drivers/base/firmware_class.o, 3k).
> In which use cases can users safely disable this option, and on what
> devices have you verified that CONFIG_FILE_LOCKING=n kernels actually
> work in practice?
As long as they don't use NFS (realistic in many production
environments) and that the applications do not rely on advisory locking
(flock() and fnctl() F_GETLK and F_SETLK), file locking can be
disabled. In practice, I only tested a CONFIG_FILE_LOCKING=n kernel
with a basic Busybox under Qemu.
Sincerly,
Thomas
--
Thomas Petazzoni, Free Electrons
Kernel, drivers and embedded Linux development,
consulting, training and support.
http://free-electrons.com
--
To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply
* Re: [patch 2/4] Configure out file locking features
From: Adrian Bunk @ 2008-07-31 13:53 UTC (permalink / raw)
To: Thomas Petazzoni
Cc: linux-kernel, linux-embedded, michael, Matt Mackall, matthew,
linux-fsdevel, akpm
In-Reply-To: <20080731093220.969460336@free-electrons.com>
On Thu, Jul 31, 2008 at 11:27:05AM +0200, Thomas Petazzoni wrote:
> This patch adds the CONFIG_FILE_LOCKING option which allows to remove
> support for advisory locks. With this patch enabled, the flock()
> system call, the F_GETLK, F_SETLK and F_SETLKW operations of fcntl()
> and NFS support are disabled. These features are not necessarly needed
> on embedded systems. It allows to save ~11 Kb of kernel code and data:
>
> text data bss dec hex filename
> 1125436 118764 212992 1457192 163c28 vmlinux.old
> 1114299 118564 212992 1445855 160fdf vmlinux
> -11137 -200 0 -11337 -2C49 +/-
>
> This patch has originally been written by Matt Mackall
> <mpm@selenic.com>, and is part of the Linux Tiny project.
>...
As I've already said in the past I'm personally not a huge fan of these
patches, but if it brings advantages in real-life situations it's hard
to argue against it.
In which use cases can users safely disable this option, and on what
devices have you verified that CONFIG_FILE_LOCKING=n kernels actually
work in practice?
cu
Adrian
--
"Is there not promise of rain?" Ling Tan asked suddenly out
of the darkness. There had been need of rain for many days.
"Only a promise," Lao Er said.
Pearl S. Buck - Dragon Seed
^ permalink raw reply
* conceptual map of the kernel source (Interactive Linux kernel map)
From: Constantine Shulyupin @ 2008-07-31 13:44 UTC (permalink / raw)
To: linux-kernel, linux-embedded
Hi,
I am updating the kernel map: http://www.makelinux.net/kernel_map
It is conceptual map consisting of more than 200 most important kernel
functions.
Could you please have a look and suggest fixes and updates.
PS:
png image: http://www.makelinux.info/kernel_map.d/LKM63_2048.png
Other comments:
http://makelinux.wordpress.com/2008/06/07/comments-on-interactive-linux-kernel-map/
Regards.
--
Constantine Shulyupin
http://www.linuxdriver.co.il/
^ permalink raw reply
* Re: [patch 3/4] Configure out ethtool support
From: David Miller @ 2008-07-31 11:50 UTC (permalink / raw)
To: dwmw2
Cc: thomas.petazzoni, linux-kernel, linux-embedded, michael, mpm,
jgarzik, netdev, akpm
In-Reply-To: <1217504801.3454.170.camel@pmac.infradead.org>
From: David Woodhouse <dwmw2@infradead.org>
Date: Thu, 31 Jul 2008 12:46:41 +0100
> I only said I'd submit them directly to Linus because I _think_ he'd
> agree with Andrew and I, and take them despite your objections. And
> because I think that's the right thing for him to do.
I guess Linus is unable to participate in the discussion, voice his
opinion, and sort this out with the rest of us unless you submit the
patches directly to him for inclusion, right?
Can you at least begin to see why your doing that might irritate me?
> > You just let me know when you think they are ready for inclusion.
>
> I'll do that; thanks.
No problem.
^ permalink raw reply
* Re: [patch 3/4] Configure out ethtool support
From: David Woodhouse @ 2008-07-31 11:46 UTC (permalink / raw)
To: David Miller
Cc: thomas.petazzoni, linux-kernel, linux-embedded, michael, mpm,
jgarzik, netdev, akpm
In-Reply-To: <20080731.043355.143045459.davem@davemloft.net>
On Thu, 2008-07-31 at 04:33 -0700, David Miller wrote:
> From: David Woodhouse <dwmw2@infradead.org>
> Date: Thu, 31 Jul 2008 12:29:30 +0100
>
> > After an offline discussion, I understand that if we can sort out the
> > actual technical issues, you'll carry this in the net tree. Thanks.
>
> I will, but only because you threatened to bypass me and send them
> directly to Linus. And frankly fighting someone willing to do things
> like that is simply not worth my time, so I'll just merge them
> blindly.
That doesn't make a lot of sense, Dave. Just because I _submit_ them to
Linus, that doesn't mean he automatically takes them.
I only said I'd submit them directly to Linus because I _think_ he'd
agree with Andrew and I, and take them despite your objections. And
because I think that's the right thing for him to do.
I wasn't going to hack hera and just put them into his tree by myself.
But don't let me talk you out of taking the patches... :)
> You just let me know when you think they are ready for inclusion.
I'll do that; thanks.
--
dwmw2
^ permalink raw reply
* Re: [patch 3/4] Configure out ethtool support
From: David Miller @ 2008-07-31 11:33 UTC (permalink / raw)
To: dwmw2
Cc: thomas.petazzoni, linux-kernel, linux-embedded, michael, mpm,
jgarzik, netdev, akpm
In-Reply-To: <1217503770.3454.162.camel@pmac.infradead.org>
From: David Woodhouse <dwmw2@infradead.org>
Date: Thu, 31 Jul 2008 12:29:30 +0100
> After an offline discussion, I understand that if we can sort out the
> actual technical issues, you'll carry this in the net tree. Thanks.
I will, but only because you threatened to bypass me and send them
directly to Linus. And frankly fighting someone willing to do things
like that is simply not worth my time, so I'll just merge them blindly.
You just let me know when you think they are ready for inclusion.
^ permalink raw reply
* Re: [patch 3/4] Configure out ethtool support
From: David Woodhouse @ 2008-07-31 11:29 UTC (permalink / raw)
To: David Miller
Cc: thomas.petazzoni, linux-kernel, linux-embedded, michael, mpm,
jgarzik, netdev, akpm
In-Reply-To: <20080731.035121.177567346.davem@davemloft.net>
On Thu, 2008-07-31 at 03:51 -0700, David Miller wrote:
> CONFIG_INET needs it too.
>
> dev_disable_lro() calls the ethtool ops directly.
Ah, right. That's why it didn't show up in my grep.
There are solutions to that, as I said. Either we could 'select ETHTOOL'
in those drivers which enable LRO by default, or we could just make sure
they _don't_ enable LRO by default when CONFIG_ETHTOOL isn't set.
And if we end up doing LRO generically in software where the hardware
doesn't handle it, presumably we can do that without having to use
ethtool to change the hardware behaviour?
> But it still needs to be conditional, because as I said what I see
> happening next is this CONFIG_ETHTOOL thing getting jammed into each
> and every network driver. (in fact, ethtool support code in a single
> driver probably drarfs the 6K savings this initial patch is giving
> us).
I think we can avoid that. If the actual functions and the struct
ethtool_ops are static, and if we have something like...
#ifdef CONFIG_ETHTOOL
#define dev_set_ethtool_ops(dev, ops) dev->ethtool_ops = ops
#else
#define dev_set_ethtool_ops(dev, ops) (void)ops
#endif
... then it should all fall out silently. (Yeah, those definitions would
need 'hardening' but they're a proof of concept).
> And at which point you'll have a broken system for drivers that
> enable LRO and the user enables forwarding.
Obviously that needs avoiding. Thanks for the technical feedback.
After an offline discussion, I understand that if we can sort out the
actual technical issues, you'll carry this in the net tree. Thanks.
--
David Woodhouse Open Source Technology Centre
David.Woodhouse@intel.com Intel Corporation
^ permalink raw reply
* Re: [patch 3/4] Configure out ethtool support
From: David Miller @ 2008-07-31 10:57 UTC (permalink / raw)
To: dwmw2
Cc: bhutchings, thomas.petazzoni, linux-kernel, linux-embedded,
michael, mpm, jgarzik, netdev, akpm
In-Reply-To: <1217501664.3454.151.camel@pmac.infradead.org>
From: David Woodhouse <dwmw2@infradead.org>
Date: Thu, 31 Jul 2008 11:54:24 +0100
> Other potential approaches include not enabling LRO by default if
> !CONFIG_ETHTOOL. Or having the driver(s) which _do_ enable LRO by
> default 'select ETHTOOL'.
It is possible for us to generically enable LRO for every device,
since it is a software algorithm. And likely we will do something
like this in the not too distant future.
^ permalink raw reply
* Re: [patch 3/4] Configure out ethtool support
From: David Woodhouse @ 2008-07-31 10:54 UTC (permalink / raw)
To: David Miller
Cc: bhutchings, thomas.petazzoni, linux-kernel, linux-embedded,
michael, mpm, jgarzik, netdev, akpm
In-Reply-To: <20080731.034904.190066778.davem@davemloft.net>
On Thu, 2008-07-31 at 03:49 -0700, David Miller wrote:
> From: Ben Hutchings <bhutchings@solarflare.com>
> Date: Thu, 31 Jul 2008 11:40:05 +0100
>
> > You also need to conditionalise dev_disable_lro().
>
> That can only be done once the CONFIG_ETHTOOL select statement
> is added for CONFIG_INET.
>
> Which basically makes this CONFIG_ETHTOOL thing completely pointless.
Other potential approaches include not enabling LRO by default if
!CONFIG_ETHTOOL. Or having the driver(s) which _do_ enable LRO by
default 'select ETHTOOL'.
--
dwmw2
^ permalink raw reply
* Re: [patch 3/4] Configure out ethtool support
From: David Miller @ 2008-07-31 10:51 UTC (permalink / raw)
To: dwmw2
Cc: thomas.petazzoni, linux-kernel, linux-embedded, michael, mpm,
jgarzik, netdev, akpm
In-Reply-To: <1217500967.3454.140.camel@pmac.infradead.org>
From: David Woodhouse <dwmw2@infradead.org>
Date: Thu, 31 Jul 2008 11:42:47 +0100
> It's alleged that these functions are called from 'core' network code in
> some places, although I can't actually see any evidence of that anywhere
> in Linus' tree except for vlans and bridging.
>
> If that's actually the case, perhaps it makes sense to add a
> WARN_ON_ONCE() to these empty functions, so that a developer who
> disables CONFIG_ETHTOOL when they shouldn't will see a nasty warning
> about it rather than a silent failure?
>
> Btw, I see you've made bridging 'select ETHTOOL'; did you do the same
> for vlan support?
CONFIG_INET needs it too.
dev_disable_lro() calls the ethtool ops directly.
But it still needs to be conditional, because as I said what I see
happening next is this CONFIG_ETHTOOL thing getting jammed into each
and every network driver. (in fact, ethtool support code in a single
driver probably drarfs the 6K savings this initial patch is giving
us).
And at which point you'll have a broken system for drivers that
enable LRO and the user enables forwarding.
^ permalink raw reply
* Re: [patch 3/4] Configure out ethtool support
From: David Miller @ 2008-07-31 10:49 UTC (permalink / raw)
To: bhutchings
Cc: thomas.petazzoni, linux-kernel, linux-embedded, michael, mpm,
jgarzik, netdev, akpm
In-Reply-To: <20080731104004.GN10471@solarflare.com>
From: Ben Hutchings <bhutchings@solarflare.com>
Date: Thu, 31 Jul 2008 11:40:05 +0100
> You also need to conditionalise dev_disable_lro().
That can only be done once the CONFIG_ETHTOOL select statement
is added for CONFIG_INET.
Which basically makes this CONFIG_ETHTOOL thing completely pointless.
^ permalink raw reply
* Re: [patch 3/4] Configure out ethtool support
From: David Woodhouse @ 2008-07-31 10:42 UTC (permalink / raw)
To: Thomas Petazzoni
Cc: linux-kernel, linux-embedded, michael, Matt Mackall, jgarzik,
netdev, davem, akpm
In-Reply-To: <20080731093221.236840420@free-electrons.com>
On Thu, 2008-07-31 at 11:27 +0200, Thomas Petazzoni wrote:
>
> +#else
> +static inline u32 ethtool_op_get_link(struct net_device *dev) { return 0; }
> +static inline u32 ethtool_op_get_tx_csum(struct net_device *dev) { return 0; }
> +static inline int ethtool_op_set_tx_csum(struct net_device *dev, u32 data) { return 0; }
> +static inline int ethtool_op_set_tx_hw_csum(struct net_device *dev, u32 data) { return 0; }
> +static inline int ethtool_op_set_tx_ipv6_csum(struct net_device *dev, u32 data) { return 0; }
> +static inline u32 ethtool_op_get_sg(struct net_device *dev) { return 0; }
> +static inline int ethtool_op_set_sg(struct net_device *dev, u32 data) { return 0; }
> +static inline u32 ethtool_op_get_tso(struct net_device *dev) { return 0; }
> +static inline int ethtool_op_set_tso(struct net_device *dev, u32 data) { return 0; }
> +static inline u32 ethtool_op_get_ufo(struct net_device *dev) { return 0; }
> +static inline int ethtool_op_set_ufo(struct net_device *dev, u32 data) { return 0; }
> +static inline u32 ethtool_op_get_flags(struct net_device *dev) { return 0; }
> +static inline int ethtool_op_set_flags(struct net_device *dev, u32 data) { return 0; }
> +#endif
It's alleged that these functions are called from 'core' network code in
some places, although I can't actually see any evidence of that anywhere
in Linus' tree except for vlans and bridging.
If that's actually the case, perhaps it makes sense to add a
WARN_ON_ONCE() to these empty functions, so that a developer who
disables CONFIG_ETHTOOL when they shouldn't will see a nasty warning
about it rather than a silent failure?
Btw, I see you've made bridging 'select ETHTOOL'; did you do the same
for vlan support?
--
David Woodhouse Open Source Technology Centre
David.Woodhouse@intel.com Intel Corporation
^ permalink raw reply
* Re: [patch 3/4] Configure out ethtool support
From: Ben Hutchings @ 2008-07-31 10:40 UTC (permalink / raw)
To: Thomas Petazzoni
Cc: linux-kernel, linux-embedded, michael, Matt Mackall, jgarzik,
netdev, davem, akpm
In-Reply-To: <20080731093221.236840420@free-electrons.com>
Thomas Petazzoni wrote:
[...]
> --- linuxdev.orig/include/linux/ethtool.h
> +++ linuxdev/include/linux/ethtool.h
> @@ -283,6 +283,7 @@
> struct net_device;
>
> /* Some generic methods drivers may use in their ethtool_ops */
> +#ifdef CONFIG_ETHTOOL
> u32 ethtool_op_get_link(struct net_device *dev);
> u32 ethtool_op_get_tx_csum(struct net_device *dev);
> int ethtool_op_set_tx_csum(struct net_device *dev, u32 data);
> @@ -296,6 +297,21 @@
> int ethtool_op_set_ufo(struct net_device *dev, u32 data);
> u32 ethtool_op_get_flags(struct net_device *dev);
> int ethtool_op_set_flags(struct net_device *dev, u32 data);
> +#else
> +static inline u32 ethtool_op_get_link(struct net_device *dev) { return 0; }
> +static inline u32 ethtool_op_get_tx_csum(struct net_device *dev) { return 0; }
> +static inline int ethtool_op_set_tx_csum(struct net_device *dev, u32 data) { return 0; }
> +static inline int ethtool_op_set_tx_hw_csum(struct net_device *dev, u32 data) { return 0; }
> +static inline int ethtool_op_set_tx_ipv6_csum(struct net_device *dev, u32 data) { return 0; }
> +static inline u32 ethtool_op_get_sg(struct net_device *dev) { return 0; }
> +static inline int ethtool_op_set_sg(struct net_device *dev, u32 data) { return 0; }
> +static inline u32 ethtool_op_get_tso(struct net_device *dev) { return 0; }
> +static inline int ethtool_op_set_tso(struct net_device *dev, u32 data) { return 0; }
> +static inline u32 ethtool_op_get_ufo(struct net_device *dev) { return 0; }
> +static inline int ethtool_op_set_ufo(struct net_device *dev, u32 data) { return 0; }
> +static inline u32 ethtool_op_get_flags(struct net_device *dev) { return 0; }
> +static inline int ethtool_op_set_flags(struct net_device *dev, u32 data) { return 0; }
The dummy setter functions should return -EOPNOTSUPP. The getter functions
just read device feature flags and could be made inline. They have no way
of returning failure.
[...]
> ===================================================================
> --- linuxdev.orig/net/core/dev.c
> +++ linuxdev/net/core/dev.c
> @@ -3669,6 +3669,7 @@
> return ret;
>
> case SIOCETHTOOL:
> +#ifdef CONFIG_ETHTOOL
> dev_load(net, ifr.ifr_name);
> rtnl_lock();
> ret = dev_ethtool(net, &ifr);
> @@ -3681,6 +3682,9 @@
> ret = -EFAULT;
> }
> return ret;
> +#else
> + return -EINVAL;
> +#endif
>
> /*
> * These ioctl calls:
You also need to conditionalise dev_disable_lro().
Ben.
--
Ben Hutchings, Senior Software Engineer, Solarflare Communications
Not speaking for my employer; that's the marketing department's job.
They asked us to note that Solarflare product names are trademarked.
^ permalink raw reply
* Re: [patch 0/4] [resend] Add configuration options to disable features not needed on embedded devices
From: David Miller @ 2008-07-31 10:25 UTC (permalink / raw)
To: dwmw2; +Cc: torvalds, thomas.petazzoni, linux-kernel, linux-embedded, michael
In-Reply-To: <1217499316.3454.118.camel@pmac.infradead.org>
From: David Woodhouse <dwmw2@infradead.org>
Date: Thu, 31 Jul 2008 11:15:16 +0100
> While I agree with Andrew's observation, I'd also respectfully submit
> that your argument is more fundamentally bogus than that. TCP and UDP
> are _not_ universally available. They go away if you set CONFIG_INET=n.
Like I said, people can locally patch their systems if they really
want to rip out fundamental things like multicast support.
Some folks might find it instructive to do a google code search
or similar on the multicast socket options this things dikes out
of the tree.
Even simple things like NTP will spew failures with this CONFIG_IGMP
thing turned off. And that's just the tip of the iceberg.
^ permalink raw reply
* Re: [patch 0/4] [resend] Add configuration options to disable features not needed on embedded devices
From: David Woodhouse @ 2008-07-31 10:15 UTC (permalink / raw)
To: David Miller
Cc: torvalds, thomas.petazzoni, linux-kernel, linux-embedded, michael
In-Reply-To: <20080731.030205.153067850.davem@davemloft.net>
On Thu, 2008-07-31 at 03:02 -0700, David Miller wrote:
> I explained why I didn't want to apply the IGMP one too.
>
> Andrew didn't like my objections, but that doesn't mean I
> need to defend my position further.
You said that it was part of the core BSD socket API and "Like TCP and
UDP, multicast capabilities are something applications can always depend
upon being available".
Andrew's response was that embedded devices have full control over their
userspace and that they are in a position to _know_ that they never use
multicast, so your argument is bogus. If they _know_ they don't want
multicast, it makes a lot of sense for them to turn it off.
While I agree with Andrew's observation, I'd also respectfully submit
that your argument is more fundamentally bogus than that. TCP and UDP
are _not_ universally available. They go away if you set CONFIG_INET=n.
--
dwmw2
^ permalink raw reply
* Re: [patch 1/4] Configure out AIO support
From: Adrian Bunk @ 2008-07-31 10:12 UTC (permalink / raw)
To: Bernhard Fischer
Cc: Thomas Petazzoni, linux-kernel, linux-embedded, michael,
Matt Mackall, bcrl, linux-aio, akpm
In-Reply-To: <20080731100929.GA9206@mx.loc>
On Thu, Jul 31, 2008 at 12:09:29PM +0200, Bernhard Fischer wrote:
> On Thu, Jul 31, 2008 at 11:27:04AM +0200, Thomas Petazzoni wrote:
> >This patchs adds the CONFIG_AIO option which allows to remove support
> >for asynchronous I/O operations, that are not necessarly used by
> >applications, particularly on embedded devices. As this is a
> >size-reduction option, it depends on CONFIG_EMBEDDED. It allows to
> >save ~7 kilobytes of kernel code/data:
>
> Shouldn't this also make sure not to install aio_abi.h or at least an
> empty aio_abi.h?
The userspace headers are independent of any kernel configuration
(except for the architecture).
cu
Adrian
--
"Is there not promise of rain?" Ling Tan asked suddenly out
of the darkness. There had been need of rain for many days.
"Only a promise," Lao Er said.
Pearl S. Buck - Dragon Seed
^ 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