Re: [PATCH] Small kfree cleanup, save a local variable.

Re: [PATCH] Small kfree cleanup, save a local variable.

[Michael Buesch]

[-- Attachment #1: Type: text/plain, Size: 1003 bytes --]

Quoting Jesper Juhl <juhl-lkml@dif.dk>:
> Here's a patch with a small improvement to kernel/auditsc.c .
> There's no need for the local variable  struct audit_entry *e  , 
> we can just call kfree directly on container_of() .

Did you look at the assembly output? Does it change?
I think the compiler optimizes this variable away, anyway.
So, if there's no improvement, I would personally prefer the
original form as it's more readable.

>  kernel/auditsc.c |    5 ++---
>  1 files changed, 2 insertions(+), 3 deletions(-)
> 
> --- linux-2.6.12-orig/kernel/auditsc.c	2005-06-17 21:48:29.000000000 +0200
> +++ linux-2.6.12/kernel/auditsc.c	2005-06-19 21:21:37.000000000 +0200
> @@ -202,8 +202,7 @@ static inline int audit_add_rule(struct 
>  
>  static void audit_free_rule(struct rcu_head *head)
>  {
> -	struct audit_entry *e = container_of(head, struct audit_entry, rcu);
> -	kfree(e);
> +	kfree(container_of(head, struct audit_entry, rcu));
>  }

-- 
Greetings, Michael



[-- Attachment #2: Type: application/pgp-signature, Size: 189 bytes --]

[PATCH] Small kfree cleanup, save a local variable.

[Jesper Juhl]

Here's a patch with a small improvement to kernel/auditsc.c .
There's no need for the local variable  struct audit_entry *e  , 
we can just call kfree directly on container_of() .
Patch also removes an extra space a little further down in the file.

Signed-off-by: Jesper Juhl <juhl-lkml@dif.dk>
---

 kernel/auditsc.c |    5 ++---
 1 files changed, 2 insertions(+), 3 deletions(-)

--- linux-2.6.12-orig/kernel/auditsc.c	2005-06-17 21:48:29.000000000 +0200
+++ linux-2.6.12/kernel/auditsc.c	2005-06-19 21:21:37.000000000 +0200
@@ -202,8 +202,7 @@ static inline int audit_add_rule(struct 
 
 static void audit_free_rule(struct rcu_head *head)
 {
-	struct audit_entry *e = container_of(head, struct audit_entry, rcu);
-	kfree(e);
+	kfree(container_of(head, struct audit_entry, rcu));
 }
 
 /* Note that audit_add_rule and audit_del_rule are called via
@@ -612,7 +611,7 @@ static inline void audit_free_context(st
 		audit_free_names(context);
 		audit_free_aux(context);
 		kfree(context);
-		context  = previous;
+		context = previous;
 	} while (context);
 	if (count >= 10)
 		printk(KERN_ERR "audit: freed %d contexts\n", count);

Re: [PATCH] Small kfree cleanup, save a local variable.

[Jesper Juhl]

On 6/19/05, Michael Buesch <mbuesch@freenet.de> wrote:
> Quoting Jesper Juhl <juhl-lkml@dif.dk>:
> > Here's a patch with a small improvement to kernel/auditsc.c .
> > There's no need for the local variable  struct audit_entry *e  ,
> > we can just call kfree directly on container_of() .
> 
> Did you look at the assembly output? Does it change?
> I think the compiler optimizes this variable away, anyway.
> So, if there's no improvement, I would personally prefer the
> original form as it's more readable.
> 
gcc does optimize it away. 
Personally I find both forms equally readable, and with the change the
source matches what the code actually ends up as, but I don't care
much one way or the other.

-- 
Jesper Juhl <jesper.juhl@gmail.com>
Don't top-post  http://www.catb.org/~esr/jargon/html/T/top-post.html
Plain text mails only, please      http://www.expita.com/nomime.html

Re: [PATCH] Small kfree cleanup, save a local variable.

[Chris Wright]

* Jesper Juhl (juhl-lkml@dif.dk) wrote:
> Here's a patch with a small improvement to kernel/auditsc.c .
> There's no need for the local variable  struct audit_entry *e  , 
> we can just call kfree directly on container_of() .
> Patch also removes an extra space a little further down in the file.

Please Cc: linux-audit@redhat.com on audit patches.  I tend to agree
with Michael, it's optimized away, and readable as is.

thanks,
-chris

> Signed-off-by: Jesper Juhl <juhl-lkml@dif.dk>
> ---
> 
>  kernel/auditsc.c |    5 ++---
>  1 files changed, 2 insertions(+), 3 deletions(-)
> 
> --- linux-2.6.12-orig/kernel/auditsc.c	2005-06-17 21:48:29.000000000 +0200
> +++ linux-2.6.12/kernel/auditsc.c	2005-06-19 21:21:37.000000000 +0200
> @@ -202,8 +202,7 @@ static inline int audit_add_rule(struct 
>  
>  static void audit_free_rule(struct rcu_head *head)
>  {
> -	struct audit_entry *e = container_of(head, struct audit_entry, rcu);
> -	kfree(e);
> +	kfree(container_of(head, struct audit_entry, rcu));
>  }
>  
>  /* Note that audit_add_rule and audit_del_rule are called via
> @@ -612,7 +611,7 @@ static inline void audit_free_context(st
>  		audit_free_names(context);
>  		audit_free_aux(context);
>  		kfree(context);
> -		context  = previous;
> +		context = previous;
>  	} while (context);
>  	if (count >= 10)
>  		printk(KERN_ERR "audit: freed %d contexts\n", count);

Re: [PATCH] Small kfree cleanup, save a local variable.

[Jesper Juhl]

On 6/19/05, Chris Wright <chrisw@osdl.org> wrote:
> * Jesper Juhl (juhl-lkml@dif.dk) wrote:
> > Here's a patch with a small improvement to kernel/auditsc.c .
> > There's no need for the local variable  struct audit_entry *e  ,
> > we can just call kfree directly on container_of() .
> > Patch also removes an extra space a little further down in the file.
> 
> Please Cc: linux-audit@redhat.com on audit patches.

I didn't find that address in MAINTAINERS nor in the source file. I
had no idea it existed. Perhaps it ought to be listed in MAINTAINERS
somewhere...


>  I tend to agree
> with Michael, it's optimized away, and readable as is.
> 
Fair enough, we'll just leave it as is :)


-- 
Jesper Juhl <jesper.juhl@gmail.com>
Don't top-post  http://www.catb.org/~esr/jargon/html/T/top-post.html
Plain text mails only, please      http://www.expita.com/nomime.html

Re: [PATCH] Small kfree cleanup, save a local variable.

[Chris Wright]

* Jesper Juhl (jesper.juhl@gmail.com) wrote:
> On 6/19/05, Chris Wright <chrisw@osdl.org> wrote:
> > * Jesper Juhl (juhl-lkml@dif.dk) wrote:
> > > Here's a patch with a small improvement to kernel/auditsc.c .
> > > There's no need for the local variable  struct audit_entry *e  ,
> > > we can just call kfree directly on container_of() .
> > > Patch also removes an extra space a little further down in the file.
> > 
> > Please Cc: linux-audit@redhat.com on audit patches.
> 
> I didn't find that address in MAINTAINERS nor in the source file. I
> had no idea it existed. Perhaps it ought to be listed in MAINTAINERS
> somewhere...

Ahh, good point, that needs to be fixed.

thanks,
-chris