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

On 17-01-14 07:49 AM, Jiri Pirko wrote:

Sat, Jan 14, 2017 at 04:39:24PM CET, jhs@mojatatu.com wrote:

quoted

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

quoted

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

quoted

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

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

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.

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.

quoted

quoted

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

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.

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.