On Wed, Jan 25, 2017 at 04:32:25PM +1000, Martin Pieuchot wrote:
> I just enabled the NET_LOCK() again and I'm looking for test reports.
> Please go build a kernel from sources or wait for the next snapshot,
> run it and report back.
>
> If you're looking for some small coding tasks related to the NET_LOCK()
> just do:
>
> # sysctl kern.splassert=2
> # sysctl kern.pool_debug=2
>
> Then watch for the traces on your console.
>
> You'll see something like:
>
> Starting stack trace...
> yield(0,1,d09dac52,f5549dbc,d94e9378) at yield+0xa4
> yield(d0bc8f40,1,f5549e18,80,14) at yield+0xa4
> pool_get(d0bc8f40,1,f5549ec8,d03ecbfb,d97815f4) at pool_get+0x1ba
> m_get(1,3,f5549ec0,d03a9362,d0bc22e0) at m_get+0x30
> doaccept(d977e6c4,3,cf7ee4f8,cf7ee4ec,2000) at doaccept+0x193
> sys_accept(d977e6c4,f5549f5c,f5549f7c,0,f5549fa8) at sys_accept+0x37
> syscall() at syscall+0x250
>
> This means accept(2) is doing a memory allocation that can sleep, here
> with m_get(9), while holding the NET_LOCK(). Even if these should be
> ok, it is easy to avoid them. In the case of doaccept() a mbuf could
> be allocated beforehand or simply use the stack for that.
>
> Cheers,
> Martin
>
Allocate the mbuf beforehand. Also, move the setting of nflag closer
to where its value is used.
Index: uipc_syscalls.c
===================================================================
RCS file: /cvs/src/sys/kern/uipc_syscalls.c,v
retrieving revision 1.147
diff -u -p -r1.147 uipc_syscalls.c
--- uipc_syscalls.c 25 Jan 2017 07:35:31 -0000 1.147
+++ uipc_syscalls.c 26 Jan 2017 01:03:13 -0000
@@ -265,7 +265,7 @@ doaccept(struct proc *p, int sock, struc
{
struct filedesc *fdp = p->p_fd;
struct file *fp, *headfp;
- struct mbuf *nam = NULL;
+ struct mbuf *nam;
socklen_t namelen;
int error, s, tmpfd;
struct socket *head, *so;
@@ -288,7 +288,8 @@ doaccept(struct proc *p, int sock, struc
return (error);
}
-redo:
+ nam = m_get(M_WAIT, MT_SONAME);
+
NET_LOCK(s);
head = headfp->f_data;
if (isdnssocket(head) || (head->so_options & SO_ACCEPTCONN) == 0) {
@@ -318,30 +319,16 @@ redo:
goto out;
}
- /* Figure out whether the new socket should be non-blocking. */
- nflag = flags & SOCK_NONBLOCK_INHERIT ? (headfp->f_flag & FNONBLOCK)
- : (flags & SOCK_NONBLOCK ? FNONBLOCK : 0);
-
- nam = m_get(M_WAIT, MT_SONAME);
-
- /*
- * Check whether the queue emptied while we slept: m_get() may have
- * blocked, allowing the connection to be reset or another thread or
- * process to accept it. If so, start over.
- */
- if (head->so_qlen == 0) {
- NET_UNLOCK(s);
- m_freem(nam);
- nam = NULL;
- goto redo;
- }
-
/*
* Do not sleep after we have taken the socket out of the queue.
*/
so = TAILQ_FIRST(&head->so_q);
if (soqremque(so, 1) == 0)
panic("accept");
+
+ /* Figure out whether the new socket should be non-blocking. */
+ nflag = flags & SOCK_NONBLOCK_INHERIT ? (headfp->f_flag & FNONBLOCK)
+ : (flags & SOCK_NONBLOCK ? FNONBLOCK : 0);
/* connection has been removed from the listen queue */
KNOTE(&head->so_rcv.sb_sel.si_note, 0);
