linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH net] ixgbe: allow to increase MTU to some extent with XDP enalbed
@ 2023-01-21  8:55 Jason Xing
  2023-01-22 20:21 ` [Intel-wired-lan] " Paul Menzel
                   ` (2 more replies)
  0 siblings, 3 replies; 7+ messages in thread
From: Jason Xing @ 2023-01-21  8:55 UTC (permalink / raw)
  To: jesse.brandeburg, anthony.l.nguyen, davem, edumazet, kuba,
	pabeni, richardcochran, ast, daniel, hawk, john.fastabend
  Cc: intel-wired-lan, netdev, linux-kernel, bpf, kerneljasonxing, Jason Xing

From: Jason Xing <kernelxing@tencent.com>

I encountered one case where I cannot increase the MTU size with XDP
enabled if the server is equipped with IXGBE card, which happened on
thousands of servers. I noticed it was prohibited from 2017[1] and
added size checks[2] if allowed soon after the previous patch.

Interesting part goes like this:
1) Changing MTU directly from 1500 (default value) to 2000 doesn't
work because the driver finds out that 'new_frame_size >
ixgbe_rx_bufsz(ring)' in ixgbe_change_mtu() function.
2) However, if we change MTU to 1501 then change from 1501 to 2000, it
does work, because the driver sets __IXGBE_RX_3K_BUFFER when MTU size
is converted to 1501, which later size check policy allows.

The default MTU value for most servers is 1500 which cannot be adjusted
directly to the value larger than IXGBE_MAX_2K_FRAME_BUILD_SKB (1534 or
1536) if it loads XDP.

After I do a quick study on the manner of i40E driver allowing two kinds
of buffer size (one is 2048 while another is 3072) to support XDP mode in
i40e_max_xdp_frame_size(), I believe the default MTU size is possibly not
satisfied in XDP mode when IXGBE driver is in use, we sometimes need to
insert a new header, say, vxlan header. So setting the 3K-buffer flag
could solve the issue.

[1] commit 38b7e7f8ae82 ("ixgbe: Do not allow LRO or MTU change with XDP")
[2] commit fabf1bce103a ("ixgbe: Prevent unsupported configurations with
XDP")

Signed-off-by: Jason Xing <kernelxing@tencent.com>
---
 drivers/net/ethernet/intel/ixgbe/ixgbe_main.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c b/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c
index ab8370c413f3..dc016582f91e 100644
--- a/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c
+++ b/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c
@@ -4313,6 +4313,9 @@ static void ixgbe_set_rx_buffer_len(struct ixgbe_adapter *adapter)
 		if (IXGBE_2K_TOO_SMALL_WITH_PADDING ||
 		    (max_frame > (ETH_FRAME_LEN + ETH_FCS_LEN)))
 			set_bit(__IXGBE_RX_3K_BUFFER, &rx_ring->state);
+
+		if (ixgbe_enabled_xdp_adapter(adapter))
+			set_bit(__IXGBE_RX_3K_BUFFER, &rx_ring->state);
 #endif
 	}
 }
-- 
2.37.3


^ permalink raw reply related	[flat|nested] 7+ messages in thread

* Re: [Intel-wired-lan] [PATCH net] ixgbe: allow to increase MTU to some extent with XDP enalbed
  2023-01-21  8:55 [PATCH net] ixgbe: allow to increase MTU to some extent with XDP enalbed Jason Xing
@ 2023-01-22 20:21 ` Paul Menzel
  2023-01-23  1:53   ` Jason Xing
  2023-01-25  7:35 ` Jason Xing
  2023-01-26 12:17 ` Maciej Fijalkowski
  2 siblings, 1 reply; 7+ messages in thread
From: Paul Menzel @ 2023-01-22 20:21 UTC (permalink / raw)
  To: Jason Xing
  Cc: jesse.brandeburg, anthony.l.nguyen, davem, edumazet, kuba,
	pabeni, richardcochran, ast, daniel, hawk, john.fastabend,
	netdev, linux-kernel, intel-wired-lan, bpf, Jason Xing

Dear Jason,


Thank you for your patch.

Am 21.01.23 um 09:55 schrieb Jason Xing:
> From: Jason Xing <kernelxing@tencent.com>

