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-head@freebsd.org mailing list https://lists.freebsd.org/mailman/listinfo/svn-src-head To unsubscribe, send any mail to "svn-src-head-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-head@freebsd.org mailing list https://lists.freebsd.org/mailman/listinfo/svn-src-head To unsubscribe, send any mail to "svn-src-head-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-head@freebsd.org mailing list https://lists.freebsd.org/mailman/listinfo/svn-src-head To unsubscribe, send any mail to "svn-src-head-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-head@freebsd.org mailing list https://lists.freebsd.org/mailman/listinfo/svn-src-head To unsubscribe, send any mail to "svn-src-head-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-head@freebsd.org mailing list https://lists.freebsd.org/mailman/listinfo/svn-src-head To unsubscribe, send any mail to "svn-src-head-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-head@freebsd.org mailing list https://lists.freebsd.org/mailman/listinfo/svn-src-head To unsubscribe, send any mail to "svn-src-head-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 *)>if_addr_et == 0);
> > - epoch_enter_preempt(net_epoch_preempt, >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, >if_addr_et);
> > -#ifdef INVARIANTS
> > - bzero(>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 *)>if_maddr_et == 0);
> > - epoch_enter_preempt(net_epoch_preempt, >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, >if_maddr_et);
> > -#ifdef INVARIANTS
> > - bzero(>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;
> >
> > /*
> >
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 *)>if_addr_et == 0);
> - epoch_enter_preempt(net_epoch_preempt, >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, >if_addr_et);
> -#ifdef INVARIANTS
> - bzero(>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 *)>if_maddr_et == 0);
> - epoch_enter_preempt(net_epoch_preempt, >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, >if_maddr_et);
> -#ifdef INVARIANTS
> - bzero(>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 /* _KERNEL */
> #endif /*
