It doesn't seem to have a problem. Sorry for my delay. ok yasuoka
On Wed, 9 Nov 2022 13:24:21 +0300 Vitaliy Makkoveev <m...@openbsd.org> wrote: > ping... > > On Tue, Nov 01, 2022 at 03:16:02PM +0300, Vitaliy Makkoveev wrote: >> Push netlock down to pppx_add_session(). The 'pppx_if' structure has >> the `pxi_ready' member to prevent access to incomplete `pxi', so we >> don't need to hold netlock during all initialisation process. This >> removes potential PR_WAITOK/M_WAITOK allocations impact on packet >> processing. Also this removes relock dances around if_attach() and >> if_detach() calls. >> >> Do not grab netlock for FIONREAD. mbuf(9) queue doesn't rely on it. >> >> Do not grab netlock around pipex_ioctl() call. pipex(4) has its own >> protection and doesn't rely on netlock. We need to unlink pipex(4) >> session before destroy associated `pxi', it can't be killed >> concurrently. Also this stops to block packet processing when npppd(8) >> periodically does PIPEXGCLOSED ioctl(2) commands. >> >> The dummy FIONBIO case doesn't require any lock to be held. >> >> The netlock remains to be taken around pppx_del_session() and >> pppx_set_session_descr() because pppx(4) data structures rely on it. >> >> Index: sys/net/if_pppx.c >> =================================================================== >> RCS file: /cvs/src/sys/net/if_pppx.c,v >> retrieving revision 1.122 >> diff -u -p -r1.122 if_pppx.c >> --- sys/net/if_pppx.c 29 Aug 2022 07:51:45 -0000 1.122 >> +++ sys/net/if_pppx.c 1 Nov 2022 10:08:37 -0000 >> @@ -414,7 +414,6 @@ pppxioctl(dev_t dev, u_long cmd, caddr_t >> struct pppx_dev *pxd = pppx_dev2pxd(dev); >> int error = 0; >> >> - NET_LOCK(); >> switch (cmd) { >> case PIPEXASESSION: >> error = pppx_add_session(pxd, >> @@ -422,13 +421,17 @@ pppxioctl(dev_t dev, u_long cmd, caddr_t >> break; >> >> case PIPEXDSESSION: >> + NET_LOCK(); >> error = pppx_del_session(pxd, >> (struct pipex_session_close_req *)addr); >> + NET_UNLOCK(); >> break; >> >> case PIPEXSIFDESCR: >> + NET_LOCK(); >> error = pppx_set_session_descr(pxd, >> (struct pipex_session_descr_req *)addr); >> + NET_UNLOCK(); >> break; >> >> case FIONBIO: >> @@ -441,7 +444,6 @@ pppxioctl(dev_t dev, u_long cmd, caddr_t >> error = pipex_ioctl(pxd, cmd, addr); >> break; >> } >> - NET_UNLOCK(); >> >> return (error); >> } >> @@ -607,6 +609,7 @@ pppx_add_session(struct pppx_dev *pxd, s >> >> pxi->pxi_session = session; >> >> + NET_LOCK(); >> /* try to set the interface up */ >> unit = pppx_if_next_unit(); >> if (unit < 0) { >> @@ -624,6 +627,7 @@ pppx_add_session(struct pppx_dev *pxd, s >> goto out; >> } >> LIST_INSERT_HEAD(&pxd->pxd_pxis, pxi, pxi_list); >> + NET_UNLOCK(); >> >> snprintf(ifp->if_xname, sizeof(ifp->if_xname), "%s%d", "pppx", unit); >> ifp->if_mtu = req->pr_peer_mru; /* XXX */ >> @@ -638,13 +642,12 @@ pppx_add_session(struct pppx_dev *pxd, s >> /* ifp->if_rdomain = req->pr_rdomain; */ >> if_counters_alloc(ifp); >> >> - /* XXXSMP breaks atomicity */ >> - NET_UNLOCK(); >> if_attach(ifp); >> - NET_LOCK(); >> >> + NET_LOCK(); >> if_addgroup(ifp, "pppx"); >> if_alloc_sadl(ifp); >> + NET_UNLOCK(); >> >> #if NBPFILTER > 0 >> bpfattach(&ifp->if_bpf, ifp, DLT_LOOP, sizeof(u_int32_t)); >> @@ -680,6 +683,7 @@ pppx_add_session(struct pppx_dev *pxd, s >> >> ia->ia_netmask = ia->ia_sockmask.sin_addr.s_addr; >> >> + NET_LOCK(); >> error = in_ifinit(ifp, ia, &ifaddr, 1); >> if (error) { >> printf("pppx: unable to set addresses for %s, error=%d\n", >> @@ -687,26 +691,29 @@ pppx_add_session(struct pppx_dev *pxd, s >> } else { >> if_addrhooks_run(ifp); >> } >> + NET_UNLOCK(); >> >> error = pipex_link_session(session, ifp, pxd); >> if (error) >> goto detach; >> >> + NET_LOCK(); >> SET(ifp->if_flags, IFF_RUNNING); >> pxi->pxi_ready = 1; >> + NET_UNLOCK(); >> >> return (error); >> >> detach: >> - /* XXXSMP breaks atomicity */ >> - NET_UNLOCK(); >> if_detach(ifp); >> - NET_LOCK(); >> >> + NET_LOCK(); >> if (RBT_REMOVE(pppx_ifs, &pppx_ifs, pxi) == NULL) >> panic("%s: inconsistent RB tree", __func__); >> LIST_REMOVE(pxi, pxi_list); >> out: >> + NET_UNLOCK(); >> + >> pool_put(&pppx_if_pl, pxi); >> pipex_rele_session(session); >> >