Re: convert sppp(4) to taskq
On 15/11/13(Fri) 15:45, Stefan Sperling wrote:
On Fri, Nov 15, 2013 at 03:20:48PM +0100, Mike Belopuhov wrote:
On 15 November 2013 15:13, Stefan Sperling s...@openbsd.org wrote:
Is this done right?
Works here with pppoe(4) for both IPv4 and IPv6.
i think this diff might lack task_del's in the detach code.
Ooops, good catch.
have you tried destroying your pppoe interface?
Yes, but evidently not while a task was scheduled.
Same diff with task_del calls added.
Even if right now calling task_del() is enough, do you know if there's
an easy way to convert this code without putting the task storage in
the chunk of memory it manipulates? In other words having the struct
task outside of the softc?
I'm asking because if you can do it, this will make the task totally
independent and won't require any task_del(). The idea behind that
is that tomorrow we will try to have more kernel threads running in
parallel, and if your task is about to run or already running when
your interface is destroyed you might end up freeing the memory the
task is manipulating.
Since the current code is already using allocating some memory for the
arguments of the task, I'd argue that this is better than putting the
task storage on the same memory chunk that it manipulates. But since
this problem exists in other places in our tree, we might think of an
alternative solution/api/whatever.
Other than that your diff looks ok.
Index: if_sppp.h
===
RCS file: /cvs/src/sys/net/if_sppp.h,v
retrieving revision 1.19
diff -u -p -r1.19 if_sppp.h
--- if_sppp.h 14 Nov 2013 16:52:33 - 1.19
+++ if_sppp.h 15 Nov 2013 13:48:47 -
@@ -93,6 +93,12 @@ struct spppreq {
#ifdef _KERNEL
#include sys/timeout.h
+#include sys/task.h
+
+#ifdef INET6
+#include netinet/in.h
+#include netinet6/in6_var.h
+#endif
#define IDX_LCP 0/* idx into state table */
@@ -124,8 +130,13 @@ struct sipcp {
#define IPV6CP_MYIFID_SEEN 2 /* have seen my suggested ifid */
u_int32_t saved_hisaddr; /* if hisaddr (IPv4) is dynamic, save
* original one here, in network byte order */
- u_int32_t req_hisaddr; /* remote address requested */
- u_int32_t req_myaddr; /* local address requested */
+ u_int32_t req_hisaddr; /* remote address requested (IPv4) */
+ u_int32_t req_myaddr; /* local address requested (IPv4) */
+#ifdef INET6
+ struct in6_aliasreq req_ifid; /* local ifid requested (IPv6) */
+#endif
+ struct task set_addr_task; /* set address from process context */
+ struct task clear_addr_task;/* clear address from process context */
};
struct sauth {
Index: if_spppsubr.c
===
RCS file: /cvs/src/sys/net/if_spppsubr.c,v
retrieving revision 1.112
diff -u -p -r1.112 if_spppsubr.c
--- if_spppsubr.c 14 Nov 2013 16:52:33 - 1.112
+++ if_spppsubr.c 15 Nov 2013 14:37:24 -
@@ -46,7 +46,6 @@
#include sys/syslog.h
#include sys/malloc.h
#include sys/mbuf.h
-#include sys/workq.h
#include sys/timeout.h
#include crypto/md5.h
@@ -72,10 +71,6 @@
#include net/if_sppp.h
-#ifdef INET6
-#include netinet6/in6_var.h
-#endif
-
# define UNTIMEOUT(fun, arg, handle) \
timeout_del((handle))
@@ -291,6 +286,7 @@ HIDE void sppp_lcp_check_and_close(struc
HIDE int sppp_ncp_check(struct sppp *sp);
HIDE void sppp_ipcp_init(struct sppp *sp);
+HIDE void sppp_ipcp_destroy(struct sppp *sp);
HIDE void sppp_ipcp_up(struct sppp *sp);
HIDE void sppp_ipcp_down(struct sppp *sp);
HIDE void sppp_ipcp_open(struct sppp *sp);
@@ -306,6 +302,7 @@ HIDE void sppp_ipcp_tlf(struct sppp *sp)
HIDE void sppp_ipcp_scr(struct sppp *sp);
HIDE void sppp_ipv6cp_init(struct sppp *sp);
+HIDE void sppp_ipv6cp_destroy(struct sppp *sp);
HIDE void sppp_ipv6cp_up(struct sppp *sp);
HIDE void sppp_ipv6cp_down(struct sppp *sp);
HIDE void sppp_ipv6cp_open(struct sppp *sp);
@@ -902,6 +899,9 @@ sppp_detach(struct ifnet *ifp)
struct sppp **q, *p, *sp = (struct sppp*) ifp;
int i;
+ sppp_ipcp_destroy(sp);
+ sppp_ipv6cp_destroy(sp);
+
/* Remove the entry from the keepalive list. */
for (q = spppq; (p = *q); q = p-pp_next)
if (p == sp) {
@@ -2637,6 +2637,15 @@ sppp_ipcp_init(struct sppp *sp)
sp-ipcp.flags = 0;
sp-state[IDX_IPCP] = STATE_INITIAL;
sp-fail_counter[IDX_IPCP] = 0;
+ task_set(sp-ipcp.set_addr_task, sppp_set_ip_addrs, sp, NULL);
+ task_set(sp-ipcp.clear_addr_task, sppp_clear_ip_addrs, sp, NULL);
+}
+
+HIDE void
+sppp_ipcp_destroy(struct sppp *sp)
+{
+ task_del(systq, sp-ipcp.set_addr_task);
+ task_del(systq, sp-ipcp.clear_addr_task);
}
HIDE void
@@ -2955,38 +2964,11 @@ sppp_ipcp_RCN_nak(struct sppp *sp, struc
addlog(\n);
Re: convert sppp(4) to taskq
On Mon, Nov 18, 2013 at 12:37:53PM +0100, Martin Pieuchot wrote: Even if right now calling task_del() is enough, do you know if there's an easy way to convert this code without putting the task storage in the chunk of memory it manipulates? In other words having the struct task outside of the softc? Well, tasks are stored in softc everywhere I looked for examples. The task storage and the storage for the task's arguments needs to be known when task_set() is called. As I understand, task_set() is called once, at attach time. Then the task gets scheduled or cancelled with task_add()/task_del(), and when the task runs it gets its arguments from the storage given to task_set(). If we wanted to store the task and its arguments outside the softc, I guess we'd need to malloc() task memory at attach time and free it on detach(). I.e. it has the same lifetime as the softc. So I don't see the point of uncoupling it from the softc. Or we would need to call task_set() and task_add() every time we want to schedule the task. I don't think that's what the tasqk API is intending. Or we would have the task itself free its own storage (task and arguments), instead of freeing it when the softc is destroyed. But I don't think that's what the API is intending either. I'm asking because if you can do it, this will make the task totally independent and won't require any task_del(). The idea behind that is that tomorrow we will try to have more kernel threads running in parallel, and if your task is about to run or already running when your interface is destroyed you might end up freeing the memory the task is manipulating. I'm assuming the task won't be interrupted in a way that would make the ifp storage go away while it runs. Perhaps in a new world with more kernel threads that won't hold. But that would cause quite a lot of problems everywhere, not just sppp(4). When the ifp goes away, the task must be not be allowed to run if already scheduled. Because a workq task cannot be cancelled, the bug where the interface is deleted before the workq task gets to run is present in the current code and fixed by switching to taskq, with the task_del() call at interface detach time. How can we fix this bug without task_del()? Would you have the task itself cope with an ifp that has disappeared? Since the current code is already using allocating some memory for the arguments of the task, I'd argue that this is better than putting the task storage on the same memory chunk that it manipulates. But since this problem exists in other places in our tree, we might think of an alternative solution/api/whatever. For now, I'd like to stick with the task in softc idiom I saw elsewhere. Generally, I think your concern is out of scope for my sppp(4) changes and should be discussed in a separate discussion thread because using a different idiom would affect many drivers.
Re: convert sppp(4) to taskq
On 18/11/13(Mon) 13:35, Stefan Sperling wrote: On Mon, Nov 18, 2013 at 12:37:53PM +0100, Martin Pieuchot wrote: Even if right now calling task_del() is enough, do you know if there's an easy way to convert this code without putting the task storage in the chunk of memory it manipulates? In other words having the struct task outside of the softc? Well, tasks are stored in softc everywhere I looked for examples. I know :) But how many of these softc can be freed? I also introduced a similar situation in audio(4) if it is attached to uaudio(4), and I'm looking for an alternative way of doing it. The task storage and the storage for the task's arguments needs to be known when task_set() is called. As I understand, task_set() is called once, at attach time. Then the task gets scheduled or cancelled with task_add()/task_del(), and when the task runs it gets its arguments from the storage given to task_set(). If we wanted to store the task and its arguments outside the softc, I guess we'd need to malloc() task memory at attach time and free it on detach(). I.e. it has the same lifetime as the softc. So I don't see the point of uncoupling it from the softc. The point is that there's no way to check if the task queue still has a reference to your task when you free() your softc, right now big lock is protecting us™ :) Or we would need to call task_set() and task_add() every time we want to schedule the task. I don't think that's what the tasqk API is intending. Or we would have the task itself free its own storage (task and arguments), instead of freeing it when the softc is destroyed. But I don't think that's what the API is intending either. That's why I'm asking, maybe you or somebody else already has an idea. I'm asking because if you can do it, this will make the task totally independent and won't require any task_del(). The idea behind that is that tomorrow we will try to have more kernel threads running in parallel, and if your task is about to run or already running when your interface is destroyed you might end up freeing the memory the task is manipulating. I'm assuming the task won't be interrupted in a way that would make the ifp storage go away while it runs. Perhaps in a new world with more kernel threads that won't hold. But that would cause quite a lot of problems everywhere, not just sppp(4). When the ifp goes away, the task must be not be allowed to run if already scheduled. Because a workq task cannot be cancelled, the bug where the interface is deleted before the workq task gets to run is present in the current code and fixed by switching to taskq, with the task_del() call at interface detach time. How can we fix this bug without task_del()? Would you have the task itself cope with an ifp that has disappeared? Yep, that's another way to fix this problem. You can pass the index of the interface to the task and call if_get() when it runs. If the interface is gone don't do anything otherwise run the task. But this approach only makes sense if the task storage is not freed when the interface is destroyed. Since the current code is already using allocating some memory for the arguments of the task, I'd argue that this is better than putting the task storage on the same memory chunk that it manipulates. But since this problem exists in other places in our tree, we might think of an alternative solution/api/whatever. For now, I'd like to stick with the task in softc idiom I saw elsewhere. Fine with me. Generally, I think your concern is out of scope for my sppp(4) changes and should be discussed in a separate discussion thread because using a different idiom would affect many drivers. It is, clearly, but maybe you had an easy way to avoid it, apparently not ;) To be clear, my concern is only about drivers that can be destroyed/unplugged, that's hopefully not affecting so many drivers.
Re: convert sppp(4) to taskq
On 15 November 2013 15:13, Stefan Sperling s...@openbsd.org wrote: Is this done right? Works here with pppoe(4) for both IPv4 and IPv6. i think this diff might lack task_del's in the detach code. have you tried destroying your pppoe interface?
Re: convert sppp(4) to taskq
On Fri, Nov 15, 2013 at 03:20:48PM +0100, Mike Belopuhov wrote:
On 15 November 2013 15:13, Stefan Sperling s...@openbsd.org wrote:
Is this done right?
Works here with pppoe(4) for both IPv4 and IPv6.
i think this diff might lack task_del's in the detach code.
Ooops, good catch.
have you tried destroying your pppoe interface?
Yes, but evidently not while a task was scheduled.
Same diff with task_del calls added.
Index: if_sppp.h
===
RCS file: /cvs/src/sys/net/if_sppp.h,v
retrieving revision 1.19
diff -u -p -r1.19 if_sppp.h
--- if_sppp.h 14 Nov 2013 16:52:33 - 1.19
+++ if_sppp.h 15 Nov 2013 13:48:47 -
@@ -93,6 +93,12 @@ struct spppreq {
#ifdef _KERNEL
#include sys/timeout.h
+#include sys/task.h
+
+#ifdef INET6
+#include netinet/in.h
+#include netinet6/in6_var.h
+#endif
#define IDX_LCP 0 /* idx into state table */
@@ -124,8 +130,13 @@ struct sipcp {
#define IPV6CP_MYIFID_SEEN 2 /* have seen my suggested ifid */
u_int32_t saved_hisaddr; /* if hisaddr (IPv4) is dynamic, save
* original one here, in network byte order */
- u_int32_t req_hisaddr; /* remote address requested */
- u_int32_t req_myaddr; /* local address requested */
+ u_int32_t req_hisaddr; /* remote address requested (IPv4) */
+ u_int32_t req_myaddr; /* local address requested (IPv4) */
+#ifdef INET6
+ struct in6_aliasreq req_ifid; /* local ifid requested (IPv6) */
+#endif
+ struct task set_addr_task; /* set address from process context */
+ struct task clear_addr_task;/* clear address from process context */
};
struct sauth {
Index: if_spppsubr.c
===
RCS file: /cvs/src/sys/net/if_spppsubr.c,v
retrieving revision 1.112
diff -u -p -r1.112 if_spppsubr.c
--- if_spppsubr.c 14 Nov 2013 16:52:33 - 1.112
+++ if_spppsubr.c 15 Nov 2013 14:37:24 -
@@ -46,7 +46,6 @@
#include sys/syslog.h
#include sys/malloc.h
#include sys/mbuf.h
-#include sys/workq.h
#include sys/timeout.h
#include crypto/md5.h
@@ -72,10 +71,6 @@
#include net/if_sppp.h
-#ifdef INET6
-#include netinet6/in6_var.h
-#endif
-
# define UNTIMEOUT(fun, arg, handle) \
timeout_del((handle))
@@ -291,6 +286,7 @@ HIDE void sppp_lcp_check_and_close(struc
HIDE int sppp_ncp_check(struct sppp *sp);
HIDE void sppp_ipcp_init(struct sppp *sp);
+HIDE void sppp_ipcp_destroy(struct sppp *sp);
HIDE void sppp_ipcp_up(struct sppp *sp);
HIDE void sppp_ipcp_down(struct sppp *sp);
HIDE void sppp_ipcp_open(struct sppp *sp);
@@ -306,6 +302,7 @@ HIDE void sppp_ipcp_tlf(struct sppp *sp)
HIDE void sppp_ipcp_scr(struct sppp *sp);
HIDE void sppp_ipv6cp_init(struct sppp *sp);
+HIDE void sppp_ipv6cp_destroy(struct sppp *sp);
HIDE void sppp_ipv6cp_up(struct sppp *sp);
HIDE void sppp_ipv6cp_down(struct sppp *sp);
HIDE void sppp_ipv6cp_open(struct sppp *sp);
@@ -902,6 +899,9 @@ sppp_detach(struct ifnet *ifp)
struct sppp **q, *p, *sp = (struct sppp*) ifp;
int i;
+ sppp_ipcp_destroy(sp);
+ sppp_ipv6cp_destroy(sp);
+
/* Remove the entry from the keepalive list. */
for (q = spppq; (p = *q); q = p-pp_next)
if (p == sp) {
@@ -2637,6 +2637,15 @@ sppp_ipcp_init(struct sppp *sp)
sp-ipcp.flags = 0;
sp-state[IDX_IPCP] = STATE_INITIAL;
sp-fail_counter[IDX_IPCP] = 0;
+ task_set(sp-ipcp.set_addr_task, sppp_set_ip_addrs, sp, NULL);
+ task_set(sp-ipcp.clear_addr_task, sppp_clear_ip_addrs, sp, NULL);
+}
+
+HIDE void
+sppp_ipcp_destroy(struct sppp *sp)
+{
+ task_del(systq, sp-ipcp.set_addr_task);
+ task_del(systq, sp-ipcp.clear_addr_task);
}
HIDE void
@@ -2955,38 +2964,11 @@ sppp_ipcp_RCN_nak(struct sppp *sp, struc
addlog(\n);
}
-struct sppp_set_ip_addrs_args {
- struct sppp *sp;
- u_int32_t myaddr;
- u_int32_t hisaddr;
-};
-
HIDE void
sppp_ipcp_tlu(struct sppp *sp)
{
- struct ifnet *ifp = sp-pp_if;
- struct sppp_set_ip_addrs_args *args;
-
- args = malloc(sizeof(*args), M_TEMP, M_NOWAIT);
- if (args == NULL)
- return;
-
- args-sp = sp;
-
- /* we are up. Set addresses and notify anyone interested */
- sppp_get_ip_addrs(sp, args-myaddr, args-hisaddr, 0);
- if ((sp-ipcp.flags IPCP_MYADDR_DYN)
- (sp-ipcp.flags IPCP_MYADDR_SEEN))
- args-myaddr = sp-ipcp.req_myaddr;
- if ((sp-ipcp.flags IPCP_HISADDR_DYN)
- (sp-ipcp.flags IPCP_HISADDR_SEEN))
- args-hisaddr = sp-ipcp.req_hisaddr;
-
- if (workq_add_task(NULL, 0, sppp_set_ip_addrs, args, NULL)) {
- free(args, M_TEMP);
- printf(%s: workq_add_task failed, cannot set
- addresses\n, ifp-if_xname);
- }
+ if (sp-ipcp.req_myaddr
Re: convert sppp(4) to taskq
On 15 November 2013 15:45, Stefan Sperling s...@openbsd.org wrote: On Fri, Nov 15, 2013 at 03:20:48PM +0100, Mike Belopuhov wrote: On 15 November 2013 15:13, Stefan Sperling s...@openbsd.org wrote: Is this done right? Works here with pppoe(4) for both IPv4 and IPv6. i think this diff might lack task_del's in the detach code. Ooops, good catch. have you tried destroying your pppoe interface? Yes, but evidently not while a task was scheduled. Same diff with task_del calls added. looks fine to me so far.
