Re: [PATCH v3 2/9] kexec_file: Generalize kexec_add_buffer.

From: Thiago Jung Bauermann
Date: Fri Jul 01 2016 - 13:51:29 EST


Am Donnerstag, 30 Juni 2016, 17:43:57 schrieb Dave Young:
> On 06/30/16 at 01:42pm, Thiago Jung Bauermann wrote:
> > Am Donnerstag, 30 Juni 2016, 12:49:44 schrieb Thiago Jung Bauermann:
> > > To be honest I think struct kexec_buf is an implementation detail
> > > inside
> > > kexec_locate_mem_hole, made necessary because the callback functions
> > > it
> > > uses need to access its arguments. Callers of kexec_locate_mem_hole,
> > > kexec_add_handover_buffer and kexec_add_buffer shouldn't need to know
> > > it
> > > exists.
> >
> > Elaborating a bit more: the argument list for these three functions are
> > equal or similar because kexec_add_handover_buffer uses
> > kexec_add_buffer,
> > which uses kexec_locate_mem_hole.
> >
> > It could be beneficial to have a struct to collect the arguments to
> > these
> > functions if someone using one of them would be likely to use another
> > one
> > with the same arguments. In that case, you set up kexec_buf once and
> > then
> > just pass it whenever you need to call one of those functions.
> >
> > But that is unlikely to happen. A user of the kexec API will need to use
> > only one of these functions with a given set of arguments, so they don't
> > gain anything by setting up a struct.
> >
> > Syntactically, I also don't think it's clearer to set struct members
> > instead of simply passing arguments to a function, even if the argument
> > list is long.
>
> Sorry, I'm not sure I get your points but the long argument list really
> looks ugly, since you are introducing more callbacks I still think a
> cleanup is necessary.
>
> kexec_buffer struct is pretty fine to be a abstract of all these buffers.

What I understood from what you said is that making the following change
results in code that is easier to understand:

@@ -650,6 +639,7 @@ static int __kexec_load_purgatory(struct kimage *image, unsigned long min,
const Elf_Shdr *sechdrs_c;
Elf_Shdr *sechdrs = NULL;
void *purgatory_buf = NULL;
+ struct kexec_buf buf;

/*
* sechdrs_c points to section headers in purgatory and are read
@@ -757,11 +747,19 @@ static int __kexec_load_purgatory(struct kimage *image, unsigned long min,
buf_align = bss_align;

/* Add buffer to segment list */
- ret = kexec_add_buffer(image, purgatory_buf, buf_sz, memsz,
- buf_align, min, max, top_down,
- &pi->purgatory_load_addr);
+ memset(&buf, 0, sizeof(struct kexec_buf));
+ buf.image = image;
+ buf.buffer = purgatory_buf;
+ buf.bufsz = buf_sz,
+ buf.memsz = memsz;
+ buf.buf_align = buf_align;
+ buf.buf_min = min;
+ buf.buf_max = max;
+ buf.top_down = top_down;
+ ret = kexec_add_buffer(&buf);
if (ret)
goto out;
+ pi->purgatory_load_addr = buf.mem;

/* Load SHF_ALLOC sections */
buf_addr = purgatory_buf;

There are 9 calls to kexec_add_buffer in the kernel (including arch/x86,
arch/powerpc/ and kernel/), plus 1 to kexec_locate_mem_hole
and 1 to kexec_add_handover_buffer, so there would be 11 places in
the code settings up kexec_buf. My opinion is that this change doesn't
improve code readability.

Also, I think that kexec_buf abstracts something that, from the
perspective of the user of the kexec API, lives only for the duration
of a single call to either of kexec_add_buffer, kexec_locate_mem_hole,
or kexec_add_handover_buffer. Because of this, there's no need from the
perspective of the API user to initialize this "object", so this just
adds to their cognitive load without any benefit to them.

I understand that this is all somewhat subjective, so if you still disagree
with my points I can provide a patch set implementing the change above.

[]'s
Thiago Jung Bauermann
IBM Linux Technology Center