There is a small typo in the summary: ena*bl*ed

> I encountered one case where I cannot increase the MTU size with XDP
> enabled if the server is equipped with IXGBE card, which happened on
> thousands of servers. I noticed it was prohibited from 2017[1] and

That’s included since Linux 4.19-rc1.

> added size checks[2] if allowed soon after the previous patch.
> 
> Interesting part goes like this:
> 1) Changing MTU directly from 1500 (default value) to 2000 doesn't
> work because the driver finds out that 'new_frame_size >
> ixgbe_rx_bufsz(ring)' in ixgbe_change_mtu() function.
> 2) However, if we change MTU to 1501 then change from 1501 to 2000, it
> does work, because the driver sets __IXGBE_RX_3K_BUFFER when MTU size
> is converted to 1501, which later size check policy allows.
> 
> The default MTU value for most servers is 1500 which cannot be adjusted
> directly to the value larger than IXGBE_MAX_2K_FRAME_BUILD_SKB (1534 or
> 1536) if it loads XDP.
> 
> After I do a quick study on the manner of i40E driver allowing two kinds
> of buffer size (one is 2048 while another is 3072) to support XDP mode in
> i40e_max_xdp_frame_size(), I believe the default MTU size is possibly not
> satisfied in XDP mode when IXGBE driver is in use, we sometimes need to
> insert a new header, say, vxlan header. So setting the 3K-buffer flag
> could solve the issue.

What card did you test with exactly?

> [1] commit 38b7e7f8ae82 ("ixgbe: Do not allow LRO or MTU change with XDP")
> [2] commit fabf1bce103a ("ixgbe: Prevent unsupported configurations with
> XDP")

I’d say to not break the line in references.

> Signed-off-by: Jason Xing <kernelxing@tencent.com>
> ---
>   drivers/net/ethernet/intel/ixgbe/ixgbe_main.c | 3 +++
>   1 file changed, 3 insertions(+)
> 
> diff --git a/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c b/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c
> index ab8370c413f3..dc016582f91e 100644
> --- a/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c
> +++ b/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c
> @@ -4313,6 +4313,9 @@ static void ixgbe_set_rx_buffer_len(struct ixgbe_adapter *adapter)
>   		if (IXGBE_2K_TOO_SMALL_WITH_PADDING ||
>   		    (max_frame > (ETH_FRAME_LEN + ETH_FCS_LEN)))
>   			set_bit(__IXGBE_RX_3K_BUFFER, &rx_ring->state);
> +
> +		if (ixgbe_enabled_xdp_adapter(adapter))
> +			set_bit(__IXGBE_RX_3K_BUFFER, &rx_ring->state);
>   #endif
>   	}
>   }


Kind regards,

Paul

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [Intel-wired-lan] [PATCH net] ixgbe: allow to increase MTU to some extent with XDP enalbed
  2023-01-22 20:21 ` [Intel-wired-lan] " Paul Menzel
@ 2023-01-23  1:53   ` Jason Xing
  0 siblings, 0 replies; 7+ messages in thread
From: Jason Xing @ 2023-01-23  1:53 UTC (permalink / raw)
  To: Paul Menzel
  Cc: jesse.brandeburg, anthony.l.nguyen, davem, edumazet, kuba,
	pabeni, richardcochran, ast, daniel, hawk, john.fastabend,
	netdev, linux-kernel, intel-wired-lan, bpf, Jason Xing

On Mon, Jan 23, 2023 at 4:21 AM Paul Menzel <pmenzel@molgen.mpg.de> wrote:
>
> Dear Jason,
>
>
> Thank you for your patch.

Hello Paul,

Thanks for the review.

>
> Am 21.01.23 um 09:55 schrieb Jason Xing:
> > From: Jason Xing <kernelxing@tencent.com>
>
> There is a small typo in the summary: ena*bl*ed
>

Sure it is. That's my fault.

