Since there was some (albeit very brief) discussion last week about the
need to cleanup mm/slab.c, I figured I'd post these patches. I was
inspired to cleanup mm/slab.c since I'm working on a project (to be posted
shortly) that touched a bunch of slab code. I found slab.c to be
inconsistent, to say the least.
-Matt
Pretty self-explanatory. Whitespace cleanps, comment fixes,
spelling/typos, etc.
mcd@arrakis:~/linux/source/linux-2.6.14+slab_cleanup/patches $ diffstat
CodingStyle-slab_c.patch
slab.c | 653
+++++++++++++++++++++++++++++++++--------------------------------
1 files changed, 338 insertions(+), 315 deletions(-)
-Matt
We refer to a node number as: "nodeid", "node", "nid", and possibly other
names. Let's choose one, and I choose "nid".
mcd@arrakis:~/linux/source/linux-2.6.14+slab_cleanup/patches $ diffstat
use_nid.patch
slab.c | 250
++++++++++++++++++++++++++++++++---------------------------------
1 files changed, 125 insertions(+), 125 deletions(-)
-Matt
We don't actually ever use the 'align' parameter, so drop it.
mcd@arrakis:~/linux/source/linux-2.6.14+slab_cleanup/patches $ diffstat
alloc_percpu.patch
include/linux/percpu.h | 7 +++----
mm/slab.c | 10 +++-------
net/ipv6/af_inet6.c | 4 ++--
3 files changed, 8 insertions(+), 13 deletions(-)
-Matt
Cleanup kmem_cache_create()
mcd@arrakis:~/linux/source/linux-2.6.14+slab_cleanup/patches $ diffstat
kmem_cache_create.patch
slab.c | 69
++++++++++++++++++++++++++++-------------------------------------
1 files changed, 30 insertions(+), 39 deletions(-)
-Matt
Cleanup cache_reap().
Note, I did not include the change to remove the '+ smp_processor_id()'
from the schedule_delayed_work() calls. This may cause rejects, which I,
or any sane person :), can trivially resolve.
mcd@arrakis:~/linux/source/linux-2.6.14+slab_cleanup/patches $ diffstat
cache_reap.patch
slab.c | 36 +++++++++++++-----------------------
1 files changed, 13 insertions(+), 23 deletions(-)
-Matt
Cleanup slabinfo_write().
mcd@arrakis:~/linux/source/linux-2.6.14+slab_cleanup/patches $ diffstat
slabinfo_write.patch
slab.c | 23 +++++++++--------------
1 files changed, 9 insertions(+), 14 deletions(-)
-Matt
Cleanup a loop in set_slab_attr().
mcd@arrakis:~/linux/source/linux-2.6.14+slab_cleanup/patches $ diffstat
set_slab_attr.patch
slab.c | 4 ++--
1 files changed, 2 insertions(+), 2 deletions(-)
-Matt
I found three functions in slab.c that have only 1 caller (kmem_getpages,
alloc_slabmgmt, and set_slab_attr), so let's inline them.
mcd@arrakis:~/linux/source/linux-2.6.14+slab_cleanup/patches $ diffstat
inline_functions.patch
slab.c | 9 +++++----
1 files changed, 5 insertions(+), 4 deletions(-)
-Matt
> * Replace a constant (4096) with what it represents (PAGE_SIZE)
This seems dangerous. I don't pretend to understand the slab code,
but the current code works on architectures with PAGE_SIZE != 4096.
Are you sure this change is correct?
- R.
On Mon, 7 Nov 2005, Roland Dreier wrote:
> > * Replace a constant (4096) with what it represents (PAGE_SIZE)
>
> This seems dangerous. I don't pretend to understand the slab code,
> but the current code works on architectures with PAGE_SIZE != 4096.
> Are you sure this change is correct?
Looks ok to me except that it should be a separate patch (it is not a
trivial cleanup because it changes how the code works).
Pekka
On Mon, 7 Nov 2005, Matthew Dobson wrote:
> I found three functions in slab.c that have only 1 caller (kmem_getpages,
> alloc_slabmgmt, and set_slab_attr), so let's inline them.
Why? They aren't on the hot path and I don't see how this is an
improvement...
Pekka
On Mon, 7 Nov 2005, Matthew Dobson wrote:
> @@ -1652,9 +1649,9 @@ kmem_cache_t *kmem_cache_create(const ch
> * gfp() funcs are more friendly towards high-order requests,
> * this should be changed.
> */
> - do {
> - unsigned int break_flag = 0;
> -cal_wastage:
> + unsigned int break_flag = 0;
> +
> + for ( ; ; cachep->gfporder++) {
> cache_estimate(cachep->gfporder, size, align, flags,
> &left_over, &cachep->num);
> if (break_flag)
> @@ -1662,13 +1659,13 @@ cal_wastage:
> if (cachep->gfporder >= MAX_GFP_ORDER)
> break;
> if (!cachep->num)
> - goto next;
> - if (flags & CFLGS_OFF_SLAB &&
> - cachep->num > offslab_limit) {
> + continue;
> + if ((flags & CFLGS_OFF_SLAB) &&
> + (cachep->num > offslab_limit)) {
> /* This num of objs will cause problems. */
> - cachep->gfporder--;
> + cachep->gfporder -= 2;
This is not an improvement IMHO. The use of for construct is non-intuitive
and neither is the above. A suggested cleanup is to keep the loop as is but
extract it to a function of its own.
Pekka
On Mon, 7 Nov 2005, Matthew Dobson wrote:
> We refer to a node number as: "nodeid", "node", "nid", and possibly other
> names. Let's choose one, and I choose "nid".
Such a pity as nodeid is much more readable...
Pekka
On Mon, 7 Nov 2005, Matthew Dobson wrote:
> Since there was some (albeit very brief) discussion last week about the
> need to cleanup mm/slab.c, I figured I'd post these patches. I was
> inspired to cleanup mm/slab.c since I'm working on a project (to be posted
> shortly) that touched a bunch of slab code. I found slab.c to be
> inconsistent, to say the least.
Thank you for doing this. Overall, they look good to me except for the
bits I commented on. In future, please inline patches to the mail and cc
Manfred Spraul who more or less maintains mm/slab.c (curiously, I see no
entry in MAINTAINERS though).
Pekka
On Mon, Nov 07, 2005 at 04:57:27PM -0800, Matthew Dobson wrote:
> * Set 'res' at declaration instead of later in the function.
I hate to initialize a varible two miles away from the place where it's
used.
> --- linux-2.6.14+slab_cleanup.orig/mm/slab.c
> +++ linux-2.6.14+slab_cleanup/mm/slab.c
> @@ -3533,7 +3533,7 @@ ssize_t slabinfo_write(struct file *file
> size_t count, loff_t *ppos)
> {
> - int limit, batchcount, shared, res;
> + int limit, batchcount, shared, res = -EINVAL;
> /* Find the cache in the chain of caches. */
> down(&cache_chain_sem);
> - res = -EINVAL;
> list_for_each(p,&cache_chain) {
> kmem_cache_t *cachep = list_entry(p, kmem_cache_t, next);
> + if (strcmp(cachep->name, kbuf))
> + continue;
>
> - if (!strcmp(cachep->name, kbuf)) {
> - if (limit < 1 || batchcount < 1 ||
> - batchcount > limit || shared < 0) {
> - res = 0;
> - } else {
> - res = do_tune_cpucache(cachep, limit,
> - batchcount, shared);
> - }
> - break;
> - }
> + res = do_tune_cpucache(cachep, limit, batchcount, shared);
> + if (res >= 0)
> + res = count;
> + break;
> }
> up(&cache_chain_sem);
> - if (res >= 0)
> - res = count;
> return res;
> }
On Mon, Nov 07, 2005 at 04:53:33PM -0800, Matthew Dobson wrote:
> @@ -1652,9 +1649,9 @@ kmem_cache_t *kmem_cache_create(const ch
> * gfp() funcs are more friendly towards high-order requests,
> * this should be changed.
> */
> - do {
> - unsigned int break_flag = 0;
> -cal_wastage:
> + unsigned int break_flag = 0;
> +
> + for ( ; ; cachep->gfporder++) {
> cache_estimate(cachep->gfporder, size, align, flags,
> &left_over, &cachep->num);
> if (break_flag)
> @@ -1662,13 +1659,13 @@ cal_wastage:
> if (cachep->gfporder >= MAX_GFP_ORDER)
> break;
> if (!cachep->num)
> - goto next;
> - if (flags & CFLGS_OFF_SLAB &&
> - cachep->num > offslab_limit) {
> + continue;
> + if ((flags & CFLGS_OFF_SLAB) &&
> + (cachep->num > offslab_limit)) {
> /* This num of objs will cause problems. */
> - cachep->gfporder--;
> + cachep->gfporder -= 2;
> break_flag++;
> - goto cal_wastage;
> + continue;
> }
>
> /*
> @@ -1680,33 +1677,29 @@ cal_wastage:
>
> if ((left_over*8) <= (PAGE_SIZE<<cachep->gfporder))
> break; /* Acceptable internal fragmentation. */
> -next:
> - cachep->gfporder++;
> - } while (1);
> + }
> }
I also don't like your changes to this. Might I suggest:
Index: mm/slab.c
===================================================================
RCS file: /var/cvs/linux-2.6/mm/slab.c,v
retrieving revision 1.31
diff -u -p -r1.31 slab.c
--- mm/slab.c 14 Feb 2005 02:55:36 -0000 1.31
+++ mm/slab.c 8 Nov 2005 14:58:35 -0000
@@ -1150,6 +1150,53 @@ static void slab_destroy (kmem_cache_t *
}
}
+/*
+ * Calculate size (in pages) of slabs, and the num of objs per slab. This
+ * could be made much more intelligent. For now, try to avoid using high
+ * page-orders for slabs. When the gfp() funcs are more friendly towards
+ * high-order requests, this should be changed.
+ */
+static size_t find_best_slab_order(kmem_cache_t *cachep, size_t size,
+ size_t align, unsigned long flags)
+{
+ size_t left_over = 0;
+
+ for ( ; ; cachep->gfporder++) {
+ unsigned int num;
+ size_t remainder;
+
+ if (cachep->gfporder > MAX_GFP_ORDER) {
+ cachep->num = 0;
+ break;
+ }
+
+ cache_estimate(cachep->gfporder, size, align, flags,
+ &remainder, &num);
+ if (!num)
+ continue;
+
+ if (flags & CFLGS_OFF_SLAB && num > offslab_limit) {
+ /* This num of objs will cause problems. */
+ break;
+ }
+
+ cachep->num = num;
+ left_over = remainder;
+
+ /*
+ * Large num of objs is good, but v. large slabs are
+ * currently bad for the gfp()s.
+ */
+ if (cachep->gfporder >= slab_break_gfp_order)
+ break;
+
+ if ((left_over*8) <= (PAGE_SIZE<<cachep->gfporder))
+ break; /* Acceptable internal fragmentation. */
+ }
+
+ return left_over;
+}
+
/**
* kmem_cache_create - Create a cache.
* @name: A string which is used in /proc/slabinfo to identify this cache.
@@ -1330,44 +1377,7 @@ kmem_cache_create (const char *name, siz
cache_estimate(cachep->gfporder, size, align, flags,
&left_over, &cachep->num);
} else {
- /*
- * Calculate size (in pages) of slabs, and the num of objs per
- * slab. This could be made much more intelligent. For now,
- * try to avoid using high page-orders for slabs. When the
- * gfp() funcs are more friendly towards high-order requests,
- * this should be changed.
- */
- do {
- unsigned int break_flag = 0;
-cal_wastage:
- cache_estimate(cachep->gfporder, size, align, flags,
- &left_over, &cachep->num);
- if (break_flag)
- break;
- if (cachep->gfporder >= MAX_GFP_ORDER)
- break;
- if (!cachep->num)
- goto next;
- if (flags & CFLGS_OFF_SLAB &&
- cachep->num > offslab_limit) {
- /* This num of objs will cause problems. */
- cachep->gfporder--;
- break_flag++;
- goto cal_wastage;
- }
-
- /*
- * Large num of objs is good, but v. large slabs are
- * currently bad for the gfp()s.
- */
- if (cachep->gfporder >= slab_break_gfp_order)
- break;
-
- if ((left_over*8) <= (PAGE_SIZE<<cachep->gfporder))
- break; /* Acceptable internal fragmentation. */
-next:
- cachep->gfporder++;
- } while (1);
+ left_over = find_best_slab_order(cachep, size, align, flags);
}
if (!cachep->num) {
On Tue, 8 Nov 2005, Matthew Wilcox wrote:
> +/*
> + * Calculate size (in pages) of slabs, and the num of objs per slab. This
> + * could be made much more intelligent. For now, try to avoid using high
> + * page-orders for slabs. When the gfp() funcs are more friendly towards
> + * high-order requests, this should be changed.
> + */
> +static size_t find_best_slab_order(kmem_cache_t *cachep, size_t size,
> + size_t align, unsigned long flags)
> +{
Looks ok to me. I would prefer this to be called calculate_slab_order()
instead though.
Pekka
Pekka J Enberg wrote:
> On Mon, 7 Nov 2005, Roland Dreier wrote:
>
>
>> > * Replace a constant (4096) with what it represents (PAGE_SIZE)
>>
>>This seems dangerous. I don't pretend to understand the slab code,
>>but the current code works on architectures with PAGE_SIZE != 4096.
>>Are you sure this change is correct?
>
>
> Looks ok to me except that it should be a separate patch (it is not a
> trivial cleanup because it changes how the code works).
>
> Pekka
That's very reasonable, Pekka. I will respin 4/8 without that change and
add a 9/8 that is JUST that one change.
Thank you both for the review and comments!
-Matt
On Mon, 7 Nov 2005, Roland Dreier wrote:
> > * Replace a constant (4096) with what it represents (PAGE_SIZE)
>
> This seems dangerous. I don't pretend to understand the slab code,
> but the current code works on architectures with PAGE_SIZE != 4096.
> Are you sure this change is correct?
Leave the constant. The 4096 is only used for debugging and is a boundary
at which redzoning and last user accounting is given up.
A large object in terms of this patch is a object greater than 4096 bytes
not an object greater than PAGE_SIZE. I think the absolute size is
desired.
Would you CC manfred on all your patches?
Pekka J Enberg wrote:
> On Mon, 7 Nov 2005, Matthew Dobson wrote:
>
>>@@ -1652,9 +1649,9 @@ kmem_cache_t *kmem_cache_create(const ch
>> * gfp() funcs are more friendly towards high-order requests,
>> * this should be changed.
>> */
>>- do {
>>- unsigned int break_flag = 0;
>>-cal_wastage:
>>+ unsigned int break_flag = 0;
>>+
>>+ for ( ; ; cachep->gfporder++) {
>> cache_estimate(cachep->gfporder, size, align, flags,
>> &left_over, &cachep->num);
>> if (break_flag)
>>@@ -1662,13 +1659,13 @@ cal_wastage:
>> if (cachep->gfporder >= MAX_GFP_ORDER)
>> break;
>> if (!cachep->num)
>>- goto next;
>>- if (flags & CFLGS_OFF_SLAB &&
>>- cachep->num > offslab_limit) {
>>+ continue;
>>+ if ((flags & CFLGS_OFF_SLAB) &&
>>+ (cachep->num > offslab_limit)) {
>> /* This num of objs will cause problems. */
>>- cachep->gfporder--;
>>+ cachep->gfporder -= 2;
>
>
> This is not an improvement IMHO. The use of for construct is non-intuitive
> and neither is the above. A suggested cleanup is to keep the loop as is but
> extract it to a function of its own.
>
> Pekka
To me the for loop is more readable and intuitive, but that is definitely a
matter of opinion. Moving the code to it's own helper function is a better
idea than leaving it alone, or changing to a for loop, though. Will resend
later today.
Thanks!
-Matt
Pekka J Enberg wrote:
> On Mon, 7 Nov 2005, Matthew Dobson wrote:
>
>>Since there was some (albeit very brief) discussion last week about the
>>need to cleanup mm/slab.c, I figured I'd post these patches. I was
>>inspired to cleanup mm/slab.c since I'm working on a project (to be posted
>>shortly) that touched a bunch of slab code. I found slab.c to be
>>inconsistent, to say the least.
>
>
> Thank you for doing this. Overall, they look good to me except for the
> bits I commented on. In future, please inline patches to the mail and cc
> Manfred Spraul who more or less maintains mm/slab.c (curiously, I see no
> entry in MAINTAINERS though).
>
> Pekka
As there have been many comments regarding the patches (many more than I
expected! :), I'll resend the whole series later today, and I'll be sure to
cc Manfred. If he wants, I'll even include a patch to add him to the
MAINTAINERS file...?
-Matt
On Tue, 8 Nov 2005, Alexey Dobriyan wrote:
> On Mon, Nov 07, 2005 at 04:57:27PM -0800, Matthew Dobson wrote:
> > * Set 'res' at declaration instead of later in the function.
>
> I hate to initialize a varible two miles away from the place where it's
> used.
> > - int limit, batchcount, shared, res;
> > + int limit, batchcount, shared, res = -EINVAL;
Looks more confusing than before.
On Tue, 8 Nov 2005, Pekka J Enberg wrote:
> On Mon, 7 Nov 2005, Matthew Dobson wrote:
> > I found three functions in slab.c that have only 1 caller (kmem_getpages,
> > alloc_slabmgmt, and set_slab_attr), so let's inline them.
>
> Why? They aren't on the hot path and I don't see how this is an
> improvement...
It avoids the call/return sequences so it may decrease code size a bit and
allow the compiler (if its up to the task) to do more CSE optimizations.
Christoph Lameter wrote:
> On Mon, 7 Nov 2005, Roland Dreier wrote:
>
>
>> > * Replace a constant (4096) with what it represents (PAGE_SIZE)
>>
>>This seems dangerous. I don't pretend to understand the slab code,
>>but the current code works on architectures with PAGE_SIZE != 4096.
>>Are you sure this change is correct?
>
>
> Leave the constant. The 4096 is only used for debugging and is a boundary
> at which redzoning and last user accounting is given up.
>
> A large object in terms of this patch is a object greater than 4096 bytes
> not an object greater than PAGE_SIZE. I think the absolute size is
> desired.
Would you be OK with at least NAMING the constant? I won't name it
PAGE_SIZE (of course), but LARGE_OBJECT_SIZE or something?
> Would you CC manfred on all your patches?
Yes. I will repost my patches later today and I will be sure to CC Manfred
on all of them.
Thanks for the review,
-Matt
Pekka J Enberg wrote:
> On Mon, 7 Nov 2005, Matthew Dobson wrote:
>
>>I found three functions in slab.c that have only 1 caller (kmem_getpages,
>>alloc_slabmgmt, and set_slab_attr), so let's inline them.
>
>
> Why? They aren't on the hot path and I don't see how this is an
> improvement...
>
> Pekka
Well, no, they aren't on the hot path. I just figured since they are only
ever called from one other function, why not inline them? If the sentiment
is that it's a BAD idea, I'll drop it.
-Matt
Christoph Lameter wrote:
> On Tue, 8 Nov 2005, Alexey Dobriyan wrote:
>
>
>>On Mon, Nov 07, 2005 at 04:57:27PM -0800, Matthew Dobson wrote:
>>
>>>* Set 'res' at declaration instead of later in the function.
>>
>>I hate to initialize a varible two miles away from the place where it's
>>used.
>
>
>
>
>>>- int limit, batchcount, shared, res;
>>>+ int limit, batchcount, shared, res = -EINVAL;
>
>
> Looks more confusing than before.
Fair enough. I'll drop that bit.
-Matt
On Tue, 8 Nov 2005, Matthew Dobson wrote:
> > A large object in terms of this patch is a object greater than 4096 bytes
> > not an object greater than PAGE_SIZE. I think the absolute size is
> > desired.
> Would you be OK with at least NAMING the constant? I won't name it
> PAGE_SIZE (of course), but LARGE_OBJECT_SIZE or something?
Ask Manfred about this. I think he coded it that way and he usually has
good reasons for it.
Thanks for the cleanup work!
Pekka J Enberg wrote:
> On Tue, 8 Nov 2005, Matthew Wilcox wrote:
>
>>+/*
>>+ * Calculate size (in pages) of slabs, and the num of objs per slab. This
>>+ * could be made much more intelligent. For now, try to avoid using high
>>+ * page-orders for slabs. When the gfp() funcs are more friendly towards
>>+ * high-order requests, this should be changed.
>>+ */
>>+static size_t find_best_slab_order(kmem_cache_t *cachep, size_t size,
>>+ size_t align, unsigned long flags)
>>+{
>
>
> Looks ok to me. I would prefer this to be called calculate_slab_order()
> instead though.
>
> Pekka
Agreed. Will include this in the next version, due out this afternoon.
Thank you both for the review and comments.
-Matt
Christoph Lameter wrote:
> On Tue, 8 Nov 2005, Matthew Dobson wrote:
>
>
>>>A large object in terms of this patch is a object greater than 4096 bytes
>>>not an object greater than PAGE_SIZE. I think the absolute size is
>>>desired.
>>
>>Would you be OK with at least NAMING the constant? I won't name it
>>PAGE_SIZE (of course), but LARGE_OBJECT_SIZE or something?
>
>
> Ask Manfred about this. I think he coded it that way and he usually has
> good reasons for it.
>
> Thanks for the cleanup work!
Manfred, any reason not to name this constant in slab.c? If there's a good
reason not to, I'm perfectly happy to leave it alone. :)
-Matt
Matthew Dobson wrote:
>Manfred, any reason not to name this constant in slab.c? If there's a good
>reason not to, I'm perfectly happy to leave it alone. :)
>
>
>
No, there is no reason. It's debug-only code, thus I was too lazy to
create a constant.
And no - don't make me maintainer of slab.c. I didn't even have enough
time to review the numa patches properly.
--
Manfred
On Tue, Nov 08, 2005 at 11:08:30AM -0800, Matthew Dobson wrote:
> Pekka J Enberg wrote:
> > On Mon, 7 Nov 2005, Matthew Dobson wrote:
> >
> >>I found three functions in slab.c that have only 1 caller (kmem_getpages,
> >>alloc_slabmgmt, and set_slab_attr), so let's inline them.
> >
> >
> > Why? They aren't on the hot path and I don't see how this is an
> > improvement...
> >
> > Pekka
>
> Well, no, they aren't on the hot path. I just figured since they are only
> ever called from one other function, why not inline them? If the sentiment
> is that it's a BAD idea, I'll drop it.
And if there will one day be a second caller, noone will remember to
remove the inline...
At least with unit-at-a-time [1], gcc should be smart enough to inline
all static functions when it does make sense.
> -Matt
cu
Adrian
[1] currently disabled in the kernel on i386, but this will change at
least for the latest gcc in the mid-term future
--
"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
Adrian Bunk wrote:
> On Tue, Nov 08, 2005 at 11:08:30AM -0800, Matthew Dobson wrote:
>
>>Pekka J Enberg wrote:
>>
>>>On Mon, 7 Nov 2005, Matthew Dobson wrote:
>>>
>>>
>>>>I found three functions in slab.c that have only 1 caller (kmem_getpages,
>>>>alloc_slabmgmt, and set_slab_attr), so let's inline them.
>>>
>>>
>>>Why? They aren't on the hot path and I don't see how this is an
>>>improvement...
>>>
>>> Pekka
>>
>>Well, no, they aren't on the hot path. I just figured since they are only
>>ever called from one other function, why not inline them? If the sentiment
>>is that it's a BAD idea, I'll drop it.
>
>
> And if there will one day be a second caller, noone will remember to
> remove the inline...
So are you suggesting that we don't mark these functions 'inline', or are
you just pointing out that we'll need to drop the 'inline' if there is ever
another caller?
-Matt
On Thu, Nov 10, 2005 at 09:04:20AM -0800, Matthew Dobson wrote:
> Adrian Bunk wrote:
> >
> >>Well, no, they aren't on the hot path. I just figured since they are only
> >>ever called from one other function, why not inline them? If the sentiment
> >>is that it's a BAD idea, I'll drop it.
> >
> >
> > And if there will one day be a second caller, noone will remember to
> > remove the inline...
>
> So are you suggesting that we don't mark these functions 'inline', or are
> you just pointing out that we'll need to drop the 'inline' if there is ever
> another caller?
I'd suggest to not mark them 'inline'.
> -Matt
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
Am Donnerstag, 10. November 2005 18:38 schrieb Adrian Bunk:
> > So are you suggesting that we don't mark these functions 'inline', or are
> > you just pointing out that we'll need to drop the 'inline' if there is ever
> > another caller?
>
> I'd suggest to not mark them 'inline'.
It seems you have found one more use for sparse. How about a tag
like __single_inline that will cause a warning if a function having it
is called from more than one place?
Regards
Oliver
On Thu, Nov 10, 2005 at 07:04:22PM +0100, Oliver Neukum wrote:
> Am Donnerstag, 10. November 2005 18:38 schrieb Adrian Bunk:
> > > So are you suggesting that we don't mark these functions 'inline', or are
> > > you just pointing out that we'll need to drop the 'inline' if there is ever
> > > another caller?
> >
> > I'd suggest to not mark them 'inline'.
>
> It seems you have found one more use for sparse. How about a tag
> like __single_inline that will cause a warning if a function having it
> is called from more than one place?
Why should such a function be manually marked "inline" at all?
If a static function is called exactly once it is the job of the
compiler to inline the function.
> Regards
> Oliver
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
Am Donnerstag, 10. November 2005 19:20 schrieb Adrian Bunk:
> On Thu, Nov 10, 2005 at 07:04:22PM +0100, Oliver Neukum wrote:
> > Am Donnerstag, 10. November 2005 18:38 schrieb Adrian Bunk:
> > > > So are you suggesting that we don't mark these functions 'inline', or are
> > > > you just pointing out that we'll need to drop the 'inline' if there is ever
> > > > another caller?
> > >
> > > I'd suggest to not mark them 'inline'.
> >
> > It seems you have found one more use for sparse. How about a tag
> > like __single_inline that will cause a warning if a function having it
> > is called from more than one place?
>
> Why should such a function be manually marked "inline" at all?
>
> If a static function is called exactly once it is the job of the
> compiler to inline the function.
It should indeed. This documentation says it does:
http://gcc.gnu.org/onlinedocs/gcc/Optimize-Options.html
That makes me wonder what is the problem.
Puzzeled
Oliver
On Thu, Nov 10, 2005 at 08:22:52PM +0100, Oliver Neukum wrote:
> Am Donnerstag, 10. November 2005 19:20 schrieb Adrian Bunk:
> > On Thu, Nov 10, 2005 at 07:04:22PM +0100, Oliver Neukum wrote:
> > > Am Donnerstag, 10. November 2005 18:38 schrieb Adrian Bunk:
> > > > > So are you suggesting that we don't mark these functions 'inline', or are
> > > > > you just pointing out that we'll need to drop the 'inline' if there is ever
> > > > > another caller?
> > > >
> > > > I'd suggest to not mark them 'inline'.
> > >
> > > It seems you have found one more use for sparse. How about a tag
> > > like __single_inline that will cause a warning if a function having it
> > > is called from more than one place?
> >
> > Why should such a function be manually marked "inline" at all?
> >
> > If a static function is called exactly once it is the job of the
> > compiler to inline the function.
>
> It should indeed. This documentation says it does:
> http://gcc.gnu.org/onlinedocs/gcc/Optimize-Options.html
> That makes me wonder what is the problem.
On i386, we have the problem that we are using -fno-unit-at-a-time to
avoid stack usage problems.
But the proper solution will be to remove -fno-unit-at-a-time from the
CFLAGS for gcc >= 4.1 or >= 4.2 and check whether this will cause any
new stack usage problems.
> Puzzeled
> Oliver
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