Hi,
On 2017/12/07 14:21, Taylor R Campbell wrote:
> I dropped this thread on the floor a while ago and I forget what the
> status was. I've had a patch sitting in my tree for a while which I
> brushed off to put the list update logic in separate functions, as I
> think chuq requested a while ago, but still keep it all isolated to
> subr_psref.c and avoid defining any new macros.
>
> If you've measured that SLIST works better -- which would make sense
> because the typical bracketed psref_acquire/release nesting makes a
> nice LIFO stack of the things so that SLIST_REMOVE should usually be
> done in the first iteration -- then I'm happy to replace it by SLIST.
> We should just make sure to fix this bug before netbsd-8 goes out!
>
> Thoughts?
I measure IPv4 forwarding performance your patch(PSREF_LIST) version
and SLIST version. At first, the result is quite affected by
optimization option "-falign-functions"... Based on this, it seems
there is almost no difference between PSREF_LIST and SLIST.
However, it seems your patch has large diff... From the point of code
stability, smaller diff SLIST version would be better for netbsd-8 branch
to fix the bug. Because your patch causes some new ATF failures such as
ldp_regen and route_change_ifp (reported by ozaki-r@n.o). We can probably
fix them at once but guaranteeing its stability would take more time.
The SLIST version patch is following.
====================
diff --git a/sys/kern/subr_psref.c b/sys/kern/subr_psref.c
index c3f76ab0e74..9eac19def3f 100644
--- a/sys/kern/subr_psref.c
+++ b/sys/kern/subr_psref.c
@@ -78,7 +78,7 @@ __KERNEL_RCSID(0, "$NetBSD: subr_psref.c,v 1.7 2017/06/01
02:45:13 chs Exp $");
#include <sys/queue.h>
#include <sys/xcall.h>
-LIST_HEAD(psref_head, psref);
+SLIST_HEAD(psref_head, psref);
static bool _psref_held(const struct psref_target *, struct psref_class *,
bool);
@@ -135,7 +135,7 @@ psref_cpu_drained_p(void *p, void *cookie, struct cpu_info
*ci __unused)
const struct psref_cpu *pcpu = p;
bool *retp = cookie;
- if (!LIST_EMPTY(&pcpu->pcpu_head))
+ if (!SLIST_EMPTY(&pcpu->pcpu_head))
*retp = false;
}
@@ -194,7 +194,7 @@ psref_check_duplication(struct psref_cpu *pcpu, struct
psref *psref,
bool found = false;
struct psref *_psref;
- LIST_FOREACH(_psref, &pcpu->pcpu_head, psref_entry) {
+ SLIST_FOREACH(_psref, &pcpu->pcpu_head, psref_entry) {
if (_psref == psref &&
_psref->psref_target == target) {
found = true;
@@ -250,7 +250,7 @@ psref_acquire(struct psref *psref, const struct
psref_target *target,
#endif
/* Record our reference. */
- LIST_INSERT_HEAD(&pcpu->pcpu_head, psref, psref_entry);
+ SLIST_INSERT_HEAD(&pcpu->pcpu_head, psref, psref_entry);
psref->psref_target = target;
psref->psref_lwp = curlwp;
psref->psref_cpu = curcpu();
@@ -273,6 +273,7 @@ void
psref_release(struct psref *psref, const struct psref_target *target,
struct psref_class *class)
{
+ struct psref_cpu *pcpu;
int s;
KASSERTMSG((kpreempt_disabled() || cpu_softintr_p() ||
@@ -302,7 +303,9 @@ psref_release(struct psref *psref, const struct
psref_target *target,
* (as does blocking interrupts).
*/
s = splraiseipl(class->prc_iplcookie);
- LIST_REMOVE(psref, psref_entry);
+ pcpu = percpu_getref(class->prc_percpu);
+ SLIST_REMOVE(&pcpu->pcpu_head, psref, psref, psref_entry);
+ percpu_putref(class->prc_percpu);
splx(s);
/* If someone is waiting for users to drain, notify 'em. */
@@ -353,7 +356,7 @@ psref_copy(struct psref *pto, const struct psref *pfrom,
pcpu = percpu_getref(class->prc_percpu);
/* Record the new reference. */
- LIST_INSERT_HEAD(&pcpu->pcpu_head, pto, psref_entry);
+ SLIST_INSERT_HEAD(&pcpu->pcpu_head, pto, psref_entry);
pto->psref_target = pfrom->psref_target;
pto->psref_lwp = curlwp;
pto->psref_cpu = curcpu();
@@ -474,7 +477,7 @@ _psref_held(const struct psref_target *target, struct
psref_class *class,
pcpu = percpu_getref(class->prc_percpu);
/* Search through all the references on this CPU. */
- LIST_FOREACH(psref, &pcpu->pcpu_head, psref_entry) {
+ SLIST_FOREACH(psref, &pcpu->pcpu_head, psref_entry) {
/* Sanity-check the reference's CPU. */
KASSERTMSG((psref->psref_cpu == curcpu()),
"passive reference transferred from CPU %u to CPU %u",
diff --git a/sys/sys/psref.h b/sys/sys/psref.h
index 88db6dbb603..9096a3798d6 100644
--- a/sys/sys/psref.h
+++ b/sys/sys/psref.h
@@ -69,7 +69,7 @@ struct psref_target {
* written only on the local CPU.
*/
struct psref {
- LIST_ENTRY(psref) psref_entry;
+ SLIST_ENTRY(psref) psref_entry;
const struct psref_target *psref_target;
struct lwp *psref_lwp;
struct cpu_info *psref_cpu;
====================
Of course, we also think this bug must be fixed before netbsd-8 rc.
Thanks,
--
//////////////////////////////////////////////////////////////////////
Internet Initiative Japan Inc.
Device Engineering Section,
IoT Platform Development Department,
Network Division,
Technology Unit
Kengo NAKAHARA <k-nakah...@iij.ad.jp>
