Re: struct ifnet locking [was Re: IFEF_MPSAFE]

[Nick Hudson <skrll%netbsd.org@localhost>]

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.


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

Thanks,
Nick