Skip to contentSkip to conversation

kill pointless perfmon abstractions

PATCH In review John Levon 2 participants 3 messages Oct 8, 2003

lore
Share & tools
lore

John Levon

author
#0 lore raw

PATCH 1/1 kill pointless perfmon abstractions

Another small patch, there's no benefit to having the extra code here.

regards
john

Index: linux-ia64/arch/ia64/kernel/perfmon.c =================================RCS file: /home/cvs/linux-2.5/arch/ia64/kernel/perfmon.c,v retrieving revision 1.35

arch/ia64/kernel/perfmon.c +6 -10
diff -u -a -p -r1.35 perfmon.c
@@ -544,14 +542,8 @@ static struct vm_operations_struct pfm_v
 	close: pfm_vm_close
 };
 
-#define pfm_wait_task_inactive(t)	wait_task_inactive(t)
 #define pfm_get_cpu_var(v)		__ia64_per_cpu_var(v)
 #define pfm_get_cpu_data(a,b)		per_cpu(a, b)
-typedef	irqreturn_t	pfm_irq_handler_t;
-#define PFM_IRQ_HANDLER_RET(v)	do {  \
-		put_cpu_no_resched(); \
-		return IRQ_HANDLED;   \
-	} while(0);
 
 static inline void
 pfm_put_task(struct task_struct *task)

@@ -2588,7 +2567,7 @@ pfm_task_incompatible(pfm_context_t *ctx
 	/*
 	 * make sure the task is off any CPU
 	 */
-	pfm_wait_task_inactive(task);
+	wait_task_inactive(task);
 
 	/* more to come... */
 

@@ -4686,7 +4665,7 @@ pfm_check_task_state(pfm_context_t *ctx,
 
 	UNPROTECT_CTX(ctx, flags);
 
-	pfm_wait_task_inactive(task);
+	wait_task_inactive(task);
 
 	PROTECT_CTX(ctx, flags);
 

@@ -5407,7 +5386,7 @@ report_spurious:
 	return -1;
 }
 
-static pfm_irq_handler_t
+static irqreturn_t
 pfm_interrupt_handler(int irq, void *arg, struct pt_regs *regs)
 {
 	unsigned long start_cycles, total_cycles;

@@ -5436,7 +5415,9 @@ pfm_interrupt_handler(int irq, void *arg
 
 		pfm_stats[this_cpu].pfm_ovfl_intr_cycles += total_cycles;
 	}
-	PFM_IRQ_HANDLER_RET();
+
+	put_cpu_no_resched();
+	return IRQ_HANDLED;
 }
#1 lore raw

John,

Quoted 4 lines · John Levon

Another small patch, there's no benefit to having the extra code here.

Well, it is because you only know part of the story ;-< These abstractions are here to mask differences between the various kernels out there. Internally I try to maintain the same perfmon-2 (perfmon.c/perfmon.h) for 2.6, 2.4, RH EL, and Suse SLES. All of those have slight differences which are abstracted by the code you are trying to remove in this patch. The version I export to David contains only the support for 2.6, the version-specific definitions are maintained in a separate header file in my tree. Only the section relevant to 2.6 is included in David's perfmon.c file. I have tried to keep those abstractions to a minimum and avoid all #ifdef.

Quoted 64 lines

Index: linux-ia64/arch/ia64/kernel/perfmon.c =================================> RCS file: /home/cvs/linux-2.5/arch/ia64/kernel/perfmon.c,v retrieving revision 1.35
diff -u -a -p -r1.35 perfmon.c

arch/ia64/kernel/perfmon.c +6 -11
@@ -544,14 +542,8 @@ static struct vm_operations_struct pfm_v
 	close: pfm_vm_close
 };
 
-#define pfm_wait_task_inactive(t)	wait_task_inactive(t)
 #define pfm_get_cpu_var(v)		__ia64_per_cpu_var(v)
 #define pfm_get_cpu_data(a,b)		per_cpu(a, b)
-typedef	irqreturn_t	pfm_irq_handler_t;
-#define PFM_IRQ_HANDLER_RET(v)	do {  \
-		put_cpu_no_resched(); \
-		return IRQ_HANDLED;   \
-	} while(0);
 
 static inline void
 pfm_put_task(struct task_struct *task)

@@ -2588,7 +2567,7 @@ pfm_task_incompatible(pfm_context_t *ctx
 	/*
 	 * make sure the task is off any CPU
 	 */
-	pfm_wait_task_inactive(task);
+	wait_task_inactive(task);
 
 	/* more to come... */
 

@@ -4686,7 +4665,7 @@ pfm_check_task_state(pfm_context_t *ctx,
 
 	UNPROTECT_CTX(ctx, flags);
 
-	pfm_wait_task_inactive(task);
+	wait_task_inactive(task);
 
 	PROTECT_CTX(ctx, flags);
 

@@ -5407,7 +5386,7 @@ report_spurious:
 	return -1;
 }
 
-static pfm_irq_handler_t
+static irqreturn_t
 pfm_interrupt_handler(int irq, void *arg, struct pt_regs *regs)
 {
 	unsigned long start_cycles, total_cycles;

@@ -5436,7 +5415,9 @@ pfm_interrupt_handler(int irq, void *arg
 
 		pfm_stats[this_cpu].pfm_ovfl_intr_cycles += total_cycles;
 	}
-	PFM_IRQ_HANDLER_RET();
+
+	put_cpu_no_resched();
+	return IRQ_HANDLED;
 }
 
 
-

To unsubscribe from this list: send the line "unsubscribe linux-ia64" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

Signature · 1 line
-Stephane
author
#2 lore raw
In reply to John Levon
Quoted 2 lines · Stephane Eranian

Well, it is because you only know part of the story ;-< These abstractions are here to mask differences between the various kernels out there.

I'd suspected that it might be an issue with 2.4 kernels so I checked against 2.4.22 - none of the removed macros even exist or are relevant. I understand that it can be often be useful to allow easy merges with 2.4, but it doesn't seem to apply in this case.

Quoted 1 line

RH EL, and Suse SLES.

I must be missing something here. Why are vendor kernels at all relevant to Linus's tree ? Vendor kernels are surely a vendor problem.

Even if they were: there's a much better way to do this. For example, instead of pfm_irq_handler_t, simply use irqreturn_t in the source (so it looks like 2.6 code). Then vendors can do :

#include "vendor.h"

or whatever that has
#define irqreturn_t void, #define IRQ_HANDLED /* nothing */, etc.

I believe this is general approach preferred by, for example, the net drivers. It means that the Linus kernel tree looks sane, whilst not sacrificing any merging ability with older trees.

Another obvious candidate here is pfm_do_munmap()

regards,
john

Signature · 2 lines
Khendon's Law:
If the same point is made twice by the same person, the thread is over.