DPDK patches and discussions
 help / color / mirror / Atom feed
* [dpdk-dev] [PATCH] net/mlx5: fix instruction hotspot on replenishing Rx buffer
@ 2019-01-09  8:54 Yongseok Koh
  2019-01-09  9:38 ` David Marchand
                   ` (4 more replies)
  0 siblings, 5 replies; 35+ messages in thread
From: Yongseok Koh @ 2019-01-09  8:54 UTC (permalink / raw)
  To: shahafs; +Cc: dev, stable

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 <yskoh@mellanox.com>
---
 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

^ permalink raw reply	[flat|nested] 35+ messages in thread

* Re: [dpdk-dev] [PATCH] net/mlx5: fix instruction hotspot on replenishing Rx buffer
  2019-01-09  8:54 [dpdk-dev] [PATCH] net/mlx5: fix instruction hotspot on replenishing Rx buffer Yongseok Koh
@ 2019-01-09  9:38 ` David Marchand
  2019-01-09  9:52   ` Olivier Matz
  2019-01-09 13:19 ` [dpdk-dev] [PATCH v2 1/2] mbuf: add function returning default buffer address Yongseok Koh
                   ` (3 subsequent siblings)
  4 siblings, 1 reply; 35+ messages in thread
From: David Marchand @ 2019-01-09  9:38 UTC (permalink / raw)
  To: Yongseok Koh; +Cc: shahafs, dev, stable

On Wed, Jan 9, 2019 at 9:54 AM Yongseok Koh <yskoh@mellanox.com> 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 <yskoh@mellanox.com>
> ---
>  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().


-- 
David Marchand

^ permalink raw reply	[flat|nested] 35+ messages in thread

* Re: [dpdk-dev] [PATCH] net/mlx5: fix instruction hotspot on replenishing Rx buffer
  2019-01-09  9:38 ` David Marchand
@ 2019-01-09  9:52   ` Olivier Matz
  2019-01-09  9:56     ` Yongseok Koh
  0 siblings, 1 reply; 35+ messages in thread
From: Olivier Matz @ 2019-01-09  9:52 UTC (permalink / raw)
  To: David Marchand; +Cc: Yongseok Koh, shahafs, dev, stable

On Wed, Jan 09, 2019 at 10:38:07AM +0100, David Marchand wrote:
> On Wed, Jan 9, 2019 at 9:54 AM Yongseok Koh <yskoh@mellanox.com> 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 <yskoh@mellanox.com>
> > ---
> >  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) ?

Side note, is the assert() correct in the patch? I'd say there's a
difference of RTE_PKTMBUF_HEADROOM between the 2 values.

Olivier

^ permalink raw reply	[flat|nested] 35+ messages in thread

* Re: [dpdk-dev] [PATCH] net/mlx5: fix instruction hotspot on replenishing Rx buffer
  2019-01-09  9:52   ` Olivier Matz
@ 2019-01-09  9:56     ` Yongseok Koh
  2019-01-09 10:05       ` David Marchand
  0 siblings, 1 reply; 35+ messages in thread
From: Yongseok Koh @ 2019-01-09  9:56 UTC (permalink / raw)
  To: Olivier Matz; +Cc: David Marchand, Shahaf Shuler, dev, stable


> On Jan 9, 2019, at 1:52 AM, Olivier Matz <olivier.matz@6wind.com> 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 <yskoh@mellanox.com> 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 <yskoh@mellanox.com>
>>> ---
>>> 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. :-)

Thanks,
Yongseok

^ permalink raw reply	[flat|nested] 35+ messages in thread

* Re: [dpdk-dev] [PATCH] net/mlx5: fix instruction hotspot on replenishing Rx buffer
  2019-01-09  9:56     ` Yongseok Koh
@ 2019-01-09 10:05       ` David Marchand
  2019-01-09 10:11         ` Yongseok Koh
  0 siblings, 1 reply; 35+ messages in thread
From: David Marchand @ 2019-01-09 10:05 UTC (permalink / raw)
  To: Yongseok Koh; +Cc: Olivier Matz, Shahaf Shuler, dev, stable

On Wed, Jan 9, 2019 at 10:56 AM Yongseok Koh <yskoh@mellanox.com> wrote:

>
> > On Jan 9, 2019, at 1:52 AM, Olivier Matz <olivier.matz@6wind.com> 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 <yskoh@mellanox.com> 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 <yskoh@mellanox.com>
> >>> ---
> >>> 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

^ permalink raw reply	[flat|nested] 35+ messages in thread

* Re: [dpdk-dev] [PATCH] net/mlx5: fix instruction hotspot on replenishing Rx buffer
  2019-01-09 10:05       ` David Marchand
@ 2019-01-09 10:11         ` Yongseok Koh
  0 siblings, 0 replies; 35+ messages in thread
From: Yongseok Koh @ 2019-01-09 10:11 UTC (permalink / raw)
  To: David Marchand; +Cc: Olivier Matz, Shahaf Shuler, dev, stable



> On Jan 9, 2019, at 2:05 AM, David Marchand <david.marchand@redhat.com> wrote:
> 
> 
> 
> On Wed, Jan 9, 2019 at 10:56 AM Yongseok Koh <yskoh@mellanox.com> wrote:
> 
> > On Jan 9, 2019, at 1:52 AM, Olivier Matz <olivier.matz@6wind.com> 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 <yskoh@mellanox.com> 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 <yskoh@mellanox.com>
> >>> ---
> >>> 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.

It is still good to have. This can catch corruption of mbuf content which sometimes
happens due to wrong mbuf handling in PMD or potential HW memory corruption.


Thanks,
Yongseok

^ permalink raw reply	[flat|nested] 35+ messages in thread

* [dpdk-dev] [PATCH v2 1/2] mbuf: add function returning default buffer address
  2019-01-09  8:54 [dpdk-dev] [PATCH] net/mlx5: fix instruction hotspot on replenishing Rx buffer Yongseok Koh
  2019-01-09  9:38 ` David Marchand
