From: Vitaliy Makkoveev <mvs@openbsd.org>
Subject: Re: Please test: shared solock for all intet sockets within knote(9) routines
To: Alexander Bluhm <alexander.bluhm@gmx.net>
Cc: tech@openbsd.org
Date: Mon, 29 Jan 2024 16:20:02 +0300

On Mon, Jan 29, 2024 at 02:04:57PM +0100, Alexander Bluhm wrote:
> On Sun, Jan 28, 2024 at 10:25:14PM +0300, Vitaliy Makkoveev wrote:
> > On Sun, Jan 28, 2024 at 12:54:24PM +0100, Alexander Bluhm wrote:
> > > On Fri, Jan 26, 2024 at 07:59:02PM +0300, Vitaliy Makkoveev wrote:
> > > > Not for commit yet, but could be interesting. The idea is the same as
> > > > for PCB layer: take shared netlock and exclusive `so_lock' to serialize
> > > > access to the socket. The tcp(4) sockets permit this because knote(9)
> > > > routines do read-only access to sockets.
> > > 
> > > Passes regress with witness.
> > > 
> > > OK bluhm@
> > > 
> > > > Should provide some additional performance.
> > > 
> > > My setup is currently busy with some other test run.
> > > Can do performance comparison in a few days.
> > 
> > Thanks for attention. Unfortunately current soassertlocked() is very
> > dumb for inet case:
> > 
> > soassertlocked(struct socket *so)
> > {       
> >         switch (so->so_proto->pr_domain->dom_family) {
> >         case PF_INET:
> >         case PF_INET6:
> >                 NET_ASSERT_LOCKED();
> >                 break;
> >         default:
> >                 rw_assert_wrlock(&so->so_lock);
> >                 break;
> >         }
> > }
> > 
> > Could we update it first and check `so_lock' state for shared netlock
> > case? Since we already use solock_shared(), I want to raise assertion
> > instead of panic.
> 
> A month ago I tired a similar diff and it paniced at multiple places.
> So splassert_fail() is much safer than panic.  The places with wrong
> assertions that my diff identified are: sorwakeup(), sowwakeup(),
> klist_soassertlk(), sowakeup(), sbspace().
> 
> When I saw you sofilt_lock() diff, I was hoping that it could fix
> some of them.  So in what order should we proceed?
> 
> 1. Test soassertlocked() diff.
> 2. Commit sofilt_lock() and hope for less soassertlocked() findings.
> 3. Commit soassertlocked() and watch for splassert_fail() messages.
> 4. Test sofilt_lock() diff together with soassertlocked() diff.
> 
> I am currently doing 1.  I think 2 should be the next step.  But
> maybe another order makes more sense.  How confident are you with
> sofilt_lock() diff?
> 

I'm confident with sofilt_lock() diff, but I want to commit
soassertlocked() diff first. The current implementation is inconsistent
for soclock_shared() so it's better to rework it before it's expansion.
I intentionally used splassert_fail() instead of rw_assert_wrlock() to
made this commit safe.

So, I want to commit soassertlocked() right now. sofilt_lock() diff
could be pushed to the snaps, so we could test them together.