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

[Thiago Jung Bauermann]

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