Second take at cleaning up mm/slab.c. Patch series has picked up 2 new
patches and dropped one old one. The 2 new patches create new helper
functions, and I've dropped the patch to inline 3 functions after it was
deemed unecessary.
Appologies for the delay in getting version 2 out. /me is easily
distracted by shiny things.
-Matt
mm/slab.c is MESSY. Tabs where there should be spaces, spaces where there
should be tabs. Long lines, bad formatting, spelling errors, etc.
Everything you'd expect from such an oft-patched file. Let's start with
CodingStyle fixes.
Oh, and since I forgot to mention it in the 0/9 email, all these patches
are against 2.6.14.
-Matt
Rather than various and sundry names for a node's number, let's use 'nid'.
-Matt
alloc_percpu takes an 'align' argument that is completely ignored. Remove it.
-Matt
Most of the s_start() function body is just printing out a header for
/proc/slabinfo. Move it to its own function.
-Matt
There's a loop in kmem_cache_create() where we try to determine the best
page order for a slab. It's a big, ugly loop, so let's move it to its own
function: calculate_slab_order().
-Matt
kmem_cache_create() is a pretty big function, so we'll devote one whole
patch just to cleaning it up.
-Matt
cache_reap() could also use a little love. Here's a patch to clean it up.
-Matt
Shuffle a little code around in slabinfo_write(), both for efficiency and
readability.
-Matt
Last, but not least, fix a loop in set_slab_attr() to match the rest of the
functionally similar loops in mm/slab.c.
-Matt
On Thu, 10 Nov 2005, Matthew Dobson wrote:
> Second take at cleaning up mm/slab.c. Patch series has picked up 2 new
> patches and dropped one old one. The 2 new patches create new helper
> functions, and I've dropped the patch to inline 3 functions after it was
> deemed unecessary.
Looks good to me. Thanks Matthew!
Pekka
On Friday 11 November 2005 01:04, Matthew Dobson wrote:
> - if (size < 4096 || fls(size - 1) == fls(size - 1 + 3 * BYTES_PER_WORD))
> + if (size < RED_ZONE_LIMIT ||
> + fls(size - 1) == fls(size - 1 + 3 * BYTES_PER_WORD))
> flags |= SLAB_RED_ZONE|SLAB_STORE_USER;
I would suggest sth. like
if (size < RED_TONE_LIMIT
|| fls(size - 1) = fls(size - 1 + 3 * BYTES_PER_WORD))
flags |= SLAB_RED_ZONE | SLAB_STORE_USER
Reason: A binary operator in front is a huge hint
that this is a continued line.
Just compare when you go to a store next time.
1
+ 2
- 3
* 4
is much more readable then
1 +
2 -
3 *
4
right?
Regards
Ingo Oeser
Ingo Oeser wrote:
> On Friday 11 November 2005 01:04, Matthew Dobson wrote:
>
>>- if (size < 4096 || fls(size - 1) == fls(size - 1 + 3 * BYTES_PER_WORD))
>>+ if (size < RED_ZONE_LIMIT ||
>>+ fls(size - 1) == fls(size - 1 + 3 * BYTES_PER_WORD))
>> flags |= SLAB_RED_ZONE|SLAB_STORE_USER;
>
>
> I would suggest sth. like
>
> if (size < RED_TONE_LIMIT
> || fls(size - 1) = fls(size - 1 + 3 * BYTES_PER_WORD))
> flags |= SLAB_RED_ZONE | SLAB_STORE_USER
>
>
> Reason: A binary operator in front is a huge hint
> that this is a continued line.
>
> Just compare when you go to a store next time.
>
> 1
> + 2
> - 3
> * 4
>
> is much more readable then
>
> 1 +
> 2 -
> 3 *
> 4
>
> right?
>
>
> Regards
>
> Ingo Oeser
You make a very good point. It's not the way that I'm used to writing
multi-line ifs, but it is a bit more readable. However, since the rest of
the multi-line ifs in the file have the binary operator at the end of the
first line, I'm inclined to leave it the way it is for consistency. If
anyone really feels strongly, I could come up with a patch for the whole
file...?
Thanks for the feedback!
-Matt
On Thu 10-11-05 15:58:53, Matthew Dobson wrote:
> Rather than various and sundry names for a node's number, let's use 'nid'.
I'd say nodeid is better name... and please inline patches.
Pavel
--
64 bytes from 195.113.31.123: icmp_seq=28 ttl=51 time=448769.1 ms