@ 2019-01-09 13:19 ` Yongseok Koh
  2019-01-09 13:19   ` [dpdk-dev] [PATCH v2 2/2] net/mlx5: fix instruction hotspot on replenishing Rx buffer Yongseok Koh
  2019-01-09 13:46   ` [dpdk-dev] [PATCH v2 1/2] mbuf: add function returning default buffer address David Marchand
  2019-01-10 18:35 ` [dpdk-dev] [PATCH v3 " Yongseok Koh
                   ` (2 subsequent siblings)
  4 siblings, 2 replies; 35+ messages in thread
From: Yongseok Koh @ 2019-01-09 13:19 UTC (permalink / raw)
  To: olivier.matz, shahafs; +Cc: dev, david.marchand

This patch introduces two new functions - rte_mbuf_buf_addr_default() and
rte_mbuf_data_baddr_default().

rte_mbuf_buf_addr_default() reutrns the default buffer address of given
mbuf which comes after mbuf structure and private data.

rte_mbuf_data_baddr_default() returns the default address of mbuf data
taking the headroom into account.

Signed-off-by: Yongseok Koh <yskoh@mellanox.com>
---

v2:
* initial implementation

 lib/librte_mbuf/rte_mbuf.h | 43 ++++++++++++++++++++++++++++++++++++++++---
 1 file changed, 40 insertions(+), 3 deletions(-)

diff --git a/lib/librte_mbuf/rte_mbuf.h b/lib/librte_mbuf/rte_mbuf.h
index bc562dc8a9..6c4c1bd3e8 100644
--- a/lib/librte_mbuf/rte_mbuf.h
+++ b/lib/librte_mbuf/rte_mbuf.h
@@ -788,8 +788,47 @@ rte_mbuf_from_indirect(struct rte_mbuf *mi)
 }
 
 /**
+ * Return the default buffer address of the mbuf.
+ *
+ * @param mb
+ *   The pointer to the mbuf.
+ * @param mp
+ *   The pointer to the mempool of the mbuf.
+ * @return
+ *   The pointer of the mbuf buffer.
+ */
+static inline char *
+rte_mbuf_buf_addr_default(struct rte_mbuf *mb, struct rte_mempool *mp)
+{
+	char *buffer_addr;
+
+	buffer_addr = (char *)mb + sizeof(*mb) + rte_pktmbuf_priv_size(mp);
+	return buffer_addr;
+}
+
+
+/**
+ * Return the default address of the beginning of the mbuf data.
+ *
+ * @param mb
+ *   The pointer to the mbuf.
+ * @return
+ *   The pointer of the beginning of the mbuf data.
+ */
+static inline char *
+rte_mbuf_data_baddr_default(struct rte_mbuf *mb)
+{
+	return rte_mbuf_buf_addr_default(mb, mb->pool) + RTE_PKTMBUF_HEADROOM;
+}
+
+/**
  * Return the buffer address embedded in the given mbuf.
  *
+ * Note that accessing mempool pointer of a mbuf is expensive because the
+ * pointer is stored in the 2nd cache line of mbuf. If mempool is known, it
+ * is better not to reference the mempool pointer in mbuf but calling
+ * rte_mbuf_buf_addr_default() would be more efficient.
+ *
  * @param md
  *   The pointer to the mbuf.
  * @return
@@ -798,9 +837,7 @@ rte_mbuf_from_indirect(struct rte_mbuf *mi)
 static inline char *
 rte_mbuf_to_baddr(struct rte_mbuf *md)
 {
-	char *buffer_addr;
-	buffer_addr = (char *)md + sizeof(*md) + rte_pktmbuf_priv_size(md->pool);
-	return buffer_addr;
+	return rte_mbuf_buf_addr_default(md, md->pool);
 }
 
 /**
-- 
2.11.0

^ permalink raw reply	[flat|nested] 35+ messages in thread

* [dpdk-dev] [PATCH v2 2/2] net/mlx5: fix instruction hotspot on replenishing Rx buffer
  2019-01-09 13:19 ` [dpdk-dev] [PATCH v2 1/2] mbuf: add function returning default buffer address Yongseok Koh
@ 2019-01-09 13:19   ` Yongseok Koh
  2019-01-09 13:46   ` [dpdk-dev] [PATCH v2 1/2] mbuf: add function returning default buffer address David Marchand
  1 sibling, 0 replies; 35+ messages in thread
From: Yongseok Koh @ 2019-01-09 13:19 UTC (permalink / raw)
  To: olivier.matz, shahafs; +Cc: dev, david.marchand, stable

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 <yskoh@mellanox.com>
---

v2:
* use the newly introduced API - rte_mbuf_buf_addr_default()
* fix error in assert

 drivers/net/mlx5/mlx5_rxtx_vec.h | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/drivers/net/mlx5/mlx5_rxtx_vec.h b/drivers/net/mlx5/mlx5_rxtx_vec.h
index fda7004e2d..154934d92d 100644
--- a/drivers/net/mlx5/mlx5_rxtx_vec.h
+++ b/drivers/net/mlx5/mlx5_rxtx_vec.h
@@ -102,8 +102,10 @@ 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);
+		void *buf_addr = rte_mbuf_buf_addr_default(elts[i], rxq->mp);
+
+		assert(buf_addr == elts[i]->buf_addr);
+		wq[i].addr = rte_cpu_to_be_64((uintptr_t)buf_addr + RTE_PKTMBUF_HEADROOM);
 		/* 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

^ permalink raw reply	[flat|nested] 35+ messages in thread

* Re: [dpdk-dev] [PATCH v2 1/2] mbuf: add function returning default buffer address
  2019-01-09 13:19 ` [dpdk-dev] [PATCH v2 1/2] mbuf: add function returning default buffer address Yongseok Koh
  2019-01-09 13:19   ` [dpdk-dev] [PATCH v2 2/2] net/mlx5: fix instruction hotspot on replenishing Rx buffer Yongseok Koh
@ 2019-01-09 13:46   ` David Marchand
  2019-01-10  1:39     ` Rami Rosen
  2019-01-10 18:22     ` Yongseok Koh
  1 sibling, 2 replies; 35+ messages in thread
From: David Marchand @ 2019-01-09 13:46 UTC (permalink / raw)
  To: Yongseok Koh; +Cc: Olivier Matz, Shahaf Shuler, dev

On Wed, Jan 9, 2019 at 2:19 PM Yongseok Koh <yskoh@mellanox.com> wrote:

> This patch introduces two new functions - rte_mbuf_buf_addr_default() and
> rte_mbuf_data_baddr_default().
>
> rte_mbuf_buf_addr_default() reutrns the default buffer address of given
> mbuf which comes after mbuf structure and private data.
>

The buffer address should always be the same for a given mbuf, there is no
"default" value for it, there is only one value.
So for me s/rte_mbuf_buf_addr_default/rte_mbuf_buf_addr/g.


> rte_mbuf_data_baddr_default() returns the default address of mbuf data
> taking the headroom into account.
>

Or just rte_mbuf_data_addr_default() ?


> Signed-off-by: Yongseok Koh <yskoh@mellanox.com>
>

Those are new functions, they should go through the EXPERIMENTAL api
marking process.


-- 
David Marchand

^ permalink raw reply	[flat|nested] 35+ messages in thread

* Re: [dpdk-dev] [PATCH v2 1/2] mbuf: add function returning default buffer address
  2019-01-09 13:46   ` [dpdk-dev] [PATCH v2 1/2] mbuf: add function returning default buffer address David Marchand
@ 2019-01-10  1:39     ` Rami Rosen
  2019-01-10 18:18       ` Yongseok Koh
  2019-01-10 18:22     ` Yongseok Koh
  1 sibling, 1 reply; 35+ messages in thread
From: Rami Rosen @ 2019-01-10  1:39 UTC (permalink / raw)
  To: David Marchand; +Cc: Yongseok Koh, Olivier Matz, Shahaf Shuler, dev

Hi, Yongseok,

Maybe you should consider using the pool member of the mbuf, instead of
passing it as a parameter for rte_mbuf_buf_addr_default().
The pool member of mbuf indicates the pool from which it was allocated.
See
http://git.dpdk.org/dpdk/tree/lib/librte_mbuf/rte_mbuf.h#n631

And actually the is exactly what you do in the second method,
rte_mbuf_data_baddr_default(),
when invoking the first,  this is also kind of non consistency.

>> David Marchand wrote:

> So for me s/rte_mbuf_buf_addr_default/rte_mbuf_buf_addr/g


> Or just rte_mbuf_data_addr_default() ?

+ 1

>> David Marchand wrote:

>Those are new functions, they should go through the EXPERIMENTAL api

>marking process.


+1

Regards,
Rami Rosen

בתאריך יום ד׳, 9 בינו׳ 2019, 15:47, מאת David Marchand <
david.marchand@redhat.com>:

> On Wed, Jan 9, 2019 at 2:19 PM Yongseok Koh <yskoh@mellanox.com> wrote:
>
> > This patch introduces two new functions - rte_mbuf_buf_addr_default() and
> > rte_mbuf_data_baddr_default().
> >
> > rte_mbuf_buf_addr_default() reutrns the default buffer address of given
> > mbuf which comes after mbuf structure and private data.
> >
>
> The buffer address should always be the same for a given mbuf, there is no
> "default" value for it, there is only one value.
> So for me s/rte_mbuf_buf_addr_default/rte_mbuf_buf_addr/g.
>
>
> > rte_mbuf_data_baddr_default() returns the default address of mbuf data
> > taking the headroom into account.
> >
>
> Or just rte_mbuf_data_addr_default() ?
>
>
> > Signed-off-by: Yongseok Koh <yskoh@mellanox.com>
> >
>
> Those are new functions, they should go through the EXPERIMENTAL api
> marking process.
>
>
> --
> David Marchand
>

^ permalink raw reply	[flat|nested] 35+ messages in thread

* Re: [dpdk-dev] [PATCH v2 1/2] mbuf: add function returning default buffer address
  2019-01-10  1:39     ` Rami Rosen
@ 2019-01-10 18:18       ` Yongseok Koh
  0 siblings, 0 replies; 35+ messages in thread
From: Yongseok Koh @ 2019-01-10 18:18 UTC (permalink / raw)
  To: Rami Rosen; +Cc: David Marchand, Olivier Matz, Shahaf Shuler, dev


> On Jan 9, 2019, at 5:39 PM, Rami Rosen <roszenrami@gmail.com> wrote:
> 
> Hi, Yongseok,
> 
> Maybe you should consider using the pool member of the mbuf, instead of passing it as a parameter for rte_mbuf_buf_addr_default().
> The pool member of mbuf indicates the pool from which it was allocated.
> See 
> http://git.dpdk.org/dpdk/tree/lib/librte_mbuf/rte_mbuf.h#n631
> 
> And actually the is exactly what you do in the second method, rte_mbuf_data_baddr_default(), when invoking the first,  this is also kind of non consistency.

Original intention is to not access mbuf struct to avoid any potential load stall.
I've mentioned it in the comments.
For example, on Rx, mbuf mempool is known to PMD and there's only one mempool,
it doesn't need to pull the pointer from mbuf struct.

So, idea is to know the initial buf_addr without accessing mbuf struct.

Thanks,
Yongseok

> >> David Marchand wrote: 
> > So for me s/rte_mbuf_buf_addr_default/rte_mbuf_buf_addr/g
> > Or just rte_mbuf_data_addr_default() ?
> + 1
> >> David Marchand wrote: 
> >Those are new functions, they should go through the EXPERIMENTAL api
> >marking process.
> 
> +1 
> 
> Regards,
> Rami Rosen
> 
> בתאריך יום ד׳, 9 בינו׳ 2019, 15:47, מאת David Marchand <david.marchand@redhat.com>:
> On Wed, Jan 9, 2019 at 2:19 PM Yongseok Koh <yskoh@mellanox.com> wrote:
> 
> > This patch introduces two new functions - rte_mbuf_buf_addr_default() and
> > rte_mbuf_data_baddr_default().
> >
> > rte_mbuf_buf_addr_default() reutrns the default buffer address of given
> > mbuf which comes after mbuf structure and private data.
> >
> 
> The buffer address should always be the same for a given mbuf, there is no
> "default" value for it, there is only one value.
> So for me s/rte_mbuf_buf_addr_default/rte_mbuf_buf_addr/g.
> 
> 
> > rte_mbuf_data_baddr_default() returns the default address of mbuf data
> > taking the headroom into account.
> >
> 
> Or just rte_mbuf_data_addr_default() ?
> 
> 
> > Signed-off-by: Yongseok Koh <yskoh@mellanox.com>
> >
> 
> Those are new functions, they should go through the EXPERIMENTAL api
> marking process.
> 
> 
> -- 
> David Marchand


^ permalink raw reply	[flat|nested] 35+ messages in thread

* Re: [dpdk-dev] [PATCH v2 1/2] mbuf: add function returning default buffer address
  2019-01-09 13:46   ` [dpdk-dev] [PATCH v2 1/2] mbuf: add function returning default buffer address David Marchand
  2019-01-10  1:39     ` Rami Rosen
@ 2019-01-10 18:22     ` Yongseok Koh
  1 sibling, 0 replies; 35+ messages in thread
From: Yongseok Koh @ 2019-01-10 18:22 UTC (permalink / raw)
  To: David Marchand; +Cc: Olivier Matz, Shahaf Shuler, dev


On Jan 9, 2019, at 5:46 AM, David Marchand <david.marchand@redhat.com<mailto:david.marchand@redhat.com>> wrote:


On Wed, Jan 9, 2019 at 2:19 PM Yongseok Koh <yskoh@mellanox.com<mailto:yskoh@mellanox.com>> wrote:
This patch introduces two new functions - rte_mbuf_buf_addr_default() and
rte_mbuf_data_baddr_default().

rte_mbuf_buf_addr_default() reutrns the default buffer address of given
mbuf which comes after mbuf structure and private data.

The buffer address should always be the same for a given mbuf, there is no "default" value for it, there is only one value.
So for me s/rte_mbuf_buf_addr_default/rte_mbuf_buf_addr/g.

+1



rte_mbuf_data_baddr_default() returns the default address of mbuf data
taking the headroom into account.

Or just rte_mbuf_data_addr_default() ?

+1



Signed-off-by: Yongseok Koh <yskoh@mellanox.com<mailto:yskoh@mellanox.com>>

Those are new functions, they should go through the EXPERIMENTAL api marking process.

If that's the rule, I'll follow anyway, although these are too obvious to be experimental.

Thanks,
Yongseok

^ permalink raw reply	[flat|nested] 35+ messages in thread

* [dpdk-dev] [PATCH v3 1/2] mbuf: add function returning default buffer address
  2019-01-09  8:54 [dpdk-dev] [PATCH] net/mlx5: fix instruction hotspot on replenishing Rx buffer Yongseok Koh
  2019-01-09  9:38 ` David Marchand
  2019-01-09 13:19 ` [dpdk-dev] [PATCH v2 1/2] mbuf: add function returning default buffer address Yongseok Koh
@ 2019-01-10 18:35 ` Yongseok Koh
  2019-01-10 18:35   ` [dpdk-dev] [PATCH v3 2/2] net/mlx5: fix instruction hotspot on replenishing Rx buffer Yongseok Koh
  2019-01-11  8:14   ` [dpdk-dev] [PATCH v3 1/2] mbuf: add function returning default buffer address Andrew Rybchenko
  2019-01-10 22:40 ` [dpdk-dev] [PATCH v4 " Yongseok Koh
  2019-01-14 21:16 ` [dpdk-dev] [PATCH v5 1/2] mbuf: add function returning " Yongseok Koh
  4 siblings, 2 replies; 35+ messages in thread
From: Yongseok Koh @ 2019-01-10 18:35 UTC (permalink / raw)
  To: olivier.matz, shahafs; +Cc: dev, roszenrami, david.marchand

This patch introduces two new functions - rte_mbuf_buf_addr() and
rte_mbuf_data_addr_default().

rte_mbuf_buf_addr() reutrns the default buffer address of given mbuf which
comes after mbuf structure and private data.

rte_mbuf_data_addr_default() returns the default address of mbuf data
taking the headroom into account.

Signed-off-by: Yongseok Koh <yskoh@mellanox.com>
---

v3:
* rename functions

v2:
* initial implementation

 lib/librte_mbuf/rte_mbuf.h | 43 ++++++++++++++++++++++++++++++++++++++++---
 1 file changed, 40 insertions(+), 3 deletions(-)

diff --git a/lib/librte_mbuf/rte_mbuf.h b/lib/librte_mbuf/rte_mbuf.h
index bc562dc8a9..486566fc28 100644
--- a/lib/librte_mbuf/rte_mbuf.h
+++ b/lib/librte_mbuf/rte_mbuf.h
@@ -788,8 +788,47 @@ rte_mbuf_from_indirect(struct rte_mbuf *mi)
 }
 
 /**
+ * Return the default buffer address of the mbuf.
+ *
+ * @param mb
+ *   The pointer to the mbuf.
+ * @param mp
+ *   The pointer to the mempool of the mbuf.
+ * @return
+ *   The pointer of the mbuf buffer.
+ */
+static inline char * __rte_experimental
+rte_mbuf_buf_addr(struct rte_mbuf *mb, struct rte_mempool *mp)
+{
+	char *buffer_addr;
+
+	buffer_addr = (char *)mb + sizeof(*mb) + rte_pktmbuf_priv_size(mp);
+	return buffer_addr;
+}
+
+
+/**
+ * Return the default address of the beginning of the mbuf data.
+ *
+ * @param mb
+ *   The pointer to the mbuf.
+ * @return
+ *   The pointer of the beginning of the mbuf data.
+ */
+static inline char * __rte_experimental
+rte_mbuf_data_addr_default(struct rte_mbuf *mb)
+{
+	return rte_mbuf_buf_addr(mb, mb->pool) + RTE_PKTMBUF_HEADROOM;
+}
+
+/**
  * Return the buffer address embedded in the given mbuf.
  *
+ * Note that accessing mempool pointer of a mbuf is expensive because the
+ * pointer is stored in the 2nd cache line of mbuf. If mempool is known, it
+ * is better not to reference the mempool pointer in mbuf but calling
+ * rte_mbuf_buf_addr() would be more efficient.
+ *
  * @param md
  *   The pointer to the mbuf.
  * @return
@@ -798,9 +837,7 @@ rte_mbuf_from_indirect(struct rte_mbuf *mi)
 static inline char *
 rte_mbuf_to_baddr(struct rte_mbuf *md)
 {
-	char *buffer_addr;
-	buffer_addr = (char *)md + sizeof(*md) + rte_pktmbuf_priv_size(md->pool);
-	return buffer_addr;
+	return rte_mbuf_buf_addr(md, md->pool);
 }
 
 /**
-- 
2.11.0

^ permalink raw reply	[flat|nested] 35+ messages in thread

* [dpdk-dev] [PATCH v3 2/2] net/mlx5: fix instruction hotspot on replenishing Rx buffer
  2019-01-10 18:35 ` [dpdk-dev] [PATCH v3 " Yongseok Koh
@ 2019-01-10 18:35   ` Yongseok Koh
  2019-01-10 19:10     ` Shahaf Shuler
  2019-01-11  8:14   ` [dpdk-dev] [PATCH v3 1/2] mbuf: add function returning default buffer address Andrew Rybchenko
  1 sibling, 1 reply; 35+ messages in thread
From: Yongseok Koh @ 2019-01-10 18:35 UTC (permalink / raw)
  To: olivier.matz, shahafs; +Cc: dev, roszenrami, david.marchand, stable

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 <yskoh@mellanox.com>
---

v3:
* rte_mbuf_buf_addr_default() -> rte_mbuf_buf_addr()

v2:
* use the newly introduced API - rte_mbuf_buf_addr_default()
* fix error in assert

 drivers/net/mlx5/mlx5_rxtx_vec.h | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/drivers/net/mlx5/mlx5_rxtx_vec.h b/drivers/net/mlx5/mlx5_rxtx_vec.h
index fda7004e2d..989a1fdce5 100644
--- a/drivers/net/mlx5/mlx5_rxtx_vec.h
+++ b/drivers/net/mlx5/mlx5_rxtx_vec.h
@@ -102,8 +102,10 @@ 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);
+		void *buf_addr = rte_mbuf_buf_addr(elts[i], rxq->mp);
+
+		assert(buf_addr == elts[i]->buf_addr);
+		wq[i].addr = rte_cpu_to_be_64((uintptr_t)buf_addr + RTE_PKTMBUF_HEADROOM);
 		/* 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

^ permalink raw reply	[flat|nested] 35+ messages in thread

* Re: [dpdk-dev] [PATCH v3 2/2] net/mlx5: fix instruction hotspot on replenishing Rx buffer
  2019-01-10 18:35   ` [dpdk-dev] [PATCH v3 2/2] net/mlx5: fix instruction hotspot on replenishing Rx buffer Yongseok Koh
@ 2019-01-10 19:10     ` Shahaf Shuler
  0 siblings, 0 replies; 35+ messages in thread
From: Shahaf Shuler @ 2019-01-10 19:10 UTC (permalink / raw)
  To: Yongseok Koh, olivier.matz; +Cc: dev, roszenrami, david.marchand, stable

Thursday, January 10, 2019 8:35 PM, Yongseok Koh:
> Subject: [dpdk-dev] [PATCH v3 2/2] net/mlx5: fix instruction hotspot on
> replenishing Rx buffer
> 
> 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 <yskoh@mellanox.com>

Acked-by: Shahaf Shuler <shahafs@mellanox.com>

> ---
> 
> v3:
> * rte_mbuf_buf_addr_default() -> rte_mbuf_buf_addr()
> 
> v2:
> * use the newly introduced API - rte_mbuf_buf_addr_default()
> * fix error in assert
> 
>  drivers/net/mlx5/mlx5_rxtx_vec.h | 6 ++++--
>  1 file changed, 4 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/net/mlx5/mlx5_rxtx_vec.h
> b/drivers/net/mlx5/mlx5_rxtx_vec.h
> index fda7004e2d..989a1fdce5 100644
> --- a/drivers/net/mlx5/mlx5_rxtx_vec.h
> +++ b/drivers/net/mlx5/mlx5_rxtx_vec.h
> @@ -102,8 +102,10 @@ 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);
> +		void *buf_addr = rte_mbuf_buf_addr(elts[i], rxq->mp);
> +
> +		assert(buf_addr == elts[i]->buf_addr);
> +		wq[i].addr = rte_cpu_to_be_64((uintptr_t)buf_addr +
> +RTE_PKTMBUF_HEADROOM);
>  		/* 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

^ permalink raw reply	[flat|nested] 35+ messages in thread

* [dpdk-dev] [PATCH v4 1/2] mbuf: add function returning default buffer address
  2019-01-09  8:54 [dpdk-dev] [PATCH] net/mlx5: fix instruction hotspot on replenishing Rx buffer Yongseok Koh
                   ` (2 preceding siblings ...)
  2019-01-10 18:35 ` [dpdk-dev] [PATCH v3 " Yongseok Koh
@ 2019-01-10 22:40 ` Yongseok Koh
  2019-01-10 22:40   ` [dpdk-dev] [PATCH v4 2/2] net/mlx5: fix instruction hotspot on replenishing Rx buffer Yongseok Koh
                     ` (2 more replies)
  2019-01-14 21:16 ` [dpdk-dev] [PATCH v5 1/2] mbuf: add function returning " Yongseok Koh
  4 siblings, 3 replies; 35+ messages in thread
From: Yongseok Koh @ 2019-01-10 22:40 UTC (permalink / raw)
  To: olivier.matz, shahafs; +Cc: dev, roszenrami, david.marchand

This patch introduces two new functions - rte_mbuf_buf_addr() and
rte_mbuf_data_addr_default().

rte_mbuf_buf_addr() reutrns the default buffer address of given mbuf which
comes after mbuf structure and private data.

rte_mbuf_data_addr_default() returns the default address of mbuf data
taking the headroom into account.

Signed-off-by: Yongseok Koh <yskoh@mellanox.com>
---

Please ignore v3, there were errors with some compilers.

v4:
* fix compilation error due to the experimental tag
* add warning of experimental tag to the comments

v3:
* rename functions

v2:
* initial implementation

 lib/librte_mbuf/rte_mbuf.h | 46 ++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 46 insertions(+)

diff --git a/lib/librte_mbuf/rte_mbuf.h b/lib/librte_mbuf/rte_mbuf.h
index bc562dc8a9..5787616999 100644
--- a/lib/librte_mbuf/rte_mbuf.h
+++ b/lib/librte_mbuf/rte_mbuf.h
@@ -788,8 +788,54 @@ rte_mbuf_from_indirect(struct rte_mbuf *mi)
 }
 
 /**
+ * Return the default buffer address of the mbuf.
+ *
+ * @warning
+ * @b EXPERIMENTAL: This API may change without prior notice.
+ * This will be used by rte_mbuf_to_baddr() which has redundant code once
+ * experimental tag is removed.
+ *
+ * @param mb
+ *   The pointer to the mbuf.
+ * @param mp
+ *   The pointer to the mempool of the mbuf.
+ * @return
+ *   The pointer of the mbuf buffer.
+ */
+static inline char * __rte_experimental
+rte_mbuf_buf_addr(struct rte_mbuf *mb, struct rte_mempool *mp)
+{
+	char *buffer_addr;
+
+	buffer_addr = (char *)mb + sizeof(*mb) + rte_pktmbuf_priv_size(mp);
+	return buffer_addr;
+}
+
+/**
+ * Return the default address of the beginning of the mbuf data.
+ *
+ * @warning
+ * @b EXPERIMENTAL: This API may change without prior notice.
+ *
+ * @param mb
+ *   The pointer to the mbuf.
+ * @return
+ *   The pointer of the beginning of the mbuf data.
+ */
+static inline char * __rte_experimental
+rte_mbuf_data_addr_default(struct rte_mbuf *mb)
+{
+	return rte_mbuf_buf_addr(mb, mb->pool) + RTE_PKTMBUF_HEADROOM;
+}
+
+/**
  * Return the buffer address embedded in the given mbuf.
  *
+ * @note: Accessing mempool pointer of a mbuf is expensive because the
+ * pointer is stored in the 2nd cache line of mbuf. If mempool is known, it
+ * is better not to reference the mempool pointer in mbuf but calling
+ * rte_mbuf_buf_addr() would be more efficient.
+ *
  * @param md
  *   The pointer to the mbuf.
  * @return
-- 
2.11.0

^ permalink raw reply	[flat|nested] 35+ messages in thread

* [dpdk-dev] [PATCH v4 2/2] net/mlx5: fix instruction hotspot on replenishing Rx buffer
  2019-01-10 22:40 ` [dpdk-dev] [PATCH v4 " Yongseok Koh
@ 2019-01-10 22:40   ` Yongseok Koh
  2019-01-11  8:05   ` [dpdk-dev] [PATCH v4 1/2] mbuf: add function returning default buffer address Olivier Matz
  2019-01-11  8:11   ` David Marchand
  2 siblings, 0 replies; 35+ messages in thread
From: Yongseok Koh @ 2019-01-10 22:40 UTC (permalink / raw)
  To: olivier.matz, shahafs; +Cc: dev, roszenrami, david.marchand, stable

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 <yskoh@mellanox.com>
---

v4:
* no change

v3:
* rte_mbuf_buf_addr_default() -> rte_mbuf_buf_addr()

v2:
* use the newly introduced API - rte_mbuf_buf_addr_default()
* fix error in assert

 drivers/net/mlx5/mlx5_rxtx_vec.h | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/drivers/net/mlx5/mlx5_rxtx_vec.h b/drivers/net/mlx5/mlx5_rxtx_vec.h
index fda7004e2d..989a1fdce5 100644
--- a/drivers/net/mlx5/mlx5_rxtx_vec.h
+++ b/drivers/net/mlx5/mlx5_rxtx_vec.h
@@ -102,8 +102,10 @@ 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);
+		void *buf_addr = rte_mbuf_buf_addr(elts[i], rxq->mp);
+
+		assert(buf_addr == elts[i]->buf_addr);
+		wq[i].addr = rte_cpu_to_be_64((uintptr_t)buf_addr + RTE_PKTMBUF_HEADROOM);
 		/* 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

^ permalink raw reply	[flat|nested] 35+ messages in thread

* Re: [dpdk-dev] [PATCH v4 1/2] mbuf: add function returning default buffer address
  2019-01-10 22:40 ` [dpdk-dev] [PATCH v4 " Yongseok Koh
  2019-01-10 22:40   ` [dpdk-dev] [PATCH v4 2/2] net/mlx5: fix instruction hotspot on replenishing Rx buffer Yongseok Koh
@ 2019-01-11  8:05   ` Olivier Matz
  2019-01-11  8:11   ` David Marchand
  2 siblings, 0 replies; 35+ messages in thread
From: Olivier Matz @ 2019-01-11  8:05 UTC (permalink / raw)
  To: Yongseok Koh; +Cc: shahafs, dev, roszenrami, david.marchand

On Thu, Jan 10, 2019 at 02:40:29PM -0800, Yongseok Koh wrote:
> This patch introduces two new functions - rte_mbuf_buf_addr() and
> rte_mbuf_data_addr_default().
> 
> rte_mbuf_buf_addr() reutrns the default buffer address of given mbuf which
> comes after mbuf structure and private data.
> 
> rte_mbuf_data_addr_default() returns the default address of mbuf data
> taking the headroom into account.
> 
> Signed-off-by: Yongseok Koh <yskoh@mellanox.com>

Acked-by: Olivier Matz <olivier.matz@6wind.com>

^ permalink raw reply	[flat|nested] 35+ messages in thread

* Re: [dpdk-dev] [PATCH v4 1/2] mbuf: add function returning default buffer address
  2019-01-10 22:40 ` [dpdk-dev] [PATCH v4 " Yongseok Koh
  2019-01-10 22:40   ` [dpdk-dev] [PATCH v4 2/2] net/mlx5: fix instruction hotspot on replenishing Rx buffer Yongseok Koh
  2019-01-11  8:05   ` [dpdk-dev] [PATCH v4 1/2] mbuf: add function returning default buffer address Olivier Matz
@ 2019-01-11  8:11   ` David Marchand
  2019-01-11  8:32     ` David Marchand
  2019-01-11 10:25     ` Thomas Monjalon
  2 siblings, 2 replies; 35+ messages in thread
From: David Marchand @ 2019-01-11  8:11 UTC (permalink / raw)
  To: Yongseok Koh, Thomas Monjalon
  Cc: Olivier Matz, Shahaf Shuler, dev, roszenrami

On Thu, Jan 10, 2019 at 11:40 PM Yongseok Koh <yskoh@mellanox.com> wrote:

> This patch introduces two new functions - rte_mbuf_buf_addr() and
> rte_mbuf_data_addr_default().
>
> rte_mbuf_buf_addr() reutrns the default buffer address of given mbuf which
>

returns


> comes after mbuf structure and private data.
>
> rte_mbuf_data_addr_default() returns the default address of mbuf data
> taking the headroom into account.
>
> Signed-off-by: Yongseok Koh <yskoh@mellanox.com>
> ---
>
> Please ignore v3, there were errors with some compilers.
>
> v4:
> * fix compilation error due to the experimental tag
> * add warning of experimental tag to the comments
>
> v3:
> * rename functions
>
> v2:
> * initial implementation
>
>  lib/librte_mbuf/rte_mbuf.h | 46
> ++++++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 46 insertions(+)
>
> diff --git a/lib/librte_mbuf/rte_mbuf.h b/lib/librte_mbuf/rte_mbuf.h
> index bc562dc8a9..5787616999 100644
> --- a/lib/librte_mbuf/rte_mbuf.h
> +++ b/lib/librte_mbuf/rte_mbuf.h
> @@ -788,8 +788,54 @@ rte_mbuf_from_indirect(struct rte_mbuf *mi)
>  }
>
>  /**
> + * Return the default buffer address of the mbuf.
>

Nit: missed it... s/default //

+ *
> + * @warning
> + * @b EXPERIMENTAL: This API may change without prior notice.
> + * This will be used by rte_mbuf_to_baddr() which has redundant code once
> + * experimental tag is removed.
>

Good point.
I wonder if we have a "todolist" for release n+2 so that we don't forget
about such things to do.
Thomas ?

+ *
> + * @param mb
> + *   The pointer to the mbuf.
> + * @param mp
> + *   The pointer to the mempool of the mbuf.
> + * @return
> + *   The pointer of the mbuf buffer.
> + */
> +static inline char * __rte_experimental
> +rte_mbuf_buf_addr(struct rte_mbuf *mb, struct rte_mempool *mp)
> +{
> +       char *buffer_addr;
> +
> +       buffer_addr = (char *)mb + sizeof(*mb) + rte_pktmbuf_priv_size(mp);
> +       return buffer_addr;
> +}
>

