On Thu, 2 Feb 2012 09:48:22 -0500, Christoffer Dall <cdall@xxxxxxxxxxxxxxx> wrote:
> On Thu, Feb 2, 2012 at 12:04 AM, Rusty Russell <rusty@xxxxxxxxxxxxxxx> wrote:
> > Hi all,
> >
> > Started reading through the git tree at
> > git://github.com/virtualopensystems/linux-kvm-arm.git (kvm-a15-v6-stage
> > branch), and noticed some things. I'm learning ARM as I go, so
> > apologies in advance for any dumb questions.
> >
> > Firstly, I want to say that reading this code was a pleasant surprise!
> > Many of my comments below are nit-picking...
>
> thanks.
>
> I am a little confused though, did you not already send a lot of these
> comments yesterday?
Ah, yes. I stopped and started over a couple of days, and accidentally
sent off a draft. I thought I killed it before it went out, but clearly
not...
> Also, I appreciate very much the review, but remember that I am not
> done with this patch series - which is why it has not been sent out
> yet but merely pushed to a staging branch.
Sure... I wanted to read the code, and I figured I might as well comment
as I go. Doing the latest version seemed sensible, though it means some
comments are trivial.
> >> + addr = PAGE_OFFSET;
> >> + end = ~0;
> >> + do {
> >> + next = pgd_addr_end(addr, end);
> >> + pgd = hyp_pgd + pgd_index(addr);
> >> + pud = pud_offset(pgd, addr);
> >> +
> >> + BUG_ON(pud_bad(*pud));
> >> +
> >> + if (pud_none(*pud))
> >> + continue;
> >> +
> >> + pmd = pmd_offset(pud, addr);
> >> + free_ptes(pmd, addr);
> >> + pmd_free(NULL, pmd);
> >> + } while (addr = next, addr != end);
> >
> > All your loops are like this, but I think a for () is sufficient.
> > I don't think any of them are called start == end anyway, and it
> > wouldn't matter if they were...
> >
>
> I don't understand this point. Why is a for() better. This scheme is
> used elsewhere in the kernel as well.
Indeed, there's a habit of doing this in the mm code, but that's very
old code, predating Hugh's introduction of pmd_addr_end() et al in 2005.
For non-mm hackers, it's just a confusing mess. This is what that loop
is actually doing, written in normal C:
for (addr = PAGE_OFFSET; addr != 0; addr += PGDIR_SIZE) {
pgd = hyp_pgd + pgd_index(addr);
pud = pud_offset(pgd, addr);
BUG_ON(pud_bad(*pud));
if (pud_none(*pud))
continue;
pmd = pmd_offset(pud, addr);
free_ptes(pmd, addr);
pmd_free(NULL, pmd);
}
This version doesn't make me wonder what I'm missing about the loop
boundaries.
> > As a general rule, I prefer not to see "inline" in C files. If the
> > function ever becomes unused, it's nice if gcc tells you. Though maybe
> > here that's a feature?
> >
>
> just a hint to the compiler to inline. It will probably do that
> anyway, and I like your argument, so sure.
>
> Is the inline hint completely ignored by the compiler in terms of
> actually inlining or not?
No, it works. But gcc keeps getting smarter: it will inline any
called-once function anyway, and the threshold for inlining changes with
-Os vs -O2.
Thus, I consider inline the register keyword of the 90s :)
> > The accretion of loose emulation leads to QEMU-style emulation, where
> > you can't distinguish sloppiness from deliberate hacks, and you're
> > terrified to clean up anything because some weird setup will break. I'd
> > really rather see us emulating as tightly as possible.
> >
>
> Ok. There should be no deliberate hacks in there. So anything
> imprecise is sloppiness or at least something incomplete.
>
> So I am not sure what you are suggesting. Something completely
> different or just fixing up the emulation code?
I'm suggesting fixing it up. Peter has some WIP QEMU patches to change
its emulation to a table-driven approach, I'd like to try the same kind
of thing here.
> > (And unit testing our emulation code, but that's another topic).
> >
>
> If you want to come up with a nice framework for this, be my guest ;)
OK, I'll think harder about this...
> Looking forward for more to come. I will try to release a v6 series
> real soon, so you don't have to spend time on stuff that is merged in
> the wrong patch etc.
Yeah, I'll go through that once you've posted it.
Every time I re-read this stuff, I learn a little more. Thanks!
Cheers,
Rusty.
--
To unsubscribe from this list: send the line "unsubscribe kvm" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
