Message-ID: <ZSExT+qxjHKL6NWp@nanopsycho>
Date: Sat, 7 Oct 2023 12:22:07 +0200
From: Jiri Pirko <jiri@...nulli.us>
To: Jamal Hadi Salim <jhs@...atatu.com>
Cc: Victor Nogueira <victor@...atatu.com>, xiyou.wangcong@...il.com,
	davem@...emloft.net, pabeni@...hat.com, edumazet@...gle.com,
	kuba@...nel.org, 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

Fri, Oct 06, 2023 at 09:06:45PM CEST, jhs@...atatu.com wrote:
>On Fri, Oct 6, 2023 at 12:50 PM Jiri Pirko <jiri@...nulli.us> wrote:
>>
>> Fri, Oct 06, 2023 at 05:37:41PM CEST, jhs@...atatu.com wrote:
>> >On Fri, Oct 6, 2023 at 8:59 AM Jiri Pirko <jiri@...nulli.us> wrote:
>> >>
>> >> Thu, Oct 05, 2023 at 08:42:25PM CEST, victor@...atatu.com wrote:
>> >> >__Context__
>> >> >The "tc block" is a collection of netdevs/ports which allow qdiscs to share
>> >> >match-action block instances (as opposed to the traditional tc filter per
>> >> >netdev/port)[1].
>> >> >
>> >> >Example setup:
>> >> >$ tc qdisc add dev ens7 ingress block 22
>> >> >$ tc qdisc add dev ens8 ingress block 22
>> >> >
>> >> >Once the block is created we can add a filter using the block index:
>> >> >$ tc filter add block 22 protocol ip pref 25 \
>> >> >  flower dst_ip 192.168.0.0/16 action drop
>> >> >
>> >> >A packet with dst IP matching 192.168.0.0/16 arriving on the ingress of
>> >> >either ens7 or ens8 is dropped.
>> >> >
>> >> >__This patchset__
>> >> >Up to this point in the implementation, the block is unaware of its ports.
>> >> >This patch fixes that and makes the tc block ports available to the
>> >>
>> >> Odd. You fix a bug. Is there a bug? If yes, you need to describe it. If
>> >> no, don't use "fix".
>> >
>> >Ok, Jiri;->  we will change the language.
>> >
>> >>
>> >> >datapath.
>> >> >
>> >> >For the datapath we provide a use case of the tc block in an action
>> >> >we call "blockcast" in patch 3. This action can be used in an example as
>> >> >such:
>> >> >
>> >> >$ tc qdisc add dev ens7 ingress block 22
>> >> >$ tc qdisc add dev ens8 ingress block 22
>> >> >$ tc qdisc add dev ens9 ingress block 22
>> >> >$ tc filter add block 22 protocol ip pref 25 \
>> >> >  flower dst_ip 192.168.0.0/16 action blockcast
>> >>
>> >> Seems to me a bit odd that the action works with the entity (block) is
>> >> is connected to. I would expect rather to give the action configuration:
>> >>
>> >> $ tc filter add block 22 protocol ip pref 25 \
>> >>   flower dst_ip 192.168.0.0/16 action blockcast block 22
>> >>                                                 ^^^^^^^^
>> >
>> >We are currently passing the blockid in the skb cb field so it is
>> >configuration-less. I suppose we could add this as an optional field
>> >and use it when specified.
>>
>> 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?
>
>It is not needed really. Think of an L2 switch - the broadcast action
>is to send to all ports but self.
>
>>
>> >
>> >> Then this is more flexible and allows user to use this action for any
>> >> packet, no matter from where it was received.
>> >>
>> >> Looks like this is functionality-wise similar to mirred redirect. Why
>> >> can't we have that action extended to accept block number instead of
>> >> netdev and have something like:
>> >>
>> >> $ tc filter add block 22 protocol ip pref 25 \
>> >>   flower dst_ip 192.168.0.0/16 action mirred egress redirect block 22
>> >>
>> >> This would be very much alike we do either "tc filter add dev X" or "tc
>> >> filter add block Y".
>> >>
>> >
>> >We did consider it but concluded it is a lot of work to get it done on
>> >mirred - just take a look at mirred and you'll see what i mean;->
>> >Based on that review we came to the conclusion that at some point it
>> >would be safer to separate mirred's mirror from redirect; there are
>> >too many checks to avoid one or the other based on whether you are
>> >coming from egress vs ingress etc. This one is simple, it is just a
>> >broadcast.
>>
>> Perhaps it is a nice opportunity to do such mirred cleanup, prepare the
>> code and implement block send afterwards?
>
>I was worried about breaking some existing use cases - the code has
>got too clever.
>But probably it is time to show it some love, one of us will invest
>time into it.

