Date:   Wed, 8 Feb 2023 17:27:57 +0100
From:   Maciej Fijalkowski <maciej.fijalkowski@intel.com>
To:     Alexander H Duyck <alexander.duyck@gmail.com>
CC:     Jason Xing <kerneljasonxing@gmail.com>,
        <jesse.brandeburg@intel.com>, <anthony.l.nguyen@intel.com>,
        <davem@davemloft.net>, <edumazet@google.com>, <kuba@kernel.org>,
        <pabeni@redhat.com>, <richardcochran@gmail.com>, <ast@kernel.org>,
        <daniel@iogearbox.net>, <hawk@kernel.org>,
        <john.fastabend@gmail.com>, <alexandr.lobakin@intel.com>,
        <intel-wired-lan@lists.osuosl.org>, <netdev@vger.kernel.org>,
        <linux-kernel@vger.kernel.org>, <bpf@vger.kernel.org>,
        Jason Xing <kernelxing@tencent.com>
Subject: Re: [PATCH net v4 1/3] ixgbe: allow to increase MTU to 3K with XDP
 enabled
Message-ID: <Y+PNjcrSxKc0vD3s@boxer>
References: <20230208024333.10465-1-kerneljasonxing@gmail.com>
 <2bfcd7d92a6971416f58d9aac6e74840d5ae240a.camel@gmail.com>
Content-Type: text/plain; charset="us-ascii"
Content-Disposition: inline
In-Reply-To: <2bfcd7d92a6971416f58d9aac6e74840d5ae240a.camel@gmail.com>
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0: =?us-ascii?Q?V9qcTQJjWts6DBA6llvgLOkNx/cTaIdaqdos4N1n7HsAdKI3KCJrjFWJJpe5?=
 =?us-ascii?Q?DgVrnpwpcMiOR7WpTDOdvU/2JVabtPk3Tzbgs/eQENMbBLlSEGhGZ4T7hE2O?=
 =?us-ascii?Q?EKgqWztl61amyWebwzPmaCISpxmb/FaKlke5tP9bbpalu6f2f4kiF0KxlV6d?=
 =?us-ascii?Q?DDdl+Kd8/pA/BNKYkOreSthWDi5YPrZZmMjIrTLCmVbbCGPD3pfW6O6s2Egx?=
 =?us-ascii?Q?jn08hH0bB8P9a82sRHAAfcn1mXqCvutCsskdonmr6wlkOZ0kbR1IOUtwNxFY?=
 =?us-ascii?Q?6ccpsvsLsirTzLzsj0S7Epu2yzbJk6dpWL/k0rrx3PpDON0ChYT+wpC4CT1A?=
 =?us-ascii?Q?xwIuGlBVR4ph2xsey6hPFct+T3eyekMafPdG/KcVKYSMMmg8ErN+ip5j0CDa?=
 =?us-ascii?Q?GxS8wk5GAsXelqT2l1YVALR6eU3jfxNqsVYmoTYWMyXyLjjn8uFbmzSMjO8U?=
 =?us-ascii?Q?cRBA3RjcTmL03I+QwWf3Lbn26rbQmwkDzwWfMu7GvHHftpeKseGQkHBFHfoZ?=
 =?us-ascii?Q?Hrg/2LNgOY7B+e8Sz11cJb9bBoqFSM8vtY/rWhhovajzFsBCvZyoHlV6jv4k?=
 =?us-ascii?Q?kvKHr/OFsWOyvMaiMq7mXDmsS7SiHJmzTovKuO9x1WAMmcOVGvA4bsit72m4?=
 =?us-ascii?Q?j9JayXIE63rkl1URFHI90s3iSOVWdt/z8SNCU0SOTpv8c9AyrO8OuYytjRWn?=
 =?us-ascii?Q?qWLZngah1KHYoPk12yEaG6p152kU9S5IztiXQVgo7c8ACdG8HeQvZny08Rhq?=
 =?us-ascii?Q?Ytc0+keE/GWtgWyBtEtSCNOhHC4RRGMTNhIoqDbihM8b1WMSrzu8YwEpZ5Ao?=
 =?us-ascii?Q?3FqI0s+66kVn1Oh1dYSLda+zJVGt2uHR2KvVshv3ZW6AwqakAKc/chbEl3ep?=
 =?us-ascii?Q?tffMR44ojXycD1YOi6U8f8Or11QjHgeUSw32/8PgT6gbmgm+3klg7kdl1Yh5?=
 =?us-ascii?Q?C5dTMos8KX0xxMYgC5v70pgzWQ+t7NoROffZ0o4ErSZF9UB24+CHGzSuPKJR?=
 =?us-ascii?Q?RQg8PZstMm0zB8m20a62uEQEoOOZlC8LPewdRyVBUCKTt32Tlhl3NbLMEX9O?=
 =?us-ascii?Q?jPCZgiXQFn+hEj6LbLlN5olJYR+P3JnAMUBA4frCjhCPPQEuZnULuyw0OqAs?=
 =?us-ascii?Q?N7+uCu66XH6PCHEJlS6wwaFaCDXYwwYPAxmmQLhP+n95AoPBgAuEdF5Kg8JO?=
 =?us-ascii?Q?YFSlCjLzbPlAyaPHuPYxQ2uz07hnaSiou4fPEu15gGhEDVBnb+LYw9dzoJxe?=
 =?us-ascii?Q?YrWo/CvZUhlJSzv/cMxsAIOkStbMzeIvKTDa8j1A3OdIhh7A7uDZkgcb85SP?=
 =?us-ascii?Q?rzgoHMsrVJX7cCMVD0TUL4rETjxFks7WimB00KzVuyce+HS8e7RNMOGIL5Uq?=
 =?us-ascii?Q?KnYnIQT2RpX6af2ApND3j9t005pH9hT5Vu0BpSGFzocBLT1WnRPcq295X/Fd?=
 =?us-ascii?Q?Ow5ktjasQ10PBaFagA/e+Q+BUgbJ7cBPulwR9gByvsSFhi7FoGWGdGC8H7yM?=
 =?us-ascii?Q?eUh3v06TlO1+Q4xPiqGvWvpHVQBPZmF2rKfk167FOZHDY3Mequh9Wm4rXl/k?=
 =?us-ascii?Q?4bZOHbrEJ7vHEvc8Cr/CPXr/S/eXosvHUrTxvo37sFEMh0JslIeI0l2gv/RV?=
 =?us-ascii?Q?A1Bqw04o1B/WodebZkburq8=3D?=
