On Wed, Aug 12, 2020 at 12:37:13AM +0900, YASUOKA Masahiko wrote:
> Hi,
>
> On Mon, 10 Aug 2020 16:30:27 +0300
> Vitaliy Makkoveev <m...@openbsd.org> wrote:
> > On Mon, Aug 10, 2020 at 03:12:02PM +0900, YASUOKA Masahiko wrote:
> >> 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?
>
> It might be not so expensive for you. But why do you intend to use
> that extra CPU when you have a cheaper way?
Please don't assume my objections like this. Like you, I want to keep
pppac(4) and pppx(4) close as possible.
Let me explain the reason of my objection versus your diff.
We have 2 different cases to destroy pipex(4) session:
1. pppx(4). We just destroy session by PIPEXDSESSION command. We can't
permit this session to be killed by pipex_timer().
2. pppac(4). While we performing PIPEXDSESSION command we mark session
as PIPEX_STATE_CLOSE_WAIT2 and assume that pipex_timer() will kill it.
Also pipex_timer() will always kill expired sessions.
Your diff kept pppac(4) behavior a but introduce new case for pppx(4):
expired sessions will still exist in unlinked state. Userland should do
garbage collecting. I was afraid these sessions will be not killed until
npppd(8) shutdown.
I looked to npppd(8) code, but it was no obvious to me that userland
should do garbage collecting. My fault, I had to be asked before. But
now you explained, it's assumed that userland should perform garbage
collecting.
You are right, I want to unify in-kernel garbage collecting for pppx(4)
and pppac(4). With your explanation about userland garbage collecting I
like to propose the solution, which is better then previous were.
We removed `pipex{in,out}q'. So now we can destroy pppac(4) session just
like we do in pppx(4) case. Also there is no reason to allow
pipex_timer() to destroy sessions - userland will do this by
PIPEXDSESSION. This permit us to use existing pipex_get_closed() for
both pppac(4) and pppx(4) without any modifications.
So, I propose pipex_close_session() and pipex_timer() be like below.
We simplify pppac(4) session destruction. We unify behavior with pppx(4)
- we killing session just now. There is no reason to modify
pipex_get_closed() and pipex_link_session(). pppx(4) related sessions
can be processed by pipex_timer(). There is no performance impact.
Do you like this? We can do two diffs. The first to unify destruction
and the second to re-enable in-kernel timeout for pppx(4) and revert man
pages modifications.
---- cut begin ----
pipex_close_session(struct pipex_session_close_req *req,
struct pipex_iface_context *iface)
{
struct pipex_session *session;
NET_ASSERT_LOCKED();
session = pipex_lookup_by_session_id(req->pcr_protocol,
req->pcr_session_id);
if (session == NULL)
return (EINVAL);
if (session->pipex_iface != iface)
return (EINVAL);
/* get statistics before destroy the session */
req->pcr_stat = session->stat;
/* destroy session */
pipex_destroy_session(session);
return (0);
}
pipex_timer(void *ignored_arg)
{
/* ... */
struct pipex_session *session;
timeout_add_sec(&pipex_timer_ch, pipex_prune);
NET_LOCK();
/* walk through */
LIST_FOREACH(session, &pipex_session_list, session_list) {
if (session->state != PIPEX_STATE_OPENED)
continue;
if (session->timeout_sec == 0)
continue;
session->stat.idle_time++;
if (session->stat.idle_time < session->timeout_sec)
continue;
session->state = PIPEX_STATE_CLOSE_WAIT;
LIST_INSERT_HEAD(&pipex_close_wait_list, session, state_list);
}
NET_UNLOCK();
}
---- cut end ----
>
> > 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?
>
> In usr.sbin/npppd/npppd/npppd.c:
>
> 1306 static void
> 1307 pipex_periodic(npppd *_this)
> 1308 {
> (snip)
> 1326 do {
> 1327 error = ioctl(devf, PIPEXGCLOSED, &req);
> 1328 if (error) {
> 1329 if (errno != ENXIO)
> 1330 log_printf(LOG_WARNING,
> 1331 "PIPEXGCLOSED failed: %m");
> 1332 break;
> 1333 }
> 1334 for (i = 0; i < req.plr_ppp_id_count; i++) {
> 1335 ppp_id = req.plr_ppp_id[i];
> 1336 slist_add(&dlist, (void
> *)(uintptr_t)ppp_id);
> 1337 }
> 1338 } while (req.plr_flags & PIPEX_LISTREQ_MORE);
>
> ppp sessions which are closed by pipex(4) is inserted into "dlist".
>
> 1350 /* Disconnect request */
> 1351 slist_itr_first(&dlist);
> 1352 while (slist_itr_has_next(&dlist)) {
> (snip)
> 1372 ppp_log(ppp, LOG_INFO, "Stop requested by the kernel");
> 1373 /* TODO: PIPEX doesn't return the disconect reason */
> 1374 #ifdef USE_NPPPD_RADIUS
> 1375 ppp_set_radius_terminate_cause(ppp,
> 1376 RADIUS_TERMNATE_CAUSE_IDLE_TIMEOUT);
> 1377 #endif
> 1378 ppp_stop(ppp, NULL);
>
> all ppp session are stopd at #1378. PPP is finisingh a layer by a
> layer, ppp_stop0() will called. That function will call PIPEXDSESSION.
>
> I'd like to empasize that npppd(8) takes responsibilities of pipex
> sessions' creation/deletion even when idle timeout happening.
>
