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);
> 

Reply via email to