Re: q-tools OOPS: Fixed perfmon.

Re: q-tools OOPS: Fixed perfmon.

[Peter Chubb]

>>>>> "Stephane" = Stephane Eranian <eranian@hpl.hp.com> writes:

Stephane> I don't think that perfmon-2 support CONFIG_PREEMPT. Try
Stephane> without it.

Here's a  fix for non-preemption safety in perfmon.c.

I haven't tried it while running a preemption stress test, but this
allows q-syscollect to work.

=== arch/ia64/kernel/perfmon.c 1.67 vs edited ==--- 1.67/arch/ia64/kernel/perfmon.c   Tue Oct 28 17:36:50 2003
+++ edited/arch/ia64/kernel/perfmon.c Tue Dec  9 10:55:58 2003
@@ -5475,7 +5475,7 @@
	int this_cpu;
	int ret;
 
-	this_cpu = smp_processor_id();
+	this_cpu = get_cpu();
	min      = pfm_stats[this_cpu].pfm_ovfl_intr_cycles_min;
	max      = pfm_stats[this_cpu].pfm_ovfl_intr_cycles_max;
 

Re: q-tools OOPS: Fixed perfmon.

[Matthew Wilcox]

On Tue, Dec 09, 2003 at 11:17:56AM +1100, Peter Chubb wrote:
> Here's a  fix for non-preemption safety in perfmon.c.
> 
> I haven't tried it while running a preemption stress test, but this
> allows q-syscollect to work.
> 
> === arch/ia64/kernel/perfmon.c 1.67 vs edited ==> --- 1.67/arch/ia64/kernel/perfmon.c   Tue Oct 28 17:36:50 2003
> +++ edited/arch/ia64/kernel/perfmon.c Tue Dec  9 10:55:58 2003
> @@ -5475,7 +5475,7 @@
> 	int this_cpu;
> 	int ret;
>  
> -	this_cpu = smp_processor_id();
> +	this_cpu = get_cpu();
> 	min      = pfm_stats[this_cpu].pfm_ovfl_intr_cycles_min;
> 	max      = pfm_stats[this_cpu].pfm_ovfl_intr_cycles_max;

surely there needs to be a matching put_cpu() or else preempt is forever
disabled.  no?

-- 
"Next the statesmen will invent cheap lies, putting the blame upon 
the nation that is attacked, and every man will be glad of those
conscience-soothing falsities, and will diligently study them, and refuse
to examine any refutations of them; and thus he will by and by convince 
himself that the war is just, and will thank God for the better sleep 
he enjoys after this process of grotesque self-deception." -- Mark Twain

Re: q-tools OOPS: Fixed perfmon.

[Stephane Eranian]

Peter,

Yes, clearly you do not want to preempt in the perfmon interrupt handler.
I think we need to also make sure we do not preempt during the perfmonctl()
system call, at least until we verify that there is no assumptions in there.
Similarly in all the VFS entry points, such as pfm_close(). I will try
and see what I can do before the end of the week.

Thanks for your patch.

On Tue, Dec 09, 2003 at 11:17:56AM +1100, Peter Chubb wrote:
> >>>>> "Stephane" = Stephane Eranian <eranian@hpl.hp.com> writes:
> 
> Stephane> I don't think that perfmon-2 support CONFIG_PREEMPT. Try
> Stephane> without it.
> 
> Here's a  fix for non-preemption safety in perfmon.c.
> 
> I haven't tried it while running a preemption stress test, but this
> allows q-syscollect to work.
> 
> === arch/ia64/kernel/perfmon.c 1.67 vs edited ==> --- 1.67/arch/ia64/kernel/perfmon.c   Tue Oct 28 17:36:50 2003
> +++ edited/arch/ia64/kernel/perfmon.c Tue Dec  9 10:55:58 2003
> @@ -5475,7 +5475,7 @@
> 	int this_cpu;
> 	int ret;
>  
> -	this_cpu = smp_processor_id();
> +	this_cpu = get_cpu();
> 	min      = pfm_stats[this_cpu].pfm_ovfl_intr_cycles_min;
> 	max      = pfm_stats[this_cpu].pfm_ovfl_intr_cycles_max;
>  
> -
> To unsubscribe from this list: send the line "unsubscribe linux-ia64" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

