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?
> 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.
