Re: [PATCH -mm 2/3] i/o bandwidth controller infrastructure

[Andrea Righi]

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@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/