tech-net archive

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index][Old Index]

Re: struct ifnet locking [was Re: IFEF_MPSAFE]



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




Home | Main Index | Thread Index | Old Index