Re: [PATCH v5 10/14] nEPT: Add nEPT violation/misconfigration support

[Gleb Natapov <gleb@redhat.com>]

On Thu, Aug 01, 2013 at 03:13:02PM +0200, Paolo Bonzini wrote:
> > > > > > > Not sure this explicit "inline" is needed... Gcc always inlines the small and
> > > > > > > static functions.
> > > > > > 
> > > > > > Paolo asked for it, but now I see that I did in in a wrong patch. I do
> > > > > > not care much personally either way, I agree with you though, compiler
> > > > > > will inline it anyway.
> > > > > 
> > > > > The point here was that if we use "||" below (or multiple "if"s as I
> > > > > suggested in my review), we really want to inline the function to ensure
> > > > > that the branches here is merged with the one in the caller's "if()".
> > > > > 
> > > > > With the "|" there is not much effect.
> > > > 
> > > > Even with if() do you really think there is a chance the function will not be
> > > > inlined? I see that much much bigger functions are inlined.
> > > 
> > > I don't know, it depends on the compiler flags, how much the function
> > > is used...  Zeroing the chance is not bad.
> > 
> > AFAIK inline is just a hint anyway and compiler is free to ignore it.
> > That is why we have __always_inline, but compiler should know better
> > here, do not see the reason for __always_inline.
> 
> Yes, but with "inline" the compiler will use more generous thresholds.
> 
> The GCC docs say that without "inline", -O2 only inlines "functions into
> their callers when their body is smaller than expected function call
> code (so overall size of program gets smaller)".  I'm not at all sure
> this is the case for the new is_rsvd_bits_set, and anyway "making the
> program smaller" is not the reason why we want the compiler to inline it.
> 
> Did you check that the compiler inlines it?  Perhaps you should really
> use __always_inline since that's what we really want.
> 
Since I am going to use | and not || you were saying inline is not
needed, no? But yes, I do see that compiler inlines it. I see that it
inlines even update_accessed_dirty_bits() which is much bigger.

> > > Because the function is "bool".  I dislike the magic "!= 0"
> > > that the compiler adds on conversion to bool.  It always seemed
> > > like a recipe for trouble since "int" and "bool" are otherwise
> > > interchangeable...  Without that "!= 0", s/bool/int/ would ignore
> > > the upper 32 bits and break.
> > 
> > I actually checked that before proposing.
> > 
> > printf("%d\n", (bool)0x1000000000) prints one, but of course if bool is
> > typedefed to int it will not work, but it should be not.
> 
> No, it should not be indeed, but not everyone uses bool in the same way;
> it is quite common to use "int" for something that is 0/1, and the magic
> "!= 0" is dangerous if you cut-and-paste the expression where the compiler
> will not do it...  It can even be a function argument where you do not
> see directly if it is bool, int, u64 or what.
> 
> I don't think omitting "!= 0" is common at all in the kernel, so I would
> not start doing it here. :)
> 
OK, will leave != 0. But I checked and Linux devices bool to be _Bool,
so we should be safe here.

--
			Gleb.