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

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

quoted

.. 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.

quoted

.. 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.

quoted

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.

quoted

+};
+
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