[dpdk-dev] [PATCH 1/2] app/testpmd: fix scatter offload configuration

[dpdk-dev] [PATCH 1/2] app/testpmd: fix scatter offload configuration

[Matan Azrad]

When the mbuf data size cannot contain the maximum Rx packet length with
the mbuf headroom, a packet should be scattered in more than one mbuf.

The application did not configure scatter offload in the above case.

Enable the Rx scatter offload in the above case.

Fixes: 33f9630fc23d ("app/testpmd: create mbuf based on max supported segments")
Cc: stable@dpdk.org

Signed-off-by: Matan Azrad <matan@mellanox.com>
---
 app/test-pmd/testpmd.c | 11 +++++++++++
 1 file changed, 11 insertions(+)

diff --git a/app/test-pmd/testpmd.c b/app/test-pmd/testpmd.c
index 518865a..4ae70ef 100644
--- a/app/test-pmd/testpmd.c
+++ b/app/test-pmd/testpmd.c
@@ -1191,6 +1191,17 @@ struct extmem_param {
 				warning = 1;
 			}
 		}
+		if (rx_mode.max_rx_pkt_len + RTE_PKTMBUF_HEADROOM >
+		    mbuf_data_size) {
+			if (port->dev_info.rx_queue_offload_capa &
+			    DEV_RX_OFFLOAD_SCATTER)
+				port->dev_conf.rxmode.offloads |=
+						DEV_RX_OFFLOAD_SCATTER;
+			else
+				TESTPMD_LOG(WARNING, "Configure scatter is"
+					    " needed and cannot be configured"
+					    " in the port %u\n", pid);
+		}
 	}
 
 	if (warning)
-- 
1.8.3.1

[dpdk-dev] [PATCH 2/2] app/testpmd: add bits per second to statistics

[Matan Azrad]

Traffic bps statistics is very useful for performance testing.

Add bits per second statistics for Rx and Tx.

Signed-off-by: Matan Azrad <matan@mellanox.com>
---
 app/test-pmd/config.c | 24 ++++++++++++++++++++----
 1 file changed, 20 insertions(+), 4 deletions(-)

