convert sppp(4) to taskq

classic Classic list List threaded Threaded
8 messages Options
Reply | Threaded
Open this post in threaded view
|

convert sppp(4) to taskq

Stefan Sperling-8
Is this done right?

Works here with pppoe(4) for both IPv4 and IPv6.

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 -0000 1.19
+++ if_sppp.h 15 Nov 2013 13:23:18 -0000
@@ -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 -0000 1.112
+++ if_spppsubr.c 15 Nov 2013 13:24:45 -0000
@@ -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))
 
@@ -2637,6 +2632,8 @@ 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
@@ -2955,38 +2952,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 != 0 || sp->ipcp.req_hisaddr != 0)
+ task_add(systq, &sp->ipcp.set_addr_task);
 }
 
 HIDE void
@@ -3043,15 +3013,9 @@ sppp_ipcp_tls(struct sppp *sp)
 HIDE void
 sppp_ipcp_tlf(struct sppp *sp)
 {
- struct ifnet *ifp = &sp->pp_if;
-
  if (sp->ipcp.flags & (IPCP_MYADDR_DYN|IPCP_HISADDR_DYN))
  /* Some address was dynamic, clear it again. */
- if (workq_add_task(NULL, 0,
-    sppp_clear_ip_addrs, (void *)sp, NULL)) {
- printf("%s: workq_add_task failed, cannot clear "
-    "addresses\n", ifp->if_xname);
- }
+ task_add(systq, &sp->ipcp.clear_addr_task);
 
  /* we no longer need LCP */
  sp->lcp.protos &= ~(1 << IDX_IPCP);
@@ -3110,6 +3074,8 @@ sppp_ipv6cp_init(struct sppp *sp)
  sp->ipv6cp.flags = 0;
  sp->state[IDX_IPV6CP] = STATE_INITIAL;
  sp->fail_counter[IDX_IPV6CP] = 0;
+ task_set(&sp->ipv6cp.set_addr_task, sppp_update_ip6_addr, sp,
+    &sp->ipv6cp.req_ifid);
 }
 
 HIDE void
