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 3465BA0C46; Mon, 27 Sep 2021 18:14:37 +0200 (CEST) Received: from [217.70.189.124] (localhost [127.0.0.1]) by mails.dpdk.org (Postfix) with ESMTP id B0394410DC; Mon, 27 Sep 2021 18:14:36 +0200 (CEST) Received: from mail-il1-f178.google.com (mail-il1-f178.google.com [209.85.166.178]) by mails.dpdk.org (Postfix) with ESMTP id CD66D40E3C; Mon, 27 Sep 2021 18:14:35 +0200 (CEST) Received: by mail-il1-f178.google.com with SMTP id q14so19887896ils.5; Mon, 27 Sep 2021 09:14:35 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20210112; h=mime-version:references:in-reply-to:from:date:message-id:subject:to :cc:content-transfer-encoding; bh=yaNSNIUe8O7mSwO8KqVjlcD0DTCxvCD0o5CCgiQ4plw=; b=U4Xl09QDNlC+COpydORkpii81713KMLvZQ5e7D7amaG2Y1hdQzGsifcdczcJ7/HJGL QQ6LXmWAO7UpsEcpJiA35xw+9w4Sjt8EdcfalbfmrRMRLSgSgIC+dfYuaxDCOvX45btm +RSKCpy0cPogHQVetF40EypCYa+2Ezcdey3LX7svXTVvhcCWmlppnCLpIty9sv5QfsNx 7ITcRR8wThjLBAnFI1Nz6/57TNbWuxMYdxU7SppQl1U6vnJrfTjyKf6M8JVErKIahsJZ KPcNuueBhnXLnOt1a4AIxctChgRNdZ3uSCC6xkl11YnD0XCzY+RLzMeHHpg/wQeOqAlt bqtw== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; h=x-gm-message-state:mime-version:references:in-reply-to:from:date :message-id:subject:to:cc:content-transfer-encoding; bh=yaNSNIUe8O7mSwO8KqVjlcD0DTCxvCD0o5CCgiQ4plw=; b=gojULgWfx1IX4cKYnLBh2nrirP3div0LlC8sTae6sltGaG8FRIZtsuIIY+e+uO2ODX 1c2EHg40XtvV6wBVI7od5rWKYgppGUdLhxJsR8bmn0avz7LpVGQo2J/7i9MiFq1p2xkc mN7ZruHkQJV1wIo9S0YeEVNaXmwC6X68vrYGoK0z4G6F2X9Q6wR/rlnHGv3dKfWOzWuk 9CxAtYXewDEyvWz41DoxeeldTxvJoFznqRZZJTi9sHGG6htbSBSbBoOvNMlSxNLdiMY0 8SNSAg4Teu76PCVnAASwzmZMC6cDCQ4ua/kolYFdiP2d1Vx6KzE4fVr4BET9XWS9L9LL z1Aw== X-Gm-Message-State: AOAM531FIvif4YdPtRi2rq1LLGtPTng6INiZIfid1essF6+Lm6T1Ic6O 7jQThliaP1F1hWdKn99BnapiS0xVGeiYppt4ou8EbcvS5YE= X-Google-Smtp-Source: ABdhPJwTmFWS0yzV3hOOJqu+Fr/HaJc9wWj/MbVNH9AwBjl/AGJ8CpidHcJAM8g35zMW7zyiMXZtVq9w7h4WiwosWdU= X-Received: by 2002:a05:6e02:ef4:: with SMTP id j20mr668499ilk.294.1632759274984; Mon, 27 Sep 2021 09:14:34 -0700 (PDT) MIME-Version: 1.0 References: <20210820162834.12544-1-konstantin.ananyev@intel.com> In-Reply-To: From: Jerin Jacob Date: Mon, 27 Sep 2021 21:44:08 +0530 Message-ID: To: "Ananyev, Konstantin" Cc: dpdk-dev , Thomas Monjalon , "Yigit, Ferruh" , Andrew Rybchenko , "Yang, Qiming" , "Zhang, Qi Z" , "Xing, Beilei" , "techboard@dpdk.org" Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: quoted-printable Subject: Re: [dpdk-dev] [RFC 0/7] hide eth dev related structures 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 Sender: "dev" On Wed, Sep 22, 2021 at 8:38 PM Ananyev, Konstantin wrote: > > > > > Hi Jerin, > > > > > > > > NOTE: This is just an RFC to start further discussion and collect= the feedback. > > > > > Due to significant amount of work, changes required are applied o= nly to two > > > > > PMDs so far: net/i40e and net/ice. > > > > > So to build it you'll need to add: > > > > > -Denable_drivers=3D'common/*,mempool/*,net/ice,net/i40e' > > > > > to your config options. > > > > > > > > > > > > > > That approach was selected to avoid(/minimize) possible performan= ce losses. > > > > > > > > > > So far I done only limited amount functional and performance test= ing. > > > > > Didn't spot any functional problems, and performance numbers > > > > > remains the same before and after the patch on my box (testpmd, m= acswap fwd). > > > > > > > > > > > > Based on testing on octeonxt2. We see some regression in testpmd an= d > > > > bit on l3fwd too. > > > > > > > > Without patch: 73.5mpps/core in testpmd iofwd > > > > With out patch: 72 5mpps/core in testpmd iofwd > > > > > > > > Based on my understanding it is due to additional indirection. > > > > > > From your patch below, it looks like not actually additional indirect= ion, > > > but extra memory dereference - func and dev pointers are now stored > > > at different places. > > > > Yup. I meant the same. We are on the same page. > > > > > Plus the fact that now we dereference rte_eth_devices[] > > > data inside PMD function. Which probably prevents compiler and CPU to= load > > > rte_eth_devices[port_id].data and rte_eth_devices[port_id]. pre_tx_b= urst_cbs[queue_id] > > > in advance before calling actual RX/TX function. > > > > Yes. > > > > > About your approach: I don=E2=80=99t mind to add extra opaque 'void *= data' pointer, > > > but would prefer not to expose callback invocations code into inline = function. > > > Main reason for that - I think it still need to be reworked to allow = adding/removing > > > callbacks without stopping the device. Something similar to what was = done for cryptodev > > > callbacks. To be able to do that in future without another ABI breaka= ge callbacks related part > > > needs to be kept internal. > > > Though what we probably can do: add two dynamic arrays of opaque poin= ters to rte_eth_burst_api. > > > One for rx/tx queue data pointers, second for rx/tx callback pointers= . > > > To be more specific, something like: > > > > > > typedef uint16_t (*rte_eth_rx_burst_t)( void *rxq, struct rte_mbuf **= rx_pkts, uint16_t nb_pkts, void *cbs); > > > typedef uint16_t (*rte_eth_tx_burst_t)(void *txq, struct rte_mbuf **t= x_pkts, uint16_t nb_pkts, void *cbs); > > > .... > > > > > > struct rte_eth_burst_api { > > > rte_eth_rx_burst_t rx_pkt_burst; > > > /**< PMD receive function. */ > > > rte_eth_tx_burst_t tx_pkt_burst; > > > /**< PMD transmit function. */ > > > rte_eth_tx_prep_t tx_pkt_prepare; > > > /**< PMD transmit prepare function. */ > > > rte_eth_rx_queue_count_t rx_queue_count; > > > /**< Get the number of used RX descriptors. */ > > > rte_eth_rx_descriptor_status_t rx_descriptor_status; > > > /**< Check the status of a Rx descriptor. */ > > > rte_eth_tx_descriptor_status_t tx_descriptor_status; > > > /**< Check the status of a Tx descriptor. */ > > > struct { > > > void **queue_data; /* point to rte_eth_devices[por= t_id].data-> rx_queues */ > > > void **cbs; /* points to rte_eth_d= evices[port_id].post_rx_burst_cbs */ > > > } rx_data, tx_data; > > > } __rte_cache_aligned; > > > > > > static inline uint16_t > > > rte_eth_rx_burst(uint16_t port_id, uint16_t queue_id, > > > struct rte_mbuf **rx_pkts, const uint16_t nb_pkts) > > > { > > > struct rte_eth_burst_api *p; > > > > > > if (port_id >=3D RTE_MAX_ETHPORTS || queue_id >=3D RTE_MAX_QU= EUES_PER_PORT) > > > return 0; > > > > > > p =3D &rte_eth_burst_api[port_id]; > > > return p->rx_pkt_burst(p->rx_data.queue_data[queue_id], rx_pkts= , nb_pkts, p->rx_data.cbs[queue_id]); > > > > > > > > That works. > > > > > > > } > > > > > > Same for TX. > > > > > > If that looks ok to everyone, I'll try to prepare next version based = on that. > > > > > > Looks good to me. > > > > > In theory that should avoid extra dereference problem and even reduce= indirection. > > > As a drawback data->rxq/txq should always be allocated for RTE_MAX_QU= EUES_PER_PORT entries, > > > but I presume that=E2=80=99s not a big deal. > > > > > > As a side question - is there any reason why rte_ethdev_trace_rx_burs= t() is invoked at very last point, > > > while rte_ethdev_trace_tx_burst() after CBs but before actual tx_pkt= _burst()? > > > It would make things simpler if tracng would always be done either on= entrance or exit of rx/tx_burst. > > > > exit is fine. > > > > > > > > > > > > > My suggestion to fix the problem by: > > > > Removing the additional `data` redirection and pull callback functi= on > > > > pointers back > > > > and keep rest as opaque as done in the existing patch like [1] > > > > > > > > I don't believe this has any real implication on future ABI stabili= ty > > > > as we will not be adding > > > > any new item in rte_eth_fp in any way as new features can be added = in slowpath > > > > rte_eth_dev as mentioned in the patch. > > > > Ack > > > > I will happy to test again after the rework and report any performance > > issues if any. > > Thanks Jerin, v2 is out: > https://patches.dpdk.org/project/dpdk/list/?series=3D19084 > Please have a look, when you'll get a chance. Tested the series. Looks good, No performance issue. >