> Yes, the code could be reworked by moving some of the data from the CPU
I don't quite understand that characterization of the kind of change I'm
advocating. If the common case path in context switch has really anything
at all more than the example I gave, something is wrong.
Why does it matter? When a new user breakpoint was made the
highest-priority one, it ought to update tdr[0..3] right then before the
registration call returns. It seems fine to me for it to make an
uninstalled callback right away rather than at the thread's next switch-in.
But even if you wanted to delay it, you could just set active_dr7 to zero
or something so that the unlikely case triggers.
The plan I suggested relies on setting want_dr7 with the enable bits that
do include the ones the kernel uses (for contested slots). Of course it
works as well to use either bit for this, as long as you're consistent.
But as I've said at least twice already, there is no actual meaning
whatsoever to choosing one enable bit over the other. It's just confusing
and misleading to have the code make special efforts to set one rather than
the other for different cases. You talk about them as if they meant
something, which keeps making me wonder if you're confused. Since the
hardware doesn't care which bit you set, you could overload them to record
a bit and a half of information there if really wanted to, but you're not
even doing that, unless I'm confused.
How would that happen? This would mean that some user process has been
allowed to enable ioperm for some io port that kernel drivers also send to
from interrupt handlers. Can that ever happen?
The purpose of the chbi area is to optimize this path. Make it store
whatever precomputed values are most convenient for the hot paths. This
path doesn't need num_kbps, it needs kdr7. So precompute that and do that
one load, instead of a load of chbi->num_bkps we don't otherwise need plus
a load from kdr7_masks that can be avoided altogether on hot paths.
I don't really know about the slowness of reading debug registers, though I
would guess it is slower than most common operations. But regardless, you
can avoid it because kdr7 is something you need anyway, so you're not
replacing it with a load but letting a load you already had kill two birds.
I don't really get why user breakpoints would be in chbi->bps at all.
When a debug trap hits, you can check kdr7 or whatnot to see if it was a
kernel allocation, and otherwise look in current->thbi->bps to find it.
Agreed. (I just used DR_LEN_1 as shorthand and was not hot on including
asm/debugreg.h in asm/hw_breakpoint.h in the actual version.)
There's no way to tell which of the 8 bytes were accessed, AFAIK. It's the
same as LEN8 on x86_64 or LEN[42] on i386: some byte in there was accessed.
That is probably fine too.
You're not suggesting this for lengths 2, 4, or 8 on i386/x86_64, and I
don't see the distinction. In all those cases, any low bits set in the
address are being ignored. I think it is much better to enforce the
alignment so that callers are told explicitly what their parameteres really
mean. A caller passing an unaligned address with DR_LEN_4 might be
thinking it will catch the four bytes starting at that unaligned byte,
which is not true.
How could that ever be used that would not be racy and thus buggy? A
registration call on another CPU could cause a change and callback just
after you fetched the value.
You keep rewriting it and I'll keep changing my mind!
(Just kidding, but fair warning. ;-)
I look forward to it.
Thanks,
Roland
-