> > I encountered one case where I cannot increase the MTU size with XDP
> > enabled if the server is equipped with IXGBE card, which happened on
> > thousands of servers. I noticed it was prohibited from 2017[1] and
>
> That’s included since Linux 4.19-rc1.
>
> > added size checks[2] if allowed soon after the previous patch.
> >
> > Interesting part goes like this:
> > 1) Changing MTU directly from 1500 (default value) to 2000 doesn't
> > work because the driver finds out that 'new_frame_size >
> > ixgbe_rx_bufsz(ring)' in ixgbe_change_mtu() function.
> > 2) However, if we change MTU to 1501 then change from 1501 to 2000, it
> > does work, because the driver sets __IXGBE_RX_3K_BUFFER when MTU size
> > is converted to 1501, which later size check policy allows.
> >
> > The default MTU value for most servers is 1500 which cannot be adjusted
> > directly to the value larger than IXGBE_MAX_2K_FRAME_BUILD_SKB (1534 or
> > 1536) if it loads XDP.
> >
> > After I do a quick study on the manner of i40E driver allowing two kinds
> > of buffer size (one is 2048 while another is 3072) to support XDP mode in
> > i40e_max_xdp_frame_size(), I believe the default MTU size is possibly not
> > satisfied in XDP mode when IXGBE driver is in use, we sometimes need to
> > insert a new header, say, vxlan header. So setting the 3K-buffer flag
> > could solve the issue.
>
> What card did you test with exactly?
>

It is the IXGBE driver that has such an issue.  The I40E driver I
mentioned here is only for contrast. It's not that proper from my
point of view if the IXGBE driver cannot directly adjust to 2000. Thus
I would like more reviews and suggestions.

Thanks,
Jason

