Re: [net-next PATCH v3 3/8] net: sched: add cls_u32 offload hooks for netdevs

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

On 16-02-18 04:14 AM, Jamal Hadi Salim wrote:
> On 16-02-17 06:07 PM, John Fastabend wrote:
>> [...]
>>
> 
>> Actually thinking about this a bit more I wrote this thinking
>> that there existed some hardware that actually cared if it was
>> a new rule or an existing rule. For me it doesn't matter I do
>> the same thing in the new/replace cases I just write into the
>> slot on the hardware table and if it happens to have something
>> in it well its overwritten e.g. "replaced". This works because
>> the cls_u32 layer protects us from doing something unexpected.
>>
> 
> You are describing create-or-update which is a reasonable default
> BUT: counting on the user to specify the htid+bktid+nodeid
> for every filter and knowing what that means is prone to mistakes
> when for example (using your big hammer approach right now) they
> dont specify the handle and the kernel creates one for them.
> 
> IMO, it would be better at this early stage to enforce the correct
> behavior for future generations.
> To follow the netlink semantics which a lot of people are already
> trained to think in.
> 
> Current netlink behavior is supposed to be:
> 
> 1) NEW ==> "Create".
> Ambigous - could mean a)"create if it doesnt exist" or b) "fail if it
> exists otherwise create"
> Unfortunately different parts of the kernel often assume some
> default from either #a or #b.
> 

But this is already handled by the core cls_api.c code. We never
get to u32_change if the flags are not correct.

Look at the block right above the op call into the classifiers
change() code in cls_api.c. Starting at line 287.

> 2) NEW|REPLACE flag ==> "Create if it doesnt exist and replace
> if it exists"
> 
> 3)NEW|EXCLUSIVE ==> "Create if it doesnt exist and fail if it
> exists"
> 
> 4)NEW|APPEND ==> "just fscking create; i dont care if it exists".
> 
> IOW, just add the flag field which is intepreted from whatever
> the user explicitly asks for. And reject say what the hardware
> doesnt support.

I think I agree with what your saying but I'm fairly sure this is
working as you describe.

> I have worked with tcams where we support #3. It is a bit inefficient
> because you have to check if a rule exists first. And i have worked
> in cases where #1 is assumed to mean #2 and at times #4. It is better
> user experience to be explicit.

Agreed, and I think it is :)

> 
> cheers,
> jamal