On Thu, Aug 24, 2006 at 11:20:31AM +0200, Andi Kleen wrote:
> 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 <[email protected]> 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.
I agree, The problem is doing the first step ;-<
>
> > > > @@ -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.
>
I see. Let me try this out.
>
> > > > - /*
> > > > - * 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.
>
That goes back to patchkit point. I could put it in the ctxsw patch instead of
modified.
> > 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.
>
Thread_info is u32 and we are up to bit 23 with TIF_PERFMON + 3 = 26.
But it looks cleaner and probably more efficient. I'll make the change.
Thanks.
--
-Stephane
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
[Index of Archives]
[Kernel Newbies]
[Netfilter]
[Bugtraq]
[Photo]
[Stuff]
[Gimp]
[Yosemite News]
[MIPS Linux]
[ARM Linux]
[Linux Security]
[Linux RAID]
[Video 4 Linux]
[Linux for the blind]
[Linux Resources]
