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

[John Fastabend <john.fastabend@gmail.com>]

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.