@@ -4587,16 +4553,15 @@ sppp_update_gw(struct ifnet *ifp)
 }
 
 /*
- * Work queue task adding addresses from process context.
+ * Task adding addresses from process context.
  * If an address is 0, leave it the way it is.
  */
 HIDE void
 sppp_set_ip_addrs(void *arg1, void *arg2)
 {
- struct sppp_set_ip_addrs_args *args = arg1;
- struct sppp *sp = args->sp;
- u_int32_t myaddr = args->myaddr;
- u_int32_t hisaddr = args->hisaddr;
+ struct sppp *sp = arg1;
+ u_int32_t myaddr;
+ u_int32_t hisaddr;
  struct ifnet *ifp = &sp->pp_if;
  int debug = ifp->if_flags & IFF_DEBUG;
  struct ifaddr *ifa;
@@ -4604,8 +4569,13 @@ sppp_set_ip_addrs(void *arg1, void *arg2
  struct sockaddr_in *dest;
  int s;
 
- /* Arguments are now on local stack so free temporary storage. */
- free(args, M_TEMP);
+ sppp_get_ip_addrs(sp, &myaddr, &hisaddr, NULL);
+ if ((sp->ipcp.flags & IPCP_MYADDR_DYN) &&
+    (sp->ipcp.flags & IPCP_MYADDR_SEEN))
+ myaddr = sp->ipcp.req_myaddr;
+ if ((sp->ipcp.flags & IPCP_HISADDR_DYN) &&
+    (sp->ipcp.flags & IPCP_HISADDR_SEEN))
+ hisaddr = sp->ipcp.req_hisaddr;
 
  s = splsoftnet();
 
@@ -4656,7 +4626,7 @@ sppp_set_ip_addrs(void *arg1, void *arg2
 }
 
 /*
- * Work queue task clearing addresses from process context.
+ * Task clearing addresses from process context.
  * Clear IP addresses.
  */
 HIDE void
@@ -4772,7 +4742,6 @@ sppp_update_ip6_addr(void *arg1, void *a
  if (ia == NULL) {
  /* IPv6 disabled? */
  splx(s);
- free(ifra, M_TEMP);
  return;
  }
 
@@ -4791,7 +4760,6 @@ sppp_update_ip6_addr(void *arg1, void *a
     SPP_ARGS(ifp), error);
  }
  splx(s);
- free(ifra, M_TEMP);
 }
 
 /*
@@ -4802,12 +4770,9 @@ sppp_set_ip6_addr(struct sppp *sp, const
  const struct in6_addr *dst)
 {
  struct ifnet *ifp = &sp->pp_if;
- struct in6_aliasreq *ifra;
-
- ifra = malloc(sizeof(*ifra), M_TEMP, M_NOWAIT|M_ZERO);
- if (ifra == NULL)
- return;
+ struct in6_aliasreq *ifra = &sp->ipv6cp.req_ifid;
 
+ bzero(ifra, sizeof(*ifra));
  bcopy(ifp->if_xname, ifra->ifra_name, sizeof(ifra->ifra_name));
 
  ifra->ifra_addr.sin6_len = sizeof(struct sockaddr_in6);
@@ -4831,11 +4796,7 @@ sppp_set_ip6_addr(struct sppp *sp, const
  /* DAD is redundant after an IPv6CP exchange. */
  ifra->ifra_flags |= IN6_IFF_NODAD;
 
- if (workq_add_task(NULL, 0, sppp_update_ip6_addr, sp, ifra)) {
- free(ifra, M_TEMP);
- printf("%s: workq_add_task failed, cannot set IPv6 "
-    "addresses\n", ifp->if_xname);
- }
+ task_add(systq, &sp->ipv6cp.set_addr_task);
 }
 
 /*

Reply | Threaded
Open this post in threaded view
|

Re: convert sppp(4) to taskq

Mike Belopuhov-5
On 15 November 2013 15:13, Stefan Sperling <[hidden email]> 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?

Reply | Threaded
Open this post in threaded view
|

Re: convert sppp(4) to taskq

Stefan Sperling-8
On Fri, Nov 15, 2013 at 03:20:48PM +0100, Mike Belopuhov wrote:
> On 15 November 2013 15:13, Stefan Sperling <[hidden email]> 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 -0000 1.19
+++ if_sppp.h 15 Nov 2013 13:48:47 -0000
@@ -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 -0000 1.112
+++ if_spppsubr.c 15 Nov 2013 14:37:24 -0000
@@ -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 != 0 || sp->ipcp.req_hisaddr != 0)
+ task_add(systq, &sp->ipcp.set_addr_task);
 }
 
 HIDE void
@@ -3043,15 +3025,9 @@ sppp_ipcp_tls(struct sppp *sp)
 HIDE void
 sppp_ipcp_tlf(struct sppp *sp)
 {
- struct ifnet *ifp = &sp->pp_if;
-
  if (sp->ipcp.flags & (IPCP_MYADDR_DYN|IPCP_HISADDR_DYN))
  /* Some address was dynamic, clear it again. */
- if (workq_add_task(NULL, 0,
-    sppp_clear_ip_addrs, (void *)sp, NULL)) {
- printf("%s: workq_add_task failed, cannot clear "
-    "addresses\n", ifp->if_xname);
- }
+ task_add(systq, &sp->ipcp.clear_addr_task);
 
  /* we no longer need LCP */
  sp->lcp.protos &= ~(1 << IDX_IPCP);
@@ -3110,6 +3086,14 @@ sppp_ipv6cp_init(struct sppp *sp)
  sp->ipv6cp.flags = 0;
  sp->state[IDX_IPV6CP] = STATE_INITIAL;
  sp->fail_counter[IDX_IPV6CP] = 0;
+ task_set(&sp->ipv6cp.set_addr_task, sppp_update_ip6_addr, sp,
+    &sp->ipv6cp.req_ifid);
+}
+
+HIDE void
+sppp_ipv6cp_destroy(struct sppp *sp)
+{
+ task_del(systq, &sp->ipv6cp.set_addr_task);
 }
 
 HIDE void