Nit:
+       return (char *)mb + sizeof(*mb) + rte_pktmbuf_priv_size(mp);

+
> +/**
> + * Return the default address of the beginning of the mbuf data.
> + *
> + * @warning
> + * @b EXPERIMENTAL: This API may change without prior notice.
> + *
> + * @param mb
> + *   The pointer to the mbuf.
> + * @return
> + *   The pointer of the beginning of the mbuf data.
> + */
> +static inline char * __rte_experimental
> +rte_mbuf_data_addr_default(struct rte_mbuf *mb)
> +{
> +       return rte_mbuf_buf_addr(mb, mb->pool) + RTE_PKTMBUF_HEADROOM;
> +}
> +
> +/**
>   * Return the buffer address embedded in the given mbuf.
>   *
> + * @note: Accessing mempool pointer of a mbuf is expensive because the
> + * pointer is stored in the 2nd cache line of mbuf. If mempool is known,
> it
> + * is better not to reference the mempool pointer in mbuf but calling
> + * rte_mbuf_buf_addr() would be more efficient.
> + *
>   * @param md
>   *   The pointer to the mbuf.
>   * @return
> --
> 2.11.0
>
>
Thanks Yongseok.
Reviewed-by: David Marchand <david.marchand@redhat.com>


-- 
David Marchand

^ permalink raw reply	[flat|nested] 35+ messages in thread

* Re: [dpdk-dev] [PATCH v3 1/2] mbuf: add function returning default buffer address
  2019-01-10 18:35 ` [dpdk-dev] [PATCH v3 " Yongseok Koh
  2019-01-10 18:35   ` [dpdk-dev] [PATCH v3 2/2] net/mlx5: fix instruction hotspot on replenishing Rx buffer Yongseok Koh
@ 2019-01-11  8:14   ` Andrew Rybchenko
  2019-01-11 11:03     ` Yongseok Koh
  1 sibling, 1 reply; 35+ messages in thread
From: Andrew Rybchenko @ 2019-01-11  8:14 UTC (permalink / raw)
  To: Yongseok Koh, olivier.matz, shahafs; +Cc: dev, roszenrami, david.marchand

On 1/10/19 9:35 PM, Yongseok Koh wrote:
> This patch introduces two new functions - rte_mbuf_buf_addr() and
> rte_mbuf_data_addr_default().
>
> rte_mbuf_buf_addr() reutrns the default buffer address of given mbuf which
> comes after mbuf structure and private data.
>
> rte_mbuf_data_addr_default() returns the default address of mbuf data
> taking the headroom into account.
>
> Signed-off-by: Yongseok Koh <yskoh@mellanox.com>
> ---
>
> v3:
> * rename functions
>
> v2:
> * initial implementation
>
>   lib/librte_mbuf/rte_mbuf.h | 43 ++++++++++++++++++++++++++++++++++++++++---
>   1 file changed, 40 insertions(+), 3 deletions(-)
>
> diff --git a/lib/librte_mbuf/rte_mbuf.h b/lib/librte_mbuf/rte_mbuf.h
> index bc562dc8a9..486566fc28 100644
> --- a/lib/librte_mbuf/rte_mbuf.h
> +++ b/lib/librte_mbuf/rte_mbuf.h
> @@ -788,8 +788,47 @@ rte_mbuf_from_indirect(struct rte_mbuf *mi)
>   }
>   
>   /**
> + * Return the default buffer address of the mbuf.
> + *
> + * @param mb
> + *   The pointer to the mbuf.
> + * @param mp
> + *   The pointer to the mempool of the mbuf.
> + * @return
> + *   The pointer of the mbuf buffer.
> + */
> +static inline char * __rte_experimental
> +rte_mbuf_buf_addr(struct rte_mbuf *mb, struct rte_mempool *mp)

