Re: [dpdk-dev] [PATCH v4 5/5] ixgbe: Add LRO support

[Vlad Zolotarov <vladz@cloudius-systems.com>]

On 03/08/15 23:20, Thomas Monjalon wrote:
> 2015-03-08 16:04, Vlad Zolotarov:
>> --- a/lib/librte_ether/rte_ethdev.h
>> +++ b/lib/librte_ether/rte_ethdev.h
>> @@ -312,6 +312,9 @@ enum rte_eth_tx_mq_mode {
>>   #define ETH_VMDQ_DCB_TX             ETH_MQ_TX_VMDQ_DCB
>>   #define ETH_DCB_TX                  ETH_MQ_TX_DCB
>>   
>> +/* TODO: Remove this when DPDK version bumps up to 2.0.1 */
> The last digit is reserved for maintenance.
> LRO should be merged in 2.1.0.
> By the way, why removing this macros when it's integrated? I think it would
> be a good idea to start using this kind of macro for new API.
> It's better than version checking because it supports backports.
> Opinion?

I agree. I was just under the impression that u hate macros in general 
(like I do). ;)  That's why I put this reminder... ;)
But  I agree that in situations like this having a macro defining a 
specific API presence is a good practice.

>
>> +#define RTE_ETHDEV_HAS_LRO_SUPPORT
> Is it the right location to define it?
> Is it better than defining it just above one of the new fields or at the
> beginning of the file?

I vote for a beginning of the file. If we have more like these having 
them spread across the file would make it hard to track Especially when 
one macro is responsible for more than one API line.

>
>> +
>>   /**
>>    * A structure used to configure the RX features of an Ethernet port.
>>    */
>> @@ -320,14 +323,15 @@ struct rte_eth_rxmode {
>>   	enum rte_eth_rx_mq_mode mq_mode;
>>   	uint32_t max_rx_pkt_len;  /**< Only used if jumbo_frame enabled. */
>>   	uint16_t split_hdr_size;  /**< hdr buf size (header_split enabled).*/
>> -	uint8_t header_split : 1, /**< Header Split enable. */
>> +	uint16_t header_split : 1, /**< Header Split enable. */
>>   		hw_ip_checksum   : 1, /**< IP/UDP/TCP checksum offload enable. */
>>   		hw_vlan_filter   : 1, /**< VLAN filter enable. */
>>   		hw_vlan_strip    : 1, /**< VLAN strip enable. */
>>   		hw_vlan_extend   : 1, /**< Extended VLAN enable. */
>>   		jumbo_frame      : 1, /**< Jumbo Frame Receipt enable. */
>>   		hw_strip_crc     : 1, /**< Enable CRC stripping by hardware. */
>> -		enable_scatter   : 1; /**< Enable scatter packets rx handler */
>> +		enable_scatter   : 1, /**< Enable scatter packets rx handler */
>> +		enable_lro       : 1; /**< Enable LRO */
>>   };
>>   
>>   /**
>> @@ -1515,6 +1519,7 @@ struct rte_eth_dev_data {
>>   	uint8_t port_id;           /**< Device [external] port identifier. */
>>   	uint8_t promiscuous   : 1, /**< RX promiscuous mode ON(1) / OFF(0). */
>>   		scattered_rx : 1,  /**< RX of scattered packets is ON(1) / OFF(0) */
>> +		lro          : 1,  /**< RX LRO is ON(1) / OFF(0) */
>>   		all_multicast : 1, /**< RX all multicast mode ON(1) / OFF(0). */
>>   		dev_started : 1;   /**< Device state: STARTED(1) / STOPPED(0). */
>>   };