From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-wr0-f196.google.com (mail-wr0-f196.google.com [209.85.128.196]) by dpdk.org (Postfix) with ESMTP id 5EC841B2FA for ; Mon, 30 Oct 2017 15:23:35 +0100 (CET) Received: by mail-wr0-f196.google.com with SMTP id j15so12791078wre.8 for ; Mon, 30 Oct 2017 07:23:35 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=6wind-com.20150623.gappssmtp.com; s=20150623; h=date:from:to:cc:subject:message-id:references:mime-version :content-disposition:in-reply-to; bh=W2VfauXFObxeN2sJ9Ok7iR8CD/Inju2ayfTgClRTFmc=; b=M3b9vxLlbT8+1Ykx6AarM70mQ/DjNnvkfuSmSr8czONYkBSJAexL0FUhEU7xsTrIRr Gkbj+KmhlQdN2q5wMpzya1bI4Lp2DdhdnmR1MKLIpjuDlHiKnR298UQgwIYnBLYkk7Xm MXF4aV8wodMJkmvzzTeckW1LqXWAn7fBScagtrRBeh0gYWuRg8OZF4wtB1cqhXQqqM7H m5FpVBWwdftWIfGuBqTxUnnLDemF6ePVAwDPqJt0lpD3pLWB2grY+AvuduoSvP9aBWrr VpM/XrT1GYSfV/YmAZtGvJrQX8i3SgC0rUhyZVmJZZsKSyI+ArEBR3KNACTqBtX9lj2x 0iWQ== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:date:from:to:cc:subject:message-id:references :mime-version:content-disposition:in-reply-to; bh=W2VfauXFObxeN2sJ9Ok7iR8CD/Inju2ayfTgClRTFmc=; b=rxLUOeiz2JawHNhn/m6BbW9/6MJI9dmK7+LK1DU/CmX2SoyAKVhl0dQxZDzCy1L8Hd GhvcANHGFaU7yxefv1X3s6A/M1ngZiyYjcsJiFnHypHk1vhCYZqzqAPEa/qwsjCV+Jnd 7OkvayvK3pFunLBjozcoKZjL4V4P+RWQKt5/HwDcNhGWnEuFQPdMeiJtEbjTchKTA7hM aER8SJVX46jNwUIMyvghOWZvtH4nQoI4bYImEi3+NzZo1z6zF6vBDZDnkL48r13pbhVC piS8ktHMas1rBjAVjA1utAOMy4w0d6ncNCNacmDVqZqjq1Y+vnF2z0hE+xdcC1wFg8Vj 3egw== X-Gm-Message-State: AMCzsaWqRL1pTi87ZV6RwomdVPPvljVU8pWUj9wPOKQB7vfussFNsO9S Yo28S/bmj+qTyLLffogd03IPsw== X-Google-Smtp-Source: ABhQp+RekGhAek1ayWw0LIQ0qgUDfREIuIdyLy0Z9hvBRdkvzIGGqqD0x9WtJWHZanYNpxm3x9d1OQ== X-Received: by 10.223.176.14 with SMTP id f14mr7634527wra.129.1509373414896; Mon, 30 Oct 2017 07:23:34 -0700 (PDT) Received: from 6wind.com (host.78.145.23.62.rev.coltfrance.com. [62.23.145.78]) by smtp.gmail.com with ESMTPSA id b15sm14416244wrh.35.2017.10.30.07.23.34 (version=TLS1_2 cipher=ECDHE-RSA-AES128-GCM-SHA256 bits=128/128); Mon, 30 Oct 2017 07:23:34 -0700 (PDT) Date: Mon, 30 Oct 2017 15:23:23 +0100 From: Adrien Mazarguil To: Matan Azrad Cc: dev@dpdk.org, Ophir Munk Message-ID: <20171030142323.GY26782@6wind.com> References: <1508768520-4810-1-git-send-email-ophirmu@mellanox.com> <1509358049-18854-1-git-send-email-matan@mellanox.com> <1509358049-18854-3-git-send-email-matan@mellanox.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <1509358049-18854-3-git-send-email-matan@mellanox.com> Subject: Re: [dpdk-dev] [PATCH v3 2/7] net/mlx4: associate MR to MP in a short function 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: Mon, 30 Oct 2017 14:23:35 -0000 On Mon, Oct 30, 2017 at 10:07:24AM +0000, Matan Azrad wrote: > From: Ophir Munk > > Associate memory region to mempool (on data path) > in a short function. > Handle the less common case of adding a new memory > region to mempool in a separate function. > > Signed-off-by: Ophir Munk Thanks for moving that function out of the way, a few more comments below. By the way, commit log lines can be up to 75 characters wide, only the title line is limited to 50. > --- > drivers/net/mlx4/mlx4.h | 2 ++ > drivers/net/mlx4/mlx4_mr.c | 46 +++++++++++++++++++++++++++++++++++ > drivers/net/mlx4/mlx4_rxtx.c | 57 -------------------------------------------- > drivers/net/mlx4/mlx4_rxtx.h | 33 ++++++++++++++++++++++++- > 4 files changed, 80 insertions(+), 58 deletions(-) > > diff --git a/drivers/net/mlx4/mlx4.h b/drivers/net/mlx4/mlx4.h > index e0a9853..70cf453 100644 > --- a/drivers/net/mlx4/mlx4.h > +++ b/drivers/net/mlx4/mlx4.h > @@ -160,5 +160,7 @@ int mlx4_flow_ctrl_set(struct rte_eth_dev *dev, > /* mlx4_mr.c */ > > struct ibv_mr *mlx4_mp2mr(struct ibv_pd *pd, struct rte_mempool *mp); > +uint32_t mlx4_txq_add_mr(struct txq *txq, struct rte_mempool *mp, > + uint32_t i); > > #endif /* RTE_PMD_MLX4_H_ */ > diff --git a/drivers/net/mlx4/mlx4_mr.c b/drivers/net/mlx4/mlx4_mr.c > index 9700884..ebf6547 100644 > --- a/drivers/net/mlx4/mlx4_mr.c > +++ b/drivers/net/mlx4/mlx4_mr.c > @@ -56,6 +56,7 @@ > #include > > #include "mlx4_utils.h" > +#include "mlx4_rxtx.h" Alphabetical order is preferred, or so I thought. Nothing seems to enforce this in the coding rules, it's only a suggestion. > > struct mlx4_check_mempool_data { > int ret; > @@ -181,3 +182,48 @@ struct ibv_mr * > rte_errno = errno ? errno : EINVAL; > return mr; > } > + > +/** > + * Add memory region (MR) <-> memory pool (MP) association to txq->mp2mr[]. > + * If mp2mr[] is full, remove an entry first. > + * > + * @param txq > + * Pointer to Tx queue structure. > + * @param[in] mp > + * Memory pool for which a memory region lkey must be added Missing "." > + * @param[in] i > + * Index in memory pool (MP) where to add memory region (MR) Same here. > + * > + * @return > + * Added mr->lkey on success, (uint32_t)-1 on failure. > + */ > +uint32_t mlx4_txq_add_mr(struct txq *txq, struct rte_mempool *mp, uint32_t i) Coding style again; return type must be on a separate line for definitions (not declarations). > +{ > + struct ibv_mr *mr; > + > + /* Add a new entry, register MR first. */ > + DEBUG("%p: discovered new memory pool \"%s\" (%p)", > + (void *)txq, mp->name, (void *)mp); > + mr = mlx4_mp2mr(txq->priv->pd, mp); > + if (unlikely(mr == NULL)) { > + DEBUG("%p: unable to configure MR, ibv_reg_mr() failed.", > + (void *)txq); > + return (uint32_t)-1; > + } > + if (unlikely(i == RTE_DIM(txq->mp2mr))) { > + /* Table is full, remove oldest entry. */ > + DEBUG("%p: MR <-> MP table full, dropping oldest entry.", > + (void *)txq); > + --i; > + claim_zero(ibv_dereg_mr(txq->mp2mr[0].mr)); > + memmove(&txq->mp2mr[0], &txq->mp2mr[1], > + (sizeof(txq->mp2mr) - sizeof(txq->mp2mr[0]))); > + } > + /* Store the new entry. */ > + txq->mp2mr[i].mp = mp; > + txq->mp2mr[i].mr = mr; > + txq->mp2mr[i].lkey = mr->lkey; > + DEBUG("%p: new MR lkey for MP \"%s\" (%p): 0x%08" PRIU32, > + (void *)txq, mp->name, (void *)mp, txq->mp2mr[i].lkey); > + return txq->mp2mr[i].lkey; > +} > diff --git a/drivers/net/mlx4/mlx4_rxtx.c b/drivers/net/mlx4/mlx4_rxtx.c > index 4f899ff..f89df46 100644 > --- a/drivers/net/mlx4/mlx4_rxtx.c > +++ b/drivers/net/mlx4/mlx4_rxtx.c > @@ -239,63 +239,6 @@ struct pv { > } > > /** > - * Get memory region (MR) <-> memory pool (MP) association from txq->mp2mr[]. > - * Add MP to txq->mp2mr[] if it's not registered yet. If mp2mr[] is full, > - * remove an entry first. > - * > - * @param txq > - * Pointer to Tx queue structure. > - * @param[in] mp > - * Memory pool for which a memory region lkey must be returned. > - * > - * @return > - * mr->lkey on success, (uint32_t)-1 on failure. > - */ > -uint32_t > -mlx4_txq_mp2mr(struct txq *txq, struct rte_mempool *mp) > -{ > - unsigned int i; > - struct ibv_mr *mr; > - > - for (i = 0; (i != RTE_DIM(txq->mp2mr)); ++i) { > - if (unlikely(txq->mp2mr[i].mp == NULL)) { > - /* Unknown MP, add a new MR for it. */ > - break; > - } > - if (txq->mp2mr[i].mp == mp) { > - assert(txq->mp2mr[i].lkey != (uint32_t)-1); > - assert(txq->mp2mr[i].mr->lkey == txq->mp2mr[i].lkey); > - return txq->mp2mr[i].lkey; > - } > - } > - /* Add a new entry, register MR first. */ > - DEBUG("%p: discovered new memory pool \"%s\" (%p)", > - (void *)txq, mp->name, (void *)mp); > - mr = mlx4_mp2mr(txq->priv->pd, mp); > - if (unlikely(mr == NULL)) { > - DEBUG("%p: unable to configure MR, ibv_reg_mr() failed.", > - (void *)txq); > - return (uint32_t)-1; > - } > - if (unlikely(i == RTE_DIM(txq->mp2mr))) { > - /* Table is full, remove oldest entry. */ > - DEBUG("%p: MR <-> MP table full, dropping oldest entry.", > - (void *)txq); > - --i; > - claim_zero(ibv_dereg_mr(txq->mp2mr[0].mr)); > - memmove(&txq->mp2mr[0], &txq->mp2mr[1], > - (sizeof(txq->mp2mr) - sizeof(txq->mp2mr[0]))); > - } > - /* Store the new entry. */ > - txq->mp2mr[i].mp = mp; > - txq->mp2mr[i].mr = mr; > - txq->mp2mr[i].lkey = mr->lkey; > - DEBUG("%p: new MR lkey for MP \"%s\" (%p): 0x%08" PRIu32, > - (void *)txq, mp->name, (void *)mp, txq->mp2mr[i].lkey); > - return txq->mp2mr[i].lkey; > -} > - > -/** > * Posts a single work request to a send queue. > * > * @param txq > diff --git a/drivers/net/mlx4/mlx4_rxtx.h b/drivers/net/mlx4/mlx4_rxtx.h > index 7d67748..b32fcc9 100644 > --- a/drivers/net/mlx4/mlx4_rxtx.h > +++ b/drivers/net/mlx4/mlx4_rxtx.h > @@ -36,6 +36,7 @@ > > #include > #include > +#include > > /* Verbs headers do not support -pedantic. */ > #ifdef PEDANTIC > @@ -165,7 +166,6 @@ int mlx4_rx_queue_setup(struct rte_eth_dev *dev, uint16_t idx, > > /* mlx4_rxtx.c */ > > -uint32_t mlx4_txq_mp2mr(struct txq *txq, struct rte_mempool *mp); > uint16_t mlx4_tx_burst(void *dpdk_txq, struct rte_mbuf **pkts, > uint16_t pkts_n); > uint16_t mlx4_rx_burst(void *dpdk_rxq, struct rte_mbuf **pkts, > @@ -182,4 +182,35 @@ int mlx4_tx_queue_setup(struct rte_eth_dev *dev, uint16_t idx, > const struct rte_eth_txconf *conf); > void mlx4_tx_queue_release(void *dpdk_txq); > > + Extra empty line, however see below. > +/** > + * Get memory region (MR) <-> memory pool (MP) association from txq->mp2mr[]. > + * Call mlx4_txq_add_mr() if MP is not registered yet. > + * > + * @param txq > + * Pointer to Tx queue structure. > + * @param[in] mp > + * Memory pool for which a memory region lkey must be returned. > + * > + * @return > + * mr->lkey on success, (uint32_t)-1 on failure. > + */ > +static inline uint32_t > +mlx4_txq_mp2mr(struct txq *txq, struct rte_mempool *mp) > +{ > + unsigned int i; > + > + for (i = 0; (i != RTE_DIM(txq->mp2mr)); ++i) { > + if (unlikely(txq->mp2mr[i].mp == NULL)) { > + /* Unknown MP, add a new MR for it. */ > + break; > + } > + if (txq->mp2mr[i].mp == mp) { > + assert(txq->mp2mr[i].lkey != (uint32_t)-1); > + assert(txq->mp2mr[i].mr->lkey == txq->mp2mr[i].lkey); > + return txq->mp2mr[i].lkey; > + } > + } > + return mlx4_txq_add_mr(txq, mp, i); > +} I'm OK with keeping this function as a static inline defined in a header file, however I think mlx4_rxtx.h is not the right place for it given the remaining MR-handling functions are declared by mlx4.h. You should move it to that file as well (likely below mlx4_txq_add_mr()). Note: RTE_DIM() and unlikely() require rte_common.h, NULL requires stddef.h. While there, I think you should drop both assert() statements and the reliance on assert.h because such statements shouldn't be part of a static inline function not supposed to fail. Let's assume mp2mr[] is correctly updated upstream. (Also missing empty line here.) > #endif /* MLX4_RXTX_H_ */ > -- > 1.8.3.1 > -- Adrien Mazarguil 6WIND