diff mbox series

[v7,1/4] net/i40e: use inline prefetch function

Message ID 1710972098-2209-2-git-send-email-roretzla@linux.microsoft.com (mailing list archive)
State Superseded, archived
Delegated to: Thomas Monjalon
Headers
Series remove use of RTE_MARKER fields in libraries |

Checks

Context Check Description
ci/checkpatch success coding style OK

Commit Message

Tyler Retzlaff March 20, 2024, 10:01 p.m. UTC 
  Don't directly access the cacheline1 field in rte_mbuf struct for
prefetch instead just use rte_mbuf_prefetch_part2() to prefetch.

Signed-off-by: Tyler Retzlaff <roretzla@linux.microsoft.com>
---
 drivers/net/i40e/i40e_rxtx_vec_avx512.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

Comments

Morten Brørup March 26, 2024, 10:16 a.m. UTC | #1 
> From: Tyler Retzlaff [mailto:roretzla@linux.microsoft.com]
> Sent: Wednesday, 20 March 2024 23.02
> 
> Don't directly access the cacheline1 field in rte_mbuf struct for
> prefetch instead just use rte_mbuf_prefetch_part2() to prefetch.
> 
> Signed-off-by: Tyler Retzlaff <roretzla@linux.microsoft.com>
> ---
>  drivers/net/i40e/i40e_rxtx_vec_avx512.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/net/i40e/i40e_rxtx_vec_avx512.c
> b/drivers/net/i40e/i40e_rxtx_vec_avx512.c
> index f3050cd..0238b03 100644
> --- a/drivers/net/i40e/i40e_rxtx_vec_avx512.c
> +++ b/drivers/net/i40e/i40e_rxtx_vec_avx512.c
> @@ -826,7 +826,7 @@
>  		free[0] = m;
>  		nb_free = 1;
>  		for (i = 1; i < n; i++) {
> -			rte_prefetch0(&txep[i + 3].mbuf->cacheline1);
> +			rte_mbuf_prefetch_part2(txep[i + 3].mbuf);

@Yuying Zhang:
If this prefetch is for m->next, I think it can be omitted if m->next is in the first cache line:

-			rte_prefetch0(&txep[i + 3].mbuf->cacheline1);
+#if RTE_IOVA_IN_MBUF
+			rte_mbuf_prefetch_part2(txep[i + 3].mbuf);
+#endif

If so, it belongs in a separate patch anyway.

>  			m = rte_pktmbuf_prefree_seg(txep[i].mbuf);
>  			if (likely(m)) {
>  				if (likely(m->pool == free[0]->pool)) {
> --
> 1.8.3.1

Reviewed-by: Morten Brørup <mb@smartsharesystems.com>
Tyler Retzlaff March 27, 2024, 6:14 p.m. UTC | #2 
On Tue, Mar 26, 2024 at 11:16:10AM +0100, Morten Brørup wrote:
> > From: Tyler Retzlaff [mailto:roretzla@linux.microsoft.com]
> > Sent: Wednesday, 20 March 2024 23.02
> > 
> > Don't directly access the cacheline1 field in rte_mbuf struct for
> > prefetch instead just use rte_mbuf_prefetch_part2() to prefetch.
> > 
> > Signed-off-by: Tyler Retzlaff <roretzla@linux.microsoft.com>
> > ---
> >  drivers/net/i40e/i40e_rxtx_vec_avx512.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> > diff --git a/drivers/net/i40e/i40e_rxtx_vec_avx512.c
> > b/drivers/net/i40e/i40e_rxtx_vec_avx512.c
> > index f3050cd..0238b03 100644
> > --- a/drivers/net/i40e/i40e_rxtx_vec_avx512.c
> > +++ b/drivers/net/i40e/i40e_rxtx_vec_avx512.c
> > @@ -826,7 +826,7 @@
> >  		free[0] = m;
> >  		nb_free = 1;
> >  		for (i = 1; i < n; i++) {
> > -			rte_prefetch0(&txep[i + 3].mbuf->cacheline1);
> > +			rte_mbuf_prefetch_part2(txep[i + 3].mbuf);
> 
> @Yuying Zhang:
> If this prefetch is for m->next, I think it can be omitted if m->next is in the first cache line:
> 
> -			rte_prefetch0(&txep[i + 3].mbuf->cacheline1);
> +#if RTE_IOVA_IN_MBUF
> +			rte_mbuf_prefetch_part2(txep[i + 3].mbuf);
> +#endif

Yuying Zhang any reply here to confirm?

If not I will leave it unconditionally prefetch.

> 
> If so, it belongs in a separate patch anyway.
> 
> >  			m = rte_pktmbuf_prefree_seg(txep[i].mbuf);
> >  			if (likely(m)) {
> >  				if (likely(m->pool == free[0]->pool)) {
> > --
> > 1.8.3.1
> 
> Reviewed-by: Morten Brørup <mb@smartsharesystems.com>
Morten Brørup March 27, 2024, 7:45 p.m. UTC | #3 
> From: Tyler Retzlaff [mailto:roretzla@linux.microsoft.com]
> Sent: Wednesday, 27 March 2024 19.15
> 
> On Tue, Mar 26, 2024 at 11:16:10AM +0100, Morten Brørup wrote:
> > > From: Tyler Retzlaff [mailto:roretzla@linux.microsoft.com]
> > > Sent: Wednesday, 20 March 2024 23.02
> > >
> > > Don't directly access the cacheline1 field in rte_mbuf struct for
> > > prefetch instead just use rte_mbuf_prefetch_part2() to prefetch.
> > >
> > > Signed-off-by: Tyler Retzlaff <roretzla@linux.microsoft.com>
> > > ---
> > >  drivers/net/i40e/i40e_rxtx_vec_avx512.c | 2 +-
> > >  1 file changed, 1 insertion(+), 1 deletion(-)
> > >
> > > diff --git a/drivers/net/i40e/i40e_rxtx_vec_avx512.c
> > > b/drivers/net/i40e/i40e_rxtx_vec_avx512.c
> > > index f3050cd..0238b03 100644
> > > --- a/drivers/net/i40e/i40e_rxtx_vec_avx512.c
> > > +++ b/drivers/net/i40e/i40e_rxtx_vec_avx512.c
> > > @@ -826,7 +826,7 @@
> > >  		free[0] = m;
> > >  		nb_free = 1;
> > >  		for (i = 1; i < n; i++) {
> > > -			rte_prefetch0(&txep[i + 3].mbuf->cacheline1);
> > > +			rte_mbuf_prefetch_part2(txep[i + 3].mbuf);
> >
> > @Yuying Zhang:
> > If this prefetch is for m->next, I think it can be omitted if m->next
> is in the first cache line:
> >
> > -			rte_prefetch0(&txep[i + 3].mbuf->cacheline1);
> > +#if RTE_IOVA_IN_MBUF
> > +			rte_mbuf_prefetch_part2(txep[i + 3].mbuf);
> > +#endif
> 
> Yuying Zhang any reply here to confirm?
> 
> If not I will leave it unconditionally prefetch.

I think you should leave it unconditionally prefetch under all circumstances.

An optimization to omit it (if it can be conditionally omitted) belongs in a separate patch. Also, Intel might want to test and document the performance improvement of such a patch.

> 
> >
> > If so, it belongs in a separate patch anyway.
> >
> > >  			m = rte_pktmbuf_prefree_seg(txep[i].mbuf);
> > >  			if (likely(m)) {
> > >  				if (likely(m->pool == free[0]->pool)) {
> > > --
> > > 1.8.3.1
> >
> > Reviewed-by: Morten Brørup <mb@smartsharesystems.com>
diff mbox series

Patch

diff --git a/drivers/net/i40e/i40e_rxtx_vec_avx512.c b/drivers/net/i40e/i40e_rxtx_vec_avx512.c
index f3050cd..0238b03 100644
--- a/drivers/net/i40e/i40e_rxtx_vec_avx512.c
+++ b/drivers/net/i40e/i40e_rxtx_vec_avx512.c
@@ -826,7 +826,7 @@ 
 		free[0] = m;
 		nb_free = 1;
 		for (i = 1; i < n; i++) {
-			rte_prefetch0(&txep[i + 3].mbuf->cacheline1);
+			rte_mbuf_prefetch_part2(txep[i + 3].mbuf);
 			m = rte_pktmbuf_prefree_seg(txep[i].mbuf);
 			if (likely(m)) {
 				if (likely(m->pool == free[0]->pool)) {