| Message ID | 20090611180555.f2c76568.usui@mxm.nes.nec.co.jp |
|---|---|
| State | Changes Requested, archived |
| Delegated to: | David Miller |
| Headers | show |
From: Minoru Usui <usui@mxm.nes.nec.co.jp> Date: Thu, 11 Jun 2009 18:05:55 +0900 > This patch unifies classid syntax of cls_cgroup to tc. Won't this break existing users? We really can't say "oh sorry" and change things after it's been effectively deployed already. -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Very nice, this is a big improvement. On Thu, Jun 11, 2009 at 06:05:55PM +0900, Minoru Usui wrote: > +static int get_tc_classid(u32 *classid, const char *str) Maybe make this return u32? > { > - return cgrp_cls_state(cgrp)->classid; > + u32 maj, min; > + char *p; > + > + /* check "0" for reset request */ > + if (!strcmp(str, "0")) { > + *classid = 0; > + return 0; > + } > + > + /* parse major number */ > + maj = simple_strtoul(str, &p, 16); > + if (p == str) { > + maj = 0; > + if (*p != ':') > + return -EINVAL; > + } > + > + /* parse minor number */ > + if (*p == ':') { > + if (maj >= (1<<16)) > + return -EINVAL; > + > + str = p + 1; > + min = simple_strtoul(str, &p, 16); > + if (*p != 0) > + return -EINVAL; > + if (min >= (1<<16)) > + return -EINVAL; > + } else if (*p != 0) > + return -EINVAL; What do you think about keeping things backwards compatible by accepting both the X:Y and XY format? } else if (*p != 0) return strtoul(str, NULL, 0); -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi, Thomas Thank you for your comments. On Thu, 11 Jun 2009 05:31:49 -0400 Thomas Graf <tgraf@infradead.org> wrote: > Very nice, this is a big improvement. > > On Thu, Jun 11, 2009 at 06:05:55PM +0900, Minoru Usui wrote: > > +static int get_tc_classid(u32 *classid, const char *str) > > Maybe make this return u32? Yes, you are right. I'll fix it. > > > { > > - return cgrp_cls_state(cgrp)->classid; > > + u32 maj, min; > > + char *p; > > + > > + /* check "0" for reset request */ > > + if (!strcmp(str, "0")) { > > + *classid = 0; > > + return 0; > > + } > > + > > + /* parse major number */ > > + maj = simple_strtoul(str, &p, 16); > > + if (p == str) { > > + maj = 0; > > + if (*p != ':') > > + return -EINVAL; > > + } > > + > > + /* parse minor number */ > > + if (*p == ':') { > > + if (maj >= (1<<16)) > > + return -EINVAL; > > + > > + str = p + 1; > > + min = simple_strtoul(str, &p, 16); > > + if (*p != 0) > > + return -EINVAL; > > + if (min >= (1<<16)) > > + return -EINVAL; > > + } else if (*p != 0) > > + return -EINVAL; > > What do you think about keeping things backwards compatible by > accepting both the X:Y and XY format? > > } else if (*p != 0) > return strtoul(str, NULL, 0); OK. I'll do it.
Hi, David Thank you for your comments. On Thu, 11 Jun 2009 02:13:47 -0700 (PDT) David Miller <davem@davemloft.net> wrote: > From: Minoru Usui <usui@mxm.nes.nec.co.jp> > Date: Thu, 11 Jun 2009 18:05:55 +0900 > > > This patch unifies classid syntax of cls_cgroup to tc. > > Won't this break existing users? > > We really can't say "oh sorry" and change things after it's > been effectively deployed already. It's true. If my patch applied, old syntax can't use. But I think compatibility is important, too. So I'll make a new patch which can use both old and new syntax, when we write classid to net_cls.classid file. Present implementation is an asymmetry with read and write.(read: decimal, write: hexadecimal) I think this might be interface bug, so I think we should unify to read/write syntax. Of course, if read syntax change from decimal expression, there isn't compatibility. But I think it's better to fix this problem. What do you think? If it's ok, I'd like to be able to use tc syntax(X:Y style), because cls_cgroup's purpose is select class which create by tc command. tc command can only use classid syntax X:Y style, so we should be able to use X:Y style to cls_cgroup.
From: Minoru Usui <usui@mxm.nes.nec.co.jp> Date: Fri, 12 Jun 2009 12:40:28 +0900 > Of course, if read syntax change from decimal expression, there > isn't compatibility. But I think it's better to fix this problem. > What do you think? I think compatibility is more important than symmetry. -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Thu, Jun 11, 2009 at 8:40 PM, Minoru Usui<usui@mxm.nes.nec.co.jp> wrote: > > It's true. If my patch applied, old syntax can't use. > But I think compatibility is important, too. > So I'll make a new patch which can use both old and new syntax, when we write classid to net_cls.classid file. > > Present implementation is an asymmetry with read and write.(read: decimal, write: hexadecimal) You can write the classid value in decimal too, or even octal - it just uses strtoull() to convert the string to an integer, so 0x-prefixed hex values are handled automatically. > > If it's ok, I'd like to be able to use tc syntax(X:Y style), > because cls_cgroup's purpose is select class which create by tc command. > tc command can only use classid syntax X:Y style, so we should be able to use X:Y style to cls_cgroup. We should keep it as it is - purely apart from compatibility issues, there's no need to introduce the complexity of the tc interface into the cls_cgroup kernel code. When a cgroup control file represents a simple integer, then it should be exposed as such. Paul -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi, Paul. Thank you for your advice. On Thu, 11 Jun 2009 22:06:00 -0700 Paul Menage <menage@google.com> wrote: > On Thu, Jun 11, 2009 at 8:40 PM, Minoru Usui<usui@mxm.nes.nec.co.jp> wrote: > > > > It's true. If my patch applied, old syntax can't use. > > But I think compatibility is important, too. > > So I'll make a new patch which can use both old and new syntax, when we write classid to net_cls.classid file. > > > > Present implementation is an asymmetry with read and write.(read: decimal, write: hexadecimal) > > You can write the classid value in decimal too, or even octal - it > just uses strtoull() to convert the string to an integer, so > 0x-prefixed hex values are handled automatically. > > > > > If it's ok, I'd like to be able to use tc syntax(X:Y style), > > because cls_cgroup's purpose is select class which create by tc command. > > tc command can only use classid syntax X:Y style, so we should be able to use X:Y style to cls_cgroup. > > We should keep it as it is - purely apart from compatibility issues, > there's no need to introduce the complexity of the tc interface into > the cls_cgroup kernel code. When a cgroup control file represents a > simple integer, then it should be exposed as such. I understand your opinion. You mean, kernel I/F should be simple, so we should use the same format as the kernel. I completely agree. Actually, In tc <-> kernel I/F (which uses netlink), tc sets classid to hexadecimal style not X:Y style. X:Y style is result of translating by tc command. I thought this patch was very useful at first, but it's not necessary to implementing to the kernel. This function can be implemented on user space if we need. And we can also keep compatibility of net_cls.classid I/F. :-) I drop this patch. I'm sorry for confusing a lot of people.
On Mon, Jun 15, 2009 at 12:00:35PM +0900, Minoru Usui wrote: > Actually, In tc <-> kernel I/F (which uses netlink), tc sets classid to hexadecimal style not X:Y style. > X:Y style is result of translating by tc command. > > I thought this patch was very useful at first, but it's not necessary to implementing to the kernel. > This function can be implemented on user space if we need. > And we can also keep compatibility of net_cls.classid I/F. :-) > > I drop this patch. I'm sorry for confusing a lot of people. I found this patch to be extremely useful if it wasn't to break compatibility but that issue can be resolved by accepting both formats easly. A classid is in a fact a set of two 16 bit integers and everyone is used to the X:Y notation. procfs and sysfs ought to be userfriendly to a certain degree and adding such a small parser to cls_cgroup doesn't harm anyone. -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Mon, 15 Jun 2009 06:19:19 -0400 Thomas Graf <tgraf@infradead.org> wrote: > On Mon, Jun 15, 2009 at 12:00:35PM +0900, Minoru Usui wrote: > > Actually, In tc <-> kernel I/F (which uses netlink), tc sets classid to hexadecimal style not X:Y style. > > X:Y style is result of translating by tc command. > > > > I thought this patch was very useful at first, but it's not necessary to implementing to the kernel. > > This function can be implemented on user space if we need. > > And we can also keep compatibility of net_cls.classid I/F. :-) > > > > I drop this patch. I'm sorry for confusing a lot of people. > > I found this patch to be extremely useful if it wasn't to break > compatibility but that issue can be resolved by accepting both > formats easly. Thank you for agreeing my patch. I think you said write format only. Am I right? What do you think about read format? Do you think you should keep read format as current implementation? I think we should need to unify read/write format, if we make cls_cgroup is more userfriendly. Because some of people are confused about having to read their write value as different format. (Unfortunately, current implementation is so...) But this approach breaks compatibility, so I drop this patch.
Minoru Usui wrote: > On Mon, 15 Jun 2009 06:19:19 -0400 > Thomas Graf <tgraf@infradead.org> wrote: > >> On Mon, Jun 15, 2009 at 12:00:35PM +0900, Minoru Usui wrote: >>> Actually, In tc <-> kernel I/F (which uses netlink), tc sets classid to hexadecimal style not X:Y style. >>> X:Y style is result of translating by tc command. >>> >>> I thought this patch was very useful at first, but it's not necessary to implementing to the kernel. >>> This function can be implemented on user space if we need. >>> And we can also keep compatibility of net_cls.classid I/F. :-) >>> >>> I drop this patch. I'm sorry for confusing a lot of people. >> I found this patch to be extremely useful if it wasn't to break >> compatibility but that issue can be resolved by accepting both >> formats easly. > > Thank you for agreeing my patch. > > I think you said write format only. Am I right? > What do you think about read format? > Do you think you should keep read format as current implementation? > > I think we should need to unify read/write format, if we make cls_cgroup is more userfriendly. > Because some of people are confused about having to read their write value as different format. > (Unfortunately, current implementation is so...) > > But this approach breaks compatibility, so I drop this patch. How about adding a new control file net_cls.tc_classid? -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Thu, 18 Jun 2009 10:01:27 +0800 Li Zefan <lizf@cn.fujitsu.com> wrote: > Minoru Usui wrote: > > On Mon, 15 Jun 2009 06:19:19 -0400 > > Thomas Graf <tgraf@infradead.org> wrote: > > > >> On Mon, Jun 15, 2009 at 12:00:35PM +0900, Minoru Usui wrote: > >>> Actually, In tc <-> kernel I/F (which uses netlink), tc sets classid to hexadecimal style not X:Y style. > >>> X:Y style is result of translating by tc command. > >>> > >>> I thought this patch was very useful at first, but it's not necessary to implementing to the kernel. > >>> This function can be implemented on user space if we need. > >>> And we can also keep compatibility of net_cls.classid I/F. :-) > >>> > >>> I drop this patch. I'm sorry for confusing a lot of people. > >> I found this patch to be extremely useful if it wasn't to break > >> compatibility but that issue can be resolved by accepting both > >> formats easly. > > > > Thank you for agreeing my patch. > > > > I think you said write format only. Am I right? > > What do you think about read format? > > Do you think you should keep read format as current implementation? > > > > I think we should need to unify read/write format, if we make cls_cgroup is more userfriendly. > > Because some of people are confused about having to read their write value as different format. > > (Unfortunately, current implementation is so...) > > > > But this approach breaks compatibility, so I drop this patch. > > How about adding a new control file net_cls.tc_classid? > net_cls.classid's core purpose is to read/write classid. If we add net_cls.tc_classid, its core purpose is same. Their difference is only read/write value of expression. Is this approach ok?
On Wed, Jun 17, 2009 at 11:15 PM, Minoru Usui<usui@mxm.nes.nec.co.jp> wrote: > > net_cls.classid's core purpose is to read/write classid. > If we add net_cls.tc_classid, its core purpose is same. > Their difference is only read/write value of expression. I still feel that it's unnecessary bloat to put that parsing/printing logic in the kernel when userspace can do it just as easily. But having it as a separate file would at least remove the compatibility concerns. Paul -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/net/sched/cls_cgroup.c b/net/sched/cls_cgroup.c index e5becb9..6d87d00 100644 --- a/net/sched/cls_cgroup.c +++ b/net/sched/cls_cgroup.c @@ -12,6 +12,7 @@ #include <linux/module.h> #include <linux/types.h> #include <linux/string.h> +#include <linux/ctype.h> #include <linux/errno.h> #include <linux/skbuff.h> #include <linux/cgroup.h> @@ -55,17 +56,76 @@ static void cgrp_destroy(struct cgroup_subsys *ss, struct cgroup *cgrp) kfree(cgrp_cls_state(cgrp)); } -static u64 read_classid(struct cgroup *cgrp, struct cftype *cft) +#define CLS_CGROUP_CLASSID_LEN 16 + +static ssize_t read_classid(struct cgroup *cgrp, struct cftype *cft, + struct file *file, char __user *buf, + size_t nbytes, loff_t *ppos) +{ + u32 value; + char classid[CLS_CGROUP_CLASSID_LEN]; + + value = cgrp_cls_state(cgrp)->classid; + if (value) + sprintf(classid, "%x:%x\n", + TC_H_MAJ(value)>>16, TC_H_MIN(value)); + else + strcpy(classid, "0\n"); + + return simple_read_from_buffer(buf, nbytes, ppos, + classid, strlen(classid)); +} + +static int get_tc_classid(u32 *classid, const char *str) { - return cgrp_cls_state(cgrp)->classid; + u32 maj, min; + char *p; + + /* check "0" for reset request */ + if (!strcmp(str, "0")) { + *classid = 0; + return 0; + } + + /* parse major number */ + maj = simple_strtoul(str, &p, 16); + if (p == str) { + maj = 0; + if (*p != ':') + return -EINVAL; + } + + /* parse minor number */ + if (*p == ':') { + if (maj >= (1<<16)) + return -EINVAL; + + str = p + 1; + min = simple_strtoul(str, &p, 16); + if (*p != 0) + return -EINVAL; + if (min >= (1<<16)) + return -EINVAL; + } else if (*p != 0) + return -EINVAL; + + *classid = TC_H_MAKE(maj<<16, min); + + return 0; } -static int write_classid(struct cgroup *cgrp, struct cftype *cft, u64 value) +static int write_classid(struct cgroup *cgrp, struct cftype *cft, + const char *buf) { + u32 classid; + if (!cgroup_lock_live_group(cgrp)) return -ENODEV; - cgrp_cls_state(cgrp)->classid = (u32) value; + if (get_tc_classid(&classid, buf) < 0) + return -EINVAL; + + cgrp_cls_state(cgrp)->classid = classid; cgroup_unlock(); @@ -75,8 +135,8 @@ static int write_classid(struct cgroup *cgrp, struct cftype *cft, u64 value) static struct cftype ss_files[] = { { .name = "classid", - .read_u64 = read_classid, - .write_u64 = write_classid, + .read = read_classid, + .write_string = write_classid, }, };
cls_cgroup: unify classid syntax to tc This patch unifies classid syntax of cls_cgroup to tc. In present implementation of cls_cgroup, classid read/write syntax is different from tc's one. tc 1:10 write 0x10010 (hexadecimal) read 65552 (decimal) ------------------------------------------------- # echo 0x10010 > /cgroup/net/net_cls.classid # cat /cgroup/net/net_cls.classid 65552 ------------------------------------------------- This is very inconvenient, so I made a patch which enables only tc syntax. If we don't classify with net_cls.classid, we write 0 to net_cls.classid. (This behavior is the same as present implementation.) ------------------------------------------------- # echo 1:10 > /cgroup/net/net_cls.classid # cat /cgroup/net/net_cls.classid 1:10 # echo 0 > /cgroup/net/net_cls.classid # cat /cgroup/net/net_cls.classid 0 ------------------------------------------------- Signed-off-by: Minoru Usui <usui@mxm.nes.nec.co.jp>