> > [1] commit 38b7e7f8ae82 ("ixgbe: Do not allow LRO or MTU change with XDP")
> > [2] commit fabf1bce103a ("ixgbe: Prevent unsupported configurations with
> > XDP")
>
> I’d say to not break the line in references.
>
> > Signed-off-by: Jason Xing <kernelxing@tencent.com>
> > ---
> >   drivers/net/ethernet/intel/ixgbe/ixgbe_main.c | 3 +++
> >   1 file changed, 3 insertions(+)
> >
> > diff --git a/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c b/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c
> > index ab8370c413f3..dc016582f91e 100644
> > --- a/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c
> > +++ b/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c
> > @@ -4313,6 +4313,9 @@ static void ixgbe_set_rx_buffer_len(struct ixgbe_adapter *adapter)
> >               if (IXGBE_2K_TOO_SMALL_WITH_PADDING ||
> >                   (max_frame > (ETH_FRAME_LEN + ETH_FCS_LEN)))
> >                       set_bit(__IXGBE_RX_3K_BUFFER, &rx_ring->state);
> > +
> > +             if (ixgbe_enabled_xdp_adapter(adapter))
> > +                     set_bit(__IXGBE_RX_3K_BUFFER, &rx_ring->state);
> >   #endif
> >       }
> >   }
>
>
> Kind regards,
>
> Paul

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH net] ixgbe: allow to increase MTU to some extent with XDP enalbed
  2023-01-21  8:55 [PATCH net] ixgbe: allow to increase MTU to some extent with XDP enalbed Jason Xing
  2023-01-22 20:21 ` [Intel-wired-lan] " Paul Menzel
@ 2023-01-25  7:35 ` Jason Xing
  2023-01-26 12:17 ` Maciej Fijalkowski
  2 siblings, 0 replies; 7+ messages in thread
From: Jason Xing @ 2023-01-25  7:35 UTC (permalink / raw)
  To: jesse.brandeburg, anthony.l.nguyen, davem, edumazet, kuba,
	pabeni, richardcochran, ast, daniel, hawk, John Fastabend
  Cc: intel-wired-lan, netdev, linux-kernel, bpf, Jason Xing

I'm sorry. I just noticed that I sent it to the wrong email address of
john.fastabend previously. So I corrected it here.

Thanks,
Jason

On Sat, Jan 21, 2023 at 4:55 PM Jason Xing <kerneljasonxing@gmail.com> wrote:
>
> From: Jason Xing <kernelxing@tencent.com>
>
> I encountered one case where I cannot increase the MTU size with XDP
> enabled if the server is equipped with IXGBE card, which happened on
> thousands of servers. I noticed it was prohibited from 2017[1] and
> added size checks[2] if allowed soon after the previous patch.
>
> Interesting part goes like this:
> 1) Changing MTU directly from 1500 (default value) to 2000 doesn't
> work because the driver finds out that 'new_frame_size >
> ixgbe_rx_bufsz(ring)' in ixgbe_change_mtu() function.
> 2) However, if we change MTU to 1501 then change from 1501 to 2000, it
> does work, because the driver sets __IXGBE_RX_3K_BUFFER when MTU size
> is converted to 1501, which later size check policy allows.
>
> The default MTU value for most servers is 1500 which cannot be adjusted
> directly to the value larger than IXGBE_MAX_2K_FRAME_BUILD_SKB (1534 or
> 1536) if it loads XDP.
>
> After I do a quick study on the manner of i40E driver allowing two kinds
> of buffer size (one is 2048 while another is 3072) to support XDP mode in
> i40e_max_xdp_frame_size(), I believe the default MTU size is possibly not
> satisfied in XDP mode when IXGBE driver is in use, we sometimes need to
> insert a new header, say, vxlan header. So setting the 3K-buffer flag
> could solve the issue.
>
> [1] commit 38b7e7f8ae82 ("ixgbe: Do not allow LRO or MTU change with XDP")
> [2] commit fabf1bce103a ("ixgbe: Prevent unsupported configurations with
> XDP")
>
> Signed-off-by: Jason Xing <kernelxing@tencent.com>
> ---
>  drivers/net/ethernet/intel/ixgbe/ixgbe_main.c | 3 +++
>  1 file changed, 3 insertions(+)
>
> diff --git a/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c b/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c
> index ab8370c413f3..dc016582f91e 100644
> --- a/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c
> +++ b/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c
> @@ -4313,6 +4313,9 @@ static void ixgbe_set_rx_buffer_len(struct ixgbe_adapter *adapter)
>                 if (IXGBE_2K_TOO_SMALL_WITH_PADDING ||
>                     (max_frame > (ETH_FRAME_LEN + ETH_FCS_LEN)))
>                         set_bit(__IXGBE_RX_3K_BUFFER, &rx_ring->state);
> +
> +               if (ixgbe_enabled_xdp_adapter(adapter))
> +                       set_bit(__IXGBE_RX_3K_BUFFER, &rx_ring->state);
>  #endif
>         }
>  }
> --
> 2.37.3
>

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH net] ixgbe: allow to increase MTU to some extent with XDP enalbed
  2023-01-21  8:55 [PATCH net] ixgbe: allow to increase MTU to some extent with XDP enalbed Jason Xing
  2023-01-22 20:21 ` [Intel-wired-lan] " Paul Menzel
  2023-01-25  7:35 ` Jason Xing
@ 2023-01-26 12:17 ` Maciej Fijalkowski
  2023-01-26 15:56   ` Alexander Lobakin
  2 siblings, 1 reply; 7+ messages in thread
From: Maciej Fijalkowski @ 2023-01-26 12:17 UTC (permalink / raw)
  To: Jason Xing
  Cc: jesse.brandeburg, anthony.l.nguyen, davem, edumazet, kuba,
	pabeni, richardcochran, ast, daniel, hawk, john.fastabend,
	intel-wired-lan, netdev, linux-kernel, bpf, Jason Xing,
	magnus.karlsson, tirthendu.sarkar, alexandr.lobakin

On Sat, Jan 21, 2023 at 04:55:21PM +0800, Jason Xing wrote:
> From: Jason Xing <kernelxing@tencent.com>
> 
> I encountered one case where I cannot increase the MTU size with XDP
> enabled if the server is equipped with IXGBE card, which happened on
> thousands of servers. I noticed it was prohibited from 2017[1] and
> added size checks[2] if allowed soon after the previous patch.
> 
> Interesting part goes like this:
> 1) Changing MTU directly from 1500 (default value) to 2000 doesn't
> work because the driver finds out that 'new_frame_size >
> ixgbe_rx_bufsz(ring)' in ixgbe_change_mtu() function.
> 2) However, if we change MTU to 1501 then change from 1501 to 2000, it
> does work, because the driver sets __IXGBE_RX_3K_BUFFER when MTU size
> is converted to 1501, which later size check policy allows.
> 
> The default MTU value for most servers is 1500 which cannot be adjusted
> directly to the value larger than IXGBE_MAX_2K_FRAME_BUILD_SKB (1534 or
> 1536) if it loads XDP.
> 
> After I do a quick study on the manner of i40E driver allowing two kinds
> of buffer size (one is 2048 while another is 3072) to support XDP mode in
> i40e_max_xdp_frame_size(), I believe the default MTU size is possibly not
> satisfied in XDP mode when IXGBE driver is in use, we sometimes need to
> insert a new header, say, vxlan header. So setting the 3K-buffer flag
> could solve the issue.
> 
> [1] commit 38b7e7f8ae82 ("ixgbe: Do not allow LRO or MTU change with XDP")
> [2] commit fabf1bce103a ("ixgbe: Prevent unsupported configurations with
> XDP")
> 
> Signed-off-by: Jason Xing <kernelxing@tencent.com>
> ---
>  drivers/net/ethernet/intel/ixgbe/ixgbe_main.c | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c b/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c
> index ab8370c413f3..dc016582f91e 100644
> --- a/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c
> +++ b/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c
> @@ -4313,6 +4313,9 @@ static void ixgbe_set_rx_buffer_len(struct ixgbe_adapter *adapter)
>  		if (IXGBE_2K_TOO_SMALL_WITH_PADDING ||
>  		    (max_frame > (ETH_FRAME_LEN + ETH_FCS_LEN)))
>  			set_bit(__IXGBE_RX_3K_BUFFER, &rx_ring->state);
> +
> +		if (ixgbe_enabled_xdp_adapter(adapter))
> +			set_bit(__IXGBE_RX_3K_BUFFER, &rx_ring->state);

This will result with unnecessary overhead for 1500 MTU because you will
be working on order-1 pages. Instead I would focus on fixing
ixgbe_change_mtu() and stop relying on ixgbe_rx_bufsz() in there. You can
check what we do on ice/i40e sides.

I'm not looking actively into ixgbe internals but I don't think that there
is anything that stops us from using 3k buffers with XDP.

>  #endif
>  	}
>  }
> -- 
> 2.37.3
> 

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH net] ixgbe: allow to increase MTU to some extent with XDP enalbed
  2023-01-26 12:17 ` Maciej Fijalkowski
