Re: [RFC][PATCH][1/4] RSS controller setup

[Balbir Singh]

Andrew Morton wrote:
> On Mon, 19 Feb 2007 12:20:26 +0530 Balbir Singh <balbir@xxxxxxxxxx> wrote:
>
> > This patch sets up the basic controller infrastructure on top of the
> > containers infrastructure. Two files are provided for monitoring
> > and control  memctlr_usage and memctlr_limit.
>
> The patches use the identifier "memctlr" a lot.  It is hard to remember,
> and unpronounceable.  Something like memcontrol or mem_controller or
> memory_controller would be more typical.

I'll change the name to memory_controller

> > ...
> >
> > +	BUG_ON(!mem);
> > +	if ((buffer = kmalloc(nbytes + 1, GFP_KERNEL)) == 0)
> > +		return -ENOMEM;
>
> Please prefer to do
>
> 	buffer = kmalloc(nbytes + 1, GFP_KERNEL);
> 	if (buffer == NULL)
> 		reutrn -ENOMEM;
>
> ie: avoid the assign-and-test-in-the-same-statement thing.  This affects
> the whole patchset.

I'll fix that

> Also, please don't compare pointers to literal zero like that.  It makes me
> get buried it patches to convert it to "NULL".  I think this is a sparse
> thing.

Good point, I'll fix it.

> > +	buffer[nbytes] = 0;
> > +	if (copy_from_user(buffer, userbuf, nbytes)) {
> > +		ret = -EFAULT;
> > +		goto out_err;
> > +	}
> > +
> > +	container_manage_lock();
> > +	if (container_is_removed(cont)) {
> > +		ret = -ENODEV;
> > +		goto out_unlock;
> > +	}
> > +
> > +	limit = simple_strtoul(buffer, NULL, 10);
> > +	/*
> > +	 * 0 is a valid limit (unlimited resource usage)
> > +	 */
> > +	if (!limit && strcmp(buffer, "0"))
> > +		goto out_unlock;
> > +
> > +	spin_lock(&mem->lock);
> > +	mem->counter.limit = limit;
> > +	spin_unlock(&mem->lock);
>
> The patches do this a lot: a single atomic assignment with a
> pointless-looking lock/unlock around it.  It's often the case that this
> idiom indicates a bug, or needless locking.  I think the only case where it
> makes sense is when there's some other code somewhere which is doing
>
> 	spin_lock(&mem->lock);
> 	...
> 	use1(mem->counter.limit);
> 	...
> 	use2(mem->counter.limit);
> 	...
> 	spin_unlock(&mem->lock);
>
> where use1() and use2() expect the two reads of mem->counter.limit to
> return the same value.
>
> Is that the case in these patches?  If not, we might have a problem in
> there.

The next set of patches move to atomic values for the limits. That should
fix the locking.

> > +
> > +static ssize_t memctlr_read(struct container *cont, struct cftype *cft,
> > +				struct file *file, char __user *userbuf,
> > +				size_t nbytes, loff_t *ppos)
> > +{
> > +	unsigned long usage, limit;
> > +	char usagebuf[64];		/* Move away from stack later */
> > +	char *s = usagebuf;
> > +	struct memctlr *mem = memctlr_from_cont(cont);
> > +
> > +	spin_lock(&mem->lock);
> > +	usage = mem->counter.usage;
> > +	limit = mem->counter.limit;
> > +	spin_unlock(&mem->lock);
> > +
> > +	s += sprintf(s, "usage %lu, limit %ld\n", usage, limit);
> > +	return simple_read_from_buffer(userbuf, nbytes, ppos, usagebuf,
> > +					s - usagebuf);
> > +}
>
> This output is hard to parse and to extend.  I'd suggest either two
> separate files, or multi-line output:
>
> usage: %lu kB
> limit: %lu kB
>
> and what are the units of these numbers?  Page counts?  If so, please don't
> do that: it requires appplications and humans to know the current kernel's
> page size.

Yes, this looks much better. I'll move to this format. I get myself lost
in "bc" at times, that should have been a hint.

> > +static struct cftype memctlr_usage = {
> > +	.name = "memctlr_usage",
> > +	.read = memctlr_read,
> > +};
> > +
> > +static struct cftype memctlr_limit = {
> > +	.name = "memctlr_limit",
> > +	.write = memctlr_write,
> > +};
> > +
> > +static int memctlr_populate(struct container_subsys *ss,
> > +				struct container *cont)
> > +{
> > +	int rc;
> > +	if ((rc = container_add_file(cont, &memctlr_usage)) < 0)
> > +		return rc;
> > +	if ((rc = container_add_file(cont, &memctlr_limit)) < 0)
>
> Clean up the first file here?

I used cpuset_populate() as an example to code this one up.
I don't think there is an easy way in containers to clean up
files. I'll double check

> > +		return rc;
> > +	return 0;
> > +}
> > +
> > +static struct container_subsys memctlr_subsys = {
> > +	.name = "memctlr",
> > +	.create = memctlr_create,
> > +	.destroy = memctlr_destroy,
> > +	.populate = memctlr_populate,
> > +};
> > +
> > +int __init memctlr_init(void)
> > +{
> > +	int id;
> > +
> > +	id = container_register_subsys(&memctlr_subsys);
> > +	printk("Initializing memctlr version %s, id %d\n", version, id);
> > +	return id < 0 ? id : 0;
> > +}
> > +
> > +module_init(memctlr_init);


Thanks for the detailed review,

--
	Warm Regards,
	Balbir Singh
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/