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@xxxxxxxxxxxxxxxx>
> ---
> 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 ;) ).
_______________________________________________
Virtualization mailing list
Virtualization@xxxxxxxxxxxxxxxxxxxxxxxxxx
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
