Re: [PATCH 1/3] x86: process: Unify 32-bit and 64-bit copy_thread I/O bitmap handling

[Alexander van Heukelum <heukelum@fastmail.fm>]

Hi Josh,

Since you added me to the CC, I had a look... Comments inline

On Tue, Oct 22, 2013, at 3:34, Josh Triplett wrote:
> The 32-bit and 64-bit versions of copy_thread have functionally
> identical handling for copying the I/O bitmap, modulo differences in
> error handling.  Clean up the error paths in both by moving the copy of
> the I/O bitmap to the end, to eliminate the need to free it if
> subsequent copy steps fail; move the resulting identical code to a
> static inline in a common header.
> 
> Signed-off-by: Josh Triplett <josh@joshtriplett.org>
> ---
>  arch/x86/kernel/process-io.h | 22 ++++++++++++++++++++++

I don't particularly like this new file. It's also not in a proper place.

>  arch/x86/kernel/process_32.c | 29 ++++++++---------------------
>  arch/x86/kernel/process_64.c | 26 +++++---------------------

Yay, this I like :). However, unification is best done in a separate step. First
make the two parts equal in one or two patches. Then, in a separate patch,
do the mechanical unification.

>  3 files changed, 35 insertions(+), 42 deletions(-)
>  create mode 100644 arch/x86/kernel/process-io.h
> 
> diff --git a/arch/x86/kernel/process-io.h b/arch/x86/kernel/process-io.h
> new file mode 100644
> index 0000000..d884444
> --- /dev/null
> +++ b/arch/x86/kernel/process-io.h
> @@ -0,0 +1,22 @@
> +#ifndef _X86_KERNEL_PROCESS_IO_H
> +#define _X86_KERNEL_PROCESS_IO_H
> +
> +static inline int copy_io_bitmap(struct task_struct *me,
> +				 struct task_struct *p)
> +{
> +	if (unlikely(test_tsk_thread_flag(me, TIF_IO_BITMAP))) {
> +		p->thread.io_bitmap_ptr = kmemdup(me->thread.io_bitmap_ptr,
> +						  IO_BITMAP_BYTES, GFP_KERNEL);
> +		if (!p->thread.io_bitmap_ptr) {
> +			p->thread.io_bitmap_max = 0;
> +			return -ENOMEM;
> +		}
> +		set_tsk_thread_flag(p, TIF_IO_BITMAP);
> +	} else {
> +		p->thread.io_bitmap_ptr = NULL;
> +	}
> +
> +	return 0;
> +}
> +

I really don't like the .h-file like this. Could you try avoiding it? I think
it's possible to unify the copy_thread function and in the end move it
to process.c instead. The arch-specific parts can be split out in static
functions guarded by CONFIG_X86_64 (cooking up good names is
the challenge here ;) ).