From: Joel Fernandes <joel@joelfernandes.org>
To: Shuah Khan <skhan@linuxfoundation.org>
Cc: gregkh@linuxfoundation.org, arve@android.com, tkjos@android.com,
maco@android.com, christian@brauner.io, hridya@google.com,
surenb@google.com, keescook@chromium.org,
devel@driverdev.osuosl.org, linux-kernel@vger.kernel.org
Subject: Re: [PATCH 07/11] drivers/android/binder: convert stats, transaction_log to counter_atomic32
Date: Sun, 27 Sep 2020 19:39:31 -0400 [thread overview]
Message-ID: <20200927233931.GB500818@google.com> (raw)
In-Reply-To: <ab2cb30977ac35cc172d30306da25178b742c328.1601073127.git.skhan@linuxfoundation.org>
On Fri, Sep 25, 2020 at 05:47:21PM -0600, Shuah Khan wrote:
> counter_atomic* is introduced to be used when a variable is used as
> a simple counter and doesn't guard object lifetimes. This clearly
> differentiates atomic_t usages that guard object lifetimes.
>
> counter_atomic* variables will wrap around to 0 when it overflows and
> should not be used to guard resource lifetimes, device usage and
> open counts that control state changes, and pm states.
>
> stats tracks per-process binder statistics. Unsure if there is a chance
> of this overflowing, other than stats getting reset to 0. Convert it to
> use counter_atomic.
>
> binder_transaction_log:cur is used to keep track of the current log entry
> location. Overflow is handled in the code. Since it is used as a
> counter, convert it to use counter_atomic32.
>
> This conversion doesn't change the overflow wrap around behavior.
>
> Signed-off-by: Shuah Khan <skhan@linuxfoundation.org>
Reviewed-by: Joel Fernandes (Google) <joel@joelfernandes.org>
thanks,
- Joel
> ---
> drivers/android/binder.c | 41 ++++++++++++++++---------------
> drivers/android/binder_internal.h | 3 ++-
> 2 files changed, 23 insertions(+), 21 deletions(-)
>
> diff --git a/drivers/android/binder.c b/drivers/android/binder.c
> index f936530a19b0..52175cd6a62b 100644
> --- a/drivers/android/binder.c
> +++ b/drivers/android/binder.c
> @@ -66,6 +66,7 @@
> #include <linux/syscalls.h>
> #include <linux/task_work.h>
> #include <linux/sizes.h>
> +#include <linux/counters.h>
>
> #include <uapi/linux/android/binder.h>
> #include <uapi/linux/android/binderfs.h>
> @@ -172,22 +173,22 @@ enum binder_stat_types {
> };
>
> struct binder_stats {
> - atomic_t br[_IOC_NR(BR_FAILED_REPLY) + 1];
> - atomic_t bc[_IOC_NR(BC_REPLY_SG) + 1];
> - atomic_t obj_created[BINDER_STAT_COUNT];
> - atomic_t obj_deleted[BINDER_STAT_COUNT];
> + struct counter_atomic32 br[_IOC_NR(BR_FAILED_REPLY) + 1];
> + struct counter_atomic32 bc[_IOC_NR(BC_REPLY_SG) + 1];
> + struct counter_atomic32 obj_created[BINDER_STAT_COUNT];
> + struct counter_atomic32 obj_deleted[BINDER_STAT_COUNT];
> };
>
> static struct binder_stats binder_stats;
>
> static inline void binder_stats_deleted(enum binder_stat_types type)
> {
> - atomic_inc(&binder_stats.obj_deleted[type]);
> + counter_atomic32_inc(&binder_stats.obj_deleted[type]);
> }
>
> static inline void binder_stats_created(enum binder_stat_types type)
> {
> - atomic_inc(&binder_stats.obj_created[type]);
> + counter_atomic32_inc(&binder_stats.obj_created[type]);
> }
>
> struct binder_transaction_log binder_transaction_log;
> @@ -197,7 +198,7 @@ static struct binder_transaction_log_entry *binder_transaction_log_add(
> struct binder_transaction_log *log)
> {
> struct binder_transaction_log_entry *e;
> - unsigned int cur = atomic_inc_return(&log->cur);
> + unsigned int cur = counter_atomic32_inc_return(&log->cur);
>
> if (cur >= ARRAY_SIZE(log->entry))
> log->full = true;
> @@ -3615,9 +3616,9 @@ static int binder_thread_write(struct binder_proc *proc,
> ptr += sizeof(uint32_t);
> trace_binder_command(cmd);
> if (_IOC_NR(cmd) < ARRAY_SIZE(binder_stats.bc)) {
> - atomic_inc(&binder_stats.bc[_IOC_NR(cmd)]);
> - atomic_inc(&proc->stats.bc[_IOC_NR(cmd)]);
> - atomic_inc(&thread->stats.bc[_IOC_NR(cmd)]);
> + counter_atomic32_inc(&binder_stats.bc[_IOC_NR(cmd)]);
> + counter_atomic32_inc(&proc->stats.bc[_IOC_NR(cmd)]);
> + counter_atomic32_inc(&thread->stats.bc[_IOC_NR(cmd)]);
> }
> switch (cmd) {
> case BC_INCREFS:
> @@ -4047,9 +4048,9 @@ static void binder_stat_br(struct binder_proc *proc,
> {
> trace_binder_return(cmd);
> if (_IOC_NR(cmd) < ARRAY_SIZE(binder_stats.br)) {
> - atomic_inc(&binder_stats.br[_IOC_NR(cmd)]);
> - atomic_inc(&proc->stats.br[_IOC_NR(cmd)]);
> - atomic_inc(&thread->stats.br[_IOC_NR(cmd)]);
> + counter_atomic32_inc(&binder_stats.br[_IOC_NR(cmd)]);
> + counter_atomic32_inc(&proc->stats.br[_IOC_NR(cmd)]);
> + counter_atomic32_inc(&thread->stats.br[_IOC_NR(cmd)]);
> }
> }
>
> @@ -5841,7 +5842,7 @@ static void print_binder_stats(struct seq_file *m, const char *prefix,
> BUILD_BUG_ON(ARRAY_SIZE(stats->bc) !=
> ARRAY_SIZE(binder_command_strings));
> for (i = 0; i < ARRAY_SIZE(stats->bc); i++) {
> - int temp = atomic_read(&stats->bc[i]);
> + int temp = counter_atomic32_read(&stats->bc[i]);
>
> if (temp)
> seq_printf(m, "%s%s: %d\n", prefix,
> @@ -5851,7 +5852,7 @@ static void print_binder_stats(struct seq_file *m, const char *prefix,
> BUILD_BUG_ON(ARRAY_SIZE(stats->br) !=
> ARRAY_SIZE(binder_return_strings));
> for (i = 0; i < ARRAY_SIZE(stats->br); i++) {
> - int temp = atomic_read(&stats->br[i]);
> + int temp = counter_atomic32_read(&stats->br[i]);
>
> if (temp)
> seq_printf(m, "%s%s: %d\n", prefix,
> @@ -5863,8 +5864,8 @@ static void print_binder_stats(struct seq_file *m, const char *prefix,
> BUILD_BUG_ON(ARRAY_SIZE(stats->obj_created) !=
> ARRAY_SIZE(stats->obj_deleted));
> for (i = 0; i < ARRAY_SIZE(stats->obj_created); i++) {
> - int created = atomic_read(&stats->obj_created[i]);
> - int deleted = atomic_read(&stats->obj_deleted[i]);
> + int created = counter_atomic32_read(&stats->obj_created[i]);
> + int deleted = counter_atomic32_read(&stats->obj_deleted[i]);
>
> if (created || deleted)
> seq_printf(m, "%s%s: active %d total %d\n",
> @@ -6054,7 +6055,7 @@ static void print_binder_transaction_log_entry(struct seq_file *m,
> int binder_transaction_log_show(struct seq_file *m, void *unused)
> {
> struct binder_transaction_log *log = m->private;
> - unsigned int log_cur = atomic_read(&log->cur);
> + unsigned int log_cur = counter_atomic32_read(&log->cur);
> unsigned int count;
> unsigned int cur;
> int i;
> @@ -6124,8 +6125,8 @@ static int __init binder_init(void)
> if (ret)
> return ret;
>
> - atomic_set(&binder_transaction_log.cur, ~0U);
> - atomic_set(&binder_transaction_log_failed.cur, ~0U);
> + counter_atomic32_set(&binder_transaction_log.cur, ~0U);
> + counter_atomic32_set(&binder_transaction_log_failed.cur, ~0U);
>
> binder_debugfs_dir_entry_root = debugfs_create_dir("binder", NULL);
> if (binder_debugfs_dir_entry_root)
> diff --git a/drivers/android/binder_internal.h b/drivers/android/binder_internal.h
> index 283d3cb9c16e..c77960c01430 100644
> --- a/drivers/android/binder_internal.h
> +++ b/drivers/android/binder_internal.h
> @@ -12,6 +12,7 @@
> #include <linux/stddef.h>
> #include <linux/types.h>
> #include <linux/uidgid.h>
> +#include <linux/counters.h>
>
> struct binder_context {
> struct binder_node *binder_context_mgr_node;
> @@ -136,7 +137,7 @@ struct binder_transaction_log_entry {
> };
>
> struct binder_transaction_log {
> - atomic_t cur;
> + struct counter_atomic32 cur;
> bool full;
> struct binder_transaction_log_entry entry[32];
> };
> --
> 2.25.1
>
next prev parent reply other threads:[~2020-09-27 23:39 UTC|newest]
Thread overview: 30+ messages / expand[flat|nested] mbox.gz Atom feed top
2020-09-25 23:47 [PATCH 00/11] Introduce Simple atomic and non-atomic counters Shuah Khan
2020-09-25 23:47 ` [PATCH 01/11] counters: Introduce counter_simple* and counter_atomic* counters Shuah Khan
2020-09-25 23:47 ` [PATCH 02/11] selftests:lib:test_counters: add new test for counters Shuah Khan
2020-09-25 23:47 ` [PATCH 03/11] drivers/base: convert deferred_trigger_count and probe_count to counter_atomic32 Shuah Khan
2020-09-25 23:47 ` [PATCH 04/11] drivers/base/devcoredump: convert devcd_count " Shuah Khan
2020-09-25 23:47 ` [PATCH 05/11] drivers/acpi: convert seqno counter_atomic32 Shuah Khan
2020-09-25 23:47 ` [PATCH 06/11] drivers/acpi/apei: " Shuah Khan
2020-09-25 23:47 ` [PATCH 07/11] drivers/android/binder: convert stats, transaction_log to counter_atomic32 Shuah Khan
2020-09-27 23:39 ` Joel Fernandes [this message]
2020-09-25 23:47 ` [PATCH 08/11] drivers/base/test/test_async_driver_probe: convert to use counter_atomic32 Shuah Khan
2020-09-25 23:47 ` [PATCH 09/11] drivers/char/ipmi: convert stats " Shuah Khan
2020-09-26 0:15 ` Corey Minyard
2020-09-26 2:05 ` Shuah Khan
2020-09-25 23:47 ` [PATCH 10/11] drivers/misc/vmw_vmci: convert num guest devices counter to counter_atomic32 Shuah Khan
2020-09-25 23:47 ` [PATCH 11/11] drivers/edac: convert pci counters " Shuah Khan
2020-09-28 12:05 ` Borislav Petkov
2020-09-25 23:52 ` [PATCH 00/11] Introduce Simple atomic and non-atomic counters Kees Cook
2020-09-26 0:13 ` Shuah Khan
2020-09-26 16:33 ` Kees Cook
2020-09-28 22:52 ` Shuah Khan
2020-09-26 16:22 ` Kees Cook
2020-09-28 22:42 ` Shuah Khan
2020-09-26 16:29 ` Kees Cook
2020-09-28 22:41 ` Shuah Khan
2020-09-28 23:13 ` Kees Cook
2020-10-06 15:21 ` Shuah Khan
2020-09-27 23:35 ` Joel Fernandes
2020-09-28 20:34 ` Kees Cook
2020-09-28 21:17 ` Joel Fernandes
2020-09-28 23:01 ` Shuah Khan
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20200927233931.GB500818@google.com \
--to=joel@joelfernandes.org \
--cc=arve@android.com \
--cc=christian@brauner.io \
--cc=devel@driverdev.osuosl.org \
--cc=gregkh@linuxfoundation.org \
--cc=hridya@google.com \
--cc=keescook@chromium.org \
--cc=linux-kernel@vger.kernel.org \
--cc=maco@android.com \
--cc=skhan@linuxfoundation.org \
--cc=surenb@google.com \
--cc=tkjos@android.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox