On Mon, Aug 10, 2020 at 03:12:02PM +0900, YASUOKA Masahiko wrote: > Hi, > > Thank you for your review. > > On Sun, 9 Aug 2020 20:03:50 +0300 > Vitaliy Makkoveev <m...@openbsd.org> wrote: > > On Sun, Aug 09, 2020 at 06:20:13PM +0300, Vitaliy Makkoveev wrote: > >> You propose to unlink pppx(4) related session which reached timeout. I'm > >> ok with this direction. But I see no reason to rework _get_closed() > >> routines. > >> > >> in pppac(4) case it's assumed what if session is not yet destroyed by > >> garbage collector, it will be destroyed while we performing PIPEXGCLOSED > >> command. We can make pppx(4) behavior the same and I propose to > >> pppx_get_closed() be like below. > >> > >> Also, nothing requires to modify pipex_get_closed(). > >> > >> ---- cut begin ---- > > > > Sorry, I mean > > > > pppx_get_closed(struct pppx_dev *pxd, struct pipex_session_list_req *req) > > { > > struct pppx_if *pxi; > > > > memset(req, 0, sizeof(*req)); > > > > while ((pxi = LIST_FIRST(&pxd->pxd_pxis))) { > > if (pxi->pxi_session->state == session->state = > > PIPEX_STATE_CLOSED) { > > req->plr_ppp_id[req->plr_ppp_id_count++] = > > pxi->pxi_session->ppp_id; > > pppx_if_destroy(pxi); > > } > > } > > > > return 0; > > } > > Yes, the diff doesn't seem to be completed but this way also will work. > > Usually there is few CLOSED session even if there is a lot of session. > Also there is no CLOSED session if idle-timeout is not configured. I > avoided that way because I think checking all sessions' state to find > such the few sessions is too expensive. > > A way I am suggesting: > > @@ -622,7 +625,7 @@ pipex_get_stat(struct pipex_session_stat > > Static int > pipex_get_closed(struct pipex_session_list_req *req, > - struct pipex_iface_context *iface) > + int (*isowner)(void *, struct pipex_session *), void *ctx) > { > struct pipex_session *session, *session_tmp; > > @@ -630,7 +633,7 @@ pipex_get_closed(struct pipex_session_li > bzero(req, sizeof(*req)); > LIST_FOREACH_SAFE(session, &pipex_close_wait_list, state_list, > session_tmp) { > - if (session->pipex_iface != iface) > + if (!isowner(ctx, session)) > continue; > req->plr_ppp_id[req->plr_ppp_id_count++] = session->ppp_id; > LIST_REMOVE(session, state_list); > > uses pipex_close_wait_list which contains only sessions which is timed > out.
You are right. pipex_get_closed() walks through `pipex_close_wait_list' which contains only CLOSE_WAIT sessions. According to npppd(8) code we do PIPEXGCLOSED related walkthrough once per NPPPD_TIMER_TICK_IVAL seconds, which is defined as 4. Is this such performance impact? Also who should destroy these sessions? It's assumed npppd(8) will destroy them by l2tp_ctrl_timeout() and pptp_ctrl_timeout()? Excuse me if I'm wrong, but who will destroy sessions in pppoe case? > > >> Also I have one inlined comment within your diff. > > >> > @@ -430,6 +425,7 @@ pipex_link_session(struct pipex_session > >> > struct pipex_iface_context *iface) > >> > { > >> > struct pipex_hash_head *chain; > >> > + struct ifnet *ifp; > >> > > >> > NET_ASSERT_LOCKED(); > >> > > >> > @@ -442,6 +438,11 @@ pipex_link_session(struct pipex_session > >> > session->pipex_iface = iface; > >> > session->ifindex = iface->ifindex; > >> > > >> > + ifp = if_get(iface->ifindex); > >> > + if (ifp != NULL && ifp->if_flags & IFF_POINTOPOINT) > >> > + session->is_p2p = 1; > >> > + if_put(ifp); > >> > + > >> > >> I guess NULL `ifp' here exposes us a bug. I like to have assertion here. > > ok, I agree here. > > > The diff is updated. > > Index: sys/net/if_pppx.c > =================================================================== > RCS file: /cvs/src/sys/net/if_pppx.c,v > retrieving revision 1.98 > diff -u -p -r1.98 if_pppx.c > --- sys/net/if_pppx.c 28 Jul 2020 09:53:36 -0000 1.98 > +++ sys/net/if_pppx.c 10 Aug 2020 06:09:52 -0000 > @@ -185,6 +185,7 @@ int pppx_config_session(struct pppx_dev > struct pipex_session_config_req *); > int pppx_get_stat(struct pppx_dev *, > struct pipex_session_stat_req *); > +int pppx_is_owner(void *, struct pipex_session *); > int pppx_get_closed(struct pppx_dev *, > struct pipex_session_list_req *); > int pppx_set_session_descr(struct pppx_dev *, > @@ -645,14 +646,6 @@ pppx_add_session(struct pppx_dev *pxd, s > struct in_ifaddr *ia; > struct sockaddr_in ifaddr; > > - /* > - * XXX: As long as `session' is allocated as part of a `pxi' > - * it isn't possible to free it separately. So disallow > - * the timeout feature until this is fixed. > - */ > - if (req->pr_timeout_sec != 0) > - return (EINVAL); > - > error = pipex_init_session(&session, req); > if (error) > return (error); > @@ -812,12 +805,22 @@ pppx_get_stat(struct pppx_dev *pxd, stru > } > > int > -pppx_get_closed(struct pppx_dev *pxd, struct pipex_session_list_req *req) > +pppx_is_owner(void *ctx, struct pipex_session *session) > { > - /* XXX: Only opened sessions exist for pppx(4) */ > - memset(req, 0, sizeof(*req)); > + struct pppx_dev *pxd = ctx; > + struct pppx_if *pxi; > > - return 0; > + pxi = pppx_if_find(pxd, session->session_id, session->protocol); > + if (pxi != NULL) > + return (1); > + > + return (0); > +} > + > +int > +pppx_get_closed(struct pppx_dev *pxd, struct pipex_session_list_req *req) > +{ > + return (pipex_get_closed(req, pppx_is_owner, pxd)); > } > > int > @@ -1059,6 +1062,7 @@ static int pppac_ioctl(struct ifnet *, u > static int pppac_output(struct ifnet *, struct mbuf *, struct sockaddr *, > struct rtentry *); > static void pppac_start(struct ifnet *); > +static int pppac_is_owner(void *, struct pipex_session *); > > static inline struct pppac_softc * > pppac_lookup(dev_t dev) > @@ -1251,6 +1255,16 @@ pppacwrite(dev_t dev, struct uio *uio, i > } > > int > +pppac_is_owner(void *ctx, struct pipex_session *session) > +{ > + struct pppac_softc *sc = ctx; > + > + if (session->ifindex == sc->sc_if.if_index) > + return (1); > + return (0); > +} > + > +int > pppacioctl(dev_t dev, u_long cmd, caddr_t data, int flags, struct proc *p) > { > struct pppac_softc *sc = pppac_lookup(dev); > @@ -1264,6 +1278,13 @@ pppacioctl(dev_t dev, u_long cmd, caddr_ > break; > case FIONREAD: > *(int *)data = mq_hdatalen(&sc->sc_mq); > + break; > + > + case PIPEXGCLOSED: > + NET_LOCK(); > + error = pipex_get_closed((struct pipex_session_list_req *)data, > + pppac_is_owner, sc); > + NET_UNLOCK(); > break; > > default: > Index: sys/net/pipex.c > =================================================================== > RCS file: /cvs/src/sys/net/pipex.c,v > retrieving revision 1.123 > diff -u -p -r1.123 pipex.c > --- sys/net/pipex.c 4 Aug 2020 09:32:05 -0000 1.123 > +++ sys/net/pipex.c 10 Aug 2020 06:09:52 -0000 > @@ -240,11 +240,6 @@ pipex_ioctl(struct pipex_iface_context * > pipex_iface); > break; > > - case PIPEXGCLOSED: > - ret = pipex_get_closed((struct pipex_session_list_req *)data, > - pipex_iface); > - break; > - > default: > ret = ENOTTY; > break; > @@ -430,6 +425,7 @@ pipex_link_session(struct pipex_session > struct pipex_iface_context *iface) > { > struct pipex_hash_head *chain; > + struct ifnet *ifp; > > NET_ASSERT_LOCKED(); > > @@ -442,6 +438,12 @@ pipex_link_session(struct pipex_session > session->pipex_iface = iface; > session->ifindex = iface->ifindex; > > + ifp = if_get(iface->ifindex); > + KASSERT(ifp != NULL); > + if (ifp->if_flags & IFF_POINTOPOINT) > + session->is_p2p = 1; > + if_put(ifp); > + > LIST_INSERT_HEAD(&pipex_session_list, session, session_list); > chain = PIPEX_ID_HASHTABLE(session->session_id); > LIST_INSERT_HEAD(chain, session, id_chain); > @@ -469,6 +471,8 @@ pipex_unlink_session(struct pipex_sessio > session->ifindex = 0; > > NET_ASSERT_LOCKED(); > + if (session->state == PIPEX_STATE_CLOSED) > + return; > LIST_REMOVE(session, id_chain); > #if defined(PIPEX_PPTP) || defined(PIPEX_L2TP) > switch (session->protocol) { > @@ -622,7 +626,7 @@ pipex_get_stat(struct pipex_session_stat > > Static int > pipex_get_closed(struct pipex_session_list_req *req, > - struct pipex_iface_context *iface) > + int (*isowner)(void *, struct pipex_session *), void *ctx) > { > struct pipex_session *session, *session_tmp; > > @@ -630,7 +634,7 @@ pipex_get_closed(struct pipex_session_li > bzero(req, sizeof(*req)); > LIST_FOREACH_SAFE(session, &pipex_close_wait_list, state_list, > session_tmp) { > - if (session->pipex_iface != iface) > + if (!isowner(ctx, session)) > continue; > req->plr_ppp_id[req->plr_ppp_id_count++] = session->ppp_id; > LIST_REMOVE(session, state_list); > @@ -766,13 +770,13 @@ pipex_timer(void *ignored_arg) > if (session->stat.idle_time < PIPEX_CLOSE_TIMEOUT) > continue; > > - if (session->state == PIPEX_STATE_CLOSE_WAIT) > - LIST_REMOVE(session, state_list); > - session->state = PIPEX_STATE_CLOSED; > /* FALLTHROUGH */ > > case PIPEX_STATE_CLOSED: > - pipex_destroy_session(session); > + if (session->is_p2p) > + pipex_unlink_session(session); > + else > + pipex_destroy_session(session); > break; > > default: > Index: sys/net/pipex_local.h > =================================================================== > RCS file: /cvs/src/sys/net/pipex_local.h,v > retrieving revision 1.39 > diff -u -p -r1.39 pipex_local.h > --- sys/net/pipex_local.h 4 Aug 2020 09:32:05 -0000 1.39 > +++ sys/net/pipex_local.h 10 Aug 2020 06:09:52 -0000 > @@ -169,7 +169,8 @@ struct pipex_session { > uint16_t ip_forward:1, /* [N] {en|dis}ableIP forwarding */ > ip6_forward:1, /* [I] {en|dis}able IPv6 forwarding */ > is_multicast:1, /* [I] virtual entry for multicast */ > - reserved:13; > + is_p2p:1, /* [I] interface is point2point(pppx) */ > + reserved:12; > uint16_t protocol; /* [I] tunnel protocol (PK) */ > uint16_t session_id; /* [I] session-id (PK) */ > uint16_t peer_session_id; /* [I] peer's session-id */ > @@ -391,7 +392,7 @@ int pipex_config_sessi > int pipex_get_stat (struct pipex_session_stat_req *, > struct pipex_iface_context *); > int pipex_get_closed (struct pipex_session_list_req *, > - struct pipex_iface_context *); > + int (*)(void *, struct pipex_session *), void *); > int pipex_destroy_session (struct pipex_session *); > struct pipex_session *pipex_lookup_by_ip_address (struct in_addr); > struct pipex_session *pipex_lookup_by_session_id (int, int); >