On 17-01-14 07:49 AM, Jiri Pirko wrote: > Sat, Jan 14, 2017 at 04:39:24PM CET, j...@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.