From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mails.dpdk.org (mails.dpdk.org [217.70.189.124]) by inbox.dpdk.org (Postfix) with ESMTP id DCDC242B90; Wed, 24 May 2023 20:45:11 +0200 (CEST) Received: from mails.dpdk.org (localhost [127.0.0.1]) by mails.dpdk.org (Postfix) with ESMTP id 6930940156; Wed, 24 May 2023 20:45:11 +0200 (CEST) Received: from us-smtp-delivery-124.mimecast.com (us-smtp-delivery-124.mimecast.com [170.10.129.124]) by mails.dpdk.org (Postfix) with ESMTP id 4AC11400EF for ; Wed, 24 May 2023 20:45:09 +0200 (CEST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=mimecast20190719; t=1684953908; h=from:from:reply-to:subject:subject:date:date:message-id:message-id: to:to:cc:cc:mime-version:mime-version:content-type:content-type: content-transfer-encoding:content-transfer-encoding: in-reply-to:in-reply-to:references:references; bh=iFxaw/KtLerc7bTU9uhhKJEjFFEdQT2Sj6R/QAclcro=; b=TxW+Ezykhq4Si9TbrzTXMK8DWn7MjvVq/vBS48gPRoj01INGxtzsMzWdFq2pCqVLdEBK/r ZJHAc0jHyt7zEdcUthKvDV86HYWD8SFxIxqYMhQ6aZkOjq5jNtlPXo8QSbJwFHiNOLkF8U LhHoOg1NozFqSyfSTuesoPHcw1ULtck= Received: from mail-pl1-f198.google.com (mail-pl1-f198.google.com [209.85.214.198]) by relay.mimecast.com with ESMTP with STARTTLS (version=TLSv1.3, cipher=TLS_AES_256_GCM_SHA384) id us-mta-343-RLJu4NUqOTa07ueRyO5tlw-1; Wed, 24 May 2023 14:45:07 -0400 X-MC-Unique: RLJu4NUqOTa07ueRyO5tlw-1 Received: by mail-pl1-f198.google.com with SMTP id d9443c01a7336-1ae4c08429eso3254505ad.1 for ; Wed, 24 May 2023 11:45:07 -0700 (PDT) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20221208; t=1684953906; x=1687545906; h=content-transfer-encoding:cc:to:subject:message-id:date:from :in-reply-to:references:mime-version:x-gm-message-state:from:to:cc :subject:date:message-id:reply-to; bh=iFxaw/KtLerc7bTU9uhhKJEjFFEdQT2Sj6R/QAclcro=; b=XYXT/BLQiMCrqwfNNEEFE7R7DAO+gn9P8YXW13rZMK+YrHbJSeffqQi9soy/YfD6zU zBey8J5oucz806suj6ngUSErcJ7MF7XQZzAxgPoqFo6B0OsiVn6kA7carwb56SO246mp ROnEmGwBuMtJoCFENS6ItbbaUZ1LEkx94/W+DeXZ43/1GiTg63nk58QTUHSqaGj2E+ZM aRpQnXMyVIiARMIOXv3IjwzZB6jxSA9lBWno+fvT+vkw5hwF89QphUXP+5Oj2oGLp6EA NshnjHW/PN6y45dt3k/T1BnCHOrPDeStn9+GZNABSlkycfWNBLhecf/dOQAWVIdDvIl2 Tydg== X-Gm-Message-State: AC+VfDwJihCCXveWYCMYOM1VYt/ByOOXLu9vqI71DaKrCdbuIrbTgZXf ostJ1/9IXcVDXMl+J22pD9Oz8+rl47LBMccrQDGdFYmzTGVTusHicsXd/xcvtzqMfJWUe4Rnsd4 qeuXCMoau8TyQn+ho3j4= X-Received: by 2002:a17:902:c20c:b0:1ae:29a8:d6d0 with SMTP id 12-20020a170902c20c00b001ae29a8d6d0mr16857819pll.59.1684953906139; Wed, 24 May 2023 11:45:06 -0700 (PDT) X-Google-Smtp-Source: ACHHUZ4S+/NNyjf9BuZLeYz+VLHt/m774O5mFLYM4OPOTApm9k+V8eE768UK04+ifZiP/nTuk/qoWcfszTxHVgK5daQ= X-Received: by 2002:a17:902:c20c:b0:1ae:29a8:d6d0 with SMTP id 12-20020a170902c20c00b001ae29a8d6d0mr16857807pll.59.1684953905669; Wed, 24 May 2023 11:45:05 -0700 (PDT) MIME-Version: 1.0 References: <20230505103102.2912297-1-david.marchand@redhat.com> In-Reply-To: From: David Marchand Date: Wed, 24 May 2023 20:44:54 +0200 Message-ID: Subject: Re: [RFC PATCH] ethdev: advertise flow restore in mbuf To: Ori Kam Cc: "dev@dpdk.org" , "NBU-Contact-Thomas Monjalon (EXTERNAL)" , "i.maximets@ovn.org" , Aman Singh , Yuying Zhang , Matan Azrad , Slava Ovsiienko , Andrew Rybchenko , Ferruh Yigit X-Mimecast-Spam-Score: 0 X-Mimecast-Originator: redhat.com Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: quoted-printable X-BeenThere: dev@dpdk.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: DPDK patches and discussions List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: dev-bounces@dpdk.org Hello Ori, On Wed, May 24, 2023 at 6:00=E2=80=AFPM Ori Kam wrote: > > As reported by Ilya [1], unconditionally calling > > rte_flow_get_restore_info() impacts an application performance for driv= ers > > 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- > > 7f659bfa40de@ovn.org/ > > Signed-off-by: David Marchand > > --- > > 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. > > > > I think your approach has a good base but what happens if > it is not relevant for all flows? In this case your solution will not wor= k. Sorry, I am not following. Could you develop? > > In any case I think there are some issues with the implementation. > > > > --- > > 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 =3D MAX_STRING_LEN; > > size_t cur_len =3D 0; > > + uint64_t restore_info_dynflag; > > > > if (!nb_pkts) > > return; > > + restore_info_dynflag =3D 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 =3D 0; i < nb_pkts; i++) { > > - int ret; > > struct rte_flow_error error; > > struct rte_flow_restore_info info =3D { 0, }; > > > > mb =3D pkts[i]; > > + ol_flags =3D 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 =3D RTE_BE_TO_CPU_16(eth_hdr->ether_type); > > packet_type =3D mb->packet_type; > > is_encapsulation =3D RTE_ETH_IS_TUNNEL_PKT(packet_type); > > - ret =3D rte_flow_get_restore_info(port_id, mb, &info, &er= ror); > > - if (!ret) { > > + if ((ol_flags & restore_info_dynflag) !=3D 0 && > > + rte_flow_get_restore_info(port_id, mb, &i= nfo, > > &error) =3D=3D 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=3D%s - type=3D0x%04x - length=3D%u - > > nb_segs=3D%d", > > mb->pool->name, eth_type, (unsigned int) mb- > > >pkt_len, > > (int)mb->nb_segs); > > - ol_flags =3D mb->ol_flags; > > if (ol_flags & RTE_MBUF_F_RX_RSS_HASH) { > > MKDUMPSTR(print_buf, buf_size, cur_len, > > " - RSS hash=3D0x%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 =3D mlx5_flow_restore_info_register(); > > + if (err) { > > + DRV_LOG(ERR, "Could not register mbuf dynflag for > > rte_flow_get_restore_info"); > > + goto error; > > + } > > err =3D mlx5_alloc_tunnel_hub(sh); > > - if (err) { > > - DRV_LOG(ERR, "mlx5_alloc_tunnel_hub failed err=3D%d", err= ); > > - goto error; > > + if (err) { > > + DRV_LOG(ERR, "mlx5_alloc_tunnel_hub failed > > err=3D%d", err); > > + goto error; > > + } > > } > > if (sh->config.reclaim_mode =3D=3D 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 *erro= r); > > 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 =3D 0; > > } > > > > +static uint64_t mlx5_restore_info_dynflag; > > + > > +int > > +mlx5_flow_restore_info_register(void) > > +{ > > + int err =3D 0; > > + > > + if (mlx5_restore_info_dynflag =3D=3D 0) { > > + if > > (rte_flow_restore_info_dynflag_register(&mlx5_restore_info_dynflag) < 0= ) > > + err =3D 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) > > Why did you change the name? This helper now prepares multiple dynamic flags. If you have a better name.. ? > > > { > > struct mlx5_priv *priv =3D 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 =3D > > rte_flow_dynf_metadata_offs; > > data->flow_meta_port_mask =3D priv->sh- > > >dv_meta_mask; > > } > > + data->mark_flag =3D RTE_MBUF_F_RX_FDIR_ID; > > + if (is_tunnel_offload_active(dev)) > > + data->mark_flag |=3D 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 =3D RTE_MBUF_F_RX_FDIR | > > RTE_MBUF_F_RX_FDIR_ID; > > > > - if (!is_tunnel_offload_active(dev)) { > > - info->flags =3D 0; > > - return 0; > > - } > > - > > + if ((ol_flags & mlx5_restore_info_dynflag) =3D=3D 0) > > + goto err; > > if ((ol_flags & mask) !=3D mask) > > goto err; > > tble =3D 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 |=3D RTE_MBUF_F_RX_FDIR; > > if (mark !=3D RTE_BE32(MLX5_FLOW_MARK_DEFAULT)) { > > - pkt->ol_flags |=3D RTE_MBUF_F_RX_FDIR_ID; > > + pkt->ol_flags |=3D rxq->mark_flag; > > pkt->hash.fdir.hi =3D > > 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_tr= igger.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 queue= s. > > */ > > 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 > > #include > > > > +#include > > #include > > #include > > > > @@ -135,12 +136,8 @@ sfc_dp_ft_ctx_id_register(void) > > .size =3D sizeof(uint8_t), > > .align =3D __alignof__(uint8_t), > > }; > > - static const struct rte_mbuf_dynflag ft_ctx_id_valid =3D { > > - .name =3D "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 =3D rte_mbuf_dynflag_register(&ft_ctx_id_valid); > > - if (flag < 0) { > > + if (rte_flow_restore_info_dynflag_register(&sfc_dp_ft_ctx_id_vali= d) < > > 0) { > > SFC_GENERIC_LOG(ERR, "%s() failed to register ft_ctx_id > > dynflag", > > __func__); > > return -1; > > } > > > > sfc_dp_ft_ctx_id_offset =3D field_offset; > > - sfc_dp_ft_ctx_id_valid =3D 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 =3D { > > + .desc =3D { .name =3D "RTE_MBUF_F_RX_RESTORE_INFO", }, > > +}; > > Why not have this structure in the register function? The dynamic flag value is resolved once, at register time. The accessor below uses this symbol to skip a lookup. > > > + > > +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 =3D=3D 0) { > > + int offset =3D > > rte_mbuf_dynflag_register(&flow_restore_info_dynflag.desc); > > + > > + if (offset < 0) > > + return -1; > > + flow_restore_info_dynflag.value =3D RTE_BIT64(offset); > > + } > > + if (*flag) > > + *flag =3D rte_flow_restore_info_dynflag(); > > + > > + return 0; > > +} > > + > > int > > rte_flow_tunnel_action_decap_release(uint16_t port_id, > > struct rte_flow_action *actions, --=20 David Marchand