Re: struct ifnet locking [was Re: IFEF_MPSAFE]

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

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:
>>>
>>> On Wed, Nov 15, 2017 at 1:53 PM, Ryota Ozaki <ozaki.ryota%gmail.com@localhost>
>>> wrote:
>>>>
>>>> On Fri, Nov 10, 2017 at 6:35 PM, Ryota Ozaki <ozaki.ryota%gmail.com@localhost>
>>>> wrote:
>>>>>
>>>>> Hi,
>>>>>
>>>>> http://www.netbsd.org/~ozaki-r/IFEF_MPSAFE.diff
>>>>>
>>>>> I'm going to commit the above change that integrates
>>>>> IFEF_OUTPUT_MPSAFE and IFEF_START_MPSAFE flags into
>>>>> IFEF_MPSAFE.
>>>>>
>>>>> The motivation is to not waste if_extflags bits. I'm now
>>>>> trying to make if_ioctl() hold KERNEL_LOCK selectively
>>>>> for some reasons as well as if_start() and if_output().
>>>>
>>>> BTW this is a patch for this plan:
>>>>    http://www.netbsd.org/~ozaki-r/if_ioctl-no-KERNEL_LOCK.diff
>>>>
>>>> It removes KERNEL_LOCK for if_ioctl from soo_ioctl and
>>>> selectively takes it in doifioctl. To this end, some fine-grain
>>>> KERNEL_LOCKs have to be added where calling components/functions
>>>> that aren't MP-safe.
>>>
>>> Revised rebased on latest NET_MPSAFE macros. The new patch also provides
>>> similar macros:
>>>    http://www.netbsd.org/~ozaki-r/if_ioctl-no-KERNEL_LOCK.revised.diff
>>>
>>>    ozaki-r
>>>
>> 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.

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'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.)

Thanks,
  ozaki-r