Re: racy acccess in kern_runq.c
Le 06/12/2019 à 20:37, Mindaugas Rasiukevicius a écrit : Maxime Villard wrote: Le 06/12/2019 à 17:53, Andrew Doran a écrit : On Fri, Dec 06, 2019 at 05:22:39PM +0900, Kengo NAKAHARA wrote: /* Update the worker */ - worker_ci = hci; + atomic_swap_ptr(_ci, hci); Why atomic_swap_ptr() not atomic_store_relaxed()? I don't see any bug that it fixes. Other than that it look OK to me. Because I suggested it; my concern was that if not explicitly atomic, the cpu could make two writes internally (even though the compiler produces only one instruction), and in that case a page fault would have been possible because of garbage dereference. No, atomic_store_relaxed() is sufficient; that is what it is for. Alright. The patch needs to be completed though; ie, 'spc_mcount++' too should use relaxed atomics, UVM style ([1]). [1] https://nxr.netbsd.org/diff/src/sys/uvm/uvm_fault.c?r2=%2Fsrc%2Fsys%2Fuvm%2Fuvm_fault.c%401.209=%2Fsrc%2Fsys%2Fuvm%2Fuvm_fault.c%401.208
Re: racy acccess in kern_runq.c
jnem...@cue.bc.ca (John Nemeth) writes: > Are you perhaps thinking of COBOL, which is traditionally all >upper case. I could be mistaken since I've never written and likely >have never seen ALGOL, but I have written COBOL. There were several variants to distinguish between keywords and variable names. One was to write keywords in upper case and variables in lower case. Another one was to write everything in upper case and to quote keywords. On systems that only support upper case this makes as much sense as trigraphs in C. -- -- Michael van Elst Internet: mlel...@serpens.de "A potential Snark may lurk in every tree."
Re: racy acccess in kern_runq.c
On Dec 6, 5:22pm, Don Lee wrote: } } Writing Kernel code *requires* knowledge of what code is generated } sometimes. In my experience, there have been standard techniques, } like pragmas and insertions of assembly code to suppress this } sort of undesirable optimization. } } Don't those techniques exist any more? My compiler friends used } to put them in for just this purpose, and they tried to make them } as portable as possible. Surely GCC does this. No? Pragmas and assembly code, of course, still exist, but they are extremely unportable. It is my understanding that Clang emulates many of GCCs pragmas, but there is no guarantee as pragmas are very much a compiler dependent feature. Writing short bits of assembly code is going to have dependence on the compiler and the CPU. Even within a particular CPU architecture, there are often significantly varying feature sets. Both of these options are best avoided in portable code. Newer versions of the C standard provide ways to annotate your code to tell the compiler what you want, which is what one should be using. }-- End of excerpt from Don Lee
Re: racy acccess in kern_runq.c
On Dec 6, 3:02pm, Jason Thorpe wrote: } > On Dec 6, 2019, at 11:44 AM, paul.kon...@dell.com wrote: } > } > For clean semantics, I like ALGOL; too bad it is no longer used } } There's just too much shouting in ALGOL. Are you perhaps thinking of COBOL, which is traditionally all upper case. I could be mistaken since I've never written and likely have never seen ALGOL, but I have written COBOL. }-- End of excerpt from Jason Thorpe
Re: racy acccess in kern_runq.c
Please educate me. It’s been a while for me. Writing Kernel code *requires* knowledge of what code is generated sometimes. In my experience, there have been standard techniques, like pragmas and insertions of assembly code to suppress this sort of undesirable optimization. Don’t those techniques exist any more? My compiler friends used to put them in for just this purpose, and they tried to make them as portable as possible. Surely GCC does this. No? Inquiring minds… -dgl- > On Dec 6, 2019, at 1:44 PM, > wrote: > > > >> On Dec 6, 2019, at 10:21 AM, Mouse wrote: >> >> >> [EXTERNAL EMAIL] >> >>> Compilers have became much more aggressive over the years. But they >>> are allowed to be so by the C standard. Specifically, in addition to >>> code-level re-ordering, plain accesses (loads/stores) are subject to >>> load/store fusing, tearing as well as invented loads/stores. >> >> Then, honestly, it sounds to me as though "the latest revision of C" is >> no longer an appropriate language for writing kernels. I see no reason >> to twist the kernel code into a pretzel to work around latitude a new >> language gives to its compilers - and that's what C11 is, a new >> language, albeit one closely related to various previous languages. >> >> One of the prime considerations when choosing a language and/or >> compiler for building a kernel is that it produce relatively >> predictable code, for exactly this kind of reason. If the latest C and >> the latest gcc no longer do that, then IMO they are no longer >> appropriate for writing/compiling kernels. > > C11 isn't all that new, of course. And I don't know if the rules about > optimization that you object to are anywhere near that new in any case. What > seems to be the case instead is that compilers are more and more doing what > the language has for a long time allowed them to do. > > Consider for example "Undefined behavior -- what happened to my code?" by > Wang et al. (MIT and Tsinghua University). It describes all sorts of allowed > transformations that come as a surprise to programmers. > > Yes, it certainly would be possible to create a programming language with > less surprising semantics. C is not in any way shape or form a clean > language with clean semantics. But as for wanting predictable code, that is > certainly doable. Unfortunately, it requires a sufficiently deep > understanding of the rules of the language. Typical textbooks are not all > that good for this, they are too superficial. The language standard tends to > be better. But again, a problem with most programming languages is that > their semantics are not well defined and/or much more complex than they > should be. Programmming in C++ is particularly scary for this reason, but C > is also problematic. For clean semantics, I like ALGOL; too bad it is no > longer used, though it did at one time serve to build successful operating > systems. > > paul >
Re: racy acccess in kern_runq.c
> On Dec 6, 2019, at 11:44 AM, paul.kon...@dell.com wrote: > > For clean semantics, I like ALGOL; too bad it is no longer used There's just too much shouting in ALGOL. -- thorpej
Re: racy acccess in kern_runq.c
> On Dec 6, 2019, at 10:21 AM, Mouse wrote: > > > [EXTERNAL EMAIL] > >> Compilers have became much more aggressive over the years. But they >> are allowed to be so by the C standard. Specifically, in addition to >> code-level re-ordering, plain accesses (loads/stores) are subject to >> load/store fusing, tearing as well as invented loads/stores. > > Then, honestly, it sounds to me as though "the latest revision of C" is > no longer an appropriate language for writing kernels. I see no reason > to twist the kernel code into a pretzel to work around latitude a new > language gives to its compilers - and that's what C11 is, a new > language, albeit one closely related to various previous languages. > > One of the prime considerations when choosing a language and/or > compiler for building a kernel is that it produce relatively > predictable code, for exactly this kind of reason. If the latest C and > the latest gcc no longer do that, then IMO they are no longer > appropriate for writing/compiling kernels. C11 isn't all that new, of course. And I don't know if the rules about optimization that you object to are anywhere near that new in any case. What seems to be the case instead is that compilers are more and more doing what the language has for a long time allowed them to do. Consider for example "Undefined behavior -- what happened to my code?" by Wang et al. (MIT and Tsinghua University). It describes all sorts of allowed transformations that come as a surprise to programmers. Yes, it certainly would be possible to create a programming language with less surprising semantics. C is not in any way shape or form a clean language with clean semantics. But as for wanting predictable code, that is certainly doable. Unfortunately, it requires a sufficiently deep understanding of the rules of the language. Typical textbooks are not all that good for this, they are too superficial. The language standard tends to be better. But again, a problem with most programming languages is that their semantics are not well defined and/or much more complex than they should be. Programmming in C++ is particularly scary for this reason, but C is also problematic. For clean semantics, I like ALGOL; too bad it is no longer used, though it did at one time serve to build successful operating systems. paul
Re: racy acccess in kern_runq.c
On Fri, Dec 06, 2019 at 09:00:33AM +, Andrew Doran wrote: > > Please don't commit this. These accesses are racy by design. There is no > safety issue and we do not want to disturb the activity of other CPUs in > this code path by locking them. We also don't want to use atomics either. I'm sorry, at this point I can't help myself: /* You are not expected to understand this. */ Thor
Re: racy acccess in kern_runq.c
Maxime Villard wrote: > Le 06/12/2019 à 17:53, Andrew Doran a écrit : > > On Fri, Dec 06, 2019 at 05:22:39PM +0900, Kengo NAKAHARA wrote: > > > >>/* Update the worker */ > >> - worker_ci = hci; > >> + atomic_swap_ptr(_ci, hci); > > > > Why atomic_swap_ptr() not atomic_store_relaxed()? I don't see any bug > > that it fixes. Other than that it look OK to me. > > Because I suggested it; my concern was that if not explicitly atomic, the > cpu could make two writes internally (even though the compiler produces > only one instruction), and in that case a page fault would have been > possible because of garbage dereference. No, atomic_store_relaxed() is sufficient; that is what it is for. -- Mindaugas
Re: racy acccess in kern_runq.c
On Fri, Dec 06, 2019 at 06:33:32PM +0100, Maxime Villard wrote: > Le 06/12/2019 à 17:53, Andrew Doran a écrit : > >Why atomic_swap_ptr() not atomic_store_relaxed()? I don't see any bug that > >it fixes. Other than that it look OK to me. > > Because I suggested it; my concern was that if not explicitly atomic, the > cpu could make two writes internally (even though the compiler produces > only one instruction), and in that case a page fault would have been possible > because of garbage dereference. atomic_store_relaxed() is not explicitly atomic? Dave -- David Young dyo...@pobox.comUrbana, IL(217) 721-9981
Re: racy acccess in kern_runq.c
Le 06/12/2019 à 17:53, Andrew Doran a écrit : On Fri, Dec 06, 2019 at 05:22:39PM +0900, Kengo NAKAHARA wrote: /* Update the worker */ - worker_ci = hci; + atomic_swap_ptr(_ci, hci); Why atomic_swap_ptr() not atomic_store_relaxed()? I don't see any bug that it fixes. Other than that it look OK to me. Because I suggested it; my concern was that if not explicitly atomic, the cpu could make two writes internally (even though the compiler produces only one instruction), and in that case a page fault would have been possible because of garbage dereference. To be honest, I'm not totally sure whether it is a valid concern; theoretically, it is.
Re: racy acccess in kern_runq.c
On Fri, Dec 06, 2019 at 05:22:39PM +0900, Kengo NAKAHARA wrote: > /* Update the worker */ > - worker_ci = hci; > + atomic_swap_ptr(_ci, hci); Why atomic_swap_ptr() not atomic_store_relaxed()? I don't see any bug that it fixes. Other than that it look OK to me. Cheers, Andrew
Re: racy acccess in kern_runq.c
On Fri, Dec 06, 2019 at 02:55:47PM +, Mindaugas Rasiukevicius wrote: > Compilers have became much more aggressive over the years. But they are > allowed to be so by the C standard. Specifically, in addition to code-level > re-ordering, Yup the rules around that are well understood. > plain accesses (loads/stores) are subject to load/store fusing, tearing as > well as invented loads/stores. At least load/store fusing and tearing > *have* been observed in reality [1] [2] [3]. So, they are *not* merely > theoretical or esoteric problems, although there are plenty of these in > the C11 memory model too [4] [5]. Thank you for the references, very interesting - if distressing - reading. So it does happen. > Linux kernel developers went through this already. Perhaps the C standard > will plug the holes or perhaps compilers will just change their behaviour, > as they get enough criticism [6] [7]. However, in the mean time, I think > let's just accept that things moved on and let's embrace the new primitives. > While these primitives might be slightly verbose, they are in C11, they fix > real bugs, they definitely make code less error-prone and they have other > merits too (e.g. they accommodate static analysers which find some real bugs). Well I'm firmly of the opinion that this is a bug in the compiler. By all means a nice behaviour to have as an option and I very much see the use of it, but having it as the default for compiling old code is bad news indeed. No argument from me on accommodating static analysers I think that's a good thing. Presumably then we have no other choice than to work around it, because even if we can tell the compiler not to be a jerk today, that option won't be feasible to use in the future. Thanks again, Andrew
Re: racy acccess in kern_runq.c
> Compilers have became much more aggressive over the years. But they > are allowed to be so by the C standard. Specifically, in addition to > code-level re-ordering, plain accesses (loads/stores) are subject to > load/store fusing, tearing as well as invented loads/stores. Then, honestly, it sounds to me as though "the latest revision of C" is no longer an appropriate language for writing kernels. I see no reason to twist the kernel code into a pretzel to work around latitude a new language gives to its compilers - and that's what C11 is, a new language, albeit one closely related to various previous languages. One of the prime considerations when choosing a language and/or compiler for building a kernel is that it produce relatively predictable code, for exactly this kind of reason. If the latest C and the latest gcc no longer do that, then IMO they are no longer appropriate for writing/compiling kernels. > While these primitives might be slightly verbose, they are in C11, > they fix real bugs, they definitely make code less error-prone and > they have other merits too (e.g. they accommodate static analysers > which find some real bugs). How many of those "real bugs" exist only because C11 gave compilers new latitude to produce unexpected code? /~\ The ASCII Mouse \ / Ribbon Campaign X Against HTMLmo...@rodents-montreal.org / \ Email! 7D C8 61 52 5D E7 2D 39 4E F1 31 3E E8 B3 27 4B
Re: racy acccess in kern_runq.c
Le 06/12/2019 à 11:28, Andrew Doran a écrit :
> On Fri, Dec 06, 2019 at 10:27:20AM +0100, Maxime Villard wrote:
>
>> With 'worker_ci', there is an actual safety issue, because the compiler could
>> split the accesses and the hardware may not use atomics by default like x86.
>> This could cause random page faults; so it needs to be strictly atomic.
>
> No I don't accept that.
>
> The ability to load and store a native word sized int (and in more recent
> years a pointer) with a single instruction is a fundamental assumption that
> every operating system written in C rests upon.
1) That it is done with a single instruction, does not necessarily mean it is
atomic. There are many stupid things microcodes are allowed to do (not on x86,
however).
2) That there is a one-line assignment of a variable in C, does not necessarily
mean that there will be one move instruction. There are many things the compiler
is allowed to do in optimization passes, as said already.
3) "every operating system written in C rests upon" I have some difficulty with
that. Linux has our equivalent of relaxed atomics (called {READ,WRITE}_ONCE),
with 1000+ combined references in their tree. So no, here's a big open source
kernel that does not rest upon unclear assumptions for lockless operations. No
way to check Windows, but I wouldn't be surprised if they had a similar
approach.
> If the compiler splits one of those acceses, then you are either using some
> other data type, or have a broken compiler on your hands.
Or you're just using a compiler that knows well enough how to arrange
instructions for optimized pipelining, and this compiler decides to re-order
the accesses based on desirable performance properties, as allowed by the
standard.
> https://www.gnu.org/software/libc/manual/html_node/Atomic-Types.html
I don't see anything in this link that is really reassuring. "you can assume"
that things are mostly ok on "systems we know of".
At least the rationale for {READ,WRITE}_ONCE is based on standards, and not
wild assumptions:
https://github.com/google/ktsan/wiki/READ_ONCE-and-WRITE_ONCE
To me, Kengo's 2nd patch remains needed.
Maxime
Re: racy acccess in kern_runq.c
On Fri, Dec 06, 2019 at 10:27:20AM +0100, Maxime Villard wrote: > With 'worker_ci', there is an actual safety issue, because the compiler could > split the accesses and the hardware may not use atomics by default like x86. > This could cause random page faults; so it needs to be strictly atomic. No I don't accept that. The ability to load and store a native word sized int (and in more recent years a pointer) with a single instruction is a fundamental assumption that every operating system written in C rests upon. If the compiler splits one of those acceses, then you are either using some other data type, or have a broken compiler on your hands. If the compiler is broken it's the compiler you should be looking at, not the program it compiled. It's as simple as that. https://www.gnu.org/software/libc/manual/html_node/Atomic-Types.html Andrew
Re: racy acccess in kern_runq.c
Hi, On 2019/12/06 18:00, Andrew Doran wrote: Hi, On Fri, Dec 06, 2019 at 03:52:23PM +0900, Kengo NAKAHARA wrote: 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. Please don't commit this. These accesses are racy by design. There is no safety issue and we do not want to disturb the activity of other CPUs in this code path by locking them. We also don't want to use atomics either. This code path is extremely hot using atomics would impose a severe performance penalty on the system under certain workloads. I see, I don't commit it. Indeed, I am worry about performance degradation. Also if you change this to use strong ordering, you're quite likely to change the selection behaviour of LWPs. This is something delicate that exhibits reasonable scheduling behaviour in large part through randomness i.e by accident, so serialising it it's likely to have strong effects on how LWPs are distributed. Lastly I have a number of experimental changes to this code which I'll be committing in the near future allowing people to change the LWP selection policy to see how if we can improve performance under different workloads. They will also likely show up in KCSAN as racy/dangerous accesses. My suggestion is to find a way to teach KCSAN that we know something is racy, we like it being racy, and that it's not a problem, so that it no longer shows up in the KCSAN output. I see. I will add __nocsan attribute to the functions in my local sources to find other races. Thanks, -- // Internet Initiative Japan Inc. Device Engineering Section, Product Development Department, Product Division, Technology Unit Kengo NAKAHARA
Re: racy acccess in kern_runq.c
Le 06/12/2019 à 10:00, Andrew Doran a écrit :
> Hi,
>
> On Fri, Dec 06, 2019 at 03:52:23PM +0900, Kengo NAKAHARA wrote:
>
>> 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.
>
> Please don't commit this. These accesses are racy by design. There is no
> safety issue and we do not want to disturb the activity of other CPUs in
> this code path by locking them. We also don't want to use atomics either.
> This code path is extremely hot using atomics would impose a severe
> performance penalty on the system under certain workloads.
With 'worker_ci', there is an actual safety issue, because the compiler could
split the accesses and the hardware may not use atomics by default like x86.
This could cause random page faults; so it needs to be strictly atomic.
Apart from that, yes, there is no other safety issue and locking would be too
expensive. All we possibly care about is making sure the accesses aren't split,
for the sake of not basing the scheduling policy on systematic garbage, and
atomic_relaxed seems like the right thing to do (Kengo's 2nd patch),
considering that it costs ~nothing.
> Also if you change this to use strong ordering, you're quite likely to
> change the selection behaviour of LWPs. This is something delicate that
> exhibits reasonable scheduling behaviour in large part through randomness
> i.e by accident, so serialising it it's likely to have strong effects on how
> LWPs are distributed.
>
> Lastly I have a number of experimental changes to this code which I'll be
> committing in the near future allowing people to change the LWP selection
> policy to see how if we can improve performance under different workloads.
> They will also likely show up in KCSAN as racy/dangerous accesses.
>
> My suggestion is to find a way to teach KCSAN that we know something is
> racy, we like it being racy, and that it's not a problem, so that it no
> longer shows up in the KCSAN output.
Maybe we should indeed have a macro to say "yes this access is racy but we
don't care". Currently this macro is atomic_{store,load}_relaxed()...
Re: racy acccess in kern_runq.c
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_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(_ci, hci);
>
> if (nocallout == NULL)
> callout_schedule(_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 = >ci_schedstate;
>
It just so happens we were talking about this yesterday.
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.
Re: racy acccess in kern_runq.c
Hi, On Fri, Dec 06, 2019 at 03:52:23PM +0900, Kengo NAKAHARA wrote: > 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. Please don't commit this. These accesses are racy by design. There is no safety issue and we do not want to disturb the activity of other CPUs in this code path by locking them. We also don't want to use atomics either. This code path is extremely hot using atomics would impose a severe performance penalty on the system under certain workloads. Also if you change this to use strong ordering, you're quite likely to change the selection behaviour of LWPs. This is something delicate that exhibits reasonable scheduling behaviour in large part through randomness i.e by accident, so serialising it it's likely to have strong effects on how LWPs are distributed. Lastly I have a number of experimental changes to this code which I'll be committing in the near future allowing people to change the LWP selection policy to see how if we can improve performance under different workloads. They will also likely show up in KCSAN as racy/dangerous accesses. My suggestion is to find a way to teach KCSAN that we know something is racy, we like it being racy, and that it's not a problem, so that it no longer shows up in the KCSAN output. Thank you, Andrew
Re: racy acccess in kern_runq.c
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_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(_ci, hci);
if (nocallout == NULL)
callout_schedule(_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 = >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_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_avgcount);
+ mcount = atomic_load_relaxed(>spc_mcount);
+ avg = (weight * avg + (100 - weight) * 256 * mcount) / 100;
+ atomic_store_relaxed(>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(_ci, hci);
if (nocallout == NULL)
callout_schedule(_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_count) != 0) {
return;
}
/* Reset the counter, and call the balancer */
- spc->spc_avgcount = 0;
+ atomic_store_relaxed(>spc_avgcount, 0);
sched_balance(ci);
- tci = worker_ci;
+ atomic_swap_ptr(, worker_ci);
tspc = >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
racy acccess in kern_runq.c
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_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(_ci, hci);
if (nocallout == NULL)
callout_schedule(_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 = >ci_schedstate;
Can I commit this patch?
Thanks,
--
//
Internet Initiative Japan Inc.
Device Engineering Section,
Product Development Department,
Product Division,
Technology Unit
Kengo NAKAHARA
