On Wed, 12 Aug 2020 12:26:22 +0300
Vitaliy Makkoveev <m...@openbsd.org> wrote:
> We destroy pppx(4) related sessions while we performing PIPEXDSESSION
> command. But with pppac(4) we set session's state to
> PIPEX_STATE_CLOSE_WAIT2 and we wait garbage collector to do destruction.
pppac's PIPEXDSESSION set the states PIPEX_STATE_CLOSED. It is to
wait until pipex{in,out}q becomes empty.
> We removed `pipex{in,out}q'. So we can safe destroy session in any time.
> I propose to make pppac(4) session destruction path the same as pppx(4)
> does. Now we destroy them while performing PIPEXDSESSION commad too.
Yes. I agree this point.
> Also there is no in-kernel garbage collector for pppac(4) sessions.
> yasuoka@ pointed me that npppd(8) should kill expired sessions.
>
> This not only makes pppac(4) closer to pppx(4) but simplify code and
> allow us to make safe pppx(4) session processing by pipex_timer().
> So this is preparation step to restore in-kernel timeout for pppx(4)
> too.
Below, I am asking to keep the timeout behavior. There is a bug for
pppx(4) but it had been working for pppac(4) for long time. If you
really want to change the behavior please provide a reason. I have
not so strong opinion but I don't want to change the behavior without
a reason.
> Index: sys/net/pipex.c
> ===================================================================
> RCS file: /cvs/src/sys/net/pipex.c,v
> retrieving revision 1.124
> diff -u -p -r1.124 pipex.c
> --- sys/net/pipex.c 12 Aug 2020 08:41:39 -0000 1.124
> +++ sys/net/pipex.c 12 Aug 2020 09:07:12 -0000
> @@ -536,29 +536,6 @@ out:
> return error;
> }
>
> -int
> -pipex_notify_close_session(struct pipex_session *session)
> -{
> - NET_ASSERT_LOCKED();
> - session->state = PIPEX_STATE_CLOSE_WAIT;
> - session->stat.idle_time = 0;
> - LIST_INSERT_HEAD(&pipex_close_wait_list, session, state_list);
> -
> - return (0);
> -}
> -
Unrelated but ok.
> -int
> -pipex_notify_close_session_all(void)
> -{
> - struct pipex_session *session;
> -
> - NET_ASSERT_LOCKED();
> - LIST_FOREACH(session, &pipex_session_list, session_list)
> - if (session->state == PIPEX_STATE_OPENED)
> - pipex_notify_close_session(session);
> - return (0);
> -}
> -
Unrelated but ok. Since it's not used.
> Static int
> pipex_close_session(struct pipex_session_close_req *req,
> struct pipex_iface_context *iface)
> @@ -573,13 +550,9 @@ pipex_close_session(struct pipex_session
> if (session->pipex_iface != iface)
> return (EINVAL);
>
> - /* remove from close_wait list */
> - if (session->state == PIPEX_STATE_CLOSE_WAIT)
> - LIST_REMOVE(session, state_list);
> -
This must be kept. Useland may PIPEXDSESSION before PIPEXGCLOSED for
this session.
> /* get statistics before destroy the session */
> req->pcr_stat = session->stat;
> - session->state = PIPEX_STATE_CLOSED;
> + pipex_destroy_session(session);
>
> return (0);
> }
ok
> @@ -739,47 +712,25 @@ pipex_timer_stop(void)
> Static void
> pipex_timer(void *ignored_arg)
> {
> - struct pipex_session *session, *session_tmp;
> + struct pipex_session *session;
>
> 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;
> -
> - pipex_notify_close_session(session);
> - break;
> -
> - case PIPEX_STATE_CLOSE_WAIT:
> - case PIPEX_STATE_CLOSE_WAIT2:
> - /* Wait PIPEXDSESSION from userland */
> - session->stat.idle_time++;
> - 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 */
> + LIST_FOREACH(session, &pipex_session_list, session_list) {
> + if (session->state != PIPEX_STATE_OPENED)
> + continue;
> + if (session->timeout_sec == 0)
> + continue;
>
> - case PIPEX_STATE_CLOSED:
> - pipex_destroy_session(session);
> - break;
> + session->stat.idle_time++;
> + if (session->stat.idle_time < session->timeout_sec)
> + continue;
>
> - default:
> - break;
> - }
> + session->state = PIPEX_STATE_CLOSE_WAIT;
> + LIST_INSERT_HEAD(&pipex_close_wait_list, session, state_list);
> }
> -
> NET_UNLOCK();
> }
>
Please keep this since it's unrelated.
> Index: sys/net/pipex.h
> ===================================================================
> RCS file: /cvs/src/sys/net/pipex.h,v
> retrieving revision 1.27
> diff -u -p -r1.27 pipex.h
> --- sys/net/pipex.h 4 Aug 2020 09:32:05 -0000 1.27
> +++ sys/net/pipex.h 12 Aug 2020 09:07:13 -0000
> @@ -197,9 +197,6 @@ void pipex_init (void);
> void pipex_iface_init (struct pipex_iface_context *, u_int);
> void pipex_iface_fini (struct pipex_iface_context *);
>
> -int pipex_notify_close_session(struct pipex_session
> *session);
> -int pipex_notify_close_session_all(void);
> -
> struct mbuf *pipex_output (struct mbuf *, int, int, struct
> pipex_iface_context *);
> struct pipex_session *pipex_pppoe_lookup_session (struct mbuf *);
> struct mbuf *pipex_pppoe_input (struct mbuf *, struct
> pipex_session *);
