Re: [PATCH net-next] net/sched: cls_flower: Add user specified data

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

On 17-01-15 09:36 AM, Paul Blakey wrote:
> 
> 
> On 08/01/2017 19:12, Jiri Pirko wrote:
>> Mon, Jan 02, 2017 at 03:59:49PM CET, jhs@mojatatu.com wrote:
>>>
>>> We have been using a cookie as well for actions (which we have been
>>> using but have been too lazy to submit so far). I am going to port
>>> it over to the newer kernels and post it.
>>
>> Hard to deal with something we can't look at :)
>>
>>
>>> In our case that is intended to be opaque to the kernel i.e kernel
>>> never inteprets it; in that case it is similar to the kernel
>>> FIB protocol field.
>>
>> In case of this patch, kernel also never interprets it. What makes you
>> think otherwise. Bot kernel, it is always a binary blob.
>>
>>
>>>
>>> In your case - could this cookie have been a class/flowid
>>> (a 32 bit)?
>>> And would it not make more sense for it the cookie to be
>>> generic to all classifiers? i.e why is it specific to flower?
>>
>> Correct, makes sense to have it generic for all cls and perhaps also
>> acts.
>>
>>
> 
> Hi all,
> I've started implementing a general cookie for all classifiers,
> I added the cookie on tcf_proto struct but then I realized that it won't work as
> I want. What I want is cookie per internal element (those that are identified by
> handles), which we can have many per one tcf_proto:
> 
> tc filter add dev <DEV> parent ffff: prio 1 cookie 0x123 basic action drop
> tc filter add dev <DEV> parent ffff: prio 1 cookie 0x456 "port6" basic action drop
> tc filter add dev <DEV> parent ffff: prio 1 cookie 0x777 basic action drop
> 
> So there is three options to do that:
> 1) Implement it in each classifier, using some generic function. (kinda like
> stats, where classifiler_dump() calls tcf_exts_dump_stats())
> 2) Have a global hash table for cookies [prio,handle]->[cookie]
> 3) Have it on flower only for now :)
> 
> With the first one we will have to change each classifier (or leave it
> unsupported as default).
> Second is less code to change (instead of changing each classifier), but might
> be slower. I'm afraid how it will affect dumping several filters.
> Third is this patch.
> 
> Thanks,
> Paul.

Avoid (2) it creates way more problems than its worth like is it locked/not
locked, how is it synced, collisions, etc. Seems way overkill.

I like the generic functionality of (1) but unless we see this pop up in
different filters I wouldn't require it for now. If you just do it in flower
(option 3) when its added to another classifier we can generalize it. As a
middle ground creating nice helper routines as needed goes a long way.

.John