On Wed, 12 Jan 2011 07:57:36 -0600 Rob Landley <rlandley@xxxxxxxxxxxxx> wrote: > On 01/11/2011 03:30 PM, Jeff Layton wrote: > > I've got a patch queued that rearranges some fields in TCP_Server_Info > > according to pahole's recommendations. You may want to base this patch > > on that. > > Queued where? > It was sent to the list a week or so ago: http://article.gmane.org/gmane.linux.kernel.cifs/2205 ...not sure if/when Steve is going to merge it though. > > It might also be good to run pahole on this after your patch to see if > > it might be better placed... > > I put it right after the network address fields affected by it. I can > stick it between: > > char *hostname; /* hostname portion of UNC string */ > struct socket *ssocket; > > So that two consecutive pointers become three consecutive pointers. But > again, I haven't seen what you did to the structure yet... > Your placement may be fine, but you might consider using pahole afterward to see whether it might be better placed elsewhere. > >> spin_lock(&cifs_tcp_ses_lock); > >> list_for_each_entry(server, &cifs_tcp_ses_list, tcp_ses_list) { > >> + if (HAVE_NET_NS && > >> + cifs_net_ns(server) != current->nsproxy->net_ns) > >> + continue; > >> + > > > > This HAVE_NET_NS thing is pretty ugly. > > It's a compile time constant optimized away by the compiler, and cleanly > taking out the entire dependent clause with it when it's false (because > && only evaluates the right side when the left side is true, meaning the > if can never trigger, thus dead code elimination). > > How is that ugly? It does exactly what it says. I'm not sure what the > criteria are here. (Was it better when it looked like a function rather > than a constant? I'd use the CONFIG_NET_NS symbol directly if it was 0 > or 1 rather than undefined or 1, but it isn't. It's designed for use > with #ifdefs only, not from C code.) > It's ugly because it makes the code difficult to read, and readability trumps these sorts of micro-optimizations. If this were a high traffic codepath, I might feel differently but this is generally only touched once per mount. > > This is not a high-performance > > codepath. My vote would be to get rid of this and just have the useless > > test when CONFIG_NET_NS isn't set. > > > > Alternately, you could turn the comparison into a static inline or > > macro, and simply have that compile down to nothing when CONFIG_NET_NS > > isn't set. > > I tried returning a constant from a static inline, it doesn't propogate > the constant far enough to do compile time dead code elimination based > on the return value under gcc 4.4.3. > > As for a macro, the constant already is a macro. So adding more #ifdefs > to the headers to make the code in the function look like it's doing > something other than it actually is somehow improves matters? (I'm not > understanding the aesthetic criteria here at all.) > > I think I'll just ignore the bloat. Make allnoconfig is already 800k > compressed, what's a few more bytes... > > >> @@ -1754,6 +1762,8 @@ cifs_get_tcp_session(struct smb_vol *volume_info) > >> out_err_crypto_release: > >> cifs_crypto_shash_release(tcp_ses); > >> > >> + put_net(cifs_net_ns(tcp_ses)); > >> + > > ^^^^ > > This looks like it will oops if you end up doing the "goto > > out_err_crypto_release" after the extract_hostname call. > > Hmmm... Yup, failure case when CONFIG_NET_NS enabled will dereference > the null pointer. I need to move the initialization up a few lines. That seems like the simplest fix. -- Jeff Layton <jlayton@xxxxxxxxxx> -- To unsubscribe from this list: send the line "unsubscribe linux-cifs" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