struct rte_mbuf has pool member. So, I don't understand why mp
argument is required. I guess there is a reason, but it should be
explained in comments. I see motivation in rte_mbuf_to_baddr()
description, but rte_mbuf_buf_add() does not explain it.

Also right now the function name looks like simple get accessor for
buf_addr and I'd expect to seem one line implementation may be
with extra debug checks: return mb->buf_addr.

May be rte_mbuf_direct_buf_addr() ?
If so, similar below rte_mbuf_direct_data_addr_default().

> +{
> +	char *buffer_addr;
> +
> +	buffer_addr = (char *)mb + sizeof(*mb) + rte_pktmbuf_priv_size(mp);
> +	return buffer_addr;
> +}
> +
> +
> +/**
> + * Return the default address of the beginning of the mbuf data.
> + *
> + * @param mb
> + *   The pointer to the mbuf.
> + * @return
> + *   The pointer of the beginning of the mbuf data.
> + */
> +static inline char * __rte_experimental
> +rte_mbuf_data_addr_default(struct rte_mbuf *mb)
> +{
> +	return rte_mbuf_buf_addr(mb, mb->pool) + RTE_PKTMBUF_HEADROOM;
> +}
> +
> +/**
>    * Return the buffer address embedded in the given mbuf.
>    *
> + * Note that accessing mempool pointer of a mbuf is expensive because the
> + * pointer is stored in the 2nd cache line of mbuf. If mempool is known, it
> + * is better not to reference the mempool pointer in mbuf but calling
> + * rte_mbuf_buf_addr() would be more efficient.
> + *
>    * @param md
>    *   The pointer to the mbuf.
>    * @return
> @@ -798,9 +837,7 @@ rte_mbuf_from_indirect(struct rte_mbuf *mi)
>   static inline char *
>   rte_mbuf_to_baddr(struct rte_mbuf *md)
>   {
> -	char *buffer_addr;
> -	buffer_addr = (char *)md + sizeof(*md) + rte_pktmbuf_priv_size(md->pool);
> -	return buffer_addr;
> +	return rte_mbuf_buf_addr(md, md->pool);
>   }
>   
>   /**

^ permalink raw reply	[flat|nested] 35+ messages in thread

* Re: [dpdk-dev] [PATCH v4 1/2] mbuf: add function returning default buffer address
  2019-01-11  8:11   ` David Marchand
@ 2019-01-11  8:32     ` David Marchand
  2019-01-11 11:09       ` Yongseok Koh
  2019-01-11 10:25     ` Thomas Monjalon
  1 sibling, 1 reply; 35+ messages in thread
From: David Marchand @ 2019-01-11  8:32 UTC (permalink / raw)
  To: Yongseok Koh, Thomas Monjalon
  Cc: Olivier Matz, Shahaf Shuler, dev, roszenrami

On Fri, Jan 11, 2019 at 9:11 AM David Marchand <david.marchand@redhat.com>
wrote:

> On Thu, Jan 10, 2019 at 11:40 PM Yongseok Koh <yskoh@mellanox.com> wrote:
> diff --git a/lib/librte_mbuf/rte_mbuf.h b/lib/librte_mbuf/rte_mbuf.h
>
>> index bc562dc8a9..5787616999 100644
>> --- a/lib/librte_mbuf/rte_mbuf.h
>> +++ b/lib/librte_mbuf/rte_mbuf.h
>> @@ -788,8 +788,54 @@ rte_mbuf_from_indirect(struct rte_mbuf *mi)
>>  }
>>
>>  /**
>> + * Return the default buffer address of the mbuf.
>>
>
> Nit: missed it... s/default //
>
> + *
>> + * @warning
>> + * @b EXPERIMENTAL: This API may change without prior notice.
>> + * This will be used by rte_mbuf_to_baddr() which has redundant code once
>> + * experimental tag is removed.
>>
>
> Good point.
> I wonder if we have a "todolist" for release n+2 so that we don't forget
> about such things to do.
> Thomas ?
>

Maybe we could have something explicit in rte_mbuf_to_baddr that would
avoid it "inherits" the experimental tag.

@@ -844,9 +844,13 @@ struct rte_mbuf_ext_shared_info {
 static inline char *
 rte_mbuf_to_baddr(struct rte_mbuf *md)
 {
+#ifdef ALLOW_EXPERIMENTAL_API
+       return rte_mbuf_buf_addr(md, md->pool);
+#else
        char *buffer_addr;
        buffer_addr = (char *)md + sizeof(*md) +
rte_pktmbuf_priv_size(md->pool);
        return buffer_addr;
+#endif
 }

 /**


-- 
David Marchand

^ permalink raw reply	[flat|nested] 35+ messages in thread

* Re: [dpdk-dev] [PATCH v4 1/2] mbuf: add function returning default buffer address
  2019-01-11  8:11   ` David Marchand
  2019-01-11  8:32     ` David Marchand
@ 2019-01-11 10:25     ` Thomas Monjalon
  1 sibling, 0 replies; 35+ messages in thread
From: Thomas Monjalon @ 2019-01-11 10:25 UTC (permalink / raw)
  To: David Marchand; +Cc: Yongseok Koh, Olivier Matz, Shahaf Shuler, dev, roszenrami

11/01/2019 09:11, David Marchand:
> On Thu, Jan 10, 2019 at 11:40 PM Yongseok Koh <yskoh@mellanox.com> wrote:
> > + * @warning
> > + * @b EXPERIMENTAL: This API may change without prior notice.
> > + * This will be used by rte_mbuf_to_baddr() which has redundant code once
> > + * experimental tag is removed.
> 
> Good point.
> I wonder if we have a "todolist" for release n+2 so that we don't forget
> about such things to do.
> Thomas ?

If code comment is not enough, we can use Bugzilla.

^ permalink raw reply	[flat|nested] 35+ messages in thread

* Re: [dpdk-dev] [PATCH v3 1/2] mbuf: add function returning default buffer address
  2019-01-11  8:14   ` [dpdk-dev] [PATCH v3 1/2] mbuf: add function returning default buffer address Andrew Rybchenko
@ 2019-01-11 11:03     ` Yongseok Koh
  2019-01-11 11:17       ` Andrew Rybchenko
  0 siblings, 1 reply; 35+ messages in thread
From: Yongseok Koh @ 2019-01-11 11:03 UTC (permalink / raw)
  To: Andrew Rybchenko
  Cc: olivier.matz, Shahaf Shuler, dev, roszenrami, david.marchand

On Fri, Jan 11, 2019 at 11:14:22AM +0300, Andrew Rybchenko wrote:
> On 1/10/19 9:35 PM, Yongseok Koh wrote:
> > This patch introduces two new functions - rte_mbuf_buf_addr() and
> > rte_mbuf_data_addr_default().
> > 
> > rte_mbuf_buf_addr() reutrns the default buffer address of given mbuf which
> > comes after mbuf structure and private data.
> > 
> > rte_mbuf_data_addr_default() returns the default address of mbuf data
> > taking the headroom into account.
> > 
> > Signed-off-by: Yongseok Koh <yskoh@mellanox.com>
> > ---
> > 
> > v3:
> > * rename functions
> > 
> > v2:
> > * initial implementation
> > 
> >   lib/librte_mbuf/rte_mbuf.h | 43 ++++++++++++++++++++++++++++++++++++++++---
> >   1 file changed, 40 insertions(+), 3 deletions(-)
> > 
> > diff --git a/lib/librte_mbuf/rte_mbuf.h b/lib/librte_mbuf/rte_mbuf.h
> > index bc562dc8a9..486566fc28 100644
> > --- a/lib/librte_mbuf/rte_mbuf.h
> > +++ b/lib/librte_mbuf/rte_mbuf.h
> > @@ -788,8 +788,47 @@ rte_mbuf_from_indirect(struct rte_mbuf *mi)
> >   }
> >   /**
> > + * Return the default buffer address of the mbuf.
> > + *
> > + * @param mb
> > + *   The pointer to the mbuf.
> > + * @param mp
> > + *   The pointer to the mempool of the mbuf.
> > + * @return
> > + *   The pointer of the mbuf buffer.
> > + */
> > +static inline char * __rte_experimental
> > +rte_mbuf_buf_addr(struct rte_mbuf *mb, struct rte_mempool *mp)
> 
> struct rte_mbuf has pool member. So, I don't understand why mp
> argument is required. I guess there is a reason, but it should be
> explained in comments. I see motivation in rte_mbuf_to_baddr()
> description, but rte_mbuf_buf_add() does not explain it.