@ 2023-01-26 15:56   ` Alexander Lobakin
  2023-01-27  4:16     ` Jason Xing
  0 siblings, 1 reply; 7+ messages in thread
From: Alexander Lobakin @ 2023-01-26 15:56 UTC (permalink / raw)
  To: Jason Xing
  Cc: Maciej Fijalkowski, jesse.brandeburg, anthony.l.nguyen, davem,
	edumazet, kuba, pabeni, richardcochran, ast, daniel, hawk,
	john.fastabend, intel-wired-lan, netdev, linux-kernel, bpf,
	Jason Xing, magnus.karlsson, tirthendu.sarkar

From: Maciej Fijalkowski <maciej.fijalkowski@intel.com>
Date: Thu, 26 Jan 2023 13:17:20 +0100

> On Sat, Jan 21, 2023 at 04:55:21PM +0800, Jason Xing wrote:
>> From: Jason Xing <kernelxing@tencent.com>
>>
>> I encountered one case where I cannot increase the MTU size with XDP
>> enabled if the server is equipped with IXGBE card, which happened on
>> thousands of servers. I noticed it was prohibited from 2017[1] and
>> added size checks[2] if allowed soon after the previous patch.
>>
>> Interesting part goes like this:
>> 1) Changing MTU directly from 1500 (default value) to 2000 doesn't
>> work because the driver finds out that 'new_frame_size >
>> ixgbe_rx_bufsz(ring)' in ixgbe_change_mtu() function.
>> 2) However, if we change MTU to 1501 then change from 1501 to 2000, it
>> does work, because the driver sets __IXGBE_RX_3K_BUFFER when MTU size
>> is converted to 1501, which later size check policy allows.
>>
>> The default MTU value for most servers is 1500 which cannot be adjusted
>> directly to the value larger than IXGBE_MAX_2K_FRAME_BUILD_SKB (1534 or
>> 1536) if it loads XDP.
>>
>> After I do a quick study on the manner of i40E driver allowing two kinds
>> of buffer size (one is 2048 while another is 3072) to support XDP mode in
>> i40e_max_xdp_frame_size(), I believe the default MTU size is possibly not
>> satisfied in XDP mode when IXGBE driver is in use, we sometimes need to
>> insert a new header, say, vxlan header. So setting the 3K-buffer flag
>> could solve the issue.
>>
>> [1] commit 38b7e7f8ae82 ("ixgbe: Do not allow LRO or MTU change with XDP")
>> [2] commit fabf1bce103a ("ixgbe: Prevent unsupported configurations with
>> XDP")
>>
>> Signed-off-by: Jason Xing <kernelxing@tencent.com>
>> ---
>>  drivers/net/ethernet/intel/ixgbe/ixgbe_main.c | 3 +++
>>  1 file changed, 3 insertions(+)
>>
>> diff --git a/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c b/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c
>> index ab8370c413f3..dc016582f91e 100644
>> --- a/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c
>> +++ b/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c
>> @@ -4313,6 +4313,9 @@ static void ixgbe_set_rx_buffer_len(struct ixgbe_adapter *adapter)
>>  		if (IXGBE_2K_TOO_SMALL_WITH_PADDING ||
>>  		    (max_frame > (ETH_FRAME_LEN + ETH_FCS_LEN)))
>>  			set_bit(__IXGBE_RX_3K_BUFFER, &rx_ring->state);
>> +
>> +		if (ixgbe_enabled_xdp_adapter(adapter))
>> +			set_bit(__IXGBE_RX_3K_BUFFER, &rx_ring->state);
> 
> This will result with unnecessary overhead for 1500 MTU because you will
> be working on order-1 pages. Instead I would focus on fixing
> ixgbe_change_mtu() and stop relying on ixgbe_rx_bufsz() in there. You can
> check what we do on ice/i40e sides.
> 
> I'm not looking actively into ixgbe internals but I don't think that there
> is anything that stops us from using 3k buffers with XDP.

I think it uses the same logics as the rest of drivers: splits a 4k page
into two 2k buffers when MTU is <= 1536, otherwise uses order-1 pages
and uses 3k buffers.

OTOH ixgbe is not fully correct in terms how it calculates Rx headroom,
but the main problem is how it calculates the maximum MTU available when
XDP is on. Our usual MTU supported when XDP is on is 3046 bytes.
For MTU <= 1536, 2k buffers are used even for XDP, so the fix is not
correct. Maciej is right that i40e and ice do that way better and don't
have such issue.

> 
>>  #endif
>>  	}
>>  }
>> -- 
>> 2.37.3
>>

