Re: RFC: PERCPU_LIST to fix PR kern/52515

[Kengo NAKAHARA <k-nakahara%iij.ad.jp@localhost>]

Hi,

On 2017/12/09 0:18, Taylor R Campbell wrote:
>> Date: Fri, 8 Dec 2017 18:51:28 +0900
>> From: Kengo NAKAHARA <k-nakahara%iij.ad.jp@localhost>
>>
>> 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.
> 
> Not surprising I made a bug somewhere in there!  The SLIST version is
> fine by me.  However, there's one small nit:
> 
>> 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;
> 
> In order to avoid changing the kernel ABI in netbsd-8, please add a
> void *psref_unused0 after psref_entry.  That way we don't have to
> think about the consequences of an ABI change in the branch, and we
> still have the opportunity to switch back to a doubly-linked list if
> we want.

Thank you for your reviewing. I see. Here is the updated patch.
====================
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..d1ab606e117 100644
--- a/sys/sys/psref.h
+++ b/sys/sys/psref.h
@@ -69,7 +69,9 @@ struct psref_target {
  *	written only on the local CPU.
  */
 struct psref {
-	LIST_ENTRY(psref)		psref_entry;
+	SLIST_ENTRY(psref)		psref_entry;
+	/* To keep ABI with LIST_ENTRY(psref) version. */
+	void				*psref_unused0;
 	const struct psref_target	*psref_target;
 	struct lwp			*psref_lwp;
 	struct cpu_info			*psref_cpu;

====================

If there is no problem, can I commit and pullup -8 the patch?


Thanks,

-- 
//////////////////////////////////////////////////////////////////////
Internet Initiative Japan Inc.

Device Engineering Section,
IoT Platform Development Department,
Network Division,
Technology Unit

Kengo NAKAHARA <k-nakahara%iij.ad.jp@localhost>