Re: struct ifnet locking [was Re: IFEF_MPSAFE]

[Ryota Ozaki <ozaki.ryota%gmail.com@localhost>]

On Fri, Dec 8, 2017 at 7:53 PM, Nick Hudson <skrll%netbsd.org@localhost> wrote:
> On 12/06/17 11:11, Ryota Ozaki wrote:
>>
>> On Tue, Nov 28, 2017 at 6:40 PM, Ryota Ozaki <ozaki.ryota%gmail.com@localhost>
>> wrote:
>>>
>>> On Mon, Nov 27, 2017 at 12:24 AM, Nick Hudson <skrll%netbsd.org@localhost> wrote:
>>>>
>>>> On 11/17/17 07:44, Ryota Ozaki wrote:
>>>>>
>>>>>
> [snip]
>>>>
>>>> Hi,
>>>
>>> (Sorry for late replying. I was sick in bed for days...)
>>>
>>>> Can you document the protection requirements of the struct ifnet
>>>> members,
>>>> e.g. if_flags.
>>>>
>>>> Ideally by annotating it like struct proc
>>>>
>>>> http://src.illumos.org/source/xref/netbsd-src/sys/sys/proc.h#230
>>>
>>> OK. I'm trying to add annotations like that.
>
>
> It's my turn to say sorry for a late reply... Sorry and thanks for working
> on this.

NP. I tend to be late...

>
>
>> Some more changes are needed though, I fixed some race conditions on ifnet
>> and wrote the lock requirements of ifnet:
>>     http://www.netbsd.org/~ozaki-r/ifnet-locks.diff
>>
>> There remain some unsafe members. Those for carp, agr and pf should be
>> fixed
>> when making them MP-safe. Another remainder is a set of statistic
>> counters,
>> which will be MP-safe (by making them per-CPU counters) sooner or later.
>>
>> if_flags should be modified with holding if_ioct_lock. If an interface
>> enables
>> IEFE_MPSAFE, the interface must meet the contract, which enforces the
>> interface to update if_flags only in XXX_ioctl and also the interface has
>> to
>> give up using IFF_OACTIVE flag which doesn't suit for multi-core systems
>> and hardware multi-queue NICs.
>
>
> I'd pretty much come to the same conclusion, but wanted to make sure we
> shared the same understanding.
>
>> I'm not sure yet we have to do
>> synchronization between updates and reads on if_flags. (Anyway we should
>> NOT do that because the synchronization will hurt performance.)
>
>
> Not sure I follow this. I think we agree that the driver should not use
> if_flags in the rx/tx path (anymore).

Yes. Drivers should provide their own method.

Anyway I updated the document that reflects recent changes:
  http://www.netbsd.org/~ozaki-r/ifnet-locks.v2.diff

  ozaki-r