Re: git: 289082428c76 - main - inpcb: add single peer local port caching
- In reply to: Gleb Smirnoff : "git: 289082428c76 - main - inpcb: add single peer local port caching"
- Go to: [ bottom of page ] [ top of archives ] [ this month ]
Date: Fri, 23 Oct 2026 18:50:45 UTC
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 <glebius@FreeBSD.org>
> AuthorDate: 2026-10-23 17:33:04 +0000
> Commit: Gleb Smirnoff <glebius@FreeBSD.org>
> 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.