On Thu, Aug 04, 2022 at 01:42:48PM +0200, Alexander Bluhm wrote:
> On Thu, Aug 04, 2022 at 02:18:49AM +0300, Vitaliy Makkoveev wrote:
> > Also, I like to have exclusive layer locks like `tcp_lock???,
> > `udp_lock???, etc.. And take them with shared netlock held as the
> > first step of inet sockets unlocking.
>
> With PRU_LOCK and PRU_UNLOCK each layer can decide itself which
> kind is appropriate. For divert packet per PCB lock is trivial.
> For loops delivering to all sockets like UDP multicast or RAW
> sockets, a mutex per layer an easier start.
>
> > > sblock()/sbunlock() relie on exclusive socket or layer lock,
> > > which protects `sb_flags??? modification. This diff breaks them,
> > > because nothing stops concurrent soreceive() threads to set
> > > SB_LOCK flag and and be sure that socket buffer is locked.
>
> Either sb_flags is protected by the exclusice netlock or by a shared
> netlock and PCB mutex. So only one thread can access them. Right?
>
Yes, you are right, I missed the PRU_LOCK/PRU_UNLOCK calls.
With your diff, sbwait() releases only netlock and leaves `inp_mtx'
locked. So when we enter sbwait() in the soreceive() path, the
concurrent sbappendaddr() thread will spin instead of append mbut to
`so_rcv'.
It seems sbwait() should release `inp_mtx' and keep shared netlock
locked.
@@ -907,7 +907,7 @@ restart:
sbunlock(so, &so->so_rcv);
error = sbwait(so, &so->so_rcv);
if (error) {
- sounlock(so);
+ sounlock_shared(so);
return (error);
}
@@ -1140,7 +1140,7 @@ dontblock:
error = sbwait(so, &so->so_rcv);
if (error) {
sbunlock(so, &so->so_rcv);
- sounlock(so);
+ sounlock_shared(so);
return (0);
}
@@ -221,22 +221,15 @@ divert_packet(struct mbuf *m, int dir, u
if_put(ifp);
}
+ mtx_enter(&inp->inp_mtx);
so = inp->inp_socket;
- /*
- * XXXSMP sbappendaddr() is not MP safe and this function is called
- * from pf with shared netlock. To call only one sbappendaddr() from
- * divert_packet(), protect it with kernel lock. All other places
- * call sbappendaddr() with exclusive net lock. This blocks
- * divert_packet() as we have the shared lock.
- */
- KERNEL_LOCK();
if (sbappendaddr(so, &so->so_rcv, sintosa(&sin), m, NULL) == 0) {
- KERNEL_UNLOCK();
+ mtx_leave(&inp->inp_mtx);
divstat_inc(divs_fullsock);
goto bad;
}
+ mtx_leave(&inp->inp_mtx);
