Re: [RFC][UPDATED PATCH 2.6.16] [Patch 9/9] Generic netlink interface for delay accounting

[Balbir Singh <balbir@in.ibm.com>]

On Thu, Mar 23, 2006 at 09:04:46AM -0500, jamal wrote:
> 
> Hi Balbir,
> 
> Looking good.
> This is a quick scan, so i didnt look at little details.

Thanks for your detailed feedback. Please find my response interspersed
along with your comments.

> Some comments embedded.
> 
> On Wed, 2006-22-03 at 13:19 +0530, Balbir Singh wrote:
> 
> 
> 
> 
> >  
> 
> > diff -puN /dev/null include/linux/taskstats.h
> 
> > + * The struct is versioned. Newer versions should only add fields to
> > + * the bottom of the struct to maintain backward compatibility.
> > + *
> > + * To create the next version, bump up the taskstats_version variable
> > + * and delineate the start of newly added fields with a comment indicating
> > + * the version number.
> > + */
> > +
> 
> 
> 
> > +enum {
> > +	TASKSTATS_MSG_UNICAST,		/* send data only to requester */
> > +	TASKSTATS_MSG_MULTICAST,	/* send data to a group */
> > +};
> > +
> 
> Above will never be used outside of the kernel.
> Should it go under the ifdef kernel below?
>

Will do
 
