Message-ID: <ZSFSfPFXuvMC/max@nanopsycho>
Date: Sat, 7 Oct 2023 14:43:40 +0200
From: Jiri Pirko <jiri@...nulli.us>
To: Jamal Hadi Salim <jhs@...atatu.com>
Cc: Jakub Kicinski <kuba@...nel.org>, Victor Nogueira <victor@...atatu.com>,
	xiyou.wangcong@...il.com, davem@...emloft.net, pabeni@...hat.com,
	edumazet@...gle.com, mleitner@...hat.com, vladbu@...dia.com,
	simon.horman@...igine.com, pctammela@...atatu.com,
	netdev@...r.kernel.org, kernel@...atatu.com
Subject: Re: [PATCH net-next v4 0/3] net/sched: Introduce tc block ports
 tracking and use

Sat, Oct 07, 2023 at 01:06:43PM CEST, jhs@...atatu.com wrote:
>On Sat, Oct 7, 2023 at 6:20 AM Jiri Pirko <jiri@...nulli.us> wrote:
>>
>> Sat, Oct 07, 2023 at 01:00:00AM CEST, jhs@...atatu.com wrote:
>> >On Fri, Oct 6, 2023 at 6:25 PM Jakub Kicinski <kuba@...nel.org> wrote:
>> >>
>> >> On Fri, 6 Oct 2023 15:06:45 -0400 Jamal Hadi Salim wrote:
>> >> > > 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.


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


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



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


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

Powered by blists - more mailing lists