Re: [PATCH net-next 1/7] net: sched: Add a trap-and-forward action

[Jamal Hadi Salim <jhs@mojatatu.com>]

On 2021-04-09 7:03 a.m., Petr Machata wrote:
> 
> Jamal Hadi Salim <jhs@mojatatu.com> writes:
> 
>> I am concerned about adding new opcodes which only make sense if you
>> offload (or make sense only if you are running in s/w).
>>
>> Those opcodes are intended to be generic abstractions so the dispatcher
>> can decide what to do next. Adding things that are specific only
>> to scenarios of hardware offload removes that opaqueness.
>> I must have missed the discussion on ACT_TRAP because it is the
>> same issue there i.e shouldnt be an opcode. For details see:
>> https://people.netfilter.org/pablo/netdev0.1/papers/Linux-Traffic-Control-Classifier-Action-Subsystem-Architecture.pdf
> 
> Trap has been in since 4.13, so 2017ish. It's done and dusted at this
> point.
> 

I am afraid that is not a good arguement. With all due respect,
here's how it translates:
"We already made a mistake, therefore, its ok to build on it and
make more mistakes". Touching those opcodes is really dirty; at
least i have seen no convincing arguement _at all_ for it. And,
it is not too late not to make more mistakes.
I dont remember, I may have spoken against TRAP; what i know is had
i seen the patch i would have said something - maybe i did and should
have been louder. Mea culpa.

>> IMO:
>> It seems to me there are two actions here encapsulated in one.
>> The first is to "trap" and the second is to "drop".
>>
>> This is no different semantically than say "mirror and drop"
>> offload being enunciated by "skip_sw".
>>
>> Does the spectrum not support multiple actions?
>> e.g with a policy like:
>>   match blah action trap action drop skip_sw
> 
> Trap drops implicitly. We need a "trap, but don't drop". Expressed in
> terms of existing actions it would be "mirred egress redirect dev
> $cpu_port". But how to express $cpu_port except again by a HW-specific
> magic token I don't know.


Note: mirred was originally intended to send redirect/mirror
packets to user space (the comment is still there in the code).
Infact there is a patch lying around somewhere that does that with
packet sockets (the author hasnt been serious about pushing it
upstream). In that case the semantics are redirecting to a file
descriptor. Could we have something like that here which points
to whatever representation $cpu_port has? Sounds like semantics
for "trap and forward" are just "mirror and forward".

I think there is value in having something like trap action
which generalizes the combinations only to the fact that
it will make it easier to relay the info to the offload without
much transformation.
If i was to do it i would write one action configured by user space:
- to return DROP if you want action trap-and-drop semantics.
- to return STOLEN if you want trap
- to return PIPE if you want trap and forward. You will need a second
action composed to forward.

I said dummy because this action has no value in s/w. Someone could
use it in s/w but it would be no different than gact.
Maybe it could be extended to work also in s/w by adding the "trap to
fd" in userspace.

cheers,
jamal