Re: [PATCH nf-next v2 1/2] netfilter: flowtable: carry a priority into the offload

> The packets the flowtable forwards bypass the rules that classified
> the connection, so a priority set by "meta priority set" ahead of
> "flow add" reaches the qdisc only on the packets that traversed the
> ruleset before the flow existed; the rest keep the priority they
> arrived with, which for a forwarded packet is normally none. A flow
> rule handed to a driver has the same gap: it describes NAT,
> encapsulation and the output device, but not how the flow should be
> treated on the way out, so hardware with priority queues can only
> fall back on the DSCP the packet carries.
> 
> Store skb->priority of the packet that created the flow. The software
> fast path applies it to the packets it forwards; the hardware path
> emits it as FLOW_ACTION_PRIORITY, the action act_skbedit already
> emits on the tc path, so the rule handed to the driver describes what
> the software path does. A flow without a priority emits no action and
> leaves skb->priority of the packets it forwards alone.
> 
> Of the in-tree consumers of these rules, mtk and airoha ignore the new
> action as they do FLOW_ACTION_CSUM. mlx5 has no parser for it and
> rejects the rule, so a flow with a priority stays on the software path
> there, as a flow with PPPoE encapsulation already does.
> 
> The flowtable holds one flow for both directions and the expression
> runs once, so the priority applies to both; per-direction
> classification is not carried.
> 
> Assisted-by: Claude:claude-opus-5
> Signed-off-by: Julius Bairaktaris <[email protected]>
> ---
> Changes in v2:
> - apply the priority on the software fast path as well, in the IPv4
>   and IPv6 flowtable hooks, so a flow forwarded in software and one
>   forwarded by hardware get the same treatment (Lorenzo Bianconi)
> - describe it in nf_flowtable.rst
> - add the selftest in 2/2
> 
> v1: https://lore.kernel.org/netfilter-devel/[email protected]/

Hi Julius,

I think this patch is technically correct (just a nit inline), but IIRC Pablo
would lean towards to a more general solution where you can specify these new
parameters (e.g. priority) inside the flowtable nft configuration. @Pablo?

Regards,
Lorenzo

