[PATCH net-next 1/1] net sched actions: Add support for user cookies

[PATCH net-next 1/1] net sched actions: Add support for user cookies

[Jamal Hadi Salim]

From: Jamal Hadi Salim <hadi@mojatatu.com>

Introduce optional 128-bit action cookie.
Like all other cookie schemes in the networking world (eg in protocols
like http or existing kernel fib protocol field, etc) the idea is to save
user state that when retrieved serves as a correlator. The kernel
_should not_ intepret it.  The user can store whatever they wish in the
128 bits.

Sample exercise(using two 64bit values to represent the 128 bits):

.. create an accept action with cookie 0xA:0xa0a0a0a0a0a0a0
sudo $TC actions add action ok index 1 cookie 0xA 0xa0a0a0a0a0a0a0

.. dump all gact actions..
sudo $TC -s actions ls action gact

	action order 0: gact action pass
	 random type none pass val 0
	 index 1 ref 2 bind 1 installed 1221 sec used 27 sec
 	Action statistics:
	Sent 373248 bytes 5056 pkt (dropped 0, overlimits 0 requeues 0)
	backlog 0b 0p requeues 0
	 cookie(0000000a:00000000:a0a0a0a0:00a0a0a0)

.. bind the accept action to a filter..
sudo $TC filter add dev lo parent ffff: protocol ip prio 1 \
u32 match ip dst 127.0.0.1/32 flowid 1:1 action gact index 1

... send some traffic..
$ ping 127.0.0.1 -c 3
PING 127.0.0.1 (127.0.0.1) 56(84) bytes of data.
64 bytes from 127.0.0.1: icmp_seq=1 ttl=64 time=0.020 ms
64 bytes from 127.0.0.1: icmp_seq=2 ttl=64 time=0.027 ms
64 bytes from 127.0.0.1: icmp_seq=3 ttl=64 time=0.038 ms

--- 127.0.0.1 ping statistics ---
3 packets transmitted, 3 received, 0% packet loss, time 2109ms
rtt min/avg/max/mdev = 0.020/0.028/0.038/0.008 ms 1

... show some stats
$ sudo $TC -s actions get  action gact index 1

	action order 1: gact action pass
	 random type none pass val 0
	 index 1 ref 3 bind 1 installed 2182 sec used 1 sec
 	Action statistics:
	Sent 700344 bytes 9486 pkt (dropped 0, overlimits 0 requeues 0)
	backlog 0b 0p requeues 0

Signed-off-by: Jamal Hadi Salim <jhs@mojatatu.com>
---
 include/net/act_api.h        |  7 +++++++
 include/uapi/linux/pkt_cls.h |  7 +++++++
 net/sched/act_api.c          | 27 +++++++++++++++++++++++++--
 3 files changed, 39 insertions(+), 2 deletions(-)

