Messages in this thread | | | Date | Thu, 15 Nov 2007 23:17:29 +0100 (CET) | From | Thomas Gleixner <> | Subject | Re: [PATCH] x86: on x86_64, correct reading of PC RTC when update in progress in time_64.c |
| |
On Thu, 15 Nov 2007, David P. Reed wrote:
> There are a couple of things I don't understand on this one. And I presume > you thought the other two bug fixing patches I sent before this were OK to go, > since on my system
I had to fix up all of them.
> Thomas Gleixner wrote: > > > Still whitespace wreckage in your patches. I guess the kernel tree you > > made your patches against is already white space wrecked. > > > > I fixed that up manually, but please be more careful about that next > > time. > > Um ... I fixed the whitespaces I detected from the first round with > checkpatch.pl. Then for good measure > I ran checkpatch.pl on the patches, then pasted the files directly into the > emails. No problems detected.
Never paste patches into mail. Also I should have checked your mail client earlier. Thunderbird is famous for this :) Documentation/email-clients.txt has some info on that.
> And I also just tried checkpatch.pl on the "sent" folder copy. No problems > detected there.
Try to apply your own patches right from the sent folder copy. They will reject.
> Where was the whitespace? Was it in the patches? Would you mind showing me > the output so I can do a better job in the future?
Well the output was a simple reject when applying the patch.
Whitespace damage was for example here:
unsigned long flags; 20 20 09 75 6e 73 69 67 6e 65 64 20 6c 6f 6e 67 20 66 6c 61 67 73 3b 0a
where the correct patch should read:
20 09 75 6e 73 69 67 6e 65 64 20 6c 6f 6e 67 20 66 6c 61 67 73 3b 0a
Your mailer or the paste added a second blank (0x20) at the beginning of the line.
> > > Correct potentially unstable PC RTC time register reading in time_64.c > > > > > > Stop the use of an incorrect technique for reading the standard PC RTC > > > timer, which is documented to "disconnect" time registers from the bus > > > while updates are in progress. The use of UIP flag while interrupts > > > are disabled to protect a 244 microsecond window is one of the > > > Motorola spec sheet's documented ways to read the RTC time registers > > > reliably. > > > > > > The patch updates the misleading comments and also minimizes the amount of > > > time that the kernel disables interrupts during the reading. > > > > > > > While I think that the UIP change is correct and a must have, the > > locking change is not really useful. read_persistent_clock is called > > from exactly three places: > > > What locking change? I didn't change how locking works in > read_persistent_clock at all. > I did introduce cpu_relax() because if anyone else ever calls from a hot path, > that would be good practice and its' one line.
Hot path calls to this code would be extremly stupid and are forbidden by the Penguin Law. :)
cpu_relax is not the problem, but you actuall changed the locking:
Old code:
spin_lock(); do { .... } while (stupid check); spin_unlock();
New code: while (1) { spin_lock(); if (useful_check) { spin_unlock(); cpu_relax(); } else break; }
So with the old code I can take out the inner loop and do what paravirtualization in the 32 bit code does:
spin_lock_irqsave(&rtc_lock, flags); result = get_wallclock(); spin_unlock_irqrestore(&rtc_lock, flags);
Where get_wallclock() either resolves to the plain rtc code or to the paravirtualized function depending on the boot mode of the kernel.
With your change we either would need to put lokck/unlock of rtc_lock into each incarnation of paravirt or do some nasty hack, where we convey the flags to the called function. Neither of this makes sense and is worth the work.
> > Right after boot, right before suspend and right after resume. None of > > those places is a hot path, where we really care about the interrupt > > enable/disable. IIRC, this is even called with interrupts disabled > > most of the time, so no real gain here. > > > > Another reason not to do the locking change is the paravirt stuff > > which is coming for 64bit. I looked into the existing 32bit code and > > doing the same lock thing would introduce a real nasty hackery, which > > is definitely not worth the trouble. > > > I presume time_64.c and time_32.c will be unified at some point, discarding > time_64.c. There's no arch-specific reason to be separate. The current > time_32.c depends on a different nmi path (that does some weird stuff saving > and restoring the CMOS index register!), and I didn't dare usurp your > long-term plan to unify architectures. But a simple cleanup here makes sense, > lest someone copy the bad technique as if it were good.
Yeah, those files are on the radar. The cleanup branch of the x86 git tree has a first go on this already. And I'm currently figuring out what we can do about this ugly CMOS index hack.
Anyway I keep the locking straight and simple as it is right now, the cpu_relax() works fine with a lock held, while we are waiting the bunch of usecs for UIP going away.
Thanks,
tglx - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
| |