As reported by Ilya [1], unconditionally calling rte_flow_get_restore_info() impacts an application performance for drivers that do not provide this ops. It could also impact processing of packets that require no call to rte_flow_get_restore_info() at all.
Advertise in mbuf (via a dynamic flag) whether the driver has more metadata to provide via rte_flow_get_restore_info(). The application can then call it only when required. Link: http://inbox.dpdk.org/dev/5248c2ca-f2a6-3fb0-38b8-7f659bfa4...@ovn.org/ Signed-off-by: David Marchand <david.march...@redhat.com> --- Note: I did not test this RFC patch yet but I hope we can resume and maybe conclude on the discussion for the tunnel offloading API. --- app/test-pmd/util.c | 9 +++++---- drivers/net/mlx5/linux/mlx5_os.c | 14 ++++++++++---- drivers/net/mlx5/mlx5.h | 3 ++- drivers/net/mlx5/mlx5_flow.c | 26 ++++++++++++++++++++------ drivers/net/mlx5/mlx5_rx.c | 2 +- drivers/net/mlx5/mlx5_rx.h | 1 + drivers/net/mlx5/mlx5_trigger.c | 4 ++-- drivers/net/sfc/sfc_dp.c | 9 ++------- lib/ethdev/ethdev_driver.h | 8 ++++++++ lib/ethdev/rte_flow.c | 29 +++++++++++++++++++++++++++++ lib/ethdev/rte_flow.h | 19 ++++++++++++++++++- lib/ethdev/version.map | 4 ++++ 12 files changed, 102 insertions(+), 26 deletions(-) diff --git a/app/test-pmd/util.c b/app/test-pmd/util.c index f9df5f69ef..5aa69ed545 100644 --- a/app/test-pmd/util.c +++ b/app/test-pmd/util.c @@ -88,18 +88,20 @@ dump_pkt_burst(uint16_t port_id, uint16_t queue, struct rte_mbuf *pkts[], char print_buf[MAX_STRING_LEN]; size_t buf_size = MAX_STRING_LEN; size_t cur_len = 0; + uint64_t restore_info_dynflag; if (!nb_pkts) return; + restore_info_dynflag = rte_flow_restore_info_dynflag(); MKDUMPSTR(print_buf, buf_size, cur_len, "port %u/queue %u: %s %u packets\n", port_id, queue, is_rx ? "received" : "sent", (unsigned int) nb_pkts); for (i = 0; i < nb_pkts; i++) { - int ret; struct rte_flow_error error; struct rte_flow_restore_info info = { 0, }; mb = pkts[i]; + ol_flags = mb->ol_flags; if (rxq_share > 0) MKDUMPSTR(print_buf, buf_size, cur_len, "port %u, ", mb->port); @@ -107,8 +109,8 @@ dump_pkt_burst(uint16_t port_id, uint16_t queue, struct rte_mbuf *pkts[], eth_type = RTE_BE_TO_CPU_16(eth_hdr->ether_type); packet_type = mb->packet_type; is_encapsulation = RTE_ETH_IS_TUNNEL_PKT(packet_type); - ret = rte_flow_get_restore_info(port_id, mb, &info, &error); - if (!ret) { + if ((ol_flags & restore_info_dynflag) != 0 && + rte_flow_get_restore_info(port_id, mb, &info, &error) == 0) { MKDUMPSTR(print_buf, buf_size, cur_len, "restore info:"); if (info.flags & RTE_FLOW_RESTORE_INFO_TUNNEL) { @@ -153,7 +155,6 @@ dump_pkt_burst(uint16_t port_id, uint16_t queue, struct rte_mbuf *pkts[], " - pool=%s - type=0x%04x - length=%u - nb_segs=%d", mb->pool->name, eth_type, (unsigned int) mb->pkt_len, (int)mb->nb_segs); - ol_flags = mb->ol_flags; if (ol_flags & RTE_MBUF_F_RX_RSS_HASH) { MKDUMPSTR(print_buf, buf_size, cur_len, " - RSS hash=0x%x", diff --git a/drivers/net/mlx5/linux/mlx5_os.c b/drivers/net/mlx5/linux/mlx5_os.c index 980234e2ac..e6e3784013 100644 --- a/drivers/net/mlx5/linux/mlx5_os.c +++ b/drivers/net/mlx5/linux/mlx5_os.c @@ -575,11 +575,17 @@ mlx5_alloc_shared_dr(struct mlx5_priv *priv) goto error; } #endif - if (!sh->tunnel_hub && sh->config.dv_miss_info) + if (!sh->tunnel_hub && sh->config.dv_miss_info) { + err = mlx5_flow_restore_info_register(); + if (err) { + DRV_LOG(ERR, "Could not register mbuf dynflag for rte_flow_get_restore_info"); + goto error; + } err = mlx5_alloc_tunnel_hub(sh); - if (err) { - DRV_LOG(ERR, "mlx5_alloc_tunnel_hub failed err=%d", err); - goto error; + if (err) { + DRV_LOG(ERR, "mlx5_alloc_tunnel_hub failed err=%d", err); + goto error; + } } if (sh->config.reclaim_mode == MLX5_RCM_AGGR) { mlx5_glue->dr_reclaim_domain_memory(sh->rx_domain, 1); diff --git a/drivers/net/mlx5/mlx5.h b/drivers/net/mlx5/mlx5.h index 9eae692037..77cdc802da 100644 --- a/drivers/net/mlx5/mlx5.h +++ b/drivers/net/mlx5/mlx5.h @@ -2116,7 +2116,8 @@ int mlx5_flow_query_counter(struct rte_eth_dev *dev, struct rte_flow *flow, int mlx5_flow_dev_dump_ipool(struct rte_eth_dev *dev, struct rte_flow *flow, FILE *file, struct rte_flow_error *error); #endif -void mlx5_flow_rxq_dynf_metadata_set(struct rte_eth_dev *dev); +int mlx5_flow_restore_info_register(void); +void mlx5_flow_rxq_dynf_set(struct rte_eth_dev *dev); int mlx5_flow_get_aged_flows(struct rte_eth_dev *dev, void **contexts, uint32_t nb_contexts, struct rte_flow_error *error); int mlx5_validate_action_ct(struct rte_eth_dev *dev, diff --git a/drivers/net/mlx5/mlx5_flow.c b/drivers/net/mlx5/mlx5_flow.c index d0275fdd00..715b7d327d 100644 --- a/drivers/net/mlx5/mlx5_flow.c +++ b/drivers/net/mlx5/mlx5_flow.c @@ -1779,6 +1779,20 @@ flow_rxq_flags_clear(struct rte_eth_dev *dev) priv->sh->shared_mark_enabled = 0; } +static uint64_t mlx5_restore_info_dynflag; + +int +mlx5_flow_restore_info_register(void) +{ + int err = 0; + + if (mlx5_restore_info_dynflag == 0) { + if (rte_flow_restore_info_dynflag_register(&mlx5_restore_info_dynflag) < 0) + err = ENOMEM; + } + return err; +} + /** * Set the Rx queue dynamic metadata (mask and offset) for a flow * @@ -1786,7 +1800,7 @@ flow_rxq_flags_clear(struct rte_eth_dev *dev) * Pointer to the Ethernet device structure. */ void -mlx5_flow_rxq_dynf_metadata_set(struct rte_eth_dev *dev) +mlx5_flow_rxq_dynf_set(struct rte_eth_dev *dev) { struct mlx5_priv *priv = dev->data->dev_private; unsigned int i; @@ -1809,6 +1823,9 @@ mlx5_flow_rxq_dynf_metadata_set(struct rte_eth_dev *dev) data->flow_meta_offset = rte_flow_dynf_metadata_offs; data->flow_meta_port_mask = priv->sh->dv_meta_mask; } + data->mark_flag = RTE_MBUF_F_RX_FDIR_ID; + if (is_tunnel_offload_active(dev)) + data->mark_flag |= mlx5_restore_info_dynflag; } } @@ -11453,11 +11470,8 @@ mlx5_flow_tunnel_get_restore_info(struct rte_eth_dev *dev, const struct mlx5_flow_tbl_data_entry *tble; const uint64_t mask = RTE_MBUF_F_RX_FDIR | RTE_MBUF_F_RX_FDIR_ID; - if (!is_tunnel_offload_active(dev)) { - info->flags = 0; - return 0; - } - + if ((ol_flags & mlx5_restore_info_dynflag) == 0) + goto err; if ((ol_flags & mask) != mask) goto err; tble = tunnel_mark_decode(dev, m->hash.fdir.hi); diff --git a/drivers/net/mlx5/mlx5_rx.c b/drivers/net/mlx5/mlx5_rx.c index a2be523e9e..71c4638251 100644 --- a/drivers/net/mlx5/mlx5_rx.c +++ b/drivers/net/mlx5/mlx5_rx.c @@ -857,7 +857,7 @@ rxq_cq_to_mbuf(struct mlx5_rxq_data *rxq, struct rte_mbuf *pkt, if (MLX5_FLOW_MARK_IS_VALID(mark)) { pkt->ol_flags |= RTE_MBUF_F_RX_FDIR; if (mark != RTE_BE32(MLX5_FLOW_MARK_DEFAULT)) { - pkt->ol_flags |= RTE_MBUF_F_RX_FDIR_ID; + pkt->ol_flags |= rxq->mark_flag; pkt->hash.fdir.hi = mlx5_flow_mark_get(mark); } } diff --git a/drivers/net/mlx5/mlx5_rx.h b/drivers/net/mlx5/mlx5_rx.h index 52c35c83f8..3514edd84e 100644 --- a/drivers/net/mlx5/mlx5_rx.h +++ b/drivers/net/mlx5/mlx5_rx.h @@ -136,6 +136,7 @@ struct mlx5_rxq_data { struct mlx5_uar_data uar_data; /* CQ doorbell. */ uint32_t cqn; /* CQ number. */ uint8_t cq_arm_sn; /* CQ arm seq number. */ + uint64_t mark_flag; /* ol_flags to set with marks. */ uint32_t tunnel; /* Tunnel information. */ int timestamp_offset; /* Dynamic mbuf field for timestamp. */ uint64_t timestamp_rx_flag; /* Dynamic mbuf flag for timestamp. */ diff --git a/drivers/net/mlx5/mlx5_trigger.c b/drivers/net/mlx5/mlx5_trigger.c index bbaa7d2aa0..7bdb897612 100644 --- a/drivers/net/mlx5/mlx5_trigger.c +++ b/drivers/net/mlx5/mlx5_trigger.c @@ -1282,8 +1282,8 @@ mlx5_dev_start(struct rte_eth_dev *dev) dev->data->port_id); goto error; } - /* Set a mask and offset of dynamic metadata flows into Rx queues. */ - mlx5_flow_rxq_dynf_metadata_set(dev); + /* Set dynamic fields and flags into Rx queues. */ + mlx5_flow_rxq_dynf_set(dev); /* Set flags and context to convert Rx timestamps. */ mlx5_rxq_timestamp_set(dev); /* Set a mask and offset of scheduling on timestamp into Tx queues. */ diff --git a/drivers/net/sfc/sfc_dp.c b/drivers/net/sfc/sfc_dp.c index 9f2093b353..8b2dbea325 100644 --- a/drivers/net/sfc/sfc_dp.c +++ b/drivers/net/sfc/sfc_dp.c @@ -11,6 +11,7 @@ #include <string.h> #include <errno.h> +#include <ethdev_driver.h> #include <rte_log.h> #include <rte_mbuf_dyn.h> @@ -135,12 +136,8 @@ sfc_dp_ft_ctx_id_register(void) .size = sizeof(uint8_t), .align = __alignof__(uint8_t), }; - static const struct rte_mbuf_dynflag ft_ctx_id_valid = { - .name = "rte_net_sfc_dynflag_ft_ctx_id_valid", - }; int field_offset; - int flag; SFC_GENERIC_LOG(INFO, "%s() entry", __func__); @@ -156,15 +153,13 @@ sfc_dp_ft_ctx_id_register(void) return -1; } - flag = rte_mbuf_dynflag_register(&ft_ctx_id_valid); - if (flag < 0) { + if (rte_flow_restore_info_dynflag_register(&sfc_dp_ft_ctx_id_valid) < 0) { SFC_GENERIC_LOG(ERR, "%s() failed to register ft_ctx_id dynflag", __func__); return -1; } sfc_dp_ft_ctx_id_offset = field_offset; - sfc_dp_ft_ctx_id_valid = UINT64_C(1) << flag; SFC_GENERIC_LOG(INFO, "%s() done", __func__); diff --git a/lib/ethdev/ethdev_driver.h b/lib/ethdev/ethdev_driver.h index 2c9d615fb5..6c17d84d1b 100644 --- a/lib/ethdev/ethdev_driver.h +++ b/lib/ethdev/ethdev_driver.h @@ -1949,6 +1949,14 @@ __rte_internal int rte_eth_ip_reassembly_dynfield_register(int *field_offset, int *flag); +/** + * @internal + * Register mbuf dynamic flag for rte_flow_get_restore_info. + */ +__rte_internal +int +rte_flow_restore_info_dynflag_register(uint64_t *flag); + /* * Legacy ethdev API used internally by drivers. diff --git a/lib/ethdev/rte_flow.c b/lib/ethdev/rte_flow.c index 69e6e749f7..10cd9d12ba 100644 --- a/lib/ethdev/rte_flow.c +++ b/lib/ethdev/rte_flow.c @@ -1401,6 +1401,35 @@ rte_flow_get_restore_info(uint16_t port_id, NULL, rte_strerror(ENOTSUP)); } +static struct { + const struct rte_mbuf_dynflag desc; + uint64_t value; +} flow_restore_info_dynflag = { + .desc = { .name = "RTE_MBUF_F_RX_RESTORE_INFO", }, +}; + +uint64_t +rte_flow_restore_info_dynflag(void) +{ + return flow_restore_info_dynflag.value; +} + +int +rte_flow_restore_info_dynflag_register(uint64_t *flag) +{ + if (flow_restore_info_dynflag.value == 0) { + int offset = rte_mbuf_dynflag_register(&flow_restore_info_dynflag.desc); + + if (offset < 0) + return -1; + flow_restore_info_dynflag.value = RTE_BIT64(offset); + } + if (*flag) + *flag = rte_flow_restore_info_dynflag(); + + return 0; +} + int rte_flow_tunnel_action_decap_release(uint16_t port_id, struct rte_flow_action *actions, diff --git a/lib/ethdev/rte_flow.h b/lib/ethdev/rte_flow.h index 713ba8b65c..5ce2db4bbd 100644 --- a/lib/ethdev/rte_flow.h +++ b/lib/ethdev/rte_flow.h @@ -4918,7 +4918,24 @@ rte_flow_tunnel_match(uint16_t port_id, struct rte_flow_error *error); /** - * Populate the current packet processing state, if exists, for the given mbuf. + * On reception of a mbuf from HW, a call to rte_flow_get_restore_info() may be + * required to retrieve some metadata. + * This function returns the associated mbuf ol_flags. + * + * Note: the dynamic flag is registered during the probing of the first device + * that requires it. If this function returns a non 0 value, this value won't + * change for the rest of the life of the application. + * + * @return + * The offload flag indicating rte_flow_get_restore_info() must be called. + */ +__rte_experimental +uint64_t +rte_flow_restore_info_dynflag(void); + +/** + * If a mbuf contains the rte_flow_restore_info_dynflag() flag in ol_flags, + * populate the current packet processing state. * * One should negotiate tunnel metadata delivery from the NIC to the HW. * @see rte_eth_rx_metadata_negotiate() diff --git a/lib/ethdev/version.map b/lib/ethdev/version.map index 357d1a88c0..bf5668e928 100644 --- a/lib/ethdev/version.map +++ b/lib/ethdev/version.map @@ -299,6 +299,9 @@ EXPERIMENTAL { rte_flow_action_handle_query_update; rte_flow_async_action_handle_query_update; rte_flow_async_create_by_index; + + # added in 23.07 + rte_flow_restore_info_dynflag; }; INTERNAL { @@ -328,4 +331,5 @@ INTERNAL { rte_eth_representor_id_get; rte_eth_switch_domain_alloc; rte_eth_switch_domain_free; + rte_flow_restore_info_dynflag_register; }; -- 2.40.0
