[PATCH 0/4] taskstats: four more tidy-ups in the genl path

[PATCH 0/4] taskstats: four more tidy-ups in the genl path

[Bradley Morgan]

Four small cleanups from reading kernel/taskstats.c again.  No functional
change in any of them.

Patch 1 uses nla_put_u32() in mk_reply() instead of open coding the u32
attribute with nla_put() and a sizeof/& pair.

Patch 2 grabs the inode once with file_inode() in exe_add_tsk(), which
walks exe_file->f_inode twice today, once for the device and once for the
number.

Patch 3 reads the cgroupstats fd out of na, which the function already
fetched, instead of indexing info->attrs[] again for the same attribute.

Patch 4 converts the elapsed time with div_u64() in
tgid_stats_add_task(), like the utime and stime lines four below it do,
instead of do_div().

Balbir Singh has moved to balbirs@nvidia.com, MAINTAINERS still lists the
old gmail address.

Bradley Morgan (4):
  taskstats: use nla_put_u32() in mk_reply()
  taskstats: take the inode once in exe_add_tsk()
  taskstats: reuse na in cgroupstats_user_cmd()
  taskstats: use div_u64() for the elapsed time in tgid_stats_add_task()

 kernel/taskstats.c | 14 +++++++-------
 1 file changed, 7 insertions(+), 7 deletions(-)

-- 
2.53.0

[PATCH 1/4] taskstats: use nla_put_u32() in mk_reply()

[Bradley Morgan]

mk_reply() open codes a u32 attribute, nla_put() with sizeof(pid) and
&pid fed to it by hand.  nla_put_u32() is those same three arguments
already collapsed, and it is what the readers of this attribute use.
The helper returns 0 or a negative error, so the bare if() catches
exactly what the < 0 comparison caught.

No functional change.

Signed-off-by: Bradley Morgan <brads@mainlining.org>
---
 kernel/taskstats.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/kernel/taskstats.c b/kernel/taskstats.c
index bf517788b8409..37422c43b8caa 100644
--- a/kernel/taskstats.c
+++ b/kernel/taskstats.c
@@ -386,7 +386,7 @@ static struct taskstats *mk_reply(struct sk_buff *skb, int type, u32 pid)
 	if (!na)
 		goto err;
 
-	if (nla_put(skb, type, sizeof(pid), &pid) < 0) {
+	if (nla_put_u32(skb, type, pid)) {
 		nla_nest_cancel(skb, na);
 		goto err;
 	}
-- 
2.53.0

[PATCH 2/4] taskstats: take the inode once in exe_add_tsk()

[Bradley Morgan]

exe_add_tsk() walks exe_file->f_inode twice, once for the superblock
device and once for the inode number.  file_inode() is the accessor for
that field, and hoisting the inode out of the two assignments also gets
stats->ac_exe_dev onto one line instead of a split continuation.

Signed-off-by: Bradley Morgan <brads@mainlining.org>
---
 kernel/taskstats.c | 7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)