diff --git a/include/net/act_api.h b/include/net/act_api.h
index 1d71644..b948db9 100644
--- a/include/net/act_api.h
+++ b/include/net/act_api.h
@@ -20,6 +20,12 @@ struct tcf_hashinfo {
 
 struct tc_action_ops;
 
+union act_cookie {
+	u16 ck16[8];
+	u32 ck32[4];
+	u64 ck64[2];
+};
+
 struct tc_action {
 	const struct tc_action_ops	*ops;
 	__u32				type; /* for backward compat(TCA_OLD_COMPAT) */
@@ -41,6 +47,7 @@ struct tc_action {
 	struct rcu_head			tcfa_rcu;
 	struct gnet_stats_basic_cpu __percpu *cpu_bstats;
 	struct gnet_stats_queue __percpu *cpu_qstats;
+	union act_cookie	*ck;
 };
 #define tcf_head	common.tcfa_head
 #define tcf_index	common.tcfa_index
diff --git a/include/uapi/linux/pkt_cls.h b/include/uapi/linux/pkt_cls.h
index 1e5e1dd..6379af3 100644
--- a/include/uapi/linux/pkt_cls.h
+++ b/include/uapi/linux/pkt_cls.h
@@ -4,6 +4,12 @@
 #include <linux/types.h>
 #include <linux/pkt_sched.h>
 
+union u_act_cookie {
+	__u16 ck16[8];
+	__u32 ck32[4];
+	__u64 ck64[2];
+};
+
 /* Action attributes */
 enum {
 	TCA_ACT_UNSPEC,
@@ -12,6 +18,7 @@ enum {
 	TCA_ACT_INDEX,
 	TCA_ACT_STATS,
 	TCA_ACT_PAD,
+	TCA_ACT_COOKIE,
 	__TCA_ACT_MAX
 };
 
diff --git a/net/sched/act_api.c b/net/sched/act_api.c
index f04715a..85e77181 100644
--- a/net/sched/act_api.c
+++ b/net/sched/act_api.c
@@ -33,6 +33,7 @@ static void free_tcf(struct rcu_head *head)
 
 	free_percpu(p->cpu_bstats);
 	free_percpu(p->cpu_qstats);
+	kfree(p->ck);
 	kfree(p);
 }
 
@@ -464,8 +465,8 @@ int tcf_action_destroy(struct list_head *actions, int bind)
 	return a->ops->dump(skb, a, bind, ref);
 }
 
-int
-tcf_action_dump_1(struct sk_buff *skb, struct tc_action *a, int bind, int ref)
+int tcf_action_dump_1(struct sk_buff *skb, struct tc_action *a, int bind,
+		      int ref)
 {
 	int err = -EINVAL;
 	unsigned char *b = skb_tail_pointer(skb);
@@ -475,6 +476,12 @@ int tcf_action_destroy(struct list_head *actions, int bind)
 		goto nla_put_failure;
 	if (tcf_action_copy_stats(skb, a, 0))
 		goto nla_put_failure;
+	if (a->ck) {
+		if (nla_put(skb, TCA_ACT_COOKIE, sizeof(union act_cookie),
+			    a->ck))
+			goto nla_put_failure;
+	}
+
 	nest = nla_nest_start(skb, TCA_OPTIONS);
 	if (nest == NULL)
 		goto nla_put_failure;
@@ -575,6 +582,22 @@ struct tc_action *tcf_action_init_1(struct net *net, struct nlattr *nla,
 	if (err < 0)
 		goto err_mod;
 
+	if (tb[TCA_ACT_COOKIE]) {
+		if (nla_len(tb[TCA_ACT_COOKIE]) != sizeof(union act_cookie)) {
+			err = -EINVAL;
+			goto err_mod;
+		}
+
+		a->ck = kzalloc(sizeof(union act_cookie), GFP_KERNEL);
+		if (unlikely(!a->ck)) {
+			err = -ENOMEM;
+			goto err_mod;
+		}
+
+		memcpy((void *)a->ck, nla_data(tb[TCA_ACT_COOKIE]),
+		       sizeof(union act_cookie));
+	}
+
 	/* module count goes up only when brand new policy is created
 	 * if it exists and is only bound to in a_o->init() then
 	 * ACT_P_CREATED is not returned (a zero is).
-- 
1.9.1

Re: [PATCH net-next 1/1] net sched actions: Add support for user cookies

[Jiri Pirko]

Sat, Jan 14, 2017 at 03:59:18PM CET, jhs@mojatatu.com wrote:
>From: Jamal Hadi Salim <hadi@mojatatu.com>
>
>Introduce optional 128-bit action cookie.
>Like all other cookie schemes in the networking world (eg in protocols
>like http or existing kernel fib protocol field, etc) the idea is to save
>user state that when retrieved serves as a correlator. The kernel
>_should not_ intepret it.  The user can store whatever they wish in the
>128 bits.
>
>Sample exercise(using two 64bit values to represent the 128 bits):
>
>.. create an accept action with cookie 0xA:0xa0a0a0a0a0a0a0
>sudo $TC actions add action ok index 1 cookie 0xA 0xa0a0a0a0a0a0a0

2x 64bit values? Why can't this have variable length, according to what
user needs:

sudo $TC actions add action ok index 1 cookie a0
sudo $TC actions add action ok index 1 cookie a01122
sudo $TC actions add action ok index 1 cookie a01122334455
sudo $TC actions add action ok index 1 cookie a01122334455aabbccddeeff


>
>.. dump all gact actions..
>sudo $TC -s actions ls action gact
>
>	action order 0: gact action pass
>	 random type none pass val 0
>	 index 1 ref 2 bind 1 installed 1221 sec used 27 sec
> 	Action statistics:
>	Sent 373248 bytes 5056 pkt (dropped 0, overlimits 0 requeues 0)
>	backlog 0b 0p requeues 0
>	 cookie(0000000a:00000000:a0a0a0a0:00a0a0a0)

Input is 2x64 and dump is 4x32? That is confusing. With my suggested
example, this would be:

	 cookie a0
	 cookie a01122
	 cookie a01122334455
	 cookie a01122334455aabbccddeeff


>
>.. bind the accept action to a filter..
>sudo $TC filter add dev lo parent ffff: protocol ip prio 1 \
>u32 match ip dst 127.0.0.1/32 flowid 1:1 action gact index 1
>
>... send some traffic..
>$ ping 127.0.0.1 -c 3
>PING 127.0.0.1 (127.0.0.1) 56(84) bytes of data.
>64 bytes from 127.0.0.1: icmp_seq=1 ttl=64 time=0.020 ms
>64 bytes from 127.0.0.1: icmp_seq=2 ttl=64 time=0.027 ms
>64 bytes from 127.0.0.1: icmp_seq=3 ttl=64 time=0.038 ms
>
>--- 127.0.0.1 ping statistics ---
>3 packets transmitted, 3 received, 0% packet loss, time 2109ms
>rtt min/avg/max/mdev = 0.020/0.028/0.038/0.008 ms 1
>
>... show some stats
>$ sudo $TC -s actions get  action gact index 1
>
>	action order 1: gact action pass
>	 random type none pass val 0
>	 index 1 ref 3 bind 1 installed 2182 sec used 1 sec
> 	Action statistics:
>	Sent 700344 bytes 9486 pkt (dropped 0, overlimits 0 requeues 0)
>	backlog 0b 0p requeues 0
>
>Signed-off-by: Jamal Hadi Salim <jhs@mojatatu.com>
>---
> include/net/act_api.h        |  7 +++++++
> include/uapi/linux/pkt_cls.h |  7 +++++++
> net/sched/act_api.c          | 27 +++++++++++++++++++++++++--
> 3 files changed, 39 insertions(+), 2 deletions(-)
>
>diff --git a/include/net/act_api.h b/include/net/act_api.h
>index 1d71644..b948db9 100644
>--- a/include/net/act_api.h
>+++ b/include/net/act_api.h
>@@ -20,6 +20,12 @@ struct tcf_hashinfo {
> 
> struct tc_action_ops;
> 
>+union act_cookie {
>+	u16 ck16[8];
>+	u32 ck32[4];
>+	u64 ck64[2];

Since this should be never interpreted by kernel, I don't understand why
this union is needed. Why just don't pass a char array?

Also, whatever format this is, could we make is shared with cls cookie?


>+};
>+
> struct tc_action {
> 	const struct tc_action_ops	*ops;
> 	__u32				type; /* for backward compat(TCA_OLD_COMPAT) */
>@@ -41,6 +47,7 @@ struct tc_action {
> 	struct rcu_head			tcfa_rcu;
> 	struct gnet_stats_basic_cpu __percpu *cpu_bstats;
> 	struct gnet_stats_queue __percpu *cpu_qstats;
>+	union act_cookie	*ck;
> };
> #define tcf_head	common.tcfa_head
> #define tcf_index	common.tcfa_index
>diff --git a/include/uapi/linux/pkt_cls.h b/include/uapi/linux/pkt_cls.h
>index 1e5e1dd..6379af3 100644
>--- a/include/uapi/linux/pkt_cls.h
>+++ b/include/uapi/linux/pkt_cls.h
>@@ -4,6 +4,12 @@
> #include <linux/types.h>
> #include <linux/pkt_sched.h>
> 
>+union u_act_cookie {
>+	__u16 ck16[8];
>+	__u32 ck32[4];
>+	__u64 ck64[2];
>+};

Again, the same struct? I don't understand why twice.


>+
> /* Action attributes */
> enum {
> 	TCA_ACT_UNSPEC,
>@@ -12,6 +18,7 @@ enum {
> 	TCA_ACT_INDEX,
> 	TCA_ACT_STATS,
> 	TCA_ACT_PAD,
>+	TCA_ACT_COOKIE,
> 	__TCA_ACT_MAX
> };
> 
>diff --git a/net/sched/act_api.c b/net/sched/act_api.c
>index f04715a..85e77181 100644
>--- a/net/sched/act_api.c
>+++ b/net/sched/act_api.c
>@@ -33,6 +33,7 @@ static void free_tcf(struct rcu_head *head)
> 
> 	free_percpu(p->cpu_bstats);
> 	free_percpu(p->cpu_qstats);
>+	kfree(p->ck);
> 	kfree(p);
> }
> 
>@@ -464,8 +465,8 @@ int tcf_action_destroy(struct list_head *actions, int bind)
> 	return a->ops->dump(skb, a, bind, ref);
> }
> 
>-int
>-tcf_action_dump_1(struct sk_buff *skb, struct tc_action *a, int bind, int ref)
>+int tcf_action_dump_1(struct sk_buff *skb, struct tc_action *a, int bind,
>+		      int ref)
> {
> 	int err = -EINVAL;
> 	unsigned char *b = skb_tail_pointer(skb);
>@@ -475,6 +476,12 @@ int tcf_action_destroy(struct list_head *actions, int bind)
> 		goto nla_put_failure;
> 	if (tcf_action_copy_stats(skb, a, 0))
> 		goto nla_put_failure;
>+	if (a->ck) {
>+		if (nla_put(skb, TCA_ACT_COOKIE, sizeof(union act_cookie),
>+			    a->ck))
>+			goto nla_put_failure;
>+	}
>+
> 	nest = nla_nest_start(skb, TCA_OPTIONS);
> 	if (nest == NULL)
> 		goto nla_put_failure;
>@@ -575,6 +582,22 @@ struct tc_action *tcf_action_init_1(struct net *net, struct nlattr *nla,
> 	if (err < 0)
> 		goto err_mod;
> 
>+	if (tb[TCA_ACT_COOKIE]) {
>+		if (nla_len(tb[TCA_ACT_COOKIE]) != sizeof(union act_cookie)) {
>+			err = -EINVAL;
>+			goto err_mod;
>+		}
>+
>+		a->ck = kzalloc(sizeof(union act_cookie), GFP_KERNEL);
>+		if (unlikely(!a->ck)) {
>+			err = -ENOMEM;
>+			goto err_mod;
>+		}
>+
>+		memcpy((void *)a->ck, nla_data(tb[TCA_ACT_COOKIE]),
>+		       sizeof(union act_cookie));
>+	}
>+
> 	/* module count goes up only when brand new policy is created
> 	 * if it exists and is only bound to in a_o->init() then
> 	 * ACT_P_CREATED is not returned (a zero is).
>-- 
>1.9.1
>

Re: [PATCH net-next 1/1] net sched actions: Add support for user cookies

[Jamal Hadi Salim]

On 17-01-14 10:22 AM, Jiri Pirko wrote:

>> .. create an accept action with cookie 0xA:0xa0a0a0a0a0a0a0
>> sudo $TC actions add action ok index 1 cookie 0xA 0xa0a0a0a0a0a0a0
>
> 2x 64bit values? Why can't this have variable length, according to what
> user needs:


You can intepret it however you wish. It is 128 bits. You can make it
2x64, 4x32, 8x16, 16x8

>
> sudo $TC actions add action ok index 1 cookie a0
> sudo $TC actions add action ok index 1 cookie a01122
> sudo $TC actions add action ok index 1 cookie a01122334455
> sudo $TC actions add action ok index 1 cookie a01122334455aabbccddeeff
>

Sure you can do that too..
I will add add 16 8b fields to the union.


>>
>> .. dump all gact actions..
>> sudo $TC -s actions ls action gact
>>
>> 	action order 0: gact action pass
>> 	 random type none pass val 0
>> 	 index 1 ref 2 bind 1 installed 1221 sec used 27 sec
>> 	Action statistics:
>> 	Sent 373248 bytes 5056 pkt (dropped 0, overlimits 0 requeues 0)
>> 	backlog 0b 0p requeues 0
>> 	 cookie(0000000a:00000000:a0a0a0a0:00a0a0a0)
>
> Input is 2x64 and dump is 4x32? That is confusing. With my suggested
> example, this would be:
>
> 	 cookie a0
> 	 cookie a01122
> 	 cookie a01122334455
> 	 cookie a01122334455aabbccddeeff
>

Your suggestion is more sensible for a user space cli tool like tc.
I will add a uchar cku8[16] field and make changes to iproute2.

>> struct tc_action_ops;
>>
>> +union act_cookie {
>> +	u16 ck16[8];
>> +	u32 ck32[4];
>> +	u64 ck64[2];
>
> Since this should be never interpreted by kernel, I don't understand why
> this union is needed. Why just don't pass a char array?
>

programmatic usability.

> Also, whatever format this is, could we make is shared with cls cookie?
>
>

The structure could be shared (and because it is in pkt_cls.h
that makes it easier). But the TLVs are domain specific. We need another
one for classifiers.

>> +};
>> +
>> struct tc_action {
>> 	const struct tc_action_ops	*ops;
>> 	__u32				type; /* for backward compat(TCA_OLD_COMPAT) */
>> @@ -41,6 +47,7 @@ struct tc_action {
>> 	struct rcu_head			tcfa_rcu;
>> 	struct gnet_stats_basic_cpu __percpu *cpu_bstats;
>> 	struct gnet_stats_queue __percpu *cpu_qstats;
>> +	union act_cookie	*ck;
>> };
>> #define tcf_head	common.tcfa_head
>> #define tcf_index	common.tcfa_index
>> diff --git a/include/uapi/linux/pkt_cls.h b/include/uapi/linux/pkt_cls.h
>> index 1e5e1dd..6379af3 100644
>> --- a/include/uapi/linux/pkt_cls.h
>> +++ b/include/uapi/linux/pkt_cls.h
>> @@ -4,6 +4,12 @@
>> #include <linux/types.h>
>> #include <linux/pkt_sched.h>
>>
>> +union u_act_cookie {
>> +	__u16 ck16[8];
>> +	__u32 ck32[4];
>> +	__u64 ck64[2];
>> +};
>
> Again, the same struct? I don't understand why twice.

Just old habits.
user vs kernel api? Standard action approach one says
__u32 other says u32; hanging off the user variant to kernel
didnt feel right.

cheers,
jamal

Re: [PATCH net-next 1/1] net sched actions: Add support for user cookies

[Jiri Pirko]

Sat, Jan 14, 2017 at 04:39:24PM CET, jhs@mojatatu.com wrote:
>On 17-01-14 10:22 AM, Jiri Pirko wrote:
>
>> > .. create an accept action with cookie 0xA:0xa0a0a0a0a0a0a0
>> > sudo $TC actions add action ok index 1 cookie 0xA 0xa0a0a0a0a0a0a0
>> 
>> 2x 64bit values? Why can't this have variable length, according to what
>> user needs:
>
>
>You can intepret it however you wish. It is 128 bits. You can make it
>2x64, 4x32, 8x16, 16x8
>
>> 
>> sudo $TC actions add action ok index 1 cookie a0
>> sudo $TC actions add action ok index 1 cookie a01122
>> sudo $TC actions add action ok index 1 cookie a01122334455
>> sudo $TC actions add action ok index 1 cookie a01122334455aabbccddeeff
>> 
>
>Sure you can do that too..
>I will add add 16 8b fields to the union.
>
>
>> > 
>> > .. dump all gact actions..
>> > sudo $TC -s actions ls action gact
>> > 
>> > 	action order 0: gact action pass
>> > 	 random type none pass val 0
>> > 	 index 1 ref 2 bind 1 installed 1221 sec used 27 sec
>> > 	Action statistics:
>> > 	Sent 373248 bytes 5056 pkt (dropped 0, overlimits 0 requeues 0)
>> > 	backlog 0b 0p requeues 0
>> > 	 cookie(0000000a:00000000:a0a0a0a0:00a0a0a0)
>> 
>> Input is 2x64 and dump is 4x32? That is confusing. With my suggested
>> example, this would be:
>> 
>> 	 cookie a0
>> 	 cookie a01122
>> 	 cookie a01122334455
>> 	 cookie a01122334455aabbccddeeff
>> 
>
>Your suggestion is more sensible for a user space cli tool like tc.
>I will add a uchar cku8[16] field and make changes to iproute2.
>
>> > struct tc_action_ops;
>> > 
>> > +union act_cookie {
>> > +	u16 ck16[8];
>> > +	u32 ck32[4];
>> > +	u64 ck64[2];
>> 
>> Since this should be never interpreted by kernel, I don't understand why
>> this union is needed. Why just don't pass a char array?
>> 
>
>programmatic usability.

I don't see why. In userspace you can map whatever struct you need to the
mem with chararray. It's totally up to you as an app developer. There is
no need to make that part of kernel api. Really.


>
>> Also, whatever format this is, could we make is shared with cls cookie?
>> 
>> 
>
>The structure could be shared (and because it is in pkt_cls.h
>that makes it easier). But the TLVs are domain specific. We need another
>one for classifiers.
>
>> > +};
>> > +
>> > struct tc_action {
>> > 	const struct tc_action_ops	*ops;
>> > 	__u32				type; /* for backward compat(TCA_OLD_COMPAT) */
>> > @@ -41,6 +47,7 @@ struct tc_action {
>> > 	struct rcu_head			tcfa_rcu;
>> > 	struct gnet_stats_basic_cpu __percpu *cpu_bstats;
>> > 	struct gnet_stats_queue __percpu *cpu_qstats;
>> > +	union act_cookie	*ck;
>> > };
>> > #define tcf_head	common.tcfa_head
>> > #define tcf_index	common.tcfa_index
>> > diff --git a/include/uapi/linux/pkt_cls.h b/include/uapi/linux/pkt_cls.h
>> > index 1e5e1dd..6379af3 100644
>> > --- a/include/uapi/linux/pkt_cls.h
>> > +++ b/include/uapi/linux/pkt_cls.h
>> > @@ -4,6 +4,12 @@
>> > #include <linux/types.h>
>> > #include <linux/pkt_sched.h>
>> > 
>> > +union u_act_cookie {
>> > +	__u16 ck16[8];
>> > +	__u32 ck32[4];
>> > +	__u64 ck64[2];
>> > +};
>> 
>> Again, the same struct? I don't understand why twice.
>
>Just old habits.
>user vs kernel api? Standard action approach one says
>__u32 other says u32; hanging off the user variant to kernel
>didnt feel right.

Just have it in uapi and use it from within kernel. But did you see what
I suggested in the other thread (regarding IFLA_PHYS_PORT_ID and
IFLA_PHYS_SWITCH_ID)? If you do it what way, you don't need no struct.

Re: [PATCH net-next 1/1] net sched actions: Add support for user cookies

[John Fastabend]

On 17-01-14 07:49 AM, Jiri Pirko wrote:
> Sat, Jan 14, 2017 at 04:39:24PM CET, jhs@mojatatu.com wrote:
>> On 17-01-14 10:22 AM, Jiri Pirko wrote:
>>
>>>> .. create an accept action with cookie 0xA:0xa0a0a0a0a0a0a0
>>>> sudo $TC actions add action ok index 1 cookie 0xA 0xa0a0a0a0a0a0a0
>>>
>>> 2x 64bit values? Why can't this have variable length, according to what
>>> user needs:
>>
>>
>> You can intepret it however you wish. It is 128 bits. You can make it
>> 2x64, 4x32, 8x16, 16x8
>>
>>>
>>> sudo $TC actions add action ok index 1 cookie a0
>>> sudo $TC actions add action ok index 1 cookie a01122
>>> sudo $TC actions add action ok index 1 cookie a01122334455
>>> sudo $TC actions add action ok index 1 cookie a01122334455aabbccddeeff
>>>
>>
>> Sure you can do that too..
>> I will add add 16 8b fields to the union.
>>
>>
>>>>
>>>> .. dump all gact actions..
>>>> sudo $TC -s actions ls action gact
>>>>
>>>> 	action order 0: gact action pass
>>>> 	 random type none pass val 0
>>>> 	 index 1 ref 2 bind 1 installed 1221 sec used 27 sec
>>>> 	Action statistics:
>>>> 	Sent 373248 bytes 5056 pkt (dropped 0, overlimits 0 requeues 0)
>>>> 	backlog 0b 0p requeues 0
>>>> 	 cookie(0000000a:00000000:a0a0a0a0:00a0a0a0)
>>>
>>> Input is 2x64 and dump is 4x32? That is confusing. With my suggested
>>> example, this would be:
>>>
>>> 	 cookie a0
>>> 	 cookie a01122
>>> 	 cookie a01122334455
>>> 	 cookie a01122334455aabbccddeeff
>>>
>>
>> Your suggestion is more sensible for a user space cli tool like tc.
>> I will add a uchar cku8[16] field and make changes to iproute2.
>>
>>>> struct tc_action_ops;
>>>>
>>>> +union act_cookie {
>>>> +	u16 ck16[8];
>>>> +	u32 ck32[4];
>>>> +	u64 ck64[2];
>>>
>>> Since this should be never interpreted by kernel, I don't understand why
>>> this union is needed. Why just don't pass a char array?
>>>
>>
>> programmatic usability.
> 
> I don't see why. In userspace you can map whatever struct you need to the
> mem with chararray. It's totally up to you as an app developer. There is
> no need to make that part of kernel api. Really.
> 
> 
>>
>>> Also, whatever format this is, could we make is shared with cls cookie?
>>>
>>>
>>
>> The structure could be shared (and because it is in pkt_cls.h
>> that makes it easier). But the TLVs are domain specific. We need another
>> one for classifiers.
>>
>>>> +};
>>>> +
>>>> struct tc_action {
>>>> 	const struct tc_action_ops	*ops;
>>>> 	__u32				type; /* for backward compat(TCA_OLD_COMPAT) */
>>>> @@ -41,6 +47,7 @@ struct tc_action {
>>>> 	struct rcu_head			tcfa_rcu;
>>>> 	struct gnet_stats_basic_cpu __percpu *cpu_bstats;
>>>> 	struct gnet_stats_queue __percpu *cpu_qstats;
>>>> +	union act_cookie	*ck;
>>>> };
>>>> #define tcf_head	common.tcfa_head
>>>> #define tcf_index	common.tcfa_index
>>>> diff --git a/include/uapi/linux/pkt_cls.h b/include/uapi/linux/pkt_cls.h
>>>> index 1e5e1dd..6379af3 100644
>>>> --- a/include/uapi/linux/pkt_cls.h
>>>> +++ b/include/uapi/linux/pkt_cls.h
>>>> @@ -4,6 +4,12 @@
>>>> #include <linux/types.h>
>>>> #include <linux/pkt_sched.h>
>>>>
>>>> +union u_act_cookie {
>>>> +	__u16 ck16[8];
>>>> +	__u32 ck32[4];
>>>> +	__u64 ck64[2];
>>>> +};
>>>
>>> Again, the same struct? I don't understand why twice.
>>
>> Just old habits.
>> user vs kernel api? Standard action approach one says
>> __u32 other says u32; hanging off the user variant to kernel
>> didnt feel right.
> 
> Just have it in uapi and use it from within kernel. But did you see what
> I suggested in the other thread (regarding IFLA_PHYS_PORT_ID and
> IFLA_PHYS_SWITCH_ID)? If you do it what way, you don't need no struct.
> 

+1 on using something like MAX_PHYS_ITEM_ID_LEN seems much nicer to me
and easy to extend later as Jiri notes.

[PATCH net-next 1/1] net sched actions: Add support for user cookies

[Jamal Hadi Salim]

From: Jamal Hadi Salim <jhs@mojatatu.com>

Introduce optional 128-bit action cookie.
Like all other cookie schemes in the networking world (eg in protocols
like http or existing kernel fib protocol field, etc) the idea is to save
user state that when retrieved serves as a correlator. The kernel
_should not_ intepret it.  The user can store whatever they wish in the
128 bits.

Sample exercise(using two 64bit values to represent the 128 bits):

.. create an accept action with cookie a1b2c3d4
sudo $TC actions add action ok index 1 cookie a1b2c3d4

.. dump all gact actions..
sudo $TC -s actions ls action gact

	action order 0: gact action pass
	 random type none pass val 0
	 index 1 ref 1 bind 0 installed 5 sec used 5 sec
 	Action statistics:
	Sent 0 bytes 0 pkt (dropped 0, overlimits 0 requeues 0)
	backlog 0b 0p requeues 0
	cookie a1b2c3d4

.. bind the accept action to a filter..
sudo $TC filter add dev lo parent ffff: protocol ip prio 1 \
u32 match ip dst 127.0.0.1/32 flowid 1:1 action gact index 1

... send some traffic..
$ ping 127.0.0.1 -c 3
PING 127.0.0.1 (127.0.0.1) 56(84) bytes of data.
64 bytes from 127.0.0.1: icmp_seq=1 ttl=64 time=0.020 ms
64 bytes from 127.0.0.1: icmp_seq=2 ttl=64 time=0.027 ms
64 bytes from 127.0.0.1: icmp_seq=3 ttl=64 time=0.038 ms

--- 127.0.0.1 ping statistics ---
3 packets transmitted, 3 received, 0% packet loss, time 2109ms
rtt min/avg/max/mdev = 0.020/0.028/0.038/0.008 ms 1

... show some stats
$ sudo $TC -s actions get action gact index 1

	action order 1: gact action pass
	 random type none pass val 0
	 index 1 ref 2 bind 1 installed 204 sec used 5 sec
 	Action statistics:
        Sent 12168 bytes 164 pkt (dropped 0, overlimits 0 requeues 0)
	backlog 0b 0p requeues 0
	cookie a1b2c3d4

.. try longer cookie...
$ sudo $TC actions replace action ok index 1 cookie 1234567890abcdef
.. dump..
$ sudo $TC -s actions ls action gact

	action order 1: gact action pass
	 random type none pass val 0
	 index 1 ref 2 bind 1 installed 204 sec used 5 sec
 	Action statistics:
        Sent 12168 bytes 164 pkt (dropped 0, overlimits 0 requeues 0)
	backlog 0b 0p requeues 0
	cookie 1234567890abcdef

Signed-off-by: Jamal Hadi Salim <jhs@mojatatu.com>
---
 include/net/act_api.h        |  1 +
 include/uapi/linux/pkt_cls.h | 11 +++++++++++
 net/sched/act_api.c          | 28 ++++++++++++++++++++++++++--
 3 files changed, 38 insertions(+), 2 deletions(-)

diff --git a/include/net/act_api.h b/include/net/act_api.h
index 1d71644..0692458 100644
--- a/include/net/act_api.h
+++ b/include/net/act_api.h
@@ -41,6 +41,7 @@ struct tc_action {
 	struct rcu_head			tcfa_rcu;
 	struct gnet_stats_basic_cpu __percpu *cpu_bstats;
 	struct gnet_stats_queue __percpu *cpu_qstats;
+	struct tc_cookie	*act_ck;
 };
 #define tcf_head	common.tcfa_head
 #define tcf_index	common.tcfa_index
diff --git a/include/uapi/linux/pkt_cls.h b/include/uapi/linux/pkt_cls.h
index 1e5e1dd..063bc89 100644
--- a/include/uapi/linux/pkt_cls.h
+++ b/include/uapi/linux/pkt_cls.h
@@ -4,6 +4,16 @@
 #include <linux/types.h>
 #include <linux/pkt_sched.h>
 
+#define MAX_TC_COOKIE_SZ 16
+
+/* This structure holds cookie structure that is passed from user
+ * to the kernel for actions and classifiers
+ */
+struct tc_cookie {
+	unsigned char ck[MAX_TC_COOKIE_SZ];
+	unsigned char ck_len;
+};
+
 /* Action attributes */
 enum {
 	TCA_ACT_UNSPEC,
@@ -12,6 +22,7 @@ enum {
 	TCA_ACT_INDEX,
 	TCA_ACT_STATS,
 	TCA_ACT_PAD,
+	TCA_ACT_COOKIE,
 	__TCA_ACT_MAX
 };
 
diff --git a/net/sched/act_api.c b/net/sched/act_api.c
index f04715a..b82908a 100644
--- a/net/sched/act_api.c
+++ b/net/sched/act_api.c
@@ -33,6 +33,7 @@ static void free_tcf(struct rcu_head *head)
 
 	free_percpu(p->cpu_bstats);
 	free_percpu(p->cpu_qstats);
+	kfree(p->act_ck);
 	kfree(p);
 }
 
@@ -464,8 +465,8 @@ int tcf_action_destroy(struct list_head *actions, int bind)
 	return a->ops->dump(skb, a, bind, ref);
 }
 
-int
-tcf_action_dump_1(struct sk_buff *skb, struct tc_action *a, int bind, int ref)
+int tcf_action_dump_1(struct sk_buff *skb, struct tc_action *a, int bind,
+		      int ref)
 {
 	int err = -EINVAL;
 	unsigned char *b = skb_tail_pointer(skb);
@@ -475,6 +476,12 @@ int tcf_action_destroy(struct list_head *actions, int bind)
 		goto nla_put_failure;
 	if (tcf_action_copy_stats(skb, a, 0))
 		goto nla_put_failure;
+	if (a->act_ck) {
+		if (nla_put(skb, TCA_ACT_COOKIE, a->act_ck->ck_len,
+			    a->act_ck))
+			goto nla_put_failure;
+	}
+
 	nest = nla_nest_start(skb, TCA_OPTIONS);
 	if (nest == NULL)
 		goto nla_put_failure;
@@ -575,6 +582,23 @@ struct tc_action *tcf_action_init_1(struct net *net, struct nlattr *nla,
 	if (err < 0)
 		goto err_mod;
 
+	if (tb[TCA_ACT_COOKIE]) {
+		if (nla_len(tb[TCA_ACT_COOKIE]) > MAX_TC_COOKIE_SZ) {
+			err = -EINVAL;
+			goto err_mod;
+		}
+
+		a->act_ck = kzalloc(sizeof(*a->act_ck), GFP_KERNEL);
+		if (unlikely(!a->act_ck)) {
+			err = -ENOMEM;
+			goto err_mod;
+		}
+
+		memcpy((void *)a->act_ck->ck, nla_data(tb[TCA_ACT_COOKIE]),
+		       nla_len(tb[TCA_ACT_COOKIE]));
+		a->act_ck->ck_len = nla_len(tb[TCA_ACT_COOKIE]);
+	}
+
 	/* module count goes up only when brand new policy is created
 	 * if it exists and is only bound to in a_o->init() then
 	 * ACT_P_CREATED is not returned (a zero is).
-- 
1.9.1

Re: [PATCH net-next 1/1] net sched actions: Add support for user cookies

[Jiri Pirko]

Subject should contain "V2"

Sat, Jan 14, 2017 at 10:52:44PM CET, jhs@mojatatu.com wrote:
>From: Jamal Hadi Salim <jhs@mojatatu.com>
>
>Introduce optional 128-bit action cookie.
>Like all other cookie schemes in the networking world (eg in protocols
>like http or existing kernel fib protocol field, etc) the idea is to save
>user state that when retrieved serves as a correlator. The kernel
>_should not_ intepret it.  The user can store whatever they wish in the
>128 bits.
>
>Sample exercise(using two 64bit values to represent the 128 bits):

Looks like you did not update the description.

>
>.. create an accept action with cookie a1b2c3d4
>sudo $TC actions add action ok index 1 cookie a1b2c3d4
>
>.. dump all gact actions..
>sudo $TC -s actions ls action gact
>
>	action order 0: gact action pass
>	 random type none pass val 0
>	 index 1 ref 1 bind 0 installed 5 sec used 5 sec
> 	Action statistics:
>	Sent 0 bytes 0 pkt (dropped 0, overlimits 0 requeues 0)
>	backlog 0b 0p requeues 0
>	cookie a1b2c3d4
>
>.. bind the accept action to a filter..
>sudo $TC filter add dev lo parent ffff: protocol ip prio 1 \
>u32 match ip dst 127.0.0.1/32 flowid 1:1 action gact index 1
>
>... send some traffic..
>$ ping 127.0.0.1 -c 3
>PING 127.0.0.1 (127.0.0.1) 56(84) bytes of data.
>64 bytes from 127.0.0.1: icmp_seq=1 ttl=64 time=0.020 ms
>64 bytes from 127.0.0.1: icmp_seq=2 ttl=64 time=0.027 ms
>64 bytes from 127.0.0.1: icmp_seq=3 ttl=64 time=0.038 ms
>
>--- 127.0.0.1 ping statistics ---
>3 packets transmitted, 3 received, 0% packet loss, time 2109ms
>rtt min/avg/max/mdev = 0.020/0.028/0.038/0.008 ms 1
>
>... show some stats
>$ sudo $TC -s actions get action gact index 1
>
>	action order 1: gact action pass
>	 random type none pass val 0
>	 index 1 ref 2 bind 1 installed 204 sec used 5 sec
> 	Action statistics:
>        Sent 12168 bytes 164 pkt (dropped 0, overlimits 0 requeues 0)
>	backlog 0b 0p requeues 0
>	cookie a1b2c3d4
>
>.. try longer cookie...
>$ sudo $TC actions replace action ok index 1 cookie 1234567890abcdef
>.. dump..
>$ sudo $TC -s actions ls action gact
>
>	action order 1: gact action pass
>	 random type none pass val 0
>	 index 1 ref 2 bind 1 installed 204 sec used 5 sec
> 	Action statistics:
>        Sent 12168 bytes 164 pkt (dropped 0, overlimits 0 requeues 0)
>	backlog 0b 0p requeues 0
>	cookie 1234567890abcdef
>
>Signed-off-by: Jamal Hadi Salim <jhs@mojatatu.com>

V1->V2 changelog would ne nice.


>---
> include/net/act_api.h        |  1 +
> include/uapi/linux/pkt_cls.h | 11 +++++++++++
> net/sched/act_api.c          | 28 ++++++++++++++++++++++++++--
> 3 files changed, 38 insertions(+), 2 deletions(-)
>
>diff --git a/include/net/act_api.h b/include/net/act_api.h
>index 1d71644..0692458 100644
>--- a/include/net/act_api.h
>+++ b/include/net/act_api.h
>@@ -41,6 +41,7 @@ struct tc_action {
> 	struct rcu_head			tcfa_rcu;
> 	struct gnet_stats_basic_cpu __percpu *cpu_bstats;
> 	struct gnet_stats_queue __percpu *cpu_qstats;
>+	struct tc_cookie	*act_ck;

I wonder if we just can't do:
struct tc_cookie act_ck;
You would safe kzalloc. I don't have strong opinion though..


> };
> #define tcf_head	common.tcfa_head
> #define tcf_index	common.tcfa_index
>diff --git a/include/uapi/linux/pkt_cls.h b/include/uapi/linux/pkt_cls.h
>index 1e5e1dd..063bc89 100644
>--- a/include/uapi/linux/pkt_cls.h
>+++ b/include/uapi/linux/pkt_cls.h
>@@ -4,6 +4,16 @@
> #include <linux/types.h>
> #include <linux/pkt_sched.h>
> 
>+#define MAX_TC_COOKIE_SZ 16

I like to have some "namespace" prefix for user api.
"TC_COOKIE_MAX_SIZE" perhaps?

>+
>+/* This structure holds cookie structure that is passed from user
>+ * to the kernel for actions and classifiers
>+ */
>+struct tc_cookie {
>+	unsigned char ck[MAX_TC_COOKIE_SZ];
>+	unsigned char ck_len;

This struct should certainly not be in UAPI header.


>+};
>+
> /* Action attributes */
> enum {
> 	TCA_ACT_UNSPEC,
>@@ -12,6 +22,7 @@ enum {
> 	TCA_ACT_INDEX,
> 	TCA_ACT_STATS,
> 	TCA_ACT_PAD,
>+	TCA_ACT_COOKIE,
> 	__TCA_ACT_MAX
> };
> 
>diff --git a/net/sched/act_api.c b/net/sched/act_api.c
>index f04715a..b82908a 100644
>--- a/net/sched/act_api.c
>+++ b/net/sched/act_api.c
>@@ -33,6 +33,7 @@ static void free_tcf(struct rcu_head *head)
> 
> 	free_percpu(p->cpu_bstats);
> 	free_percpu(p->cpu_qstats);
>+	kfree(p->act_ck);
> 	kfree(p);
> }
> 
>@@ -464,8 +465,8 @@ int tcf_action_destroy(struct list_head *actions, int bind)
> 	return a->ops->dump(skb, a, bind, ref);
> }
> 
>-int
>-tcf_action_dump_1(struct sk_buff *skb, struct tc_action *a, int bind, int ref)
>+int tcf_action_dump_1(struct sk_buff *skb, struct tc_action *a, int bind,
>+		      int ref)
> {
> 	int err = -EINVAL;
> 	unsigned char *b = skb_tail_pointer(skb);
>@@ -475,6 +476,12 @@ int tcf_action_destroy(struct list_head *actions, int bind)
> 		goto nla_put_failure;
> 	if (tcf_action_copy_stats(skb, a, 0))
> 		goto nla_put_failure;
>+	if (a->act_ck) {
>+		if (nla_put(skb, TCA_ACT_COOKIE, a->act_ck->ck_len,
>+			    a->act_ck))
>+			goto nla_put_failure;
>+	}
>+
> 	nest = nla_nest_start(skb, TCA_OPTIONS);
> 	if (nest == NULL)
> 		goto nla_put_failure;
>@@ -575,6 +582,23 @@ struct tc_action *tcf_action_init_1(struct net *net, struct nlattr *nla,
> 	if (err < 0)
> 		goto err_mod;
> 
>+	if (tb[TCA_ACT_COOKIE]) {
>+		if (nla_len(tb[TCA_ACT_COOKIE]) > MAX_TC_COOKIE_SZ) {
>+			err = -EINVAL;
>+			goto err_mod;
>+		}
>+
>+		a->act_ck = kzalloc(sizeof(*a->act_ck), GFP_KERNEL);
>+		if (unlikely(!a->act_ck)) {
>+			err = -ENOMEM;
>+			goto err_mod;
>+		}
>+
>+		memcpy((void *)a->act_ck->ck, nla_data(tb[TCA_ACT_COOKIE]),

                       Unneeded (void *) cast.


>+		       nla_len(tb[TCA_ACT_COOKIE]));
>+		a->act_ck->ck_len = nla_len(tb[TCA_ACT_COOKIE]);
>+	}
>+
> 	/* module count goes up only when brand new policy is created
> 	 * if it exists and is only bound to in a_o->init() then
> 	 * ACT_P_CREATED is not returned (a zero is).
>-- 
>1.9.1
>

Re: [PATCH net-next 1/1] net sched actions: Add support for user cookies

[Jamal Hadi Salim]

On 17-01-15 04:11 AM, Jiri Pirko wrote:
> Subject should contain "V2"
>
> Sat, Jan 14, 2017 at 10:52:44PM CET, jhs@mojatatu.com wrote:
>> From: Jamal Hadi Salim <jhs@mojatatu.com>
>>
>> Introduce optional 128-bit action cookie.
>> Like all other cookie schemes in the networking world (eg in protocols
>> like http or existing kernel fib protocol field, etc) the idea is to save
>> user state that when retrieved serves as a correlator. The kernel
>> _should not_ intepret it.  The user can store whatever they wish in the
>> 128 bits.
>>
>> Sample exercise(using two 64bit values to represent the 128 bits):
>
> Looks like you did not update the description.
>

Yikes. Yes - will send v3.

>>

>> Signed-off-by: Jamal Hadi Salim <jhs@mojatatu.com>
>
> V1->V2 changelog would ne nice.
>

Will do.

>
>> ---
>> include/net/act_api.h        |  1 +
>> include/uapi/linux/pkt_cls.h | 11 +++++++++++
>> net/sched/act_api.c          | 28 ++++++++++++++++++++++++++--
>> 3 files changed, 38 insertions(+), 2 deletions(-)
>>
>> diff --git a/include/net/act_api.h b/include/net/act_api.h
>> index 1d71644..0692458 100644
>> --- a/include/net/act_api.h
>> +++ b/include/net/act_api.h
>> @@ -41,6 +41,7 @@ struct tc_action {
>> 	struct rcu_head			tcfa_rcu;
>> 	struct gnet_stats_basic_cpu __percpu *cpu_bstats;
>> 	struct gnet_stats_queue __percpu *cpu_qstats;
>> +	struct tc_cookie	*act_ck;
>
> I wonder if we just can't do:
> struct tc_cookie act_ck;
> You would safe kzalloc. I don't have strong opinion though..
>

Lets spare some RAM. I'll keep it the way it is.

>
>> };
>> #define tcf_head	common.tcfa_head
>> #define tcf_index	common.tcfa_index
>> diff --git a/include/uapi/linux/pkt_cls.h b/include/uapi/linux/pkt_cls.h
>> index 1e5e1dd..063bc89 100644
>> --- a/include/uapi/linux/pkt_cls.h
>> +++ b/include/uapi/linux/pkt_cls.h
>> @@ -4,6 +4,16 @@
>> #include <linux/types.h>
>> #include <linux/pkt_sched.h>
>>
>> +#define MAX_TC_COOKIE_SZ 16
>
> I like to have some "namespace" prefix for user api.
> "TC_COOKIE_MAX_SIZE" perhaps?
>

I just cutnpasted from something familiar: MAX_PHYS_ITEM_ID_LEN ;->
Will make the change.

>> +
>> +/* This structure holds cookie structure that is passed from user
>> + * to the kernel for actions and classifiers
>> + */
>> +struct tc_cookie {
>> +	unsigned char ck[MAX_TC_COOKIE_SZ];
>> +	unsigned char ck_len;
>
> This struct should certainly not be in UAPI header.
>

I will move it.

>
>> +};
>> +
>> /* Action attributes */
>> enum {
>> 	TCA_ACT_UNSPEC,
>> @@ -12,6 +22,7 @@ enum {
>> 	TCA_ACT_INDEX,
>> 	TCA_ACT_STATS,
>> 	TCA_ACT_PAD,
>> +	TCA_ACT_COOKIE,
>> 	__TCA_ACT_MAX
>> };


>> +		memcpy((void *)a->act_ck->ck, nla_data(tb[TCA_ACT_COOKIE]),
>
>                        Unneeded (void *) cast.
>

ok. I think gcc whined for some reason.

Will post after some test.

cheers,
jamal