Awesome.


>
>> If I omit the code for now, from user perspective, this functionality
>> belongs into mirred, don't you think? Just replace "dev" by "block" and
>> you got what you need.
>
>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?


>
>cheers,
>jamal
>
>>
>> >
>> >
>> >> Regarding the filtering, that could be a simple flag config of mirred
>> >> action:
>> >>
>> >> $ tc filter add block 22 protocol ip pref 25 \
>> >>   flower dst_ip 192.168.0.0/16 action mirred egress redirect block 22
>> >>   srcfilter
>> >>
>> >> Or something like that.
>> >>
>> >
>> >See my comment above.
>> >
>> >cheers,
>> >jamal
>> >> Makes sense?
>> >>
>> >>
>> >>
>> >> >
>> >> >When a packet(matching dst IP 192.168.0.0/16) arrives on the ingress of any
>> >> >of ens7, ens8 or ens9 it will be copied to all ports other than itself.
>> >> >For example, if it arrives on ens8 then a copy of the packet will be
>> >> >"blockcasted";-> to both ens7 and ens9 (unmodified), but not to ens8.
>> >> >
>> >> >Patch 1 introduces the required infra. Patch 2 exposes the tc block to the
>> >> >tc datapath and patch 3 implements datapath usage via a new tc action
>> >> >"blockcast".
>> >> >
>> >> >__Acknowledgements__
>> >> >Suggestions from Vlad Buslov and Marcelo Ricardo Leitner made this patchset
>> >> >better. The idea of integrating the ports into the tc block was suggested
>> >> >by Jiri Pirko.
>> >> >
>> >> >[1] See commit ca46abd6f89f ("Merge branch 'net-sched-allow-qdiscs-to-share-filter-block-instances'")
>> >> >
>> >> >Changes in v2:
>> >> >  - Remove RFC tag
>> >> >  - Add more details in patch 0(Jiri)
>> >> >  - When CONFIG_NET_TC_SKB_EXT is selected we have unused qdisc_cb
>> >> >    Reported-by: kernel test robot <lkp@...el.com> (and horms@...nel.org)
>> >> >  - Fix bad dev dereference in printk of blockcast action (Simon)
>> >> >
>> >> >Changes in v3:
>> >> >  - Add missing xa_destroy (pointed out by Vlad)
>> >> >  - Remove bugfix pointed by Vlad (will send in separate patch)
>> >> >  - Removed ports from subject in patch #2 and typos (suggested by Marcelo)
>> >> >  - Remove net_notice_ratelimited debug messages in error
>> >> >    cases (suggested by Marcelo)
>> >> >  - Minor changes to appease sparse's lock context warning
>> >> >
>> >> >Changes in v4:
>> >> >  - Avoid code repetition using gotos in cast_one (suggested by Paolo)
>> >> >  - Fix typo in cover letter (pointed out by Paolo)
>> >> >  - Create a module description for act_blockcast
>> >> >    (reported by Paolo and CI)
>> >> >
>> >> >Victor Nogueira (3):
>> >> >  net/sched: Introduce tc block netdev tracking infra
>> >> >  net/sched: cls_api: Expose tc block to the datapath
>> >> >  net/sched: act_blockcast: Introduce blockcast tc action
>> >> >
>> >> > include/net/sch_generic.h    |   8 +
>> >> > include/net/tc_wrapper.h     |   5 +
>> >> > include/uapi/linux/pkt_cls.h |   1 +
>> >> > net/sched/Kconfig            |  13 ++
>> >> > net/sched/Makefile           |   1 +
>> >> > net/sched/act_blockcast.c    | 297 +++++++++++++++++++++++++++++++++++
>> >> > net/sched/cls_api.c          |  12 +-
>> >> > net/sched/sch_api.c          |  58 +++++++
>> >> > net/sched/sch_generic.c      |  34 +++-
>> >> > 9 files changed, 426 insertions(+), 3 deletions(-)
>> >> > create mode 100644 net/sched/act_blockcast.c
>> >> >
>> >> >--
>> >> >2.25.1
>> >> >

Powered by blists - more mailing lists