diff --git a/kernel/taskstats.c b/kernel/taskstats.c
index 37422c43b8caa..35e9fa71526cc 100644
--- a/kernel/taskstats.c
+++ b/kernel/taskstats.c
@@ -159,10 +159,11 @@ static void exe_add_tsk(struct taskstats *stats, struct task_struct *tsk)
 	struct file *exe_file = get_task_exe_file(tsk);
 
 	if (exe_file) {
+		struct inode *inode = file_inode(exe_file);
+
 		/* Following cp_new_stat64() in stat.c . */
-		stats->ac_exe_dev =
-			huge_encode_dev(exe_file->f_inode->i_sb->s_dev);
-		stats->ac_exe_inode = exe_file->f_inode->i_ino;
+		stats->ac_exe_dev = huge_encode_dev(inode->i_sb->s_dev);
+		stats->ac_exe_inode = inode->i_ino;
 		fput(exe_file);
 	} else {
 		stats->ac_exe_dev = 0;
-- 
2.53.0

[PATCH 3/4] taskstats: reuse na in cgroupstats_user_cmd()

[Bradley Morgan]

cgroupstats_user_cmd() already pulled CGROUPSTATS_CMD_ATTR_FD into na to
check that the user sent it, then indexes info->attrs[] a second time to
read the value out of the very same attribute.  Read it from na.

No functional change.

Signed-off-by: Bradley Morgan <brads@mainlining.org>
---
 kernel/taskstats.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/kernel/taskstats.c b/kernel/taskstats.c
index 35e9fa71526cc..9e11f92216861 100644
--- a/kernel/taskstats.c
+++ b/kernel/taskstats.c
@@ -417,7 +417,7 @@ static int cgroupstats_user_cmd(struct sk_buff *skb, struct genl_info *info)
 	if (!na)
 		return -EINVAL;
 
-	fd = nla_get_u32(info->attrs[CGROUPSTATS_CMD_ATTR_FD]);
+	fd = nla_get_u32(na);
 	CLASS(fd, f)(fd);
 	if (fd_empty(f))
 		return -EBADF;
-- 
2.53.0

[PATCH 4/4] taskstats: use div_u64() for the elapsed time in tgid_stats_add_task()

[Bradley Morgan]

tgid_stats_add_task() converts utime and stime with div_u64() and the
elapsed time with do_div(), two ways of doing the same 64-bit divide four
lines apart.  do_div() clobbers its first argument, which is the only
reason delta needs a statement of its own.  Both end up at a native
divide on 64-bit and at __div64_32() on 32-bit, so this is only about
picking one.

Signed-off-by: Bradley Morgan <brads@mainlining.org>
---
 kernel/taskstats.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/kernel/taskstats.c b/kernel/taskstats.c
index 9e11f92216861..2d4ecb9a89a91 100644
--- a/kernel/taskstats.c
+++ b/kernel/taskstats.c
@@ -226,8 +226,7 @@ static void tgid_stats_add_task(struct taskstats *stats,
 	/* calculate task elapsed time in nsec */
 	delta = now_ns - tsk->start_time;
 	/* Convert to micro seconds */
-	do_div(delta, NSEC_PER_USEC);
-	stats->ac_etime += delta;
+	stats->ac_etime += div_u64(delta, NSEC_PER_USEC);
 
 	task_cputime(tsk, &utime, &stime);
 	stats->ac_utime += div_u64(utime, NSEC_PER_USEC);
-- 
2.53.0

Re: [PATCH 4/4] taskstats: use div_u64() for the elapsed time in tgid_stats_add_task()

[Balbir Singh]

On 11/1/26 11:57 AM, Bradley Morgan wrote:
> tgid_stats_add_task() converts utime and stime with div_u64() and the
> elapsed time with do_div(), two ways of doing the same 64-bit divide four
> lines apart.  do_div() clobbers its first argument, which is the only
> reason delta needs a statement of its own.  Both end up at a native
> divide on 64-bit and at __div64_32() on 32-bit, so this is only about
> picking one.
> 
> Signed-off-by: Bradley Morgan <brads@mainlining.org>
> ---
>  kernel/taskstats.c | 3 +--
>  1 file changed, 1 insertion(+), 2 deletions(-)
> 
> diff --git a/kernel/taskstats.c b/kernel/taskstats.c
> index 9e11f92216861..2d4ecb9a89a91 100644
> --- a/kernel/taskstats.c
> +++ b/kernel/taskstats.c
> @@ -226,8 +226,7 @@ static void tgid_stats_add_task(struct taskstats *stats,
>  	/* calculate task elapsed time in nsec */
>  	delta = now_ns - tsk->start_time;
>  	/* Convert to micro seconds */
> -	do_div(delta, NSEC_PER_USEC);
> -	stats->ac_etime += delta;
> +	stats->ac_etime += div_u64(delta, NSEC_PER_USEC);
>  
>  	task_cputime(tsk, &utime, &stime);
>  	stats->ac_utime += div_u64(utime, NSEC_PER_USEC);

Thanks!
Reviewed-by: Balbir Singh <balbirs@nvidia.com>

Re: [PATCH 3/4] taskstats: reuse na in cgroupstats_user_cmd()

[Balbir Singh]

On 11/1/26 11:57 AM, Bradley Morgan wrote:
> cgroupstats_user_cmd() already pulled CGROUPSTATS_CMD_ATTR_FD into na to
> check that the user sent it, then indexes info->attrs[] a second time to
> read the value out of the very same attribute.  Read it from na.
> 
> No functional change.
> 
> Signed-off-by: Bradley Morgan <brads@mainlining.org>
> ---
>  kernel/taskstats.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/kernel/taskstats.c b/kernel/taskstats.c
> index 35e9fa71526cc..9e11f92216861 100644
> --- a/kernel/taskstats.c
> +++ b/kernel/taskstats.c
> @@ -417,7 +417,7 @@ static int cgroupstats_user_cmd(struct sk_buff *skb, struct genl_info *info)
>  	if (!na)
>  		return -EINVAL;
>  
> -	fd = nla_get_u32(info->attrs[CGROUPSTATS_CMD_ATTR_FD]);
> +	fd = nla_get_u32(na);
>  	CLASS(fd, f)(fd);
>  	if (fd_empty(f))
>  		return -EBADF;

Reviewed-by: Balbir Singh <balbirs@nvidia.com>

Re: [PATCH 2/4] taskstats: take the inode once in exe_add_tsk()

[Balbir Singh]

On 11/1/26 11:57 AM, Bradley Morgan wrote:
> exe_add_tsk() walks exe_file->f_inode twice, once for the superblock
> device and once for the inode number.  file_inode() is the accessor for
> that field, and hoisting the inode out of the two assignments also gets
> stats->ac_exe_dev onto one line instead of a split continuation.
> 
> Signed-off-by: Bradley Morgan <brads@mainlining.org>
> ---
>  kernel/taskstats.c | 7 ++++---
>  1 file changed, 4 insertions(+), 3 deletions(-)
> 
> diff --git a/kernel/taskstats.c b/kernel/taskstats.c
> index 37422c43b8caa..35e9fa71526cc 100644
> --- a/kernel/taskstats.c
> +++ b/kernel/taskstats.c
> @@ -159,10 +159,11 @@ static void exe_add_tsk(struct taskstats *stats, struct task_struct *tsk)
>  	struct file *exe_file = get_task_exe_file(tsk);
>  
>  	if (exe_file) {
> +		struct inode *inode = file_inode(exe_file);
> +
>  		/* Following cp_new_stat64() in stat.c . */
> -		stats->ac_exe_dev =
> -			huge_encode_dev(exe_file->f_inode->i_sb->s_dev);
> -		stats->ac_exe_inode = exe_file->f_inode->i_ino;
> +		stats->ac_exe_dev = huge_encode_dev(inode->i_sb->s_dev);
> +		stats->ac_exe_inode = inode->i_ino;
>  		fput(exe_file);
>  	} else {
>  		stats->ac_exe_dev = 0;

Thanks!

Reviewed-by: Balbir Singh <balbirs@nvidia.com>

Re: [PATCH 1/4] taskstats: use nla_put_u32() in mk_reply()

[Balbir Singh]

On 11/1/26 11:57 AM, Bradley Morgan wrote:
> mk_reply() open codes a u32 attribute, nla_put() with sizeof(pid) and
> &pid fed to it by hand.  nla_put_u32() is those same three arguments
> already collapsed, and it is what the readers of this attribute use.
> The helper returns 0 or a negative error, so the bare if() catches
> exactly what the < 0 comparison caught.
> 
> No functional change.
> 
> Signed-off-by: Bradley Morgan <brads@mainlining.org>
> ---
>  kernel/taskstats.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/kernel/taskstats.c b/kernel/taskstats.c
> index bf517788b8409..37422c43b8caa 100644
> --- a/kernel/taskstats.c
> +++ b/kernel/taskstats.c
> @@ -386,7 +386,7 @@ static struct taskstats *mk_reply(struct sk_buff *skb, int type, u32 pid)
>  	if (!na)
>  		goto err;
>  
> -	if (nla_put(skb, type, sizeof(pid), &pid) < 0) {
> +	if (nla_put_u32(skb, type, pid)) {
>  		nla_nest_cancel(skb, na);
>  		goto err;
>  	}

Thanks!

Reviewed-by: Balbir Singh <balbirs@nvidia.com>