Message-ID: <09254e7cd6fd20f899f8a4ad3fbaabf223802503.camel@gmail.com>
Date: Tue, 30 May 2023 08:29:10 -0700
From: Alexander H Duyck <alexander.duyck@...il.com>
To: Alexander Lobakin <aleksander.lobakin@...el.com>, "David S. Miller"
	 <davem@...emloft.net>, Eric Dumazet <edumazet@...gle.com>, Jakub Kicinski
	 <kuba@...nel.org>, Paolo Abeni <pabeni@...hat.com>
Cc: Maciej Fijalkowski <maciej.fijalkowski@...el.com>, Magnus Karlsson
 <magnus.karlsson@...el.com>, Michal Kubiak <michal.kubiak@...el.com>,
 Larysa Zaremba <larysa.zaremba@...el.com>, Jesper Dangaard Brouer
 <hawk@...nel.org>, Ilias Apalodimas <ilias.apalodimas@...aro.org>,
 Christoph Hellwig <hch@....de>, Paul Menzel <pmenzel@...gen.mpg.de>,
 netdev@...r.kernel.org,  intel-wired-lan@...ts.osuosl.org,
 linux-kernel@...r.kernel.org
Subject: Re: [PATCH net-next v2 02/12] iavf: kill "legacy-rx" for good

On Thu, 2023-05-25 at 14:57 +0200, Alexander Lobakin wrote:
> Ever since build_skb() became stable, the old way with allocating an skb
> for storing the headers separately, which will be then copied manually,
> was slower, less flexible and thus obsolete.
> 
> * it had higher pressure on MM since it actually allocates new pages,
>   which then get split and refcount-biased (NAPI page cache);
> * it implies memcpy() of packet headers (40+ bytes per each frame);
> * the actual header length was calculated via eth_get_headlen(), which
>   invokes Flow Dissector and thus wastes a bunch of CPU cycles;
> * XDP makes it even more weird since it requires headroom for long and
>   also tailroom for some time (since mbuf landed). Take a look at the
>   ice driver, which is built around work-arounds to make XDP work with
>   it.
> 
> Even on some quite low-end hardware (not a common case for 100G NICs) it
> was performing worse.
> The only advantage "legacy-rx" had is that it didn't require any
> reserved headroom and tailroom. But iavf didn't use this, as it always
> splits pages into two halves of 2k, while that save would only be useful
> when striding. And again, XDP effectively removes that sole pro.
> 

The "legacy-rx" was never about performance. It was mostly about
providing a fall back in the event of an unexpected behavior. Keep in
mind that in order to enable this we are leaving the page mapped and
syncing it multiple times. In order to enable support for this we had
to add several new items that I had deemed to be a bit risky such as
support for DMA pages that were synced by the driver instead of on
map/unmap and the use of the build_skb logic.

My main concern was that if we ever ran into  header corruption we
could switch this on and then the pages would only be writable by the
device.

> There's a train of features to land in IAVF soon: Page Pool, XDP, XSk,
> multi-buffer etc. Each new would require adding more and more Danse
> Macabre for absolutely no reason, besides making hotpath less and less
> effective.
> Remove the "feature" with all the related code. This includes at least
> one very hot branch (typically hit on each new frame), which was either
> always-true or always-false at least for a complete NAPI bulk of 64
> frames, the whole private flags cruft and so on. Some stats:
> 
> Function: add/remove: 0/2 grow/shrink: 0/7 up/down: 0/-774 (-774)
> RO Data: add/remove: 0/1 grow/shrink: 0/0 up/down: 0/-40 (-40)
> 
> Signed-off-by: Alexander Lobakin <aleksander.lobakin@...el.com>

I support this 100%. The legacy-rx flag was meant as more of a fall-
back in the event that the build_skb code wasn't present or was broken
in some way. It wasn't really meant to be carried forward into drivers
as the last one I had added this to was i40e over 6 years ago.

Since it has been about 6 years without any issues I would say we are
safe to remove it.

Reviewed-by: Alexander Duyck <alexanderduyck@...com>

Powered by blists - more mailing lists