small cleanups + fixes for perfmon.c
PATCH 1/1 small cleanups + fixes for perfmon.c
The following patch, compile-tested only so far, does the following :
o removes the [UN]LOCK_BUF_FMT_LIST defines - there is no abstraction benefit to them, and they just look ugly
o fixes two KERN_ERR that should be KERN_INFO
o removes the "nolock" parameter to the find function (was unused)
o homogenises the names related to buffer formats
o removes the hand-crafted list in favour of standard linux list methods
o reduces code duplication
OK ?
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 +71 -84
@@ -513,10 +513,8 @@ static pfm_session_t pfm_sessions; /* gstatic struct proc_dir_entry *perfmon_dir;static pfm_uuid_t pfm_null_uuid = {0 ,}; -static spinlock_t pfm_smpl_fmt_lock; -static pfm_buffer_fmt_t *pfm_buffer_fmt_list; -#define LOCK_BUF_FMT_LIST () spin_lock(&pfm_smpl_fmt_lock) -#define UNLOCK_BUF_FMT_LIST () spin_unlock(&pfm_smpl_fmt_lock) +static spinlock_t pfm_buffer_fmt_lock; +static LIST_HEAD (pfm_buffer_fmt_list);/* sysctl() controls */ static pfm_sysctl_t pfm_sysctl;
@@ -1206,12 +1204,36 @@ pfm_buf_fmt_restart_active(pfm_buffer_fmreturn ret; } +static pfm_buffer_fmt_t * +__pfm_find_buffer_fmt (pfm_uuid_t uuid) +{ +struct list_head * pos; +pfm_buffer_fmt_t * entry; + +list_for_each (pos, &pfm_buffer_fmt_list) { + entry =list_entry (pos, pfm_buffer_fmt_t, fmt_list); +if (pfm_uuid_cmp (uuid, entry->fmt_uuid) =0 ) +return entry; + } +return NULL ; +} +/* +* find a buffer format based on its uuid +*/ +static pfm_buffer_fmt_t * +pfm_find_buffer_fmt (pfm_uuid_t uuid) +{ +pfm_buffer_fmt_t * fmt; +spin_lock (&pfm_buffer_fmt_lock); + fmt =__pfm_find_buffer_fmt (uuid); +spin_unlock (&pfm_buffer_fmt_lock); +return fmt; +}int pfm_register_buffer_fmt (pfm_buffer_fmt_t *fmt) { -pfm_buffer_fmt_t *p;int ret =0 ;/* some sanity checks */
@@ -1224,78 +1246,43 @@ pfm_register_buffer_fmt(pfm_buffer_fmt_t* XXX: need check validity of fmt_arg_size */ -LOCK_BUF_FMT_LIST (); - p = pfm_buffer_fmt_list; - - -while (p) { -if (pfm_uuid_cmp (fmt->fmt_uuid, p->fmt_uuid) =0 )break ; - p = p->fmt_next; - } +spin_lock (&pfm_buffer_fmt_lock); -if (p) { +if (__pfm_find_buffer_fmt (fmt->fmt_uuid)) {printk (KERN_ERR "perfmon: duplicate sampling format: %s\n" , fmt->fmt_name); ret = -EBUSY ; - }else { - fmt->fmt_prev =NULL ; - fmt->fmt_next = pfm_buffer_fmt_list; - pfm_buffer_fmt_list = fmt; -printk (KERN_ERR "perfmon: added sampling format %s\n" , fmt->fmt_name); + goto out; } -UNLOCK_BUF_FMT_LIST (); +list_add (&fmt->fmt_list, &pfm_buffer_fmt_list); +printk (KERN_INFO "perfmon: added sampling format %s\n" , fmt->fmt_name); + +out: +spin_unlock (&pfm_buffer_fmt_lock);return ret; }int pfm_unregister_buffer_fmt (pfm_uuid_t uuid) { -pfm_buffer_fmt_t *p; +pfm_buffer_fmt_t *fmt;int ret =0 ; -LOCK_BUF_FMT_LIST (); - p = pfm_buffer_fmt_list; -while (p) { -if (memcmp (uuid, p->fmt_uuid,sizeof (pfm_uuid_t)) =0 )break ; - p = p->fmt_next; - } -if (p) { -if (p->fmt_prev) - p->fmt_prev->fmt_next = p->fmt_next; -else - pfm_buffer_fmt_list = p->fmt_next; - -if (p->fmt_next) - p->fmt_next->fmt_prev = p->fmt_prev; +spin_lock (&pfm_buffer_fmt_lock); -printk (KERN_ERR "perfmon: removed sampling format: %s\n" , p->fmt_name); - p->fmt_next = p->fmt_prev =NULL ; - }else { + fmt =__pfm_find_buffer_fmt (uuid); +if (!fmt) {printk (KERN_ERR "perfmon: cannot unregister format, not found\n" ); ret = -EINVAL ; + goto out; } -UNLOCK_BUF_FMT_LIST (); -return ret; +list_del_init (&fmt->fmt_list); +printk (KERN_INFO "perfmon: removed sampling format: %s\n" , fmt->fmt_name); -} - -/* -* find a buffer format based on its uuid -*/ -static pfm_buffer_fmt_t * -pfm_find_buffer_fmt (pfm_uuid_t uuid,int nolock) -{ -pfm_buffer_fmt_t *p; - -LOCK_BUF_FMT_LIST (); -for (p = pfm_buffer_fmt_list; p ; p = p->fmt_next) { -if (pfm_uuid_cmp (uuid, p->fmt_uuid) =0 )break ; - } - -UNLOCK_BUF_FMT_LIST (); - -return p; +out: +spin_unlock (&pfm_buffer_fmt_lock); +return ret; } static int
@@ -2419,8 +2406,7 @@ pfm_setup_buffer_fmt(struct task_structint ret =0 ;#define PFM_CTXARG_BUF_ARG (a) (pfm_buffer_fmt_t *)(a+1) -/* invoke and lock buffer format, if found */ - fmt =pfm_find_buffer_fmt (arg->ctx_smpl_buf_id,0 ); + fmt =pfm_find_buffer_fmt (arg->ctx_smpl_buf_id);if (fmt =NULL ) {DPRINT (("[%d] cannot find buffer format\n" , task->pid));return -EINVAL ;
@@ -2528,8 +2514,7 @@ pfm_ctx_getsize(void *arg, size_t *sz)if (!pfm_uuid_cmp (req->ctx_smpl_buf_id, pfm_null_uuid))return 0 ; -/* no buffer locking here, will be called again */ - fmt =pfm_find_buffer_fmt (req->ctx_smpl_buf_id,1 ); + fmt =pfm_find_buffer_fmt (req->ctx_smpl_buf_id);if (fmt =NULL ) {DPRINT (("cannot find buffer format\n" ));return -EINVAL ;
@@ -5445,7 +5430,8 @@ static int pfm_proc_info(char *page) {char *p = page; -pfm_buffer_fmt_t *b; +struct list_head * pos; +pfm_buffer_fmt_t * entry;unsigned long psr;int i;
@@ -5495,29 +5481,30 @@ pfm_proc_info(char *page) pfm_sessions.pfs_ptrace_use_dbregs);UNLOCK_PFS (); -LOCK_BUF_FMT_LIST (); +spin_lock (&pfm_buffer_fmt_lock); -for (b = pfm_buffer_fmt_list; b ; b = b->fmt_next) { +list_for_each (pos, &pfm_buffer_fmt_list) { + entry =list_entry (pos, pfm_buffer_fmt_t, fmt_list); p +=sprintf (p,"format : %02x-%02x-%02x-%02x-%02x-%02x-%02x-%02x-%02x-%02x-%02x-%02x-%02x-%02x-%02x-%02x %s\n" , - b->fmt_uuid[0 ], - b->fmt_uuid[1 ], - b->fmt_uuid[2 ], - b->fmt_uuid[3 ], - b->fmt_uuid[4 ], - b->fmt_uuid[5 ], - b->fmt_uuid[6 ], - b->fmt_uuid[7 ], - b->fmt_uuid[8 ], - b->fmt_uuid[9 ], - b->fmt_uuid[10 ], - b->fmt_uuid[11 ], - b->fmt_uuid[12 ], - b->fmt_uuid[13 ], - b->fmt_uuid[14 ], - b->fmt_uuid[15 ], - b->fmt_name); + entry->fmt_uuid[0 ], + entry->fmt_uuid[1 ], + entry->fmt_uuid[2 ], + entry->fmt_uuid[3 ], + entry->fmt_uuid[4 ], + entry->fmt_uuid[5 ], + entry->fmt_uuid[6 ], + entry->fmt_uuid[7 ], + entry->fmt_uuid[8 ], + entry->fmt_uuid[9 ], + entry->fmt_uuid[10 ], + entry->fmt_uuid[11 ], + entry->fmt_uuid[12 ], + entry->fmt_uuid[13 ], + entry->fmt_uuid[14 ], + entry->fmt_uuid[15 ], + entry->fmt_name); } -UNLOCK_BUF_FMT_LIST (); +spin_unlock (&pfm_buffer_fmt_lock);return p - page; }
@@ -6338,7 +6325,7 @@ pfm_init(void)* initialize all our spinlocks */ spin_lock_init (&pfm_sessions.pfs_lock); -spin_lock_init (&pfm_smpl_fmt_lock); +spin_lock_init (&pfm_buffer_fmt_lock);init_pfm_fs ();
Index: linux-ia64/include/asm-ia64//perfmon.h =================================RCS file: /home/cvs/linux-2.5/include/asm-ia64/perfmon.h,v retrieving revision 1.13
include/asm-ia64//perfmon.h +2 -3
@@ -223,7 +223,7 @@ typedef struct { } pfm_ovfl_arg_t; -typedef struct _pfm_buffer_fmt_t { +typedef struct { char *fmt_name; pfm_uuid_t fmt_uuid; size_t fmt_arg_size;
@@ -237,8 +237,7 @@ typedef struct _pfm_buffer_fmt_t {int (*fmt_restart_active)(struct task_struct *task,pfm_ovfl_ctrl_t *ctrl,void *buf,struct pt_regs *regs);int (*fmt_exit)(struct task_struct *task,void *buf,struct pt_regs *regs); -struct _pfm_buffer_fmt_t *fmt_next; -struct _pfm_buffer_fmt_t *fmt_prev; +struct list_head fmt_list; } pfm_buffer_fmt_t;extern int pfm_register_buffer_fmt (pfm_buffer_fmt_t *fmt);
John,
Quoted 11 lines · John Levon
The following patch, compile-tested only so far, does the following :
o removes the [UN]LOCK_BUF_FMT_LIST defines - there is no abstraction benefit to them, and they just look ugly
o fixes twoKERN_ERRthat should beKERN_INFO
o removes the "nolock" parameter to the find function (was unused)
o homogenises the names related to buffer formats
o removes the hand-crafted list in favour of standard linux list methods
o reduces code duplication
Quoted 2 lines
OK ?
Yes, that one looks fine to me. I will apply it
to me version and then submit this to David.
Thanks for the cleanups.
Signature · 1 line
-Stephane