Re: CVS commit: src/sys/netinet6

[Ryota Ozaki <ozaki-r%netbsd.org@localhost>]

On Tue, Dec 26, 2017 at 4:30 PM, Paul Goyette <paul%whooppee.com@localhost> 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.

  ozaki-r