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

[Josh Triplett <josh@joshtriplett.org>]

On Thu, Oct 31, 2013 at 09:01:30PM +0100, Alexander van Heukelum wrote:
> 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.

It's a private header, and private headers are normally put in the
directories with the source code, not in the include directories.

The need for it becomes more obvious later in the patch series.

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

I could certainly split the first patch into two, sure.

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

It's theoretically possible to completely unify copy_thread by adding
several more functions like this to abstract the low-level details;
however, complete unification wasn't the goal of this patch series.  I
unified and pulled out this portion because it's also the portion I need
to *remove* when compiling with !CONFIG_X86_IOPORT.

- Josh Triplett