Re: [PATCH v4 06/19] x86/boot/reloc: create generic alloc and copy functions

From: "Jan Beulich" <JBeulich@suse.com>
To: Daniel Kiper <daniel.kiper@oracle.com>
Cc: Juergen Gross <JGross@suse.com>,
	sstabellini@kernel.org, andrew.cooper3@citrix.com,
	cardoe@cardoe.com, pgnet.dev@gmail.com, ning.sun@intel.com,
	david.vrabel@citrix.com, xen-devel@lists.xenproject.org,
	qiaowei.ren@intel.com, richard.l.maliszewski@intel.com,
	gang.wei@intel.com, fu.wei@linaro.org
Subject: Re: [PATCH v4 06/19] x86/boot/reloc: create generic alloc and copy functions
Date: Thu, 18 Aug 2016 03:41:06 -0600	[thread overview]
Message-ID: <57B59ED20200007800106F79@prv-mh.provo.novell.com> (raw)
In-Reply-To: <20160818085326.GK3476@olila.local.net-space.pl>

>>> On 18.08.16 at 10:53, <daniel.kiper@oracle.com> wrote:
> On Thu, Aug 11, 2016 at 08:17:58AM -0600, Jan Beulich wrote:
>> >>> On 11.08.16 at 16:12, <JBeulich@suse.com> wrote:
>> >>>> On 06.08.16 at 01:04, <daniel.kiper@oracle.com> wrote:
>> >> --- a/xen/arch/x86/boot/reloc.c
>> >> +++ b/xen/arch/x86/boot/reloc.c
>> >> @@ -32,60 +32,69 @@ typedef unsigned int u32;
>> >>
>> >>  static u32 alloc;
>> >>
>> >> -static void *reloc_mbi_struct(void *old, unsigned int bytes)
>> >> +static u32 alloc_mem(u32 bytes)
>> >
>> > Conversion of alloc to be of pointer type (in the earlier patch), and
>> > then making the return type here and ...
>> >
>> >> +static u32 copy_mem(u32 src, u32 bytes)
>> >
>> > ... all of the types here follow suit would apparently be quite
>> > beneficial to the number of casts needed.
>>
>> Or maybe, considering patch 8, in a slight variation thereof: Do
>> the conversion as suggested, but have a helper wrapper of the
>> type above, taking care of all the casting. That way both the
>> actual implementation and the callers can stay (mostly) cast free.
> 
> We should take into account patch 9 here too. Looking at code after
> it I think that right now it is very well optimized in terms of casts.
> I cannot see room for further improvement. Every change you proposed
> here and there does not improve final code. It justs move/change casts
> to/in different places. So, I think that it does not pay change casts
> here and in earlier patches. At least in the way you proposed until now.

What I've suggested above at least makes both the actual function
and its wrapper consistent, and hence usable (without casts) by
callers dealing with either only numbers of only pointers. Are you
saying there are no such "clean" callers? That would put the overall
code in a pretty bad light imo.

Jan

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