[dpdk-dev] [PATCH] app/testpmd: add packet data prefetch in macswap loop

[dpdk-dev] [PATCH] app/testpmd: add packet data prefetch in macswap loop

[Jerin Jacob]

prefetch the next packet data address in advance in macswap loop
for performance improvement.

Signed-off-by: Jerin Jacob <jerin.jacob@caviumnetworks.com>
---
 app/test-pmd/macswap.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/app/test-pmd/macswap.c b/app/test-pmd/macswap.c
index 154889d..c10f4b5 100644
--- a/app/test-pmd/macswap.c
+++ b/app/test-pmd/macswap.c
@@ -113,6 +113,9 @@ pkt_burst_mac_swap(struct fwd_stream *fs)
 	if (txp->tx_ol_flags & TESTPMD_TX_OFFLOAD_INSERT_QINQ)
 		ol_flags |= PKT_TX_QINQ_PKT;
 	for (i = 0; i < nb_rx; i++) {
+		if (likely(i < nb_rx - 1))
+			rte_prefetch0(rte_pktmbuf_mtod(pkts_burst[i + 1],
+						       void *));
 		mb = pkts_burst[i];
 		eth_hdr = rte_pktmbuf_mtod(mb, struct ether_hdr *);
 
-- 
2.1.0

Re: [dpdk-dev] [PATCH] app/testpmd: add packet data prefetch in macswap loop

[De Lara Guarch, Pablo]

Hi Jerin,

> -----Original Message-----
> From: Jerin Jacob [mailto:jerin.jacob@caviumnetworks.com]
> Sent: Monday, May 02, 2016 1:00 PM
> To: dev@dpdk.org
> Cc: De Lara Guarch, Pablo; Jerin Jacob
> Subject: [dpdk-dev] [PATCH] app/testpmd: add packet data prefetch in
> macswap loop
> 
> prefetch the next packet data address in advance in macswap loop
> for performance improvement.
> 
> Signed-off-by: Jerin Jacob <jerin.jacob@caviumnetworks.com>
> ---
>  app/test-pmd/macswap.c | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/app/test-pmd/macswap.c b/app/test-pmd/macswap.c
> index 154889d..c10f4b5 100644
> --- a/app/test-pmd/macswap.c
> +++ b/app/test-pmd/macswap.c
> @@ -113,6 +113,9 @@ pkt_burst_mac_swap(struct fwd_stream *fs)
>  	if (txp->tx_ol_flags & TESTPMD_TX_OFFLOAD_INSERT_QINQ)
>  		ol_flags |= PKT_TX_QINQ_PKT;
>  	for (i = 0; i < nb_rx; i++) {
> +		if (likely(i < nb_rx - 1))
> +			rte_prefetch0(rte_pktmbuf_mtod(pkts_burst[i + 1],
> +						       void *));
>  		mb = pkts_burst[i];
>  		eth_hdr = rte_pktmbuf_mtod(mb, struct ether_hdr *);
> 
> --
> 2.1.0

This looks good. Could you also add it in the other forwarding modes (the ones that make changes in the packets)?

Thanks,
Pablo

Re: [dpdk-dev] [PATCH] app/testpmd: add packet data prefetch in macswap loop

[Bruce Richardson]

On Mon, May 02, 2016 at 05:29:37PM +0530, Jerin Jacob wrote:
> prefetch the next packet data address in advance in macswap loop
> for performance improvement.
> 
> Signed-off-by: Jerin Jacob <jerin.jacob@caviumnetworks.com>
> ---
>  app/test-pmd/macswap.c | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/app/test-pmd/macswap.c b/app/test-pmd/macswap.c
> index 154889d..c10f4b5 100644
> --- a/app/test-pmd/macswap.c
> +++ b/app/test-pmd/macswap.c
> @@ -113,6 +113,9 @@ pkt_burst_mac_swap(struct fwd_stream *fs)
>  	if (txp->tx_ol_flags & TESTPMD_TX_OFFLOAD_INSERT_QINQ)
>  		ol_flags |= PKT_TX_QINQ_PKT;
>  	for (i = 0; i < nb_rx; i++) {
> +		if (likely(i < nb_rx - 1))
> +			rte_prefetch0(rte_pktmbuf_mtod(pkts_burst[i + 1],
> +						       void *));

At least on IA platforms, there is no issue with prefetching beyond the end of
the array, since it's only a hint to the cpu. If this is true for other platforms,
then I suggest we just drop the conditional and just always prefetch.

/Bruce

>  		mb = pkts_burst[i];
>  		eth_hdr = rte_pktmbuf_mtod(mb, struct ether_hdr *);
>  
> -- 
> 2.1.0
> 

Re: [dpdk-dev] [PATCH] app/testpmd: add packet data prefetch in macswap loop

[De Lara Guarch, Pablo]

> -----Original Message-----
> From: Richardson, Bruce
> Sent: Tuesday, May 03, 2016 10:45 AM
> To: Jerin Jacob
> Cc: dev@dpdk.org; De Lara Guarch, Pablo
> Subject: Re: [dpdk-dev] [PATCH] app/testpmd: add packet data prefetch in
> macswap loop
> 
> On Mon, May 02, 2016 at 05:29:37PM +0530, Jerin Jacob wrote:
> > prefetch the next packet data address in advance in macswap loop
> > for performance improvement.
> >
> > Signed-off-by: Jerin Jacob <jerin.jacob@caviumnetworks.com>
> > ---
> >  app/test-pmd/macswap.c | 3 +++
> >  1 file changed, 3 insertions(+)
> >
> > diff --git a/app/test-pmd/macswap.c b/app/test-pmd/macswap.c
> > index 154889d..c10f4b5 100644
> > --- a/app/test-pmd/macswap.c
> > +++ b/app/test-pmd/macswap.c
> > @@ -113,6 +113,9 @@ pkt_burst_mac_swap(struct fwd_stream *fs)
> >  	if (txp->tx_ol_flags & TESTPMD_TX_OFFLOAD_INSERT_QINQ)
> >  		ol_flags |= PKT_TX_QINQ_PKT;
> >  	for (i = 0; i < nb_rx; i++) {
> > +		if (likely(i < nb_rx - 1))
> > +			rte_prefetch0(rte_pktmbuf_mtod(pkts_burst[i + 1],
> > +						       void *));
> 
> At least on IA platforms, there is no issue with prefetching beyond the end of
> the array, since it's only a hint to the cpu. If this is true for other platforms,
> then I suggest we just drop the conditional and just always prefetch.

That's what I thought when I saw this patch, but the problem is that the prefetch is not for pkts_burst,
but for rte_pktmbuf_mtod(pkts_burst...), so there must be a limit in nb_rx - 2, or there will be a seg fault.

Pablo
> 
> /Bruce
> 
> >  		mb = pkts_burst[i];
> >  		eth_hdr = rte_pktmbuf_mtod(mb, struct ether_hdr *);
> >
> > --
> > 2.1.0
> >

Re: [dpdk-dev] [PATCH] app/testpmd: add packet data prefetch in macswap loop

[Ivan Boule]

On 05/03/2016 11:45 AM, Bruce Richardson wrote:
> On Mon, May 02, 2016 at 05:29:37PM +0530, Jerin Jacob wrote:
>> prefetch the next packet data address in advance in macswap loop
>> for performance improvement.
>>
>> ...
>>   	for (i = 0; i < nb_rx; i++) {
>> +		if (likely(i < nb_rx - 1))
>> +			rte_prefetch0(rte_pktmbuf_mtod(pkts_burst[i + 1],
>> +						       void *));
>
> At least on IA platforms, there is no issue with prefetching beyond the end of
> the array, since it's only a hint to the cpu. If this is true for other platforms,
> then I suggest we just drop the conditional and just always prefetch.

This is an interesting point.
Bruce, are you suggesting that prefetching at an invalid [virtual] 
address won't trigger a CPU exception?

Ivan

>
> /Bruce
>
>>   		mb = pkts_burst[i];
>>   		eth_hdr = rte_pktmbuf_mtod(mb, struct ether_hdr *);

Re: [dpdk-dev] [PATCH] app/testpmd: add packet data prefetch in macswap loop

[Bruce Richardson]

On Tue, May 03, 2016 at 10:48:34AM +0100, De Lara Guarch, Pablo wrote:
> 
> 
> > -----Original Message-----
> > From: Richardson, Bruce
> > Sent: Tuesday, May 03, 2016 10:45 AM
> > To: Jerin Jacob
> > Cc: dev@dpdk.org; De Lara Guarch, Pablo
> > Subject: Re: [dpdk-dev] [PATCH] app/testpmd: add packet data prefetch in
> > macswap loop
> > 
> > On Mon, May 02, 2016 at 05:29:37PM +0530, Jerin Jacob wrote:
> > > prefetch the next packet data address in advance in macswap loop
> > > for performance improvement.
> > >
> > > Signed-off-by: Jerin Jacob <jerin.jacob@caviumnetworks.com>
> > > ---
> > >  app/test-pmd/macswap.c | 3 +++
> > >  1 file changed, 3 insertions(+)
> > >
> > > diff --git a/app/test-pmd/macswap.c b/app/test-pmd/macswap.c
> > > index 154889d..c10f4b5 100644
> > > --- a/app/test-pmd/macswap.c
> > > +++ b/app/test-pmd/macswap.c
> > > @@ -113,6 +113,9 @@ pkt_burst_mac_swap(struct fwd_stream *fs)
> > >  	if (txp->tx_ol_flags & TESTPMD_TX_OFFLOAD_INSERT_QINQ)
> > >  		ol_flags |= PKT_TX_QINQ_PKT;
> > >  	for (i = 0; i < nb_rx; i++) {
> > > +		if (likely(i < nb_rx - 1))
> > > +			rte_prefetch0(rte_pktmbuf_mtod(pkts_burst[i + 1],
> > > +						       void *));
> > 
> > At least on IA platforms, there is no issue with prefetching beyond the end of
> > the array, since it's only a hint to the cpu. If this is true for other platforms,
> > then I suggest we just drop the conditional and just always prefetch.
> 
> That's what I thought when I saw this patch, but the problem is that the prefetch is not for pkts_burst,
> but for rte_pktmbuf_mtod(pkts_burst...), so there must be a limit in nb_rx - 2, or there will be a seg fault.
> 
> Pablo
Good point, Pablo, I missed that subtlety here.

/Bruce
> > 
> > /Bruce
> > 
> > >  		mb = pkts_burst[i];
> > >  		eth_hdr = rte_pktmbuf_mtod(mb, struct ether_hdr *);
> > >
> > > --
> > > 2.1.0
> > >

Re: [dpdk-dev] [PATCH] app/testpmd: add packet data prefetch in macswap loop

[Bruce Richardson]

On Tue, May 03, 2016 at 11:50:31AM +0200, Ivan Boule wrote:
> On 05/03/2016 11:45 AM, Bruce Richardson wrote:
> >On Mon, May 02, 2016 at 05:29:37PM +0530, Jerin Jacob wrote:
> >>prefetch the next packet data address in advance in macswap loop
> >>for performance improvement.
> >>
> >>...
> >>  	for (i = 0; i < nb_rx; i++) {
> >>+		if (likely(i < nb_rx - 1))
> >>+			rte_prefetch0(rte_pktmbuf_mtod(pkts_burst[i + 1],
> >>+						       void *));
> >
> >At least on IA platforms, there is no issue with prefetching beyond the end of
> >the array, since it's only a hint to the cpu. If this is true for other platforms,
> >then I suggest we just drop the conditional and just always prefetch.
> 
> This is an interesting point.
> Bruce, are you suggesting that prefetching at an invalid [virtual] address
> won't trigger a CPU exception?
> 

Yep. For example, adding "rte_prefetch0(NULL)" at the start of main in testpmd
causes no ill effects when running the app.

/Bruce

Re: [dpdk-dev] [PATCH] app/testpmd: add packet data prefetch in macswap loop

[Jerin Jacob]

On Mon, May 02, 2016 at 05:48:02PM +0000, De Lara Guarch, Pablo wrote:
> Hi Jerin,
> 
> > -----Original Message-----
> > From: Jerin Jacob [mailto:jerin.jacob@caviumnetworks.com]
> > Sent: Monday, May 02, 2016 1:00 PM
> > To: dev@dpdk.org
> > Cc: De Lara Guarch, Pablo; Jerin Jacob
> > Subject: [dpdk-dev] [PATCH] app/testpmd: add packet data prefetch in
> > macswap loop
> > 
> > prefetch the next packet data address in advance in macswap loop
> > for performance improvement.
> > 
> > Signed-off-by: Jerin Jacob <jerin.jacob@caviumnetworks.com>
> > ---
> >  app/test-pmd/macswap.c | 3 +++
> >  1 file changed, 3 insertions(+)
> > 
> > diff --git a/app/test-pmd/macswap.c b/app/test-pmd/macswap.c
> > index 154889d..c10f4b5 100644
> > --- a/app/test-pmd/macswap.c
> > +++ b/app/test-pmd/macswap.c
> > @@ -113,6 +113,9 @@ pkt_burst_mac_swap(struct fwd_stream *fs)
> >  	if (txp->tx_ol_flags & TESTPMD_TX_OFFLOAD_INSERT_QINQ)
> >  		ol_flags |= PKT_TX_QINQ_PKT;
> >  	for (i = 0; i < nb_rx; i++) {
> > +		if (likely(i < nb_rx - 1))
> > +			rte_prefetch0(rte_pktmbuf_mtod(pkts_burst[i + 1],
> > +						       void *));
> >  		mb = pkts_burst[i];
> >  		eth_hdr = rte_pktmbuf_mtod(mb, struct ether_hdr *);
> > 
> > --
> > 2.1.0
> 
> This looks good. Could you also add it in the other forwarding modes (the ones that make changes in the packets)?

OK Pablo.

I will add the similar logic in the following forwarding modes in
testpmd.

macswap
macfwd
macfwd-retry
csumonly
icmpecho

/Jerin

> 
> Thanks,
> Pablo

[dpdk-dev] [PATCH v2] app/testpmd: add packet data pointer prefetch in the forwarding loop

[Jerin Jacob]

prefetch the next packet data address in advance in forwarding loop
for performance improvement.

Signed-off-by: Jerin Jacob <jerin.jacob@caviumnetworks.com>
---
V2:
Extend the packet data prefetch logic to other similar forwarding modes like
macfwd, macfwd-retry, csumonly, icmpecho as suggested by Pablo
http://dpdk.org/dev/patchwork/patch/12330/
---
 app/test-pmd/csumonly.c     | 3 +++
 app/test-pmd/icmpecho.c     | 3 +++
 app/test-pmd/macfwd-retry.c | 3 +++
 app/test-pmd/macfwd.c       | 3 +++
 app/test-pmd/macswap.c      | 3 +++
 5 files changed, 15 insertions(+)

diff --git a/app/test-pmd/csumonly.c b/app/test-pmd/csumonly.c
index 7e4f662..1d6cda1 100644
--- a/app/test-pmd/csumonly.c
+++ b/app/test-pmd/csumonly.c
@@ -676,6 +676,9 @@ pkt_burst_checksum_forward(struct fwd_stream *fs)
 	info.tso_segsz = txp->tso_segsz;
 
 	for (i = 0; i < nb_rx; i++) {
+		if (likely(i < nb_rx - 1))
+			rte_prefetch0(rte_pktmbuf_mtod(pkts_burst[i + 1],
+						       void *));
 
 		ol_flags = 0;
 		info.is_tunnel = 0;
diff --git a/app/test-pmd/icmpecho.c b/app/test-pmd/icmpecho.c
index e510f9b..ed6e924 100644
--- a/app/test-pmd/icmpecho.c
+++ b/app/test-pmd/icmpecho.c
@@ -346,6 +346,9 @@ reply_to_icmp_echo_rqsts(struct fwd_stream *fs)
 	fs->rx_packets += nb_rx;
 	nb_replies = 0;
 	for (i = 0; i < nb_rx; i++) {
+		if (likely(i < nb_rx - 1))
+			rte_prefetch0(rte_pktmbuf_mtod(pkts_burst[i + 1],
+						       void *));
 		pkt = pkts_burst[i];
 		eth_h = rte_pktmbuf_mtod(pkt, struct ether_hdr *);
 		eth_type = RTE_BE_TO_CPU_16(eth_h->ether_type);
diff --git a/app/test-pmd/macfwd-retry.c b/app/test-pmd/macfwd-retry.c
index 3a96b3d..d8cd069 100644
--- a/app/test-pmd/macfwd-retry.c
+++ b/app/test-pmd/macfwd-retry.c
@@ -117,6 +117,9 @@ pkt_burst_mac_retry_forward(struct fwd_stream *fs)
 #endif
 	fs->rx_packets += nb_rx;
 	for (i = 0; i < nb_rx; i++) {
+		if (likely(i < nb_rx - 1))
+			rte_prefetch0(rte_pktmbuf_mtod(pkts_burst[i + 1],
+						       void *));
 		mb = pkts_burst[i];
 		eth_hdr = rte_pktmbuf_mtod(mb, struct ether_hdr *);
 		ether_addr_copy(&peer_eth_addrs[fs->peer_addr],
diff --git a/app/test-pmd/macfwd.c b/app/test-pmd/macfwd.c
index 3b7fffb..07a399a 100644
--- a/app/test-pmd/macfwd.c
+++ b/app/test-pmd/macfwd.c
@@ -113,6 +113,9 @@ pkt_burst_mac_forward(struct fwd_stream *fs)
 	if (txp->tx_ol_flags & TESTPMD_TX_OFFLOAD_INSERT_QINQ)
 		ol_flags |= PKT_TX_QINQ_PKT;
 	for (i = 0; i < nb_rx; i++) {
+		if (likely(i < nb_rx - 1))
+			rte_prefetch0(rte_pktmbuf_mtod(pkts_burst[i + 1],
+						       void *));
 		mb = pkts_burst[i];
 		eth_hdr = rte_pktmbuf_mtod(mb, struct ether_hdr *);
 		ether_addr_copy(&peer_eth_addrs[fs->peer_addr],
diff --git a/app/test-pmd/macswap.c b/app/test-pmd/macswap.c
index 154889d..c10f4b5 100644
--- a/app/test-pmd/macswap.c
+++ b/app/test-pmd/macswap.c
@@ -113,6 +113,9 @@ pkt_burst_mac_swap(struct fwd_stream *fs)
 	if (txp->tx_ol_flags & TESTPMD_TX_OFFLOAD_INSERT_QINQ)
 		ol_flags |= PKT_TX_QINQ_PKT;
 	for (i = 0; i < nb_rx; i++) {
+		if (likely(i < nb_rx - 1))
+			rte_prefetch0(rte_pktmbuf_mtod(pkts_burst[i + 1],
+						       void *));
 		mb = pkts_burst[i];
 		eth_hdr = rte_pktmbuf_mtod(mb, struct ether_hdr *);
 
-- 
2.1.0

Re: [dpdk-dev] [PATCH v2] app/testpmd: add packet data pointer prefetch in the forwarding loop

[Thomas Monjalon]

2016-05-03 19:37, Jerin Jacob:
> prefetch the next packet data address in advance in forwarding loop
> for performance improvement.

I would like to get some attention to other patches adding some prefetch:
	http://dpdk.org/dev/patchwork/project/dpdk/list/?q=prefetch

	http://dpdk.org/patch/4678
	http://dpdk.org/patch/8867

Maybe they deserve to be discussed. Thanks

Re: [dpdk-dev] [PATCH] app/testpmd: add packet data prefetch in macswap loop

[Ananyev, Konstantin]

> -----Original Message-----
> From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Bruce Richardson
> Sent: Tuesday, May 03, 2016 11:20 AM
> To: Ivan Boule
> Cc: Jerin Jacob; dev@dpdk.org; De Lara Guarch, Pablo
> Subject: Re: [dpdk-dev] [PATCH] app/testpmd: add packet data prefetch in macswap loop
> 
> On Tue, May 03, 2016 at 11:50:31AM +0200, Ivan Boule wrote:
> > On 05/03/2016 11:45 AM, Bruce Richardson wrote:
> > >On Mon, May 02, 2016 at 05:29:37PM +0530, Jerin Jacob wrote:
> > >>prefetch the next packet data address in advance in macswap loop
> > >>for performance improvement.
> > >>
> > >>...
> > >>  	for (i = 0; i < nb_rx; i++) {
> > >>+		if (likely(i < nb_rx - 1))
> > >>+			rte_prefetch0(rte_pktmbuf_mtod(pkts_burst[i + 1],
> > >>+						       void *));
> > >
> > >At least on IA platforms, there is no issue with prefetching beyond the end of
> > >the array, since it's only a hint to the cpu. If this is true for other platforms,
> > >then I suggest we just drop the conditional and just always prefetch.
> >
> > This is an interesting point.
> > Bruce, are you suggesting that prefetching at an invalid [virtual] address
> > won't trigger a CPU exception?
> >
> 
> Yep. For example, adding "rte_prefetch0(NULL)" at the start of main in testpmd
> causes no ill effects when running the app.
> 

One correction - while on IA prefetch(inval_addr) wouldn't cause any functional problems,
it still might cause DTLB miss and can be a source of noticeable performance degradation.
So it is better to avoid such constructions for performance critical code.
Konstantin

Re: [dpdk-dev] [PATCH v2] app/testpmd: add packet data pointer prefetch in the forwarding loop

[De Lara Guarch, Pablo]

> -----Original Message-----
> From: Jerin Jacob [mailto:jerin.jacob@caviumnetworks.com]
> Sent: Tuesday, May 03, 2016 3:08 PM
> To: dev@dpdk.org
> Cc: Richardson, Bruce; thomas.monjalon@6wind.com; De Lara Guarch,
> Pablo; Jerin Jacob
> Subject: [dpdk-dev] [PATCH v2] app/testpmd: add packet data pointer
> prefetch in the forwarding loop
> 
> prefetch the next packet data address in advance in forwarding loop
> for performance improvement.
> 
> Signed-off-by: Jerin Jacob <jerin.jacob@caviumnetworks.com>

Patch looks good, but there is going to be a conflict with http://dpdk.org/dev/patchwork/patch/13134/,
since that patch is removing the mac_retry forwarding mode.

Apart from that,
Acked-by: Pablo de Lara <pablo.de.lara.guarch@intel.com>

Re: [dpdk-dev] [PATCH v2] app/testpmd: add packet data pointer prefetch in the forwarding loop

[Thomas Monjalon]

2016-06-07 11:19, De Lara Guarch, Pablo:
> From: Jerin Jacob [mailto:jerin.jacob@caviumnetworks.com]
> > prefetch the next packet data address in advance in forwarding loop
> > for performance improvement.
> > 
> > Signed-off-by: Jerin Jacob <jerin.jacob@caviumnetworks.com>
> 
> Patch looks good, but there is going to be a conflict with http://dpdk.org/dev/patchwork/patch/13134/,
> since that patch is removing the mac_retry forwarding mode.
> 
> Apart from that,
> Acked-by: Pablo de Lara <pablo.de.lara.guarch@intel.com>

Applied, thanks