X-MS-Exchange-CrossTenant-Network-Message-Id: 3221f783-3ead-42a8-1b68-08db09f17976
X-MS-Exchange-CrossTenant-AuthSource: DM4PR11MB6117.namprd11.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 08 Feb 2023 16:28:13.4327
 (UTC)
X-MS-Exchange-CrossTenant-FromEntityHeader: Hosted
X-MS-Exchange-CrossTenant-Id: 46c98d88-e344-4ed4-8496-4ed7712e255d
X-MS-Exchange-CrossTenant-MailboxType: HOSTED
X-MS-Exchange-CrossTenant-UserPrincipalName: 2z0K3ZleH42H9YTEgDfM/mPmM728udb5rcuk/Q1LWDZoOhxfDeuiNLi6kqwTlO7VNWa23TNj2KemPuMpElXDm12RF2OUFVxiJtuUFU6YdMg=
X-MS-Exchange-Transport-CrossTenantHeadersStamped: SN7PR11MB7638
X-OriginatorOrg: intel.com
Precedence: bulk
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org

On Wed, Feb 08, 2023 at 07:37:57AM -0800, Alexander H Duyck wrote:
> On Wed, 2023-02-08 at 10:43 +0800, Jason Xing wrote:
> > From: Jason Xing <kernelxing@tencent.com>
> > 
> > Recently I encountered one case where I cannot increase the MTU size
> > directly from 1500 to a much bigger value with XDP enabled if the
> > server is equipped with IXGBE card, which happened on thousands of
> > servers in production environment. After appling the current patch,
> > we can set the maximum MTU size to 3K.
> > 
> > This patch follows the behavior of changing MTU as i40e/ice does.
> > 
> > Referrences:
> > [1] commit 23b44513c3e6 ("ice: allow 3k MTU for XDP")
> > [2] commit 0c8493d90b6b ("i40e: add XDP support for pass and drop actions")
> > 
> > Fixes: fabf1bce103a ("ixgbe: Prevent unsupported configurations with XDP")
> > Signed-off-by: Jason Xing <kernelxing@tencent.com>
> 
> This is based on the broken premise that w/ XDP we are using a 4K page.
> The ixgbe driver isn't using page pool and is therefore running on
> different limitations. The ixgbe driver is only using 2K slices of the
> 4K page. In addition that is reduced to 1.5K to allow for headroom and
> the shared info in the buffer.
> 
> Currently the only way a 3K buffer would work is if FCoE is enabled and
> in that case the driver is using order 1 pages and still using the
> split buffer approach.

