inpcb: retire the inpcb global list

The iteration over all pcbs is possible without the global list. The
newborn inpcbs are put on a global list of unconnected inpcbs, then after
connect(2) or bind(2) they move to respective hash slot list.

This adds a bit of complexity to inp_next(), but the storage scheme is
actually simplified.

One potential problem before this change was that a couple of pcbs fall
into the same hash slot and are linked A->B there, but they also sit next
to each other in the global list, linked as B->A.  This can deadlock of
course.  The problem was never observed in the wild, but I was able to
instrument it with lots of effort: just few pcbs in the system, hash size
reduced down to 2 and a lot of repetitive calls into two kinds of
iterators.

However the main motivation is not the above problem, but make a step
towards splitting the big hash lock into per-slot locks.

Differential Revision:	https://reviews.freebsd.org/D55967

sys/netinet/in_pcb.c

@@ -240,8 +240,6 @@ SYSCTL_INT(_net_inet_ip, OID_AUTO, connect_inaddr_wild,
     "Allow connecting to INADDR_ANY or INADDR_BROADCAST for connect(2)");
 #endif
 
-static void in_pcbremhash(struct inpcb *);
-
 /*
  * in_pcb.c: manage the Protocol Control Blocks.
  *
@@ -563,7 +561,7 @@ in_pcbinfo_init(struct inpcbinfo *pcbinfo, struct inpcbstorage *pcbstor,
 #ifdef VIMAGE
 	pcbinfo->ipi_vnet = curvnet;
 #endif
-	CK_LIST_INIT(&pcbinfo->ipi_listhead);
+	CK_LIST_INIT(&pcbinfo->ipi_list_unconn);
 	pcbinfo->ipi_count = 0;
 
 	ha.size = hash_nelements;
@@ -698,7 +696,7 @@ in_pcballoc(struct socket *so, struct inpcbinfo *pcbinfo)
 	INP_HASH_WLOCK(pcbinfo);
 	pcbinfo->ipi_count++;
 	inp->inp_gencnt = ++pcbinfo->ipi_gencnt;
-	CK_LIST_INSERT_HEAD(&pcbinfo->ipi_listhead, inp, inp_list);
+	CK_LIST_INSERT_HEAD(&pcbinfo->ipi_list_unconn, inp, inp_unconn_list);
 	INP_HASH_WUNLOCK(pcbinfo);
 	so->so_pcb = inp;
 
@@ -1429,7 +1427,9 @@ in_pcbdisconnect(struct inpcb *inp)
 	KASSERT(inp->inp_smr == SMR_SEQ_INVALID,
 	    ("%s: inp %p was already disconnected", __func__, inp));
 
-	in_pcbremhash_locked(inp);
+	in_pcbremhash(inp);
+	CK_LIST_INSERT_HEAD(&inp->inp_pcbinfo->ipi_list_unconn, inp,
+	    inp_unconn_list);
 
 	if ((inp->inp_socket->so_proto->pr_flags & PR_CONNREQUIRED) == 0) {
 		/* See the comment in in_pcbinshash(). */
@@ -1552,8 +1552,17 @@ inp_smr_lock(struct inpcb *inp, const inp_lookup_t lock)
  *
  * - Iterator can have either write-lock or read-lock semantics, that can not
  *   be changed later.
- * - Iterator can iterate either over all pcbs list (INP_ALL_LIST), or through
+ * - Iterator has three modes of operation, defined by value of .hash member
- *   a single hash slot.  Note: only rip_input() does the latter.
+ *   on the first call:
+ *   - .hash = INP_ALL_LIST: the iterator will go through the unconnected
+ *     list, then all wildcard hash slots and then all exact hash slots.
+ *   - .hash = INP_UNCONN_LIST: the iterator will go through the list of
+ *     unconnected pcbs only.
+ *   - .hash initialized with an arbitrary positive value: iterator will go
+ *     through this exact hash slot only.
+ *   Note: only rip_input() and sysctl_setsockopt() use the latter.
+ *   The interface may be extended for iteration over single wildcard hash
+ *   slot, but there is no use case for that today.
  * - Iterator may have optional bool matching function.  The matching function
  *   will be executed for each inpcb in the SMR context, so it can not acquire
  *   locks and can safely access only immutable fields of inpcb.
@@ -1571,49 +1580,72 @@ inp_smr_lock(struct inpcb *inp, const inp_lookup_t lock)
  * - Removed entries won't stop traversal as long as they are not added to
  *   a different list. This is violated by in_pcbrehash().
  */
