From mboxrd@z Thu Jan 1 00:00:00 1970 From: Rusty Russell Subject: Re: Partial review of kvm arm git patches. Date: Tue, 07 Feb 2012 09:43:26 +1030 Message-ID: <87haz3imi1.fsf@rustcorp.com.au> References: <87obthj05g.fsf@rustcorp.com.au> Mime-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: QUOTED-PRINTABLE Cc: KVM General , android-virt , Hugh Dickins To: Christoffer Dall Return-path: Received: from ozlabs.org ([203.10.76.45]:47953 "EHLO ozlabs.org" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1756085Ab2BGAaM convert rfc822-to-8bit (ORCPT ); Mon, 6 Feb 2012 19:30:12 -0500 In-Reply-To: Sender: kvm-owner@vger.kernel.org List-ID: On Thu, 2 Feb 2012 09:48:22 -0500, Christoffer Dall wrote: > On Thu, Feb 2, 2012 at 12:04 AM, Rusty Russell wrote: > > Hi all, > > > > =C2=A0 =C2=A0 =C2=A0 =C2=A0Started reading through the git tree at > > git://github.com/virtualopensystems/linux-kvm-arm.git (kvm-a15-v6-s= tage > > branch), and noticed some things. =C2=A0I'm learning ARM as I go, s= o > > apologies in advance for any dumb questions. > > > > Firstly, I want to say that reading this code was a pleasant surpri= se! > > Many of my comments below are nit-picking... >=20 > thanks. >=20 > I am a little confused though, did you not already send a lot of thes= e > 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 clearl= y 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 commen= t as I go. Doing the latest version seemed sensible, though it means som= e comments are trivial. > >> + =C2=A0 =C2=A0 addr =3D PAGE_OFFSET; > >> + =C2=A0 =C2=A0 end =3D ~0; > >> + =C2=A0 =C2=A0 do { > >> + =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 next =3D pgd_addr_end(= addr, end); > >> + =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 pgd =3D hyp_pgd + pgd_= index(addr); > >> + =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 pud =3D pud_offset(pgd= , addr); > >> + > >> + =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 BUG_ON(pud_bad(*pud)); > >> + > >> + =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 if (pud_none(*pud)) > >> + =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2= =A0 continue; > >> + > >> + =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 pmd =3D pmd_offset(pud= , addr); > >> + =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 free_ptes(pmd, addr); > >> + =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 pmd_free(NULL, pmd); > >> + =C2=A0 =C2=A0 } while (addr =3D next, addr !=3D end); > > > > All your loops are like this, but I think a for () is sufficient. > > I don't think any of them are called start =3D=3D end anyway, and i= t > > wouldn't matter if they were... > > >=20 > 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= =2E =46or non-mm hackers, it's just a confusing mess. This is what that lo= op is actually doing, written in normal C: for (addr =3D PAGE_OFFSET; addr !=3D 0; addr +=3D PGDIR_SIZE) { pgd =3D hyp_pgd + pgd_index(addr); pud =3D pud_offset(pgd, addr); BUG_ON(pud_bad(*pud)); if (pud_none(*pud)) continue; pmd =3D 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. =C2=A0I= f the > > function ever becomes unused, it's nice if gcc tells you. =C2=A0Tho= ugh maybe > > here that's a feature? > > >=20 > just a hint to the compiler to inline. It will probably do that > anyway, and I like your argument, so sure. >=20 > 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 wit= h -Os vs -O2. Thus, I consider inline the register keyword of the 90s :) > > The accretion of loose emulation leads to QEMU-style emulation, whe= re > > you can't distinguish sloppiness from deliberate hacks, and you're > > terrified to clean up anything because some weird setup will break.= =C2=A0I'd > > really rather see us emulating as tightly as possible. > > >=20 > Ok. There should be no deliberate hacks in there. So anything > imprecise is sloppiness or at least something incomplete. >=20 > 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). > > >=20 > 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.