Re: [PATCH 0/2] lib: add TCP IPv4 GRO support

From: "Wiles, Keith" <keith.wiles@intel.com>
To: "Morten Brørup" <mb@smartsharesystems.com>
Cc: "Ananyev, Konstantin" <konstantin.ananyev@intel.com>,
	"Hu, Jiayu" <jiayu.hu@intel.com>,
	Yuanhan Liu <yuanhan.liu@linux.intel.com>,
	"Richardson, Bruce" <bruce.richardson@intel.com>,
	Stephen Hemminger <stephen@networkplumber.org>,
	"Yigit, Ferruh" <ferruh.yigit@intel.com>,
	"Liang, Cunming" <cunming.liang@intel.com>,
	Thomas Monjalon <thomas.monjalon@6wind.com>,
	"dev@dpdk.org" <dev@dpdk.org>
Subject: Re: [PATCH 0/2] lib: add TCP IPv4 GRO support
Date: Wed, 29 Mar 2017 12:12:33 +0000	[thread overview]
Message-ID: <3C57097F-7B9D-4354-B103-E92781C52794@intel.com> (raw)
In-Reply-To: <98CBD80474FA8B44BF855DF32C47DC359EAE68@smartserver.smartshare.dk>

> On Mar 29, 2017, at 5:47 AM, Morten Brørup <mb@smartsharesystems.com> wrote:
> 
> I have two points to this discussion:
> 
> GRO/LRO must be disabled by default! Truly transparent network appliances, such as classic Layer 3 routers and Layer 2 switches (and our SmartShare StraightShaper appliance), may not want to merge multiple smaller packets into one larger packet, regardless of any processing performance benefit. Also refer to Wikipedia (https://en.wikipedia.org/wiki/Large_receive_offload), especially reference 9 (https://bugzilla.redhat.com/show_bug.cgi?id=772317). And if any of you developers don't intuitively agree, just ask any old network engineer about all the mess he had to deal with "back in the days" when packet sizes were important... flaky path MTU discovery, MSS clamping for PPPoE and VPN tunnels, packet reassembly causing performance degradation and packet loss on underpowered VPN routers, etc.

I agree we should not have any offload enabled by default and I assumed that was not the case here unless I missed something. My point was to make GRO more transparent when enabled instead of the application having to use a different API for GRO when we can put this feature inline with current rx_burst APIs. Using the RX callbacks or some other method as long as the performance of the current rx_burst APIs is not effected when the GRO is not enabled.

> 
> We should consider librte_net a library for miscellaneous utilities, i.e. mainly stateless functions. Functions for packet merging (of multiple IP packets or actual IP fragments), which clearly requires a lot of memory and statefulness, does not belong here. This is worth noting, not only for the GRO/GSO library, but for similar future discussions. (The size of the compiled library is irrelevant - only its purpose matters.)

I already decided to except the new library, but the name is not great as other features could be added to the new library like LRO. If we name it GRO then it can only be GRO.

> 
> 
> Med venlig hilsen / kind regards
> 
> Morten Brørup
> CTO
> 
> 
> SmartShare Systems A/S
> Tonsbakken 16-18
> DK-2740 Skovlunde
> Denmark
> 
> Office      +45 70 20 00 93
> Direct      +45 89 93 50 22
> Mobile     +45 25 40 82 12
> 
> mb@smartsharesystems.com
> www.smartsharesystems.com

Regards,
Keith