On Sat, Oct 7, 2023 at 8:43 AM Jiri Pirko [off-list ref] wrote:

Sat, Oct 07, 2023 at 01:06:43PM CEST, jhs@mojatatu.com wrote:

quoted

On Sat, Oct 7, 2023 at 6:20 AM Jiri Pirko [off-list ref] wrote:

quoted

Sat, Oct 07, 2023 at 01:00:00AM CEST, jhs@mojatatu.com wrote:

quoted

On Fri, Oct 6, 2023 at 6:25 PM Jakub Kicinski [off-list ref] wrote:

quoted

On Fri, 6 Oct 2023 15:06:45 -0400 Jamal Hadi Salim wrote:

quoted

quoted

I don't understand the need for configuration less here. You don't have
it for the rest of the actions. Why this is special?

+1, FWIW

We dont have any rule that says all actions MUST have parameters ;->
There is nothing speacial about any action that doesnt have a
parameter.

You are getting the configuration from the block/device the action is
attached to. Can you point me to another action doing that?

We are entering a pedantic road i am afraid. If there is no existing
action that has zero config then consider this one the first one. We

Nope, nothing pedantic about it. I was just curious if there's anything
out there I missed.

Not sure if you noticed in the patch: the blockid on which the skb
arrived on is now available in the tc_cb[] so when it shows up at the
action we can just use it.

quoted

use skb->metadata all the time as a source of information for actions,
classifiers, qdiscs. If i dont need config i dont need to invent one

skb->metadata is something that is specific to a packet. That has
nothing to do with the actual configuration.

Essentially we turned blockid into skb metadata. A user specifying
configuration of a different blockid is certainly useful. My point is
we can have both worlds: when such a user config is missing we'll
assume a default which happens to be in the skb.

quoted

just because, well, all other actions are using one or more config;->
Your suggestion to specify an extra config to select a block - which
may be different than the one the one packet on - is a useful
feature(it just adds more code) but really should be optional. i.e if
you dont specify a block id configuration then we pick the metadata
one.

My primary point is, this should be mirred redirect to block instead of
what we currently have only for dev. That's it.

Agreed (and such a feature should be added regardless of this action).
The tc block provides a simple abstraction, but do you think it is
enough? Alternative is to use a list of ports given to mirred: it
allows us to group ports from different tc blocks or even just a
subset of what is in a tc block - but it will require a lot more code
to express such functionality.

quoted

quoted

quoted

If we can adequately cleanup mirred,  then we can put it there but
certainly now we are adding more buttons to click on mirred. It may
make sense to refactor the mirred code then reuse the refactored code
in a new action.

I don't understand why you need any new action. mirred redirect to block
instead of dev is exactly what you need. Isn't it?

The actions have different meanings and lumping them together then
selecting via a knob is not the right approach.
There's shared code for sure. Infact the sending code was ripped from
mirred so as not to touch the rest because like i said mirred has
since grown a couple of horns and tails. In retrospect mirred should
have been two actions with shared code - but it is too late to change
now because it is very widely used. If someone like me was afraid of
touching it is because there's a maintainance challenge. I consider it
in the same zone as trying to restructure something in the skb.
I agree mirroring to a group of ports with a simple config is a useful
feature. Mirroring to a group via a tc block is simpler but adding a
list of ports instead is more powerful. So this feature is useful to
have in mirred - just the adding of yet one more button to say "skip
this port" is my concern.

Why? Perhaps skb->iif could be used for check in the tx iteration.

We use skb->dev->ifindex to find the exception. Is iif better?.
Jiri - but why does this have to be part of mirred::mirror? I am
asking the same question of why mirror and redirect have to be part
mirred instead of separate actions.

cheers,
jamal