From owner-freebsd-net@FreeBSD.ORG Thu Mar 6 21:21:23 2014 Return-Path: Delivered-To: freebsd-net@freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [8.8.178.115]) (using TLSv1 with cipher ADH-AES256-SHA (256/256 bits)) (No client certificate requested) by hub.freebsd.org (Postfix) with ESMTPS id 0C791F92; Thu, 6 Mar 2014 21:21:23 +0000 (UTC) Received: from mail-we0-x234.google.com (mail-we0-x234.google.com [IPv6:2a00:1450:400c:c03::234]) (using TLSv1 with cipher ECDHE-RSA-RC4-SHA (128/128 bits)) (No client certificate requested) by mx1.freebsd.org (Postfix) with ESMTPS id 6BD50C94; Thu, 6 Mar 2014 21:21:22 +0000 (UTC) Received: by mail-we0-f180.google.com with SMTP id p61so3855182wes.11 for ; Thu, 06 Mar 2014 13:21:20 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20120113; h=mime-version:sender:date:message-id:subject:from:to:content-type; bh=V/rCI85u6latF+7E0Ku7bocyb5VIFwdMd8mUVIGGAUU=; b=IaPD0mc+F7YOkQT/XjJhd/1aHHro4lSljGcIU9UV0xLR6+MuDhmvGcko51obI36yBG z7yjheYgqGiDdg2++KQyuEDYOg3AcnE+fus99AcFjIe6NQBVGL40MCiFJj4HajeLFbbG YvNvotlPJM4Xmq20gcL0buA57i/d0WqmSu+xbB/I2o1sjXUCEAGAiHEJ5DfnMPNID7Pw rBf68ufLMLW3SqFOcByH4E/RNtjt0u0/YkXy4XxubMqgw97ucZcvC3kL8esGnxpdkebz UtEzfAnCmTwZser4/dypPKLVvXM5v87kHx8jczotizvQWYtiY2ULt8/pufxqAuK6E0G2 mkCA== MIME-Version: 1.0 X-Received: by 10.194.87.104 with SMTP id w8mr6873529wjz.90.1394140880847; Thu, 06 Mar 2014 13:21:20 -0800 (PST) Sender: asomers@gmail.com Received: by 10.194.168.197 with HTTP; Thu, 6 Mar 2014 13:21:20 -0800 (PST) Date: Thu, 6 Mar 2014 14:21:20 -0700 X-Google-Sender-Auth: LK6PDRk4BK1iXRYVaSDq1fNvLxk Message-ID: Subject: Review: patch for kern/185812 send(2) on a UNIX domain SEQPACKET socket returns EMSGSIZE instead of EAGAIN From: Alan Somers To: FreeBSD Net , rwatson@freebsd.org Content-Type: text/plain; charset=ISO-8859-1 X-BeenThere: freebsd-net@freebsd.org X-Mailman-Version: 2.1.17 Precedence: list List-Id: Networking and TCP/IP with FreeBSD List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Thu, 06 Mar 2014 21:21:23 -0000 Replace 4.4BSD Lite's unix domain socket backpressure hack with a cleaner mechanism, based on the new SB_STOP sockbuf flag. The old hack dynamically changed the sending sockbuf's high water mark whenever adding or removing data from the receiving sockbuf. It worked for stream sockets, but it never worked for SOCK_SEQPACKET sockets because of their atomic nature. If the sockbuf was partially full, it might return EMSGSIZE instead of blocking. The new solution is based on DragonFlyBSD's fix from commit 3a6117bbe0ed6a87605c1e43e12a1438d8844380 on 2008-05-27. It adds an SB_STOP flag to sockbufs. Whenever uipc_send surpasses the socket's size limit, it sets SB_STOP on the sending sockbuf. sbspace() will then return 0 for that sockbuf, causing sosend_generic and friends to block. uipc_rcvd will likewise clear SB_STOP. There are two fringe benefits: uipc_{send,rcvd} no longer need to call chgsbsize() on every send and receive because they don't change the sockbuf's high water mark. Also, uipc_sense no longer needs to acquire the UIPC linkage lock, because it's simpler to compute the st_blksizes. There is one drawback: since sbspace() will only ever return 0 or the maximum, sosend_generic will allow the sockbuf to exceed its nominal maximum size by at most one packet of size less than the max. I don't think that's a serious problem. In fact, I'm not even positive that FreeBSD guarantees a socket will always stay within its nominal size limit. sys/sys/sockbuf.h Add the SB_STOP flag and adjust sbspace() sys/sys/unpcb.h Delete the obsolete unp_cc and unp_mbcnt fields from struct unpcb. sys/kern/uipc_usrreq.c Adjust uipc_rcvd, uipc_send, and uipc_sense to use the SB_STOP backpressure mechanism. Removing obsolete unpcb fields from db_show_unpcb. tests/sys/socket/unix_seqpacket/unix_seqpacket.c Clear expected failures from ATF. Obtained from: DragonFly BSD PR: kern/185812 Index: sys/kern/uipc_usrreq.c =================================================================== --- sys/kern/uipc_usrreq.c (revision 262867) +++ sys/kern/uipc_usrreq.c (working copy) @@ -51,7 +51,6 @@ * * TODO: * RDM - * distinguish datagram size limits from flow control limits in SEQPACKET * rethink name space problems * need a proper out-of-band */ @@ -789,7 +788,6 @@ struct unpcb *unp, *unp2; struct socket *so2; u_int mbcnt, sbcc; - u_long newhiwat; unp = sotounpcb(so); KASSERT(unp != NULL, ("uipc_rcvd: unp == NULL")); @@ -811,6 +809,15 @@ mbcnt = so->so_rcv.sb_mbcnt; sbcc = so->so_rcv.sb_cc; SOCKBUF_UNLOCK(&so->so_rcv); + /* + * There is a benign race condition at this point. If we're planning to + * clear SB_STOP, but uipc_send is called on the connected socket at + * this instant, it might add data to the sockbuf and set SB_STOP. Then + * we would erroneously clear SB_STOP below, even though the sockbuf is + * full. The race is benign because the only ill effect is to allow the + * sockbuf to exceed its size limit, and the size limits are not + * strictly guaranteed anyway. + */ UNP_PCB_LOCK(unp); unp2 = unp->unp_conn; if (unp2 == NULL) { @@ -819,13 +826,9 @@ } so2 = unp2->unp_socket; SOCKBUF_LOCK(&so2->so_snd); - so2->so_snd.sb_mbmax += unp->unp_mbcnt - mbcnt; - newhiwat = so2->so_snd.sb_hiwat + unp->unp_cc - sbcc; - (void)chgsbsize(so2->so_cred->cr_uidinfo, &so2->so_snd.sb_hiwat, - newhiwat, RLIM_INFINITY); + if (sbcc < so2->so_snd.sb_hiwat && mbcnt < so2->so_snd.sb_mbmax) + so2->so_snd.sb_flags &= ~SB_STOP; sowwakeup_locked(so2); - unp->unp_mbcnt = mbcnt; - unp->unp_cc = sbcc; UNP_PCB_UNLOCK(unp); return (0); } @@ -836,8 +839,7 @@ { struct unpcb *unp, *unp2; struct socket *so2; - u_int mbcnt_delta, sbcc; - u_int newhiwat; + u_int mbcnt, sbcc; int error = 0; unp = sotounpcb(so); @@ -991,27 +993,21 @@ } } - /* - * XXXRW: While fine for SOCK_STREAM, this conflates maximum - * datagram size and back-pressure for SOCK_SEQPACKET, which - * can lead to undesired return of EMSGSIZE on send instead - * of more desirable blocking. - */ - mbcnt_delta = so2->so_rcv.sb_mbcnt - unp2->unp_mbcnt; - unp2->unp_mbcnt = so2->so_rcv.sb_mbcnt; + mbcnt = so2->so_rcv.sb_mbcnt; sbcc = so2->so_rcv.sb_cc; sorwakeup_locked(so2); + /* + * The PCB lock on unp2 protects the SB_STOP flag. Without it, + * it would be possible for uipc_rcvd to be called at this + * point, drain the receiving sockbuf, clear SB_STOP, and then + * we would set SB_STOP below. That could lead to an empty + * sockbuf having SB_STOP set + */ SOCKBUF_LOCK(&so->so_snd); - if ((int)so->so_snd.sb_hiwat >= (int)(sbcc - unp2->unp_cc)) - newhiwat = so->so_snd.sb_hiwat - (sbcc - unp2->unp_cc); - else - newhiwat = 0; - (void)chgsbsize(so->so_cred->cr_uidinfo, &so->so_snd.sb_hiwat, - newhiwat, RLIM_INFINITY); - so->so_snd.sb_mbmax -= mbcnt_delta; + if (sbcc >= so->so_snd.sb_hiwat || mbcnt >= so->so_snd.sb_mbmax) + so->so_snd.sb_flags |= SB_STOP; SOCKBUF_UNLOCK(&so->so_snd); - unp2->unp_cc = sbcc; UNP_PCB_UNLOCK(unp2); m = NULL; break; @@ -1049,27 +1045,18 @@ static int uipc_sense(struct socket *so, struct stat *sb) { - struct unpcb *unp, *unp2; - struct socket *so2; + struct unpcb *unp; unp = sotounpcb(so); KASSERT(unp != NULL, ("uipc_sense: unp == NULL")); sb->st_blksize = so->so_snd.sb_hiwat; - UNP_LINK_RLOCK(); UNP_PCB_LOCK(unp); - unp2 = unp->unp_conn; - if ((so->so_type == SOCK_STREAM || so->so_type == SOCK_SEQPACKET) && - unp2 != NULL) { - so2 = unp2->unp_socket; - sb->st_blksize += so2->so_rcv.sb_cc; - } sb->st_dev = NODEV; if (unp->unp_ino == 0) unp->unp_ino = (++unp_ino == 0) ? ++unp_ino : unp_ino; sb->st_ino = unp->unp_ino; UNP_PCB_UNLOCK(unp); - UNP_LINK_RUNLOCK(); return (0); } @@ -2497,8 +2484,7 @@ /* XXXRW: Would be nice to print the full address, if any. */ db_printf("unp_addr: %p\n", unp->unp_addr); - db_printf("unp_cc: %d unp_mbcnt: %d unp_gencnt: %llu\n", - unp->unp_cc, unp->unp_mbcnt, + db_printf("unp_gencnt: %llu\n", (unsigned long long)unp->unp_gencnt); db_printf("unp_flags: %x (", unp->unp_flags); Index: sys/sys/sockbuf.h =================================================================== --- sys/sys/sockbuf.h (revision 262867) +++ sys/sys/sockbuf.h (working copy) @@ -52,6 +52,7 @@ #define SB_NOCOALESCE 0x200 /* don't coalesce new data into existing mbufs */ #define SB_IN_TOE 0x400 /* socket buffer is in the middle of an operation */ #define SB_AUTOSIZE 0x800 /* automatically size socket buffer */ +#define SB_STOP 0x1000 /* backpressure indicator */ #define SBS_CANTSENDMORE 0x0010 /* can't send more data to peer */ #define SBS_CANTRCVMORE 0x0020 /* can't receive more data from peer */ @@ -168,10 +169,20 @@ * still be negative (cc > hiwat or mbcnt > mbmax). Should detect * overflow and return 0. Should use "lmin" but it doesn't exist now. */ -#define sbspace(sb) \ - ((long) imin((int)((sb)->sb_hiwat - (sb)->sb_cc), \ - (int)((sb)->sb_mbmax - (sb)->sb_mbcnt))) +static __inline +long +sbspace(struct sockbuf *sb) +{ + long bleft; + long mleft; + if (sb->sb_flags & SB_STOP) + return(0); + bleft = sb->sb_hiwat - sb->sb_cc; + mleft = sb->sb_mbmax - sb->sb_mbcnt; + return((bleft < mleft) ? bleft : mleft); +} + /* adjust counters in sb reflecting allocation of m */ #define sballoc(sb, m) { \ (sb)->sb_cc += (m)->m_len; \ Index: sys/sys/unpcb.h =================================================================== --- sys/sys/unpcb.h (revision 262867) +++ sys/sys/unpcb.h (working copy) @@ -74,8 +74,8 @@ struct unp_head unp_refs; /* referencing socket linked list */ LIST_ENTRY(unpcb) unp_reflink; /* link in unp_refs list */ struct sockaddr_un *unp_addr; /* bound address of socket */ - int unp_cc; /* copy of rcv.sb_cc */ - int unp_mbcnt; /* copy of rcv.sb_mbcnt */ + int reserved1; + int reserved2; unp_gen_t unp_gencnt; /* generation count of this instance */ short unp_flags; /* flags */ short unp_gcflag; /* Garbage collector flags. */ Index: tests/sys/kern/unix_seqpacket_test.c =================================================================== --- tests/sys/kern/unix_seqpacket_test.c (revision 262867) +++ tests/sys/kern/unix_seqpacket_test.c (working copy) @@ -853,25 +853,21 @@ ATF_TC_WITHOUT_HEAD(eagain_8k_8k); ATF_TC_BODY(eagain_8k_8k, tc) { - atf_tc_expect_fail("PR kern/185812 send(2) on a UNIX domain SEQPACKET socket returns EMSGSIZE instead of EAGAIN"); test_eagain(8192, 8192); } ATF_TC_WITHOUT_HEAD(eagain_8k_128k); ATF_TC_BODY(eagain_8k_128k, tc) { - atf_tc_expect_fail("PR kern/185812 send(2) on a UNIX domain SEQPACKET socket returns EMSGSIZE instead of EAGAIN"); test_eagain(8192, 131072); } ATF_TC_WITHOUT_HEAD(eagain_128k_8k); ATF_TC_BODY(eagain_128k_8k, tc) { - atf_tc_expect_fail("PR kern/185812 send(2) on a UNIX domain SEQPACKET socket returns EMSGSIZE instead of EAGAIN"); test_eagain(131072, 8192); } ATF_TC_WITHOUT_HEAD(eagain_128k_128k); ATF_TC_BODY(eagain_128k_128k, tc) { - atf_tc_expect_fail("PR kern/185812 send(2) on a UNIX domain SEQPACKET socket returns EMSGSIZE instead of EAGAIN"); test_eagain(131072, 131072); } @@ -974,37 +970,24 @@ ATF_TC_WITHOUT_HEAD(pipe_8k_8k); ATF_TC_BODY(pipe_8k_8k, tc) { - atf_tc_expect_fail("PR kern/185812 send(2) on a UNIX domain SEQPACKET socket returns EMSGSIZE instead of EAGAIN"); test_pipe(8192, 8192); } ATF_TC_WITHOUT_HEAD(pipe_8k_128k); ATF_TC_BODY(pipe_8k_128k, tc) { - atf_tc_expect_fail("PR kern/185812 send(2) on a UNIX domain SEQPACKET socket returns EMSGSIZE instead of EAGAIN"); test_pipe(8192, 131072); } ATF_TC_WITHOUT_HEAD(pipe_128k_8k); ATF_TC_BODY(pipe_128k_8k, tc) { - /* - * kern/185812 causes this test case to both fail and timeout. The - * atf-c-api(3) doesn't have a way to set such an expectation. - * If you use atf_tc_expect_fail, then it will timeout. If you use - * atf_tc_expect_timeout, then it will fail. If you use both, then it - * will show up as an unexpected pass, which is much worse - * - * https://code.google.com/p/kyua/issues/detail?id=76 - */ - atf_tc_expect_fail("PR kern/185812 send(2) on a UNIX domain SEQPACKET socket returns EMSGSIZE instead of EAGAIN"); test_pipe(131072, 8192); } ATF_TC_WITHOUT_HEAD(pipe_128k_128k); ATF_TC_BODY(pipe_128k_128k, tc) { - atf_tc_expect_fail("PR kern/185812 send(2) on a UNIX domain SEQPACKET socket returns EMSGSIZE instead of EAGAIN"); test_pipe(131072, 131072); }