[PATCH 0/2] taskstats: Cleanup patches

[PATCH 0/2] taskstats: Cleanup patches

[Michael Holzheu]

Hello Andrew,

It would be great, if you could accept the taskstats cleanup patches that
are the prerequisite for the taskstats precise accounting patches. The
patches do not add any new functionality. I think they make the code better
readable and extensible:
* 01/02: taskstats: Separate taskstats commands
* 02/02: taskstats: Split fill_pid function

Michael

[PATCH 1/2] taskstats: Separate taskstats commands

[Michael Holzheu]

[-- Attachment #1: 02-taskstats-top-prepare-1.patch --]
[-- Type: text/plain, Size: 4170 bytes --]

From: Michael Holzheu <holzheu@linux.vnet.ibm.com>

This patch moves each taskstats command into a single function. This
makes the code more readable and makes it easier to add new commands.

Signed-off-by: Michael Holzheu <holzheu@linux.vnet.ibm.com>
Acked-by: Balbir Singh <balbir@linux.vnet.ibm.com>
---
 kernel/taskstats.c |  118 +++++++++++++++++++++++++++++++++++------------------
 1 file changed, 78 insertions(+), 40 deletions(-)

--- a/kernel/taskstats.c
+++ b/kernel/taskstats.c
@@ -424,39 +424,76 @@ err:
 	return rc;
 }
 
-static int taskstats_user_cmd(struct sk_buff *skb, struct genl_info *info)
+static int cmd_attr_register_cpumask(struct genl_info *info)
 {
-	int rc;
-	struct sk_buff *rep_skb;
-	struct taskstats *stats;
-	size_t size;
 	cpumask_var_t mask;
+	int rc;
 
 	if (!alloc_cpumask_var(&mask, GFP_KERNEL))
 		return -ENOMEM;
-
 	rc = parse(info->attrs[TASKSTATS_CMD_ATTR_REGISTER_CPUMASK], mask);
 	if (rc < 0)
-		goto free_return_rc;
-	if (rc == 0) {
-		rc = add_del_listener(info->snd_pid, mask, REGISTER);
-		goto free_return_rc;
-	}
+		goto out;
+	rc = add_del_listener(info->snd_pid, mask, REGISTER);
+out:
+	free_cpumask_var(mask);
+	return rc;
+}
+
+static int cmd_attr_deregister_cpumask(struct genl_info *info)
+{
+	cpumask_var_t mask;
+	int rc;
 
+	if (!alloc_cpumask_var(&mask, GFP_KERNEL))
+		return -ENOMEM;
 	rc = parse(info->attrs[TASKSTATS_CMD_ATTR_DEREGISTER_CPUMASK], mask);
 	if (rc < 0)
-		goto free_return_rc;
-	if (rc == 0) {
-		rc = add_del_listener(info->snd_pid, mask, DEREGISTER);
-free_return_rc:
-		free_cpumask_var(mask);
-		return rc;
-	}
+		goto out;
+	rc = add_del_listener(info->snd_pid, mask, DEREGISTER);
+out:
 	free_cpumask_var(mask);
+	return rc;
+}
+
+static int cmd_attr_pid(struct genl_info *info)
+{
+	struct taskstats *stats;
+	struct sk_buff *rep_skb;
+	size_t size;
+	u32 pid;
+	int rc;
+
+	size = nla_total_size(sizeof(u32)) +
+		nla_total_size(sizeof(struct taskstats)) + nla_total_size(0);
+
+	rc = prepare_reply(info, TASKSTATS_CMD_NEW, &rep_skb, size);
+	if (rc < 0)
+		return rc;
+
+	rc = -EINVAL;
+	pid = nla_get_u32(info->attrs[TASKSTATS_CMD_ATTR_PID]);
+	stats = mk_reply(rep_skb, TASKSTATS_TYPE_PID, pid);
+	if (!stats)
+		goto err;
+
+	rc = fill_pid(pid, NULL, stats);
+	if (rc < 0)
+		goto err;
+	return send_reply(rep_skb, info);
+err:
+	nlmsg_free(rep_skb);
+	return rc;
+}
+
+static int cmd_attr_tgid(struct genl_info *info)
+{
+	struct taskstats *stats;
+	struct sk_buff *rep_skb;
+	size_t size;
+	u32 tgid;
+	int rc;
 
-	/*
-	 * Size includes space for nested attributes
-	 */
 	size = nla_total_size(sizeof(u32)) +
 		nla_total_size(sizeof(struct taskstats)) + nla_total_size(0);
 
@@ -465,33 +502,34 @@ free_return_rc:
 		return rc;
 
 	rc = -EINVAL;
-	if (info->attrs[TASKSTATS_CMD_ATTR_PID]) {
-		u32 pid = nla_get_u32(info->attrs[TASKSTATS_CMD_ATTR_PID]);
-		stats = mk_reply(rep_skb, TASKSTATS_TYPE_PID, pid);
-		if (!stats)
-			goto err;
-
-		rc = fill_pid(pid, NULL, stats);
-		if (rc < 0)
-			goto err;
-	} else if (info->attrs[TASKSTATS_CMD_ATTR_TGID]) {
-		u32 tgid = nla_get_u32(info->attrs[TASKSTATS_CMD_ATTR_TGID]);
-		stats = mk_reply(rep_skb, TASKSTATS_TYPE_TGID, tgid);
-		if (!stats)
-			goto err;
-
-		rc = fill_tgid(tgid, NULL, stats);
-		if (rc < 0)
-			goto err;
-	} else
+	tgid = nla_get_u32(info->attrs[TASKSTATS_CMD_ATTR_TGID]);
+	stats = mk_reply(rep_skb, TASKSTATS_TYPE_TGID, tgid);
+	if (!stats)
 		goto err;
 
+	rc = fill_tgid(tgid, NULL, stats);
+	if (rc < 0)
+		goto err;
 	return send_reply(rep_skb, info);
 err:
 	nlmsg_free(rep_skb);
 	return rc;
 }
 
