Re: [Dle-develop] [PATCH v12 3/3] trace, x86: code-sharing between non-trace and trace irq handlers

[Seiji A. <sei...@hd...>]

> This as a separate patch actually makes things more confusing to review.
> It should be merged into the previous patch. If you want to break up the
> changes, I would first add the entering_irq(), and exiting_irq() as
> patch 1, and then do the rest of the changes in patch 2.

Thanks. I will update this patchset as above.

Seiji

> -----Original Message-----
> From: Steven Rostedt [mailto:ro...@go...]
> Sent: Thursday, May 23, 2013 3:55 PM
> To: Seiji Aguchi
> Cc: lin...@vg...; x8...@ke...; hp...@zy...; Thomas Gleixner (tg...@li...); 'mi...@el...'
> (mi...@el...); Borislav Petkov (bp...@al...); lin...@vg...; Luck, Tony (ton...@in...); dle-
> de...@li...; Tomoki Sekiyama
> Subject: Re: [PATCH v12 3/3] trace,x86: code-sharing between non-trace and trace irq handlers
> 
> On Fri, 2013-04-05 at 19:21 +0000, Seiji Aguchi wrote:
> > [Issue]
> >
> > Currently, irq vector handlers for tracing are just
> > copied non-trace handlers by simply inserting tracepoints.
> >
> > It is difficult to manage the codes.
> >
> > [Solution]
> 
> 
> This as a separate patch actually makes things more confusing to review.
> It should be merged into the previous patch. If you want to break up the
> changes, I would first add the entering_irq(), and exiting_irq() as
> patch 1, and then do the rest of the changes in patch 2.
> 
> -- Steve
> 
> > This patch shares common codes between non-trace and trace handlers
> > as follows to make them manageable and readable.
> >
> > Non-trace irq handler:
> > smp_irq_handler()
> > {
> > 	entering_irq(); /* pre-processing of this handler */
> > 	__smp_irq_handler(); /*
> >                           * common logic between non-trace and trace handlers
> >                           * in a vector.
> >                           */
> > 	exiting_irq(); /* post-processing of this handler */
> >
> > }
> >
> > Trace irq_handler:
> > smp_trace_irq_handler()
> > {
> > 	entering_irq(); /* pre-processing of this handler */
> > 	trace_irq_entry(); /* tracepoint for irq entry */
> > 	__smp_irq_handler(); /*
> >                           * common logic between non-trace and trace handlers
> >                           * in a vector.
> >                           */
> > 	trace_irq_exit(); /* tracepoint for irq exit */
> > 	exiting_irq(); /* post-processing of this handler */
> >
> > }
> >
> > If tracepoints can place outside entering_irq()/exiting_irq() as follows, it looks \
> > cleaner.
> >
> > smp_trace_irq_handler()
> > {
> > 	trace_irq_entry();
> > 	smp_irq_handler();
> > 	trace_irq_exit();
> > }
> >
> > But it doesn't work.
> > The problem is with irq_enter/exit() being called. They must be called before \
> > trace_irq_enter/exit(),  because of the rcu_irq_enter() must be called before any \
> > tracepoints are used, as tracepoints use  rcu to synchronize.
> >
> > As a possible alternative, we may be able to call irq_enter() first as follows if \
> > irq_enter() can nest.
> >
> > smp_trace_irq_hander()
> > {
> > 	irq_entry();
> > 	trace_irq_entry();
> > 	smp_irq_handler();
> > 	trace_irq_exit();
> > 	irq_exit();
> > }
> >
> > But it doesn't work, either.
> > If irq_enter() is nested, it may have a time penalty because it has to check if it \
> > was already called or not. The time penalty is not desired in performance sensitive \
> > paths even if it is tiny.
> >
> > Signed-off-by: Seiji Aguchi <sei...@hd...>
>