On Tue, Dec 26, 2017 at 12:37 PM, Paul Goyette <p...@whooppee.com> wrote: > On Tue, 26 Dec 2017, Ryota Ozaki wrote: > >> On Tue, Dec 26, 2017 at 11:35 AM, Paul Goyette <p...@whooppee.com> wrote: >>> >>> >>>> To generate a diff of this commit: >>> >>> >>> >>> # cvs rdiff -u -r1.139 -r1.140 src/sys/netinet6/nd6_nbr.c >>> @@ -1097,7 +1097,11 @@ nd6_dad_stoptimer(struct dadq *dp) >>> #ifdef NET_MPSAFE >>> callout_halt(&dp->dad_timer_ch, NULL); >>> #else >>> - callout_halt(&dp->dad_timer_ch, softnet_lock); >>> + /* XXX still need the trick for softnet_lock */ >>> + if (mutex_owned(softnet_lock)) >>> + callout_halt(&dp->dad_timer_ch, softnet_lock); >>> + else >>> + callout_halt(&dp->dad_timer_ch, NULL); >>> #endif >>> } >>> >>> This goes against the restriction noted in the mutex(9) man page: >>> >>> [mutex_owned()] should not be used to make locking decisions at run >>> time. ... >>> >>> Please find a different way to make this run-time decision. >> >> >> I know the restriction well, but for softnet_lock, following the >> restriction >> is sometimes hard. I don't have an option to statically decide if >> softnet_lock is held or not without messing up many codes. >> >> An option we can have here is to give up using callout_halt and use >> callout_stop that may be unsafe. >> >> Which is better for us? > > > 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. > > 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). ozaki-r
