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