Re: pppac(4): destroy sessions the same way as pppx(4) does
On Sat, Aug 15, 2020 at 02:01:52PM +0900, YASUOKA Masahiko wrote:
> On Wed, 12 Aug 2020 12:26:22 +0300
> Vitaliy Makkoveev 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.
>
My fault. I looked pipex_notify_close_session().
> > 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.
>
The reason is to make garbage collector's behavior identical for
pppac(4) and pppx(4). It's assumed what userland should destroy expired
sessions, so there is no reason to have differences here. Also it allows
us to not introduce differences to processing pppac(4) and pppx(4)
sessions by pipex_timer(). There is no checks "if (session->is_pppx)"
requred in future at this point. The real sense of this diff is to make
pipex_timer() only processing timeout for both cases.
> > 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 - 1.124
> > +++ sys/net/pipex.c 12 Aug 2020 09:07:12 -
> > @@ -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(_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, _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.
>
pipex_destroy_session() calls pipex_unlink_session() which checks
`state' and removes session from `state_list' if required.
> > /* 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(_timer_ch, pipex_prune);
> >
> > NET_LOCK();
> > /* walk through */
> > - LIST_FOREACH_SAFE(session, _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 <
Re: pppac(4): destroy sessions the same way as pppx(4) does
On Wed, 12 Aug 2020 12:26:22 +0300
Vitaliy Makkoveev 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 - 1.124
> +++ sys/net/pipex.c 12 Aug 2020 09:07:12 -
> @@ -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(_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, _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(_timer_ch, pipex_prune);
>
> NET_LOCK();
> /* walk through */
> - LIST_FOREACH_SAFE(session, _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, _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;
> -
pppac(4): destroy sessions the same way as pppx(4) does
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.
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.
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.
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 - 1.124
+++ sys/net/pipex.c 12 Aug 2020 09:07:12 -
@@ -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(_close_wait_list, session, state_list);
-
- return (0);
-}
-
-int
-pipex_notify_close_session_all(void)
-{
- struct pipex_session *session;
-
- NET_ASSERT_LOCKED();
- LIST_FOREACH(session, _session_list, session_list)
- if (session->state == PIPEX_STATE_OPENED)
- pipex_notify_close_session(session);
- return (0);
-}
-
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);
-
/* get statistics before destroy the session */
req->pcr_stat = session->stat;
- session->state = PIPEX_STATE_CLOSED;
+ pipex_destroy_session(session);
return (0);
}
@@ -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(_timer_ch, pipex_prune);
NET_LOCK();
/* walk through */
- LIST_FOREACH_SAFE(session, _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, _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(_close_wait_list, session, state_list);
}
-
NET_UNLOCK();
}
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 - 1.27
+++ sys/net/pipex.h 12 Aug 2020 09:07:13 -
@@ -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