-#define	II_LIST_FIRST(ipi, hash)					\
+static inline struct inpcb *
-		(((hash) == INP_ALL_LIST) ?				\
+ii_list_first(const struct inpcb_iterator *ii)
-		    CK_LIST_FIRST(&(ipi)->ipi_listhead) :		\
+{
-		    CK_LIST_FIRST(&(ipi)->ipi_hash_exact[(hash)]))
+	const struct inpcbinfo *ipi = ii->ipi;
-#define	II_LIST_NEXT(inp, hash)						\
+	const int hash = ii->hash;
-		(((hash) == INP_ALL_LIST) ?				\
+
-		    CK_LIST_NEXT((inp), inp_list) :			\
+	if (hash < 0)
-		    CK_LIST_NEXT((inp), inp_hash_exact))
+		return (CK_LIST_FIRST(&ipi->ipi_list_unconn));
-#define	II_LOCK_ASSERT(inp, lock)					\
+	else if (hash <= ipi->ipi_hashmask)
-		rw_assert(&(inp)->inp_lock,				\
+		return (CK_LIST_FIRST(&ipi->ipi_hash_wild[hash]));
-		    (lock) == INPLOOKUP_RLOCKPCB ?  RA_RLOCKED : RA_WLOCKED )
+	else
+		return (CK_LIST_FIRST(
+		    &ipi->ipi_hash_exact[hash - ipi->ipi_hashmask - 1]));
+}
+
+static inline struct inpcb *
+ii_list_next(const struct inpcb_iterator *ii, struct inpcb *inp)
+{
+	if (ii->hash < 0)
+		return (CK_LIST_NEXT(inp, inp_unconn_list));
+	else if (ii->hash <= ii->ipi->ipi_hashmask)
+		return (CK_LIST_NEXT(inp, inp_hash_wild));
+	else
+		return (CK_LIST_NEXT(inp, inp_hash_exact));
+}
+
 struct inpcb *
 inp_next(struct inpcb_iterator *ii)
 {
 	const struct inpcbinfo *ipi = ii->ipi;
+	const int hashmax = (ipi->ipi_hashmask + 1) * 2;
 	inp_match_t *match = ii->match;
 	void *ctx = ii->ctx;
 	inp_lookup_t lock = ii->lock;
-	int hash = ii->hash;
 	struct inpcb *inp;
 
 	if (ii->inp == NULL) {		/* First call. */
+		if ((ii->hash = ii->mode) >= 0) {
+			/* Targeted iterators support only the exact hash. */
+			MPASS(ii->hash <= ipi->ipi_hashmask);
+			ii->hash += ipi->ipi_hashmask + 1;
+		}
 		smr_enter(ipi->ipi_smr);
-		/* This is unrolled CK_LIST_FOREACH(). */
+next_first:
-		for (inp = II_LIST_FIRST(ipi, hash);
+		/* This is unrolled CK_LIST_FOREACH() over different headers. */
+		for (inp = ii_list_first(ii);
 		    inp != NULL;
-		    inp = II_LIST_NEXT(inp, hash)) {
+		    inp = ii_list_next(ii, inp)) {
 			if (match != NULL && (match)(inp, ctx) == false)
 				continue;
 			if (__predict_true(_inp_smr_lock(inp, lock, INP_FREED)))
 				break;
 			else {
 				smr_enter(ipi->ipi_smr);
-				MPASS(inp != II_LIST_FIRST(ipi, hash));
+				MPASS(inp != ii_list_first(ii));
-				inp = II_LIST_FIRST(ipi, hash);
+				inp = ii_list_first(ii);
 				if (inp == NULL)
 					break;
 			}
 		}
 
-		if (inp == NULL)
+		if (inp == NULL) {
+			if (ii->mode == INP_ALL_LIST && ++ii->hash < hashmax)
+				goto next_first;
 			smr_exit(ipi->ipi_smr);
-		else
+		} else
 			ii->inp = inp;
 
 		return (inp);
@@ -1623,10 +1655,16 @@ inp_next(struct inpcb_iterator *ii)
 	smr_enter(ipi->ipi_smr);
 restart:
 	inp = ii->inp;
-	II_LOCK_ASSERT(inp, lock);
+	rw_assert(&inp->inp_lock,
+	    lock == INPLOOKUP_RLOCKPCB ? RA_RLOCKED : RA_WLOCKED);
 next:
-	inp = II_LIST_NEXT(inp, hash);
+	inp = ii_list_next(ii, inp);
 	if (inp == NULL) {
+		if (ii->mode == INP_ALL_LIST && ++ii->hash < hashmax) {
+			inp_unlock(ii->inp, lock);
+			ii->inp = NULL;
+			goto next_first;
+		}
 		smr_exit(ipi->ipi_smr);
 		goto found;
 	}
@@ -1799,10 +1837,11 @@ in_pcbfree(struct inpcb *inp)
 	 */
 	INP_HASH_WLOCK(pcbinfo);
 	if (inp->inp_flags & INP_INHASHLIST)
-		in_pcbremhash_locked(inp);
+		in_pcbremhash(inp);
+	else
+		CK_LIST_REMOVE(inp, inp_unconn_list);
 	inp->inp_gencnt = ++pcbinfo->ipi_gencnt;
 	pcbinfo->ipi_count--;
-	CK_LIST_REMOVE(inp, inp_list);
 	INP_HASH_WUNLOCK(pcbinfo);
 
 #ifdef RATELIMIT
@@ -1882,8 +1921,13 @@ in_pcbdrop(struct inpcb *inp)
 	INP_WLOCK_ASSERT(inp);
 
 	inp->inp_flags |= INP_DROPPED;
-	if (inp->inp_flags & INP_INHASHLIST)
+	if (inp->inp_flags & INP_INHASHLIST) {
+		INP_HASH_WLOCK(inp->inp_pcbinfo);
 		in_pcbremhash(inp);
+		CK_LIST_INSERT_HEAD(&inp->inp_pcbinfo->ipi_list_unconn, inp,
+		    inp_unconn_list);
+		INP_HASH_WUNLOCK(inp->inp_pcbinfo);
+	}
 }
 
 #ifdef INET
@@ -2693,6 +2737,8 @@ in_pcbinshash(struct inpcb *inp)
 		inp->inp_smr = SMR_SEQ_INVALID;
 	}
 
+	CK_LIST_REMOVE(inp, inp_unconn_list);
+
 	if (connected)
 		CK_LIST_INSERT_HEAD(pcbhash, inp, inp_hash_exact);
 	else {
@@ -2710,7 +2756,7 @@ in_pcbinshash(struct inpcb *inp)
 }
 
 void
-in_pcbremhash_locked(struct inpcb *inp)
+in_pcbremhash(struct inpcb *inp)
 {
 
 	INP_WLOCK_ASSERT(inp);
@@ -2737,14 +2783,6 @@ in_pcbremhash_locked(struct inpcb *inp)
 	inp->inp_flags &= ~INP_INHASHLIST;
 }
 
-static void
-in_pcbremhash(struct inpcb *inp)
-{
-	INP_HASH_WLOCK(inp->inp_pcbinfo);
-	in_pcbremhash_locked(inp);
-	INP_HASH_WUNLOCK(inp->inp_pcbinfo);
-}
-
 /*
  * Move PCB to the proper hash bucket when { faddr, fport } have  been
  * changed. NOTE: This does not handle the case of the lport changing (the
@@ -2787,15 +2825,12 @@ in_pcbrehash(struct inpcb *inp)
 	 * When rehashing, the caller must ensure that either the new or the old
 	 * foreign address was unspecified.
 	 */
-	if (connected)
-		CK_LIST_REMOVE(inp, inp_hash_wild);
-	else
-		CK_LIST_REMOVE(inp, inp_hash_exact);
-
 	if (connected) {
+		CK_LIST_REMOVE(inp, inp_hash_wild);
 		head = &pcbinfo->ipi_hash_exact[hash];
 		CK_LIST_INSERT_HEAD(head, inp, inp_hash_exact);
 	} else {
+		CK_LIST_REMOVE(inp, inp_hash_exact);
 		head = &pcbinfo->ipi_hash_wild[hash];
 		CK_LIST_INSERT_HEAD(head, inp, inp_hash_wild);
 	}

sys/netinet/in_pcb.h

@@ -321,10 +321,15 @@ CK_LIST_HEAD(inpcblbgrouphead, inpcblbgroup);
  * Almost all fields of struct inpcb are static after creation or protected by
  * a per-inpcb rwlock, inp_lock.
  *
- * A inpcb database is indexed by addresses/ports hash as well as list of
+ * A inpcb database consists of two hash tables: one for connected pcbs and the
- * all pcbs that belong to a certain proto. Database lookups or list traversals
+ * other for wildcard-bound pcbs.  The newborn pcbs reside on the unconnected
- * are be performed inside SMR section. Once desired PCB is found its own
+ * list.  Although a pcb can be on either of these three lists, we can't share
- * lock is to be obtained and SMR section exited.
+ * the linkage pointers because unlocked readers might be iterating over them.
+ * The only thing that can be unionized is the load-balance table and exact
+ * hash, as a pcb can never participate in both tables through its entire life
+ * time.  Database lookups or list traversals are to be performed inside SMR
+ * section.  Once desired PCB is found its own lock is to be obtained and SMR
+ * section exited.
  *
  * Key:
  * (c) - Constant after initialization
@@ -350,14 +355,13 @@ struct icmp6_filter;
 struct inpcbpolicy;
 struct m_snd_tag;
 struct inpcb {
-	/* Cache line #1 (amd64) */
 	union {
-		CK_LIST_ENTRY(inpcb) inp_hash_exact;	/* hash table linkage */
+		CK_LIST_ENTRY(inpcb)	inp_hash_exact;
-		LIST_ENTRY(inpcb) inp_lbgroup_list;	/* lb group list */
+		LIST_ENTRY(inpcb)	inp_lbgroup_list;
 	};
-	CK_LIST_ENTRY(inpcb) inp_hash_wild;	/* hash table linkage */
+	CK_LIST_ENTRY(inpcb)	inp_hash_wild;
+	CK_LIST_ENTRY(inpcb)	inp_unconn_list;
 	struct rwlock	inp_lock;
-	/* Cache line #2 (amd64) */
 #define	inp_start_zero	inp_refcount
 #define	inp_zero_size	(sizeof(struct inpcb) - \
 			    offsetof(struct inpcb, inp_start_zero))
@@ -412,7 +416,6 @@ struct inpcb {
 		struct route inp_route;
 		struct route_in6 inp_route6;
 	};
-	CK_LIST_ENTRY(inpcb) inp_list;	/* (r:e/w:p) all PCBs for proto */
 };
 
 #define	inp_vnet	inp_pcbinfo->ipi_vnet