+static int taskstats_user_cmd(struct sk_buff *skb, struct genl_info *info)
+{
+	if (info->attrs[TASKSTATS_CMD_ATTR_REGISTER_CPUMASK])
+		return cmd_attr_register_cpumask(info);
+	else if (info->attrs[TASKSTATS_CMD_ATTR_DEREGISTER_CPUMASK])
+		return cmd_attr_deregister_cpumask(info);
+	else if (info->attrs[TASKSTATS_CMD_ATTR_PID])
+		return cmd_attr_pid(info);
+	else if (info->attrs[TASKSTATS_CMD_ATTR_TGID])
+		return cmd_attr_tgid(info);
+	else
+		return -EINVAL;
+}
+
 static struct taskstats *taskstats_tgid_alloc(struct task_struct *tsk)
 {
 	struct signal_struct *sig = tsk->signal;

[PATCH 2/2] taskstats: Split fill_pid function

[Michael Holzheu]

[-- Attachment #1: 03-taskstats-top-prepare-2.patch --]
[-- Type: text/plain, Size: 2960 bytes --]

From: Michael Holzheu <holzheu@linux.vnet.ibm.com>

Separate the finding of a task_struct by pid or tgid from filling the
taskstats data. This makes the code more readable.

Signed-off-by: Michael Holzheu <holzheu@linux.vnet.ibm.com>
Acked-by: Balbir Singh <balbir@linux.vnet.ibm.com>
---
 kernel/taskstats.c |   50 +++++++++++++++++++++-----------------------------
 1 file changed, 21 insertions(+), 29 deletions(-)

--- a/kernel/taskstats.c
+++ b/kernel/taskstats.c
@@ -175,22 +175,8 @@ static void send_cpu_listeners(struct sk
 	up_write(&listeners->sem);
 }
 
-static int fill_pid(pid_t pid, struct task_struct *tsk,
-		struct taskstats *stats)
+static void fill_stats(struct task_struct *tsk, struct taskstats *stats)
 {
-	int rc = 0;
-
-	if (!tsk) {
-		rcu_read_lock();
-		tsk = find_task_by_vpid(pid);
-		if (tsk)
-			get_task_struct(tsk);
-		rcu_read_unlock();
-		if (!tsk)
-			return -ESRCH;
-	} else
-		get_task_struct(tsk);
-
 	memset(stats, 0, sizeof(*stats));
 	/*
 	 * Each accounting subsystem adds calls to its functions to
@@ -209,17 +195,27 @@ static int fill_pid(pid_t pid, struct ta
 
 	/* fill in extended acct fields */
 	xacct_add_tsk(stats, tsk);
+}
 
-	/* Define err: label here if needed */
-	put_task_struct(tsk);
-	return rc;
+static int fill_stats_for_pid(pid_t pid, struct taskstats *stats)
+{
+	struct task_struct *tsk;
 
+	rcu_read_lock();
+	tsk = find_task_by_vpid(pid);
+	if (tsk)
+		get_task_struct(tsk);
+	rcu_read_unlock();
+	if (!tsk)
+		return -ESRCH;
+	fill_stats(tsk, stats);
+	put_task_struct(tsk);
+	return 0;
 }
 
-static int fill_tgid(pid_t tgid, struct task_struct *first,
-		struct taskstats *stats)
+static int fill_stats_for_tgid(pid_t tgid, struct taskstats *stats)
 {
-	struct task_struct *tsk;
+	struct task_struct *tsk, *first;
 	unsigned long flags;
 	int rc = -ESRCH;
 
@@ -228,8 +224,7 @@ static int fill_tgid(pid_t tgid, struct 
 	 * leaders who are already counted with the dead tasks
 	 */
 	rcu_read_lock();
-	if (!first)
-		first = find_task_by_vpid(tgid);
+	first = find_task_by_vpid(tgid);
 
 	if (!first || !lock_task_sighand(first, &flags))
 		goto out;
@@ -268,7 +263,6 @@ out:
 	return rc;
 }
 
-
 static void fill_tgid_exit(struct task_struct *tsk)
 {
 	unsigned long flags;
@@ -477,7 +471,7 @@ static int cmd_attr_pid(struct genl_info
 	if (!stats)
 		goto err;
 
-	rc = fill_pid(pid, NULL, stats);
+	rc = fill_stats_for_pid(pid, stats);
 	if (rc < 0)
 		goto err;
 	return send_reply(rep_skb, info);
@@ -507,7 +501,7 @@ static int cmd_attr_tgid(struct genl_inf
 	if (!stats)
 		goto err;
 
-	rc = fill_tgid(tgid, NULL, stats);
+	rc = fill_stats_for_tgid(tgid, stats);
 	if (rc < 0)
 		goto err;
 	return send_reply(rep_skb, info);
@@ -593,9 +587,7 @@ void taskstats_exit(struct task_struct *
 	if (!stats)
 		goto err;
 
-	rc = fill_pid(-1, tsk, stats);
-	if (rc < 0)
-		goto err;
+	fill_stats(tsk, stats);
 
 	/*
 	 * Doesn't matter if tsk is the leader or the last group member leaving

Re: [PATCH 0/2] taskstats: Cleanup patches

[Andrew Morton]

On Tue, 28 Sep 2010 16:20:58 +0200
Michael Holzheu <holzheu@linux.vnet.ibm.com> wrote:

> Hello Andrew,
> 
> It would be great, if you could accept the taskstats cleanup patches that
> are the prerequisite for the taskstats precise accounting patches. The
> patches do not add any new functionality. I think they make the code better
> readable and extensible:
> * 01/02: taskstats: Separate taskstats commands
> * 02/02: taskstats: Split fill_pid function
> 

Sure.

I've been sitting on a couple of taskstats patches for ages.  Mel's
delay-accounting-re-implement-c-for-getdelaysc-to-report-information-on-a-target-command.patch
and Jeff's delayacct-align-to-8-byte-boundary-on-64-bit-systems.patch.

I have notes against both of these indicating that Balbir had concerns
and as far as I know those concerns remain unresolved.  So I'll drop
those patches now - can you guys please reactivate them if you still
think we should be making these changes?

Re: [PATCH 0/2] taskstats: Cleanup patches

[Balbir Singh]

* Andrew Morton <akpm@linux-foundation.org> [2010-09-28 13:51:17]:

> On Tue, 28 Sep 2010 16:20:58 +0200
> Michael Holzheu <holzheu@linux.vnet.ibm.com> wrote:
> 
> > Hello Andrew,
> > 
> > It would be great, if you could accept the taskstats cleanup patches that
> > are the prerequisite for the taskstats precise accounting patches. The
> > patches do not add any new functionality. I think they make the code better
> > readable and extensible:
> > * 01/02: taskstats: Separate taskstats commands
> > * 02/02: taskstats: Split fill_pid function
> > 
> 
> Sure.
> 
> I've been sitting on a couple of taskstats patches for ages.  Mel's
> delay-accounting-re-implement-c-for-getdelaysc-to-report-information-on-a-target-command.patch
> and Jeff's delayacct-align-to-8-byte-boundary-on-64-bit-systems.patch.
> 
> I have notes against both of these indicating that Balbir had concerns
> and as far as I know those concerns remain unresolved.  So I'll drop
> those patches now - can you guys please reactivate them if you still
> think we should be making these changes?
> 

Hi, Andrew,

My concern with Jeff's patch was that it might break existing
applications. He clarified it does not, I had requested for a version
bump since the patches change some definitions

I had no concerns (IIRC) with Mel's patches. Mel wanted me to
implement the "-c" option we had earlier.


-- 
	Three Cheers,
	Balbir

Re: [PATCH 0/2] taskstats: Cleanup patches

[Andrew Morton]

On Wed, 29 Sep 2010 13:20:44 +0530
Balbir Singh <balbir@linux.vnet.ibm.com> wrote:

> * Andrew Morton <akpm@linux-foundation.org> [2010-09-28 13:51:17]:
> 
> > On Tue, 28 Sep 2010 16:20:58 +0200
> > Michael Holzheu <holzheu@linux.vnet.ibm.com> wrote:
> > 
> > > Hello Andrew,
> > > 
> > > It would be great, if you could accept the taskstats cleanup patches that
> > > are the prerequisite for the taskstats precise accounting patches. The
> > > patches do not add any new functionality. I think they make the code better
> > > readable and extensible:
> > > * 01/02: taskstats: Separate taskstats commands
> > > * 02/02: taskstats: Split fill_pid function
> > > 
> > 
> > Sure.
> > 
> > I've been sitting on a couple of taskstats patches for ages.  Mel's
> > delay-accounting-re-implement-c-for-getdelaysc-to-report-information-on-a-target-command.patch
> > and Jeff's delayacct-align-to-8-byte-boundary-on-64-bit-systems.patch.
> > 
> > I have notes against both of these indicating that Balbir had concerns
> > and as far as I know those concerns remain unresolved.  So I'll drop
> > those patches now - can you guys please reactivate them if you still
> > think we should be making these changes?
> > 
> 
> Hi, Andrew,
> 
> My concern with Jeff's patch was that it might break existing
> applications. He clarified it does not, I had requested for a version
> bump since the patches change some definitions
> 
> I had no concerns (IIRC) with Mel's patches. Mel wanted me to
> implement the "-c" option we had earlier.
> 

hm, OK, thanks, I requeued them for 2.6.37.

Re: [PATCH 0/2] taskstats: Cleanup patches

[Balbir Singh]

* Andrew Morton <akpm@linux-foundation.org> [2010-09-29 11:01:13]:

> On Wed, 29 Sep 2010 13:20:44 +0530
> Balbir Singh <balbir@linux.vnet.ibm.com> wrote:
> 
> > * Andrew Morton <akpm@linux-foundation.org> [2010-09-28 13:51:17]:
> > 
> > > On Tue, 28 Sep 2010 16:20:58 +0200
> > > Michael Holzheu <holzheu@linux.vnet.ibm.com> wrote:
> > > 
> > > > Hello Andrew,
> > > > 
> > > > It would be great, if you could accept the taskstats cleanup patches that
> > > > are the prerequisite for the taskstats precise accounting patches. The
> > > > patches do not add any new functionality. I think they make the code better
> > > > readable and extensible:
> > > > * 01/02: taskstats: Separate taskstats commands
> > > > * 02/02: taskstats: Split fill_pid function
> > > > 
> > > 
> > > Sure.
> > > 
> > > I've been sitting on a couple of taskstats patches for ages.  Mel's
> > > delay-accounting-re-implement-c-for-getdelaysc-to-report-information-on-a-target-command.patch
> > > and Jeff's delayacct-align-to-8-byte-boundary-on-64-bit-systems.patch.
> > > 
> > > I have notes against both of these indicating that Balbir had concerns
> > > and as far as I know those concerns remain unresolved.  So I'll drop
> > > those patches now - can you guys please reactivate them if you still
> > > think we should be making these changes?
> > > 
> > 
> > Hi, Andrew,
> > 
> > My concern with Jeff's patch was that it might break existing
> > applications. He clarified it does not, I had requested for a version
> > bump since the patches change some definitions
> > 
> > I had no concerns (IIRC) with Mel's patches. Mel wanted me to
> > implement the "-c" option we had earlier.
> > 
> 
> hm, OK, thanks, I requeued them for 2.6.37.

Thanks, Andrew!

-- 
	Three Cheers,
	Balbir