lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
Open Source and information security mailing list archives
| ||
|
Date: Mon, 14 Jul 2008 23:26:51 +0200 From: Andrea Righi <righi.andrea@...il.com> To: Li Zefan <lizf@...fujitsu.com> CC: Balbir Singh <balbir@...ux.vnet.ibm.com>, Paul Menage <menage@...gle.com>, akpm@...ux-foundation.org, Carl Henrik Lunde <chlunde@...g.uio.no>, axboe@...nel.dk, matt@...ehost.com, roberto@...it.it, randy.dunlap@...cle.com, Divyesh Shah <dpshah@...gle.com>, subrata@...ux.vnet.ibm.com, eric.rannaud@...il.com, containers@...ts.linux-foundation.org, linux-kernel@...r.kernel.org Subject: Re: [PATCH -mm 2/3] i/o bandwidth controller infrastructure Li Zefan wrote: >> +/* The various types of throttling algorithms */ >> +enum iothrottle_strategy { >> + IOTHROTTLE_LEAKY_BUCKET, > > It's better to explicitly assigned 0 to IOTHROTTLE_LEAKY_BUCKET. OK. > >> + IOTHROTTLE_TOKEN_BUCKET, >> +}; > >> +static int iothrottle_parse_args(char *buf, size_t nbytes, dev_t *dev, >> + u64 *iorate, >> + enum iothrottle_strategy *strategy, >> + s64 *bucket_size) >> +{ >> + char *p = buf; >> + int count = 0; >> + char *s[3]; >> + unsigned long strategy_val; >> + int ret; >> + >> + /* split the colon-delimited input string into its elements */ >> + memset(s, 0, sizeof(s)); >> + while (count < ARRAY_SIZE(s)) { >> + p = memchr(p, ':', buf + nbytes - p); >> + if (!p) >> + break; >> + *p++ = '\0'; >> + if (p >= buf + nbytes) >> + break; >> + s[count++] = p; >> + } > > use strsep() OK. > >> + >> + /* i/o bandwidth limit */ >> + if (!s[0]) >> + return -EINVAL; >> + ret = strict_strtoull(s[0], 10, iorate); >> + if (ret < 0) >> + return ret; >> + if (!*iorate) { >> + /* >> + * we're deleting a limiting rule, so just ignore the other >> + * parameters >> + */ >> + *strategy = 0; >> + *bucket_size = 0; >> + goto out; >> + } >> + *iorate = ALIGN(*iorate, 1024); >> + >> + /* throttling strategy */ >> + if (!s[1]) >> + return -EINVAL; >> + ret = strict_strtoul(s[1], 10, &strategy_val); >> + if (ret < 0) >> + return ret; >> + *strategy = (enum iothrottle_strategy)strategy_val; >> + switch (*strategy) { >> + case IOTHROTTLE_LEAKY_BUCKET: >> + /* leaky bucket ignores bucket size */ >> + *bucket_size = 0; >> + goto out; >> + case IOTHROTTLE_TOKEN_BUCKET: >> + break; >> + default: >> + return -EINVAL; >> + } >> + >> + /* bucket size */ >> + if (!s[2]) >> + return -EINVAL; >> + ret = strict_strtoll(s[2], 10, bucket_size); >> + if (ret < 0) >> + return ret; >> + if (*bucket_size < 0) >> + return -EINVAL; >> + *bucket_size = ALIGN(*bucket_size, 1024); >> +out: >> + >> + /* block device number */ >> + *dev = devname2dev_t(buf); > > why not parse dev before parse bandwidth limit ? Well... due to the filesystem lookup in devname2dev_t() this option is the most expensive to be parsed. For this reason I put it at the end, so if any other parameter is wrong we can skip the lookup_bdev(). Does it make sense? > >> + return *dev ? 0 : -EINVAL; >> +} >> + >> +static int iothrottle_write(struct cgroup *cgrp, struct cftype *cft, >> + const char *buffer) >> +{ >> + struct iothrottle *iot; >> + struct iothrottle_node *n, *newn = NULL; >> + dev_t dev; >> + u64 iorate; >> + enum iothrottle_strategy strategy; >> + s64 bucket_size; >> + char *buf; >> + size_t nbytes = strlen(buffer); >> + int ret = 0; >> + >> + buf = kmalloc(nbytes + 1, GFP_KERNEL); >> + if (!buf) >> + return -ENOMEM; >> + memcpy(buf, buffer, nbytes + 1); >> + > > redundant kmalloc, just use buffer, and ... uhmm... I would apply strsep() directly to buffer in this way, that is a const char *. > >> + ret = iothrottle_parse_args(buf, nbytes, &dev, &iorate, >> + &strategy, &bucket_size); >> + if (ret) >> + goto out1; >> + if (iorate) { >> + newn = kmalloc(sizeof(*newn), GFP_KERNEL); >> + if (!newn) { >> + ret = -ENOMEM; >> + goto out1; >> + } >> + newn->dev = dev; >> + newn->iorate = iorate; >> + newn->strategy = strategy; >> + newn->bucket_size = bucket_size; >> + newn->timestamp = jiffies; >> + atomic_long_set(&newn->stat, 0); >> + atomic_long_set(&newn->token, 0); >> + } >> + if (!cgroup_lock_live_group(cgrp)) { >> + kfree(newn); >> + ret = -ENODEV; >> + goto out1; >> + } >> + iot = cgroup_to_iothrottle(cgrp); >> + >> + spin_lock(&iot->lock); >> + if (!iorate) { >> + /* Delete a block device limiting rule */ >> + n = iothrottle_delete_node(iot, dev); >> + goto out2; >> + } >> + n = iothrottle_search_node(iot, dev); >> + if (n) { >> + /* Update a block device limiting rule */ >> + iothrottle_replace_node(iot, n, newn); >> + goto out2; >> + } >> + /* Add a new block device limiting rule */ >> + iothrottle_insert_node(iot, newn); >> +out2: >> + spin_unlock(&iot->lock); >> + cgroup_unlock(); >> + if (n) { >> + synchronize_rcu(); >> + kfree(n); >> + } >> +out1: >> + kfree(buf); >> + return ret; >> +} >> + >> +static struct cftype files[] = { >> + { >> + .name = "bandwidth", >> + .read_seq_string = iothrottle_read, >> + .write_string = iothrottle_write, > > and you should specify .max_write_len = XXX unless XXX <= 64. > You use 1024 in v4. OK. Anyway, probably something like 256 would be enough. Thanks again for the detailed revision Li! I'll include your fixes in a new patchset version. -Andrea -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@...r.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Powered by blists - more mailing lists