On Tue, Dec 26, 2017 at 6:30 PM, Paul Goyette <p...@whooppee.com> wrote: > On Tue, 26 Dec 2017, Ryota Ozaki wrote: > >> On Tue, Dec 26, 2017 at 4:30 PM, Paul Goyette <p...@whooppee.com> wrote: >>> >>> On Tue, 26 Dec 2017, Ryota Ozaki wrote: >>> >>>>> Well, since the lock _might_ be released (and subsequently reacquired) >>>>> by callout_halt(), it might be easiest to modify all the callers to >>>>> just unlock it before calling nd6_dad_stoptimer(), and reacquire the >>>>> mutex after it returns (as well as re-establishing any values that >>>>> might have changed while the mutex was released). >>>>> >>>>> The callers needs to be prepared for the release/reacquire situation >>>>> anyway, so the change should not be significant. As noted in the >>>>> callout_halt(9) man page >>>>> >>>>> ...to avoid race conditions the caller should always assume that >>>>> [the] interlock has been released and reacquired, and act >>>>> accordingly. >>>>> >>>>> >>>>> Alternatively, you could modify all the callers to always acquire the >>>>> mutex before calling nd6_dad_stoptimer(). >>>> >>>> >>>> >>>> I mean such changes are tough and mess up many codes which we don't >>>> hope. >>> >>> >>> >>> Yes, the changes are not trivial. But the first option above should >>> already >>> have been done when you changed from callout_stop() to _halt(). >>> >>>>> But making a run-time decision with mutex_owned() is not a good idea. >>>> >>>> >>>> >>>> If it must be respected we can back to callout_stop because it's unsafe >>>> but NetBSD 7 uses it without any issues; using callout_stop in >>>> nd6_dad_stoptimer might be safe somehow (if not NET_MPSAFE). >>> >>> >>> >>> IMHO, we definitely do not want to use mutex_owned() in this way. >>> >>> I do not believe that going backwards to callout_stop() is the right >>> approach. >>> >>> Again IMHO, the _right_ thing to do is to continue using callout_halt() >>> and >>> modify the callers. Yes, it might be a lot of work, and it might >>> initially >>> introduce new errors, but in the long run it is the correct approach. >>> IMHO. >> >> >> The right thing here is to kill softnet_lock. IMHO, we shouldn't >> straightforwardly cope with softnet_lock and change the code a lot >> due to softnet_lock. Backing to callout_stop is a good compromise, I >> think. > > > A compromise is Ok, but only as a temporary solution, as long as you > have a plan for removing softnet_lock. The code should be clearly > marked with a XXX indicating why you are using callout_stop() instead > of callout_halt() and telling us what needs to be done.
Yes, I'll do so. Thanks. > > I'd really like to see other peoples' opinions on this matter before > making a final decision. (And I'm rather surprised that no-one else > has yet made any comments!) I guess nobody wants to be involved with softnet_lock :-/ ozaki-r
