From 560efa06be97651252caff4ba9bc2c014cf62ff9 Mon Sep 17 00:00:00 2001 From: Bobi Jam Date: Fri, 29 Jun 2012 15:33:04 +0800 Subject: [PATCH] LU-1282 lprocfs: fix multi-thread contention glitch * Revert clear stats patch committed on 8c831cb8, it is not multi- thread safe. * Should protect the change of lprocfs_stats::ls_biggest_alloc_num * Add a LPROCFS_STATS_FLAG_IRQ_SAFE flag, when a stat is non-percpu stats with this flag, lprocfs_stats_lock() should disable irq. Signed-off-by: Bobi Jam Change-Id: I42f29e97ff75bf3817249940c8bb491af123d1d9 Reviewed-on: http://review.whamcloud.com/3240 Reviewed-by: Liang Zhen Tested-by: Hudson Tested-by: Maloo Reviewed-by: Andreas Dilger Reviewed-by: Oleg Drokin --- lustre/include/lprocfs_status.h | 66 +++++++++++++++++++++++++++------------- lustre/lvfs/lvfs_lib.c | 21 ++++++++++--- lustre/obdclass/class_obd.c | 3 +- lustre/obdclass/lprocfs_status.c | 21 ++----------- 4 files changed, 66 insertions(+), 45 deletions(-) diff --git a/lustre/include/lprocfs_status.h b/lustre/include/lprocfs_status.h index 8a2a73d..c8f16b8 100644 --- a/lustre/include/lprocfs_status.h +++ b/lustre/include/lprocfs_status.h @@ -175,9 +175,10 @@ struct lprocfs_percpu { #define LPROCFS_GET_SMP_ID 0x0002 enum lprocfs_stats_flags { - LPROCFS_STATS_FLAG_NONE = 0x0000, /* per cpu counter */ - LPROCFS_STATS_FLAG_NOPERCPU = 0x0001, /* stats have no percpu - * area and need locking */ + LPROCFS_STATS_FLAG_NONE = 0x0000, /* per cpu counter */ + LPROCFS_STATS_FLAG_NOPERCPU = 0x0001, /* stats have no percpu + * area and need locking */ + LPROCFS_STATS_FLAG_IRQ_SAFE = 0x0002, /* alloc need irq safe */ }; enum lprocfs_fields_flags { @@ -197,8 +198,9 @@ struct lprocfs_stats { * been allocated, the 0th entry is * a statically intialized template */ int ls_flags; /* See LPROCFS_STATS_FLAG_* */ - cfs_spinlock_t ls_lock; /* Lock used only when there are - * no percpu stats areas */ + /* Lock used when there are no percpu stats areas; For percpu stats, + * it is used to protect ls_biggest_alloc_num change */ + cfs_spinlock_t ls_lock; struct lprocfs_percpu *ls_percpu[0]; }; @@ -381,31 +383,41 @@ extern int lprocfs_stats_alloc_one(struct lprocfs_stats *stats, static inline int lprocfs_stats_lock(struct lprocfs_stats *stats, int opc, unsigned long *flags) { - int rc = 0; + int rc = 0; + unsigned int cpuid; switch (opc) { default: LBUG(); case LPROCFS_GET_SMP_ID: - if (stats->ls_flags & LPROCFS_STATS_FLAG_NOPERCPU) { - cfs_spin_lock_irqsave(&stats->ls_lock, *flags); - return 0; - } else { - unsigned int cpuid = cfs_get_cpu(); + /* percpu counter stats */ + if ((stats->ls_flags & LPROCFS_STATS_FLAG_NOPERCPU) == 0) { + cpuid = cfs_get_cpu(); if (unlikely(stats->ls_percpu[cpuid + 1] == NULL)) rc = lprocfs_stats_alloc_one(stats, cpuid + 1); return rc < 0 ? rc : cpuid + 1; } - case LPROCFS_GET_NUM_CPU: - if (stats->ls_flags & LPROCFS_STATS_FLAG_NOPERCPU) { + /* non-percpu counter stats */ + if ((stats->ls_flags & LPROCFS_STATS_FLAG_IRQ_SAFE) != 0) cfs_spin_lock_irqsave(&stats->ls_lock, *flags); - return 1; - } else { + else + cfs_spin_lock(&stats->ls_lock); + return 0; + + case LPROCFS_GET_NUM_CPU: + /* percpu counter stats */ + if ((stats->ls_flags & LPROCFS_STATS_FLAG_NOPERCPU) == 0) return stats->ls_biggest_alloc_num; - } + + /* non-percpu counter stats */ + if ((stats->ls_flags & LPROCFS_STATS_FLAG_IRQ_SAFE) != 0) + cfs_spin_lock_irqsave(&stats->ls_lock, *flags); + else + cfs_spin_lock(&stats->ls_lock); + return 1; } } @@ -417,15 +429,27 @@ static inline void lprocfs_stats_unlock(struct lprocfs_stats *stats, int opc, LBUG(); case LPROCFS_GET_SMP_ID: - if (stats->ls_flags & LPROCFS_STATS_FLAG_NOPERCPU) - cfs_spin_unlock_irqrestore(&stats->ls_lock, *flags); - else + if (stats->ls_flags & LPROCFS_STATS_FLAG_NOPERCPU) { + if (stats->ls_flags & LPROCFS_STATS_FLAG_IRQ_SAFE) { + cfs_spin_unlock_irqrestore(&stats->ls_lock, + *flags); + } else { + cfs_spin_unlock(&stats->ls_lock); + } + } else { cfs_put_cpu(); + } return; case LPROCFS_GET_NUM_CPU: - if (stats->ls_flags & LPROCFS_STATS_FLAG_NOPERCPU) - cfs_spin_unlock_irqrestore(&stats->ls_lock, *flags); + if (stats->ls_flags & LPROCFS_STATS_FLAG_NOPERCPU) { + if (stats->ls_flags & LPROCFS_STATS_FLAG_IRQ_SAFE) { + cfs_spin_unlock_irqrestore(&stats->ls_lock, + *flags); + } else { + cfs_spin_unlock(&stats->ls_lock); + } + } return; } } diff --git a/lustre/lvfs/lvfs_lib.c b/lustre/lvfs/lvfs_lib.c index ffdfadf..075c1d0 100644 --- a/lustre/lvfs/lvfs_lib.c +++ b/lustre/lvfs/lvfs_lib.c @@ -148,8 +148,9 @@ EXPORT_SYMBOL(lprocfs_counter_sub); int lprocfs_stats_alloc_one(struct lprocfs_stats *stats, unsigned int idx) { - unsigned int percpusize; - int rc = -ENOMEM; + unsigned int percpusize; + int rc = -ENOMEM; + unsigned long flags = 0; /* the 1st percpu entry was statically allocated in * lprocfs_alloc_stats() */ @@ -162,8 +163,20 @@ int lprocfs_stats_alloc_one(struct lprocfs_stats *stats, unsigned int idx) OBD_ALLOC_GFP(stats->ls_percpu[idx], percpusize, CFS_ALLOC_ATOMIC); if (stats->ls_percpu[idx] != NULL) { rc = 0; - if (unlikely(stats->ls_biggest_alloc_num <= idx)) - stats->ls_biggest_alloc_num = idx + 1; + if (unlikely(stats->ls_biggest_alloc_num <= idx)) { + if (stats->ls_flags & LPROCFS_STATS_FLAG_IRQ_SAFE) + cfs_spin_lock_irqsave(&stats->ls_lock, flags); + else + cfs_spin_lock(&stats->ls_lock); + if (stats->ls_biggest_alloc_num <= idx) + stats->ls_biggest_alloc_num = idx + 1; + if (stats->ls_flags & LPROCFS_STATS_FLAG_IRQ_SAFE) { + cfs_spin_unlock_irqrestore(&stats->ls_lock, + flags); + } else { + cfs_spin_unlock(&stats->ls_lock); + } + } /* initialize the ls_percpu[idx] by copying the 0th template * entry */ diff --git a/lustre/obdclass/class_obd.c b/lustre/obdclass/class_obd.c index e6b2ff2..fcf7cc5 100644 --- a/lustre/obdclass/class_obd.c +++ b/lustre/obdclass/class_obd.c @@ -540,7 +540,8 @@ int init_obdclass(void) obd_zombie_impexp_init(); #ifdef LPROCFS obd_memory = lprocfs_alloc_stats(OBD_STATS_NUM, - LPROCFS_STATS_FLAG_NONE); + LPROCFS_STATS_FLAG_NONE | + LPROCFS_STATS_FLAG_IRQ_SAFE); if (obd_memory == NULL) { CERROR("kmalloc of 'obd_memory' failed\n"); RETURN(-ENOMEM); diff --git a/lustre/obdclass/lprocfs_status.c b/lustre/obdclass/lprocfs_status.c index 099e6a8..415b186 100644 --- a/lustre/obdclass/lprocfs_status.c +++ b/lustre/obdclass/lprocfs_status.c @@ -1324,13 +1324,8 @@ struct lprocfs_stats *lprocfs_alloc_stats(unsigned int num, stats->ls_num = num; stats->ls_biggest_alloc_num = 1; - if (flags & LPROCFS_STATS_FLAG_NOPERCPU) { - stats->ls_flags = flags; - cfs_spin_lock_init(&stats->ls_lock); - /* Use this lock only if there are no percpu areas */ - } else { - stats->ls_flags = 0; - } + stats->ls_flags = flags; + cfs_spin_lock_init(&stats->ls_lock); percpusize = offsetof(struct lprocfs_percpu, lp_cntr[num]); if (num_entry > 1) @@ -1378,25 +1373,13 @@ void lprocfs_clear_stats(struct lprocfs_stats *stats) int i; int j; unsigned int num_entry; - unsigned int percpusize; unsigned long flags = 0; num_entry = lprocfs_stats_lock(stats, LPROCFS_GET_NUM_CPU, &flags); - percpusize = offsetof(struct lprocfs_percpu, lp_cntr[stats->ls_num]); - if (num_entry > 1) - percpusize = CFS_L1_CACHE_ALIGN(percpusize); - for (i = 0; i < num_entry; i++) { if (stats->ls_percpu[i] == NULL) continue; - /* the 1st percpu entry was statically allocated in - * lprocfs_alloc_stats() */ - if (i > 0) { - OBD_FREE(stats->ls_percpu[i], percpusize); - stats->ls_percpu[i] = NULL; - continue; - } for (j = 0; j < stats->ls_num; j++) { percpu_cntr = &(stats->ls_percpu[i])->lp_cntr[j]; cfs_atomic_inc(&percpu_cntr->lc_cntl.la_entry); -- 1.8.3.1