-- 

-Stephane

Re: q-tools OOPS: Fixed perfmon.

[David Mosberger]

>>>>> On Mon, 8 Dec 2003 17:57:04 -0800, Stephane Eranian <eranian@hpl.hp.com> said:

  Stephane> Peter, Yes, clearly you do not want to preempt in the
  Stephane> perfmon interrupt handler.  I think we need to also make
  Stephane> sure we do not preempt during the perfmonctl() system
  Stephane> call, at least until we verify that there is no
  Stephane> assumptions in there.  Similarly in all the VFS entry
  Stephane> points, such as pfm_close(). I will try and see what I can
  Stephane> do before the end of the week.

  Stephane> Thanks for your patch.

I did put in Peter's fix in my tree, since it fixes a known crash and
is obviously correct.

	--david

Re: q-tools OOPS: Fixed perfmon.

[Martin Hicks]

[-- Attachment #1: Type: text/plain, Size: 1088 bytes --]

On Mon, 2003-12-08 at 20:24, Matthew Wilcox wrote:
> On Tue, Dec 09, 2003 at 11:17:56AM +1100, Peter Chubb wrote:
> > Here's a  fix for non-preemption safety in perfmon.c.
> > 
> > I haven't tried it while running a preemption stress test, but this
> > allows q-syscollect to work.
> > 
> > ===== arch/ia64/kernel/perfmon.c 1.67 vs edited =====
> > --- 1.67/arch/ia64/kernel/perfmon.c   Tue Oct 28 17:36:50 2003
> > +++ edited/arch/ia64/kernel/perfmon.c Tue Dec  9 10:55:58 2003
> > @@ -5475,7 +5475,7 @@
> > 	int this_cpu;
> > 	int ret;
> >  
> > -	this_cpu = smp_processor_id();
> > +	this_cpu = get_cpu();
> > 	min      = pfm_stats[this_cpu].pfm_ovfl_intr_cycles_min;
> > 	max      = pfm_stats[this_cpu].pfm_ovfl_intr_cycles_max;
> 
> surely there needs to be a matching put_cpu() or else preempt is forever
> disabled.  no?

I agree.

(from include/linux/smp.h)
#define get_cpu()               ({ preempt_disable(); smp_processor_id(); })
#define put_cpu()               preempt_enable()

mh

-- 
Martin Hicks || mort@bork.org || PGP/GnuPG: 0x4C7F2BEE

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 189 bytes --]

Re: q-tools OOPS: Fixed perfmon.

[David Mosberger]

>>>>> On Tue, 09 Dec 2003 08:46:06 -0500, Martin Hicks <mort@bork.org> said:

  Martin> On Mon, 2003-12-08 at 20:24, Matthew Wilcox wrote:

  >> surely there needs to be a matching put_cpu() or else preempt is forever
  >> disabled.  no?

  Martin> I agree.

The put is already there!  See put_cpu_no_resched().

Remember that patches are diffs... ;-)

	--david

Re: q-tools OOPS: Fixed perfmon.

[Stephane Eranian]

Peter,

On Mon, Dec 08, 2003 at 05:57:04PM -0800, Stephane Eranian wrote:
> 
> Yes, clearly you do not want to preempt in the perfmon interrupt handler.
> I think we need to also make sure we do not preempt during the perfmonctl()
> system call, at least until we verify that there is no assumptions in there.
> Similarly in all the VFS entry points, such as pfm_close(). I will try
> and see what I can do before the end of the week.
> 
I checked the code and I believe we are covered because we rely upon
spin_lock_irqsave/restore for most entry points into perfmon-2. That
is enough to block/unblock preemption in all the calls that need it.
That applies to the sys_perfmonctl() entry, the VFS entry points
as well as the internal entry points for copy_threads() and a few others.
The only one missing is the one you found. The PMU state save/restore
are called from the scheduler and are therefore protected as well.

Thanks for tracking down that one bug.

-- 

-Stephane