Well, I don't like to put same comment here and there but I'll add small comment
here.

> Also right now the function name looks like simple get accessor for
> buf_addr and I'd expect to seem one line implementation may be
> with extra debug checks: return mb->buf_addr.

This func is suggested by David and Olivier because same code is being repeated
in multiple locations. This can be used to initialize a mbuf when mb->buf_addr is
null. And second use-case (this is my use-case) is to get the buf_addr without
accessing the mbuf struct when mempool of mbuf is known, e.g. Rx buffer
replenishment. It is definitely beneficial for performance, especially RISC
cores.

> May be rte_mbuf_direct_buf_addr() ?
> If so, similar below rte_mbuf_direct_data_addr_default().

Regarding naming, people have different tastes. As it is acked by Olivier and
David, I'll keep the names.

Thanks,
Yongseok

> > +{
> > +	char *buffer_addr;
> > +
> > +	buffer_addr = (char *)mb + sizeof(*mb) + rte_pktmbuf_priv_size(mp);
> > +	return buffer_addr;
> > +}
> > +
> > +
> > +/**
> > + * Return the default address of the beginning of the mbuf data.
> > + *
> > + * @param mb
> > + *   The pointer to the mbuf.
> > + * @return
> > + *   The pointer of the beginning of the mbuf data.
> > + */
> > +static inline char * __rte_experimental
> > +rte_mbuf_data_addr_default(struct rte_mbuf *mb)
> > +{
> > +	return rte_mbuf_buf_addr(mb, mb->pool) + RTE_PKTMBUF_HEADROOM;
> > +}
> > +
> > +/**
> >    * Return the buffer address embedded in the given mbuf.
> >    *
> > + * Note that accessing mempool pointer of a mbuf is expensive because the
> > + * pointer is stored in the 2nd cache line of mbuf. If mempool is known, it
> > + * is better not to reference the mempool pointer in mbuf but calling
> > + * rte_mbuf_buf_addr() would be more efficient.
> > + *
> >    * @param md
> >    *   The pointer to the mbuf.
> >    * @return
> > @@ -798,9 +837,7 @@ rte_mbuf_from_indirect(struct rte_mbuf *mi)
> >   static inline char *
> >   rte_mbuf_to_baddr(struct rte_mbuf *md)
> >   {
> > -	char *buffer_addr;
> > -	buffer_addr = (char *)md + sizeof(*md) + rte_pktmbuf_priv_size(md->pool);
> > -	return buffer_addr;
> > +	return rte_mbuf_buf_addr(md, md->pool);
> >   }
> >   /**
> 

^ permalink raw reply	[flat|nested] 35+ messages in thread

* Re: [dpdk-dev] [PATCH v4 1/2] mbuf: add function returning default buffer address
  2019-01-11  8:32     ` David Marchand
@ 2019-01-11 11:09       ` Yongseok Koh
  0 siblings, 0 replies; 35+ messages in thread
From: Yongseok Koh @ 2019-01-11 11:09 UTC (permalink / raw)
  To: David Marchand
  Cc: Thomas Monjalon, Olivier Matz, Shahaf Shuler, dev, roszenrami

On Fri, Jan 11, 2019 at 09:32:52AM +0100, David Marchand wrote:
> On Fri, Jan 11, 2019 at 9:11 AM David Marchand <david.marchand@redhat.com>
> wrote:
> 
> > On Thu, Jan 10, 2019 at 11:40 PM Yongseok Koh <yskoh@mellanox.com> wrote:
> > diff --git a/lib/librte_mbuf/rte_mbuf.h b/lib/librte_mbuf/rte_mbuf.h
> >
> >> index bc562dc8a9..5787616999 100644
> >> --- a/lib/librte_mbuf/rte_mbuf.h
> >> +++ b/lib/librte_mbuf/rte_mbuf.h
> >> @@ -788,8 +788,54 @@ rte_mbuf_from_indirect(struct rte_mbuf *mi)
> >>  }
> >>
> >>  /**
> >> + * Return the default buffer address of the mbuf.
> >>
> >
> > Nit: missed it... s/default //

Okay

> >
> > + *
> >> + * @warning
> >> + * @b EXPERIMENTAL: This API may change without prior notice.
> >> + * This will be used by rte_mbuf_to_baddr() which has redundant code once
> >> + * experimental tag is removed.
> >>
> >
> > Good point.
> > I wonder if we have a "todolist" for release n+2 so that we don't forget
> > about such things to do.
> > Thomas ?
> >
> 
> Maybe we could have something explicit in rte_mbuf_to_baddr that would
> avoid it "inherits" the experimental tag.
> 
> @@ -844,9 +844,13 @@ struct rte_mbuf_ext_shared_info {
>  static inline char *
>  rte_mbuf_to_baddr(struct rte_mbuf *md)
>  {
> +#ifdef ALLOW_EXPERIMENTAL_API
> +       return rte_mbuf_buf_addr(md, md->pool);
> +#else
>         char *buffer_addr;
>         buffer_addr = (char *)md + sizeof(*md) +
> rte_pktmbuf_priv_size(md->pool);
>         return buffer_addr;
> +#endif

I like it, so that we can't forget to change it.

Yongseok

^ permalink raw reply	[flat|nested] 35+ messages in thread

* Re: [dpdk-dev] [PATCH v3 1/2] mbuf: add function returning default buffer address
  2019-01-11 11:03     ` Yongseok Koh
@ 2019-01-11 11:17       ` Andrew Rybchenko
  2019-01-11 11:37         ` Yongseok Koh
  2019-01-11 11:57         ` Bruce Richardson
  0 siblings, 2 replies; 35+ messages in thread
From: Andrew Rybchenko @ 2019-01-11 11:17 UTC (permalink / raw)
  To: Yongseok Koh, olivier.matz, david.marchand; +Cc: Shahaf Shuler, dev, roszenrami

Olivier, David,

could you take a look at naming suggested below and share your thoughts.
My fear is that rte_mbuf_buf_addr() is too generic and true for direct mbuf
only. That's why I'd like to highlight it in the function name.

Thanks,
Andrew.

On 1/11/19 2:03 PM, Yongseok Koh wrote:
> On Fri, Jan 11, 2019 at 11:14:22AM +0300, Andrew Rybchenko wrote:
>> On 1/10/19 9:35 PM, Yongseok Koh wrote:
>>> This patch introduces two new functions - rte_mbuf_buf_addr() and
>>> rte_mbuf_data_addr_default().
>>>
>>> rte_mbuf_buf_addr() reutrns the default buffer address of given mbuf which
>>> comes after mbuf structure and private data.
>>>
>>> rte_mbuf_data_addr_default() returns the default address of mbuf data
>>> taking the headroom into account.
>>>
>>> Signed-off-by: Yongseok Koh <yskoh@mellanox.com>
>>> ---
>>>
>>> v3:
>>> * rename functions
>>>
>>> v2:
>>> * initial implementation
>>>
>>>    lib/librte_mbuf/rte_mbuf.h | 43 ++++++++++++++++++++++++++++++++++++++++---
>>>    1 file changed, 40 insertions(+), 3 deletions(-)
>>>
>>> diff --git a/lib/librte_mbuf/rte_mbuf.h b/lib/librte_mbuf/rte_mbuf.h
>>> index bc562dc8a9..486566fc28 100644
>>> --- a/lib/librte_mbuf/rte_mbuf.h
>>> +++ b/lib/librte_mbuf/rte_mbuf.h
>>> @@ -788,8 +788,47 @@ rte_mbuf_from_indirect(struct rte_mbuf *mi)
>>>    }
>>>    /**
>>> + * Return the default buffer address of the mbuf.
>>> + *
>>> + * @param mb
>>> + *   The pointer to the mbuf.
>>> + * @param mp
>>> + *   The pointer to the mempool of the mbuf.
>>> + * @return
>>> + *   The pointer of the mbuf buffer.
>>> + */
>>> +static inline char * __rte_experimental
>>> +rte_mbuf_buf_addr(struct rte_mbuf *mb, struct rte_mempool *mp)
>> struct rte_mbuf has pool member. So, I don't understand why mp
>> argument is required. I guess there is a reason, but it should be
>> explained in comments. I see motivation in rte_mbuf_to_baddr()
>> description, but rte_mbuf_buf_add() does not explain it.
> Well, I don't like to put same comment here and there but I'll add small comment
> here.
>
>> Also right now the function name looks like simple get accessor for
>> buf_addr and I'd expect to seem one line implementation may be
>> with extra debug checks: return mb->buf_addr.
> This func is suggested by David and Olivier because same code is being repeated
> in multiple locations. This can be used to initialize a mbuf when mb->buf_addr is
> null. And second use-case (this is my use-case) is to get the buf_addr without
> accessing the mbuf struct when mempool of mbuf is known, e.g. Rx buffer
> replenishment. It is definitely beneficial for performance, especially RISC
> cores.
>
>> May be rte_mbuf_direct_buf_addr() ?
>> If so, similar below rte_mbuf_direct_data_addr_default().
> Regarding naming, people have different tastes. As it is acked by Olivier and
> David, I'll keep the names.

