makes sense to me, ok. maybe it's time to re-evaluate siphash?
> On 23 Jun 2023, at 05:50, Alexander Bluhm <alexander.bl...@gmx.net> wrote: > > Hi, > > I am working on a diff to run UDP input in parallel. Btrace kstack > analysis shows that SIP hash for PCB lookup is quite expensive. > When running in parallel we get lock contention on the PCB table > mutex. > > So it results in better performance to calculate the hash value > before taking the mutex. Code gets a bit more complicated as I > have to pass the value around. > > The hash secret has to be constant as hash calculation must not > depend on values protected by the table mutex. I see no security > benefit in reseeding when the hash table gets resized. > > Analysis also shows that asserting a rw_lock while holding a > mutex is a bit expensive. Just remove the netlock assert. > > ok? > > bluhm > > Index: netinet/in_pcb.c > =================================================================== > RCS file: /data/mirror/openbsd/cvs/src/sys/netinet/in_pcb.c,v > retrieving revision 1.276 > diff -u -p -r1.276 in_pcb.c > --- netinet/in_pcb.c 3 Oct 2022 16:43:52 -0000 1.276 > +++ netinet/in_pcb.c 22 Jun 2023 06:26:14 -0000 > @@ -121,15 +121,15 @@ struct baddynamicports rootonlyports; > struct pool inpcb_pool; > > void in_pcbhash_insert(struct inpcb *); > -struct inpcb *in_pcbhash_lookup(struct inpcbtable *, u_int, > +struct inpcb *in_pcbhash_lookup(struct inpcbtable *, uint64_t, u_int, > const struct in_addr *, u_short, const struct in_addr *, u_short); > int in_pcbresize(struct inpcbtable *, int); > > #define INPCBHASH_LOADFACTOR(_x) (((_x) * 3) / 4) > > -struct inpcbhead *in_pcbhash(struct inpcbtable *, u_int, > +uint64_t in_pcbhash(struct inpcbtable *, u_int, > const struct in_addr *, u_short, const struct in_addr *, u_short); > -struct inpcbhead *in_pcblhash(struct inpcbtable *, u_int, u_short); > +uint64_t in_pcblhash(struct inpcbtable *, u_int, u_short); > > /* > * in_pcb is used for inet and inet6. in6_pcb only contains special > @@ -142,7 +142,7 @@ in_init(void) > IPL_SOFTNET, 0, "inpcb", NULL); > } > > -struct inpcbhead * > +uint64_t > in_pcbhash(struct inpcbtable *table, u_int rdomain, > const struct in_addr *faddr, u_short fport, > const struct in_addr *laddr, u_short lport) > @@ -156,11 +156,10 @@ in_pcbhash(struct inpcbtable *table, u_i > SipHash24_Update(&ctx, &fport, sizeof(fport)); > SipHash24_Update(&ctx, laddr, sizeof(*laddr)); > SipHash24_Update(&ctx, &lport, sizeof(lport)); > - > - return (&table->inpt_hashtbl[SipHash24_End(&ctx) & table->inpt_mask]); > + return SipHash24_End(&ctx); > } > > -struct inpcbhead * > +uint64_t > in_pcblhash(struct inpcbtable *table, u_int rdomain, u_short lport) > { > SIPHASH_CTX ctx; > @@ -169,8 +168,7 @@ in_pcblhash(struct inpcbtable *table, u_ > SipHash24_Init(&ctx, &table->inpt_lkey); > SipHash24_Update(&ctx, &nrdom, sizeof(nrdom)); > SipHash24_Update(&ctx, &lport, sizeof(lport)); > - > - return (&table->inpt_lhashtbl[SipHash24_End(&ctx) & table->inpt_lmask]); > + return SipHash24_End(&ctx); > } > > void > @@ -816,11 +814,14 @@ in_pcblookup_local(struct inpcbtable *ta > struct in6_addr *laddr6 = (struct in6_addr *)laddrp; > #endif > struct inpcbhead *head; > + uint64_t lhash; > u_int rdomain; > > rdomain = rtable_l2(rtable); > + lhash = in_pcblhash(table, rdomain, lport); > + > mtx_enter(&table->inpt_mtx); > - head = in_pcblhash(table, rdomain, lport); > + head = &table->inpt_lhashtbl[lhash & table->inpt_lmask]; > LIST_FOREACH(inp, head, inp_lhash) { > if (rtable_l2(inp->inp_rtableid) != rdomain) > continue; > @@ -1056,37 +1057,38 @@ in_pcbhash_insert(struct inpcb *inp) > { > struct inpcbtable *table = inp->inp_table; > struct inpcbhead *head; > + uint64_t hash, lhash; > > - NET_ASSERT_LOCKED(); > MUTEX_ASSERT_LOCKED(&table->inpt_mtx); > > - head = in_pcblhash(table, inp->inp_rtableid, inp->inp_lport); > + lhash = in_pcblhash(table, inp->inp_rtableid, inp->inp_lport); > + head = &table->inpt_lhashtbl[lhash & table->inpt_lmask]; > LIST_INSERT_HEAD(head, inp, inp_lhash); > #ifdef INET6 > if (inp->inp_flags & INP_IPV6) > - head = in6_pcbhash(table, rtable_l2(inp->inp_rtableid), > + hash = in6_pcbhash(table, rtable_l2(inp->inp_rtableid), > &inp->inp_faddr6, inp->inp_fport, > &inp->inp_laddr6, inp->inp_lport); > else > #endif /* INET6 */ > - head = in_pcbhash(table, rtable_l2(inp->inp_rtableid), > + hash = in_pcbhash(table, rtable_l2(inp->inp_rtableid), > &inp->inp_faddr, inp->inp_fport, > &inp->inp_laddr, inp->inp_lport); > + head = &table->inpt_hashtbl[hash & table->inpt_mask]; > LIST_INSERT_HEAD(head, inp, inp_hash); > } > > struct inpcb * > -in_pcbhash_lookup(struct inpcbtable *table, u_int rdomain, > +in_pcbhash_lookup(struct inpcbtable *table, uint64_t hash, u_int rdomain, > const struct in_addr *faddr, u_short fport, > const struct in_addr *laddr, u_short lport) > { > struct inpcbhead *head; > struct inpcb *inp; > > - NET_ASSERT_LOCKED(); > MUTEX_ASSERT_LOCKED(&table->inpt_mtx); > > - head = in_pcbhash(table, rdomain, faddr, fport, laddr, lport); > + head = &table->inpt_hashtbl[hash & table->inpt_mask]; > LIST_FOREACH(inp, head, inp_hash) { > #ifdef INET6 > if (ISSET(inp->inp_flags, INP_IPV6)) > @@ -1140,8 +1142,6 @@ in_pcbresize(struct inpcbtable *table, i > table->inpt_mask = nmask; > table->inpt_lmask = nlmask; > table->inpt_size = hashsize; > - arc4random_buf(&table->inpt_key, sizeof(table->inpt_key)); > - arc4random_buf(&table->inpt_lkey, sizeof(table->inpt_lkey)); > > TAILQ_FOREACH(inp, &table->inpt_queue, inp_queue) { > LIST_REMOVE(inp, inp_lhash); > @@ -1172,13 +1172,18 @@ in_pcblookup(struct inpcbtable *table, s > u_int fport, struct in_addr laddr, u_int lport, u_int rtable) > { > struct inpcb *inp; > + uint64_t hash; > u_int rdomain; > > rdomain = rtable_l2(rtable); > + hash = in_pcbhash(table, rdomain, &faddr, fport, &laddr, lport); > + > mtx_enter(&table->inpt_mtx); > - inp = in_pcbhash_lookup(table, rdomain, &faddr, fport, &laddr, lport); > + inp = in_pcbhash_lookup(table, hash, rdomain, > + &faddr, fport, &laddr, lport); > in_pcbref(inp); > mtx_leave(&table->inpt_mtx); > + > #ifdef DIAGNOSTIC > if (inp == NULL && in_pcbnotifymiss) { > printf("%s: faddr=%08x fport=%d laddr=%08x lport=%d rdom=%u\n", > @@ -1202,6 +1207,7 @@ in_pcblookup_listen(struct inpcbtable *t > { > const struct in_addr *key1, *key2; > struct inpcb *inp; > + uint64_t hash; > u_int16_t lport = lport_arg; > u_int rdomain; > > @@ -1239,14 +1245,20 @@ in_pcblookup_listen(struct inpcbtable *t > #endif > > rdomain = rtable_l2(rtable); > + hash = in_pcbhash(table, rdomain, &zeroin_addr, 0, key1, lport); > + > mtx_enter(&table->inpt_mtx); > - inp = in_pcbhash_lookup(table, rdomain, &zeroin_addr, 0, key1, lport); > + inp = in_pcbhash_lookup(table, hash, rdomain, > + &zeroin_addr, 0, key1, lport); > if (inp == NULL && key1->s_addr != key2->s_addr) { > - inp = in_pcbhash_lookup(table, rdomain, > + hash = in_pcbhash(table, rdomain, > + &zeroin_addr, 0, key2, lport); > + inp = in_pcbhash_lookup(table, hash, rdomain, > &zeroin_addr, 0, key2, lport); > } > in_pcbref(inp); > mtx_leave(&table->inpt_mtx); > + > #ifdef DIAGNOSTIC > if (inp == NULL && in_pcbnotifymiss) { > printf("%s: laddr=%08x lport=%d rdom=%u\n", > Index: netinet/in_pcb.h > =================================================================== > RCS file: /data/mirror/openbsd/cvs/src/sys/netinet/in_pcb.h,v > retrieving revision 1.135 > diff -u -p -r1.135 in_pcb.h > --- netinet/in_pcb.h 3 Oct 2022 16:43:52 -0000 1.135 > +++ netinet/in_pcb.h 21 Jun 2023 18:43:30 -0000 > @@ -172,7 +172,7 @@ struct inpcbtable { > TAILQ_HEAD(inpthead, inpcb) inpt_queue; /* [t] inet PCB queue */ > struct inpcbhead *inpt_hashtbl; /* [t] local and foreign hash */ > struct inpcbhead *inpt_lhashtbl; /* [t] local port hash */ > - SIPHASH_KEY inpt_key, inpt_lkey; /* [t] secrets for hashes */ > + SIPHASH_KEY inpt_key, inpt_lkey; /* [I] secrets for hashes */ > u_long inpt_mask, inpt_lmask; /* [t] hash masks */ > int inpt_count, inpt_size; /* [t] queue count, hash size */ > }; > @@ -294,8 +294,7 @@ struct inpcb * > in_pcblookup_listen(struct inpcbtable *, struct in_addr, u_int, > struct mbuf *, u_int); > #ifdef INET6 > -struct inpcbhead * > - in6_pcbhash(struct inpcbtable *, u_int, const struct in6_addr *, > +uint64_t in6_pcbhash(struct inpcbtable *, u_int, const struct in6_addr *, > u_short, const struct in6_addr *, u_short); > struct inpcb * > in6_pcblookup(struct inpcbtable *, const struct in6_addr *, > Index: netinet6/in6_pcb.c > =================================================================== > RCS file: /data/mirror/openbsd/cvs/src/sys/netinet6/in6_pcb.c,v > retrieving revision 1.123 > diff -u -p -r1.123 in6_pcb.c > --- netinet6/in6_pcb.c 3 Sep 2022 22:43:38 -0000 1.123 > +++ netinet6/in6_pcb.c 21 Jun 2023 18:43:30 -0000 > @@ -126,10 +126,10 @@ > > const struct in6_addr zeroin6_addr; > > -struct inpcb *in6_pcbhash_lookup(struct inpcbtable *, u_int, > +struct inpcb *in6_pcbhash_lookup(struct inpcbtable *, uint64_t, u_int, > const struct in6_addr *, u_short, const struct in6_addr *, u_short); > > -struct inpcbhead * > +uint64_t > in6_pcbhash(struct inpcbtable *table, u_int rdomain, > const struct in6_addr *faddr, u_short fport, > const struct in6_addr *laddr, u_short lport) > @@ -143,8 +143,7 @@ in6_pcbhash(struct inpcbtable *table, u_ > SipHash24_Update(&ctx, &fport, sizeof(fport)); > SipHash24_Update(&ctx, laddr, sizeof(*laddr)); > SipHash24_Update(&ctx, &lport, sizeof(lport)); > - > - return (&table->inpt_hashtbl[SipHash24_End(&ctx) & table->inpt_mask]); > + return SipHash24_End(&ctx); > } > > int > @@ -541,7 +540,7 @@ in6_pcbnotify(struct inpcbtable *table, > } > > struct inpcb * > -in6_pcbhash_lookup(struct inpcbtable *table, u_int rdomain, > +in6_pcbhash_lookup(struct inpcbtable *table, uint64_t hash, u_int rdomain, > const struct in6_addr *faddr, u_short fport, > const struct in6_addr *laddr, u_short lport) > { > @@ -551,7 +550,7 @@ in6_pcbhash_lookup(struct inpcbtable *ta > NET_ASSERT_LOCKED(); > MUTEX_ASSERT_LOCKED(&table->inpt_mtx); > > - head = in6_pcbhash(table, rdomain, faddr, fport, laddr, lport); > + head = &table->inpt_hashtbl[hash & table->inpt_mask]; > LIST_FOREACH(inp, head, inp_hash) { > if (!ISSET(inp->inp_flags, INP_IPV6)) > continue; > @@ -581,13 +580,18 @@ in6_pcblookup(struct inpcbtable *table, > u_int fport, const struct in6_addr *laddr, u_int lport, u_int rtable) > { > struct inpcb *inp; > + uint64_t hash; > u_int rdomain; > > rdomain = rtable_l2(rtable); > + hash = in6_pcbhash(table, rdomain, faddr, fport, laddr, lport); > + > mtx_enter(&table->inpt_mtx); > - inp = in6_pcbhash_lookup(table, rdomain, faddr, fport, laddr, lport); > + inp = in6_pcbhash_lookup(table, hash, rdomain, > + faddr, fport, laddr, lport); > in_pcbref(inp); > mtx_leave(&table->inpt_mtx); > + > #ifdef DIAGNOSTIC > if (inp == NULL && in_pcbnotifymiss) { > printf("%s: faddr= fport=%d laddr= lport=%d rdom=%u\n", > @@ -603,6 +607,7 @@ in6_pcblookup_listen(struct inpcbtable * > { > const struct in6_addr *key1, *key2; > struct inpcb *inp; > + uint64_t hash; > u_int rdomain; > > key1 = laddr; > @@ -636,14 +641,20 @@ in6_pcblookup_listen(struct inpcbtable * > #endif > > rdomain = rtable_l2(rtable); > + hash = in6_pcbhash(table, rdomain, &zeroin6_addr, 0, key1, lport); > + > mtx_enter(&table->inpt_mtx); > - inp = in6_pcbhash_lookup(table, rdomain, &zeroin6_addr, 0, key1, lport); > + inp = in6_pcbhash_lookup(table, hash, rdomain, > + &zeroin6_addr, 0, key1, lport); > if (inp == NULL && ! IN6_ARE_ADDR_EQUAL(key1, key2)) { > - inp = in6_pcbhash_lookup(table, rdomain, > + hash = in6_pcbhash(table, rdomain, > + &zeroin6_addr, 0, key2, lport); > + inp = in6_pcbhash_lookup(table, hash, rdomain, > &zeroin6_addr, 0, key2, lport); > } > in_pcbref(inp); > mtx_leave(&table->inpt_mtx); > + > #ifdef DIAGNOSTIC > if (inp == NULL && in_pcbnotifymiss) { > printf("%s: laddr= lport=%d rdom=%u\n", >