@@ -3498,6 +3482,11 @@ sppp_ipv6cp_init(struct sppp *sp)
 }
 
 HIDE void
+sppp_ipv6cp_destroy(struct sppp *sp)
+{
+}
+
+HIDE void
 sppp_ipv6cp_up(struct sppp *sp)
 {
 }
@@ -4587,16 +4576,15 @@ sppp_update_gw(struct ifnet *ifp)
 }
 
 /*
- * Work queue task adding addresses from process context.
+ * Task adding addresses from process context.
  * If an address is 0, leave it the way it is.
  */
 HIDE void
 sppp_set_ip_addrs(void *arg1, void *arg2)
 {
- struct sppp_set_ip_addrs_args *args = arg1;
- struct sppp *sp = args->sp;
- u_int32_t myaddr = args->myaddr;
- u_int32_t hisaddr = args->hisaddr;
+ struct sppp *sp = arg1;
+ u_int32_t myaddr;
+ u_int32_t hisaddr;
  struct ifnet *ifp = &sp->pp_if;
  int debug = ifp->if_flags & IFF_DEBUG;
  struct ifaddr *ifa;
@@ -4604,8 +4592,13 @@ sppp_set_ip_addrs(void *arg1, void *arg2
  struct sockaddr_in *dest;
  int s;
 
- /* Arguments are now on local stack so free temporary storage. */
- free(args, M_TEMP);
+ sppp_get_ip_addrs(sp, &myaddr, &hisaddr, NULL);
+ if ((sp->ipcp.flags & IPCP_MYADDR_DYN) &&
+    (sp->ipcp.flags & IPCP_MYADDR_SEEN))
+ myaddr = sp->ipcp.req_myaddr;
+ if ((sp->ipcp.flags & IPCP_HISADDR_DYN) &&
+    (sp->ipcp.flags & IPCP_HISADDR_SEEN))
+ hisaddr = sp->ipcp.req_hisaddr;
 
  s = splsoftnet();
 
@@ -4656,7 +4649,7 @@ sppp_set_ip_addrs(void *arg1, void *arg2
 }
 
 /*
- * Work queue task clearing addresses from process context.
+ * Task clearing addresses from process context.
  * Clear IP addresses.
  */
 HIDE void
@@ -4772,7 +4765,6 @@ sppp_update_ip6_addr(void *arg1, void *a
  if (ia == NULL) {
  /* IPv6 disabled? */
  splx(s);
- free(ifra, M_TEMP);
  return;
  }
 
@@ -4791,7 +4783,6 @@ sppp_update_ip6_addr(void *arg1, void *a
     SPP_ARGS(ifp), error);
  }
  splx(s);
- free(ifra, M_TEMP);
 }
 
 /*
@@ -4802,12 +4793,9 @@ sppp_set_ip6_addr(struct sppp *sp, const
  const struct in6_addr *dst)
 {
  struct ifnet *ifp = &sp->pp_if;
- struct in6_aliasreq *ifra;
-
- ifra = malloc(sizeof(*ifra), M_TEMP, M_NOWAIT|M_ZERO);
- if (ifra == NULL)
- return;
+ struct in6_aliasreq *ifra = &sp->ipv6cp.req_ifid;
 
+ bzero(ifra, sizeof(*ifra));
  bcopy(ifp->if_xname, ifra->ifra_name, sizeof(ifra->ifra_name));
 
  ifra->ifra_addr.sin6_len = sizeof(struct sockaddr_in6);
@@ -4831,11 +4819,7 @@ sppp_set_ip6_addr(struct sppp *sp, const
  /* DAD is redundant after an IPv6CP exchange. */
  ifra->ifra_flags |= IN6_IFF_NODAD;
 
- if (workq_add_task(NULL, 0, sppp_update_ip6_addr, sp, ifra)) {
- free(ifra, M_TEMP);
- printf("%s: workq_add_task failed, cannot set IPv6 "
-    "addresses\n", ifp->if_xname);
- }
+ task_add(systq, &sp->ipv6cp.set_addr_task);
 }
 
 /*

Reply | Threaded
Open this post in threaded view
|

Re: convert sppp(4) to taskq

Mike Belopuhov-5
On 15 November 2013 15:45, Stefan Sperling <[hidden email]> wrote:

> On Fri, Nov 15, 2013 at 03:20:48PM +0100, Mike Belopuhov wrote:
>> On 15 November 2013 15:13, Stefan Sperling <[hidden email]> 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.

Reply | Threaded
Open this post in threaded view
|

Re: convert sppp(4) to taskq

Martin Pieuchot-2
In reply to this post by Stefan Sperling-8
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 <[hidden email]> 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 -0000 1.19
> +++ if_sppp.h 15 Nov 2013 13:48:47 -0000
> @@ -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 -0000 1.112
> +++ if_spppsubr.c 15 Nov 2013 14:37:24 -0000
> @@ -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 != 0 || sp->ipcp.req_hisaddr != 0)
> + task_add(systq, &sp->ipcp.set_addr_task);
>  }
>  
>  HIDE void
> @@ -3043,15 +3025,9 @@ sppp_ipcp_tls(struct sppp *sp)
>  HIDE void
>  sppp_ipcp_tlf(struct sppp *sp)
>  {
> - struct ifnet *ifp = &sp->pp_if;
> -
>   if (sp->ipcp.flags & (IPCP_MYADDR_DYN|IPCP_HISADDR_DYN))
>   /* Some address was dynamic, clear it again. */
> - if (workq_add_task(NULL, 0,
> -    sppp_clear_ip_addrs, (void *)sp, NULL)) {
> - printf("%s: workq_add_task failed, cannot clear "
> -    "addresses\n", ifp->if_xname);
> - }
> + task_add(systq, &sp->ipcp.clear_addr_task);
>  
>   /* we no longer need LCP */
>   sp->lcp.protos &= ~(1 << IDX_IPCP);
> @@ -3110,6 +3086,14 @@ sppp_ipv6cp_init(struct sppp *sp)
>   sp->ipv6cp.flags = 0;
>   sp->state[IDX_IPV6CP] = STATE_INITIAL;
>   sp->fail_counter[IDX_IPV6CP] = 0;
> + task_set(&sp->ipv6cp.set_addr_task, sppp_update_ip6_addr, sp,
> +    &sp->ipv6cp.req_ifid);
> +}
> +
> +HIDE void
> +sppp_ipv6cp_destroy(struct sppp *sp)
> +{
> + task_del(systq, &sp->ipv6cp.set_addr_task);
>  }
>  
>  HIDE void
> @@ -3498,6 +3482,11 @@ sppp_ipv6cp_init(struct sppp *sp)
>  }
>  
>  HIDE void
> +sppp_ipv6cp_destroy(struct sppp *sp)
> +{
> +}
> +
> +HIDE void
>  sppp_ipv6cp_up(struct sppp *sp)
>  {
>  }
> @@ -4587,16 +4576,15 @@ sppp_update_gw(struct ifnet *ifp)
>  }
>  
>  /*
> - * Work queue task adding addresses from process context.
> + * Task adding addresses from process context.
>   * If an address is 0, leave it the way it is.
>   */
>  HIDE void
>  sppp_set_ip_addrs(void *arg1, void *arg2)
>  {
> - struct sppp_set_ip_addrs_args *args = arg1;
> - struct sppp *sp = args->sp;
> - u_int32_t myaddr = args->myaddr;
> - u_int32_t hisaddr = args->hisaddr;
> + struct sppp *sp = arg1;
> + u_int32_t myaddr;
> + u_int32_t hisaddr;
>   struct ifnet *ifp = &sp->pp_if;
>   int debug = ifp->if_flags & IFF_DEBUG;
>   struct ifaddr *ifa;
> @@ -4604,8 +4592,13 @@ sppp_set_ip_addrs(void *arg1, void *arg2
>   struct sockaddr_in *dest;
>   int s;
>  
> - /* Arguments are now on local stack so free temporary storage. */
> - free(args, M_TEMP);
> + sppp_get_ip_addrs(sp, &myaddr, &hisaddr, NULL);
> + if ((sp->ipcp.flags & IPCP_MYADDR_DYN) &&
> +    (sp->ipcp.flags & IPCP_MYADDR_SEEN))
> + myaddr = sp->ipcp.req_myaddr;
> + if ((sp->ipcp.flags & IPCP_HISADDR_DYN) &&
> +    (sp->ipcp.flags & IPCP_HISADDR_SEEN))
> + hisaddr = sp->ipcp.req_hisaddr;
>  
>   s = splsoftnet();
>  
> @@ -4656,7 +4649,7 @@ sppp_set_ip_addrs(void *arg1, void *arg2
>  }
>  
>  /*
> - * Work queue task clearing addresses from process context.
> + * Task clearing addresses from process context.
>   * Clear IP addresses.
>   */
>  HIDE void
> @@ -4772,7 +4765,6 @@ sppp_update_ip6_addr(void *arg1, void *a
>   if (ia == NULL) {
>   /* IPv6 disabled? */
>   splx(s);
> - free(ifra, M_TEMP);
>   return;
>   }
>  
> @@ -4791,7 +4783,6 @@ sppp_update_ip6_addr(void *arg1, void *a
>      SPP_ARGS(ifp), error);
>   }
>   splx(s);
> - free(ifra, M_TEMP);
>  }
>  
>  /*
> @@ -4802,12 +4793,9 @@ sppp_set_ip6_addr(struct sppp *sp, const
>   const struct in6_addr *dst)
>  {
>   struct ifnet *ifp = &sp->pp_if;
> - struct in6_aliasreq *ifra;
> -
> - ifra = malloc(sizeof(*ifra), M_TEMP, M_NOWAIT|M_ZERO);
> - if (ifra == NULL)
> - return;
> + struct in6_aliasreq *ifra = &sp->ipv6cp.req_ifid;
>  
> + bzero(ifra, sizeof(*ifra));
>   bcopy(ifp->if_xname, ifra->ifra_name, sizeof(ifra->ifra_name));
>  
>   ifra->ifra_addr.sin6_len = sizeof(struct sockaddr_in6);
> @@ -4831,11 +4819,7 @@ sppp_set_ip6_addr(struct sppp *sp, const
>   /* DAD is redundant after an IPv6CP exchange. */
>   ifra->ifra_flags |= IN6_IFF_NODAD;
>  
> - if (workq_add_task(NULL, 0, sppp_update_ip6_addr, sp, ifra)) {
> - free(ifra, M_TEMP);
> - printf("%s: workq_add_task failed, cannot set IPv6 "
> -    "addresses\n", ifp->if_xname);
> - }
> + task_add(systq, &sp->ipv6cp.set_addr_task);
>  }
>  
>  /*
>

Reply | Threaded
Open this post in threaded view
|

Re: convert sppp(4) to taskq

Stefan Sperling-8
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.

Reply | Threaded
Open this post in threaded view
|

Re: convert sppp(4) to taskq

Martin Pieuchot-2
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.

Reply | Threaded
Open this post in threaded view
|

Re: convert sppp(4) to taskq

David Gwynne-5

On 18 Nov 2013, at 11:59 pm, Martin Pieuchot <[hidden email]> wrote:

> 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™ :)

yes there is.

task_del returns 1 if the task was taken off the list. if the task is on the list and task_del returns 1, then you know it wont run in the future. if it was on the list and returns 0, you know its currently running or has run.
 
> 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.

you are free to do that, but i dont see how that solves the problem.

generally a task is doing work on something more than itself, ie, you would pass it a reference to something in a softc anyway so you still cant free the softc until you're sure the task is done or not going to run. except now you dont have a reference to the task so you cant task_del it outside the task handler.

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

this is naive. passing an index doesnt stop an ifp from being freed after you got a pointer to it with if_get() but while you're in the middle of using it in a task. no matter how you pass a handle to an ifp, either via an index or a direct pointer, you still need to lock or reference count access to it.

> But this approach only makes sense if the task storage is not freed when
> the interface is destroyed.

once you task_del you know that the actual struct task memory isnt referenced anywhere and can be safely freed, just like timeouts. it makes no promises about what you passed to task_set though.

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

agreed. where do you want to talk about this stuff?

dlg