We are doing all wrong :)
We can just unlink pppx(4) related session from `pipex_session_list' if
it's time expired. But since this unlinked session is still exists in
pppx(4) layer we can access through pppx_get_closed() without any
search. We should only add flag to session which identifies it as
pppx(4) related.
I hope you like this idea.
---- cut begin ----
Static void
pipex_timer(void *ignored_arg)
{
struct pipex_session *session, *session_tmp;
timeout_add_sec(&pipex_timer_ch, pipex_prune);
NET_LOCK();
/* walk through */
LIST_FOREACH_SAFE(session, &pipex_session_list, session_list,
session_tmp) {
switch (session->state) {
case PIPEX_STATE_OPENED:
if (session->timeout_sec == 0)
continue;
session->stat.idle_time++;
if (session->stat.idle_time < session->timeout_sec)
continue;
if (session->pppx_session)
pipex_unlink_session(session);
else
pipex_notify_close_session(session);
break;
/* ... */
}
pppx_get_closed(struct pppx_dev *pxd, struct pipex_session_list_req *req)
{
struct pppx_if *pxi;
pxi = pppx_if_find(pxd, req->pdr_session_id, req->pdr_protocol);
if (pxi == NULL)
return (EINVAL);
memset(req, 0, sizeof(*req));
if (session->state == PIPEX_STATE_CLOSED) {
req->plr_ppp_id[req->plr_ppp_id_count++] = session->ppp_id;
pppx_if_destroy(pxi);
}
return 0;
}
---- cut end ----
On Mon, Aug 10, 2020 at 04:30:27PM +0300, Vitaliy Makkoveev wrote:
> 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);
> >
>