diff --git a/app/test-pmd/config.c b/app/test-pmd/config.c
index 1a5a5c1..88bc716 100644
--- a/app/test-pmd/config.c
+++ b/app/test-pmd/config.c
@@ -119,9 +119,12 @@
 {
 	static uint64_t prev_pkts_rx[RTE_MAX_ETHPORTS];
 	static uint64_t prev_pkts_tx[RTE_MAX_ETHPORTS];
+	static uint64_t prev_bytes_rx[RTE_MAX_ETHPORTS];
+	static uint64_t prev_bytes_tx[RTE_MAX_ETHPORTS];
 	static uint64_t prev_cycles[RTE_MAX_ETHPORTS];
-	uint64_t diff_pkts_rx, diff_pkts_tx, diff_cycles;
-	uint64_t mpps_rx, mpps_tx;
+	uint64_t diff_pkts_rx, diff_pkts_tx, diff_bytes_rx, diff_bytes_tx,
+								diff_cycles;
+	uint64_t mpps_rx, mpps_tx, mbps_rx, mbps_tx;
 	struct rte_eth_stats stats;
 	struct rte_port *port = &ports[port_id];
 	uint8_t i;
@@ -192,9 +195,22 @@
 		diff_pkts_rx * rte_get_tsc_hz() / diff_cycles : 0;
 	mpps_tx = diff_cycles > 0 ?
 		diff_pkts_tx * rte_get_tsc_hz() / diff_cycles : 0;
+
+	diff_bytes_rx = (stats.ibytes > prev_bytes_rx[port_id]) ?
+		(stats.ibytes - prev_bytes_rx[port_id]) : 0;
+	diff_bytes_tx = (stats.obytes > prev_bytes_tx[port_id]) ?
+		(stats.obytes - prev_bytes_tx[port_id]) : 0;
+	prev_bytes_rx[port_id] = stats.ibytes;
+	prev_bytes_tx[port_id] = stats.obytes;
+	mbps_rx = diff_cycles > 0 ?
+		diff_bytes_rx * rte_get_tsc_hz() / diff_cycles : 0;
+	mbps_tx = diff_cycles > 0 ?
+		diff_bytes_tx * rte_get_tsc_hz() / diff_cycles : 0;
+
 	printf("\n  Throughput (since last show)\n");
-	printf("  Rx-pps: %12"PRIu64"\n  Tx-pps: %12"PRIu64"\n",
-			mpps_rx, mpps_tx);
+	printf("  Rx-pps: %12"PRIu64"          Rx-bps: %12"PRIu64"\n  Tx-pps: %12"
+	       PRIu64"          Tx-bps: %12"PRIu64"\n", mpps_rx, mbps_rx * 8,
+	       mpps_tx, mbps_tx * 8);
 
 	printf("  %s############################%s\n",
 	       nic_stats_border, nic_stats_border);
-- 
1.8.3.1

Re: [dpdk-dev] [PATCH 2/2] app/testpmd: add bits per second to statistics

[Moti Haimovsky]

> -----Original Message-----
> From: dev <dev-bounces@dpdk.org> On Behalf Of Matan Azrad
> Sent: Monday, July 29, 2019 3:37 PM
> To: Wenzhuo Lu <wenzhuo.lu@intel.com>; Jingjing Wu
> <jingjing.wu@intel.com>
> Cc: dev@dpdk.org
> Subject: [dpdk-dev] [PATCH 2/2] app/testpmd: add bits per second to
> statistics
> 
> Traffic bps statistics is very useful for performance testing.
> 
> Add bits per second statistics for Rx and Tx.
> 
> Signed-off-by: Matan Azrad <matan@mellanox.com>
Acked-by: Moti Haimovsky <motih@mellanox.com>
> ---
>  app/test-pmd/config.c | 24 ++++++++++++++++++++----
>  1 file changed, 20 insertions(+), 4 deletions(-)
> 
> diff --git a/app/test-pmd/config.c b/app/test-pmd/config.c index
> 1a5a5c1..88bc716 100644
> --- a/app/test-pmd/config.c
> +++ b/app/test-pmd/config.c
> @@ -119,9 +119,12 @@
>  {
>  	static uint64_t prev_pkts_rx[RTE_MAX_ETHPORTS];
>  	static uint64_t prev_pkts_tx[RTE_MAX_ETHPORTS];
> +	static uint64_t prev_bytes_rx[RTE_MAX_ETHPORTS];
> +	static uint64_t prev_bytes_tx[RTE_MAX_ETHPORTS];
>  	static uint64_t prev_cycles[RTE_MAX_ETHPORTS];
> -	uint64_t diff_pkts_rx, diff_pkts_tx, diff_cycles;
> -	uint64_t mpps_rx, mpps_tx;
> +	uint64_t diff_pkts_rx, diff_pkts_tx, diff_bytes_rx, diff_bytes_tx,
> +								diff_cycles;
> +	uint64_t mpps_rx, mpps_tx, mbps_rx, mbps_tx;
>  	struct rte_eth_stats stats;
>  	struct rte_port *port = &ports[port_id];
>  	uint8_t i;
> @@ -192,9 +195,22 @@
>  		diff_pkts_rx * rte_get_tsc_hz() / diff_cycles : 0;
>  	mpps_tx = diff_cycles > 0 ?
>  		diff_pkts_tx * rte_get_tsc_hz() / diff_cycles : 0;
> +
> +	diff_bytes_rx = (stats.ibytes > prev_bytes_rx[port_id]) ?
> +		(stats.ibytes - prev_bytes_rx[port_id]) : 0;
> +	diff_bytes_tx = (stats.obytes > prev_bytes_tx[port_id]) ?
> +		(stats.obytes - prev_bytes_tx[port_id]) : 0;
> +	prev_bytes_rx[port_id] = stats.ibytes;
> +	prev_bytes_tx[port_id] = stats.obytes;
> +	mbps_rx = diff_cycles > 0 ?
> +		diff_bytes_rx * rte_get_tsc_hz() / diff_cycles : 0;
> +	mbps_tx = diff_cycles > 0 ?
> +		diff_bytes_tx * rte_get_tsc_hz() / diff_cycles : 0;
> +
>  	printf("\n  Throughput (since last show)\n");
> -	printf("  Rx-pps: %12"PRIu64"\n  Tx-pps: %12"PRIu64"\n",
> -			mpps_rx, mpps_tx);
> +	printf("  Rx-pps: %12"PRIu64"          Rx-bps: %12"PRIu64"\n  Tx-
> pps: %12"
> +	       PRIu64"          Tx-bps: %12"PRIu64"\n", mpps_rx, mbps_rx * 8,
> +	       mpps_tx, mbps_tx * 8);
> 
>  	printf("  %s############################%s\n",
>  	       nic_stats_border, nic_stats_border);
> --
> 1.8.3.1

Re: [dpdk-dev] [PATCH 2/2] app/testpmd: add bits per second to statistics

[Yigit, Ferruh]

On 7/30/2019 12:41 PM, Moti Haimovsky wrote:
>> -----Original Message-----
>> From: dev <dev-bounces@dpdk.org> On Behalf Of Matan Azrad
>> Sent: Monday, July 29, 2019 3:37 PM
>> To: Wenzhuo Lu <wenzhuo.lu@intel.com>; Jingjing Wu
>> <jingjing.wu@intel.com>
>> Cc: dev@dpdk.org
>> Subject: [dpdk-dev] [PATCH 2/2] app/testpmd: add bits per second to
>> statistics
>>
>> Traffic bps statistics is very useful for performance testing.
>>
>> Add bits per second statistics for Rx and Tx.
>>
>> Signed-off-by: Matan Azrad <matan@mellanox.com>
> Acked-by: Moti Haimovsky <motih@mellanox.com>

Reviewed-by: Ferruh Yigit <ferruh.yigit@intel.com>

Applied to dpdk-next-net/master, thanks.

Re: [dpdk-dev] [PATCH 1/2] app/testpmd: fix scatter offload configuration

[Matan Azrad]

Hi all

Any review?

> -----Original Message-----
> From: dev <dev-bounces@dpdk.org> On Behalf Of Matan Azrad
> Sent: Monday, July 29, 2019 3:37 PM
> To: Wenzhuo Lu <wenzhuo.lu@intel.com>; Jingjing Wu
> <jingjing.wu@intel.com>
> Cc: dev@dpdk.org; stable@dpdk.org
> Subject: [dpdk-dev] [PATCH 1/2] app/testpmd: fix scatter offload
> configuration
> 
> When the mbuf data size cannot contain the maximum Rx packet length with
> the mbuf headroom, a packet should be scattered in more than one mbuf.
> 
> The application did not configure scatter offload in the above case.
> 
> Enable the Rx scatter offload in the above case.
> 
> Fixes: 33f9630fc23d ("app/testpmd: create mbuf based on max supported
> segments")
> Cc: stable@dpdk.org
> 
> Signed-off-by: Matan Azrad <matan@mellanox.com>
> ---
>  app/test-pmd/testpmd.c | 11 +++++++++++
>  1 file changed, 11 insertions(+)
> 
> diff --git a/app/test-pmd/testpmd.c b/app/test-pmd/testpmd.c index
> 518865a..4ae70ef 100644
> --- a/app/test-pmd/testpmd.c
> +++ b/app/test-pmd/testpmd.c
> @@ -1191,6 +1191,17 @@ struct extmem_param {
>  				warning = 1;
>  			}
>  		}
> +		if (rx_mode.max_rx_pkt_len + RTE_PKTMBUF_HEADROOM
> >
> +		    mbuf_data_size) {
> +			if (port->dev_info.rx_queue_offload_capa &
> +			    DEV_RX_OFFLOAD_SCATTER)
> +				port->dev_conf.rxmode.offloads |=
> +						DEV_RX_OFFLOAD_SCATTER;
> +			else
> +				TESTPMD_LOG(WARNING, "Configure
> scatter is"
> +					    " needed and cannot be
> configured"
> +					    " in the port %u\n", pid);
> +		}
>  	}
> 
>  	if (warning)
> --
> 1.8.3.1

Re: [dpdk-dev] [PATCH 1/2] app/testpmd: fix scatter offload configuration

[Moti Haimovsky]

> > -----Original Message-----
> > From: dev <dev-bounces@dpdk.org> On Behalf Of Matan Azrad
> > Sent: Monday, July 29, 2019 3:37 PM
> > To: Wenzhuo Lu <wenzhuo.lu@intel.com>; Jingjing Wu
> > <jingjing.wu@intel.com>
> > Cc: dev@dpdk.org; stable@dpdk.org
> > Subject: [dpdk-dev] [PATCH 1/2] app/testpmd: fix scatter offload
> > configuration
> >
> > When the mbuf data size cannot contain the maximum Rx packet length
> > with the mbuf headroom, a packet should be scattered in more than one
> mbuf.
> >
> > The application did not configure scatter offload in the above case.
> >
> > Enable the Rx scatter offload in the above case.
> >
> > Fixes: 33f9630fc23d ("app/testpmd: create mbuf based on max supported
> > segments")
> > Cc: stable@dpdk.org
> >
> > Signed-off-by: Matan Azrad <matan@mellanox.com>
Acked-by: Moti Haimovsky <motih@mellanox.com>
> > ---
> >  app/test-pmd/testpmd.c | 11 +++++++++++
> >  1 file changed, 11 insertions(+)
> >
> > diff --git a/app/test-pmd/testpmd.c b/app/test-pmd/testpmd.c index
> > 518865a..4ae70ef 100644
> > --- a/app/test-pmd/testpmd.c
> > +++ b/app/test-pmd/testpmd.c
> > @@ -1191,6 +1191,17 @@ struct extmem_param {
> >  				warning = 1;
> >  			}
> >  		}
> > +		if (rx_mode.max_rx_pkt_len + RTE_PKTMBUF_HEADROOM
> > >
> > +		    mbuf_data_size) {
> > +			if (port->dev_info.rx_queue_offload_capa &
> > +			    DEV_RX_OFFLOAD_SCATTER)
> > +				port->dev_conf.rxmode.offloads |=
> > +						DEV_RX_OFFLOAD_SCATTER;
> > +			else
> > +				TESTPMD_LOG(WARNING, "Configure
> > scatter is"
> > +					    " needed and cannot be
> > configured"
> > +					    " in the port %u\n", pid);
> > +		}
> >  	}
> >
> >  	if (warning)
> > --
> > 1.8.3.1

