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

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 18.08.16 at 14:18, <daniel.kiper@xxxxxxxxxx> wrote:
> On Thu, Aug 18, 2016 at 03:41:06AM -0600, Jan Beulich wrote:
>> >>> On 18.08.16 at 10:53, <daniel.kiper@xxxxxxxxxx> wrote:
>> > On Thu, Aug 11, 2016 at 08:17:58AM -0600, Jan Beulich wrote:
>> >> >>> On 11.08.16 at 16:12, <JBeulich@xxxxxxxx> wrote:
>> >> >>>> On 06.08.16 at 01:04, <daniel.kiper@xxxxxxxxxx> 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.
> 
> alloc_mem() is mostly used by callers playing with numbers only. copy_mem()
> is only one user of it which plays with pointers. However, copy_mem() 
> returns
> numbers, so, wrapper does not change a lot. It just moves casts to other 
> places.
> Am I missing something?

I can't easily tell without seeing a tree with everything applied.

Jan


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