Re: racy acccess in kern_runq.c

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

Hi,

Thank you for your comment.

On 2019/12/06 16:42, Maxime Villard wrote:
> Le 06/12/2019 à 07:52, Kengo NAKAHARA a écrit :
> > Hi,
> >
> > There are some racy accesses in kern_runq.c detected by KCSAN.  Those
> > racy access messages is so frequency that they cover other messages,
> > so I want to fix them.  They can be fixed by the following patch.
> >
> > ====================
> > diff --git a/sys/kern/kern_runq.c b/sys/kern/kern_runq.c
> > index 04ff1732016..bb0815689cd 100644
> > --- a/sys/kern/kern_runq.c
> > +++ b/sys/kern/kern_runq.c
> > @@ -627,7 +627,7 @@ sched_balance(void *nocallout)
> >       /* Make lockless countings */
> >       for (CPU_INFO_FOREACH(cii, ci)) {
> >           spc = &ci->ci_schedstate;
> > -
> > +        spc_lock(ci);
> >           /*
> >            * Average count of the threads
> >            *
> > @@ -643,10 +643,11 @@ sched_balance(void *nocallout)
> >               hci = ci;
> >               highest = spc->spc_avgcount;
> >           }
> > +        spc_unlock(ci);
> >       }
> >   
> >       /* Update the worker */
> > -    worker_ci = hci;
> > +    atomic_swap_ptr(&worker_ci, hci);
> >   
> >       if (nocallout == NULL)
> >           callout_schedule(&balance_ch, balance_period);
> > @@ -734,12 +735,15 @@ sched_idle(void)
> >       spc_unlock(ci);
> >   
> >   no_migration:
> > +    spc_lock(ci);
> >       if ((spc->spc_flags & SPCF_OFFLINE) != 0 || spc->spc_count != 0) {
> > +        spc_unlock(ci);
> >           return;
> >       }
> >   
> >       /* Reset the counter, and call the balancer */
> >       spc->spc_avgcount = 0;
> > +    spc_unlock(ci);
> >       sched_balance(ci);
> >       tci = worker_ci;
> >       tspc = &tci->ci_schedstate;
> > ====================
>
> It just so happens we were talking about this yesterday.

Oh, I missed the thread, sorry.

> worker_ci indeed needs to be a real atomic, but it means we also have to
> fetch it atomically here:
>
> 744 	tci = worker_ci;
>
> For the other variables, my understanding was that we don't care about
> races, but only care about making sure the accesses are not split, so it
> seems to me atomic_relaxed would suffice.

I see.  I update the patch.
====================
diff --git a/sys/kern/kern_runq.c b/sys/kern/kern_runq.c
index 04ff1732016..dbd0f73585a 100644
--- a/sys/kern/kern_runq.c
+++ b/sys/kern/kern_runq.c
@@ -627,6 +627,7 @@ sched_balance(void *nocallout)
 	/* Make lockless countings */
 	for (CPU_INFO_FOREACH(cii, ci)) {
 		spc = &ci->ci_schedstate;
+		u_int avg, mcount;
 
 		/*
 		 * Average count of the threads
@@ -634,19 +635,20 @@ sched_balance(void *nocallout)
 		 * The average is computed as a fixpoint number with
 		 * 8 fractional bits.
 		 */
-		spc->spc_avgcount = (
-			weight * spc->spc_avgcount + (100 - weight) * 256 * spc->spc_mcount
-			) / 100;
+		avg = atomic_load_relaxed(&spc->spc_avgcount);
+		mcount = atomic_load_relaxed(&spc->spc_mcount);
+		avg = (weight * avg + (100 - weight) * 256 * mcount) / 100;
+		atomic_store_relaxed(&spc->spc_avgcount, avg);
 
 		/* Look for CPU with the highest average */
-		if (spc->spc_avgcount > highest) {
+		if (avg > highest) {
 			hci = ci;
-			highest = spc->spc_avgcount;
+			highest = avg;
 		}
 	}
 
 	/* Update the worker */
-	worker_ci = hci;
+	atomic_swap_ptr(&worker_ci, hci);
 
 	if (nocallout == NULL)
 		callout_schedule(&balance_ch, balance_period);
@@ -734,14 +736,15 @@ sched_idle(void)
 	spc_unlock(ci);
 
 no_migration:
-	if ((spc->spc_flags & SPCF_OFFLINE) != 0 || spc->spc_count != 0) {
+	if ((spc->spc_flags & SPCF_OFFLINE) != 0
+	    || atomic_load_relaxed(&spc->spc_count) != 0) {
 		return;
 	}
 
 	/* Reset the counter, and call the balancer */
-	spc->spc_avgcount = 0;
+	atomic_store_relaxed(&spc->spc_avgcount, 0);
 	sched_balance(ci);
-	tci = worker_ci;
+	atomic_swap_ptr(&tci, worker_ci);
 	tspc = &tci->ci_schedstate;
 	if (ci == tci || spc->spc_psid != tspc->spc_psid)
 		return;
====================

Is this appropriate?


Thanks,

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

Device Engineering Section,
Product Development Department,
Product Division,
Technology Unit

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