From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from dpdk.org (dpdk.org [92.243.14.124]) by inbox.dpdk.org (Postfix) with ESMTP id E95D6A04DE; Fri, 30 Oct 2020 13:51:22 +0100 (CET) Received: from [92.243.14.124] (localhost [127.0.0.1]) by dpdk.org (Postfix) with ESMTP id 14FB0C9F8; Fri, 30 Oct 2020 13:41:51 +0100 (CET) Received: from mail-io1-f68.google.com (mail-io1-f68.google.com [209.85.166.68]) by dpdk.org (Postfix) with ESMTP id B6A39CA48 for ; Fri, 30 Oct 2020 13:41:24 +0100 (CET) Received: by mail-io1-f68.google.com with SMTP id z5so7375196iob.1 for ; Fri, 30 Oct 2020 05:41:24 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20161025; h=mime-version:references:in-reply-to:from:date:message-id:subject:to :cc; bh=qEtk4NknQo9k+s+r1GfZ1EYDG9wKU6zpishAMq6y4Zc=; b=m3/Gx9q6uwVGKvoC7z92AyGPR3D8CGMqLoZHF8aKPnQ/ZEGXQLnP3aH5BRTDHXOMZ6 LyWp+uJ/lZ6unUF3+eUTDGVtujWifTFasDndWODR+a3In/GsQT/jJ3R60bqdlLxxRTsJ 7hRCfeGmiUVFeLh6uODbl7hnS/fHC2vrH9np/3qzQz/MPgLUetndhCkxYTH5W63947Op Y1pOttHE73aTaUoZjkMbL3O74HIXlVzfeKmHT9v2EXEIvpgUhYqh1yq69mE9rMx1w8EM NO4OmpPp3MRwMZrtVg1WCj31NiCly6ctZWp3xAlB8NTu852e6ESavq1PKIycWY1YQwiv bGUw== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:mime-version:references:in-reply-to:from:date :message-id:subject:to:cc; bh=qEtk4NknQo9k+s+r1GfZ1EYDG9wKU6zpishAMq6y4Zc=; b=Txu1+syKNj3HVZB2l2kDP0KoznD66AvSspLixFEPyggu4V0t9CysQn9uFJEDw+QZEe dLdY4ioCp70tKG9SFjWDTzNq8nyetRQURX+yPPxYSMcT+xDQAIpH4DcWpCpZ9fLYuVb2 Ti2zSPrkPrilMofdNewMZqmdM455+2BS03EgFkikeaSjWfqCisIxioFpuiJ2zbhg94qx vhlExTYFZLdMwPLMdmUboE266+Drr6thwKlbPMg31yfWmkfnHMaj5nUSVjnCZ3wgJEoz UePKZ1fVpf8CY5ONN4+pmEUYerv+sb8CkzbZQShUTuOhmtjQNzabNmbxUpgaJmqDmSG/ mPsg== X-Gm-Message-State: AOAM533Lf4o6BPqOffPfpbjOsr8llIMmV5IGnrhOCJ/p5NyVzf0W5u8L AQ0n/ErconIkIBPE16BxmMlDaSPHBQ/UfpDXMAk= X-Google-Smtp-Source: ABdhPJyeIxkmPuE3/CyitqD1aig9Aixq6a4jeim2mW+M4Mfi51oTsoplRHJcvoet+KoInEoShjb4/XkzOo+jgvkX4zQ= X-Received: by 2002:a5d:9615:: with SMTP id w21mr1643780iol.59.1604061683066; Fri, 30 Oct 2020 05:41:23 -0700 (PDT) MIME-Version: 1.0 References: <20201029092751.3837177-1-thomas@monjalon.net> <20201029092751.3837177-11-thomas@monjalon.net> <69cd6699-290f-102f-038a-f9fdde5874a1@oktetlabs.ru> In-Reply-To: From: Jerin Jacob Date: Fri, 30 Oct 2020 18:11:06 +0530 Message-ID: To: Slava Ovsiienko Cc: Andrew Rybchenko , NBU-Contact-Thomas Monjalon , "dev@dpdk.org" , "ferruh.yigit@intel.com" , "david.marchand@redhat.com" , "bruce.richardson@intel.com" , "olivier.matz@6wind.com" , "jerinj@marvell.com" , Nithin Dabilpuram , Kiran Kumar K , Ray Kinsella , Neil Horman Content-Type: text/plain; charset="UTF-8" Subject: Re: [dpdk-dev] [PATCH 10/15] net/octeontx2: switch timestamp to dynamic mbuf field X-BeenThere: dev@dpdk.org X-Mailman-Version: 2.1.15 Precedence: list List-Id: DPDK patches and discussions List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: dev-bounces@dpdk.org Sender: "dev" On Thu, Oct 29, 2020 at 5:22 PM Slava Ovsiienko wrote: > > Just five cents - exporting the offset (making it global) might have side effect impacting the performance. I agree with Slava. The offset value should be stored in the PMD structure. IMO, We can have an ethdev API to get the offset and store it in PMD's fastpath structures in the slow path to use in fastpath. > Offset might be located in some memory sharing the cacheline with some other variables. > If these variables are writable and are being updated frequently - we might get the cache contention. > I'd prefer to keep all dynamic offsets In the PMD and entirely control memory allocation > attributes for these ones. Hence, exporting is OK, but practical usage in datapath is questionable. > > With best regards, Slava > > > -----Original Message----- > > From: Andrew Rybchenko > > Sent: Thursday, October 29, 2020 13:02 > > To: NBU-Contact-Thomas Monjalon ; dev@dpdk.org > > Cc: ferruh.yigit@intel.com; david.marchand@redhat.com; > > bruce.richardson@intel.com; olivier.matz@6wind.com; jerinj@marvell.com; > > Slava Ovsiienko ; Nithin Dabilpuram > > ; Kiran Kumar K ; > > Ray Kinsella ; Neil Horman > > Subject: Re: [PATCH 10/15] net/octeontx2: switch timestamp to dynamic mbuf > > field > > > > On 10/29/20 12:27 PM, Thomas Monjalon wrote: > > > The mbuf timestamp is moved to a dynamic field in order to allow > > > removal of the deprecated static field. > > > The related mbuf flag is also replaced. > > > > > > Signed-off-by: Thomas Monjalon > > > --- > > > drivers/net/octeontx2/otx2_ethdev.c | 33 > > +++++++++++++++++++++++++++++ > > > drivers/net/octeontx2/otx2_rx.h | 19 ++++++++++++++--- > > > drivers/net/octeontx2/version.map | 7 ++++++ > > > 3 files changed, 56 insertions(+), 3 deletions(-) > > > > > > diff --git a/drivers/net/octeontx2/otx2_ethdev.c > > > b/drivers/net/octeontx2/otx2_ethdev.c > > > index cfb733a4b5..ad95219438 100644 > > > --- a/drivers/net/octeontx2/otx2_ethdev.c > > > +++ b/drivers/net/octeontx2/otx2_ethdev.c > > > @@ -4,6 +4,7 @@ > > > > > > #include > > > > > > +#include > > > #include > > > #include > > > #include > > > @@ -14,6 +15,35 @@ > > > #include "otx2_ethdev.h" > > > #include "otx2_ethdev_sec.h" > > > > > > +uint64_t rte_pmd_octeontx2_timestamp_rx_dynflag; > > > +int rte_pmd_octeontx2_timestamp_dynfield_offset = -1; > > > + > > > +static int > > > +otx2_rx_timestamp_setup(uint16_t flags) { > > > + int timestamp_rx_dynflag_offset; > > > + > > > + if ((flags & NIX_RX_OFFLOAD_TSTAMP_F) == 0) > > > + return 0; > > > + > > > + rte_pmd_octeontx2_timestamp_dynfield_offset = > > rte_mbuf_dynfield_lookup( > > > + RTE_MBUF_DYNFIELD_TIMESTAMP_NAME, NULL); > > > + if (rte_pmd_octeontx2_timestamp_dynfield_offset < 0) { > > > + otx2_err("Failed to lookup timestamp field"); > > > + return -rte_errno; > > > + } > > > + timestamp_rx_dynflag_offset = rte_mbuf_dynflag_lookup( > > > + RTE_MBUF_DYNFLAG_RX_TIMESTAMP_NAME, NULL); > > > + if (timestamp_rx_dynflag_offset < 0) { > > > + otx2_err("Failed to lookup Rx timestamp flag"); > > > + return -rte_errno; > > > + } > > > + rte_pmd_octeontx2_timestamp_rx_dynflag = > > > + RTE_BIT64(timestamp_rx_dynflag_offset); > > > + > > > + return 0; > > > +} > > > + > > > static inline uint64_t > > > nix_get_rx_offload_capa(struct otx2_eth_dev *dev) { @@ -1874,6 > > > +1904,9 @@ otx2_nix_configure(struct rte_eth_dev *eth_dev) > > > dev->tx_offload_flags |= nix_tx_offload_flags(eth_dev); > > > dev->rss_info.rss_grps = NIX_RSS_GRPS; > > > > > > + if (otx2_rx_timestamp_setup(dev->rx_offload_flags) != 0) > > > + goto fail_offloads; > > > + > > > nb_rxq = RTE_MAX(data->nb_rx_queues, 1); > > > nb_txq = RTE_MAX(data->nb_tx_queues, 1); > > > > > > diff --git a/drivers/net/octeontx2/otx2_rx.h > > > b/drivers/net/octeontx2/otx2_rx.h index 61a5c436dd..6981edce82 100644 > > > --- a/drivers/net/octeontx2/otx2_rx.h > > > +++ b/drivers/net/octeontx2/otx2_rx.h > > > @@ -63,6 +63,18 @@ union mbuf_initializer { > > > uint64_t value; > > > }; > > > > > > +/* variables are exported because this file is included in other > > > +drivers */ extern uint64_t rte_pmd_octeontx2_timestamp_rx_dynflag; > > > +extern int rte_pmd_octeontx2_timestamp_dynfield_offset; > > > + > > > +static inline rte_mbuf_timestamp_t * > > > +otx2_timestamp_dynfield(struct rte_mbuf *mbuf) { > > > + return RTE_MBUF_DYNFIELD(mbuf, > > > + rte_pmd_octeontx2_timestamp_dynfield_offset, > > > + rte_mbuf_timestamp_t *); > > > +} > > > + > > > > May be ethdev should provide the inline function? > Just five cents - exporting the offset (making it global) might have side effect impacting the performance. > Offset might be located in some memory sharing the cacheline with some other variables. > If these variables are writable and are being updated frequently - we might get the cache contention. > I'd prefer to keep all dynamic offsets In the PMD and entirely control memory allocation > attributes for these ones. Hence, exporting/inline function is possible, > but practical usage, say, in datapath, is questionable. > > With best regards, Slava >