Re: CVS commit: src/sys/netinet6

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

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