Re: svn commit: r340413 - in head/sys: kern net sys
On 11/15/18 2:15 AM, Gleb Smirnoff wrote: I wish to do that, but struct thread is exposed to userland, and all epoch structures are not. There is another way to solve this problem which doesn't involve "struct thread" and is more safe and guards against recursive use of these functions! Can you have a look at this review: https://reviews.freebsd.org/D17996 --HPS ___ svn-src-all@freebsd.org mailing list https://lists.freebsd.org/mailman/listinfo/svn-src-all To unsubscribe, send any mail to "svn-src-all-unsubscr...@freebsd.org"
Re: svn commit: r340413 - in head/sys: kern net sys
On Wed, Nov 14, 2018 at 10:27:48PM +0200, Konstantin Belousov wrote: K> On Wed, Nov 14, 2018 at 08:28:31AM -0800, Gleb Smirnoff wrote: K> > On Wed, Nov 14, 2018 at 11:06:38AM +0100, Hans Petter Selasky wrote: K> > H> On 11/14/18 10:33 AM, Cy Schubert wrote: K> > H> > + epoch_thread_init(td); K> > H> K> > H> Did you forget to call epoch_thread_init() for thread0 ? K> > K> > Yes, this is my guess. I'm preparing patch for Cy to test. K> K> Is this stuff allocated for each thread, unconditionally ? K> If yes, why is it allocatable instead of embedding it into struct thread ? I wish to do that, but struct thread is exposed to userland, and all epoch structures are not. -- Gleb Smirnoff ___ svn-src-all@freebsd.org mailing list https://lists.freebsd.org/mailman/listinfo/svn-src-all To unsubscribe, send any mail to "svn-src-all-unsubscr...@freebsd.org"
Re: svn commit: r340413 - in head/sys: kern net sys
On Wed, Nov 14, 2018 at 08:28:31AM -0800, Gleb Smirnoff wrote: > On Wed, Nov 14, 2018 at 11:06:38AM +0100, Hans Petter Selasky wrote: > H> On 11/14/18 10:33 AM, Cy Schubert wrote: > H> > +epoch_thread_init(td); > H> > H> Did you forget to call epoch_thread_init() for thread0 ? > > Yes, this is my guess. I'm preparing patch for Cy to test. Is this stuff allocated for each thread, unconditionally ? If yes, why is it allocatable instead of embedding it into struct thread ? ___ svn-src-all@freebsd.org mailing list https://lists.freebsd.org/mailman/listinfo/svn-src-all To unsubscribe, send any mail to "svn-src-all-unsubscr...@freebsd.org"
Re: svn commit: r340413 - in head/sys: kern net sys
On Wed, Nov 14, 2018 at 11:06:38AM +0100, Hans Petter Selasky wrote: H> On 11/14/18 10:33 AM, Cy Schubert wrote: H> > + epoch_thread_init(td); H> H> Did you forget to call epoch_thread_init() for thread0 ? Yes, this is my guess. I'm preparing patch for Cy to test. -- Gleb Smirnoff ___ svn-src-all@freebsd.org mailing list https://lists.freebsd.org/mailman/listinfo/svn-src-all To unsubscribe, send any mail to "svn-src-all-unsubscr...@freebsd.org"
Re: svn commit: r340413 - in head/sys: kern net sys
In message <178c3d69-4a3b-49cb-dab4-f6f4139df...@selasky.org>, Hans Petter Sela sky writes: > On 11/14/18 10:33 AM, Cy Schubert wrote: > > + epoch_thread_init(td); > > Did you forget to call epoch_thread_init() for thread0 ? > > --HPS It appears that interfaces that call if_maddr_rlock(ifp) have the issue. This suggests epoch_thread_init() for thread0 hasn't been called yet when interfaces (specifically those that call if_maddr_rlock()) attach. -- Cheers, Cy Schubert FreeBSD UNIX: Web: http://www.FreeBSD.org The need of the many outweighs the greed of the few. ___ svn-src-all@freebsd.org mailing list https://lists.freebsd.org/mailman/listinfo/svn-src-all To unsubscribe, send any mail to "svn-src-all-unsubscr...@freebsd.org"
Re: svn commit: r340413 - in head/sys: kern net sys
On 11/14/18 10:33 AM, Cy Schubert wrote: + epoch_thread_init(td); Did you forget to call epoch_thread_init() for thread0 ? --HPS ___ svn-src-all@freebsd.org mailing list https://lists.freebsd.org/mailman/listinfo/svn-src-all To unsubscribe, send any mail to "svn-src-all-unsubscr...@freebsd.org"
Re: svn commit: r340413 - in head/sys: kern net sys
Sorry. This should have been a private email. But now that it's in the wild, if_sk.c calls epoch_enter_preempt() at line 84 which causes panic early in boot. -- Cheers, Cy Schubert FreeBSD UNIX: Web: http://www.FreeBSD.org The need of the many outweighs the greed of the few. In message <201811140933.wae9xxsl003...@slippy.cwsent.com>, Cy Schubert writes: > In message <201811132258.wadmwctl063...@repo.freebsd.org>, Gleb > Smirnoff writes > : > > Author: glebius > > Date: Tue Nov 13 22:58:38 2018 > > New Revision: 340413 > > URL: https://svnweb.freebsd.org/changeset/base/340413 > > > > Log: > > For compatibility KPI functions like if_addr_rlock() that used to have > > mutexes but now are converted to epoch(9) use thread-private epoch_tracke > r. > > Embedding tracker into ifnet(9) or ifnet derived structures creates a non > > reentrable function, that will fail miserably if called simultaneously fr > om > > two different contexts. > > A thread private tracker will provide a single tracker that would allow t > o > > call these functions safely. It doesn't allow nested call, but this is no > t > > expected from compatibility KPIs. > > > > Reviewed by: markj > > > > Modified: > > head/sys/kern/kern_thread.c > > head/sys/kern/subr_epoch.c > > head/sys/net/if.c > > head/sys/net/if_var.h > > head/sys/sys/epoch.h > > head/sys/sys/proc.h > > > > Modified: head/sys/kern/kern_thread.c > > === > == > > = > > --- head/sys/kern/kern_thread.c Tue Nov 13 22:41:20 2018(r34041 > > 2) > > +++ head/sys/kern/kern_thread.c Tue Nov 13 22:58:38 2018(r34041 > > 3) > > @@ -272,6 +272,7 @@ thread_init(void *mem, int size, int flags) > > td->td_rlqe = NULL; > > EVENTHANDLER_DIRECT_INVOKE(thread_init, td); > > umtx_thread_init(td); > > + epoch_thread_init(td); > > td->td_kstack = 0; > > td->td_sel = NULL; > > return (0); > > @@ -291,6 +292,7 @@ thread_fini(void *mem, int size) > > turnstile_free(td->td_turnstile); > > sleepq_free(td->td_sleepqueue); > > umtx_thread_fini(td); > > + epoch_thread_fini(td); > > seltdfini(td); > > } > > > > > > Modified: head/sys/kern/subr_epoch.c > > === > == > > = > > --- head/sys/kern/subr_epoch.c Tue Nov 13 22:41:20 2018(r34041 > > 2) > > +++ head/sys/kern/subr_epoch.c Tue Nov 13 22:58:38 2018(r34041 > > 3) > > @@ -667,3 +667,17 @@ in_epoch(epoch_t epoch) > > { > > return (in_epoch_verbose(epoch, 0)); > > } > > + > > +void > > +epoch_thread_init(struct thread *td) > > +{ > > + > > + td->td_et = malloc(sizeof(struct epoch_tracker), M_EPOCH, M_WAITOK); > > +} > > + > > +void > > +epoch_thread_fini(struct thread *td) > > +{ > > + > > + free(td->td_et, M_EPOCH); > > +} > > > > Modified: head/sys/net/if.c > > === > == > > = > > --- head/sys/net/if.c Tue Nov 13 22:41:20 2018(r340412) > > +++ head/sys/net/if.c Tue Nov 13 22:58:38 2018(r340413) > > @@ -1767,35 +1767,29 @@ if_data_copy(struct ifnet *ifp, struct if_data *ifd > ) > > void > > if_addr_rlock(struct ifnet *ifp) > > { > > - MPASS(*(uint64_t *)&ifp->if_addr_et == 0); > > - epoch_enter_preempt(net_epoch_preempt, &ifp->if_addr_et); > > + > > + epoch_enter_preempt(net_epoch_preempt, curthread->td_et); > > } > > > > void > > if_addr_runlock(struct ifnet *ifp) > > { > > - epoch_exit_preempt(net_epoch_preempt, &ifp->if_addr_et); > > -#ifdef INVARIANTS > > - bzero(&ifp->if_addr_et, sizeof(struct epoch_tracker)); > > -#endif > > + > > + epoch_exit_preempt(net_epoch_preempt, curthread->td_et); > > } > > > > void > > if_maddr_rlock(if_t ifp) > > { > > > > - MPASS(*(uint64_t *)&ifp->if_maddr_et == 0); > > - epoch_enter_preempt(net_epoch_preempt, &ifp->if_maddr_et); > > + epoch_enter_preempt(net_epoch_preempt, curthread->td_et); > > > Hi Gleb, > > I was wrong. It's happening here, called from line 084 in if_sk.c. > > ~cy > > > } > > > > void > > if_maddr_runlock(if_t ifp) > > { > > > > - epoch_exit_preempt(net_epoch_preempt, &ifp->if_maddr_et); > > -#ifdef INVARIANTS > > - bzero(&ifp->if_maddr_et, sizeof(struct epoch_tracker)); > > -#endif > > + epoch_exit_preempt(net_epoch_preempt, curthread->td_et); > > } > > > > /* > > > > Modified: head/sys/net/if_var.h > > === > == > > = > > --- head/sys/net/if_var.h Tue Nov 13 22:41:20 2018(r340412) > > +++ head/sys/net/if_var.h Tue Nov 13 22:58:38 2018(r340413) > > @@ -381,8 +381,6 @@ struct ifnet { > > */ > > struct netdump_methods *if_netdump_methods; > > struct epoch_contextif_epoch_ctx; > > - struct epoch_trackerif_addr_et; > > - struct epoch_tracke
Re: svn commit: r340413 - in head/sys: kern net sys
In message <201811132258.wadmwctl063...@repo.freebsd.org>, Gleb Smirnoff writes : > Author: glebius > Date: Tue Nov 13 22:58:38 2018 > New Revision: 340413 > URL: https://svnweb.freebsd.org/changeset/base/340413 > > Log: > For compatibility KPI functions like if_addr_rlock() that used to have > mutexes but now are converted to epoch(9) use thread-private epoch_tracker. > Embedding tracker into ifnet(9) or ifnet derived structures creates a non > reentrable function, that will fail miserably if called simultaneously from > two different contexts. > A thread private tracker will provide a single tracker that would allow to > call these functions safely. It doesn't allow nested call, but this is not > expected from compatibility KPIs. > > Reviewed by:markj > > Modified: > head/sys/kern/kern_thread.c > head/sys/kern/subr_epoch.c > head/sys/net/if.c > head/sys/net/if_var.h > head/sys/sys/epoch.h > head/sys/sys/proc.h > > Modified: head/sys/kern/kern_thread.c > = > = > --- head/sys/kern/kern_thread.c Tue Nov 13 22:41:20 2018(r34041 > 2) > +++ head/sys/kern/kern_thread.c Tue Nov 13 22:58:38 2018(r34041 > 3) > @@ -272,6 +272,7 @@ thread_init(void *mem, int size, int flags) > td->td_rlqe = NULL; > EVENTHANDLER_DIRECT_INVOKE(thread_init, td); > umtx_thread_init(td); > + epoch_thread_init(td); > td->td_kstack = 0; > td->td_sel = NULL; > return (0); > @@ -291,6 +292,7 @@ thread_fini(void *mem, int size) > turnstile_free(td->td_turnstile); > sleepq_free(td->td_sleepqueue); > umtx_thread_fini(td); > + epoch_thread_fini(td); > seltdfini(td); > } > > > Modified: head/sys/kern/subr_epoch.c > = > = > --- head/sys/kern/subr_epoch.cTue Nov 13 22:41:20 2018(r34041 > 2) > +++ head/sys/kern/subr_epoch.cTue Nov 13 22:58:38 2018(r34041 > 3) > @@ -667,3 +667,17 @@ in_epoch(epoch_t epoch) > { > return (in_epoch_verbose(epoch, 0)); > } > + > +void > +epoch_thread_init(struct thread *td) > +{ > + > + td->td_et = malloc(sizeof(struct epoch_tracker), M_EPOCH, M_WAITOK); > +} > + > +void > +epoch_thread_fini(struct thread *td) > +{ > + > + free(td->td_et, M_EPOCH); > +} > > Modified: head/sys/net/if.c > = > = > --- head/sys/net/if.c Tue Nov 13 22:41:20 2018(r340412) > +++ head/sys/net/if.c Tue Nov 13 22:58:38 2018(r340413) > @@ -1767,35 +1767,29 @@ if_data_copy(struct ifnet *ifp, struct if_data *ifd) > void > if_addr_rlock(struct ifnet *ifp) > { > - MPASS(*(uint64_t *)&ifp->if_addr_et == 0); > - epoch_enter_preempt(net_epoch_preempt, &ifp->if_addr_et); > + > + epoch_enter_preempt(net_epoch_preempt, curthread->td_et); > } > > void > if_addr_runlock(struct ifnet *ifp) > { > - epoch_exit_preempt(net_epoch_preempt, &ifp->if_addr_et); > -#ifdef INVARIANTS > - bzero(&ifp->if_addr_et, sizeof(struct epoch_tracker)); > -#endif > + > + epoch_exit_preempt(net_epoch_preempt, curthread->td_et); > } > > void > if_maddr_rlock(if_t ifp) > { > > - MPASS(*(uint64_t *)&ifp->if_maddr_et == 0); > - epoch_enter_preempt(net_epoch_preempt, &ifp->if_maddr_et); > + epoch_enter_preempt(net_epoch_preempt, curthread->td_et); Hi Gleb, I was wrong. It's happening here, called from line 084 in if_sk.c. ~cy > } > > void > if_maddr_runlock(if_t ifp) > { > > - epoch_exit_preempt(net_epoch_preempt, &ifp->if_maddr_et); > -#ifdef INVARIANTS > - bzero(&ifp->if_maddr_et, sizeof(struct epoch_tracker)); > -#endif > + epoch_exit_preempt(net_epoch_preempt, curthread->td_et); > } > > /* > > Modified: head/sys/net/if_var.h > = > = > --- head/sys/net/if_var.h Tue Nov 13 22:41:20 2018(r340412) > +++ head/sys/net/if_var.h Tue Nov 13 22:58:38 2018(r340413) > @@ -381,8 +381,6 @@ struct ifnet { >*/ > struct netdump_methods *if_netdump_methods; > struct epoch_contextif_epoch_ctx; > - struct epoch_trackerif_addr_et; > - struct epoch_trackerif_maddr_et; > > /* >* Spare fields to be added before branching a stable branch, so > > Modified: head/sys/sys/epoch.h > = > = > --- head/sys/sys/epoch.h Tue Nov 13 22:41:20 2018(r340412) > +++ head/sys/sys/epoch.h Tue Nov 13 22:58:38 2018(r340413) > @@ -82,5 +82,8 @@ void epoch_exit_preempt(epoch_t epoch, epoch_tracker_t > void epoch_enter(epoch_t epoch); > void epoch_exit(epoch_t epoch); > > +void epoch_thread_init(struct thread *); > +void epoch_thread_fini(struct thread *); > + > #endif
svn commit: r340413 - in head/sys: kern net sys
Author: glebius Date: Tue Nov 13 22:58:38 2018 New Revision: 340413 URL: https://svnweb.freebsd.org/changeset/base/340413 Log: For compatibility KPI functions like if_addr_rlock() that used to have mutexes but now are converted to epoch(9) use thread-private epoch_tracker. Embedding tracker into ifnet(9) or ifnet derived structures creates a non reentrable function, that will fail miserably if called simultaneously from two different contexts. A thread private tracker will provide a single tracker that would allow to call these functions safely. It doesn't allow nested call, but this is not expected from compatibility KPIs. Reviewed by: markj Modified: head/sys/kern/kern_thread.c head/sys/kern/subr_epoch.c head/sys/net/if.c head/sys/net/if_var.h head/sys/sys/epoch.h head/sys/sys/proc.h Modified: head/sys/kern/kern_thread.c == --- head/sys/kern/kern_thread.c Tue Nov 13 22:41:20 2018(r340412) +++ head/sys/kern/kern_thread.c Tue Nov 13 22:58:38 2018(r340413) @@ -272,6 +272,7 @@ thread_init(void *mem, int size, int flags) td->td_rlqe = NULL; EVENTHANDLER_DIRECT_INVOKE(thread_init, td); umtx_thread_init(td); + epoch_thread_init(td); td->td_kstack = 0; td->td_sel = NULL; return (0); @@ -291,6 +292,7 @@ thread_fini(void *mem, int size) turnstile_free(td->td_turnstile); sleepq_free(td->td_sleepqueue); umtx_thread_fini(td); + epoch_thread_fini(td); seltdfini(td); } Modified: head/sys/kern/subr_epoch.c == --- head/sys/kern/subr_epoch.c Tue Nov 13 22:41:20 2018(r340412) +++ head/sys/kern/subr_epoch.c Tue Nov 13 22:58:38 2018(r340413) @@ -667,3 +667,17 @@ in_epoch(epoch_t epoch) { return (in_epoch_verbose(epoch, 0)); } + +void +epoch_thread_init(struct thread *td) +{ + + td->td_et = malloc(sizeof(struct epoch_tracker), M_EPOCH, M_WAITOK); +} + +void +epoch_thread_fini(struct thread *td) +{ + + free(td->td_et, M_EPOCH); +} Modified: head/sys/net/if.c == --- head/sys/net/if.c Tue Nov 13 22:41:20 2018(r340412) +++ head/sys/net/if.c Tue Nov 13 22:58:38 2018(r340413) @@ -1767,35 +1767,29 @@ if_data_copy(struct ifnet *ifp, struct if_data *ifd) void if_addr_rlock(struct ifnet *ifp) { - MPASS(*(uint64_t *)&ifp->if_addr_et == 0); - epoch_enter_preempt(net_epoch_preempt, &ifp->if_addr_et); + + epoch_enter_preempt(net_epoch_preempt, curthread->td_et); } void if_addr_runlock(struct ifnet *ifp) { - epoch_exit_preempt(net_epoch_preempt, &ifp->if_addr_et); -#ifdef INVARIANTS - bzero(&ifp->if_addr_et, sizeof(struct epoch_tracker)); -#endif + + epoch_exit_preempt(net_epoch_preempt, curthread->td_et); } void if_maddr_rlock(if_t ifp) { - MPASS(*(uint64_t *)&ifp->if_maddr_et == 0); - epoch_enter_preempt(net_epoch_preempt, &ifp->if_maddr_et); + epoch_enter_preempt(net_epoch_preempt, curthread->td_et); } void if_maddr_runlock(if_t ifp) { - epoch_exit_preempt(net_epoch_preempt, &ifp->if_maddr_et); -#ifdef INVARIANTS - bzero(&ifp->if_maddr_et, sizeof(struct epoch_tracker)); -#endif + epoch_exit_preempt(net_epoch_preempt, curthread->td_et); } /* Modified: head/sys/net/if_var.h == --- head/sys/net/if_var.h Tue Nov 13 22:41:20 2018(r340412) +++ head/sys/net/if_var.h Tue Nov 13 22:58:38 2018(r340413) @@ -381,8 +381,6 @@ struct ifnet { */ struct netdump_methods *if_netdump_methods; struct epoch_contextif_epoch_ctx; - struct epoch_trackerif_addr_et; - struct epoch_trackerif_maddr_et; /* * Spare fields to be added before branching a stable branch, so Modified: head/sys/sys/epoch.h == --- head/sys/sys/epoch.hTue Nov 13 22:41:20 2018(r340412) +++ head/sys/sys/epoch.hTue Nov 13 22:58:38 2018(r340413) @@ -82,5 +82,8 @@ void epoch_exit_preempt(epoch_t epoch, epoch_tracker_t void epoch_enter(epoch_t epoch); void epoch_exit(epoch_t epoch); +void epoch_thread_init(struct thread *); +void epoch_thread_fini(struct thread *); + #endif /* _KERNEL */ #endif /* _SYS_EPOCH_H_ */ Modified: head/sys/sys/proc.h == --- head/sys/sys/proc.h Tue Nov 13 22:41:20 2018(r340412) +++ head/sys/sys/proc.h Tue Nov 13 22:58:38 2018(r340413) @@ -193,6 +193,7 @@ struct trapframe; struct turnstile; struct vm_map; struct vm_map_entry; +struct epoch_tracker; /* * XXX: Does