Re: [dpdk-dev] [PATCH v2 3/5] ethdev: convert remaining apps to new offload API

[Andrew Rybchenko <arybchenko@solarflare.com>]

On 07/04/2018 03:26 PM, Thomas Monjalon wrote:
> 04/07/2018 13:16, Andrew Rybchenko:
>> On 07/03/2018 12:27 AM, Thomas Monjalon wrote:
>>> --- a/doc/guides/sample_app_ug/link_status_intr.rst
>>> +++ b/doc/guides/sample_app_ug/link_status_intr.rst
>>> @@ -137,10 +137,7 @@ The global configuration is stored in a static structure:
>>>        static const struct rte_eth_conf port_conf = {
>>>            .rxmode = {
>>>                .split_hdr_size = 0,
>>> -            .header_split = 0,   /**< Header Split disabled */
>>> -            .hw_ip_checksum = 0, /**< IP checksum offload disabled */
>>> -            .hw_vlan_filter = 0, /**< VLAN filtering disabled */
>>> -            .hw_strip_crc= 0,    /**< CRC stripped by hardware */
>>> +            .offloads = DEV_RX_OFFLOAD_CRC_STRIP,
>> Is it intended that CRC strip was disabled before and now it is becoming
>> enabled?
> Yes, I consider the comment to be the real intent.

OK. I see. Most likely yes. I agree.

>>> --- a/examples/bbdev_app/main.c
>>> +++ b/examples/bbdev_app/main.c
>>> @@ -64,11 +64,7 @@ static const struct rte_eth_conf port_conf = {
>>>    		.mq_mode = ETH_MQ_RX_NONE,
>>>    		.max_rx_pkt_len = ETHER_MAX_LEN,
>>>    		.split_hdr_size = 0,
>>> -		.header_split = 0, /**< Header Split disabled */
>>> -		.hw_ip_checksum = 0, /**< IP checksum offload disabled */
>>> -		.hw_vlan_filter = 0, /**< VLAN filtering disabled */
>>> -		.jumbo_frame = 0, /**< Jumbo Frame Support disabled */
>>> -		.hw_strip_crc = 0, /**< CRC stripped by hardware */
>>> +		.offloads = DEV_RX_OFFLOAD_CRC_STRIP,
>> Is it intended that CRC strip was disabled before and now it is becoming
>> enabled?
> Yes, I consider the comment to be the real intent.
>
>>> --- a/test/test/test_pmd_perf.c
>>> +++ b/test/test/test_pmd_perf.c
>>> @@ -97,11 +90,6 @@ static struct rte_eth_txconf tx_conf = {
>>>    	},
>>>    	.tx_free_thresh = 32, /* Use PMD default values */
>>>    	.tx_rs_thresh = 32, /* Use PMD default values */
>>> -	.txq_flags = (ETH_TXQ_FLAGS_NOMULTSEGS |
>>> -		      ETH_TXQ_FLAGS_NOVLANOFFL |
>>> -		      ETH_TXQ_FLAGS_NOXSUMSCTP |
>>> -		      ETH_TXQ_FLAGS_NOXSUMUDP |
>>> -		      ETH_TXQ_FLAGS_NOXSUMTCP)
>>>    };
>>>    
>>>    enum {
>>> @@ -808,38 +796,29 @@ test_set_rxtx_conf(cmdline_fixed_string_t mode)
>>>    
>>>    	if (!strcmp(mode, "vector")) {
>>>    		/* vector rx, tx */
>>> -		tx_conf.txq_flags = 0xf01;
>> I'd say that 100% correct equivalent would be:
>> tx_conf.offloads &= ~(DEV_TX_OFFLOAD_VLAN_INSERT |
>>             DEV_TX_OFFLOAD_IPV4_CKSUM | DEV_TX_OFFLOAD_UDP_CKSUM |
>>             DEV_TX_OFFLOAD_TCP_CKSUM | DEV_TX_OFFLOAD_SCTP_CKSUM |
>>             DEV_TX_OFFLOAD_MULTI_SEGS);
> I'd say it is a really crappy code, and probably tuned for Intel devices only.
>
>> I guess the function may be called few times with different mode set.
>> If so, similar fixes should be applied below as well.
>>
>>>    		tx_conf.tx_rs_thresh = 32;
>>>    		tx_conf.tx_free_thresh = 32;
>>> -		port_conf.rxmode.hw_ip_checksum = 0;
>>> -		port_conf.rxmode.enable_scatter = 0;
>> port_conf.rxmode.offloads &= ~(DEV_RX_OFFLOAD_CHECKSUM |
>> DEV_RX_OFFLOAD_SCATTER);
>>
>>>    		return 0;
>>>    	} else if (!strcmp(mode, "scalar")) {
>>>    		/* bulk alloc rx, full-featured tx */
>>> -		tx_conf.txq_flags = 0;
>> I think here we should enable offloads listed above to have
>> full-featured Tx:
>> tx_conf.offloads |=  ...
>>
>>>    		tx_conf.tx_rs_thresh = 32;
>>>    		tx_conf.tx_free_thresh = 32;
>>> -		port_conf.rxmode.hw_ip_checksum = 1;
>>> -		port_conf.rxmode.enable_scatter = 0;
>>> +		port_conf.rxmode.offloads |= DEV_RX_OFFLOAD_CHECKSUM;
>> port_conf.rxmode.offloads &= ~DEV_RX_OFFLOAD_SCATTER;
>>
>>>    		return 0;
>>>    	} else if (!strcmp(mode, "hybrid")) {
>>>    		/* bulk alloc rx, vector tx
>>>    		 * when vec macro not define,
>>>    		 * using the same rx/tx as scalar
>>>    		 */
>>> -		tx_conf.txq_flags = 0xf01;
>> As in similar case above.
>>
>>>    		tx_conf.tx_rs_thresh = 32;
>>>    		tx_conf.tx_free_thresh = 32;
>>> -		port_conf.rxmode.hw_ip_checksum = 1;
>>> -		port_conf.rxmode.enable_scatter = 0;
>>> +		port_conf.rxmode.offloads |= DEV_RX_OFFLOAD_CHECKSUM;
>> As in similar case above
>>
>>>    		return 0;
>>>    	} else if (!strcmp(mode, "full")) {
>>>    		/* full feature rx,tx pair */
>>> -		tx_conf.txq_flags = 0x0;   /* must condition */
>> As in similar case above.
>>
>>>    		tx_conf.tx_rs_thresh = 32;
>>>    		tx_conf.tx_free_thresh = 32;
>>> -		port_conf.rxmode.hw_ip_checksum = 0;
>>> -		port_conf.rxmode.enable_scatter = 1; /* must condition */
>>> +		port_conf.rxmode.offloads |= DEV_RX_OFFLOAD_SCATTER;
>> port_conf.rxmode.offloads &= ~DEV_RX_OFFLOAD_CHECKSUM;
>>
>>>    		return 0;
>>>    	}
>>>    
>> In general I think that it would be really good to avoid changes in
>> behaviour when technical changes are done.
> I agree, but in this case, it is impossible to know what was the real intent.
> And I am perfectly fine breaking bad code.
> The other option is to just remove the file. Maybe the best option?

I have no strong opinion. As far as I can see there is no maintainer for 
it...