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

[Dave Young]

On 07/01/16 at 02:51pm, Thiago Jung Bauermann wrote:
> 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.

But the assignment can be moved to the beginning of the function
__kexec_load_purgatory, and avoid the local variables from the very
beginning. Just use kbuf.member instead.

> 
> 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.

I still feel it should be changed if more callbacks being introduced,
though you can regard it is internal api, like above comment we do not
need to assign them seperately, the member values can be assigned
from the beginning.

Thanks
Dave