[Devel] Re: [PATCH] Teach cifs about network namespaces.
Matt Helsley
matthltc at us.ibm.com
Tue Jan 11 14:03:23 PST 2011
On Tue, Jan 11, 2011 at 08:05:38AM -0600, Rob Landley wrote:
> On 01/11/2011 01:12 AM, Matt Helsley wrote:
> > On Mon, Jan 10, 2011 at 10:35:19PM -0600, Rob Landley wrote:
> >> From: Rob Landley <rlandley at parallels.com>
> >>
> >> Teach cifs about network namespaces, so mounting uses adresses and
> >> routing visible from a container rather than from init context.
> >>
> >> For a long drawn out test reproduction sequence, see:
> >>
> >> http://landley.livejournal.com/47024.html
> >> http://landley.livejournal.com/47205.html
> >> http://landley.livejournal.com/47476.html
> >>
> >> Signed-off-by: Rob Landley <rlandley at parallels.com>
> >> ---
> >>
> >> fs/cifs/cifsglob.h | 32 ++++++++++++++++++++++++++++++++
> >> fs/cifs/connect.c | 22 +++++++++++++++++-----
> >> 2 files changed, 49 insertions(+), 5 deletions(-)
> >>
> >> diff --git a/fs/cifs/cifsglob.h b/fs/cifs/cifsglob.h
> >> index 7136c0c..86f31bb 100644
> >> --- a/fs/cifs/cifsglob.h
> >> +++ b/fs/cifs/cifsglob.h
<snip>
> >> +#ifdef CONFIG_NET_NS
> >> +#define cifs_use_net_ns() (1)
> >> +#else
> >> +#define cifs_use_net_ns() (0)
> >> +#endif
> >
> > This looks wrong -- we shouldn't need this at all. The #ifdef bits in
> > your patch already make all the cases below become empty/no-ops when
> > CONFIG_NET_NS=n.
>
> Except that bloat-o-meter said they were still generating code and
> making the kernel bigger. (Things like calling get() and put() on
> init_net to twiddle its reference count.)
I think the only code that would be generated is the test discussed
below. And we're probably talking a few instructions -- an insignificant
amount. There are probably much better places to make a substantial
impact on kernel bloat.
>
> I'm a long-time embedded programmer, I try not to make the code bigger
> than necessary, especially when we have a config symbol to remove stuff.
> I did ponder turning those into HAVE_NET_HS so the if was more
> obviously against a constant.
My recollection is that method of doing things is often frowned upon in
kernel code. There are exceptions of course, but most use the CONFIG_FOO
patterns you've already employed (or that I already suggested).
>
> >> +
> >> +/*
> >> * Session structure. One of these for each uid session with a particular host
> >> */
> >> struct cifsSesInfo {
> >> diff --git a/fs/cifs/connect.c b/fs/cifs/connect.c
> >> index cc1a860..b4faef0 100644
> >> --- a/fs/cifs/connect.c
> >> +++ b/fs/cifs/connect.c
> >> @@ -1545,6 +1545,10 @@ cifs_find_tcp_session(struct sockaddr *addr, struct smb_vol *vol)
> >>
> >> spin_lock(&cifs_tcp_ses_lock);
> >> list_for_each_entry(server, &cifs_tcp_ses_list, tcp_ses_list) {
> >> + if (cifs_use_net_ns()
> >> + && cifs_net_ns(server) == current->nsproxy->net_ns)
> >> + continue;
> >
> > This looks wrong -- you want to invert part of this I think (and drop the
> > unnecessary cifs_use_net_ns()):
>
> You're right, I got the test backwards. Thanks.
>
> The reason for the guards is that compiler couldn't tell that
> current->nsproxy->net_ns always contains the same value, so without a
> test against a constant allowing it to do dead code elimination, it will
> generate code to perform the useless test.
I think we're better off with simpler source code than trying to eliminate
a few instructions.
>
> > if (cifs_net_ns(server) != current->nsproxy->net_ns)
> > continue;
> >
> > This is obvious when you note that the context below shows that we
> > 'continue' to the next entry when the addresses don't match:
> >
> >> +
> >> if (!match_address(server, addr,
> >> (struct sockaddr *)&vol->srcaddr))
> >> continue;
> >> @@ -1572,6 +1576,9 @@ cifs_put_tcp_session(struct TCP_Server_Info *server)
> >> return;
> >> }
> >>
> >> + if (cifs_use_net_ns())
> >> + put_net(cifs_net_ns(server));
> >> +
> >
> > I think this should just be:
> >
> > put_net(cifs_net_ns(server));
>
> Hmmm... that one you're probably right, because
> include/net/net_namespace.h makes put_net() an empty inline, so as long
> as cifs_net_ns() has no side effects the compiler should be able to drop
> the whole thing out. (Whether or not it will, I have to test...)
>
> >> list_del_init(&server->tcp_ses_list);
> >> spin_unlock(&cifs_tcp_ses_lock);
> >>
> >> @@ -1677,6 +1684,9 @@ cifs_get_tcp_session(struct smb_vol *volume_info)
> >> sizeof(tcp_ses->srcaddr));
> >> ++tcp_ses->srv_count;
> >>
> >> + if (cifs_use_net_ns())
> >> + cifs_set_net_ns(tcp_ses, get_net(current->nsproxy->net_ns));
> >> +
> >
> > Just use cifs_set_net_ns() because it already turns into a no-op when
> > CONFIG_NET_NS=n
>
> no-op yes. no-code, I want to make sure. (I tried it with just the
> macros the first time through, and scripts/bloat-o-meter kept saying
> code was being generated. I'll fire up objdump and see if the
> disassembly tells me anything...)
Yup, thanks for making sure.
<snip>
> >> @@ -2145,8 +2158,8 @@ ipv4_connect(struct TCP_Server_Info *server)
> >> struct socket *socket = server->ssocket;
> >>
> >> if (socket == NULL) {
> >> - rc = sock_create_kern(PF_INET, SOCK_STREAM,
> >> - IPPROTO_TCP, &socket);
> >> + rc = __sock_create(cifs_net_ns(server), PF_INET,
> >> + SOCK_STREAM, IPPROTO_TCP, &socket, 1);
> >> if (rc < 0) {
> >> cERROR(1, "Error %d creating socket", rc);
> >> return rc;
> >> @@ -2310,11 +2323,10 @@ ipv6_connect(struct TCP_Server_Info *server)
> >> struct socket *socket = server->ssocket;
> >>
> >> if (socket == NULL) {
> >> - rc = sock_create_kern(PF_INET6, SOCK_STREAM,
> >> - IPPROTO_TCP, &socket);
> >> + rc = __sock_create(cifs_net_ns(server), PF_INET6,
> >> + SOCK_STREAM, IPPROTO_TCP, &socket, 1);
> >> if (rc < 0) {
> >> cERROR(1, "Error %d creating ipv6 socket", rc);
> >> - socket = NULL;
> >> return rc;
> >> }
>
> Note, those two add 16 bytes each on x86-64 (two extra 8-byte
> arguments), but that I couldn't easily stop. I might try to merge the
> ipv4/ipv6 functions, but that's a separate patch.
If anything I think you're saving more space with your patched code:
int sock_create_kern(int family, int type, int protocol, struct socket
**res)
{
return __sock_create(&init_net, family, type, protocol, res, 1);
}
EXPORT_SYMBOL(sock_create_kern);
It's not an inline function so I expect using sock_create_kern() will take up
more stack space than calling __sock_create() directly. On some archs
the instructions to make the extra function call might also take up
'significant' space. And you're passing an 8-byte pointer *anyway* so no size
difference there unless the compiler is more clever than I expect.
In my humble opinion you don't need to spend so much time counting bytes
:).
Cheers,
-Matt
_______________________________________________
Containers mailing list
Containers at lists.linux-foundation.org
https://lists.linux-foundation.org/mailman/listinfo/containers
More information about the Devel
mailing list