Date: Mon, 2 Apr 2012 08:48:04 -0400 From: John Baldwin <jhb@freebsd.org> To: Mikolaj Golub <to.my.trociny@gmail.com> Cc: svn-src-head@freebsd.org, Bjoern Zeeb <bz@freebsd.org>, svn-src-all@freebsd.org, src-committers@freebsd.org Subject: Re: svn commit: r228969 - head/sys/netinet Message-ID: <201204020848.04775.jhb@freebsd.org> In-Reply-To: <CAOnPXZ-JfJK-mJSSYpt3o4K5AD2rCXM3yXY6pXqEU_TfWMy%2BkA@mail.gmail.com> References: <201112292041.pBTKfGkj071711@svn.freebsd.org> <CAOnPXZ-JfJK-mJSSYpt3o4K5AD2rCXM3yXY6pXqEU_TfWMy%2BkA@mail.gmail.com>
next in thread | previous in thread | raw e-mail | index | archive | help
On Sunday, April 01, 2012 8:05:00 am Mikolaj Golub wrote: > Hi, > > On 12/29/11, John Baldwin <jhb@freebsd.org> wrote: > > Author: jhb > > Date: Thu Dec 29 20:41:16 2011 > > New Revision: 228969 > > URL: http://svn.freebsd.org/changeset/base/228969 > > > > Log: > > Defer the work of freeing IPv4 multicast options from a socket to an > > asychronous task. This avoids tearing down multicast state including > > sending IGMP leave messages and reprogramming MAC filters while holding > > the per-protocol global pcbinfo lock that is used in the receive path of > > packet processing. > > > > Reviewed by: rwatson > > MFC after: 1 month > > > > Modified: > > head/sys/netinet/in_mcast.c > > head/sys/netinet/ip_var.h > > > > Modified: head/sys/netinet/in_mcast.c > > ============================================================================== > > --- head/sys/netinet/in_mcast.c Thu Dec 29 19:01:29 2011 (r228968) > > +++ head/sys/netinet/in_mcast.c Thu Dec 29 20:41:16 2011 (r228969) > > @@ -46,6 +46,7 @@ __FBSDID("$FreeBSD$"); > > #include <sys/protosw.h> > > #include <sys/sysctl.h> > > #include <sys/ktr.h> > > +#include <sys/taskqueue.h> > > #include <sys/tree.h> > > > > #include <net/if.h> > > @@ -144,6 +145,8 @@ static void inm_purge(struct in_multi *) > > static void inm_reap(struct in_multi *); > > static struct ip_moptions * > > inp_findmoptions(struct inpcb *); > > +static void inp_freemoptions_internal(struct ip_moptions *); > > +static void inp_gcmoptions(void *, int); > > static int inp_get_source_filters(struct inpcb *, struct sockopt *); > > static int inp_join_group(struct inpcb *, struct sockopt *); > > static int inp_leave_group(struct inpcb *, struct sockopt *); > > @@ -179,6 +182,10 @@ static SYSCTL_NODE(_net_inet_ip_mcast, O > > CTLFLAG_RD | CTLFLAG_MPSAFE, sysctl_ip_mcast_filters, > > "Per-interface stack-wide source filters"); > > > > +static STAILQ_HEAD(, ip_moptions) imo_gc_list = > > + STAILQ_HEAD_INITIALIZER(imo_gc_list); > > +static struct task imo_gc_task = TASK_INITIALIZER(0, inp_gcmoptions, NULL); > > + > > /* > > * Inline function which wraps assertions for a valid ifp. > > * The ifnet layer will set the ifma's ifp pointer to NULL if the ifp > > @@ -1518,17 +1525,29 @@ inp_findmoptions(struct inpcb *inp) > > } > > > > /* > > - * Discard the IP multicast options (and source filters). > > + * Discard the IP multicast options (and source filters). To minimize > > + * the amount of work done while holding locks such as the INP's > > + * pcbinfo lock (which is used in the receive path), the free > > + * operation is performed asynchronously in a separate task. > > * > > * SMPng: NOTE: assumes INP write lock is held. > > */ > > void > > inp_freemoptions(struct ip_moptions *imo) > > { > > - struct in_mfilter *imf; > > - size_t idx, nmships; > > > > KASSERT(imo != NULL, ("%s: ip_moptions is NULL", __func__)); > > + IN_MULTI_LOCK(); > > + STAILQ_INSERT_TAIL(&imo_gc_list, imo, imo_link); > > + IN_MULTI_UNLOCK(); > > + taskqueue_enqueue(taskqueue_thread, &imo_gc_task); > > +} > > + > > +static void > > +inp_freemoptions_internal(struct ip_moptions *imo) > > +{ > > + struct in_mfilter *imf; > > + size_t idx, nmships; > > > > nmships = imo->imo_num_memberships; > > for (idx = 0; idx < nmships; ++idx) { > > @@ -1546,6 +1565,22 @@ inp_freemoptions(struct ip_moptions *imo > > free(imo, M_IPMOPTS); > > } > > > > +static void > > +inp_gcmoptions(void *context, int pending) > > +{ > > + struct ip_moptions *imo; > > + > > + IN_MULTI_LOCK(); > > + while (!STAILQ_EMPTY(&imo_gc_list)) { > > + imo = STAILQ_FIRST(&imo_gc_list); > > + STAILQ_REMOVE_HEAD(&imo_gc_list, imo_link); > > + IN_MULTI_UNLOCK(); > > + inp_freemoptions_internal(imo); > > + IN_MULTI_LOCK(); > > + } > > + IN_MULTI_UNLOCK(); > > +} > > + > > /* > > * Atomically get source filters on a socket for an IPv4 multicast group. > > * Called with INP lock held; returns with lock released. > > > > Modified: head/sys/netinet/ip_var.h > > ============================================================================== > > --- head/sys/netinet/ip_var.h Thu Dec 29 19:01:29 2011 (r228968) > > +++ head/sys/netinet/ip_var.h Thu Dec 29 20:41:16 2011 (r228969) > > @@ -93,6 +93,7 @@ struct ip_moptions { > > u_short imo_max_memberships; /* max memberships this socket */ > > struct in_multi **imo_membership; /* group memberships */ > > struct in_mfilter *imo_mfilters; /* source filters */ > > + STAILQ_ENTRY(ip_moptions) imo_link; > > }; > > > > struct ipstat { > > > > I have been observing panics like below after recent upgrade on VIMAGE kernel: > > #0 doadump (textdump=-2022567936) at pcpu.h:244 > #1 0x8051b739 in db_fncall (dummy1=1, dummy2=0, dummy3=-2127531040, > dummy4=0x872b2920 "") > at /home/golub/freebsd/base/head/sys/ddb/db_command.c:573 > #2 0x8051bb31 in db_command (last_cmdp=0x8112eefc, cmd_table=0x0, dopager=1) > at /home/golub/freebsd/base/head/sys/ddb/db_command.c:449 > #3 0x8051bc8a in db_command_loop () at > /home/golub/freebsd/base/head/sys/ddb/db_command.c:502 > #4 0x8051dc7d in db_trap (type=12, code=0) > at /home/golub/freebsd/base/head/sys/ddb/db_main.c:229 > #5 0x80a82566 in kdb_trap (type=12, code=0, tf=0x872b2bbc) > at /home/golub/freebsd/base/head/sys/kern/subr_kdb.c:629 > #6 0x80ddd26f in trap_fatal (frame=0x872b2bbc, eva=24) > at /home/golub/freebsd/base/head/sys/i386/i386/trap.c:1014 > #7 0x80ddd347 in trap_pfault (frame=0x872b2bbc, usermode=0, eva=24) > at /home/golub/freebsd/base/head/sys/i386/i386/trap.c:835 > #8 0x80dde411 in trap (frame=0x872b2bbc) > at /home/golub/freebsd/base/head/sys/i386/i386/trap.c:547 > #9 0x80dc7c6c in calltrap () at > /home/golub/freebsd/base/head/sys/i386/i386/exception.s:169 > #10 0x80b6f1fd in igmp_change_state (inm=0x8ae70480) > at /home/golub/freebsd/base/head/sys/netinet/igmp.c:2595 > #11 0x80b76f68 in in_leavegroup_locked (inm=0x8ae70480, imf=0x8a655a00) > at /home/golub/freebsd/base/head/sys/netinet/in_mcast.c:1239 > #12 0x80b76fbd in in_leavegroup (inm=0x8ae70480, imf=0x8a655a00) > at /home/golub/freebsd/base/head/sys/netinet/in_mcast.c:1184 > #13 0x80b770b4 in inp_gcmoptions (context=0x0, pending=1) > at /home/golub/freebsd/base/head/sys/netinet/in_mcast.c:1554 > #14 0x80a8ff2b in taskqueue_run_locked (queue=0x87594880) > at /home/golub/freebsd/base/head/sys/kern/subr_taskqueue.c:308 > #15 0x80a90987 in taskqueue_thread_loop (arg=0x81186bcc) > at /home/golub/freebsd/base/head/sys/kern/subr_taskqueue.c:497 > #16 0x80a1b2d8 in fork_exit (callout=0x80a90920 > <taskqueue_thread_loop>, arg=0x81186bcc, > frame=0x872b2d28) at /home/golub/freebsd/base/head/sys/kern/kern_fork.c:992 > #17 0x80dc7d14 in fork_trampoline () > at /home/golub/freebsd/base/head/sys/i386/i386/exception.s:276 > (kgdb) fr 10 > #10 0x80b6f1fd in igmp_change_state (inm=0x8ae70480) > at /home/golub/freebsd/base/head/sys/netinet/igmp.c:2595 > 2595 V_state_change_timers_running = 1; > > (kgdb) l > 2590 ("%s: enqueue record = > %d", __func__, > 2591 retval)); > 2592 > 2593 inm->inm_state = IGMP_LEAVING_MEMBER; > 2594 inm->inm_sctimer = 1; > 2595 V_state_change_timers_running = 1; > 2596 syncstates = 0; > 2597 } > 2598 break; > 2599 } > > VNET context is not set at that point. > > The attached patch fixes the issue for me. Not sure about inm->inm_ifp > != NULL assumption but I need interface to get vnet :-). I will to defer to bz@ (cc'd) on how best to fix this. Another option would be to save the current vnet in the 'ip_moptions' struct (would have to add a new field) when queueing this imo to be free'd via the task. You could then do the curvnet set/restore at a higher level without any locks held, etc. > BTW, in igmp_change_state() this looks for me a bit strange: > > if (ifp != NULL) { > /* > * Sanity check that netinet's notion of ifp is the > * same as net's. > */ > KASSERT(inm->inm_ifp == ifp, ("%s: bad ifp", __func__)); > } > > IGMP_LOCK(); > > igi = ((struct in_ifinfo *)ifp->if_afdata[AF_INET])->ii_igmp; > > The check ifp != NULL suggests that ifp may be NULL, but then it will > panic at the last shown line. Yes, it seems that the check should be removed and the KASSERT() should just always fire. -- John Baldwin
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?201204020848.04775.jhb>