@@ -431,7 +434,6 @@ struct inpcb {
  * (h) Locked by ipi_hash_lock
  */
 struct inpcbinfo {
-	struct inpcbhead	 ipi_listhead;		/* (r:e/w:h) */
 	u_int			 ipi_count;		/* (h) */
 
 	/*
@@ -460,6 +462,7 @@ struct inpcbinfo {
 	 * address, and "wild" holds the rest.
 	 */
 	struct mtx		 ipi_hash_lock;
+	struct inpcbhead	 ipi_list_unconn;	/* (r:e/w:h) */
 	struct inpcbhead 	*ipi_hash_exact;	/* (r:e/w:h) */
 	struct inpcbhead 	*ipi_hash_wild;		/* (r:e/w:h) */
 	u_long			 ipi_hashmask;		/* (c) */
@@ -670,14 +673,26 @@ int	sysctl_setsockopt(SYSCTL_HANDLER_ARGS, struct inpcbinfo *pcbinfo,
 	    int (*ctloutput_set)(struct inpcb *, struct sockopt *));
 #endif
 
+/*
+ * struct inpcb_iterator is located on the stack of a function that uses
+ * inp_next().  The caller shall initialize the const members before first
+ * invocation of inp_next().  After that, until the iterator finishes the
+ * caller is supposed to only read 'inp' until it reads NULL.  Some members
+ * have constness commented out for convenience of callers, that may reuse
+ * the iterator after it finishes.
+ * (c) - caller
+ * (n) - inp_next()
+ */
 typedef bool inp_match_t(const struct inpcb *, void *);
 struct inpcb_iterator {
 	const struct inpcbinfo	*ipi;
-	struct inpcb		*inp;
+	struct inpcb		*inp;	/* c:r, n:rw */
-	inp_match_t		*match;
+	/* const */ inp_match_t	*match;
-	void			*ctx;
+	/* const */ void	*ctx;
-	int			hash;
+	int			hash;	/* n:rw */
+	/* const */ int		mode;
 #define	INP_ALL_LIST		-1
+#define	INP_UNCONN_LIST		-2
 	const inp_lookup_t	lock;
 };
 
@@ -686,7 +701,7 @@ struct inpcb_iterator {
 	{						\
 		.ipi = (_ipi),				\
 		.lock = (_lock),			\
-		.hash = INP_ALL_LIST,			\
+		.mode = INP_ALL_LIST,			\
 		.match = (_match),			\
 		.ctx = (_ctx),				\
 	}
@@ -694,7 +709,7 @@ struct inpcb_iterator {
 	{						\
 		.ipi = (_ipi),				\
 		.lock = (_lock),			\
-		.hash = INP_ALL_LIST,			\
+		.mode = INP_ALL_LIST,			\
 	}
 
 struct inpcb *inp_next(struct inpcb_iterator *);

sys/netinet/in_pcb_var.h

@@ -57,7 +57,7 @@ struct inpcb *in_pcblookup_local(struct inpcbinfo *, struct in_addr, u_short,
 	    int, int, struct ucred *);
 int     in_pcbinshash(struct inpcb *);
 void    in_pcbrehash(struct inpcb *);
-void    in_pcbremhash_locked(struct inpcb *);
+void    in_pcbremhash(struct inpcb *);
 
 /*
  * Load balance groups used for the SO_REUSEPORT_LB socket option. Each group

sys/netinet/tcp_usrreq.c

@@ -2841,9 +2841,12 @@ db_print_bblog_state(int state)
 
 static void
 db_print_tcpcb(struct tcpcb *tp, const char *name, int indent, bool show_bblog,
-    bool show_inpcb)
+    bool show_inpcb, bool only_locked)
 {
 
+	if (only_locked && tp->t_inpcb.inp_lock.rw_lock == RW_UNLOCKED)
+		return;
+
 	db_print_indent(indent);
 	db_printf("%s at %p\n", name, tp);
 
@@ -2987,14 +2990,13 @@ DB_SHOW_COMMAND(tcpcb, db_show_tcpcb)
 	show_bblog = strchr(modif, 'b') != NULL;
 	show_inpcb = strchr(modif, 'i') != NULL;
 	tp = (struct tcpcb *)addr;
-	db_print_tcpcb(tp, "tcpcb", 0, show_bblog, show_inpcb);
+	db_print_tcpcb(tp, "tcpcb", 0, show_bblog, show_inpcb, false);
 }
 
 DB_SHOW_ALL_COMMAND(tcpcbs, db_show_all_tcpcbs)
 {
 	VNET_ITERATOR_DECL(vnet_iter);
 	struct inpcb *inp;
-	struct tcpcb *tp;
 	bool only_locked, show_bblog, show_inpcb;
 
 	only_locked = strchr(modif, 'l') != NULL;
@@ -3002,18 +3004,23 @@ DB_SHOW_ALL_COMMAND(tcpcbs, db_show_all_tcpcbs)
 	show_inpcb = strchr(modif, 'i') != NULL;
 	VNET_FOREACH(vnet_iter) {
 		CURVNET_SET(vnet_iter);
-		CK_LIST_FOREACH(inp, &V_tcbinfo.ipi_listhead, inp_list) {
+		for (u_int i = 0; i <= V_tcbinfo.ipi_porthashmask; i++)
-			if (only_locked &&
+			CK_LIST_FOREACH(inp, &V_tcbinfo.ipi_porthashbase[i],
-			    inp->inp_lock.rw_lock == RW_UNLOCKED)
+			    inp_portlist) {
-				continue;
+				db_print_tcpcb(intotcpcb(inp), "tcpcb", 0,
-			tp = intotcpcb(inp);
+				    show_bblog, show_inpcb, only_locked);
-			db_print_tcpcb(tp, "tcpcb", 0, show_bblog, show_inpcb);
+				if (db_pager_quit)
+					goto break_hash;
+			}
+break_hash:
+		CK_LIST_FOREACH(inp, &V_tcbinfo.ipi_list_unconn,
+		    inp_unconn_list) {
+			db_print_tcpcb(intotcpcb(inp), "tcpcb", 0,
+			    show_bblog, show_inpcb, only_locked);
 			if (db_pager_quit)
 				break;
 		}
 		CURVNET_RESTORE();
-		if (db_pager_quit)
-			break;
 	}
 }
 #endif

sys/netinet6/in6_pcb.c

@@ -508,7 +508,9 @@ in6_pcbdisconnect(struct inpcb *inp)
 	KASSERT(inp->inp_smr == SMR_SEQ_INVALID,
 	    ("%s: inp %p was already disconnected", __func__, inp));
 
-	in_pcbremhash_locked(inp);
+	in_pcbremhash(inp);
+	CK_LIST_INSERT_HEAD(&inp->inp_pcbinfo->ipi_list_unconn, inp,
+	    inp_unconn_list);
 
 	if ((inp->inp_socket->so_proto->pr_flags & PR_CONNREQUIRED) == 0) {
 		/* See the comment in in_pcbinshash(). */