Re: callout_drain either broken or man page needs updating

 ---- On Thu, 14 Jul 2016 21:21:57 -0700 Hans Petter Selasky <hps_at_selasky.org> wrote ---- 
 > On 07/15/16 05:45, Matthew Macy wrote: 
 > > glebius last commit needs some further re-work. 
 >  
 > Hi, 
 >  
 > Glebius commit needs to be backed out, at least the API change that  
 > changes the return value when calling callout_stop() when the callout is  
 > scheduled and being serviced. Simply because there is code out there,  
 > like Mattew and others have discovered that is "refcounting" on the  
 > callout_reset() and expecting that a subsequent callout_stop() will  
 > return 1 to "unref". 

Yes. This is the cause of the "refcnt 0 on LLE at boot..." regression.

-M



 >  
 > If you consider this impossible, maybe a fourth return value is needed  
 > for CANCELLED and DRAINING . 
 >  
 > Further, getting the callouts straight in the TCP stack is a matter of  
 > doing the locking correctly, which some has called "my magic bullet" and  
 > not the return values. I've proposed in the following revision  
 > https://svnweb.freebsd.org/changeset/base/302768 to add a new callout  
 > API that accepts a locking function so that the callout code can run its  
 > cancelled checks at the right place for situations where more than one  
 > lock is needed. 
 >  
 > Consider this case: 
 >  
 > > void 
 > > tcp_timer_2msl(void *xtp) 
 > > { 
 > >         struct tcpcb *tp = xtp; 
 > >         struct inpcb *inp; 
 > >         CURVNET_SET(tp->t_vnet); 
 > > #ifdef TCPDEBUG 
 > >         int ostate; 
 > > 
 > >         ostate = tp->t_state; 
 > > #endif 
 > >         INP_INFO_RLOCK(&V_tcbinfo); 
 > >         inp = tp->t_inpcb; 
 > >         KASSERT(inp != NULL, ("%s: tp %p tp->t_inpcb == NULL", __func__, tp)); 
 > >         INP_WLOCK(inp); 
 > >         tcp_free_sackholes(tp); 
 > >         if (callout_pending(&tp->t_timers->tt_2msl) || 
 > >             !callout_active(&tp->t_timers->tt_2msl)) { 
 >  
 > Here we have custom in-house race check that doesn't affect the return  
 > value of callout_reset() nor callout_stop(). 
 >  
 > >                 INP_WUNLOCK(tp->t_inpcb); 
 > >                 INP_INFO_RUNLOCK(&V_tcbinfo); 
 > >                 CURVNET_RESTORE(); 
 > >                 return; 
 >  
 >  
 > I propose the following solution: 
 >  
 > > 
 > > static void 
 > > tcp_timer_2msl_lock(void *xtp, int do_lock) 
 > > { 
 > >     struct tcpcb *tp = xtp; 
 > >     struct inpcb *inp; 
 > > 
 > >     inp = tp->t_inpcb; 
 > > 
 > >     if (do_lock) { 
 > >         CURVNET_SET(tp->t_vnet); 
 > >         INP_INFO_RLOCK(&V_tcbinfo); 
 > >         INP_WLOCK(inp); 
 > >     } else { 
 > >         INP_WUNLOCK(inp); 
 > >         INP_INFO_RUNLOCK(&V_tcbinfo); 
 > >         CURVNET_RESTORE(); 
 > >     } 
 > > } 
 > > 
 >  
 > callout_init_lock_function(&callout, &tcp_timer_2msl_lock,  
 > CALLOUT_RETURNUNLOCKED); 
 >  
 > Then in softclock_call_cc() it will look like this: 
 >  
 > > 
 > >     CC_UNLOCK(cc); 
 > >     if (c_lock != NULL) { 
 > >         if (have locking function) 
 > >             tcp_timer_2msl_lock(c_arg, 1); 
 > >         else 
 > >             class->lc_lock(c_lock, lock_status); 
 > >         /* 
 > >          * The callout may have been cancelled 
 > >          * while we switched locks. 
 > >          */ 
 >  
 > Actually "CC_LOCK(cc)" should be in-front of cc_exec_cancel() to avoid  
 > races testing, setting and clearing this variable, like done in hps_head. 
 >  
 > >         if (cc_exec_cancel(cc, direct)) { 
 > >             if (have locking function) 
 > >                 tcp_timer_2msl_lock(c_arg, 0); 
 > >             else 
 > >                 class->lc_unlock(c_lock); 
 > >             goto skip; 
 > >                } 
 >  >                cc_exec_cancel(cc, direct) = true; 
 > > 
 > > .... 
 > > 
 > > skip: 
 > >     if ((c_iflags & CALLOUT_RETURNUNLOCKED) == 0) { 
 > >         if (have locking function) 
 > >             ... 
 > >         else 
 > >             class->lc_unlock(c_lock); 
 > >     } 
 >  
 > The whole point about this is to make the the cancelled check atomic. 
 >  
 > 1) Lock TCP 
 > 2) Lock CC_LOCK() 
 > 3) change callout state 
 >  
 > --HPS 
 > _______________________________________________ 
 > freebsd-current_at_freebsd.org mailing list 
 > https://lists.freebsd.org/mailman/listinfo/freebsd-current 
 > To unsubscribe, send any mail to "freebsd-current-unsubscribe_at_freebsd.org" 
 >