Thanks,
Olek

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH net] ixgbe: allow to increase MTU to some extent with XDP enalbed
  2023-01-26 15:56   ` Alexander Lobakin
@ 2023-01-27  4:16     ` Jason Xing
  0 siblings, 0 replies; 7+ messages in thread
From: Jason Xing @ 2023-01-27  4:16 UTC (permalink / raw)
  To: Alexander Lobakin
  Cc: Maciej Fijalkowski, jesse.brandeburg, anthony.l.nguyen, davem,
	edumazet, kuba, pabeni, richardcochran, ast, daniel, hawk,
	john.fastabend, intel-wired-lan, netdev, linux-kernel, bpf,
	Jason Xing, magnus.karlsson, tirthendu.sarkar

On Thu, Jan 26, 2023 at 11:56 PM Alexander Lobakin
<alexandr.lobakin@intel.com> wrote:
>
> From: Maciej Fijalkowski <maciej.fijalkowski@intel.com>
> Date: Thu, 26 Jan 2023 13:17:20 +0100
>
> > On Sat, Jan 21, 2023 at 04:55:21PM +0800, Jason Xing wrote:
> >> From: Jason Xing <kernelxing@tencent.com>
> >>
> >> I encountered one case where I cannot increase the MTU size with XDP
> >> enabled if the server is equipped with IXGBE card, which happened on
> >> thousands of servers. I noticed it was prohibited from 2017[1] and
> >> added size checks[2] if allowed soon after the previous patch.
> >>
> >> Interesting part goes like this:
> >> 1) Changing MTU directly from 1500 (default value) to 2000 doesn't
> >> work because the driver finds out that 'new_frame_size >
> >> ixgbe_rx_bufsz(ring)' in ixgbe_change_mtu() function.
> >> 2) However, if we change MTU to 1501 then change from 1501 to 2000, it
> >> does work, because the driver sets __IXGBE_RX_3K_BUFFER when MTU size
> >> is converted to 1501, which later size check policy allows.
> >>
> >> The default MTU value for most servers is 1500 which cannot be adjusted
> >> directly to the value larger than IXGBE_MAX_2K_FRAME_BUILD_SKB (1534 or
> >> 1536) if it loads XDP.
> >>
> >> After I do a quick study on the manner of i40E driver allowing two kinds
> >> of buffer size (one is 2048 while another is 3072) to support XDP mode in
> >> i40e_max_xdp_frame_size(), I believe the default MTU size is possibly not
> >> satisfied in XDP mode when IXGBE driver is in use, we sometimes need to
> >> insert a new header, say, vxlan header. So setting the 3K-buffer flag
> >> could solve the issue.
> >>
> >> [1] commit 38b7e7f8ae82 ("ixgbe: Do not allow LRO or MTU change with XDP")
> >> [2] commit fabf1bce103a ("ixgbe: Prevent unsupported configurations with
> >> XDP")
> >>
> >> Signed-off-by: Jason Xing <kernelxing@tencent.com>
> >> ---
> >>  drivers/net/ethernet/intel/ixgbe/ixgbe_main.c | 3 +++
> >>  1 file changed, 3 insertions(+)
> >>
> >> diff --git a/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c b/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c
> >> index ab8370c413f3..dc016582f91e 100644
> >> --- a/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c
> >> +++ b/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c
> >> @@ -4313,6 +4313,9 @@ static void ixgbe_set_rx_buffer_len(struct ixgbe_adapter *adapter)
> >>              if (IXGBE_2K_TOO_SMALL_WITH_PADDING ||
> >>                  (max_frame > (ETH_FRAME_LEN + ETH_FCS_LEN)))
> >>                      set_bit(__IXGBE_RX_3K_BUFFER, &rx_ring->state);
> >> +
> >> +            if (ixgbe_enabled_xdp_adapter(adapter))
> >> +                    set_bit(__IXGBE_RX_3K_BUFFER, &rx_ring->state);
> >
> > This will result with unnecessary overhead for 1500 MTU because you will
> > be working on order-1 pages. Instead I would focus on fixing
> > ixgbe_change_mtu() and stop relying on ixgbe_rx_bufsz() in there. You can
> > check what we do on ice/i40e sides.

Well, now I see the commit 23b44513c3e6f in 2019. Thanks, Maciej.

> >
> > I'm not looking actively into ixgbe internals but I don't think that there
> > is anything that stops us from using 3k buffers with XDP.
>
> I think it uses the same logics as the rest of drivers: splits a 4k page
> into two 2k buffers when MTU is <= 1536, otherwise uses order-1 pages
> and uses 3k buffers.
>
> OTOH ixgbe is not fully correct in terms how it calculates Rx headroom,
> but the main problem is how it calculates the maximum MTU available when
> XDP is on. Our usual MTU supported when XDP is on is 3046 bytes.
> For MTU <= 1536, 2k buffers are used even for XDP, so the fix is not
> correct. Maciej is right that i40e and ice do that way better and don't
> have such issue.

Thank you for the detailed explanation. And yes, I checked this part
in the ice/i40e driver which introduces ice/i40e_max_xdp_frame_size()
to test if we can change MTU size when the driver is loading the XDP
program.
I will rewrite the patch as the i40e/ice does in the next submission.

Thanks,
Jason

>
> >
> >>  #endif
> >>      }
> >>  }
> >> --
> >> 2.37.3
> >>
>
> Thanks,
> Olek

^ permalink raw reply	[flat|nested] 7+ messages in thread

end of thread, other threads:[~2023-01-27  4:16 UTC | newest]

Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2023-01-21  8:55 [PATCH net] ixgbe: allow to increase MTU to some extent with XDP enalbed Jason Xing
2023-01-22 20:21 ` [Intel-wired-lan] " Paul Menzel
2023-01-23  1:53   ` Jason Xing
2023-01-25  7:35 ` Jason Xing
2023-01-26 12:17 ` Maciej Fijalkowski
2023-01-26 15:56   ` Alexander Lobakin
2023-01-27  4:16     ` Jason Xing

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).