Re: [PATCH 17/18] 2.6.17.9 perfmon2 patch for review: modified x86_64 files

[Stephane Eranian <eranian@hpl.hp.com>]

Andi,

On Thu, Aug 24, 2006 at 11:20:31AM +0200, Andi Kleen wrote:
> > > > -	/*
> > > > -	 * Now maybe reload the debug registers and handle I/O bitmaps
> > > > -	 */
> > > > -	if (unlikely((task_thread_info(next_p)->flags & _TIF_WORK_CTXSW))
> > > > -	    || test_tsk_thread_flag(prev_p, TIF_IO_BITMAP))
> > > > -		__switch_to_xtra(prev_p, next_p, tss);
> > > > +  	/*
> > > > + 	 * Now maybe reload the debug registers and handle I/O bitmaps
> > > > +  	 */
> > > > + 	if (unlikely((task_thread_info(next_p)->flags & _TIF_WORK_CTXSW)
> > > > + 	    || (task_thread_info(prev_p)->flags & _TIF_WORK_CTXSW)))
> > > > + 		__switch_to_xtra(prev_p, next_p, tss);
> > > 
> > > 
> > > This should be a separate patch for once (creating _TIF_WORK_CTXSW)
> > 
> > The _TIF_WORK_CTXSW is already in a separate patch which you have accepted
> > into your tree if I recall. It was part of the TIF_DEBUG/TIF_IO_BITMAP patch.
> > Unless you are repeating the first point you have at the top of this message
> > about group by functionality.
> 
> 
> Such a hunk just shouldn't be a hidden in a huge patch. Individual patches please.
> 
> > to get to pfm_handle_work(), we set TIF_NOTIFY_RESUME. Once in pfm_handle_work()
> > with the context properly locked, we check the reason for coming here. To mimic,
> > what we do with TIF flags in __switch_to(). I would have to add 3 new TIF flags.
> > The TIF_PERFMON flag means something different. When you come to notify_resume()
> > for a signal in a monitored thread, you may not need to go into pfm_handle_work().
> > But what is sure, is that if you do not have TIF_PERFMON set you never need to
> > get into pfm_handle_work(). So one thing I could do if to check for TIF_PERFMON
> > to miinize the number of useless calls to pfm_handle_work().
> 
> flags are cheap. Just add three if you need them.
> 
I  looked at that in more details. I can get by with 2 extra TIF flags. The problem
is that I have still hooked up to the TIF_NOTIFY_RESUME mechanism to get to  the
do_notify_resume() function. To make this work I have to either set TIF_NOTIFY_RESUME
*and* TIF_PERFMON_XXX or I have to add TIF_PERFMON_XXX to this kind of code in
entry.S:
	sysret_signal:
        	sti
        	testl $(_TIF_SIGPENDING|_TIF_NOTIFY_RESUME|_TIF_SINGLESTEP),%edx
        	jz    1f

        	/* Really a signal */
        	/* edx: work flags (arg3) */
        	leaq do_notify_resume(%rip),%rax
        	leaq -ARGOFFSET(%rsp),%rdi # &pt_regs -> arg1
        	xorl %esi,%esi # oldset -> arg2

But there seems to be some limitations on the low order 16 bits for the _TIF_ALLWORK_MASK
which is also being checked in entry.S and my TIF_PERFMON are 20 and above.

-- 
-Stephane