Re: [dpdk-dev] [PATCH v1] net/axgbe: add support for Scattered Rx

[Ferruh Yigit <ferruh.yigit@intel.com>]

On 2/28/2020 9:00 AM, Sebastian, Selwin wrote:
> 
> Thanks and Regards
> Selwin Sebastian
>  
>  
> 
> -----Original Message-----
> From: Ferruh Yigit <ferruh.yigit@intel.com> 
> Sent: Thursday, February 27, 2020 8:08 PM
> To: Sebastian, Selwin <Selwin.Sebastian@amd.com>; dev@dpdk.org
> Subject: Re: [dpdk-dev] [PATCH v1] net/axgbe: add support for Scattered Rx
> 
> [CAUTION: External Email]
> 
> On 2/27/2020 2:35 PM, Sebastian, Selwin wrote:
>> [AMD Official Use Only - Internal Distribution Only]
>>
>> Hi Ferruh,
>>       For validation of scatter using the testpmd method  mentioned in dpdk docs, we need to have these Tx offloads also enabled.
> 
> [Please don't top post, it makes conversation hard to follow.]
> 
> Can you point where there testpmd requirement in the code?
> 	https://doc.dpdk.org/dts/test_plans/scatter_test_plan.html talks about DEV_TX_OFFLOAD_MULTI_SEGS as a prerequisites for scatter . 

Above document has a good point that:
"The forwarding of scattered input packets naturally enforces the transmission
of scattered packets by PMD transmit functions."

If you only support scattered Rx, but not Tx (as your case), you may have
unexpected results.

But this doesn't mean the multi segment Tx should be announced when it is not
supported.


> When I add DEV_TX_OFFLOAD_MULTI_SEGS for tx capability ,   I was forced to add " DEV_TX_OFFLOAD_OUTER_IPV4_CKSUM , DEV_TX_OFFLOAD_UDP_TSO , DEV_TX_OFFLOAD_SCTP_CKSUM" for the mentioned testpmd to run. 

It is still no clear why you are forced to add those capabilities, if you can
point me the code in testpmd that forces you, I can comment better.

> 
> Also these offloads should be enabled when HW/Driver supports it, not for it is required by testpmd.
> 
> 	I am able to test scatter without adding "DEV_TX_OFFLOAD_MULTI_SEGS" tx capability. Shall I submit version2 of the patch after removing " DEV_TX_OFFLOAD_MULTI_SEGS, DEV_TX_OFFLOAD_OUTER_IPV4_CKSUM , DEV_TX_OFFLOAD_UDP_TSO, DEV_TX_OFFLOAD_SCTP_CKSUM" capabilities. ?

First of all, offload capabilities are set based on what offload HW/Driver
support. As far as I can see axgbe doesn't support multi segment Tx, so you
shouldn't set 'DEV_TX_OFFLOAD_MULTI_SEGS' at all, same for all offload flags.

If those list of offloads are supported by HW/Driver, but previously forgotten
to announce them as capability, please send a separate patch for it, with proper
fixes line which points the commit adding the support.

> 
>>
>> Thanks and Regards
>> Selwin Sebastian
>>
>> -----Original Message-----
>> From: Ferruh Yigit <ferruh.yigit@intel.com>
>> Sent: Thursday, February 27, 2020 7:59 PM
>> To: Sebastian, Selwin <Selwin.Sebastian@amd.com>; dev@dpdk.org
>> Subject: Re: [dpdk-dev] [PATCH v1] net/axgbe: add support for 
>> Scattered Rx
>>
>> [CAUTION: External Email]
>>
>> On 2/27/2020 6:33 AM, ssebasti@amd.com wrote:
>>> From: Selwin Sebastian <selwin.sebastian@amd.com>
>>>
>>> Enable scattered rx support and add jumbo packet transmit capability
>>>
>>> Signed-off-by: Selwin Sebastian <selwin.sebastian@amd.com>
>>
>> <...>
>>
>>> @@ -789,11 +789,17 @@ axgbe_dev_info_get(struct rte_eth_dev *dev, struct rte_eth_dev_info *dev_info)
>>>               DEV_RX_OFFLOAD_IPV4_CKSUM |
>>>               DEV_RX_OFFLOAD_UDP_CKSUM  |
>>>               DEV_RX_OFFLOAD_TCP_CKSUM  |
>>> +             DEV_RX_OFFLOAD_JUMBO_FRAME      |
>>> +             DEV_RX_OFFLOAD_SCATTER    |
>>>               DEV_RX_OFFLOAD_KEEP_CRC;
>>>
>>>       dev_info->tx_offload_capa =
>>>               DEV_TX_OFFLOAD_IPV4_CKSUM  |
>>>               DEV_TX_OFFLOAD_UDP_CKSUM   |
>>> +             DEV_TX_OFFLOAD_OUTER_IPV4_CKSUM |
>>> +             DEV_TX_OFFLOAD_UDP_TSO          |
>>> +             DEV_TX_OFFLOAD_SCTP_CKSUM       |
>>> +             DEV_TX_OFFLOAD_MULTI_SEGS       |
>>>               DEV_TX_OFFLOAD_TCP_CKSUM;
>>
>> Is the Tx offload capability update related to the this change? If it is not can you please send these updates as a separate patch, and a send new version of this patch without this bit?
>>
>> Thanks,
>> ferruh
>>
>