Re: [dpdk-dev] [PATCH 1/2] app/testpmd: fix scatter offload configuration

[Ferruh Yigit]

On 7/29/2019 1:36 PM, Matan Azrad wrote:
> When the mbuf data size cannot contain the maximum Rx packet length with
> the mbuf headroom, a packet should be scattered in more than one mbuf.
> 
> The application did not configure scatter offload in the above case.
> 
> Enable the Rx scatter offload in the above case.
> 
> Fixes: 33f9630fc23d ("app/testpmd: create mbuf based on max supported segments")
> Cc: stable@dpdk.org
> 
> Signed-off-by: Matan Azrad <matan@mellanox.com>

Deferring the patchset to next release, they were late anyway and not actually
fixing a defect, safer to defer than getting them in rc3.

Re: [dpdk-dev] [PATCH 1/2] app/testpmd: fix scatter offload configuration

[Matan Azrad]

Hi Ferruh

From: Ferruh Yigit
> Sent: Tuesday, July 30, 2019 4:09 PM
> To: Matan Azrad <matan@mellanox.com>; Wenzhuo Lu
> <wenzhuo.lu@intel.com>; Jingjing Wu <jingjing.wu@intel.com>
> Cc: dev@dpdk.org; stable@dpdk.org
> Subject: Re: [dpdk-dev] [PATCH 1/2] app/testpmd: fix scatter offload
> configuration
> 
> On 7/29/2019 1:36 PM, Matan Azrad wrote:
> > When the mbuf data size cannot contain the maximum Rx packet length
> > with the mbuf headroom, a packet should be scattered in more than one
> mbuf.
> >
> > The application did not configure scatter offload in the above case.
> >
> > Enable the Rx scatter offload in the above case.
> >
> > Fixes: 33f9630fc23d ("app/testpmd: create mbuf based on max supported
> > segments")
> > Cc: stable@dpdk.org
> >
> > Signed-off-by: Matan Azrad <matan@mellanox.com>
> 
> Deferring the patchset to next release, they were late anyway and not
> actually fixing a defect, safer to defer than getting them in rc3.

Yes this patch came late for RC3 but it is a fix.

What are you concerns here?
Why don't you think defect found?

What's about RC4?

Matan

Re: [dpdk-dev] [PATCH 1/2] app/testpmd: fix scatter offload configuration

[Ferruh Yigit]

On 7/30/2019 2:17 PM, Matan Azrad wrote:
> Hi Ferruh
> 
> From: Ferruh Yigit
>> Sent: Tuesday, July 30, 2019 4:09 PM
>> To: Matan Azrad <matan@mellanox.com>; Wenzhuo Lu
>> <wenzhuo.lu@intel.com>; Jingjing Wu <jingjing.wu@intel.com>
>> Cc: dev@dpdk.org; stable@dpdk.org
>> Subject: Re: [dpdk-dev] [PATCH 1/2] app/testpmd: fix scatter offload
>> configuration
>>
>> On 7/29/2019 1:36 PM, Matan Azrad wrote:
>>> When the mbuf data size cannot contain the maximum Rx packet length
>>> with the mbuf headroom, a packet should be scattered in more than one
>> mbuf.
>>>
>>> The application did not configure scatter offload in the above case.
>>>
>>> Enable the Rx scatter offload in the above case.
>>>
>>> Fixes: 33f9630fc23d ("app/testpmd: create mbuf based on max supported
>>> segments")
>>> Cc: stable@dpdk.org
>>>
>>> Signed-off-by: Matan Azrad <matan@mellanox.com>
>>
>> Deferring the patchset to next release, they were late anyway and not
>> actually fixing a defect, safer to defer than getting them in rc3.
> 
> Yes this patch came late for RC3 but it is a fix.
> 
> What are you concerns here?
> Why don't you think defect found?

First patch changes the behavior, when mbuf size is larger than configured size
and user didn't provided the scatter offload, should test application
automatically enable it? It may or not, but this is the change of the behavior,
I think not a fix.

And second patch adds more detail into the statistics, so I believe it is clear
that it is not a fix.

The concern is getting changes very close to release, to balance between risk
and benefit of the feature. I don't see any reason why these changes can't wait
next release, so I don't see any reason to get the risk.

> 
> What's about RC4?

No, it is even worse, there will be only a little testing after rc4 and a little
time before release.

Re: [dpdk-dev] [PATCH 1/2] app/testpmd: fix scatter offload configuration

[Matan Azrad]

Hi Ferruh

 From: Ferruh Yigit
> Sent: Tuesday, July 30, 2019 6:22 PM
> To: Matan Azrad <matan@mellanox.com>; Wenzhuo Lu
> <wenzhuo.lu@intel.com>; Jingjing Wu <jingjing.wu@intel.com>
> Cc: dev@dpdk.org; stable@dpdk.org
> Subject: Re: [dpdk-dev] [PATCH 1/2] app/testpmd: fix scatter offload
> configuration
> 
> On 7/30/2019 2:17 PM, Matan Azrad wrote:
> > Hi Ferruh
> >
> > From: Ferruh Yigit
> >> Sent: Tuesday, July 30, 2019 4:09 PM
> >> To: Matan Azrad <matan@mellanox.com>; Wenzhuo Lu
> >> <wenzhuo.lu@intel.com>; Jingjing Wu <jingjing.wu@intel.com>
> >> Cc: dev@dpdk.org; stable@dpdk.org
> >> Subject: Re: [dpdk-dev] [PATCH 1/2] app/testpmd: fix scatter offload
> >> configuration
> >>
> >> On 7/29/2019 1:36 PM, Matan Azrad wrote:
> >>> When the mbuf data size cannot contain the maximum Rx packet length
> >>> with the mbuf headroom, a packet should be scattered in more than
> >>> one
> >> mbuf.
> >>>
> >>> The application did not configure scatter offload in the above case.
> >>>
> >>> Enable the Rx scatter offload in the above case.
> >>>
> >>> Fixes: 33f9630fc23d ("app/testpmd: create mbuf based on max
> >>> supported
> >>> segments")
> >>> Cc: stable@dpdk.org
> >>>
> >>> Signed-off-by: Matan Azrad <matan@mellanox.com>
> >>
> >> Deferring the patchset to next release, they were late anyway and not
> >> actually fixing a defect, safer to defer than getting them in rc3.
> >
> > Yes this patch came late for RC3 but it is a fix.
> >
> > What are you concerns here?
> > Why don't you think defect found?
> 
> First patch changes the behavior, when mbuf size is larger than configured
> size and user didn't provided the scatter offload, should test application
> automatically enable it?

No, only when the mbuf size is smaller than the max_rx_pkt_len with headroom.
If scatter is not enabled in the above case, how can the PMD provide a packet with max_rx_pkt_len size? 

I think not enabling scatter in this case it is a user conflict in configuration and should raise an error in the PMD.  Maybe even in ethdev layer.

> It may or not, but this is the change of the behavior, I
> think not a fix.
> 
> And second patch adds more detail into the statistics, so I believe it is clear
> that it is not a fix.
 
 Agree, this can wait.

> The concern is getting changes very close to release, to balance between risk
> and benefit of the feature. I don't see any reason why these changes can't
> wait next release, so I don't see any reason to get the risk.

When  I changed the default max_rx_pkt_len and mbuf size in LRO testing I met this issue.

By default scatter will not be enabled.


> > What's about RC4?
> 
> No, it is even worse, there will be only a little testing after rc4 and a little time
> before release.

So, I hope it will be integrated in RC3.

Re: [dpdk-dev] [PATCH 1/2] app/testpmd: fix scatter offload configuration

[Ferruh Yigit]

On 7/30/2019 4:56 PM, Matan Azrad wrote:
> Hi Ferruh
> 
>  From: Ferruh Yigit
>> Sent: Tuesday, July 30, 2019 6:22 PM
>> To: Matan Azrad <matan@mellanox.com>; Wenzhuo Lu
>> <wenzhuo.lu@intel.com>; Jingjing Wu <jingjing.wu@intel.com>
>> Cc: dev@dpdk.org; stable@dpdk.org
>> Subject: Re: [dpdk-dev] [PATCH 1/2] app/testpmd: fix scatter offload
>> configuration
>>
>> On 7/30/2019 2:17 PM, Matan Azrad wrote:
>>> Hi Ferruh
>>>
>>> From: Ferruh Yigit
>>>> Sent: Tuesday, July 30, 2019 4:09 PM
>>>> To: Matan Azrad <matan@mellanox.com>; Wenzhuo Lu
>>>> <wenzhuo.lu@intel.com>; Jingjing Wu <jingjing.wu@intel.com>
>>>> Cc: dev@dpdk.org; stable@dpdk.org
>>>> Subject: Re: [dpdk-dev] [PATCH 1/2] app/testpmd: fix scatter offload
>>>> configuration
>>>>
>>>> On 7/29/2019 1:36 PM, Matan Azrad wrote:
>>>>> When the mbuf data size cannot contain the maximum Rx packet length
>>>>> with the mbuf headroom, a packet should be scattered in more than
>>>>> one
>>>> mbuf.
>>>>>
>>>>> The application did not configure scatter offload in the above case.
>>>>>
>>>>> Enable the Rx scatter offload in the above case.
>>>>>
>>>>> Fixes: 33f9630fc23d ("app/testpmd: create mbuf based on max
>>>>> supported
>>>>> segments")
>>>>> Cc: stable@dpdk.org
>>>>>
>>>>> Signed-off-by: Matan Azrad <matan@mellanox.com>
>>>>
>>>> Deferring the patchset to next release, they were late anyway and not
>>>> actually fixing a defect, safer to defer than getting them in rc3.
>>>
>>> Yes this patch came late for RC3 but it is a fix.
>>>
>>> What are you concerns here?
>>> Why don't you think defect found?
>>
>> First patch changes the behavior, when mbuf size is larger than configured
>> size and user didn't provided the scatter offload, should test application
>> automatically enable it?
> 
> No, only when the mbuf size is smaller than the max_rx_pkt_len with headroom.
> If scatter is not enabled in the above case, how can the PMD provide a packet with max_rx_pkt_len size? 
> 
> I think not enabling scatter in this case it is a user conflict in configuration and should raise an error in the PMD.  Maybe even in ethdev layer.
> 
>> It may or not, but this is the change of the behavior, I
>> think not a fix.
>>
>> And second patch adds more detail into the statistics, so I believe it is clear
>> that it is not a fix.
>  
>  Agree, this can wait.
> 
>> The concern is getting changes very close to release, to balance between risk
>> and benefit of the feature. I don't see any reason why these changes can't
>> wait next release, so I don't see any reason to get the risk.
> 
> When  I changed the default max_rx_pkt_len and mbuf size in LRO testing I met this issue.
> 
> By default scatter will not be enabled.

I think it is still arguable if scatter should be enabled by default, but isn't
there a way in testpmd to enable scatter explicitly? If so you have a way to
test LRO.

> 
> 
>>> What's about RC4?
>>
>> No, it is even worse, there will be only a little testing after rc4 and a little time
>> before release.
> 
> So, I hope it will be integrated in RC3.
> 

Re: [dpdk-dev] [PATCH 1/2] app/testpmd: fix scatter offload configuration

[Matan Azrad]

From: Ferruh Yigit 
> On 7/30/2019 4:56 PM, Matan Azrad wrote:
> > Hi Ferruh
> >
> >  From: Ferruh Yigit
> >> Sent: Tuesday, July 30, 2019 6:22 PM
> >> To: Matan Azrad <matan@mellanox.com>; Wenzhuo Lu
> >> <wenzhuo.lu@intel.com>; Jingjing Wu <jingjing.wu@intel.com>
> >> Cc: dev@dpdk.org; stable@dpdk.org
> >> Subject: Re: [dpdk-dev] [PATCH 1/2] app/testpmd: fix scatter offload
> >> configuration
> >>
> >> On 7/30/2019 2:17 PM, Matan Azrad wrote:
> >>> Hi Ferruh
> >>>
> >>> From: Ferruh Yigit
> >>>> Sent: Tuesday, July 30, 2019 4:09 PM
> >>>> To: Matan Azrad <matan@mellanox.com>; Wenzhuo Lu
> >>>> <wenzhuo.lu@intel.com>; Jingjing Wu <jingjing.wu@intel.com>
> >>>> Cc: dev@dpdk.org; stable@dpdk.org
> >>>> Subject: Re: [dpdk-dev] [PATCH 1/2] app/testpmd: fix scatter
> >>>> offload configuration
> >>>>
> >>>> On 7/29/2019 1:36 PM, Matan Azrad wrote:
> >>>>> When the mbuf data size cannot contain the maximum Rx packet
> >>>>> length with the mbuf headroom, a packet should be scattered in
> >>>>> more than one
> >>>> mbuf.
> >>>>>
> >>>>> The application did not configure scatter offload in the above case.
> >>>>>
> >>>>> Enable the Rx scatter offload in the above case.
> >>>>>
> >>>>> Fixes: 33f9630fc23d ("app/testpmd: create mbuf based on max
> >>>>> supported
> >>>>> segments")
> >>>>> Cc: stable@dpdk.org
> >>>>>
> >>>>> Signed-off-by: Matan Azrad <matan@mellanox.com>
> >>>>
> >>>> Deferring the patchset to next release, they were late anyway and
> >>>> not actually fixing a defect, safer to defer than getting them in rc3.
> >>>
> >>> Yes this patch came late for RC3 but it is a fix.
> >>>
> >>> What are you concerns here?
> >>> Why don't you think defect found?
> >>
> >> First patch changes the behavior, when mbuf size is larger than
> >> configured size and user didn't provided the scatter offload, should
> >> test application automatically enable it?
> >
> > No, only when the mbuf size is smaller than the max_rx_pkt_len with
> headroom.
> > If scatter is not enabled in the above case, how can the PMD provide a
> packet with max_rx_pkt_len size?
> >

Answer here?

> > I think not enabling scatter in this case it is a user conflict in configuration
> and should raise an error in the PMD.  Maybe even in ethdev layer.
> >
> >> It may or not, but this is the change of the behavior, I think not a
> >> fix.
> >>
> >> And second patch adds more detail into the statistics, so I believe
> >> it is clear that it is not a fix.
> >
> >  Agree, this can wait.
> >
> >> The concern is getting changes very close to release, to balance
> >> between risk and benefit of the feature. I don't see any reason why
> >> these changes can't wait next release, so I don't see any reason to get the
> risk.
> >
> > When  I changed the default max_rx_pkt_len and mbuf size in LRO testing I
> met this issue.
> >
> > By default scatter will not be enabled.
> 
> I think it is still arguable if scatter should be enabled by default,

I meant that with this patch it will not be enabled by default due to the default values of mbuf size and max_rx_pkt_len.

> but isn't there a way in testpmd to enable scatter explicitly? If so you have a way to test LRO.

Yes there is a way.

This patch is just the right way to do it.

Re: [dpdk-dev] [dpdk-stable] [PATCH 1/2] app/testpmd: fix scatter offload configuration

[Ferruh Yigit]

On 7/30/2019 7:34 PM, Matan Azrad wrote:
> 
> 
> From: Ferruh Yigit 
>> On 7/30/2019 4:56 PM, Matan Azrad wrote:
>>> Hi Ferruh
>>>
>>>  From: Ferruh Yigit
>>>> Sent: Tuesday, July 30, 2019 6:22 PM
>>>> To: Matan Azrad <matan@mellanox.com>; Wenzhuo Lu
>>>> <wenzhuo.lu@intel.com>; Jingjing Wu <jingjing.wu@intel.com>
>>>> Cc: dev@dpdk.org; stable@dpdk.org
>>>> Subject: Re: [dpdk-dev] [PATCH 1/2] app/testpmd: fix scatter offload
>>>> configuration
>>>>
>>>> On 7/30/2019 2:17 PM, Matan Azrad wrote:
>>>>> Hi Ferruh
>>>>>
>>>>> From: Ferruh Yigit
>>>>>> Sent: Tuesday, July 30, 2019 4:09 PM
>>>>>> To: Matan Azrad <matan@mellanox.com>; Wenzhuo Lu
>>>>>> <wenzhuo.lu@intel.com>; Jingjing Wu <jingjing.wu@intel.com>
>>>>>> Cc: dev@dpdk.org; stable@dpdk.org
>>>>>> Subject: Re: [dpdk-dev] [PATCH 1/2] app/testpmd: fix scatter
>>>>>> offload configuration
>>>>>>
>>>>>> On 7/29/2019 1:36 PM, Matan Azrad wrote:
>>>>>>> When the mbuf data size cannot contain the maximum Rx packet
>>>>>>> length with the mbuf headroom, a packet should be scattered in
>>>>>>> more than one
>>>>>> mbuf.
>>>>>>>
>>>>>>> The application did not configure scatter offload in the above case.
>>>>>>>
>>>>>>> Enable the Rx scatter offload in the above case.
>>>>>>>
>>>>>>> Fixes: 33f9630fc23d ("app/testpmd: create mbuf based on max
>>>>>>> supported
>>>>>>> segments")
>>>>>>> Cc: stable@dpdk.org
>>>>>>>
>>>>>>> Signed-off-by: Matan Azrad <matan@mellanox.com>
>>>>>>
>>>>>> Deferring the patchset to next release, they were late anyway and
>>>>>> not actually fixing a defect, safer to defer than getting them in rc3.
>>>>>
>>>>> Yes this patch came late for RC3 but it is a fix.
>>>>>
>>>>> What are you concerns here?
>>>>> Why don't you think defect found?
>>>>
>>>> First patch changes the behavior, when mbuf size is larger than
>>>> configured size and user didn't provided the scatter offload, should
>>>> test application automatically enable it?
>>>
>>> No, only when the mbuf size is smaller than the max_rx_pkt_len with
>> headroom.
>>> If scatter is not enabled in the above case, how can the PMD provide a
>> packet with max_rx_pkt_len size?
>>>
> 
> Answer here?

Is it because drivers also "automatically" enable scattered Rx based on other
values? - which is also open to discussion I think.

> 
>>> I think not enabling scatter in this case it is a user conflict in configuration
>> and should raise an error in the PMD.  Maybe even in ethdev layer.
>>>
>>>> It may or not, but this is the change of the behavior, I think not a
>>>> fix.
>>>>
>>>> And second patch adds more detail into the statistics, so I believe
>>>> it is clear that it is not a fix.
>>>
>>>  Agree, this can wait.
>>>
>>>> The concern is getting changes very close to release, to balance
>>>> between risk and benefit of the feature. I don't see any reason why
>>>> these changes can't wait next release, so I don't see any reason to get the
>> risk.
>>>
>>> When  I changed the default max_rx_pkt_len and mbuf size in LRO testing I
>> met this issue.
>>>
>>> By default scatter will not be enabled.
>>
>> I think it is still arguable if scatter should be enabled by default,
> 
> I meant that with this patch it will not be enabled by default due to the default values of mbuf size and max_rx_pkt_len.

I mean the same thing indeed, still I believe arguable.

> 
>> but isn't there a way in testpmd to enable scatter explicitly? If so you have a way to test LRO.
> 
> Yes there is a way.
> 
> This patch is just the right way to do it.
> 

Good to know it is not blocking anyone, patch can be reviewed by its maintainers
and discussed more.

Re: [dpdk-dev] [dpdk-stable] [PATCH 1/2] app/testpmd: fix scatter offload configuration

[Matan Azrad]

Hi Ferruh

From: Ferruh Yigit
> On 7/30/2019 7:34 PM, Matan Azrad wrote:
> >
> >
> > From: Ferruh Yigit
> >> On 7/30/2019 4:56 PM, Matan Azrad wrote:
> >>> Hi Ferruh
> >>>
> >>>  From: Ferruh Yigit
> >>>> Sent: Tuesday, July 30, 2019 6:22 PM
> >>>> To: Matan Azrad <matan@mellanox.com>; Wenzhuo Lu
> >>>> <wenzhuo.lu@intel.com>; Jingjing Wu <jingjing.wu@intel.com>
> >>>> Cc: dev@dpdk.org; stable@dpdk.org
> >>>> Subject: Re: [dpdk-dev] [PATCH 1/2] app/testpmd: fix scatter
> >>>> offload configuration
> >>>>
> >>>> On 7/30/2019 2:17 PM, Matan Azrad wrote:
> >>>>> Hi Ferruh
> >>>>>
> >>>>> From: Ferruh Yigit
> >>>>>> Sent: Tuesday, July 30, 2019 4:09 PM
> >>>>>> To: Matan Azrad <matan@mellanox.com>; Wenzhuo Lu
> >>>>>> <wenzhuo.lu@intel.com>; Jingjing Wu <jingjing.wu@intel.com>
> >>>>>> Cc: dev@dpdk.org; stable@dpdk.org
> >>>>>> Subject: Re: [dpdk-dev] [PATCH 1/2] app/testpmd: fix scatter
> >>>>>> offload configuration
> >>>>>>
> >>>>>> On 7/29/2019 1:36 PM, Matan Azrad wrote:
> >>>>>>> When the mbuf data size cannot contain the maximum Rx packet
> >>>>>>> length with the mbuf headroom, a packet should be scattered in
> >>>>>>> more than one
> >>>>>> mbuf.
> >>>>>>>
> >>>>>>> The application did not configure scatter offload in the above case.
> >>>>>>>
> >>>>>>> Enable the Rx scatter offload in the above case.
> >>>>>>>
> >>>>>>> Fixes: 33f9630fc23d ("app/testpmd: create mbuf based on max
> >>>>>>> supported
> >>>>>>> segments")
> >>>>>>> Cc: stable@dpdk.org
> >>>>>>>
> >>>>>>> Signed-off-by: Matan Azrad <matan@mellanox.com>
> >>>>>>
> >>>>>> Deferring the patchset to next release, they were late anyway and
> >>>>>> not actually fixing a defect, safer to defer than getting them in rc3.
> >>>>>
> >>>>> Yes this patch came late for RC3 but it is a fix.
> >>>>>
> >>>>> What are you concerns here?
> >>>>> Why don't you think defect found?
> >>>>
> >>>> First patch changes the behavior, when mbuf size is larger than
> >>>> configured size and user didn't provided the scatter offload,
> >>>> should test application automatically enable it?
> >>>
> >>> No, only when the mbuf size is smaller than the max_rx_pkt_len with
> >> headroom.
> >>> If scatter is not enabled in the above case, how can the PMD provide
> >>> a
> >> packet with max_rx_pkt_len size?
> >>>
> >
> > Answer here?
> 
> Is it because drivers also "automatically" enable scattered Rx based on other
> values?

Scatter is a defined RX offload.
Like other offloads I think it always should be explicitly set by the user if he wants it, and vice versa.
If the user doesn't configure it, the PMD should not scatter packets because the user doesn't expect multi-mbuf packets in datapath and maybe even doesn't handle it.

So, I think the above case is a user conflict in Rx configuration and like other conflicts it should cause an error.
In MLX5 PMD, an error will be returned from Rx setup.

Re: [dpdk-dev] [dpdk-stable] [PATCH 1/2] app/testpmd: fix scatter offload configuration

[Yigit, Ferruh]

On 7/31/2019 7:11 AM, Matan Azrad wrote:
> Hi Ferruh
> 
> From: Ferruh Yigit
>> On 7/30/2019 7:34 PM, Matan Azrad wrote:
>>>
>>>
>>> From: Ferruh Yigit
>>>> On 7/30/2019 4:56 PM, Matan Azrad wrote:
>>>>> Hi Ferruh
>>>>>
>>>>>  From: Ferruh Yigit
>>>>>> Sent: Tuesday, July 30, 2019 6:22 PM
>>>>>> To: Matan Azrad <matan@mellanox.com>; Wenzhuo Lu
>>>>>> <wenzhuo.lu@intel.com>; Jingjing Wu <jingjing.wu@intel.com>
>>>>>> Cc: dev@dpdk.org; stable@dpdk.org
>>>>>> Subject: Re: [dpdk-dev] [PATCH 1/2] app/testpmd: fix scatter
>>>>>> offload configuration
>>>>>>
>>>>>> On 7/30/2019 2:17 PM, Matan Azrad wrote:
>>>>>>> Hi Ferruh
>>>>>>>
>>>>>>> From: Ferruh Yigit
>>>>>>>> Sent: Tuesday, July 30, 2019 4:09 PM
>>>>>>>> To: Matan Azrad <matan@mellanox.com>; Wenzhuo Lu
>>>>>>>> <wenzhuo.lu@intel.com>; Jingjing Wu <jingjing.wu@intel.com>
>>>>>>>> Cc: dev@dpdk.org; stable@dpdk.org
>>>>>>>> Subject: Re: [dpdk-dev] [PATCH 1/2] app/testpmd: fix scatter
>>>>>>>> offload configuration
>>>>>>>>
>>>>>>>> On 7/29/2019 1:36 PM, Matan Azrad wrote:
>>>>>>>>> When the mbuf data size cannot contain the maximum Rx packet
>>>>>>>>> length with the mbuf headroom, a packet should be scattered in
>>>>>>>>> more than one
>>>>>>>> mbuf.
>>>>>>>>>
>>>>>>>>> The application did not configure scatter offload in the above case.
>>>>>>>>>
>>>>>>>>> Enable the Rx scatter offload in the above case.
>>>>>>>>>
>>>>>>>>> Fixes: 33f9630fc23d ("app/testpmd: create mbuf based on max
>>>>>>>>> supported
>>>>>>>>> segments")
>>>>>>>>> Cc: stable@dpdk.org
>>>>>>>>>
>>>>>>>>> Signed-off-by: Matan Azrad <matan@mellanox.com>
>>>>>>>>
>>>>>>>> Deferring the patchset to next release, they were late anyway and
>>>>>>>> not actually fixing a defect, safer to defer than getting them in rc3.
>>>>>>>
>>>>>>> Yes this patch came late for RC3 but it is a fix.
>>>>>>>
>>>>>>> What are you concerns here?
>>>>>>> Why don't you think defect found?
>>>>>>
>>>>>> First patch changes the behavior, when mbuf size is larger than
>>>>>> configured size and user didn't provided the scatter offload,
>>>>>> should test application automatically enable it?
>>>>>
>>>>> No, only when the mbuf size is smaller than the max_rx_pkt_len with
>>>> headroom.
>>>>> If scatter is not enabled in the above case, how can the PMD provide
>>>>> a
>>>> packet with max_rx_pkt_len size?
>>>>>
>>>
>>> Answer here?
>>
>> Is it because drivers also "automatically" enable scattered Rx based on other
>> values?
> 
> Scatter is a defined RX offload.
> Like other offloads I think it always should be explicitly set by the user if he wants it, and vice versa.
> If the user doesn't configure it, the PMD should not scatter packets because the user doesn't expect multi-mbuf packets in datapath and maybe even doesn't handle it.

+1

So what about having the log message but not implicitly update the offload config?

> 
> So, I think the above case is a user conflict in Rx configuration and like other conflicts it should cause an error.
> In MLX5 PMD, an error will be returned from Rx setup.
> 
> 

Re: [dpdk-dev] [dpdk-stable] [PATCH 1/2] app/testpmd: fix scatter offload configuration

[Matan Azrad]

Hi Ferruh

From: Yigit, Ferruh <ferruh.yigit@linux.intel.com>
> On 7/31/2019 7:11 AM, Matan Azrad wrote:
> > Hi Ferruh
> >
> > From: Ferruh Yigit
> >> On 7/30/2019 7:34 PM, Matan Azrad wrote:
> >>>
> >>>
> >>> From: Ferruh Yigit
> >>>> On 7/30/2019 4:56 PM, Matan Azrad wrote:
> >>>>> Hi Ferruh
> >>>>>
> >>>>>  From: Ferruh Yigit
> >>>>>> Sent: Tuesday, July 30, 2019 6:22 PM
> >>>>>> To: Matan Azrad <matan@mellanox.com>; Wenzhuo Lu
> >>>>>> <wenzhuo.lu@intel.com>; Jingjing Wu <jingjing.wu@intel.com>
> >>>>>> Cc: dev@dpdk.org; stable@dpdk.org
> >>>>>> Subject: Re: [dpdk-dev] [PATCH 1/2] app/testpmd: fix scatter
> >>>>>> offload configuration
> >>>>>>
> >>>>>> On 7/30/2019 2:17 PM, Matan Azrad wrote:
> >>>>>>> Hi Ferruh
> >>>>>>>
> >>>>>>> From: Ferruh Yigit
> >>>>>>>> Sent: Tuesday, July 30, 2019 4:09 PM
> >>>>>>>> To: Matan Azrad <matan@mellanox.com>; Wenzhuo Lu
> >>>>>>>> <wenzhuo.lu@intel.com>; Jingjing Wu <jingjing.wu@intel.com>
> >>>>>>>> Cc: dev@dpdk.org; stable@dpdk.org
> >>>>>>>> Subject: Re: [dpdk-dev] [PATCH 1/2] app/testpmd: fix scatter
> >>>>>>>> offload configuration
> >>>>>>>>
> >>>>>>>> On 7/29/2019 1:36 PM, Matan Azrad wrote:
> >>>>>>>>> When the mbuf data size cannot contain the maximum Rx
> packet
> >>>>>>>>> length with the mbuf headroom, a packet should be scattered in
> >>>>>>>>> more than one
> >>>>>>>> mbuf.
> >>>>>>>>>
> >>>>>>>>> The application did not configure scatter offload in the above
> case.
> >>>>>>>>>
> >>>>>>>>> Enable the Rx scatter offload in the above case.
> >>>>>>>>>
> >>>>>>>>> Fixes: 33f9630fc23d ("app/testpmd: create mbuf based on max
> >>>>>>>>> supported
> >>>>>>>>> segments")
> >>>>>>>>> Cc: stable@dpdk.org
> >>>>>>>>>
> >>>>>>>>> Signed-off-by: Matan Azrad <matan@mellanox.com>
> >>>>>>>>
> >>>>>>>> Deferring the patchset to next release, they were late anyway
> >>>>>>>> and not actually fixing a defect, safer to defer than getting them in
> rc3.
> >>>>>>>
> >>>>>>> Yes this patch came late for RC3 but it is a fix.
> >>>>>>>
> >>>>>>> What are you concerns here?
> >>>>>>> Why don't you think defect found?
> >>>>>>
> >>>>>> First patch changes the behavior, when mbuf size is larger than
> >>>>>> configured size and user didn't provided the scatter offload,
> >>>>>> should test application automatically enable it?
> >>>>>
> >>>>> No, only when the mbuf size is smaller than the max_rx_pkt_len
> >>>>> with
> >>>> headroom.
> >>>>> If scatter is not enabled in the above case, how can the PMD
> >>>>> provide a
> >>>> packet with max_rx_pkt_len size?
> >>>>>
> >>>
> >>> Answer here?
> >>
> >> Is it because drivers also "automatically" enable scattered Rx based
> >> on other values?
> >
> > Scatter is a defined RX offload.
> > Like other offloads I think it always should be explicitly set by the user if he
> wants it, and vice versa.
> > If the user doesn't configure it, the PMD should not scatter packets
> because the user doesn't expect multi-mbuf packets in datapath and maybe
> even doesn't handle it.
> 
> +1
> 
> So what about having the log message but not implicitly update the offload
> config?
> 

Yes, we need this massage at least.

Will work on it.

Thanks.