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

On 16-02-18 04:14 AM, Jamal Hadi Salim wrote:

On 16-02-17 06:07 PM, John Fastabend wrote:

quoted

[...]

quoted

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