| Message ID | 1395979624-8544-4-git-send-email-wangyufen@huawei.com |
|---|---|
| State | Accepted, archived |
| Delegated to: | David Miller |
| Headers | show |
On Fri, 2014-03-28 at 12:07 +0800, Wangyufen wrote: [] > diff --git a/net/ipv6/ip6_fib.c b/net/ipv6/ip6_fib.c [] > @@ -143,7 +143,7 @@ static __inline__ __be32 addr_bit_set(const void *token, int fn_bit) > addr[fn_bit >> 5]; > } Perhaps all the __inline__ uses could be changed to inline too. > -static __inline__ struct fib6_node * node_alloc(void) > +static __inline__ struct fib6_node *node_alloc(void) > { > struct fib6_node *fn; > > @@ -152,7 +152,7 @@ static __inline__ struct fib6_node * node_alloc(void) > return fn; > } > > -static __inline__ void node_free(struct fib6_node * fn) > +static __inline__ void node_free(struct fib6_node *fn) > { > kmem_cache_free(fib6_node_kmem, fn); > } -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
From: Joe Perches <joe@perches.com> Date: Thu, 27 Mar 2014 21:22:01 -0700 > On Fri, 2014-03-28 at 12:07 +0800, Wangyufen wrote: > [] >> diff --git a/net/ipv6/ip6_fib.c b/net/ipv6/ip6_fib.c > [] >> @@ -143,7 +143,7 @@ static __inline__ __be32 addr_bit_set(const void *token, int fn_bit) >> addr[fn_bit >> 5]; >> } > > Perhaps all the __inline__ uses could be changed to inline too. Or rather, deleted completely, this is a *.c file after all. -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Fri, 2014-03-28 at 01:40 -0400, David Miller wrote: > From: Joe Perches <joe@perches.com> > Date: Thu, 27 Mar 2014 21:22:01 -0700 > > > On Fri, 2014-03-28 at 12:07 +0800, Wangyufen wrote: > > [] > >> diff --git a/net/ipv6/ip6_fib.c b/net/ipv6/ip6_fib.c > > [] > >> @@ -143,7 +143,7 @@ static __inline__ __be32 addr_bit_set(const void *token, int fn_bit) > >> addr[fn_bit >> 5]; > >> } > > > > Perhaps all the __inline__ uses could be changed to inline too. > > Or rather, deleted completely, this is a *.c file after all. Maybe right. There are a lot though just in net/ $ git ls-files net | grep "\.c$" | \ xargs grep -Pw "_{0,2}inline_{0,2}" | wc -l 1512 -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 2014/3/28 13:47, Joe Perches wrote: > On Fri, 2014-03-28 at 01:40 -0400, David Miller wrote: >> From: Joe Perches <joe@perches.com> >> Date: Thu, 27 Mar 2014 21:22:01 -0700 >> >>> On Fri, 2014-03-28 at 12:07 +0800, Wangyufen wrote: >>> [] >>>> diff --git a/net/ipv6/ip6_fib.c b/net/ipv6/ip6_fib.c >>> [] >>>> @@ -143,7 +143,7 @@ static __inline__ __be32 addr_bit_set(const void *token, int fn_bit) >>>> addr[fn_bit >> 5]; >>>> } >>> >>> Perhaps all the __inline__ uses could be changed to inline too. >> >> Or rather, deleted completely, this is a *.c file after all. > > Maybe right. There are a lot though just in net/ > > $ git ls-files net | grep "\.c$" | \ > xargs grep -Pw "_{0,2}inline_{0,2}" | wc -l > 1512 > > > > > Or change to inline and move the whole function to ip6_fib.h? -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Fri, Mar 28, 2014 at 01:40:12AM -0400, David Miller wrote: > From: Joe Perches <joe@perches.com> > Date: Thu, 27 Mar 2014 21:22:01 -0700 > > > On Fri, 2014-03-28 at 12:07 +0800, Wangyufen wrote: > > [] > >> diff --git a/net/ipv6/ip6_fib.c b/net/ipv6/ip6_fib.c > > [] > >> @@ -143,7 +143,7 @@ static __inline__ __be32 addr_bit_set(const void *token, int fn_bit) > >> addr[fn_bit >> 5]; > >> } > > > > Perhaps all the __inline__ uses could be changed to inline too. > > Or rather, deleted completely, this is a *.c file after all. Smart-arsing: Removing inline keyword makes the function visible to tracing if it didn't get inlined. I think this is a nice side-effect because debug kernels are often compiled with less aggressive inlining options (readable asm kconfig option). -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
From: Hannes Frederic Sowa > > > Perhaps all the __inline__ uses could be changed to inline too. > > > > Or rather, deleted completely, this is a *.c file after all. Personally I wouldn't do a blanket removal of 'inline' from .c files. But I will agree that some large functions have been inappropriately marked 'inline'. The complier doesn't always make the right guess. > Smart-arsing: > > Removing inline keyword makes the function visible to tracing if > it didn't get inlined. I think this is a nice side-effect because debug > kernels are often compiled with less aggressive inlining options > (readable asm kconfig option). I want to debug the same binary that will run in production. Dodgy code can be affected by all sorts of compiler options. The best one was a problem with the shell deleting the last character of a $(...) substitution. The code ran off the beginning of an on-stack buffer when trimming trailing '\n' and 'found' a '\n' byte lurking in the unwritten stack (this was well down the stack, and had been written much, much earlier). The error was only ever likely on big-endian systems (the msb of a word is less likely to be '\n'. David
Hello. On 28-03-2014 8:07, Wangyufen wrote: > From: Wang Yufen <wangyufen@huawei.com> > ERROR: "(foo*)" should be "(foo *)" > ERROR: "foo * bar" should be "foo *bar" > > Suggested-by: Sergei Shtylyov <sergei.shtylyov@cogentembedded.com> I didn't really suggest to you to make this patch, I just commented on it. You can instead add: Acked-by: Sergei Shtylyov <sergei.shtylyov@cogentembedded.com> > Signed-off-by: Wang Yufen <wangyufen@huawei.com> > --- > net/ipv6/ip6_fib.c | 30 +++++++++++++++--------------- > 1 file changed, 15 insertions(+), 15 deletions(-) WBR, Sergei -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
From: wangyufen <wangyufen@huawei.com> Date: Fri, 28 Mar 2014 17:33:54 +0800 > On 2014/3/28 13:47, Joe Perches wrote: >> On Fri, 2014-03-28 at 01:40 -0400, David Miller wrote: >>> From: Joe Perches <joe@perches.com> >>> Date: Thu, 27 Mar 2014 21:22:01 -0700 >>> >>>> On Fri, 2014-03-28 at 12:07 +0800, Wangyufen wrote: >>>> [] >>>>> diff --git a/net/ipv6/ip6_fib.c b/net/ipv6/ip6_fib.c >>>> [] >>>>> @@ -143,7 +143,7 @@ static __inline__ __be32 addr_bit_set(const void *token, int fn_bit) >>>>> addr[fn_bit >> 5]; >>>>> } >>>> >>>> Perhaps all the __inline__ uses could be changed to inline too. >>> >>> Or rather, deleted completely, this is a *.c file after all. >> >> Maybe right. There are a lot though just in net/ >> >> $ git ls-files net | grep "\.c$" | \ >> xargs grep -Pw "_{0,2}inline_{0,2}" | wc -l >> 1512 >> >> >> >> >> > > Or change to inline and move the whole function to ip6_fib.h? No, that is not desirable. The whole point is to not export the interface outside of the *.c file, and to let the compiler make inlining decisions since it can see all of the call sites. -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Fri, Mar 28, 2014 at 10:54:13AM +0000, David Laight wrote: > From: Hannes Frederic Sowa > > > > Perhaps all the __inline__ uses could be changed to inline too. > > > > > > Or rather, deleted completely, this is a *.c file after all. > > Personally I wouldn't do a blanket removal of 'inline' from .c files. > But I will agree that some large functions have been inappropriately > marked 'inline'. Does inline keyword actually make a difference in case the function is static? I guess not (except for function profiling if a prologue should get included, thus the change in visibility for tracing). > The complier doesn't always make the right guess. > > > Smart-arsing: > > > > Removing inline keyword makes the function visible to tracing if > > it didn't get inlined. I think this is a nice side-effect because debug > > kernels are often compiled with less aggressive inlining options > > (readable asm kconfig option). > > I want to debug the same binary that will run in production. > Dodgy code can be affected by all sorts of compiler options. I agree. But status quo is that your binaries look very different if you have DEBUG_SECTION_MISMATCH or READABLE_ASM defined. The change in tracing visibility is already the case if you (like me) compile your kernel with DEBUG_SECTION_MISMATCH wich enables no-inline-functions-called-once, because this option does also not respect inline keyword. > The best one was a problem with the shell deleting the last > character of a $(...) substitution. > The code ran off the beginning of an on-stack buffer when trimming > trailing '\n' and 'found' a '\n' byte lurking in the unwritten > stack (this was well down the stack, and had been written much, > much earlier). > The error was only ever likely on big-endian systems (the msb of > a word is less likely to be '\n'. Heh. :) Greetings, Hannes -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/net/ipv6/ip6_fib.c b/net/ipv6/ip6_fib.c index 8cb6949..aafc5bd 100644 --- a/net/ipv6/ip6_fib.c +++ b/net/ipv6/ip6_fib.c @@ -44,7 +44,7 @@ #define RT6_TRACE(x...) do { ; } while (0) #endif -static struct kmem_cache * fib6_node_kmem __read_mostly; +static struct kmem_cache *fib6_node_kmem __read_mostly; enum fib_walk_state_t { #ifdef CONFIG_IPV6_SUBTREES @@ -143,7 +143,7 @@ static __inline__ __be32 addr_bit_set(const void *token, int fn_bit) addr[fn_bit >> 5]; } -static __inline__ struct fib6_node * node_alloc(void) +static __inline__ struct fib6_node *node_alloc(void) { struct fib6_node *fn; @@ -152,7 +152,7 @@ static __inline__ struct fib6_node * node_alloc(void) return fn; } -static __inline__ void node_free(struct fib6_node * fn) +static __inline__ void node_free(struct fib6_node *fn) { kmem_cache_free(fib6_node_kmem, fn); } @@ -288,7 +288,7 @@ static int fib6_dump_node(struct fib6_walker_t *w) static void fib6_dump_end(struct netlink_callback *cb) { - struct fib6_walker_t *w = (void*)cb->args[2]; + struct fib6_walker_t *w = (void *)cb->args[2]; if (w) { if (cb->args[4]) { @@ -298,7 +298,7 @@ static void fib6_dump_end(struct netlink_callback *cb) cb->args[2] = 0; kfree(w); } - cb->done = (void*)cb->args[3]; + cb->done = (void *)cb->args[3]; cb->args[1] = 3; } @@ -951,8 +951,8 @@ struct lookup_args { const struct in6_addr *addr; /* search key */ }; -static struct fib6_node * fib6_lookup_1(struct fib6_node *root, - struct lookup_args *args) +static struct fib6_node *fib6_lookup_1(struct fib6_node *root, + struct lookup_args *args) { struct fib6_node *fn; __be32 dir; @@ -1014,8 +1014,8 @@ backtrack: return NULL; } -struct fib6_node * fib6_lookup(struct fib6_node *root, const struct in6_addr *daddr, - const struct in6_addr *saddr) +struct fib6_node *fib6_lookup(struct fib6_node *root, const struct in6_addr *daddr, + const struct in6_addr *saddr) { struct fib6_node *fn; struct lookup_args args[] = { @@ -1047,9 +1047,9 @@ struct fib6_node * fib6_lookup(struct fib6_node *root, const struct in6_addr *da */ -static struct fib6_node * fib6_locate_1(struct fib6_node *root, - const struct in6_addr *addr, - int plen, int offset) +static struct fib6_node *fib6_locate_1(struct fib6_node *root, + const struct in6_addr *addr, + int plen, int offset) { struct fib6_node *fn; @@ -1077,9 +1077,9 @@ static struct fib6_node * fib6_locate_1(struct fib6_node *root, return NULL; } -struct fib6_node * fib6_locate(struct fib6_node *root, - const struct in6_addr *daddr, int dst_len, - const struct in6_addr *saddr, int src_len) +struct fib6_node *fib6_locate(struct fib6_node *root, + const struct in6_addr *daddr, int dst_len, + const struct in6_addr *saddr, int src_len) { struct fib6_node *fn;