Re: pipex "idle-timeout" work with pppx(4).
On Wed, Aug 12, 2020 at 09:07:15PM +0900, YASUOKA Masahiko wrote:
> Hi,
>
> On Wed, 12 Aug 2020 12:38:39 +0300
> Vitaliy Makkoveev wrote:
> > We don't need to mark pppx(4) sessions because there is no special cases
> > for them. We just need to kill pppx(4) related "pr_timeout_sec != 0"
> > checks and call pipex_get_closed() by pppx_get_closed().
>
> How do you implement that by calling pipex_get_closed() by
> pppx_get_closed()?
>
>
> PIPEXGCLOSED is to pick up expired sessions which is associated with
> the character device (/dev/{pppx,pppac}0). In pppac(4) case, the
> character device is the same object of the interface pppac. But
> pppx(4) is not the same. pipex_session has no direct referece to the
> device. This is why my diff was modifying pipex_get_closed().
>
You are right. I have my own tree where I divided pppx(4) sessions and
iface_context. So in my tree I have one `pipex_iface_context' in `struct
pppx_dev' and the usage of most pppx(4) is identical to pppac(4). Sorry,
I forgot that it's not shared yet :(
You are right, pppx_get_closed() still needs to do it's own
`pipex_close_wait_list' walkthrough.
Re: pipex "idle-timeout" work with pppx(4).
Hi,
On Wed, 12 Aug 2020 12:38:39 +0300
Vitaliy Makkoveev wrote:
> We don't need to mark pppx(4) sessions because there is no special cases
> for them. We just need to kill pppx(4) related "pr_timeout_sec != 0"
> checks and call pipex_get_closed() by pppx_get_closed().
How do you implement that by calling pipex_get_closed() by
pppx_get_closed()?
PIPEXGCLOSED is to pick up expired sessions which is associated with
the character device (/dev/{pppx,pppac}0). In pppac(4) case, the
character device is the same object of the interface pppac. But
pppx(4) is not the same. pipex_session has no direct referece to the
device. This is why my diff was modifying pipex_get_closed().
Re: pipex "idle-timeout" work with pppx(4).
On Wed, Aug 12, 2020 at 11:17:29AM +0900, YASUOKA Masahiko wrote:
> On Tue, 11 Aug 2020 23:06:45 +0300
> Vitaliy Makkoveev wrote:
> > 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.
>
> It doesn't seem to fix "idle-timeout".
>
Yes it's not. It's "pre-" step which makes following fix easier.
We don't need to mark pppx(4) sessions because there is no special cases
for them. We just need to kill pppx(4) related "pr_timeout_sec != 0"
checks and call pipex_get_closed() by pppx_get_closed().
> > 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.
>
> We need to modify pppx_get_closed() to implement idle-timeout.
>
> > 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.
>
> I have no objection to your "unify destruction".
>
> I'll rebase my diff after that work.
Thanks. I posted "unify destruction" here [1].
1. https://marc.info/?l=openbsd-tech=159722447900893=2
Re: pipex "idle-timeout" work with pppx(4).
On Tue, 11 Aug 2020 23:06:45 +0300
Vitaliy Makkoveev wrote:
> 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.
It doesn't seem to fix "idle-timeout".
> 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.
We need to modify pppx_get_closed() to implement idle-timeout.
> 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.
I have no objection to your "unify destruction".
I'll rebase my diff after that work.
Re: pipex "idle-timeout" work with pppx(4).
On Wed, Aug 12, 2020 at 01:36:38AM +0900, YASUOKA Masahiko wrote: > > my diff is to make pppx(4) have the same "idle-timeout" > functionality. I strongly think pppx(4) must have the same > functionalities of pppac(4) because I don't see any reason to have > any difference between pppx(4) and pppac(4). > Yes, I want the same :) > Your pseudo code is suggesting another thing. You would like to > change the existing behavior of pppac(4)? Then, what is a problem you > concern. I'd like you to provide what is the relation of my diff or a > background of the code. > We have the differences with in-kernel garbage collecting. I was afraid we could have the case while expired pppx(4) sessions will not be killed until npppd(8) shutdown. In your previous mail [1] you explained about npppd(8) garbage collecting. I hope you like my solution I proposed in [2] 1. https://marc.info/?l=openbsd-tech=159716033115495=2 2. https://marc.info/?l=openbsd-tech=159717643020853=2
Re: pipex "idle-timeout" work with pppx(4).
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 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 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_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, _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)
Re: pipex "idle-timeout" work with pppx(4).
my diff is to make pppx(4) have the same "idle-timeout"
functionality. I strongly think pppx(4) must have the same
functionalities of pppac(4) because I don't see any reason to have
any difference between pppx(4) and pppac(4).
Your pseudo code is suggesting another thing. You would like to
change the existing behavior of pppac(4)? Then, what is a problem you
concern. I'd like you to provide what is the relation of my diff or a
background of the code.
On Tue, 11 Aug 2020 01:20:45 +0300
Vitaliy Makkoveev wrote:
>
>
>> On 10 Aug 2020, at 19:53, Vitaliy Makkoveev wrote:
>>
>> We are doing all wrong :)
>>
>> We can just unlink pppx(4) related session from `pipex_session_list' if
>> it's time expired. But since this unlinked session is still exists in
>> pppx(4) layer we can access through pppx_get_closed() without any
>> search. We should only add flag to session which identifies it as
>> pppx(4) related.
>>
>> I hope you like this idea.
>>
>> cut begin
>> Static void
>> pipex_timer(void *ignored_arg)
>> {
>>struct pipex_session *session, *session_tmp;
>>
>>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;
>>
>> if (session->pppx_session)
>> pipex_unlink_session(session);
>> else
>> pipex_notify_close_session(session);
>>break;
>> /* ... */
>> }
>>
>> pppx_get_closed(struct pppx_dev *pxd, struct pipex_session_list_req *req)
>> {
>> struct pppx_if *pxi;
>>
>> pxi = pppx_if_find(pxd, req->pdr_session_id, req->pdr_protocol);
>> if (pxi == NULL)
>> return (EINVAL);
>>
>> memset(req, 0, sizeof(*req));
>> if (session->state == PIPEX_STATE_CLOSED) {
>> req->plr_ppp_id[req->plr_ppp_id_count++] = session->ppp_id;
>> pppx_if_destroy(pxi);
>> }
>>
>> return 0;
>> }
>
> Sorry for noise. I should avoid to write pseudo code.
Re: pipex "idle-timeout" work with pppx(4).
Hi,
On Mon, 10 Aug 2020 16:30:27 +0300
Vitaliy Makkoveev 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 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_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, _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, );
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(, (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();
1352 while (slist_itr_has_next()) {
(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.
Re: pipex "idle-timeout" work with pppx(4).
> On 10 Aug 2020, at 19:53, Vitaliy Makkoveev wrote:
>
> We are doing all wrong :)
>
> We can just unlink pppx(4) related session from `pipex_session_list' if
> it's time expired. But since this unlinked session is still exists in
> pppx(4) layer we can access through pppx_get_closed() without any
> search. We should only add flag to session which identifies it as
> pppx(4) related.
>
> I hope you like this idea.
>
> cut begin
> Static void
> pipex_timer(void *ignored_arg)
> {
>struct pipex_session *session, *session_tmp;
>
>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;
>
> if (session->pppx_session)
> pipex_unlink_session(session);
> else
> pipex_notify_close_session(session);
>break;
> /* ... */
> }
>
> pppx_get_closed(struct pppx_dev *pxd, struct pipex_session_list_req *req)
> {
> struct pppx_if *pxi;
>
> pxi = pppx_if_find(pxd, req->pdr_session_id, req->pdr_protocol);
> if (pxi == NULL)
> return (EINVAL);
>
> memset(req, 0, sizeof(*req));
> if (session->state == PIPEX_STATE_CLOSED) {
> req->plr_ppp_id[req->plr_ppp_id_count++] = session->ppp_id;
> pppx_if_destroy(pxi);
> }
>
> return 0;
> }
Sorry for noise. I should avoid to write pseudo code.
Re: pipex "idle-timeout" work with pppx(4).
We are doing all wrong :)
We can just unlink pppx(4) related session from `pipex_session_list' if
it's time expired. But since this unlinked session is still exists in
pppx(4) layer we can access through pppx_get_closed() without any
search. We should only add flag to session which identifies it as
pppx(4) related.
I hope you like this idea.
cut begin
Static void
pipex_timer(void *ignored_arg)
{
struct pipex_session *session, *session_tmp;
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;
if (session->pppx_session)
pipex_unlink_session(session);
else
pipex_notify_close_session(session);
break;
/* ... */
}
pppx_get_closed(struct pppx_dev *pxd, struct pipex_session_list_req *req)
{
struct pppx_if *pxi;
pxi = pppx_if_find(pxd, req->pdr_session_id, req->pdr_protocol);
if (pxi == NULL)
return (EINVAL);
memset(req, 0, sizeof(*req));
if (session->state == PIPEX_STATE_CLOSED) {
req->plr_ppp_id[req->plr_ppp_id_count++] = session->ppp_id;
pppx_if_destroy(pxi);
}
return 0;
}
cut end
On Mon, Aug 10, 2020 at 04:30:27PM +0300, Vitaliy Makkoveev wrote:
> 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 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_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, _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
Re: pipex "idle-timeout" work with pppx(4).
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 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_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, _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 - 1.98
> +++ sys/net/if_pppx.c 10 Aug 2020 06:09:52 -
> @@ -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
Re: pipex "idle-timeout" work with pppx(4).
Hi,
Thank you for your review.
On Sun, 9 Aug 2020 20:03:50 +0300
Vitaliy Makkoveev 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_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, _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.
>> 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 - 1.98
+++ sys/net/if_pppx.c 10 Aug 2020 06:09:52 -
@@ -185,6 +185,7 @@ int pppx_config_session(struct pppx_dev
struct pipex_session_config_req *);
intpppx_get_stat(struct pppx_dev *,
struct pipex_session_stat_req *);
+intpppx_is_owner(void *, struct pipex_session *);
intpppx_get_closed(struct pppx_dev *,
struct pipex_session_list_req *);
intpppx_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(, 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)
+{
+
Re: pipex "idle-timeout" work with pppx(4).
On Sun, Aug 09, 2020 at 06:20:13PM +0300, Vitaliy Makkoveev wrote:
> Hello Yasuoka.
>
> 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
>
> pppx_get_closed(struct pppx_dev *pxd, struct pipex_session_list_req *req)
> {
> struct pppx_if *pxi;
>
> pxi = pppx_if_find(pxd, req->pcr_session_id, req->pcr_protocol);
> if (pxi == NULL)
> return (EINVAL);
>
> memset(req, 0, sizeof(*req));
> req->plr_ppp_id[req->plr_ppp_id_count++] = pxi->pxi_session->ppp_id;
> pppx_if_destroy(pxi);
>
> return 0;
> }
>
> cut end
>
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_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;
}
> Also I have one inlined comment within your diff.
>
> On Sun, Aug 09, 2020 at 05:14:13PM +0900, YASUOKA Masahiko wrote:
> > This diff makes pipex "idle-timeout" work with pppx(4).
> >
> > ok?
> >
> > Index: sys/net/if_pppx.c
> > ===
> > RCS file: /disk/cvs/openbsd/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 - 1.98
> > +++ sys/net/if_pppx.c 9 Aug 2020 08:05:16 -
> > @@ -185,6 +185,7 @@ int pppx_config_session(struct pppx_dev
> > struct pipex_session_config_req *);
> > intpppx_get_stat(struct pppx_dev *,
> > struct pipex_session_stat_req *);
> > +intpppx_is_owner(void *, struct pipex_session *);
> > intpppx_get_closed(struct pppx_dev *,
> > struct pipex_session_list_req *);
> > intpppx_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(, 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 intpppac_ioctl(struct ifnet *, u
> > static int pppac_output(struct ifnet *, struct mbuf *, struct sockaddr *,
> > struct rtentry *);
> > static voidpppac_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_mq);
> > + break;
> > +
> > + case PIPEXGCLOSED:
> > + NET_LOCK();
> > +
Re: pipex "idle-timeout" work with pppx(4).
Hello Yasuoka.
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
pppx_get_closed(struct pppx_dev *pxd, struct pipex_session_list_req *req)
{
struct pppx_if *pxi;
pxi = pppx_if_find(pxd, req->pcr_session_id, req->pcr_protocol);
if (pxi == NULL)
return (EINVAL);
memset(req, 0, sizeof(*req));
req->plr_ppp_id[req->plr_ppp_id_count++] = pxi->pxi_session->ppp_id;
pppx_if_destroy(pxi);
return 0;
}
cut end
Also I have one inlined comment within your diff.
On Sun, Aug 09, 2020 at 05:14:13PM +0900, YASUOKA Masahiko wrote:
> This diff makes pipex "idle-timeout" work with pppx(4).
>
> ok?
>
> Index: sys/net/if_pppx.c
> ===
> RCS file: /disk/cvs/openbsd/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 - 1.98
> +++ sys/net/if_pppx.c 9 Aug 2020 08:05:16 -
> @@ -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(, 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_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: /disk/cvs/openbsd/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 - 1.123
> +++ sys/net/pipex.c 9 Aug 2020 08:05:16 -
> @@ -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:
