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

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

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.
Lets see how the refactoring goes first then it will be clearer - it
is a lot of delicate work - but you are right lets give it some love
now.

cheers,
jamal