lkml.org 
[lkml]   [2007]   [Nov]   [15]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: [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/

\
 
 \ /
  Last update: 2007-11-15 23:21    [W:0.165 / U:0.368 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site