From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1750714AbXBSJAW (ORCPT ); Mon, 19 Feb 2007 04:00:22 -0500 Received: (majordomo@vger.kernel.org) by vger.kernel.org id S1750722AbXBSJAV (ORCPT ); Mon, 19 Feb 2007 04:00:21 -0500 Received: from smtp.osdl.org ([65.172.181.24]:46081 "EHLO smtp.osdl.org" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1750714AbXBSJAU (ORCPT ); Mon, 19 Feb 2007 04:00:20 -0500 Date: Mon, 19 Feb 2007 00:57:27 -0800 From: Andrew Morton To: Balbir Singh Cc: linux-kernel@vger.kernel.org, vatsa@in.ibm.com, ckrm-tech@lists.sourceforge.net, xemul@sw.ru, linux-mm@kvack.org, menage@google.com, svaidy@linux.vnet.ibm.com, devel@openvz.org Subject: Re: [RFC][PATCH][1/4] RSS controller setup Message-Id: <20070219005727.da2acdab.akpm@linux-foundation.org> In-Reply-To: <20070219065026.3626.36882.sendpatchset@balbir-laptop> References: <20070219065019.3626.33947.sendpatchset@balbir-laptop> <20070219065026.3626.36882.sendpatchset@balbir-laptop> X-Mailer: Sylpheed version 2.2.7 (GTK+ 2.8.17; x86_64-unknown-linux-gnu) Mime-Version: 1.0 Content-Type: text/plain; charset=US-ASCII Content-Transfer-Encoding: 7bit Sender: linux-kernel-owner@vger.kernel.org X-Mailing-List: linux-kernel@vger.kernel.org On Mon, 19 Feb 2007 12:20:26 +0530 Balbir Singh 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. > ... > > + 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. 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. > + 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. > + > +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. > +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? > + 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); From mboxrd@z Thu Jan 1 00:00:00 1970 Date: Mon, 19 Feb 2007 00:57:27 -0800 From: Andrew Morton Subject: Re: [RFC][PATCH][1/4] RSS controller setup Message-Id: <20070219005727.da2acdab.akpm@linux-foundation.org> In-Reply-To: <20070219065026.3626.36882.sendpatchset@balbir-laptop> References: <20070219065019.3626.33947.sendpatchset@balbir-laptop> <20070219065026.3626.36882.sendpatchset@balbir-laptop> Mime-Version: 1.0 Content-Type: text/plain; charset=US-ASCII Content-Transfer-Encoding: 7bit Sender: owner-linux-mm@kvack.org Return-Path: To: Balbir Singh Cc: linux-kernel@vger.kernel.org, vatsa@in.ibm.com, ckrm-tech@lists.sourceforge.net, xemul@sw.ru, linux-mm@kvack.org, menage@google.com, svaidy@linux.vnet.ibm.com, devel@openvz.org List-ID: On Mon, 19 Feb 2007 12:20:26 +0530 Balbir Singh 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. > ... > > + 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. 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. > + 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. > + > +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. > +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? > + 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); -- To unsubscribe, send a message with 'unsubscribe linux-mm' in the body to majordomo@kvack.org. For more info on Linux MM, see: http://www.linux-mm.org/ . Don't email: email@kvack.org