Re: [PATCH net-next,RFC 0/9] net: sched: prepare to reuse per-block callbacks from netfilter

[Pablo Neira Ayuso <pablo@netfilter.org>]

On Fri, Apr 26, 2019 at 11:29:56AM -0700, Jakub Kicinski wrote:
> On Fri, 26 Apr 2019 18:28:36 +0200, Pablo Neira Ayuso wrote:
> > On Fri, Apr 26, 2019 at 04:32:58PM +0200, Jiri Pirko wrote:
> > > Fri, Apr 26, 2019 at 02:33:37AM CEST, pablo@netfilter.org wrote:  
> > > >Hi,
> > > >
> > > >This patchset aims to introduce changes to reuse the existing .ndo_setup_tc
> > > >netdev operations from netfilter.
> > > >
> > > >The idea is to move tcf_block_cb to net/core/flow_offload.c and rename
> > > >it to flow_block_cb. This object provides the minimal infrastructure to
> > > >set up per-block callbacks that are called to offload policies to
> > > >hardware.
> > > >
> > > >The tcf_block object is specific for TC to share policies between
> > > >ingress devices. This object has a list of tcf_block_cb objects that are
> > > >called to offload the policies to hardware. In netfilter, the idea is to
> > > >store the list of tcf_block_cb objects in a chain that would be bound to
> > > >several devices, eg.
> > > >
> > > >  chain x {
> > > >	type filter hook ingress devices = { eth0, eth1 } priority 0;
> > > >	...
> > > >  }
> > > >  
> > > 
> > > Do you have the follow-up patchset somewhere? I'm curius about your
> > > goal. Without that, it is hard to understand what you are getting at.  
> > 
> > Goal is to use the TC_SETUP_BLOCK logic in the existing drivers from
> > netfilter. So Netfilter calls TC_SETUP_BLOCK by when a chain is set up
> > to configure the driver, hence reuse your whole logic with minimal
> > changes.
> > 
> > Currently, the tcf_block_cb_register() call assumes there's a
> > tcf_block object in place and it internally invokes the tc
> > .reoffload() callback. This tcf_block corresponds to the nft_chain
> > object in netfilter, and I need to add my own .reoffload() callback
> > for the nft_chain object. This patch uses the block_index instead from
> > the driver, instead of exposing tcf_block.
> 
> block_index is non-0 only for shared blocks, right?  Did you change
> that?

I didn't change that, that's still the same.

The tcf_block_cb is identified via tuple [ block index, cb, cb_ident ]
to retrieve it via lookup, so in case the block index is zero, things
will still work fine.

> > This patchset updates the TC_SETUP_BLOCK path to only configure the
> > block_cb objects. The registration is done from the core, by iterating
> > the list of block_cb's that the driver offers in the temporary
> > tc_block_offload->cb_list, and then iterate over that list and
> > register them from the core.
> > 
> > My patchset moves the tcf_block_cb object to net/core/flow_offload.c
> > (it renames it to flow_block_cb) so it can be used both by tc and
> > netfilter.
> > 
> > Follow up patchset in netfilter calls TC_SETUP_BLOCK when the offloadi
> > flag is set on. Then, it has its own version of tc_setup_cb_call(),
> > which iterates over the block_cb() in this chain to reuse existing
> > driver codebase.
> > 
> > > >Hence, this emulates the shared blocks available in TC that Jiri made.
> > > >
> > > >Note that the list of tcf_block_cb objects will be called to offload
> > > >policies in this chain.  
> > > 
> > > So you are going to use chain_id (if there is anything like that) as
> > > block_index during offload, right?  
> > 
> > Yes. But I don't need to expose this chain_index to userspace though,
> > I can internally allocate it, I only need to make sure it does not
> > overlap with any of the existing tc block_indexed. I can just use a
> > different index space which does not overlap with the tc block index
> > space.
> 
> How will the association between a block and a device work for
> netfilter?

My proposal is that Netfilter doesn't need to expose anything similar
to the TC block concept. I mean, not to the user, not through the
command line and netlink itself.

If netfilter supports for chain definitions like this:

        table x {
                chain y {
                        type filter hook ingress devices = { eth0, eth1 } priority 0;
                }
        }

Then the chain 'y' implicitly becomes the block for the 'eth0' and
'eth1' devices.

Note that the above is not yet supported, I need to extend the netlink
API for this, but having chains that are attached to multiple devices
is feasible and it makes sense for plain software configurations where
no offload is involved (as useful as the TC block for pure software to
avoid policy duplication).