> 
> The consumer of the emitted action is a DSA driver for the IPQ8074 PPE,
> maintained in OpenWrt; measured there, "meta priority set" ahead of
> "flow add" places hardware-offloaded flows in the port's priority
> queues, and with hardware offload disabled a 30 MB IPv4 and a 30 MB
> IPv6 transfer both land in the stamped priority band with only the
> handshake traversing the ruleset. The selftest in 2/2 passes on this
> series and fails on the base commit, x86_64 under QEMU, three runs
> each. The mtk and airoha hunks are compile-tested only. The new field
> grows struct flow_offload by eight bytes on 64-bit; the entry allocates
> from its own kmem_cache, so no allocation-class change.
> 
>  Documentation/networking/nf_flowtable.rst       |  4 +++-
>  drivers/net/ethernet/airoha/airoha_ppe.c        |  1 +
>  drivers/net/ethernet/mediatek/mtk_ppe_offload.c |  1 +
>  include/net/netfilter/nf_flow_table.h           |  1 +
>  net/netfilter/nf_flow_table_ip.c                |  6 ++++++
>  net/netfilter/nf_flow_table_offload.c           | 11 +++++++++++
>  net/netfilter/nft_flow_offload.c                |  5 +++++
>  7 files changed, 28 insertions(+), 1 deletion(-)
> 
> diff --git a/Documentation/networking/nf_flowtable.rst b/Documentation/networking/nf_flowtable.rst
> index d757c21c10f2..5844ab19aec6 100644
> --- a/Documentation/networking/nf_flowtable.rst
> +++ b/Documentation/networking/nf_flowtable.rst
> @@ -71,7 +71,9 @@ forwarding path including the Netfilter hooks and the flowtable fastpath bypass.
>  
>  The flowtable entry also stores the NAT configuration, so all packets are
>  mangled according to the NAT policy that is specified from the classic IP
> -forwarding path. The TTL is decremented before calling neigh_xmit(). Fragmented
> +forwarding path. The TTL is decremented before calling neigh_xmit(). The flow
> +also stores the priority of the packet that created it, so a priority set before
> +``flow add`` applies to the packets that the flowtable forwards. Fragmented
>  traffic is passed up to follow the classic IP forwarding path given that the
>  transport header is missing, in this case, flowtable lookups are not possible.
>  TCP RST and FIN packets are also passed up to the classic IP forwarding path to
> diff --git a/drivers/net/ethernet/airoha/airoha_ppe.c b/drivers/net/ethernet/airoha/airoha_ppe.c
> index 92611802801e..2afce76ad131 100644
> --- a/drivers/net/ethernet/airoha/airoha_ppe.c
> +++ b/drivers/net/ethernet/airoha/airoha_ppe.c
> @@ -1161,6 +1161,7 @@ static int airoha_ppe_flow_offload_replace(struct airoha_eth *eth,
>  		case FLOW_ACTION_REDIRECT:
>  			odev = act->dev;
>  			break;
> +		case FLOW_ACTION_PRIORITY:

here you can do something like:

diff --git a/drivers/net/ethernet/airoha/airoha_ppe.c b/drivers/net/ethernet/airoha/airoha_ppe.c
index 92611802801e..e790305ea955 100644
--- a/drivers/net/ethernet/airoha/airoha_ppe.c
+++ b/drivers/net/ethernet/airoha/airoha_ppe.c
@@ -1161,6 +1161,9 @@ static int airoha_ppe_flow_offload_replace(struct airoha_eth *eth,
 		case FLOW_ACTION_REDIRECT:
 			odev = act->dev;
 			break;
+		case FLOW_ACTION_PRIORITY:
+			priority = act->priority;
+			break;
 		case FLOW_ACTION_CSUM:
 			break;
 		case FLOW_ACTION_VLAN_PUSH:

>  		case FLOW_ACTION_CSUM:
>  			break;
>  		case FLOW_ACTION_VLAN_PUSH:
> diff --git a/drivers/net/ethernet/mediatek/mtk_ppe_offload.c b/drivers/net/ethernet/mediatek/mtk_ppe_offload.c
> index 99b28aaa7cc4..4ee99e8e4a34 100644
> --- a/drivers/net/ethernet/mediatek/mtk_ppe_offload.c
> +++ b/drivers/net/ethernet/mediatek/mtk_ppe_offload.c
> @@ -378,6 +378,7 @@ mtk_flow_offload_replace(struct mtk_eth *eth, struct flow_cls_offload *f,
>  		case FLOW_ACTION_REDIRECT:
>  			odev = act->dev;
>  			break;
> +		case FLOW_ACTION_PRIORITY:
>  		case FLOW_ACTION_CSUM:
>  			break;
>  		case FLOW_ACTION_VLAN_PUSH:
> diff --git a/include/net/netfilter/nf_flow_table.h b/include/net/netfilter/nf_flow_table.h
> index f2e2771f188f..23218c8cbc3d 100644
> --- a/include/net/netfilter/nf_flow_table.h
> +++ b/include/net/netfilter/nf_flow_table.h
> @@ -202,6 +202,7 @@ struct flow_offload {
>  	unsigned long				flags;
>  	u16					type;
>  	u32					timeout;
> +	u32					priority;
>  	struct rcu_head				rcu_head;
>  };
>  
> diff --git a/net/netfilter/nf_flow_table_ip.c b/net/netfilter/nf_flow_table_ip.c
> index c8c29a9a1684..c85e2d608c32 100644
> --- a/net/netfilter/nf_flow_table_ip.c
> +++ b/net/netfilter/nf_flow_table_ip.c
> @@ -509,6 +509,9 @@ static int nf_flow_offload_forward(struct nf_flowtable_ctx *ctx,
>  	ip_decrease_ttl(iph);
>  	skb_clear_tstamp(skb);
>  
> +	if (flow->priority)
> +		skb->priority = flow->priority;
> +
>  	if (flow_table->flags & NF_FLOWTABLE_COUNTER)
>  		nf_ct_acct_update(flow->ct, tuplehash->tuple.dir, skb->len);
>  
> @@ -1104,6 +1107,9 @@ static int nf_flow_offload_ipv6_forward(struct nf_flowtable_ctx *ctx,
>  	ip6h->hop_limit--;
>  	skb_clear_tstamp(skb);
>  
> +	if (flow->priority)
> +		skb->priority = flow->priority;
> +
>  	if (flow_table->flags & NF_FLOWTABLE_COUNTER)
>  		nf_ct_acct_update(flow->ct, tuplehash->tuple.dir, skb->len);
>  
> diff --git a/net/netfilter/nf_flow_table_offload.c b/net/netfilter/nf_flow_table_offload.c
> index 801a3dd9ceea..caaadffc2563 100644
> --- a/net/netfilter/nf_flow_table_offload.c
> +++ b/net/netfilter/nf_flow_table_offload.c
> @@ -696,6 +696,17 @@ nf_flow_rule_route_common(struct net *net, const struct flow_offload *flow,
>  	    flow_offload_eth_dst(net, flow, dir, flow_rule) < 0)
>  		return -1;
>  
> +	if (flow->priority) {
> +		struct flow_action_entry *entry;
> +
> +		entry = flow_action_entry_next(flow_rule);
> +		if (!entry)
> +			return -1;
> +
> +		entry->id = FLOW_ACTION_PRIORITY;
> +		entry->priority = flow->priority;
> +	}
> +
>  	tuple = &flow->tuplehash[dir].tuple;
>  
>  	for (i = 0; i < tuple->encap_num; i++) {
> diff --git a/net/netfilter/nft_flow_offload.c b/net/netfilter/nft_flow_offload.c
> index 32b4281038dd..ca91924b4de3 100644
> --- a/net/netfilter/nft_flow_offload.c
> +++ b/net/netfilter/nft_flow_offload.c
> @@ -117,6 +117,11 @@ static void nft_flow_offload_eval(const struct nft_expr *expr,
>  	if (tcph)
>  		flow_offload_ct_tcp(ct);
>  
> +	/* The packets the flow forwards in its place bypass the rules that
> +	 * classified this one; carry the result with the flow.
> +	 */
> +	flow->priority = pkt->skb->priority;
> +
>  	__set_bit(NF_FLOW_HW_BIDIRECTIONAL, &flow->flags);
>  	ret = flow_offload_add(flowtable, flow);
>  	if (ret < 0)
> 
> base-commit: 91ec2035134982b98fab0609a9fd8480e8217dc1
> -- 
> 2.53.0
>

-----BEGIN PGP SIGNATURE-----

iHUEABYKAB0WIQTquNwa3Txd3rGGn7Y6cBh0uS2trAUCapki6AAKCRA6cBh0uS2t
rL3cAQDhh9diYlmQ+5GMLhPnY3Z6AsI2DGeRr8ELV50bah/oygD8Cp99E+LhGw66
QbWI7C7cV/H/HSfT+2y6wvO/i2UA4gQ=
=zywy
-----END PGP SIGNATURE-----