From nobody Fri Oct 23 18:50:45 2026 X-Original-To: dev-commits-src-main@mlmmj.nyi.freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [IPv6:2610:1c1:1:606c::19:1]) by mlmmj.nyi.freebsd.org (Postfix) with ESMTP id 4jBBtJ1mtnz6wDh1; Fri, 23 Oct 2026 18:50:48 +0000 (UTC) (envelope-from markj@freebsd.org) Received: from smtp.freebsd.org (smtp.freebsd.org [96.47.72.83]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange x25519 server-signature RSA-PSS (4096 bits) server-digest SHA256) (No client certificate requested) by mx1.freebsd.org (Postfix) with ESMTPS id 4jBBtJ0pVZz4L6c; Fri, 23 Oct 2026 18:50:48 +0000 (UTC) (envelope-from markj@freebsd.org) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=freebsd.org; s=dkim; t=1792781448; h=from:from:reply-to:subject:subject:date:date:message-id:message-id: to:to:cc:cc:mime-version:mime-version:content-type:content-type: in-reply-to:in-reply-to:references:references; bh=TzEGZtCVrH+6j2WdWCDXI8HLt1u/SJCBABZnn2xhkzY=; b=UqiwJQI2niJlH5esj9FBwcZG9TTS00KeEWS/efGcPfuH5JdwO6+kKUxjD2/RGAeyoOuW53 CInR9oPjM5j+hxl5eq+roSGItzeR35qkfUjbqy3sW+VrKJtXuiWcnQa8ENRztw7GOFzE04 csPcFM80UBIii2dRtkUrkTvxDFoXLIswEtFjG4nIvOzi6qlvVom6TQfJXD2KqfVGhWbJv0 6EdYdIjA1D89Rsc+0b5MMzeYCfBgzrUsMboI1cknb8w0meRUmx5MFeBrD+cAZSyeRy38Ya 0YXxDMxILT9UOGa7bcK6R2S3WxJCDCugbkkm03YEdYuFMsoQgNgj4fZB0nEfjg== ARC-Seal: i=1; a=rsa-sha256; d=freebsd.org; s=dkim; cv=none; t=1792781448; b=oedSc8m83pFpzM1oFfofM4NQubkGa14B7HlFk0+LFgWUKJvVU+1GzNiT9wtC66EAptoCCb Ug5C+jRuJlj6PSjVuDWfIvXjBPfJd1d4jZ4gup8zNXyKSMtvDDpZl+hAVwZpJ22C94IfiZ +CxsBxitwv42TeczUmH0k24JLVIeO1OBIIM44dDQU1BwBagdLkXaS+A+EeePS7paKJjQrr fZvbiDGR+eZAlT/gMkk1p4mOV8CVpjY4iLeoLO3aFnTcsdZbg/+woWBUrTrIOnen6UZ8+o 478nuGqzDaF2bgxNqpxsQWRVTuGAvoBrtc1EtiILMsCFpuW7+CmRyaCaVdIVpw== ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=freebsd.org; s=dkim; t=1792781448; h=from:from:reply-to:subject:subject:date:date:message-id:message-id: to:to:cc:cc:mime-version:mime-version:content-type:content-type: in-reply-to:in-reply-to:references:references; bh=TzEGZtCVrH+6j2WdWCDXI8HLt1u/SJCBABZnn2xhkzY=; b=eFhhs7hS1AGTXYzNwCqbHaA0lUS7FevVYI+GETzFe5WnS/U+X2N1Mlp6JEDLecT6xGyakh 8kxo87gEQ+TEV1p/KrZAUdJrTV4eOg2Q4aH9C4ESwNcB6x90Qsy3NbqfBefR4ex6/JzGlk qo4bJAv1aXlHsIM3ujFOXemE/SGc7Vj9co0gGf+kV+eD14KTYWTTUwaOqo64Ptsa+pyaIW zqcUwyYNkS2LU0AYbuieS7G+MY7asNAZjgUJ6OzWMbigkhYcb3KRr0WehdRwWVjUSOYINP Gv7oX+PxlyIVvpESIQJSihEzJ5ce0jY19W39Xfr7eVDWaZM97QcAGNrxz8CI6Q== ARC-Authentication-Results: i=1; mx1.freebsd.org; none Received: from nuc (198-84-224-249.cpe.teksavvy.com [198.84.224.249]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange x25519 server-signature RSA-PSS (4096 bits) server-digest SHA256) (Client did not present a certificate) (Authenticated sender: markj) by smtp.freebsd.org (Postfix) with ESMTPSA id 4jBBtH5pCdz1CJH; Fri, 23 Oct 2026 18:50:47 +0000 (UTC) (envelope-from markj@freebsd.org) Date: Fri, 23 Oct 2026 14:50:45 -0400 From: Mark Johnston To: Gleb Smirnoff Cc: src-committers@freebsd.org, dev-commits-src-all@freebsd.org, dev-commits-src-main@freebsd.org Subject: Re: git: 289082428c76 - main - inpcb: add single peer local port caching Message-ID: References: <6adb9ad8.3fc51.157533ab@gitrepo.freebsd.org> List-Archive: https://lists.freebsd.org/archives/dev-commits-src-main X-BeenThere: dev-commits-src-main@freebsd.org Sender: owner-dev-commits-src-main@FreeBSD.org List-Id: Commit messages for the main branch of the src repository List-Post: List-Help: List-Subscribe: List-Unsubscribe: List-Owner: Precedence: list MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <6adb9ad8.3fc51.157533ab@gitrepo.freebsd.org> On Fri, Oct 23, 2026 at 05:35:20PM +0000, Gleb Smirnoff wrote: > The branch main has been updated by glebius: > > URL: https://cgit.FreeBSD.org/src/commit/?id=289082428c764ed282623cc6f7ee9b7448fddad1 > > commit 289082428c764ed282623cc6f7ee9b7448fddad1 > Author: Gleb Smirnoff > AuthorDate: 2026-10-23 17:33:04 +0000 > Commit: Gleb Smirnoff > CommitDate: 2026-10-23 17:33:04 +0000 > > inpcb: add single peer local port caching > > A typical caching proxy problem is port allocation to backends. We > usually have a few of them, with thousands connections to each. Upon > creation of a new connection we need to find a new unique local port for > the peer:port. This is achieved by linear search in in_pcb_lport_dest(). > The closer we get to the port exhaustion the more expensive this loop is. > It needs to be executed tens thousands of times to find a port or fail. > > Address the problem by adding advisory negative cache that is lazily > allocated upon first conflict. Once the cache is in action we skip trying > those ports that are already marked in the cache. During the search we > may encounter a cache allocated by other lookup. In this case the caches > are merged into one. The expectation is that under a stable heavy load we > are going to stabilize at a very low number of allocated caches, ideally > one per peer:port. The cache is a bitset of IPPORT_MAX, which makes it 8k. > Unfortunately metadata pushes allocation size to 16k UMA bucket. However, > the expectation is that we are going to have a very few allocated, thus > current decision is to not create a separate UMA zone. > > Differential Revision: https://reviews.freebsd.org/D58290 > --- > sys/netinet/in_pcb.c | 244 ++++++++++++++++++++++++++++++++++++++++++++++- > sys/netinet/in_pcb.h | 2 +- > sys/netinet/in_pcb_var.h | 1 + > sys/netinet6/in6_pcb.c | 1 + > 4 files changed, 246 insertions(+), 2 deletions(-) > > diff --git a/sys/netinet/in_pcb.c b/sys/netinet/in_pcb.c > index d72fb4cce587..35eb4614e2c6 100644 > --- a/sys/netinet/in_pcb.c > +++ b/sys/netinet/in_pcb.c > @@ -726,6 +726,226 @@ out: > #endif > } > > +VNET_DEFINE_STATIC(bool, lport_cache_enable) = true; > +#define V_lport_cache_enable VNET(lport_cache_enable) > +SYSCTL_BOOL(_net_inet_ip_portrange, OID_AUTO, lport_cache, > + CTLFLAG_VNET | CTLFLAG_RW, &VNET_NAME(lport_cache_enable), 0, > + "Enable local port cache for peers that have many outgoing connections to"); > + > +static MALLOC_DEFINE(M_LPORTS, "inp_lports", "inpcb lport cache"); > +#define PORTBITS (IPPORT_MAX + 1) > +BITSET_DEFINE(lportbits, PORTBITS); > +struct lport_cache { > + struct lportbits lports; > + struct lport_cache *successor; > + uint64_t age; > + u_int refcount; > +#ifdef INVARIANTS > + union in_dependaddr faddr; > + uint16_t fport; > +#endif > +}; > + > +/* > + * Remove one reference from cache, potentially freeing a chain. > + */ > +static struct lport_cache * > +lport_cache_release(struct lport_cache *cache) > +{ > + > + while (cache != NULL && refcount_release(&cache->refcount)) { > + struct lport_cache *next; > + > + next = atomic_load_ptr(&cache->successor); > + free(cache, M_LPORTS); > + cache = next; > + } > + > + return (cache); > +} > + > +static inline void > +lport_acquire(struct lportbits *lports, uint16_t port) > +{ > + BIT_SET_ATOMIC(PORTBITS, port, lports); > +} > + > +static inline void > +lport_release(struct lportbits *lports, uint16_t port) > +{ > + BIT_CLR_ATOMIC(PORTBITS, port, lports); > +} > + > +static inline bool > +lport_check(struct lportbits *lports, uint16_t port) > +{ > + return (BIT_ISSET(PORTBITS, port, lports)); > +} > + > +static inline void > +lport_merge(struct lportbits *dst, struct lportbits *src) > +{ > + BIT_OR_ATOMIC(PORTBITS, dst, src); > +} > + > +/* > + * Return inp->inp_lport_cache, potentially promoting it. > + */ > +static struct lport_cache * > +in_pcb_lport_cache(struct inpcb *inp) > +{ > + struct lport_cache *cache, *next; > + > + INP_WLOCK_ASSERT(inp); > + > + if ((cache = inp->inp_lport_cache) != NULL) > + while ((next = atomic_load_ptr(&cache->successor)) != NULL) { > + MPASS(next != cache); > + inp->inp_lport_cache = next; > + if (refcount_release(&cache->refcount)) > + free(cache, M_LPORTS); > + else > + refcount_acquire(&next->refcount); This is backwards. You can't acquire a reference on cache->successor /after/ releasing your reference to "cache". The reference to "cache" is potentially the only thing keeping the successor cache alive. > + cache = next; > + } > + > + return (inp->inp_lport_cache); > +} > + > +static void > +in_pcb_cache_lport(struct inpcb *new, struct inpcb *old) > +{ > + struct lport_cache *cache; > + > + INP_WLOCK_ASSERT(new); > + INP_WLOCK_ASSERT(old); > + > + /* > + * When conflicted with a wildcard bound inpcb, we could actually cache > + * the port number, but do not attach the cache to the wildcard inpcb. > + * This would complicate this function, making each iteration more > + * expensive. With the expectation that not so many ports in the > + * anonymous range are occupied by wildcard inpcbs, usually zero, seems > + * it would be more effective to not cover this case and have just tiny > + * increase in attempts rather then making each attempt heavier. > + */ > +#ifdef INET6 > + if (old->inp_vflag & INP_IPV6) { > + if (IN6_IS_ADDR_UNSPECIFIED(&old->in6p_faddr)) > + goto out; > + } else > +#endif > + if (in_nullhost(old->inp_faddr)) > + goto out; > + > + (void)in_pcb_lport_cache(new); > + (void)in_pcb_lport_cache(old); > + > + if (new->inp_lport_cache == NULL && > + old->inp_lport_cache == NULL) { > + static uint64_t age; > + > + cache = malloc(sizeof(*cache), M_LPORTS, M_ZERO | M_NOWAIT); > + if (__predict_false(cache == NULL)) > + goto out; > + refcount_init(&cache->refcount, 2); > + cache->age = atomic_fetchadd_64(&age, 1); > +#ifdef INVARIANTS > + cache->faddr = old->inp_inc.inc_ie.ie_dependfaddr; > + cache->fport = old->inp_fport; > +#endif > + lport_acquire(&cache->lports, ntohs(old->inp_lport)); > + new->inp_lport_cache = old->inp_lport_cache = cache; > + } else if (new->inp_lport_cache == old->inp_lport_cache) { > + /* Race: old has grabbed the port after our lport_check(). */ > + } else if (new->inp_lport_cache == NULL) { > + cache = old->inp_lport_cache; > + MPASS(memcmp(&old->inp_inc.inc_ie.ie_dependfaddr, > + &cache->faddr, sizeof(union in_dependaddr)) == 0); > + MPASS(old->inp_fport == cache->fport); > + refcount_acquire(&cache->refcount); > + new->inp_lport_cache = cache; > + } else if (old->inp_lport_cache == NULL) { > + cache = new->inp_lport_cache; > + MPASS(memcmp(&old->inp_inc.inc_ie.ie_dependfaddr, > + &cache->faddr, sizeof(union in_dependaddr)) == 0); > + MPASS(old->inp_fport == cache->fport); > + lport_acquire(&cache->lports, ntohs(old->inp_lport)); > + refcount_acquire(&cache->refcount); > + old->inp_lport_cache = cache; > + } else { > + struct lport_cache *second; > + > + /* > + * We can't use caches refcounts here, as it makes it possible > + * for two competing caches to be linked against each other by > + * two parallel threads. We use age speculating that an older > + * cache usually holds more bits. > + */ > + if (new->inp_lport_cache->age < old->inp_lport_cache->age) { > + cache = new->inp_lport_cache; > + second = old->inp_lport_cache; > + MPASS(cache != second); > + old->inp_lport_cache = cache; > + } else { > + cache = old->inp_lport_cache; > + second = new->inp_lport_cache; > + MPASS(cache != second); > + new->inp_lport_cache = cache; > + } > + MPASS(memcmp(&old->inp_inc.inc_ie.ie_dependfaddr, > + &cache->faddr, sizeof(union in_dependaddr)) == 0); > + MPASS(old->inp_fport == cache->fport); > + MPASS(memcmp(&second->faddr, &cache->faddr, > + sizeof(union in_dependaddr)) == 0); > + MPASS(second->fport == cache->fport); > + lport_merge(&cache->lports, &second->lports); > + refcount_acquire(&cache->refcount); > + /* > + * Now both inpcbs and winning cache are all set, and tricky > + * part is to dereference 'second'. Three possibilities here: > + * 1) our reference was the last and we free it 2) we were not > + * the last so make it point at the winner 3) we lost the race > + * to other thread that was doing the same. > + */ > + if (lport_cache_release(second) == second && > + atomic_cmpset_ptr((uintptr_t *)&second->successor, If we are not releasing the last reference, then it's possible that another thread will release the last reference and free the object before this atomic_cmpset is executed. > + (uintptr_t)NULL, (uintptr_t)cache)) > + refcount_acquire(&cache->refcount); > + } > +out: > + INP_WUNLOCK(old); > +} > + > +void > +in_pcb_lport_cache_free(struct inpcb *inp) > +{ > + struct lport_cache *cache = inp->inp_lport_cache; > + > + INP_WLOCK_ASSERT(inp); > + > + /* Free possible chain of caches, where we were the last reference. */ > + while (cache != NULL && refcount_release(&cache->refcount)) { > + struct lport_cache *next; > + > + MPASS(memcmp(&inp->inp_inc.inc_ie.ie_dependfaddr, > + &cache->faddr, sizeof(union in_dependaddr)) == 0); > + MPASS(inp->inp_fport == cache->fport); > + next = atomic_load_ptr(&cache->successor); > + free(cache, M_LPORTS); > + cache = next; > + } > + /* Walk through the remaining chain and clear our bit. */ > + while (cache != NULL) { > + MPASS(memcmp(&inp->inp_inc.inc_ie.ie_dependfaddr, > + &cache->faddr, sizeof(union in_dependaddr)) == 0); > + MPASS(inp->inp_fport == cache->fport); > + lport_release(&cache->lports, ntohs(inp->inp_lport)); > + cache = atomic_load_ptr(&cache->successor); > + } Here we call refcount_release(), and then access the object that the refcount is protecting. This is backwards. I understand that you waited a while for review, but you last pinged me on Monday; it's been an unusually busy week and phabricator has been unavailable for a couple of days now, so I wasn't able to comment with the < 2 days of runway I had. I think the change should be reverted, assuming I am not completely wrong about the races above.