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-nakah...@iij.ad.jp>
