[dpdk-dev] [PATCH v8 0/3]: Add LRO support to ixgbe PMD

[Zhang, Helin]

Hi Vlad

I have a concern about the code changes you added in ixgbe_type.h.
For ixgbe, all source files in librte_pmd_ixgbe/ixgbe, except ixgbe_osdep.h were called as "base driver", which was not developed by DPDK developers, and released by the other team. We never modify any code in those base driver source files, and just copy those file into DPDK project.
We did everything in DPDK developed source files. As we depends on that team to update future base driver for us, and we don't want to maintain any code changes in "base driver". So we need to think to move your code changes in ixgbe_type.h to other DPDK developed source files.

Though your code changes is good enough, but we may need to comply with what we did before. This will save our maintaining efforts in the future. Also we have similar rules for e1000, i40e, fm10k. Thank you very much!

Regards,
Helin

> -----Original Message-----
> From: dev [mailto:dev-bounces at dpdk.org] On Behalf Of Vlad Zolotarov
> Sent: Thursday, March 19, 2015 1:52 AM
> To: dev at dpdk.org
> Subject: [dpdk-dev] [PATCH v8 0/3]: Add LRO support to ixgbe PMD
> 
> This series adds the missing flow for enabling the LRO in the ethdev and adds a
> support for this feature in the ixgbe PMD. There is a big hope that this initiative
> is going to be picked up by some Intel developer that would add the LRO
> support to other Intel PMDs.
> 
> The series starts with some cleanup work in the code the final patch (the actual
> adding of the LRO support) is going to touch/use/change. There are still quite a
> few issues in the ixgbe PMD code left but they have to be a matter of a
> different series and I've left a few "TODO"
> remarks in the code.
> 
> The LRO ("RSC" in Intel's context) PMD completion handling code follows the
> same design as the corresponding Linux and FreeBSD implementation: pass the
> aggregation's cluster HEAD buffer to the NEXTP entry of the software ring till
> EOP is met.
> 
> HW configuration follows the corresponding specs: this feature is supported
> only by x540 and
> 82599 PF devices.
> 
> The feature has been tested with seastar TCP stack with the following
> configuration on Tx side:
>    - MTU: 400B
>    - 100 concurrent TCP connections.
> 
> The results were:
>    - Without LRO: total throughput: 0.12Gbps, coefficient of variance: 1.41%
>    - With LRO:    total throughput: 8.21Gbps, coefficient of variance: 0.59%
> 
> This is an almost factor 80 improvement.
> 
> New in v8:
>    - Fixed the structs naming: igb_xxx -> ixgbe_xxx (some leftovers in PATCH2).
>    - Took the RSC configuration code from ixgbe_dev_rx_init() into a separate
>      function - ixgbe_set_rsc().
>    - Added some missing macros for HW configuration.
>    - Styling adjustments:
>       - Functions names.
>       - Functions descriptions.
>    - Reworked the ixgbe_free_rsc_cluster() code to make it more readable.
>    - Kill the HEADER_SPLIT flow in ixgbe_set_rsc() since it's not supported by
>      ixgbe PMD.
> 
> New in v7:
>    - Free not-yet-completed RSC aggregations in rte_eth_dev_stop() flow.
>    - Fixed rx_bulk_alloc_allowed and rx_vec_allowed initialization:
>       - Don't set them to FALSE in rte_eth_dev_stop() flow - the following
>         rte_eth_dev_start() will need them.
>       - Reset them to TRUE in rte_eth_dev_configure() and not in a probe()
> flow.
>         This will ensure the proper behaviour if port is re-configured.
>    - Reset the sw_ring[].mbuf entry in a bulk allocation case.
>      This is needed for ixgbe_rx_queue_release_mbufs().
>    - _recv_pkts_lro(): added the missing memory barrier before RDT update in
> a
>      non-bulk allocation case.
>    - Don't allow RSC when device is configured in an SR-IOV mode.
> 
> New in v6:
>    - Fix of the typo in the "bug fixes" series that broke the compilation caused
> a
>      minor change in this follow-up series.
> 
> New in v5:
>    - Split the series into "bug fixes" and "all the rest" so that the former could
> be
>      integrated into a 2.0 release.
>    - Put the RTE_ETHDEV_HAS_LRO_SUPPORT definition at the beginning of
> rte_ethdev.h.
>    - Removed the "TODO: Remove me" comment near
> RTE_ETHDEV_HAS_LRO_SUPPORT.
> 
> New in v4:
>    - Remove CONFIG_RTE_ETHDEV_LRO_SUPPORT from
> config/common_linuxapp.
>    - Define RTE_ETHDEV_HAS_LRO_SUPPORT in rte_ethdev.h.
>    - As a result of "ixgbe: check rxd number to avoid mbuf leak" (352078e8e)
> Vector Rx
>      had to get the same treatment as Rx Bulk Alloc (see PATCH4 for more
> details).
> 
> New in v3:
>    - ixgbe_rx_alloc_bufs(): Always reset refcnt of the buffers to 1. Otherwise
> rte_pktmbuf_free()
>      won't free them.
> 
> New in v2:
>    - Removed rte_eth_dev_data.lro_bulk_alloc and added
> ixgbe_hw.rx_bulk_alloc_allowed
>      instead.
>    - Unified the rx_pkt_bulk callback setting (a separate new patch).
>    - Fixed a few styling and spelling issues.
> 
> 
> Vlad Zolotarov (3):
>   ixgbe: Cleanups
>   ixgbe: Code refactoring
>   ixgbe: Add LRO support
> 
>  lib/librte_ether/rte_ethdev.h           |   9 +-
>  lib/librte_net/rte_ip.h                 |   3 +
>  lib/librte_pmd_ixgbe/ixgbe/ixgbe_type.h |   6 +
>  lib/librte_pmd_ixgbe/ixgbe_ethdev.c     |  11 +
>  lib/librte_pmd_ixgbe/ixgbe_ethdev.h     |   5 +
>  lib/librte_pmd_ixgbe/ixgbe_rxtx.c       | 766
> +++++++++++++++++++++++++++++---
>  lib/librte_pmd_ixgbe/ixgbe_rxtx.h       |   6 +
>  7 files changed, 737 insertions(+), 69 deletions(-)
> 
> --
> 2.1.0