Hey Alex, interesting, we based this on the following logic from
ixgbe_set_rx_buffer_len() I guess:

#if (PAGE_SIZE < 8192)
		if (adapter->flags2 & IXGBE_FLAG2_RSC_ENABLED)
			set_bit(__IXGBE_RX_3K_BUFFER, &rx_ring->state);

		if (IXGBE_2K_TOO_SMALL_WITH_PADDING ||
		    (max_frame > (ETH_FRAME_LEN + ETH_FCS_LEN)))
			set_bit(__IXGBE_RX_3K_BUFFER, &rx_ring->state);
#endif

so we assumed that ixgbe is no different than i40e/ice in these terms, but
we ignored whole overhead of LRO/RSC that ixgbe carries.

I am not actively working with ixgbe but I know that you were the main dev
of it, so without premature dive into the datasheet and codebase, are you
really sure that 3k mtu for XDP is a no go?

> 
> Changing the MTU to more than 1.5K will allow multi-buffer frames which
> would break things when you try to use XDP_REDIRECT or XDP_TX on frames
> over 1.5K in size. For things like XDP_PASS, XDP_DROP, and XDP_ABORT it
> should still work as long as you don't attempt to reach beyond the 1.5K
> boundary.
> 
> Until this driver supports XDP multi-buffer I don't think you can
> increase the MTU past 1.5K. If you are wanting a larger MTU you should
> look at enabling XDP multi-buffer and then just drop the XDP
> limitations entirely.
> 
> > ---
> > v4:
> > 1) use ':' instead of '-' for kdoc
> > 
> > v3:
> > 1) modify the titile and body message.
> > 
> > v2:
> > 1) change the commit message.
> > 2) modify the logic when changing MTU size suggested by Maciej and Alexander.
> > ---
> >  drivers/net/ethernet/intel/ixgbe/ixgbe_main.c | 25 ++++++++++++-------
> >  1 file changed, 16 insertions(+), 9 deletions(-)
> > 
> > diff --git a/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c b/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c
> > index ab8370c413f3..25ca329f7d3c 100644
> > --- a/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c
> > +++ b/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c
> > @@ -6777,6 +6777,18 @@ static void ixgbe_free_all_rx_resources(struct ixgbe_adapter *adapter)
> >  			ixgbe_free_rx_resources(adapter->rx_ring[i]);
> >  }
> >  
> > +/**
> > + * ixgbe_max_xdp_frame_size - returns the maximum allowed frame size for XDP
> > + * @adapter: device handle, pointer to adapter
> > + */
> > +static int ixgbe_max_xdp_frame_size(struct ixgbe_adapter *adapter)
> > +{
> > +	if (PAGE_SIZE >= 8192 || adapter->flags2 & IXGBE_FLAG2_RX_LEGACY)
> > +		return IXGBE_RXBUFFER_2K;
> > +	else
> > +		return IXGBE_RXBUFFER_3K;
> > +}
> > +
> 
> There is no difference in the buffer allocation approach for LEGACY vs
> non-legacy. The difference is if we are building the frame around the
> buffer using build_skb or we are adding it as a frag and then copying
> out the header.
> 
> >  /**
> >   * ixgbe_change_mtu - Change the Maximum Transfer Unit
> >   * @netdev: network interface device structure
> > @@ -6788,18 +6800,13 @@ static int ixgbe_change_mtu(struct net_device *netdev, int new_mtu)
> >  {
> >  	struct ixgbe_adapter *adapter = netdev_priv(netdev);
> >  
> > -	if (adapter->xdp_prog) {
> > +	if (ixgbe_enabled_xdp_adapter(adapter)) {
> >  		int new_frame_size = new_mtu + ETH_HLEN + ETH_FCS_LEN +
> >  				     VLAN_HLEN;
> > -		int i;
> > -
> > -		for (i = 0; i < adapter->num_rx_queues; i++) {
> > -			struct ixgbe_ring *ring = adapter->rx_ring[i];
> >  
> > -			if (new_frame_size > ixgbe_rx_bufsz(ring)) {
> > -				e_warn(probe, "Requested MTU size is not supported with XDP\n");
> > -				return -EINVAL;
> > -			}
> > +		if (new_frame_size > ixgbe_max_xdp_frame_size(adapter)) {
> > +			e_warn(probe, "Requested MTU size is not supported with XDP\n");
> > +			return -EINVAL;
> >  		}
> >  	}
> >  
>