From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-ua1-f68.google.com (mail-ua1-f68.google.com [209.85.222.68]) by dpdk.org (Postfix) with ESMTP id D879E1B1F4 for ; Wed, 9 Jan 2019 11:05:46 +0100 (CET) Received: by mail-ua1-f68.google.com with SMTP id j3so2236694uap.3 for ; Wed, 09 Jan 2019 02:05:46 -0800 (PST) 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=6iQ5TBJBLyJui3uEOioWLqyiidDNO//6ReNQdqg+Gc4=; b=IuTai/RLUv5JLTq3A/Hq4d6WxGTyxjIiBPkTMIih/ga+9Aj3QIkXTCUdFG9ON1cKqA wk6fEMAS7gf5dwA97ivGSp3uPt8wHXEVpUwMfwy8mmJWQeXN4Z3NkqVnwir4Xm8pndLX MDbfb7HrjiK/RARGvXrvJDDSGHyoPP+MHtJ9RDaxkM+QoRHkohqhdWMKvKDImLWRUO8z eR3iNjOuhfhhdFG0NRL1qMrZFwS3ghit9IRI7iYqGK1FCXbi//BGKJuE0+aEW8Fhq0tI 6t6/M6Fm4AsLADn5FuPpKGTCYggA8JatWy3c2u2aYQw5aCFVJ80lDAOk90XHbQmk+xw7 lE+A== X-Gm-Message-State: AJcUukfp+QNjEF+haE9P3fLnQNjGEeU6qyuqvytvoaYepu2Q5skTOhvX nsZ0lf47gsjrziePGOpJsb6d1aiTKfDSNqi4exFBNA== X-Google-Smtp-Source: ALg8bN77OR5thQpxm0z4yX/JqsVOkqEcb7czek9vGKKTLk76CbrG95PWLJckuqmu1QpfMeyYw+vWMhvkhYEFt4bSRmY= X-Received: by 2002:ab0:4121:: with SMTP id j30mr1833271uad.65.1547028346264; Wed, 09 Jan 2019 02:05:46 -0800 (PST) MIME-Version: 1.0 References: <20190109085426.39965-1-yskoh@mellanox.com> <20190109095205.us53xaocvokx4jog@glumotte.dev.6wind.com> <45D5A06B-4014-428E-A588-6E188C87A5AA@mellanox.com> In-Reply-To: <45D5A06B-4014-428E-A588-6E188C87A5AA@mellanox.com> From: David Marchand Date: Wed, 9 Jan 2019 11:05:35 +0100 Message-ID: To: Yongseok Koh Cc: Olivier Matz , Shahaf Shuler , "dev@dpdk.org" , "stable@dpdk.org" Content-Type: text/plain; charset="UTF-8" X-Content-Filtered-By: Mailman/MimeDel 2.1.15 Subject: Re: [dpdk-dev] [PATCH] net/mlx5: fix instruction hotspot on replenishing Rx buffer 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: , X-List-Received-Date: Wed, 09 Jan 2019 10:05:47 -0000 On Wed, Jan 9, 2019 at 10:56 AM Yongseok Koh wrote: > > > On Jan 9, 2019, at 1:52 AM, Olivier Matz wrote: > > > > On Wed, Jan 09, 2019 at 10:38:07AM +0100, David Marchand wrote: > >> On Wed, Jan 9, 2019 at 9:54 AM Yongseok Koh wrote: > >> > >>> On replenishing Rx buffers for vectorized Rx, mbuf->buf_addr isn't > needed > >>> to be accessed as it is static and easily calculated from the mbuf > address. > >>> Accessing the mbuf content causes unnecessary load stall and it is > worsened > >>> on ARM. > >>> > >>> Fixes: 545b884b1da3 ("net/mlx5: fix buffer address posting in SSE Rx") > >>> Cc: stable@dpdk.org > >>> > >>> Signed-off-by: Yongseok Koh > >>> --- > >>> drivers/net/mlx5/mlx5_rxtx_vec.h | 8 ++++++-- > >>> 1 file changed, 6 insertions(+), 2 deletions(-) > >>> > >>> diff --git a/drivers/net/mlx5/mlx5_rxtx_vec.h > >>> b/drivers/net/mlx5/mlx5_rxtx_vec.h > >>> index fda7004e2d..ced5547307 100644 > >>> --- a/drivers/net/mlx5/mlx5_rxtx_vec.h > >>> +++ b/drivers/net/mlx5/mlx5_rxtx_vec.h > >>> @@ -102,8 +102,12 @@ mlx5_rx_replenish_bulk_mbuf(struct mlx5_rxq_data > >>> *rxq, uint16_t n) > >>> return; > >>> } > >>> for (i = 0; i < n; ++i) { > >>> - wq[i].addr = > rte_cpu_to_be_64((uintptr_t)elts[i]->buf_addr > >>> + > >>> - RTE_PKTMBUF_HEADROOM); > >>> + uintptr_t buf_addr = > >>> + (uintptr_t)elts[i] + sizeof(struct rte_mbuf) + > >>> + rte_pktmbuf_priv_size(rxq->mp) + > >>> RTE_PKTMBUF_HEADROOM; > >>> + > >>> + assert(buf_addr == (uintptr_t)elts[i]->buf_addr); > >>> + wq[i].addr = rte_cpu_to_be_64(buf_addr); > >>> /* If there's only one MR, no need to replace LKey in > WQE. > >>> */ > >>> if (unlikely(mlx5_mr_btree_len(&rxq->mr_ctrl.cache_bh) > > >>> 1)) > >>> wq[i].lkey = mlx5_rx_mb2mr(rxq, elts[i]); > >>> -- > >>> 2.11.0 > >>> > >>> > >> How about having a macro / inline in the mbuf api to get this > information > >> in a consistent/unique way ? > >> I can see we have this calculation at least in rte_pktmbuf_init() and > >> rte_pktmbuf_detach(). > > > > Agree. Maybe rte_mbuf_default_buf_addr(m) ? > > I'm also okay to add. Will come up with a new patch. > > > Side note, is the assert() correct in the patch? I'd say there's a > > difference of RTE_PKTMBUF_HEADROOM between the 2 values. > > Oops, my fault. Thanks for the catch, you saved a crash. :-) > Is this assert really necessary if we have a common macro ? I was under the impression that this assert is there to catch misalignement between the mbuf api and the driver. -- David Marchand