On Thu, Jun 30, 2022 at 02:32:03PM +0200, Claudio Jeker wrote:
> On Thu, Jun 30, 2022 at 03:21:40PM +0300, Vitaliy Makkoveev wrote:
> > On Thu, Jun 30, 2022 at 11:56:55AM +0200, Claudio Jeker wrote:
> > > On Thu, Jun 30, 2022 at 12:34:33PM +0300, Vitaliy Makkoveev wrote:
> > > > On Thu, Jun 30, 2022 at 11:08:48AM +0200, Claudio Jeker wrote:
> > > > > This diff converts the SRP list to a SMR list in rtsock.c
> > > > > SRP is a bit strange with how it works and the SMR code is a bit
> > > > > easier to
> > > > > understand. Since we can sleep in the SMR_TAILQ_FOREACH() we need to
> > > > > grab
> > > > > a refcount on the route pcb so that we can leave the SMR critical
> > > > > section
> > > > > and then enter the SMR critical section at the end of the loop before
> > > > > dropping the refcount again.
> > > > >
> > > > > The diff does not immeditaly explode but I doubt we can exploit
> > > > > parallelism in route_input() so this may fail at some later stage if
> > > > > it is
> > > > > wrong.
> > > > >
> > > > > Comments from the lock critics welcome
> > > >
> > > > We use `so_lock' rwlock(9) to protect route domain sockets. We can't
> > > > convert this SRP list to SMR list because we call solock() within
> > > > foreach loop.
> > >
> > > because of the so_lock the code uses a refcnt on the route pcb to make
> > > sure that the object is not freed while we sleep. So that is handled by
> > > this diff.
> > >
> > > > We can easily crash kernel by running in parallel some "route monitor"
> > > > commands and "while true; ifconfig vether0 create ; ifconfig vether0
> > > > destroy; done".
> > >
> > > That does not cause problem on my system.
> > >
> >
> > Sorry, I missed you leave SMR section before solock() call:
> >
> > > > > @@ -519,7 +498,8 @@ route_input(struct mbuf *m0, struct sock
> > > > > rop->rop_proto != sa_family)
> > > > > continue;
> > > > >
> > > > > -
> > > > > + refcnt_take(&rop->rop_refcnt);
> > > > > + smr_read_leave();
> > > > > so = rop->rop_socket;
> > > > > solock(so);
> > > > >
> >
> > My system is stable with the second version of this diff and the
> > following test.
>
> Cool but I fear that the kernel still synchronizes somewhere and we don't
> really get full concurrency here. So I'm not sure if this is able to
> trigger a reference bug.
>
route_input() and route_{attach,detach}() are fully asynchronous, except
the `rtp_lk' rwlock(9). When called from route_output() path,
route_input() is not kernel or net locked. We can add some such threads
if we want more concurrency.
