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

[Daniel Kiper <daniel.kiper@xxxxxxxxxx>]

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?

Daniel

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