Message-ID: <e04c2d9c-119f-621b-6ce9-6f6f449b6f86@mojatatu.com>
Date:   Fri, 9 Apr 2021 07:13:42 -0400
From:   Jamal Hadi Salim <jhs@...atatu.com>
To:     Jakub Kicinski <kuba@...nel.org>
Cc:     Petr Machata <petrm@...dia.com>, netdev@...r.kernel.org,
        Jiri Pirko <jiri@...dia.com>,
        "David S. Miller" <davem@...emloft.net>,
        Ido Schimmel <idosch@...dia.com>,
        Cong Wang <xiyou.wangcong@...il.com>
Subject: Re: [PATCH net-next 1/7] net: sched: Add a trap-and-forward action

On 2021-04-08 5:25 p.m., Jakub Kicinski wrote:
> On Thu, 8 Apr 2021 10:05:07 -0400 Jamal Hadi Salim wrote:
>> On 2021-04-08 9:38 a.m., Petr Machata wrote:
>>> The TC action "trap" is used to instruct the HW datapath to drop the
>>> matched packet and transfer it for processing in the SW pipeline. If
>>> instead it is desirable to forward the packet and transferring a _copy_ to
>>> the SW pipeline, there is no practical way to achieve that.
>>>
>>> To that end add a new generic action, trap_fwd. In the software pipeline,
>>> it is equivalent to an OK. When offloading, it should forward the packet to
>>> the host, but unlike trap it should not drop the packet.
>>
>> 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
>>
>> 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
> 
> To make sure I understand - are you saying that trap should become
> more general and support both "and then drop" as well as "and then
> pass" semantics?
> 

No.
Main issue is the pollution of the opcodes - whether it is
one or multiple actions is less of a concern.
Those opcodes are intended to be for the core action dispatcher's
consumption. See figure 6 and table 1 of the document i referred to.

Basically:
You dont an action then add an opcode for it even if it is hardware
offloaded (otherwise that opcode space would have grown a lot more by
now for all those actions that are offloaded).
Trap, for example, could have been a dummy action that just returns
the STOLEN/DROP/PASS opcode and does nothing else.
Typically we expect things that are offloaded to have a software
equivalent. It makes for good control consistency etc clean.

> Seems like that ship has sailed, but also - how does it make it any
> better WRT not having HW only opcodes? Or are you saying one is better
> than two?

The opcodes are not tied to whether an action is offloaded or not.
That role belongs to the "skip_sw" axes - which works well
today since we dont offload actions on their own without some
filter rule which specifies the offload option.

I will barf if someone implements 3 actions: "trap", "trap and forward",
"trap and drop" - but that is not messing up with the core architecture
so the barfing is more due to the bad taste of that approach.
A cleaner approach is to code one and change the return code for those
3 to "STOLEN", "PIPE", and "DROP"

cheers,
jamal

Powered by blists - more mailing lists