(Sorry for the delayed reply)
Paul Menage wrote:
> On Fri, Oct 22, 2010 at 1:09 AM, Li Zefan <[email protected]> wrote:
>> On x86_32, sizeof(struct cgroup_subsys) shrinks from 276 bytes
>> to 264.
>>
>> Signed-off-by: Li Zefan <[email protected]>
>
> Acked-by: Paul Menage <[email protected]>
>
> Maybe use "bool" here?
>
Do you mean:
bool active;
bool disabled;
...
?
With alignment 5-8 bool values == 8 bytes in 64-bit machine, compared to
4 bytes with the approach this patch takes.
> Paul
>
>> ---
>> include/linux/cgroup.h | 10 ++++++----
>> 1 files changed, 6 insertions(+), 4 deletions(-)
>>
>> diff --git a/include/linux/cgroup.h b/include/linux/cgroup.h
>> index ed4ba11..e23ded6 100644
>> --- a/include/linux/cgroup.h
>> +++ b/include/linux/cgroup.h
>> @@ -481,14 +481,16 @@ struct cgroup_subsys {
>> void (*bind)(struct cgroup_subsys *ss, struct cgroup *root);
>>
>> int subsys_id;
>> - int active;
>> - int disabled;
>> - int early_init;
>> +
>> + unsigned int active:1;
>> + unsigned int disabled:1;
>> + unsigned int early_init:1;
>> /*
>> * True if this subsys uses ID. ID is not available before cgroup_init()
>> * (not available in early_init time.)
>> */
>> - bool use_id;
>> + unsigned int use_id:1;
>> +
>> #define MAX_CGROUP_TYPE_NAMELEN 32
>> const char *name;
>>
>> --
>> 1.7.0.1
>>
>>
On Sun, Nov 7, 2010 at 9:23 PM, Li Zefan <[email protected]> wrote:
>
> bool active;
> bool disabled;
> ...
>
> ?
>
> With alignment 5-8 bool values == 8 bytes in 64-bit machine, compared to
> 4 bytes with the approach this patch takes.
I meant specifying it as:
bool active:1;
bool disabled:1;
i.e. keeping the bit-sized flags but also keeping the bool semantics.
Having said that, I'm not really sure why saving 12 bytes per
subsystem is worth a patch.
Paul
Paul Menage wrote:
> On Sun, Nov 7, 2010 at 9:23 PM, Li Zefan <[email protected]> wrote:
>> bool active;
>> bool disabled;
>> ...
>>
>> ?
>>
>> With alignment 5-8 bool values == 8 bytes in 64-bit machine, compared to
>> 4 bytes with the approach this patch takes.
>
> I meant specifying it as:
>
> bool active:1;
> bool disabled:1;
>
It won't compile, but unsigned char active:1 will do. ;)
> i.e. keeping the bit-sized flags but also keeping the bool semantics.
> Having said that, I'm not really sure why saving 12 bytes per
> subsystem is worth a patch.
Every thing that reduces code size (without sacrifice readability
and maintain maintainability) should be worth.
This is one of the reasons we accept patches that replacing
kmalloc+memset with kzalloc, which just saves 8 bytes in my box.
On Tue, Nov 9, 2010 at 4:52 PM, Li Zefan <[email protected]> wrote:
>>
>> bool active:1;
>> bool disabled:1;
>>
>
> It won't compile, but unsigned char active:1 will do. ;)
Are you sure? I don't have a buildable kernel tree at the moment, but
the following fragment compiled fine for me (with gcc 4.4.3):
struct foo {
_Bool b1:1;
_Bool b2:1;
};
and was sized at one byte. And "bool" is just a typedef of _Bool in
the kernel headers.
>
> Every thing that reduces code size (without sacrifice readability
> and maintain maintainability) should be worth.
Agreed, within reason. But this patch doesn't reduce code size - it
makes the code fractionally more complicated and reduces the *binary*
size by a few bytes.
>
> This is one of the reasons we accept patches that replacing
> kmalloc+memset with kzalloc, which just saves 8 bytes in my box.
>
Replacing two function calls with one function call is a code
simplification and hence (generally) a good thing - the minuscule
reduction in binary size reduction that comes with it is just noise.
Paul
Paul Menage wrote:
> On Tue, Nov 9, 2010 at 4:52 PM, Li Zefan <[email protected]> wrote:
>>> bool active:1;
>>> bool disabled:1;
>>>
>> It won't compile, but unsigned char active:1 will do. ;)
>
> Are you sure? I don't have a buildable kernel tree at the moment, but
> the following fragment compiled fine for me (with gcc 4.4.3):
>
> struct foo {
> _Bool b1:1;
> _Bool b2:1;
> };
>
> and was sized at one byte. And "bool" is just a typedef of _Bool in
> the kernel headers.
>
Oops, I just used bool outside kernel tree..
>> Every thing that reduces code size (without sacrifice readability
>> and maintain maintainability) should be worth.
>
> Agreed, within reason. But this patch doesn't reduce code size - it
I meant binary size.
> makes the code fractionally more complicated and reduces the *binary*
> size by a few bytes.
>
It's a commonly used skill in kernel code, so I can't say it makes
code more complicated.
That said, I'll happily drop this patch. It just came to me when I
started to add new bool values to the structure. Or if you prefer
bool xxx:1 or just bool xxx, I can do that.
>> This is one of the reasons we accept patches that replacing
>> kmalloc+memset with kzalloc, which just saves 8 bytes in my box.
>>
>
> Replacing two function calls with one function call is a code
> simplification and hence (generally) a good thing - the minuscule
> reduction in binary size reduction that comes with it is just noise.
>
On Tue, Nov 9, 2010 at 6:06 PM, Li Zefan <[email protected]> wrote:
> That said, I'll happily drop this patch. It just came to me when I
> started to add new bool values to the structure. Or if you prefer
> bool xxx:1 or just bool xxx, I can do that.
bool xxx:1 is fine with me - I think it's worth keeping the semantics
of these being boolean flags as obvious as possible.
I just wouldn't invest much effort in similar patches in the future
(given that there are only likely to be, what, <20 instances of
cgroup_subsys in the kernel?). Shrinking a structure that had
potentially very many instances, such as css_set or cg_cgroup_link,
would be different of course.
Paul