Hi, Thank you for your closeup review!
On 2016/01/11 14:48, Taylor R Campbell wrote: > Date: Wed, 6 Jan 2016 14:19:01 +0900 > From: Kengo NAKAHARA <[email protected]> > > On 2016/01/06 3:40, Taylor R Campbell wrote: > > There are a couple other things I'm concerned about in the patch -- I > > notice you're doing softint_establish with a lock held. Maybe that's > > OK, but it makes me a little nervous. I don't have time right now but > > I would like to take a closer look in the next few days at some of the > > changes like that that your patch makes. > > It works for my environment for now..., meanwhile I see your point. > Thank you for your help. I'm looking forward to your review. > > I took a closer look at your patch, and at your fix for 50522. I'm > concerned about a few things: > > - missing memory barriers in handling the reference counts, > - (future) performance impact of atomics in gifintr and gif_output, > and > - global invariants for the gif list, and overcomplexity of the > locking scheme. > > In particular: > > - The following fragment of gifintr probably needs a memory barrier on > non-x86 platforms: > > atomic_inc_uint(&sc->gif_si_refs); > if (sc->gif_pdst == NULL || sc->gif_psrc == NULL) { > ... > } The codes about gif_si_refs are removed by if_gif.c:r1.103 cvs commit, so there is no atomic operation in gif(4) codes. However, the locking notes still has gif_si_lock, sorry. BTW, I heard a memory barrier is not required after rwlock or mutex. Doesn't it true for non-x86 platforms? > Otherwise, gifintr on CPU 0 might load nonnull values from > sc->gif_pdst and sc->gif_psrc before gif_set_tunnel on CPU 1 sees > positive sc->gif_si_refs. I'm a little unclear on the rest of the > protocol -- but I suspect it can be done without atomics or mutexes > anyway in the packet-processing path. > > - Atomics are expensive interprocessor synchronization operations, so > we should avoid them where unnecessary for MP-scalability -- > particularly in the packet-processing path. In this case I think it > shouldn't be too hard to avoid: we simply need to prevent calls to > gif_output before we do softint_disestablish. Yes, I removed atomic operations from gif(4) packet-processing path, as note above. I think the fix for PR/50522 fixes the race between gif_output and softint_disestablish. > - With your proposed patch, if CPU 0 and CPU 1 both run gif_set_tunnel > with the same address, then they might > > (a) first find no existing gif(4) having that address; > (b) next configure gif0 and gif1 with the same address; and then > (c) finally leave the system in a state where gif0 and gif1 have the > same address, > > which violates the global invariant that gif_set_tunnel tries to > enforce. So we really need (a), (b), and (c) to happen atomically. I see. Sorry, I missed the pattern... > I think the following protocol should be simpler and safer: > > - One global interruptible lock for the system's gif(4) configuration. > - No per-gif(4) locks at all. > - Split gif_encap_detach (and in/in6_gif_detach) into two parts: > . gif_encap_pause: prevent further use of gif_output (encap_detach) > . gif_encap_detach: destroy data structures associated with it > (rtcache_free) > - Make gif_set_tunnel do: > 1. acquire global lock > 2. check invariant -- reject duplicate gif src/dst addresses > 3. gif_encap_pause > 4. softint_disestablish > 5. gif_encap_detach, sockaddr_free > 6. gif_encap_attach > 7. softint_establish > 8. release global lock > - Make gif_delete_tunnel do: > 1. acquire global lock > 2. gif_encap_pause > 3. softint_disestablish > 4. gif_encap_detach, sockaddr_free > 5. release global lock Hmm... ip_encap.c(encap_attach and encap_detach) is used by not only gif(4) but also stf(4) for now, so I think the global interruptible lock might be the common lock among gif(4), stf(4) and other interfaces which will use ip_encap.c. Except it, the processing seems excellent. > The global lock, for the moment, can just be KERNEL_LOCK: it is > acquired only during ifconfig(8), not in the packet-processing path. > Or you could make an interruptible lock out of a mutex, condvar, and > flag. It should be interruptible so that if `ifconfig gif0 tunnel > ...' hangs for any reason, then another process trying to do the same > can handle ^C. > > > Here's a quick sketch of an interruptible lock: I feel like avoiding KERNEL_LOCK... I will implement based on the sketch. Thank you very much for your many great advices! Thanks, -- ////////////////////////////////////////////////////////////////////// Internet Initiative Japan Inc. Device Engineering Section, Core Product Development Department, Product Division, Technology Unit Kengo NAKAHARA <[email protected]>
