From 2fd940bf349886c57e4d38fc05b2aae1c2b8e63b Mon Sep 17 00:00:00 2001 From: lipeifeng Date: Tue, 20 Feb 2024 19:01:27 +0800 Subject: [PATCH] ANDROID: uid_sys_stat: fix data-error of cputime and io 'commit b6115e14010 ("ANDROID: uid_sys_stat: split the global lock uid_lock to the fine-grained locks for each hlist in hash_table.")' The above patch split the global lock to per-uid lock to reduce lock competition. But result in data-error from uid_cputime_show and uid_io_show in some cases. E.g, if thread1 and thread2 read /proc/uid_cputime/show_uid_stat at the same time, thread2 maybe operate in partA and zero active_stime and active_utime of uid_entry when thread1 is between partB and partC, which would cause thread1 show the error data. static int uid_cputime_show(struct seq_file *m, void *v) { ... /*partA*/ for (bkt = 0, uid_entry = NULL; uid_entry == NULL && bkt < HASH_SIZE(hash_table); bkt++) { lock_uid_by_bkt(bkt); hlist_for_each_entry(uid_entry, &hash_table[bkt], hash) { uid_entry->active_stime = 0; uid_entry->active_utime = 0; } unlock_uid_by_bkt(bkt); } rcu_read_lock(); /* partB */ do_each_thread(temp, task) { ... lock_uid(uid); if (!(task->flags & PF_EXITING)) { task_cputime_adjusted(task, &utime, &stime); uid_entry->active_utime += utime; uid_entry->active_stime += stime; } unlock_uid(uid); } while_each_thread(temp, task); rcu_read_unlock(); for (bkt = 0, uid_entry = NULL; uid_entry == NULL && bkt < HASH_SIZE(hash_table); bkt++) { lock_uid_by_bkt(bkt); hlist_for_each_entry(uid_entry, &hash_table[bkt], hash) { u64 total_utime = uid_entry->utime + uid_entry->active_utime; u64 total_stime = uid_entry->stime + uid_entry->active_stime; /* partC */ seq_printf(m, "%d: %llu %llu\n", uid_entry->uid, ktime_to_us(total_utime), ktime_to_us(total_stime)); } unlock_uid_by_bkt(bkt); } The patch ensures that the calculation and seq_printf of each uid_entry is within the uid_lock range, in order to accurate data. Bug: 278138377 Change-Id: Iaa2ccd95c4b4b333f04b2ba18d7699d94017394e Signed-off-by: lipeifeng (cherry picked from commit ea35d2bd073214e84be242287a2e91741c6588ed) --- drivers/misc/uid_sys_stats.c | 216 ++++++++++++----------------------- 1 file changed, 72 insertions(+), 144 deletions(-) diff --git a/drivers/misc/uid_sys_stats.c b/drivers/misc/uid_sys_stats.c index 13acc61539ab..e2d29ac2e2e5 100644 --- a/drivers/misc/uid_sys_stats.c +++ b/drivers/misc/uid_sys_stats.c @@ -51,12 +51,9 @@ struct io_stats { #define UID_STATE_FOREGROUND 0 #define UID_STATE_BACKGROUND 1 -#define UID_STATE_BUCKET_SIZE 2 - -#define UID_STATE_TOTAL_CURR 2 -#define UID_STATE_TOTAL_LAST 3 -#define UID_STATE_DEAD_TASKS 4 -#define UID_STATE_SIZE 5 +#define UID_STATE_TOTAL_LAST 2 +#define UID_STATE_DEAD_TASKS 3 +#define UID_STATE_SIZE 4 #define MAX_TASK_COMM_LEN 256 @@ -71,8 +68,6 @@ struct uid_entry { uid_t uid; u64 utime; u64 stime; - u64 active_utime; - u64 active_stime; int state; struct io_stats io[UID_STATE_SIZE]; struct hlist_node hash; @@ -173,58 +168,47 @@ static struct uid_entry *find_or_register_uid(uid_t uid) return uid_entry; } +static void calc_uid_cputime(struct uid_entry *uid_entry, + u64 *total_utime, u64 *total_stime) +{ + struct user_namespace *user_ns = current_user_ns(); + struct task_struct *p, *t; + u64 utime, stime; + uid_t uid; + + rcu_read_lock(); + for_each_process(p) { + uid = from_kuid_munged(user_ns, task_uid(p)); + + if (uid != uid_entry->uid) + continue; + + for_each_thread(p, t) { + /* avoid double accounting of dying threads */ + if (!(t->flags & PF_EXITING)) { + task_cputime_adjusted(t, &utime, &stime); + *total_utime += utime; + *total_stime += stime; + } + } + } + rcu_read_unlock(); +} + static int uid_cputime_show(struct seq_file *m, void *v) { struct uid_entry *uid_entry = NULL; - struct task_struct *task, *temp; - struct user_namespace *user_ns = current_user_ns(); - u64 utime; - u64 stime; u32 bkt; - uid_t uid; for (bkt = 0, uid_entry = NULL; uid_entry == NULL && bkt < HASH_SIZE(hash_table); bkt++) { + lock_uid_by_bkt(bkt); hlist_for_each_entry(uid_entry, &hash_table[bkt], hash) { - uid_entry->active_stime = 0; - uid_entry->active_utime = 0; - } - unlock_uid_by_bkt(bkt); - } + u64 total_utime = uid_entry->utime; + u64 total_stime = uid_entry->stime; - rcu_read_lock(); - do_each_thread(temp, task) { - uid = from_kuid_munged(user_ns, task_uid(task)); - lock_uid(uid); - - if (!uid_entry || uid_entry->uid != uid) - uid_entry = find_or_register_uid(uid); - if (!uid_entry) { - rcu_read_unlock(); - unlock_uid(uid); - pr_err("%s: failed to find the uid_entry for uid %d\n", - __func__, uid); - return -ENOMEM; - } - /* avoid double accounting of dying threads */ - if (!(task->flags & PF_EXITING)) { - task_cputime_adjusted(task, &utime, &stime); - uid_entry->active_utime += utime; - uid_entry->active_stime += stime; - } - unlock_uid(uid); - } while_each_thread(temp, task); - rcu_read_unlock(); - - for (bkt = 0, uid_entry = NULL; uid_entry == NULL && - bkt < HASH_SIZE(hash_table); bkt++) { - lock_uid_by_bkt(bkt); - hlist_for_each_entry(uid_entry, &hash_table[bkt], hash) { - u64 total_utime = uid_entry->utime + - uid_entry->active_utime; - u64 total_stime = uid_entry->stime + - uid_entry->active_stime; + calc_uid_cputime(uid_entry, &total_utime, &total_stime); seq_printf(m, "%d: %llu %llu\n", uid_entry->uid, ktime_to_us(total_utime), ktime_to_us(total_stime)); } @@ -323,86 +307,52 @@ static void add_uid_io_stats(struct uid_entry *uid_entry, __add_uid_io_stats(uid_entry, &task->ioac, slot); } -static void update_io_stats_all(void) +static void update_io_stats_uid(struct uid_entry *uid_entry) { - struct uid_entry *uid_entry = NULL; - struct task_struct *task, *temp; struct user_namespace *user_ns = current_user_ns(); + struct task_struct *p, *t; + struct io_stats io; + + memset(&io, 0, sizeof(struct io_stats)); + + rcu_read_lock(); + for_each_process(p) { + uid_t uid = from_kuid_munged(user_ns, task_uid(p)); + + if (uid != uid_entry->uid) + continue; + + for_each_thread(p, t) { + /* avoid double accounting of dying threads */ + if (!(t->flags & PF_EXITING)) { + io.read_bytes += t->ioac.read_bytes; + io.write_bytes += compute_write_bytes(&t->ioac); + io.rchar += t->ioac.rchar; + io.wchar += t->ioac.wchar; + io.fsync += t->ioac.syscfs; + } + } + } + rcu_read_unlock(); + + compute_io_bucket_stats(&uid_entry->io[uid_entry->state], &io, + &uid_entry->io[UID_STATE_TOTAL_LAST], + &uid_entry->io[UID_STATE_DEAD_TASKS]); +} + +static int uid_io_show(struct seq_file *m, void *v) +{ + + struct uid_entry *uid_entry = NULL; u32 bkt; - uid_t uid; for (bkt = 0, uid_entry = NULL; uid_entry == NULL && bkt < HASH_SIZE(hash_table); bkt++) { lock_uid_by_bkt(bkt); hlist_for_each_entry(uid_entry, &hash_table[bkt], hash) { - memset(&uid_entry->io[UID_STATE_TOTAL_CURR], 0, - sizeof(struct io_stats)); - } - unlock_uid_by_bkt(bkt); - } - rcu_read_lock(); - do_each_thread(temp, task) { - uid = from_kuid_munged(user_ns, task_uid(task)); - lock_uid(uid); - if (!uid_entry || uid_entry->uid != uid) - uid_entry = find_or_register_uid(uid); - if (!uid_entry) { - unlock_uid(uid); - continue; - } - add_uid_io_stats(uid_entry, task, UID_STATE_TOTAL_CURR); - unlock_uid(uid); - } while_each_thread(temp, task); - rcu_read_unlock(); + update_io_stats_uid(uid_entry); - for (bkt = 0, uid_entry = NULL; uid_entry == NULL && bkt < HASH_SIZE(hash_table); - bkt++) { - lock_uid_by_bkt(bkt); - hlist_for_each_entry(uid_entry, &hash_table[bkt], hash) { - compute_io_bucket_stats(&uid_entry->io[uid_entry->state], - &uid_entry->io[UID_STATE_TOTAL_CURR], - &uid_entry->io[UID_STATE_TOTAL_LAST], - &uid_entry->io[UID_STATE_DEAD_TASKS]); - } - unlock_uid_by_bkt(bkt); - } -} - -static void update_io_stats_uid(struct uid_entry *uid_entry) -{ - struct task_struct *task, *temp; - struct user_namespace *user_ns = current_user_ns(); - - memset(&uid_entry->io[UID_STATE_TOTAL_CURR], 0, - sizeof(struct io_stats)); - - rcu_read_lock(); - do_each_thread(temp, task) { - if (from_kuid_munged(user_ns, task_uid(task)) != uid_entry->uid) - continue; - add_uid_io_stats(uid_entry, task, UID_STATE_TOTAL_CURR); - } while_each_thread(temp, task); - rcu_read_unlock(); - - compute_io_bucket_stats(&uid_entry->io[uid_entry->state], - &uid_entry->io[UID_STATE_TOTAL_CURR], - &uid_entry->io[UID_STATE_TOTAL_LAST], - &uid_entry->io[UID_STATE_DEAD_TASKS]); -} - - -static int uid_io_show(struct seq_file *m, void *v) -{ - struct uid_entry *uid_entry; - u32 bkt; - - update_io_stats_all(); - for (bkt = 0, uid_entry = NULL; uid_entry == NULL && bkt < HASH_SIZE(hash_table); - bkt++) { - - lock_uid_by_bkt(bkt); - hlist_for_each_entry(uid_entry, &hash_table[bkt], hash) { seq_printf(m, "%d %llu %llu %llu %llu %llu %llu %llu %llu %llu %llu\n", uid_entry->uid, uid_entry->io[UID_STATE_FOREGROUND].rchar, @@ -446,7 +396,6 @@ static ssize_t uid_procstat_write(struct file *file, uid_t uid; int argc, state; char input[128]; - struct uid_entry uid_entry_tmp; if (count >= sizeof(input)) return -EINVAL; @@ -475,29 +424,8 @@ static ssize_t uid_procstat_write(struct file *file, return count; } - /* - * Update_io_stats_uid_locked would take a long lock-time of uid_lock - * due to call do_each_thread to compute uid_entry->io, which would - * cause to lock competition sometime. - * - * Using uid_entry_tmp to get the result of Update_io_stats_uid, - * so that we can unlock_uid during update_io_stats_uid, in order - * to avoid the unnecessary lock-time of uid_lock. - */ - uid_entry_tmp = *uid_entry; - - unlock_uid(uid); - update_io_stats_uid(&uid_entry_tmp); - - lock_uid(uid); - hlist_for_each_entry(uid_entry, &hash_table[hash_min(uid, HASH_BITS(hash_table))], hash) { - if (uid_entry->uid == uid_entry_tmp.uid) { - memcpy(uid_entry->io, uid_entry_tmp.io, - sizeof(struct io_stats) * UID_STATE_SIZE); - uid_entry->state = state; - break; - } - } + update_io_stats_uid(uid_entry); + uid_entry->state = state; unlock_uid(uid); return count;