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

[Andi Kleen <ak@suse.de>]

On Thursday 24 August 2006 11:04, Stephane Eranian wrote:
> Andi,
> 
> On Wed, Aug 23, 2006 at 12:09:25PM +0200, Andi Kleen wrote:
> > Stephane Eranian <eranian@frankl.hpl.hp.com> writes:
> > 
> > In general this stuff would be much easier to review if you
> > really split it into logical pieces: this means not modified/new,
> > but one patch doing one thing. Then the hooks could be reviewed
> > together with the code.
> > 
> 
> Yes, I think that would be nice but it is very hard to generate 
> such patches from the source tree that I have now. I would have to
> manually edit the new/mod patches to group things based on
> functionalities.

You could do it once and then store in quilt (or git/hg if you prefer that) 
for further editing as patchkits. That will simplify review and merging.

 
> > > @@ -934,6 +935,7 @@ void setup_threshold_lvt(unsigned long l
> > >  void smp_local_timer_interrupt(struct pt_regs *regs)
> > >  {
> > >  	profile_tick(CPU_PROFILING, regs);
> > > + 	pfm_handle_switch_timeout();
> > 
> > It is still unclear why you can't use an ordinary add_timer() ?
> > 
> 
> The hook is used to decrement a timeout value used for event set switching.
> Set switching is upported for both per-thread and system-wide contexts. For
> per-thread, the timeout must be "saved/restored" when the thread is context
> switched. The timeout must be handled in the context of the monitored thread.
> I am not sure add_timer() is a good fit for this. The add_timer looks good but
> del_timer() does not as  for an active timer, it would need to return the
> leftover duration so it can be reactivated via a new add_timer() on context
> switch in.

If you always add a new add_timer with timeout jiffies+1 it will always run in this 
context. No extra hooks needed.


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

-Andi