Re: [PATCH net-next,v3 11/11] netfilter: nf_tables: add hardware offload support

Jakub Kicinski
2019-07-09T01:44:46Z

On Mon, 8 Jul 2019 18:06:13 +0200, Pablo Neira Ayuso wrote:
> This patch adds hardware offload support for nftables through the
> existing netdev_ops->ndo_setup_tc() interface, the TC_SETUP_CLSFLOWER
> classifier and the flow rule API. This hardware offload support is
> available for the NFPROTO_NETDEV family and the ingress hook.
>
> Each nftables expression has a new ->offload interface, that is used to
> populate the flow rule object that is attached to the transaction
> object.
>
> There is a new per-table NFT_TABLE_F_HW flag, that is set on to offload
> an entire table, including all of its chains.
>
> This patch supports for basic metadata (layer 3 and 4 protocol numbers),
> 5-tuple payload matching and the accept/drop actions; this also includes
> basechain hardware offload only.
>
> Signed-off-by: Pablo Neira Ayuso <pablo@netfilter.org>

Any particular reason to not fence this off with a device feature
(ethtool -k)? Then you wouldn't need that per-driver list abomination
until drivers start advertising it.. IDK if we want the per-device
offload enable flags or not in general, it seems like a good idea in
general for admin to be able to disable offload per device 🤷

> +static int nft_flow_offload_rule(struct nft_trans *trans,
> + enum tc_fl_command command)
> +{
> + struct nft_flow_rule *flow = nft_trans_flow_rule(trans);
> + struct nft_rule *rule = nft_trans_rule(trans);
> + struct tc_cls_flower_offload cls_flower = {};
> + struct nft_base_chain *basechain;
> + struct netlink_ext_ack extack;
> + __be16 proto = ETH_P_ALL;
> +
> + if (!nft_is_base_chain(trans->ctx.chain))
> + return -EOPNOTSUPP;
> +
> + basechain = nft_base_chain(trans->ctx.chain);
> +
> + if (flow)
> + proto = flow->proto;
> +
> + nft_flow_offload_common_init(&cls_flower.common, proto, &extack);
> + cls_flower.command = command;
> + cls_flower.cookie = (unsigned long) rule;
> + if (flow)
> + cls_flower.rule = flow->rule;
> +
> + return nft_setup_cb_call(basechain, TC_SETUP_CLSFLOWER, &cls_flower);
> +}

Are we 100% okay with using TC cls_flower structures and defines in nft
code?