Re: solock() & nfs_connect
On Fri, Sep 01, 2017 at 06:29:33PM +0200, Martin Pieuchot wrote:
> Here's an alternative approach that pre-allocate mbufs. This reduce
> the number of MGET() in the function an allow us to do a single
> solock()/sounlock() dance.
Yes, I like that. You could rename m to nam to be consistent with
other functions and make clear what this mbuf is used for.
> ok?
OK bluhm@
> Index: nfs//nfs_socket.c
> ===
> RCS file: /cvs/src/sys/nfs/nfs_socket.c,v
> retrieving revision 1.126
> diff -u -p -r1.126 nfs_socket.c
> --- nfs//nfs_socket.c 1 Sep 2017 15:05:31 - 1.126
> +++ nfs//nfs_socket.c 1 Sep 2017 16:09:16 -
> @@ -238,20 +238,28 @@ nfs_connect(struct nfsmount *nmp, struct
> int s, error, rcvreserve, sndreserve;
> struct sockaddr *saddr;
> struct sockaddr_in *sin;
> - struct mbuf *m;
> + struct mbuf *m = NULL, *mopt = NULL;
>
> - if (!(nmp->nm_sotype == SOCK_DGRAM || nmp->nm_sotype == SOCK_STREAM)) {
> - error = EINVAL;
> - goto bad;
> - }
> + if (!(nmp->nm_sotype == SOCK_DGRAM || nmp->nm_sotype == SOCK_STREAM))
> + return (EINVAL);
>
> nmp->nm_so = NULL;
> saddr = mtod(nmp->nm_nam, struct sockaddr *);
> error = socreate(saddr->sa_family, &nmp->nm_so, nmp->nm_sotype,
> nmp->nm_soproto);
> - if (error)
> - goto bad;
> + if (error) {
> + nfs_disconnect(nmp);
> + return (error);
> + }
> +
> + /* Allocate mbufs possibly waiting before grabbing the socket lock. */
> + if (nmp->nm_sotype == SOCK_STREAM || saddr->sa_family == AF_INET)
> + MGET(mopt, M_WAIT, MT_SOOPTS);
> + if (saddr->sa_family == AF_INET)
> + MGET(m, M_WAIT, MT_SONAME);
> +
> so = nmp->nm_so;
> + s = solock(so);
> nmp->nm_soflags = so->so_proto->pr_flags;
>
> /*
> @@ -260,42 +268,29 @@ nfs_connect(struct nfsmount *nmp, struct
>* disclosure through UDP port capture.
>*/
> if (saddr->sa_family == AF_INET) {
> - struct mbuf *mopt;
> int *ip;
>
> - MGET(mopt, M_WAIT, MT_SOOPTS);
> mopt->m_len = sizeof(int);
> ip = mtod(mopt, int *);
> *ip = IP_PORTRANGE_LOW;
> - s = solock(so);
> error = sosetopt(so, IPPROTO_IP, IP_PORTRANGE, mopt);
> - sounlock(s);
> - m_freem(mopt);
> if (error)
> goto bad;
>
> - MGET(m, M_WAIT, MT_SONAME);
> sin = mtod(m, struct sockaddr_in *);
> memset(sin, 0, sizeof(*sin));
> sin->sin_len = m->m_len = sizeof(struct sockaddr_in);
> sin->sin_family = AF_INET;
> sin->sin_addr.s_addr = INADDR_ANY;
> sin->sin_port = htons(0);
> - s = solock(so);
> error = sobind(so, m, &proc0);
> - sounlock(s);
> - m_freem(m);
> if (error)
> goto bad;
>
> - MGET(mopt, M_WAIT, MT_SOOPTS);
> mopt->m_len = sizeof(int);
> ip = mtod(mopt, int *);
> *ip = IP_PORTRANGE_DEFAULT;
> - s = solock(so);
> error = sosetopt(so, IPPROTO_IP, IP_PORTRANGE, mopt);
> - sounlock(s);
> - m_freem(mopt);
> if (error)
> goto bad;
> }
> @@ -310,12 +305,9 @@ nfs_connect(struct nfsmount *nmp, struct
> goto bad;
> }
> } else {
> - s = solock(so);
> error = soconnect(so, nmp->nm_nam);
> - if (error) {
> - sounlock(s);
> + if (error)
> goto bad;
> - }
>
> /*
>* Wait for the connection to complete. Cribbed from the
> @@ -328,23 +320,19 @@ nfs_connect(struct nfsmount *nmp, struct
> so->so_error == 0 && rep &&
> (error = nfs_sigintr(nmp, rep, rep->r_procp)) != 0){
> so->so_state &= ~SS_ISCONNECTING;
> - sounlock(s);
> goto bad;
> }
> }
> if (so->so_error) {
> error = so->so_error;
> so->so_error = 0;
> - sounlock(s);
> goto bad;
> }
> - sounlock(s);
> }
> /*
>* Always set receive timeout to detect server crash and reconnect.
>* Otherwise, we can get stuck in soreceive forever.
>*/
> - s = solock(so);
> so->so_rcv.sb_timeo = (5 * hz);
> if (nmp->nm_flag & (NFSMNT_SOFT | NFSMNT_INT))
> so->so_snd.sb_timeo = (5 * hz);
> @@ -356,18 +344,14 @@ nfs_connect(s
Re: solock() & nfs_connect
On 11/08/17(Fri) 15:32, Martin Pieuchot wrote:
> On 11/08/17(Fri) 20:31, Alexander Bluhm wrote:
> > On Fri, Aug 11, 2017 at 01:18:48PM -0400, Martin Pieuchot wrote:
> > > Diff below merge all solock()/sounlock() dances inside nfs_connect().
> >
> > Now we sleep for memory while holding the lock. Is this a good
> > idea?
Here's an alternative approach that pre-allocate mbufs. This reduce
the number of MGET() in the function an allow us to do a single
solock()/sounlock() dance.
ok?
Index: nfs//nfs_socket.c
===
RCS file: /cvs/src/sys/nfs/nfs_socket.c,v
retrieving revision 1.126
diff -u -p -r1.126 nfs_socket.c
--- nfs//nfs_socket.c 1 Sep 2017 15:05:31 - 1.126
+++ nfs//nfs_socket.c 1 Sep 2017 16:09:16 -
@@ -238,20 +238,28 @@ nfs_connect(struct nfsmount *nmp, struct
int s, error, rcvreserve, sndreserve;
struct sockaddr *saddr;
struct sockaddr_in *sin;
- struct mbuf *m;
+ struct mbuf *m = NULL, *mopt = NULL;
- if (!(nmp->nm_sotype == SOCK_DGRAM || nmp->nm_sotype == SOCK_STREAM)) {
- error = EINVAL;
- goto bad;
- }
+ if (!(nmp->nm_sotype == SOCK_DGRAM || nmp->nm_sotype == SOCK_STREAM))
+ return (EINVAL);
nmp->nm_so = NULL;
saddr = mtod(nmp->nm_nam, struct sockaddr *);
error = socreate(saddr->sa_family, &nmp->nm_so, nmp->nm_sotype,
nmp->nm_soproto);
- if (error)
- goto bad;
+ if (error) {
+ nfs_disconnect(nmp);
+ return (error);
+ }
+
+ /* Allocate mbufs possibly waiting before grabbing the socket lock. */
+ if (nmp->nm_sotype == SOCK_STREAM || saddr->sa_family == AF_INET)
+ MGET(mopt, M_WAIT, MT_SOOPTS);
+ if (saddr->sa_family == AF_INET)
+ MGET(m, M_WAIT, MT_SONAME);
+
so = nmp->nm_so;
+ s = solock(so);
nmp->nm_soflags = so->so_proto->pr_flags;
/*
@@ -260,42 +268,29 @@ nfs_connect(struct nfsmount *nmp, struct
* disclosure through UDP port capture.
*/
if (saddr->sa_family == AF_INET) {
- struct mbuf *mopt;
int *ip;
- MGET(mopt, M_WAIT, MT_SOOPTS);
mopt->m_len = sizeof(int);
ip = mtod(mopt, int *);
*ip = IP_PORTRANGE_LOW;
- s = solock(so);
error = sosetopt(so, IPPROTO_IP, IP_PORTRANGE, mopt);
- sounlock(s);
- m_freem(mopt);
if (error)
goto bad;
- MGET(m, M_WAIT, MT_SONAME);
sin = mtod(m, struct sockaddr_in *);
memset(sin, 0, sizeof(*sin));
sin->sin_len = m->m_len = sizeof(struct sockaddr_in);
sin->sin_family = AF_INET;
sin->sin_addr.s_addr = INADDR_ANY;
sin->sin_port = htons(0);
- s = solock(so);
error = sobind(so, m, &proc0);
- sounlock(s);
- m_freem(m);
if (error)
goto bad;
- MGET(mopt, M_WAIT, MT_SOOPTS);
mopt->m_len = sizeof(int);
ip = mtod(mopt, int *);
*ip = IP_PORTRANGE_DEFAULT;
- s = solock(so);
error = sosetopt(so, IPPROTO_IP, IP_PORTRANGE, mopt);
- sounlock(s);
- m_freem(mopt);
if (error)
goto bad;
}
@@ -310,12 +305,9 @@ nfs_connect(struct nfsmount *nmp, struct
goto bad;
}
} else {
- s = solock(so);
error = soconnect(so, nmp->nm_nam);
- if (error) {
- sounlock(s);
+ if (error)
goto bad;
- }
/*
* Wait for the connection to complete. Cribbed from the
@@ -328,23 +320,19 @@ nfs_connect(struct nfsmount *nmp, struct
so->so_error == 0 && rep &&
(error = nfs_sigintr(nmp, rep, rep->r_procp)) != 0){
so->so_state &= ~SS_ISCONNECTING;
- sounlock(s);
goto bad;
}
}
if (so->so_error) {
error = so->so_error;
so->so_error = 0;
- sounlock(s);
goto bad;
}
- sounlock(s);
}
/*
* Always set receive timeout to detect server crash and reconnect.
* Otherwise, we can get stuck in soreceive forever.
*/
- s = solock(so);
so->so_rcv.sb_timeo = (5 * hz);
if (nmp->nm_flag & (NFSMNT_SOFT | NFSMNT_INT))
Re: solock() & nfs_connect
On 11/08/17(Fri) 20:31, Alexander Bluhm wrote:
> On Fri, Aug 11, 2017 at 01:18:48PM -0400, Martin Pieuchot wrote:
> > Diff below merge all solock()/sounlock() dances inside nfs_connect().
>
> Now we sleep for memory while holding the lock. Is this a good
> idea?
As long as a subsystem outside the NET_LOCK() can release memory,
sleeping with the lock held is fine.
>What is the advantage of changing it?
Not introducing multiple sleeping points in a function manipulating a
socket.
> The label names "bad" and "out" are confusing. Both are bad, but
> bad also does an unlock.
I kept 'bad' which now does the unlock and get rid of the 'out' label,
ok?
Index: nfs/nfs_socket.c
===
RCS file: /cvs/src/sys/nfs/nfs_socket.c,v
retrieving revision 1.122
diff -u -p -r1.122 nfs_socket.c
--- nfs/nfs_socket.c10 Aug 2017 19:20:43 - 1.122
+++ nfs/nfs_socket.c11 Aug 2017 19:31:41 -
@@ -244,9 +244,13 @@ nfs_connect(struct nfsmount *nmp, struct
saddr = mtod(nmp->nm_nam, struct sockaddr *);
error = socreate(saddr->sa_family, &nmp->nm_so, nmp->nm_sotype,
nmp->nm_soproto);
- if (error)
- goto bad;
+ if (error) {
+ nfs_disconnect(nmp);
+ return (error);
+ }
+
so = nmp->nm_so;
+ s = solock(so);
nmp->nm_soflags = so->so_proto->pr_flags;
/*
@@ -262,9 +266,7 @@ nfs_connect(struct nfsmount *nmp, struct
mopt->m_len = sizeof(int);
ip = mtod(mopt, int *);
*ip = IP_PORTRANGE_LOW;
- s = solock(so);
error = sosetopt(so, IPPROTO_IP, IP_PORTRANGE, mopt);
- sounlock(s);
if (error)
goto bad;
@@ -275,9 +277,7 @@ nfs_connect(struct nfsmount *nmp, struct
sin->sin_family = AF_INET;
sin->sin_addr.s_addr = INADDR_ANY;
sin->sin_port = htons(0);
- s = solock(so);
error = sobind(so, m, &proc0);
- sounlock(s);
m_freem(m);
if (error)
goto bad;
@@ -286,9 +286,7 @@ nfs_connect(struct nfsmount *nmp, struct
mopt->m_len = sizeof(int);
ip = mtod(mopt, int *);
*ip = IP_PORTRANGE_DEFAULT;
- s = solock(so);
error = sosetopt(so, IPPROTO_IP, IP_PORTRANGE, mopt);
- sounlock(s);
if (error)
goto bad;
}
@@ -303,12 +301,9 @@ nfs_connect(struct nfsmount *nmp, struct
goto bad;
}
} else {
- s = solock(so);
error = soconnect(so, nmp->nm_nam);
- if (error) {
- sounlock(s);
+ if (error)
goto bad;
- }
/*
* Wait for the connection to complete. Cribbed from the
@@ -321,23 +316,19 @@ nfs_connect(struct nfsmount *nmp, struct
so->so_error == 0 && rep &&
(error = nfs_sigintr(nmp, rep, rep->r_procp)) != 0){
so->so_state &= ~SS_ISCONNECTING;
- sounlock(s);
goto bad;
}
}
if (so->so_error) {
error = so->so_error;
so->so_error = 0;
- sounlock(s);
goto bad;
}
- sounlock(s);
}
/*
* Always set receive timeout to detect server crash and reconnect.
* Otherwise, we can get stuck in soreceive forever.
*/
- s = solock(so);
so->so_rcv.sb_timeo = (5 * hz);
if (nmp->nm_flag & (NFSMNT_SOFT | NFSMNT_INT))
so->so_snd.sb_timeo = (5 * hz);
@@ -372,11 +363,11 @@ nfs_connect(struct nfsmount *nmp, struct
sizeof (u_int32_t)) * 2;
}
error = soreserve(so, sndreserve, rcvreserve);
- sounlock(s);
if (error)
goto bad;
so->so_rcv.sb_flags |= SB_NOINTR;
so->so_snd.sb_flags |= SB_NOINTR;
+ sounlock(s);
/* Initialize other non-zero congestion variables */
nfs_init_rtt(nmp);
@@ -386,6 +377,7 @@ nfs_connect(struct nfsmount *nmp, struct
return (0);
bad:
+ sounlock(s);
nfs_disconnect(nmp);
return (error);
}
Re: solock() & nfs_connect
On Fri, Aug 11, 2017 at 01:18:48PM -0400, Martin Pieuchot wrote:
> Diff below merge all solock()/sounlock() dances inside nfs_connect().
Now we sleep for memory while holding the lock. Is this a good
idea? What is the advantage of changing it?
The label names "bad" and "out" are confusing. Both are bad, but
bad also does an unlock.
bluhm
> Index: nfs/nfs_socket.c
> ===
> RCS file: /cvs/src/sys/nfs/nfs_socket.c,v
> retrieving revision 1.122
> diff -u -p -r1.122 nfs_socket.c
> --- nfs/nfs_socket.c 10 Aug 2017 19:20:43 - 1.122
> +++ nfs/nfs_socket.c 11 Aug 2017 17:14:57 -
> @@ -245,8 +245,9 @@ nfs_connect(struct nfsmount *nmp, struct
> error = socreate(saddr->sa_family, &nmp->nm_so, nmp->nm_sotype,
> nmp->nm_soproto);
> if (error)
> - goto bad;
> + goto out;
> so = nmp->nm_so;
> + s = solock(so);
> nmp->nm_soflags = so->so_proto->pr_flags;
>
> /*
> @@ -262,9 +263,7 @@ nfs_connect(struct nfsmount *nmp, struct
> mopt->m_len = sizeof(int);
> ip = mtod(mopt, int *);
> *ip = IP_PORTRANGE_LOW;
> - s = solock(so);
> error = sosetopt(so, IPPROTO_IP, IP_PORTRANGE, mopt);
> - sounlock(s);
> if (error)
> goto bad;
>
> @@ -275,9 +274,7 @@ nfs_connect(struct nfsmount *nmp, struct
> sin->sin_family = AF_INET;
> sin->sin_addr.s_addr = INADDR_ANY;
> sin->sin_port = htons(0);
> - s = solock(so);
> error = sobind(so, m, &proc0);
> - sounlock(s);
> m_freem(m);
> if (error)
> goto bad;
> @@ -286,9 +283,7 @@ nfs_connect(struct nfsmount *nmp, struct
> mopt->m_len = sizeof(int);
> ip = mtod(mopt, int *);
> *ip = IP_PORTRANGE_DEFAULT;
> - s = solock(so);
> error = sosetopt(so, IPPROTO_IP, IP_PORTRANGE, mopt);
> - sounlock(s);
> if (error)
> goto bad;
> }
> @@ -303,12 +298,9 @@ nfs_connect(struct nfsmount *nmp, struct
> goto bad;
> }
> } else {
> - s = solock(so);
> error = soconnect(so, nmp->nm_nam);
> - if (error) {
> - sounlock(s);
> + if (error)
> goto bad;
> - }
>
> /*
>* Wait for the connection to complete. Cribbed from the
> @@ -321,23 +313,19 @@ nfs_connect(struct nfsmount *nmp, struct
> so->so_error == 0 && rep &&
> (error = nfs_sigintr(nmp, rep, rep->r_procp)) != 0){
> so->so_state &= ~SS_ISCONNECTING;
> - sounlock(s);
> goto bad;
> }
> }
> if (so->so_error) {
> error = so->so_error;
> so->so_error = 0;
> - sounlock(s);
> goto bad;
> }
> - sounlock(s);
> }
> /*
>* Always set receive timeout to detect server crash and reconnect.
>* Otherwise, we can get stuck in soreceive forever.
>*/
> - s = solock(so);
> so->so_rcv.sb_timeo = (5 * hz);
> if (nmp->nm_flag & (NFSMNT_SOFT | NFSMNT_INT))
> so->so_snd.sb_timeo = (5 * hz);
> @@ -372,11 +360,11 @@ nfs_connect(struct nfsmount *nmp, struct
> sizeof (u_int32_t)) * 2;
> }
> error = soreserve(so, sndreserve, rcvreserve);
> - sounlock(s);
> if (error)
> goto bad;
> so->so_rcv.sb_flags |= SB_NOINTR;
> so->so_snd.sb_flags |= SB_NOINTR;
> + sounlock(s);
>
> /* Initialize other non-zero congestion variables */
> nfs_init_rtt(nmp);
> @@ -386,6 +374,8 @@ nfs_connect(struct nfsmount *nmp, struct
> return (0);
>
> bad:
> + sounlock(s);
> +out:
> nfs_disconnect(nmp);
> return (error);
> }
solock() & nfs_connect
Diff below merge all solock()/sounlock() dances inside nfs_connect().
ok?
Index: nfs/nfs_socket.c
===
RCS file: /cvs/src/sys/nfs/nfs_socket.c,v
retrieving revision 1.122
diff -u -p -r1.122 nfs_socket.c
--- nfs/nfs_socket.c10 Aug 2017 19:20:43 - 1.122
+++ nfs/nfs_socket.c11 Aug 2017 17:14:57 -
@@ -245,8 +245,9 @@ nfs_connect(struct nfsmount *nmp, struct
error = socreate(saddr->sa_family, &nmp->nm_so, nmp->nm_sotype,
nmp->nm_soproto);
if (error)
- goto bad;
+ goto out;
so = nmp->nm_so;
+ s = solock(so);
nmp->nm_soflags = so->so_proto->pr_flags;
/*
@@ -262,9 +263,7 @@ nfs_connect(struct nfsmount *nmp, struct
mopt->m_len = sizeof(int);
ip = mtod(mopt, int *);
*ip = IP_PORTRANGE_LOW;
- s = solock(so);
error = sosetopt(so, IPPROTO_IP, IP_PORTRANGE, mopt);
- sounlock(s);
if (error)
goto bad;
@@ -275,9 +274,7 @@ nfs_connect(struct nfsmount *nmp, struct
sin->sin_family = AF_INET;
sin->sin_addr.s_addr = INADDR_ANY;
sin->sin_port = htons(0);
- s = solock(so);
error = sobind(so, m, &proc0);
- sounlock(s);
m_freem(m);
if (error)
goto bad;
@@ -286,9 +283,7 @@ nfs_connect(struct nfsmount *nmp, struct
mopt->m_len = sizeof(int);
ip = mtod(mopt, int *);
*ip = IP_PORTRANGE_DEFAULT;
- s = solock(so);
error = sosetopt(so, IPPROTO_IP, IP_PORTRANGE, mopt);
- sounlock(s);
if (error)
goto bad;
}
@@ -303,12 +298,9 @@ nfs_connect(struct nfsmount *nmp, struct
goto bad;
}
} else {
- s = solock(so);
error = soconnect(so, nmp->nm_nam);
- if (error) {
- sounlock(s);
+ if (error)
goto bad;
- }
/*
* Wait for the connection to complete. Cribbed from the
@@ -321,23 +313,19 @@ nfs_connect(struct nfsmount *nmp, struct
so->so_error == 0 && rep &&
(error = nfs_sigintr(nmp, rep, rep->r_procp)) != 0){
so->so_state &= ~SS_ISCONNECTING;
- sounlock(s);
goto bad;
}
}
if (so->so_error) {
error = so->so_error;
so->so_error = 0;
- sounlock(s);
goto bad;
}
- sounlock(s);
}
/*
* Always set receive timeout to detect server crash and reconnect.
* Otherwise, we can get stuck in soreceive forever.
*/
- s = solock(so);
so->so_rcv.sb_timeo = (5 * hz);
if (nmp->nm_flag & (NFSMNT_SOFT | NFSMNT_INT))
so->so_snd.sb_timeo = (5 * hz);
@@ -372,11 +360,11 @@ nfs_connect(struct nfsmount *nmp, struct
sizeof (u_int32_t)) * 2;
}
error = soreserve(so, sndreserve, rcvreserve);
- sounlock(s);
if (error)
goto bad;
so->so_rcv.sb_flags |= SB_NOINTR;
so->so_snd.sb_flags |= SB_NOINTR;
+ sounlock(s);
/* Initialize other non-zero congestion variables */
nfs_init_rtt(nmp);
@@ -386,6 +374,8 @@ nfs_connect(struct nfsmount *nmp, struct
return (0);
bad:
+ sounlock(s);
+out:
nfs_disconnect(nmp);
return (error);
}
