| Message ID | 20230513121226.99723-2-npiggin@gmail.com |
|---|---|
| State | Accepted |
| Headers | show |
| Series | ipmi misc fixes | expand |
> On May 13, 2023, at 05:12, Nicholas Piggin <npiggin@gmail.com> wrote: > > Current gcc with -Os happens to generate code that re-loads the variable > in the loop, but that could change without notice, and with -O2 it does > infinite loop if sync_msg is !NULL, because it is not declared volatile > and there is no compiler barrier in the loop. I guess it’s a good thing we only ever had size constraints rather than performance ones :) > Add the usual cpu_relax() there to provide the compiler barrier. I’m trying to remember if we needed a call to go do background work if only one CPU thread but it turns out my memory is getting hazy on the details, and I 100% need more coffee right now. > Signed-off-by: Nicholas Piggin <npiggin@gmail.com> Feel free to have my first skiboot reviewed by in a while though :) Reviewed-by: Stewart Smith <stewart@flamingspork.com <mailto:stewart@flamingspork.com>>
On Mon May 15, 2023 at 6:29 AM AEST, Stewart Smith wrote: > > > On May 13, 2023, at 05:12, Nicholas Piggin <npiggin@gmail.com> wrote: > > > > Current gcc with -Os happens to generate code that re-loads the variable > > in the loop, but that could change without notice, and with -O2 it does > > infinite loop if sync_msg is !NULL, because it is not declared volatile > > and there is no compiler barrier in the loop. > > I guess it’s a good thing we only ever had size constraints rather than performance ones :) Yeah, I only found it by inspection when looking for that IPMI issue I had with kvm-unit-tests. > > Add the usual cpu_relax() there to provide the compiler barrier. > > I’m trying to remember if we needed a call to go do background work if only one CPU thread but it turns out my memory is getting hazy on the details, and I 100% need more coffee right now. Interesting point. We have time_wait vs time_wait_nopoll of course, which get to the issue you're talkingabout. And there *are* a few cpu_relax loops that also do some polling explicitly. Might be worth auditing them all (there aren't too many) and maybe rename to cpu_relax_nopoll() so it doesn't just get used without considering pollers... In this case the other CPU that owns the sync_msg that we're waiting to release is doing polling, so I think that's alright. > > Signed-off-by: Nicholas Piggin <npiggin@gmail.com> > > Feel free to have my first skiboot reviewed by in a while though :) > > Reviewed-by: Stewart Smith <stewart@flamingspork.com <mailto:stewart@flamingspork.com>> Thanks, Nick
On Sun, May 14, 2023 at 01:29:00PM -0700, Stewart Smith wrote: >Feel free to have my first skiboot reviewed by in a while though :) > >Reviewed-by: Stewart Smith <stewart@flamingspork.com <mailto:stewart@flamingspork.com>> Man, it really has been a while. HTML email?! :)
> On Jun 6, 2023, at 12:23, Reza Arbab <arbab@linux.ibm.com> wrote: > > On Sun, May 14, 2023 at 01:29:00PM -0700, Stewart Smith wrote: >> Feel free to have my first skiboot reviewed by in a while though :) >> >> Reviewed-by: Stewart Smith <stewart@flamingspork.com <mailto:stewart@flamingspork.com>> > > Man, it really has been a while. HTML email?! :) Damn Apple Mail!
diff --git a/core/ipmi.c b/core/ipmi.c index bbc1a7b6..59aa95fc 100644 --- a/core/ipmi.c +++ b/core/ipmi.c @@ -170,7 +170,8 @@ void ipmi_queue_msg_sync(struct ipmi_msg *msg) } lock(&sync_lock); - while (sync_msg); + while (sync_msg) + cpu_relax(); sync_msg = msg; if (msg->backend->disable_retry && !opal_booting()) msg->backend->disable_retry(msg);
Current gcc with -Os happens to generate code that re-loads the variable in the loop, but that could change without notice, and with -O2 it does infinite loop if sync_msg is !NULL, because it is not declared volatile and there is no compiler barrier in the loop. Add the usual cpu_relax() there to provide the compiler barrier. Signed-off-by: Nicholas Piggin <npiggin@gmail.com> --- core/ipmi.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-)