Message-ID: <ZSGTdA/5WkVI7lvQ@nanopsycho>
Date: Sat, 7 Oct 2023 19:20:52 +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 04:09:15PM CEST, jhs@...atatu.com wrote:
>On Sat, Oct 7, 2023 at 8:43 AM Jiri Pirko <jiri@...nulli.us> wrote:
>>
>> 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.
>>
>
>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.

I see, but does it has to be? I don't think so with the solution I'm
proposing.


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

Again, you attach filter to either dev or block. If you extend mirred
redirect to accept the same 2 types of target, I think it would be best.


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

iif contains ifindex of the actual ingress device. So if the netdev is
part of bond for example, this still contains the original ifindex.
So I buess that this depends on what you need. Looks to me that
skb->dev->ifindex would be probaly better. It contains the netdev that
the filter is attached on, right?


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

You have to maintain the backwards compatibility. Currently mirred is
one action right? Does not matter how you do it in kernel, user should
not tell any difference.


>
>cheers,
>jamal

Powered by blists - more mailing lists