Re: [patch net-next 0/9] net: sched: introduce chain templates support with offloading to mlxsw

From: Cong Wang <xiyou.wangcong@gmail.com>
To: Jiri Pirko <jiri@resnulli.us>
Cc: sridhar.samudrala@intel.com,
	Jakub Kicinski <jakub.kicinski@netronome.com>,
	Linux Kernel Network Developers <netdev@vger.kernel.org>,
	David Miller <davem@davemloft.net>,
	Jamal Hadi Salim <jhs@mojatatu.com>,
	Simon Horman <simon.horman@netronome.com>,
	john.hurley@netronome.com, David Ahern <dsahern@gmail.com>,
	mlxsw@mellanox.com
Subject: Re: [patch net-next 0/9] net: sched: introduce chain templates support with offloading to mlxsw
Date: Thu, 28 Jun 2018 10:32:13 -0700	[thread overview]
Message-ID: <CAM_iQpWiVeuu+MJ5VH4Nx9n2wGyG6dgaNzTGW+cLOOW=fdw3iA@mail.gmail.com> (raw)
In-Reply-To: <20180628061817.GB2413@nanopsycho>

On Wed, Jun 27, 2018 at 11:19 PM Jiri Pirko <jiri@resnulli.us> wrote:
>
> Wed, Jun 27, 2018 at 07:04:32PM CEST, xiyou.wangcong@gmail.com wrote:
> >On Wed, Jun 27, 2018 at 9:46 AM Samudrala, Sridhar
> ><sridhar.samudrala@intel.com> wrote:
> >>
> >> On 6/27/2018 12:50 AM, Jiri Pirko wrote:
> >> > if you don't like "tc filter template add dev dummy0 ingress", how
> >> > about:
> >> > "tc template add dev dummy0 ingress ..."
> >> > "tc template add dev dummy0 ingress chain 22 ..."
> >> > that makes more sense I think.
> >
> >Better than 'tc filter template', but this doesn't reflect 'template'
> >is a template of tc filter, it could be an action etc., since it is in the
>
> It's a template of filter per chain. I don't understand how it could be
> an action...

It's because you have that in your mind from very beginning.

Think about what a new TC user's reaction is to 'tc template'
after he/she learns 'tc qdisc/filter/action'. It could be a template
of either of these 3 literately...

>
>
> >same position with 'tc action/filter/qdisc'.
> >
> >
> >>
> >> Isn't it possible to avoid introducing another keyword 'template',
> >>
> >> Can't we just do
> >>        tc chain add dev dummy0 ingress flower chain_index 0
> >> to create a chain that takes any types of flower rules with index 0
> >> and
> >>       tc chain add dev dummy0 ingress flower chain_index 22
> >>              dst_mac 00:00:00:00:00:00/00:00:00:00:FF:FF
> >>       tc chain add dev dummy0 ingress flower chain_index 23
> >>              dst_ip 192.168.0.0/16
> >> to create 2 chains 22 and 23 that allow rules with specific fields.
> >
> >Sounds good too. Since filter chain can be shared by qdiscs,
> >a 'tc chain' sub-command makes sense, and would probably make
> >it easier to be shared.
>
> We don't have such specific object. It is implicit. We create it
> whenever someone users it. Either filter of chain. I don't like new "tc
> chain" object in cmdline. It really isn't.

I discussed this with you at netconf, it is similar to tc actions,
tc actions can be shared not because they are implicitly created,
but because they could be created alone via `tc action add ...`.

If you don't share the chain, it is perfectly fine to create it
implicitly. If you do share, as in current code base, making it
standalone is reasonable.