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 <cy.schub...@cschubert.com>
FreeBSD UNIX:  <c...@freebsd.org>   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_context    if_epoch_ctx;
> > -   struct epoch_tracker    if_addr_et;
> > -   struct epoch_tracker    if_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     /* _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 this belong in resource.h or resourcevar.h instead?
> > @@ -360,6 +361,7 @@ struct thread {
> >     int             td_lastcpu;     /* (t) Last cpu we were on. */
> >     int             td_oncpu;       /* (t) Which cpu we are on. */
> >     void            *td_lkpi_task;  /* LinuxKPI task struct pointer */
> > +   struct epoch_tracker *td_et;    /* (k) compat KPI spare tracker */
> >     int             td_pmcpend;
> >  };
> >  
> >
>
>
> -- 
> Cheers,
> Cy Schubert <cy.schub...@cschubert.com>
> FreeBSD UNIX:  <c...@freebsd.org>   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"

Reply via email to