> Thanks,
> Yongseok
>
>>> +{
>>> +	char *buffer_addr;
>>> +
>>> +	buffer_addr = (char *)mb + sizeof(*mb) + rte_pktmbuf_priv_size(mp);
>>> +	return buffer_addr;
>>> +}
>>> +
>>> +
>>> +/**
>>> + * Return the default address of the beginning of the mbuf data.
>>> + *
>>> + * @param mb
>>> + *   The pointer to the mbuf.
>>> + * @return
>>> + *   The pointer of the beginning of the mbuf data.
>>> + */
>>> +static inline char * __rte_experimental
>>> +rte_mbuf_data_addr_default(struct rte_mbuf *mb)
>>> +{
>>> +	return rte_mbuf_buf_addr(mb, mb->pool) + RTE_PKTMBUF_HEADROOM;
>>> +}
>>> +
>>> +/**
>>>     * Return the buffer address embedded in the given mbuf.
>>>     *
>>> + * Note that accessing mempool pointer of a mbuf is expensive because the
>>> + * pointer is stored in the 2nd cache line of mbuf. If mempool is known, it
>>> + * is better not to reference the mempool pointer in mbuf but calling
>>> + * rte_mbuf_buf_addr() would be more efficient.
>>> + *
>>>     * @param md
>>>     *   The pointer to the mbuf.
>>>     * @return
>>> @@ -798,9 +837,7 @@ rte_mbuf_from_indirect(struct rte_mbuf *mi)
>>>    static inline char *
>>>    rte_mbuf_to_baddr(struct rte_mbuf *md)
>>>    {
>>> -	char *buffer_addr;
>>> -	buffer_addr = (char *)md + sizeof(*md) + rte_pktmbuf_priv_size(md->pool);
>>> -	return buffer_addr;
>>> +	return rte_mbuf_buf_addr(md, md->pool);
>>>    }
>>>    /**

^ permalink raw reply	[flat|nested] 35+ messages in thread

* Re: [dpdk-dev] [PATCH v3 1/2] mbuf: add function returning default buffer address
  2019-01-11 11:17       ` Andrew Rybchenko
@ 2019-01-11 11:37         ` Yongseok Koh
  2019-01-11 11:57         ` Bruce Richardson
  1 sibling, 0 replies; 35+ messages in thread
From: Yongseok Koh @ 2019-01-11 11:37 UTC (permalink / raw)
  To: Andrew Rybchenko
  Cc: olivier.matz, david.marchand, Shahaf Shuler, dev, roszenrami


> On Jan 11, 2019, at 3:17 AM, Andrew Rybchenko <arybchenko@solarflare.com> wrote:
> 
> Olivier, David,
> 
> could you take a look at naming suggested below and share your thoughts.
> My fear is that rte_mbuf_buf_addr() is too generic and true for direct mbuf
> only. That's why I'd like to highlight it in the function name.

Like the existing rte_mbuf_to_baddr(), it is to return the buf_addr of
the given mbuf. It doesn't matter whether the given mbuf is direct or not.
It should be used at user's discretion.

Yongseok

> On 1/11/19 2:03 PM, Yongseok Koh wrote:
>> On Fri, Jan 11, 2019 at 11:14:22AM +0300, Andrew Rybchenko wrote:
>> 
>>> On 1/10/19 9:35 PM, Yongseok Koh wrote:
>>> 
>>>> This patch introduces two new functions - rte_mbuf_buf_addr() and
>>>> rte_mbuf_data_addr_default().
>>>> 
>>>> rte_mbuf_buf_addr() reutrns the default buffer address of given mbuf which
>>>> comes after mbuf structure and private data.
>>>> 
>>>> rte_mbuf_data_addr_default() returns the default address of mbuf data
>>>> taking the headroom into account.
>>>> 
>>>> Signed-off-by: Yongseok Koh 
>>>> <yskoh@mellanox.com>
>>>> 
>>>> ---
>>>> 
>>>> v3:
>>>> * rename functions
>>>> 
>>>> v2:
>>>> * initial implementation
>>>> 
>>>>   lib/librte_mbuf/rte_mbuf.h | 43 ++++++++++++++++++++++++++++++++++++++++---
>>>>   1 file changed, 40 insertions(+), 3 deletions(-)
>>>> 
>>>> diff --git a/lib/librte_mbuf/rte_mbuf.h b/lib/librte_mbuf/rte_mbuf.h
>>>> index bc562dc8a9..486566fc28 100644
>>>> --- a/lib/librte_mbuf/rte_mbuf.h
>>>> +++ b/lib/librte_mbuf/rte_mbuf.h
>>>> @@ -788,8 +788,47 @@ rte_mbuf_from_indirect(struct rte_mbuf *mi)
>>>>   }
>>>>   /**
>>>> + * Return the default buffer address of the mbuf.
>>>> + *
>>>> + * @param mb
>>>> + *   The pointer to the mbuf.
>>>> + * @param mp
>>>> + *   The pointer to the mempool of the mbuf.
>>>> + * @return
>>>> + *   The pointer of the mbuf buffer.
>>>> + */
>>>> +static inline char * __rte_experimental
>>>> +rte_mbuf_buf_addr(struct rte_mbuf *mb, struct rte_mempool *mp)
>>>> 
>>> struct rte_mbuf has pool member. So, I don't understand why mp
>>> argument is required. I guess there is a reason, but it should be
>>> explained in comments. I see motivation in rte_mbuf_to_baddr()
>>> description, but rte_mbuf_buf_add() does not explain it.
>>> 
>> Well, I don't like to put same comment here and there but I'll add small comment
>> here.
>> 
>> 
>>> Also right now the function name looks like simple get accessor for
>>> buf_addr and I'd expect to seem one line implementation may be
>>> with extra debug checks: return mb->buf_addr.
>>> 
>> This func is suggested by David and Olivier because same code is being repeated
>> in multiple locations. This can be used to initialize a mbuf when mb->buf_addr is
>> null. And second use-case (this is my use-case) is to get the buf_addr without
>> accessing the mbuf struct when mempool of mbuf is known, e.g. Rx buffer
>> replenishment. It is definitely beneficial for performance, especially RISC
>> cores.
>> 
>> 
>>> May be rte_mbuf_direct_buf_addr() ?
>>> If so, similar below rte_mbuf_direct_data_addr_default().
>>> 
>> Regarding naming, people have different tastes. As it is acked by Olivier and
>> David, I'll keep the names.
>> 
> 
>> Thanks,
>> Yongseok
>> 
>> 
>>>> +{
>>>> +	char *buffer_addr;
>>>> +
>>>> +	buffer_addr = (char *)mb + sizeof(*mb) + rte_pktmbuf_priv_size(mp);
>>>> +	return buffer_addr;
>>>> +}
>>>> +
>>>> +
>>>> +/**
>>>> + * Return the default address of the beginning of the mbuf data.
>>>> + *
>>>> + * @param mb
>>>> + *   The pointer to the mbuf.
>>>> + * @return
>>>> + *   The pointer of the beginning of the mbuf data.
>>>> + */
>>>> +static inline char * __rte_experimental
>>>> +rte_mbuf_data_addr_default(struct rte_mbuf *mb)
>>>> +{
>>>> +	return rte_mbuf_buf_addr(mb, mb->pool) + RTE_PKTMBUF_HEADROOM;
>>>> +}
>>>> +
>>>> +/**
>>>>    * Return the buffer address embedded in the given mbuf.
>>>>    *
>>>> + * Note that accessing mempool pointer of a mbuf is expensive because the
>>>> + * pointer is stored in the 2nd cache line of mbuf. If mempool is known, it
>>>> + * is better not to reference the mempool pointer in mbuf but calling
>>>> + * rte_mbuf_buf_addr() would be more efficient.
>>>> + *
>>>>    * @param md
>>>>    *   The pointer to the mbuf.
>>>>    * @return
>>>> @@ -798,9 +837,7 @@ rte_mbuf_from_indirect(struct rte_mbuf *mi)
>>>>   static inline char *
>>>>   rte_mbuf_to_baddr(struct rte_mbuf *md)
>>>>   {
>>>> -	char *buffer_addr;
>>>> -	buffer_addr = (char *)md + sizeof(*md) + rte_pktmbuf_priv_size(md->pool);
>>>> -	return buffer_addr;
>>>> +	return rte_mbuf_buf_addr(md, md->pool);
>>>>   }
>>>>   /**
>>>> 
> 

^ permalink raw reply	[flat|nested] 35+ messages in thread

* Re: [dpdk-dev] [PATCH v3 1/2] mbuf: add function returning default buffer address
  2019-01-11 11:17       ` Andrew Rybchenko
  2019-01-11 11:37         ` Yongseok Koh
@ 2019-01-11 11:57         ` Bruce Richardson
  2019-01-11 12:48           ` David Marchand
  1 sibling, 1 reply; 35+ messages in thread
From: Bruce Richardson @ 2019-01-11 11:57 UTC (permalink / raw)
  To: Andrew Rybchenko
  Cc: Yongseok Koh, olivier.matz, david.marchand, Shahaf Shuler, dev,
	roszenrami

On Fri, Jan 11, 2019 at 02:17:04PM +0300, Andrew Rybchenko wrote:
> Olivier, David,
> 
> could you take a look at naming suggested below and share your thoughts.
> My fear is that rte_mbuf_buf_addr() is too generic and true for direct mbuf
> only. That's why I'd like to highlight it in the function name.
> 

I would tend to agree with that concern.

/Bruce

> Thanks,
> Andrew.
> 
> On 1/11/19 2:03 PM, Yongseok Koh wrote:
> > On Fri, Jan 11, 2019 at 11:14:22AM +0300, Andrew Rybchenko wrote:
> > > On 1/10/19 9:35 PM, Yongseok Koh wrote:
> > > > This patch introduces two new functions - rte_mbuf_buf_addr() and
> > > > rte_mbuf_data_addr_default().
> > > > 
> > > > rte_mbuf_buf_addr() reutrns the default buffer address of given mbuf which
> > > > comes after mbuf structure and private data.
> > > > 
> > > > rte_mbuf_data_addr_default() returns the default address of mbuf data
> > > > taking the headroom into account.
> > > > 
> > > > Signed-off-by: Yongseok Koh <yskoh@mellanox.com>
> > > > ---
> > > > 
> > > > v3:
> > > > * rename functions
> > > > 
> > > > v2:
> > > > * initial implementation
> > > > 
> > > >    lib/librte_mbuf/rte_mbuf.h | 43 ++++++++++++++++++++++++++++++++++++++++---
> > > >    1 file changed, 40 insertions(+), 3 deletions(-)
> > > > 
> > > > diff --git a/lib/librte_mbuf/rte_mbuf.h b/lib/librte_mbuf/rte_mbuf.h
> > > > index bc562dc8a9..486566fc28 100644
> > > > --- a/lib/librte_mbuf/rte_mbuf.h
> > > > +++ b/lib/librte_mbuf/rte_mbuf.h
> > > > @@ -788,8 +788,47 @@ rte_mbuf_from_indirect(struct rte_mbuf *mi)
> > > >    }
> > > >    /**
> > > > + * Return the default buffer address of the mbuf.
> > > > + *
> > > > + * @param mb
> > > > + *   The pointer to the mbuf.
> > > > + * @param mp
> > > > + *   The pointer to the mempool of the mbuf.
> > > > + * @return
> > > > + *   The pointer of the mbuf buffer.
> > > > + */
> > > > +static inline char * __rte_experimental
> > > > +rte_mbuf_buf_addr(struct rte_mbuf *mb, struct rte_mempool *mp)
> > > struct rte_mbuf has pool member. So, I don't understand why mp
> > > argument is required. I guess there is a reason, but it should be
> > > explained in comments. I see motivation in rte_mbuf_to_baddr()
> > > description, but rte_mbuf_buf_add() does not explain it.
> > Well, I don't like to put same comment here and there but I'll add small comment
> > here.
> > 
> > > Also right now the function name looks like simple get accessor for
> > > buf_addr and I'd expect to seem one line implementation may be
> > > with extra debug checks: return mb->buf_addr.
> > This func is suggested by David and Olivier because same code is being repeated
> > in multiple locations. This can be used to initialize a mbuf when mb->buf_addr is
> > null. And second use-case (this is my use-case) is to get the buf_addr without
> > accessing the mbuf struct when mempool of mbuf is known, e.g. Rx buffer
> > replenishment. It is definitely beneficial for performance, especially RISC
> > cores.
> > 
> > > May be rte_mbuf_direct_buf_addr() ?
> > > If so, similar below rte_mbuf_direct_data_addr_default().
> > Regarding naming, people have different tastes. As it is acked by Olivier and
> > David, I'll keep the names.
> 
> > Thanks,
> > Yongseok
> > 
> > > > +{
> > > > +	char *buffer_addr;
> > > > +
> > > > +	buffer_addr = (char *)mb + sizeof(*mb) + rte_pktmbuf_priv_size(mp);
> > > > +	return buffer_addr;
> > > > +}
> > > > +
> > > > +
> > > > +/**
> > > > + * Return the default address of the beginning of the mbuf data.
> > > > + *
> > > > + * @param mb
> > > > + *   The pointer to the mbuf.
> > > > + * @return
> > > > + *   The pointer of the beginning of the mbuf data.
> > > > + */
> > > > +static inline char * __rte_experimental
> > > > +rte_mbuf_data_addr_default(struct rte_mbuf *mb)
> > > > +{
> > > > +	return rte_mbuf_buf_addr(mb, mb->pool) + RTE_PKTMBUF_HEADROOM;
> > > > +}
> > > > +
> > > > +/**
> > > >     * Return the buffer address embedded in the given mbuf.
> > > >     *
> > > > + * Note that accessing mempool pointer of a mbuf is expensive because the
> > > > + * pointer is stored in the 2nd cache line of mbuf. If mempool is known, it
> > > > + * is better not to reference the mempool pointer in mbuf but calling
> > > > + * rte_mbuf_buf_addr() would be more efficient.
> > > > + *
> > > >     * @param md
> > > >     *   The pointer to the mbuf.
> > > >     * @return
> > > > @@ -798,9 +837,7 @@ rte_mbuf_from_indirect(struct rte_mbuf *mi)
> > > >    static inline char *
> > > >    rte_mbuf_to_baddr(struct rte_mbuf *md)
> > > >    {
> > > > -	char *buffer_addr;
> > > > -	buffer_addr = (char *)md + sizeof(*md) + rte_pktmbuf_priv_size(md->pool);
> > > > -	return buffer_addr;
> > > > +	return rte_mbuf_buf_addr(md, md->pool);
> > > >    }
> > > >    /**
> 

^ permalink raw reply	[flat|nested] 35+ messages in thread

* Re: [dpdk-dev] [PATCH v3 1/2] mbuf: add function returning default buffer address
  2019-01-11 11:57         ` Bruce Richardson
@ 2019-01-11 12:48           ` David Marchand
  2019-01-14 15:51             ` Olivier Matz
  0 siblings, 1 reply; 35+ messages in thread
From: David Marchand @ 2019-01-11 12:48 UTC (permalink / raw)
  To: Andrew Rybchenko
  Cc: Yongseok Koh, olivier.matz, Shahaf Shuler, dev, roszenrami,
	Bruce Richardson

On Fri, Jan 11, 2019 at 12:57 PM Bruce Richardson <
bruce.richardson@intel.com> wrote:

> On Fri, Jan 11, 2019 at 02:17:04PM +0300, Andrew Rybchenko wrote:
> > Olivier, David,
> >
> > could you take a look at naming suggested below and share your thoughts.
> > My fear is that rte_mbuf_buf_addr() is too generic and true for direct
> mbuf
> > only. That's why I'd like to highlight it in the function name.
> >
>
> I would tend to agree with that concern.
>

I understand your concern as well.

The only usecase we have so far is for drivers on the rx side, so
implicitely direct mbufs.
But from a api user pov, explicit is always better.

I will let Olivier have the last word :-)


-- 
David Marchand

^ permalink raw reply	[flat|nested] 35+ messages in thread

* Re: [dpdk-dev] [PATCH v3 1/2] mbuf: add function returning default buffer address
  2019-01-11 12:48           ` David Marchand
@ 2019-01-14 15:51             ` Olivier Matz
  0 siblings, 0 replies; 35+ messages in thread
From: Olivier Matz @ 2019-01-14 15:51 UTC (permalink / raw)
  To: David Marchand
  Cc: Andrew Rybchenko, Yongseok Koh, Shahaf Shuler, dev, roszenrami,
	Bruce Richardson

Hi,

On Fri, Jan 11, 2019 at 01:48:12PM +0100, David Marchand wrote:
> On Fri, Jan 11, 2019 at 12:57 PM Bruce Richardson <
> bruce.richardson@intel.com> wrote:
> 
> > On Fri, Jan 11, 2019 at 02:17:04PM +0300, Andrew Rybchenko wrote:
> > > Olivier, David,
> > >
> > > could you take a look at naming suggested below and share your thoughts.
> > > My fear is that rte_mbuf_buf_addr() is too generic and true for direct
> > mbuf
> > > only. That's why I'd like to highlight it in the function name.
> > >
> >
> > I would tend to agree with that concern.
> >
> 
> I understand your concern as well.
> 
> The only usecase we have so far is for drivers on the rx side, so
> implicitely direct mbufs.
> But from a api user pov, explicit is always better.
> 
> I will let Olivier have the last word :-)

Thanks Andrew for pointing this out.

However I agree with Yongseok: we already have many functions that
applies to direct mbufs that don't have "direct" in their names.

In my opinion, rte_mbuf_buf_addr() is a good name, but I think the
doxygen comment could be improved a bit to state that it returns the
pointer to the embedded data. I also think that a small comment
explaining why the mp arg is required would be helpful.

Thanks,
Olivier

^ permalink raw reply	[flat|nested] 35+ messages in thread

* [dpdk-dev] [PATCH v5 1/2] mbuf: add function returning buffer address
  2019-01-09  8:54 [dpdk-dev] [PATCH] net/mlx5: fix instruction hotspot on replenishing Rx buffer Yongseok Koh
                   ` (3 preceding siblings ...)
  2019-01-10 22:40 ` [dpdk-dev] [PATCH v4 " Yongseok Koh
@ 2019-01-14 21:16 ` Yongseok Koh
  2019-01-14 21:16   ` [dpdk-dev] [PATCH v5 2/2] net/mlx5: fix instruction hotspot on replenishing Rx buffer Yongseok Koh
  2019-01-15  1:35   ` [dpdk-dev] [PATCH v5 1/2] mbuf: add function returning buffer address Thomas Monjalon
  4 siblings, 2 replies; 35+ messages in thread
From: Yongseok Koh @ 2019-01-14 21:16 UTC (permalink / raw)
  To: olivier.matz, shahafs; +Cc: dev, arybchenko, roszenrami, david.marchand

This patch introduces two new functions - rte_mbuf_buf_addr() and
rte_mbuf_data_addr_default().

rte_mbuf_buf_addr() reutrns the buffer address of given mbuf which comes
after mbuf structure and private data.

rte_mbuf_data_addr_default() returns the default address of mbuf data
taking the headroom into account.

Signed-off-by: Yongseok Koh <yskoh@mellanox.com>
Acked-by: Olivier Matz <olivier.matz@6wind.com>
Reviewed-by: David Marchand <david.marchand@redhat.com>
---

v5:
* add more comment regarding mp arg
* use ALLOW_EXPERIMENTAL_API for rte_mbuf_to_baddr()

v4:
* fix compilation error due to the experimental tag
* add warning of experimental tag to the comments

v3:
* rename functions

v2:
* initial implementation

 lib/librte_mbuf/rte_mbuf.h | 54 +++++++++++++++++++++++++++++++++++++++++++++-
 1 file changed, 53 insertions(+), 1 deletion(-)

diff --git a/lib/librte_mbuf/rte_mbuf.h b/lib/librte_mbuf/rte_mbuf.h
index bc562dc8a9..4a03d5e6fc 100644
--- a/lib/librte_mbuf/rte_mbuf.h
+++ b/lib/librte_mbuf/rte_mbuf.h
@@ -788,7 +788,55 @@ rte_mbuf_from_indirect(struct rte_mbuf *mi)
 }
 
 /**
- * Return the buffer address embedded in the given mbuf.
+ * Return address of buffer embedded in the given mbuf.
+ *
+ * The return value shall be same as mb->buf_addr if the mbuf is already
+ * initialized and direct. However, this API is useful if mempool of the
+ * mbuf is already known because it doesn't need to access mbuf contents in
+ * order to get the mempool pointer.
+ *
+ * @warning
+ * @b EXPERIMENTAL: This API may change without prior notice.
+ * This will be used by rte_mbuf_to_baddr() which has redundant code once
+ * experimental tag is removed.
+ *
+ * @param mb
+ *   The pointer to the mbuf.
+ * @param mp
+ *   The pointer to the mempool of the mbuf.
+ * @return
+ *   The pointer of the mbuf buffer.
+ */
+static inline char * __rte_experimental
+rte_mbuf_buf_addr(struct rte_mbuf *mb, struct rte_mempool *mp)
+{
+	return (char *)mb + sizeof(*mb) + rte_pktmbuf_priv_size(mp);
+}
+
+/**
+ * Return the default address of the beginning of the mbuf data.
+ *
+ * @warning
+ * @b EXPERIMENTAL: This API may change without prior notice.
+ *
+ * @param mb
+ *   The pointer to the mbuf.
+ * @return
+ *   The pointer of the beginning of the mbuf data.
+ */
+static inline char * __rte_experimental
+rte_mbuf_data_addr_default(struct rte_mbuf *mb)
+{
+	return rte_mbuf_buf_addr(mb, mb->pool) + RTE_PKTMBUF_HEADROOM;
+}
+
+/**
+ * Return address of buffer embedded in the given mbuf.
+ *
+ * @note: Accessing mempool pointer of a mbuf is expensive because the
+ * pointer is stored in the 2nd cache line of mbuf. If mempool is known, it
+ * is better not to reference the mempool pointer in mbuf but calling
+ * rte_mbuf_buf_addr() would be more efficient.
  *
  * @param md
  *   The pointer to the mbuf.
@@ -798,9 +846,13 @@ rte_mbuf_from_indirect(struct rte_mbuf *mi)
 static inline char *
 rte_mbuf_to_baddr(struct rte_mbuf *md)
 {
+#ifdef ALLOW_EXPERIMENTAL_API
+	return rte_mbuf_buf_addr(md, md->pool);
+#else
 	char *buffer_addr;
 	buffer_addr = (char *)md + sizeof(*md) + rte_pktmbuf_priv_size(md->pool);
 	return buffer_addr;
+#endif
 }
 
 /**
-- 
2.11.0

^ permalink raw reply	[flat|nested] 35+ messages in thread

* [dpdk-dev] [PATCH v5 2/2] net/mlx5: fix instruction hotspot on replenishing Rx buffer
  2019-01-14 21:16 ` [dpdk-dev] [PATCH v5 1/2] mbuf: add function returning " Yongseok Koh
@ 2019-01-14 21:16   ` Yongseok Koh
  2019-02-06 15:54     ` Kevin Traynor
  2019-01-15  1:35   ` [dpdk-dev] [PATCH v5 1/2] mbuf: add function returning buffer address Thomas Monjalon
  1 sibling, 1 reply; 35+ messages in thread
From: Yongseok Koh @ 2019-01-14 21:16 UTC (permalink / raw)
  To: olivier.matz, shahafs; +Cc: dev, arybchenko, roszenrami, david.marchand, stable

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 <yskoh@mellanox.com>
Acked-by: Shahaf Shuler <shahafs@mellanox.com>
---

v5:
* no change

v4:
* no change

v3:
* rte_mbuf_buf_addr_default() -> rte_mbuf_buf_addr()

v2:
* use the newly introduced API - rte_mbuf_buf_addr_default()
* fix error in assert

 drivers/net/mlx5/mlx5_rxtx_vec.h | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/drivers/net/mlx5/mlx5_rxtx_vec.h b/drivers/net/mlx5/mlx5_rxtx_vec.h
index fda7004e2d..5df8e291e6 100644
--- a/drivers/net/mlx5/mlx5_rxtx_vec.h
+++ b/drivers/net/mlx5/mlx5_rxtx_vec.h
@@ -102,7 +102,10 @@ 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 +
+		void *buf_addr = rte_mbuf_buf_addr(elts[i], rxq->mp);
+
+		assert(buf_addr == elts[i]->buf_addr);
+		wq[i].addr = rte_cpu_to_be_64((uintptr_t)buf_addr +
 					      RTE_PKTMBUF_HEADROOM);
 		/* 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))
-- 
2.11.0

^ permalink raw reply	[flat|nested] 35+ messages in thread

* Re: [dpdk-dev] [PATCH v5 1/2] mbuf: add function returning buffer address
  2019-01-14 21:16 ` [dpdk-dev] [PATCH v5 1/2] mbuf: add function returning " Yongseok Koh
  2019-01-14 21:16   ` [dpdk-dev] [PATCH v5 2/2] net/mlx5: fix instruction hotspot on replenishing Rx buffer Yongseok Koh
@ 2019-01-15  1:35   ` Thomas Monjalon
  1 sibling, 0 replies; 35+ messages in thread
From: Thomas Monjalon @ 2019-01-15  1:35 UTC (permalink / raw)
  To: Yongseok Koh
  Cc: dev, olivier.matz, shahafs, arybchenko, roszenrami, david.marchand

14/01/2019 22:16, Yongseok Koh:
> This patch introduces two new functions - rte_mbuf_buf_addr() and
> rte_mbuf_data_addr_default().
> 
> rte_mbuf_buf_addr() reutrns the buffer address of given mbuf which comes
> after mbuf structure and private data.
> 
> rte_mbuf_data_addr_default() returns the default address of mbuf data
> taking the headroom into account.
> 
> Signed-off-by: Yongseok Koh <yskoh@mellanox.com>
> Acked-by: Olivier Matz <olivier.matz@6wind.com>
> Reviewed-by: David Marchand <david.marchand@redhat.com>

Series applied, thanks

^ permalink raw reply	[flat|nested] 35+ messages in thread

* Re: [dpdk-dev] [PATCH v5 2/2] net/mlx5: fix instruction hotspot on replenishing Rx buffer
  2019-01-14 21:16   ` [dpdk-dev] [PATCH v5 2/2] net/mlx5: fix instruction hotspot on replenishing Rx buffer Yongseok Koh
@ 2019-02-06 15:54     ` Kevin Traynor
  2019-02-21 19:10       ` Kevin Traynor
  0 siblings, 1 reply; 35+ messages in thread
From: Kevin Traynor @ 2019-02-06 15:54 UTC (permalink / raw)
  To: Yongseok Koh, olivier.matz, shahafs
  Cc: dev, arybchenko, roszenrami, david.marchand, stable

On 01/14/2019 09:16 PM, 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
> 

This is using the API introduced in 1/2, so it's not really suitable for
backport. Maybe you want to send an alternative for stable?

> Signed-off-by: Yongseok Koh <yskoh@mellanox.com>
> Acked-by: Shahaf Shuler <shahafs@mellanox.com>
> ---

^ permalink raw reply	[flat|nested] 35+ messages in thread

* Re: [dpdk-dev] [PATCH v5 2/2] net/mlx5: fix instruction hotspot on replenishing Rx buffer
  2019-02-06 15:54     ` Kevin Traynor
@ 2019-02-21 19:10       ` Kevin Traynor
  2019-03-08  2:05         ` Yongseok Koh
  0 siblings, 1 reply; 35+ messages in thread
From: Kevin Traynor @ 2019-02-21 19:10 UTC (permalink / raw)
  To: Yongseok Koh
  Cc: olivier.matz, shahafs, dev, arybchenko, roszenrami,
	david.marchand, stable

Hi Yongseok,

Can you let me know how you want to proceed with the below. I think we
could just drop as it's a performance optimization, or maybe you have a
different idea.

thanks,
Kevin.

On 06/02/2019 15:54, Kevin Traynor wrote:
> On 01/14/2019 09:16 PM, 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
>>
> 
> This is using the API introduced in 1/2, so it's not really suitable for
> backport. Maybe you want to send an alternative for stable?
> 
>> Signed-off-by: Yongseok Koh <yskoh@mellanox.com>
>> Acked-by: Shahaf Shuler <shahafs@mellanox.com>
>> ---

^ permalink raw reply	[flat|nested] 35+ messages in thread

* Re: [dpdk-dev] [PATCH v5 2/2] net/mlx5: fix instruction hotspot on replenishing Rx buffer
  2019-02-21 19:10       ` Kevin Traynor
@ 2019-03-08  2:05         ` Yongseok Koh
  0 siblings, 0 replies; 35+ messages in thread
From: Yongseok Koh @ 2019-03-08  2:05 UTC (permalink / raw)
  To: Kevin Traynor
  Cc: Olivier Matz, Shahaf Shuler, dev, Andrew Rybchenko, Rami Rosen,
	David Marchand, stable

Oops, I missed this email somehow,
probably out of mind during my long vacation. :-)

I've also encountered this issue with 17.11.6.
http://git.dpdk.org/dpdk-stable/commit/?h=17.11&id=63f06f3fccc87c55adb33248e5c68a0175d213f1

I'll send a backport to you.


Sorry for late reply.
Yongseok

> On Feb 21, 2019, at 11:10 AM, Kevin Traynor <ktraynor@redhat.com> wrote:
> 
> Hi Yongseok,
> 
> Can you let me know how you want to proceed with the below. I think we
> could just drop as it's a performance optimization, or maybe you have a
> different idea.
> 
> thanks,
> Kevin.
> 
> On 06/02/2019 15:54, Kevin Traynor wrote:
>> On 01/14/2019 09:16 PM, 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
>>> 
>> 
>> This is using the API introduced in 1/2, so it's not really suitable for
>> backport. Maybe you want to send an alternative for stable?
>> 
>>> Signed-off-by: Yongseok Koh <yskoh@mellanox.com>
>>> Acked-by: Shahaf Shuler <shahafs@mellanox.com>
>>> ---
> 

^ permalink raw reply	[flat|nested] 35+ messages in thread

end of thread, other threads:[~2019-03-08  2:05 UTC | newest]

Thread overview: 35+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-01-09  8:54 [dpdk-dev] [PATCH] net/mlx5: fix instruction hotspot on replenishing Rx buffer Yongseok Koh
2019-01-09  9:38 ` David Marchand
2019-01-09  9:52   ` Olivier Matz
2019-01-09  9:56     ` Yongseok Koh
2019-01-09 10:05       ` David Marchand
2019-01-09 10:11         ` Yongseok Koh
2019-01-09 13:19 ` [dpdk-dev] [PATCH v2 1/2] mbuf: add function returning default buffer address Yongseok Koh
2019-01-09 13:19   ` [dpdk-dev] [PATCH v2 2/2] net/mlx5: fix instruction hotspot on replenishing Rx buffer Yongseok Koh
2019-01-09 13:46   ` [dpdk-dev] [PATCH v2 1/2] mbuf: add function returning default buffer address David Marchand
2019-01-10  1:39     ` Rami Rosen
2019-01-10 18:18       ` Yongseok Koh
2019-01-10 18:22     ` Yongseok Koh
2019-01-10 18:35 ` [dpdk-dev] [PATCH v3 " Yongseok Koh
2019-01-10 18:35   ` [dpdk-dev] [PATCH v3 2/2] net/mlx5: fix instruction hotspot on replenishing Rx buffer Yongseok Koh
2019-01-10 19:10     ` Shahaf Shuler
2019-01-11  8:14   ` [dpdk-dev] [PATCH v3 1/2] mbuf: add function returning default buffer address Andrew Rybchenko
2019-01-11 11:03     ` Yongseok Koh
2019-01-11 11:17       ` Andrew Rybchenko
2019-01-11 11:37         ` Yongseok Koh
2019-01-11 11:57         ` Bruce Richardson
2019-01-11 12:48           ` David Marchand
2019-01-14 15:51             ` Olivier Matz
2019-01-10 22:40 ` [dpdk-dev] [PATCH v4 " Yongseok Koh
2019-01-10 22:40   ` [dpdk-dev] [PATCH v4 2/2] net/mlx5: fix instruction hotspot on replenishing Rx buffer Yongseok Koh
2019-01-11  8:05   ` [dpdk-dev] [PATCH v4 1/2] mbuf: add function returning default buffer address Olivier Matz
2019-01-11  8:11   ` David Marchand
2019-01-11  8:32     ` David Marchand
2019-01-11 11:09       ` Yongseok Koh
2019-01-11 10:25     ` Thomas Monjalon
2019-01-14 21:16 ` [dpdk-dev] [PATCH v5 1/2] mbuf: add function returning " Yongseok Koh
2019-01-14 21:16   ` [dpdk-dev] [PATCH v5 2/2] net/mlx5: fix instruction hotspot on replenishing Rx buffer Yongseok Koh
2019-02-06 15:54     ` Kevin Traynor
2019-02-21 19:10       ` Kevin Traynor
2019-03-08  2:05         ` Yongseok Koh
2019-01-15  1:35   ` [dpdk-dev] [PATCH v5 1/2] mbuf: add function returning buffer address Thomas Monjalon

DPDK patches and discussions

This inbox may be cloned and mirrored by anyone:

	git clone --mirror https://inbox.dpdk.org/dev/0 dev/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 dev dev/ https://inbox.dpdk.org/dev \
		dev@dpdk.org
	public-inbox-index dev

Example config snippet for mirrors.
Newsgroup available over NNTP:
	nntp://inbox.dpdk.org/inbox.dpdk.dev


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git