Message-ID: <ZSA60cyLDVw13cLi@nanopsycho>
Date: Fri, 6 Oct 2023 18:50:25 +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 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?


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

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.


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