[dpdk-dev] [PATCH] net/mlx5: fix instruction hotspot on replenishing Rx buffer

[dpdk-dev] [PATCH] net/mlx5: fix instruction hotspot on replenishing Rx buffer

[Yongseok Koh]

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

Re: [dpdk-dev] [PATCH] net/mlx5: fix instruction hotspot on replenishing Rx buffer

[David Marchand]

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

Re: [dpdk-dev] [PATCH] net/mlx5: fix instruction hotspot on replenishing Rx buffer

[Olivier Matz]

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

Re: [dpdk-dev] [PATCH] net/mlx5: fix instruction hotspot on replenishing Rx buffer

[Yongseok Koh]

> 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

Re: [dpdk-dev] [PATCH] net/mlx5: fix instruction hotspot on replenishing Rx buffer

[David Marchand]

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

Re: [dpdk-dev] [PATCH] net/mlx5: fix instruction hotspot on replenishing Rx buffer

[Yongseok Koh]

> 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

[dpdk-dev] [PATCH v2 1/2] mbuf: add function returning default buffer address

[Yongseok Koh]

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

[dpdk-dev] [PATCH v2 2/2] net/mlx5: fix instruction hotspot on replenishing Rx buffer

[Yongseok Koh]

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

Re: [dpdk-dev] [PATCH v2 1/2] mbuf: add function returning default buffer address

[David Marchand]

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

Re: [dpdk-dev] [PATCH v2 1/2] mbuf: add function returning default buffer address

[Rami Rosen]

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
>

Re: [dpdk-dev] [PATCH v2 1/2] mbuf: add function returning default buffer address

[Yongseok Koh]

> 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

Re: [dpdk-dev] [PATCH v2 1/2] mbuf: add function returning default buffer address

[Yongseok Koh]

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

[dpdk-dev] [PATCH v3 1/2] mbuf: add function returning default buffer address

[Yongseok Koh]

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

[dpdk-dev] [PATCH v3 2/2] net/mlx5: fix instruction hotspot on replenishing Rx buffer

[Yongseok Koh]

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

Re: [dpdk-dev] [PATCH v3 2/2] net/mlx5: fix instruction hotspot on replenishing Rx buffer

[Shahaf Shuler]

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

Re: [dpdk-dev] [PATCH v3 1/2] mbuf: add function returning default buffer address

[Andrew Rybchenko]

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);
>   }
>   
>   /**

Re: [dpdk-dev] [PATCH v3 1/2] mbuf: add function returning default buffer address

[Yongseok Koh]

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);
> >   }
> >   /**
> 

Re: [dpdk-dev] [PATCH v3 1/2] mbuf: add function returning default buffer address

[Andrew Rybchenko]

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);
>>>    }
>>>    /**

Re: [dpdk-dev] [PATCH v3 1/2] mbuf: add function returning default buffer address

[Yongseok Koh]

> 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);
>>>>   }
>>>>   /**
>>>> 
> 

Re: [dpdk-dev] [PATCH v3 1/2] mbuf: add function returning default buffer address

[Bruce Richardson]

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);
> > > >    }
> > > >    /**
> 

Re: [dpdk-dev] [PATCH v3 1/2] mbuf: add function returning default buffer address

[David Marchand]

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

Re: [dpdk-dev] [PATCH v3 1/2] mbuf: add function returning default buffer address

[Olivier Matz]

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

[dpdk-dev] [PATCH v4 1/2] mbuf: add function returning default buffer address

[Yongseok Koh]

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

[dpdk-dev] [PATCH v4 2/2] net/mlx5: fix instruction hotspot on replenishing Rx buffer

[Yongseok Koh]

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

Re: [dpdk-dev] [PATCH v4 1/2] mbuf: add function returning default buffer address

[Olivier Matz]

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>

Re: [dpdk-dev] [PATCH v4 1/2] mbuf: add function returning default buffer address

[David Marchand]

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

Re: [dpdk-dev] [PATCH v4 1/2] mbuf: add function returning default buffer address

[David Marchand]

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

Re: [dpdk-dev] [PATCH v4 1/2] mbuf: add function returning default buffer address

[Yongseok Koh]

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

Re: [dpdk-dev] [PATCH v4 1/2] mbuf: add function returning default buffer address

[Thomas Monjalon]

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.

[dpdk-dev] [PATCH v5 1/2] mbuf: add function returning buffer address

[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>
---

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

[dpdk-dev] [PATCH v5 2/2] net/mlx5: fix instruction hotspot on replenishing Rx buffer

[Yongseok Koh]

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

Re: [dpdk-dev] [PATCH v5 2/2] net/mlx5: fix instruction hotspot on replenishing Rx buffer

[Kevin Traynor]

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>
> ---

Re: [dpdk-dev] [PATCH v5 2/2] net/mlx5: fix instruction hotspot on replenishing Rx buffer

[Kevin Traynor]

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>
>> ---

Re: [dpdk-dev] [PATCH v5 2/2] net/mlx5: fix instruction hotspot on replenishing Rx buffer

[Yongseok Koh]

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>
>>> ---
> 

Re: [dpdk-dev] [PATCH v5 1/2] mbuf: add function returning buffer address

[Thomas Monjalon]

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