> 
> > +#ifdef __KERNEL__
> > +
> 
> Note: some people will argue that you should probably have
> two header files. One for all kernel things and includes another
> which contains all the stuff above ifdef __KERNEL__
> 
> > +
> > +#endif /* __KERNEL__ */
> > +#endif /* _LINUX_TASKSTATS_H */
> 
> 
> 
> 
> > diff -puN kernel/Makefile~delayacct-genetlink kernel/Makefile
> > --- linux-2.6.16/kernel/Makefile~delayacct-genetlink	2006-03-22 11:56:03.000000000 +0530
> > +++ linux-2.6.16-balbir/kernel/Makefile	2006-03-22 11:56:03.000000000 +0530
> 
> > +
> > +const int taskstats_version = TASKSTATS_VERSION;
> > +static DEFINE_PER_CPU(__u32, taskstats_seqnum) = { 0 };
> > +static int family_registered = 0;
> > +
> > +static struct genl_family family = {
> > +	.id             = GENL_ID_GENERATE,
> > +	.name           = TASKSTATS_GENL_NAME,
> > +	.version        = TASKSTATS_GENL_VERSION,
> > +	.hdrsize        = 0,
> 
> Do you need to specify hdrsize of 0?

I will remove it

> 
> 
> > +static int prepare_reply(struct genl_info *info, u8 cmd, struct sk_buff **skbp,
> > +			 void **replyp)
> > +{
> > +	struct sk_buff *skb;
> > +	void *reply;
> > +
> > +	skb = nlmsg_new(NLMSG_GOODSIZE);
> 
> Ok,  getting a size of NLMSG_GOODSIZE is not a good idea. 
> The max size youll ever get it seems to me is 2*32-bit-data TLVs +
> sizeof struct stats. Why dont you allocate that size?
> 

Will do

> > +	if (!skb)
> > +		return -ENOMEM;
> > +
> > +	if (!info) {
> > +		int seq = get_cpu_var(taskstats_seqnum)++;
> > +		put_cpu_var(taskstats_seqnum);
> > +
> > +		reply = genlmsg_put(skb, 0, seq,
> > +				    family.id, 0, NLM_F_REQUEST,
> > +				    cmd, family.version);
> 
> Double check if you need NLM_F_REQUEST
> 

It is not required, I will remove it

> > +	} else
> > +		reply = genlmsg_put(skb, info->snd_pid, info->snd_seq,
> > +				    family.id, 0, info->nlhdr->nlmsg_flags,
> > +				    info->genlhdr->cmd, family.version);
> 
> A Response to a GET is a NEW. So i dont think info->genlhdr->cmd is the
> right thing?
> 
> 

I will change it

> 
> 
> 
> > +static int taskstats_send_stats(struct sk_buff *skb, struct genl_info *info)
> > +{
> > +	int rc;
> > +	struct sk_buff *rep_skb;
> > +	struct taskstats stats;
> > +	void *reply;
> > +
> > +	memset(&stats, 0, sizeof(stats));
> > +	rc = prepare_reply(info, info->genlhdr->cmd, &rep_skb, &reply);
> 
> Same comment as before: a response to a GET is a NEW; so
> info->genlhdr->cmd doesnt seem right.
> 

I will change it

> > +	if (rc < 0)
> > +		return rc;
> > +
> > +	if (info->attrs[TASKSTATS_CMD_ATTR_PID]) {
> > +		u32 pid = nla_get_u32(info->attrs[TASKSTATS_CMD_ATTR_PID]);
> > +		NLA_PUT_U32(rep_skb, TASKSTATS_TYPE_PID, pid);
> > +		rc = fill_pid((pid_t)pid, NULL, &stats);
> > +		if (rc < 0)
> > +			return rc;
> > +	}
> > +
> > +	if (info->attrs[TASKSTATS_CMD_ATTR_TGID]) {
> > +		u32 tgid = nla_get_u32(info->attrs[TASKSTATS_CMD_ATTR_TGID]);
> > +		NLA_PUT_U32(rep_skb, TASKSTATS_TYPE_TGID, tgid);
> > +		rc = fill_tgid((pid_t)tgid, NULL, &stats);
> > +		if (rc < 0)
> > +			return rc;
> > +	}
> > +
> 
> Should there be at least either a pid or tgid? If yes, you need to
> validate here...
>

Yes, you are correct. One of my test cases caught it too.. But I did
not want to untidy the code with if-else's which will keep growing if
the attributes change in the future. I just followed the controller
example. I will change it and validate it. Currently if the attribute
is not valid, a stat of all zero's is returned back.
 
> > +	NLA_PUT_TYPE(rep_skb, struct taskstats, TASKSTATS_TYPE_STATS, stats);
> > +	return send_reply(rep_skb, info->snd_pid, TASKSTATS_MSG_UNICAST);
> > +
> > +nla_put_failure:
> > +	return genlmsg_cancel(rep_skb, reply);
> > +
> 
> As a general comment double check your logic for errors; if you already
> have stashed something in the skb, you need to remove it etc.
> 

Wouldn't genlmsg_cancel() take care of cleaning all attributes?


> > +}
> > +
> > +
> > +/* Send pid data out on exit */
> > +void taskstats_exit_pid(struct task_struct *tsk)
> > +{
> > +	int rc;
> > +	struct sk_buff *rep_skb;
> > +	void *reply;
> > +	struct taskstats stats;
> > +
> > +	/*
> > +	 * tasks can start to exit very early. Ensure that the family
> > +	 * is registered before notifications are sent out
> > +	 */
> > +	if (!family_registered)
> > +		return;
> > +
> > +	memset(&stats, 0, sizeof(stats));
> > +	rc = prepare_reply(NULL, TASKSTATS_CMD_NEW, &rep_skb, &reply);
> > +	if (rc < 0)
> > +		return;
> > +
> > +	NLA_PUT_U32(rep_skb, TASKSTATS_TYPE_PID, (u32)tsk->pid);
> > +	rc = fill_pid(tsk->pid, tsk, &stats);
> > +	if (rc < 0)
> > +		return;
> > +
> > +	NLA_PUT_TYPE(rep_skb, struct taskstats, TASKSTATS_TYPE_STATS, stats);
> > +	rc = send_reply(rep_skb, 0, TASKSTATS_MSG_MULTICAST);
> > +
> > +	if (rc || thread_group_empty(tsk))
> > +		return;
> > +
> > +	/* Send tgid data too */
> > +	rc = prepare_reply(NULL, TASKSTATS_CMD_NEW, &rep_skb, &reply);
> > +	if (rc < 0)
> > +		return;
> > +
> 
> A single message with PID+TGID sounds reasonable. Why two messages with
> two stats? all you will need to do is get rid of the prepare_reply()
> above and NLA_PUT_U32() below (just like you do in a response to a GET.
> 

The reason for two stats is that for TGID, we return accumulated values
(of all threads in the group) and for PID we return the value just
for that pid. The return value is

pid
<stats for pid>
tgid
<accumulated stats for tgid>

> > +	NLA_PUT_U32(rep_skb, TASKSTATS_TYPE_TGID, (u32)tsk->tgid);
> > +	rc = fill_tgid(tsk->tgid, tsk, &stats);
> > +	if (rc < 0)
> > +		return;
> > +
> > +	NLA_PUT_TYPE(rep_skb, struct taskstats, TASKSTATS_TYPE_STATS, stats);
> > +	send_reply(rep_skb, 0, TASKSTATS_MSG_MULTICAST);
> > +
> > +nla_put_failure:
> > +	genlmsg_cancel(rep_skb, reply);
> > +}
> 
> 
> Other than the above comments - I believe you have it right.
> 

Thanks, I will get back to you with the updated patch soon.

> cheers,
> jamal

Thanks,
Balbir