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

From: Petr Machata <petrm@nvidia.com>
To: Jamal Hadi Salim <jhs@mojatatu.com>
Cc: Petr Machata <petrm@nvidia.com>, <netdev@vger.kernel.org>,
	Jiri Pirko <jiri@nvidia.com>,
	"David S. Miller" <davem@davemloft.net>,
	Jakub Kicinski <kuba@kernel.org>,
	Ido Schimmel <idosch@nvidia.com>,
	Cong Wang <xiyou.wangcong@gmail.com>,
	Jiri Pirko <jiri@resnulli.us>
Subject: Re: [PATCH net-next 1/7] net: sched: Add a trap-and-forward action
Date: Fri, 9 Apr 2021 15:43:54 +0200	[thread overview]
Message-ID: <8735vz609x.fsf@nvidia.com> (raw)
In-Reply-To: <6424d667-86a9-8fd1-537e-331cf4e5970c@mojatatu.com>

Jamal Hadi Salim <jhs@mojatatu.com> writes:

> 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.
>>> [...]
>>> 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.
>
> here's how it translates:
> "We already made a mistake, therefore, its ok to build on it and
> make more mistakes".

I can see how it reads that way, but that was not the intention. I was
actually thinking about whether there might be a way to gradually
migrate all this stuff over to mirred, but at this point, trap is very
much baked in.

>>> 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.

(I meant mirred egress mirror, not redirect.)

> 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".

Hmm, we have devlink ports, the CPU port is exposed there. But that's
the only thing that comes to mind. Those are specific for the given
device though, it doesn't look suitable...

> 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 think your STOLEN and PIPE are the same behavior. Both are "transfer
the packet to the SW datapath, but keep it in the HW datapath".

In general I have no issue expressing this stuff as a new action,
instead of an opcode. I'll take a look at this.