Discussion:
IFEF_MPSAFE
(too old to reply)
Ryota Ozaki
2017-11-10 09:35:05 UTC
Permalink
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().
But adding one more flag for if_ioctl() is I think
wasteful. Also there are other functions such as if_init()
and if_slowtimo() that would also need a flag.

So I propose to have just one flag for indications of
MP-safe. If an interface have both MP-safe and non-MP-safe
operations at a time, we have to set the IFEF_MPSAFE flag
and let callees of non-MP-safe operations take KERNEL_LOCK.

This change breaks ABI and need a kernel version bump,
however, IFEF_*_MPSAFE flags are new to netbsd-8 so it
doesn't break backward compatibility.

Any comments or objections?

Thanks,
ozaki-r

--
Posted automagically by a mail2news gateway at muc.de e.V.
Please direct questions, flames, donations, etc. to news-***@muc.de
Ryota Ozaki
2017-11-15 04:53:32 UTC
Permalink
Post by Ryota Ozaki
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.

ozaki-r
Post by Ryota Ozaki
But adding one more flag for if_ioctl() is I think
wasteful. Also there are other functions such as if_init()
and if_slowtimo() that would also need a flag.
So I propose to have just one flag for indications of
MP-safe. If an interface have both MP-safe and non-MP-safe
operations at a time, we have to set the IFEF_MPSAFE flag
and let callees of non-MP-safe operations take KERNEL_LOCK.
This change breaks ABI and need a kernel version bump,
however, IFEF_*_MPSAFE flags are new to netbsd-8 so it
doesn't break backward compatibility.
Any comments or objections?
Thanks,
ozaki-r
--
Posted automagically by a mail2news gateway at muc.de e.V.
Please direct questions, flames, donations, etc. to news-***@muc.de
Ryota Ozaki
2017-11-17 07:44:02 UTC
Permalink
Post by Ryota Ozaki
Post by Ryota Ozaki
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().
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

--
Posted automagically by a mail2news gateway at muc.de e.V.
Please direct questions, flames, donations, etc. to news-***@muc.de
Nick Hudson
2017-11-26 15:24:19 UTC
Permalink
Post by Ryota Ozaki
Post by Ryota Ozaki
Post by Ryota Ozaki
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().
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
http://www.netbsd.org/~ozaki-r/if_ioctl-no-KERNEL_LOCK.revised.diff
ozaki-r
Hi,

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

Thanks,

Nick


--
Posted automagically by a mail2news gateway at muc.de e.V.
Please direct questions, flames, donations, etc. to news-***@muc.de
Ryota Ozaki
2017-11-28 09:40:04 UTC
Permalink
Post by Ryota Ozaki
Post by Ryota Ozaki
Post by Ryota Ozaki
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().
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
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.

ozaki-r

--
Posted automagically by a mail2news gateway at muc.de e.V.
Please direct questions, flames, donations, etc. to news-***@muc.de
Ryota Ozaki
2017-12-06 11:11:37 UTC
Permalink
Post by Ryota Ozaki
Post by Ryota Ozaki
Post by Ryota Ozaki
Post by Ryota Ozaki
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().
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
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

--
Posted automagically by a mail2news gateway at muc.de e.V.
Please direct questions, flames, donations, etc. to news-***@muc.de
Ryota Ozaki
2017-12-14 10:48:49 UTC
Permalink
[snip]
Post by Ryota Ozaki
Post by Ryota Ozaki
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...
Post by Ryota Ozaki
Some more changes are needed though, I fixed some race conditions on 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.
Post by Ryota Ozaki
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

--
Posted automagically by a mail2news gateway at muc.de e.V.
Please direct questions, flames, donations, etc. to news-***@muc.de
Ryota Ozaki
2017-12-19 03:43:41 UTC
Permalink
Post by Ryota Ozaki
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.
Great.
Post by Ryota Ozaki
http://www.netbsd.org/~ozaki-r/ifnet-locks.v2.diff
Some wording improvement suggestions...
@@ -391,11 +429,33 @@ typedef struct ifnet {
#define IFEF_NO_LINK_STATE_CHANGE __BIT(1) /* doesn't
use link state interrupts */
/*
- * The following if_XXX() handlers don't take KERNEL_LOCK if the interface
- * - if_start
- * - if_output
- * - if_ioctl
+ * The guidline to enable IFEF_MPSAFE on an interface
The guidelines for converting an interface to IFEF_MPSAFE are as
follows
+ * Enabling IFEF_MPSAFE on an interface suppress taking KERNEL_LOCK
Enabling IFEF_MPSAFE on an interface suppresses taking KERNEL_LOCK
when
Thanks. I reflected the suggestions and committed the updated document.

BTW I committed a change that disables IFEF_MPSAFE by default on
all interfaces because it seems that IFEF_MPSAFE requires additional
changes to work safely with it. We should enable it by default if an
interface is guaranteed to be safe.

ozaki-r

--
Posted automagically by a mail2news gateway at muc.de e.V.
Please direct questions, flames, donations, etc. to news-***@muc.de
Ryota Ozaki
2017-12-19 08:21:50 UTC
Permalink
Post by Ryota Ozaki
BTW I committed a change that disables IFEF_MPSAFE by default on
all interfaces because it seems that IFEF_MPSAFE requires additional
changes to work safely with it. We should enable it by default if an
interface is guaranteed to be safe.
What additional changes?
For example, avoid updating if_flags (and reading it and depending on
the result) in Tx/Rx paths and using if_watchdog.

ozaki-r

--
Posted automagically by a mail2news gateway at muc.de e.V.
Please direct questions, flames, donations, etc. to news-***@muc.de
Loading...