[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [Xen-devel] [PATCH v3 5/4] x86: reduce code size of struct cpu_info member accesses



>>> On 30.03.16 at 16:28, <konrad.wilk@xxxxxxxxxx> wrote:
> On Tue, Mar 29, 2016 at 12:59:24AM -0600, Jan Beulich wrote:
>> >>> On 25.03.16 at 19:47, <konrad.wilk@xxxxxxxxxx> wrote:
>> > On Thu, Mar 17, 2016 at 10:14:22AM -0600, Jan Beulich wrote:
>> > 
>> > Something is off with your patch. This is 5/4 :-)
>> 
>> Well, yes - this got added later on top of the previously sent series,
>> to make the dependency obvious.
>> 
>> >> Instead of addressing these fields via the base of the stack (which
>> >> uniformly requires 4-byte displacements), address them from the end
>> >> (which for everything other than guest_cpu_user_regs requires just
>> >> 1-byte ones). This yields a code size reduction somewhere between 8k
>> >> and 12k in my builds.
>> > 
>> > Also you made the macro a bit different - the %r is removed.
>> > 
>> > Particular reason? 
>> 
>> This is an integral part of the change, so the macro can derive
>> e.g. both %eax and %rax from the passed argument
> 
> Could you pls include that explanation in the commit description..
> 
> And with that you can put Reviewed-by: Konrad Rzeszutek Wilk 
> <konrad.wilk@xxxxxxxxxx>
> 
> on the patch.

Well, I've got one of these already, without having been asked to
do any adjustments. Did I mis-interpret
<20160325184701.GE20741@xxxxxxxxxxxxxxxxxx>?
As to adding something like this to the commit message: I have
a hard time seeing how this would belong there. The fact _that_
this is being done is very obvious from the patch itself, and once
the reader has spotted this the fact _why_ this is needed then
should become immediately obvious too. I'm all for explaining
technically difficult or hard to see things, but I'm against
needless bloat.

Jan


_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel

 


Rackspace

Lists.xenproject.org is hosted with RackSpace, monitoring our
servers 24x7x365 and backed by RackSpace's Fanatical Support®.