On Sun, 20 Mar 2005, Ralph Corderoy wrote:
>
> > and if there are places where it's important to remember that the
> > pointer might be NULL, then a simple comment would do, wouldn't it?
> >
> > kfree(foo->bar); /* kfree(NULL) is valid */
>
> I'd rather be without the same comment littering the code.
>
Me too, but if it's between having a comment or the long version of the
code with the if() wrapped around it, then I'll personally take the
comment. But the actual code should be comment enough.
> > the short version also have the real bennefits of generating shorter
> > and faster code as well as being shorter "on-screen".
>
> Faster code? I'd have thought avoiding the function call outweighed the
> overhead of checking before calling.
>
I haven't actually measured it, but that would be my guess from looking at
the actual code gcc generates.
security/selinux/ss/conditional.c::cond_policydb_destroy() for instance :
The original version with the if() in place :
void cond_policydb_destroy(struct policydb *p)
{
if (p->bool_val_to_struct)
kfree(p->bool_val_to_struct);
avtab_destroy(&p->te_cond_avtab);
cond_list_destroy(p->cond_list);
}
turns out like this :
[EMAIL PROTECTED]:~/download/kernel/linux-2.6.11-mm4$ objdump -d -S
security/selinux/ss/conditional.o
[...]
void cond_policydb_destroy(struct policydb *p)
{
220: 55 push %ebp
221: 89 e5 mov %esp,%ebp
223: 53 push %ebx
224: 89 c3 mov %eax,%ebx
if (p->bool_val_to_struct)
226: 8b 40 78 mov 0x78(%eax),%eax
229: 85 c0 test %eax,%eax
22b: 75 13 jne 240 <cond_policydb_destroy+0x20>
kfree(p->bool_val_to_struct);
avtab_destroy(&p->te_cond_avtab);
22d: 8d 43 7c lea 0x7c(%ebx),%eax
230: e8 fc ff ff ff call 231 <cond_policydb_destroy+0x11>
cond_list_destroy(p->cond_list);
235: 8b 83 84 00 00 00 mov 0x84(%ebx),%eax
23b: 5b pop %ebx
23c: c9 leave
23d: eb c1 jmp 200 <cond_list_destroy>
23f: 90 nop
240: e8 fc ff ff ff call 241 <cond_policydb_destroy+0x21>
245: 8d 43 7c lea 0x7c(%ebx),%eax
248: e8 fc ff ff ff call 249 <cond_policydb_destroy+0x29>
24d: 8b 83 84 00 00 00 mov 0x84(%ebx),%eax
253: 5b pop %ebx
254: c9 leave
255: eb a9 jmp 200 <cond_list_destroy>
257: 89 f6 mov %esi,%esi
259: 8d bc 27 00 00 00 00 lea 0x0(%edi),%edi
[...]
Whereas the version without the if() turns out like this :
[EMAIL PROTECTED]:~/download/kernel/linux-2.6.11-mm4$ objdump -d -S
security/selinux/ss/conditional.o
[...]
void cond_policydb_destroy(struct policydb *p)
{
220: 55 push %ebp
221: 89 e5 mov %esp,%ebp
223: 53 push %ebx
224: 89 c3 mov %eax,%ebx
kfree(p->bool_val_to_struct);
226: 8b 40 78 mov 0x78(%eax),%eax
229: e8 fc ff ff ff call 22a <cond_policydb_destroy+0xa>
avtab_destroy(&p->te_cond_avtab);
22e: 8d 43 7c lea 0x7c(%ebx),%eax
231: e8 fc ff ff ff call 232 <cond_policydb_destroy+0x12>
cond_list_destroy(p->cond_list);
236: 8b 83 84 00 00 00 mov 0x84(%ebx),%eax
23c: 5b pop %ebx
23d: c9 leave
23e: eb c0 jmp 200 <cond_list_destroy>
[...]
First of all that's significantly shorter, so we'll gain a bit of memory
and I'd guess it would improve cache behaviour as well (but I don't know
enough to say for sure).
I'm also assuming that in the vast majority of cases (not just here,
but all over the kernel) the pointer being tested will end up being !=NULL
so we'll end up doing the function call in any case, so saving the
conditional should be an overall win.
--
Jesper
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
