All of lore.kernel.org
 help / color / mirror / Atom feed
* [Intel-wired-lan] [PATCH net v2] i40e: Fix the inability to attach XDP program on downed interface
@ 2022-12-05 12:39 Mateusz Palczewski
  2022-12-05 23:09 ` Tony Nguyen
  2022-12-06 11:55 ` Maciej Fijalkowski
  0 siblings, 2 replies; 4+ messages in thread
From: Mateusz Palczewski @ 2022-12-05 12:39 UTC (permalink / raw)
  To: intel-wired-lan; +Cc: Staszewski, BartoszX

From: "Staszewski, BartoszX" <bartoszx.staszewski@intel.com>

Whenever interface was down, function
i40e_xdp was passing vsi->rx_buf_len field
to i40e_xdp_setup() which was equal 0.
i40e_open() calls i40e_vsi_configure_rx()
which configures that field, but that only
happens when interface is up. When it is
down, i40e_open() is not being called, thus
vsi->rx_buf_len is not set.

Solution for this is calculate buffer length
in newly created function - i40e_calculate_vsi_rx_buf_len()
that return actual buffer length. Buffer length is
being calculated based on the same rules applied previously in
i40e_vsi_configure_rx() function.

Fixes: 613142b0bb88 ("i40e: Log error for oversized MTU on device")
Fixes: 0c8493d90b6b ("i40e: add XDP support for pass and drop actions")
Signed-off-by: "Staszewski, BartoszX" <bartoszx.staszewski@intel.com>
Signed-off-by: Mateusz Palczewski <mateusz.palczewski@intel.com>
---
 v2: Fixed commit title and message, this patch is only for a case of
     fresh start so I believe there is no need for rx_buf_len clearance
---
 drivers/net/ethernet/intel/i40e/i40e_main.c | 42 +++++++++++++++------
 1 file changed, 30 insertions(+), 12 deletions(-)

diff --git a/drivers/net/ethernet/intel/i40e/i40e_main.c b/drivers/net/ethernet/intel/i40e/i40e_main.c
index b5dcd15ced36..2fec0cff282c 100644
--- a/drivers/net/ethernet/intel/i40e/i40e_main.c
+++ b/drivers/net/ethernet/intel/i40e/i40e_main.c
@@ -3693,6 +3693,30 @@ static int i40e_vsi_configure_tx(struct i40e_vsi *vsi)
 	return err;
 }
 
+/**
+ * i40e_calculate_vsi_rx_buf_len - Calculates buffer length
+ *
+ * @vsi: VSI to calculate rx_buf_len from
+ */
+static u16 i40e_calculate_vsi_rx_buf_len(struct i40e_vsi *vsi)
+{
+	u16 ret;
+
+	if (!vsi->netdev || (vsi->back->flags & I40E_FLAG_LEGACY_RX)) {
+		ret = I40E_RXBUFFER_2048;
+#if (PAGE_SIZE < 8192)
+	} else if (!I40E_2K_TOO_SMALL_WITH_PADDING &&
+		   (vsi->netdev->mtu <= ETH_DATA_LEN)) {
+		ret = I40E_RXBUFFER_1536 - NET_IP_ALIGN;
+#endif
+	} else {
+		ret = (PAGE_SIZE < 8192) ? I40E_RXBUFFER_3072 :
+					   I40E_RXBUFFER_2048;
+	}
+
+	return ret;
+}
+
 /**
  * i40e_vsi_configure_rx - Configure the VSI for Rx
  * @vsi: the VSI being configured
@@ -3704,20 +3728,14 @@ static int i40e_vsi_configure_rx(struct i40e_vsi *vsi)
 	int err = 0;
 	u16 i;
 
-	if (!vsi->netdev || (vsi->back->flags & I40E_FLAG_LEGACY_RX)) {
-		vsi->max_frame = I40E_MAX_RXBUFFER;
-		vsi->rx_buf_len = I40E_RXBUFFER_2048;
+	vsi->max_frame = I40E_MAX_RXBUFFER;
+	vsi->rx_buf_len = i40e_calculate_vsi_rx_buf_len(vsi);
+
 #if (PAGE_SIZE < 8192)
-	} else if (!I40E_2K_TOO_SMALL_WITH_PADDING &&
-		   (vsi->netdev->mtu <= ETH_DATA_LEN)) {
+	if (vsi->netdev && !I40E_2K_TOO_SMALL_WITH_PADDING &&
+	    vsi->netdev->mtu <= ETH_DATA_LEN)
 		vsi->max_frame = I40E_RXBUFFER_1536 - NET_IP_ALIGN;
-		vsi->rx_buf_len = I40E_RXBUFFER_1536 - NET_IP_ALIGN;
 #endif
-	} else {
-		vsi->max_frame = I40E_MAX_RXBUFFER;
-		vsi->rx_buf_len = (PAGE_SIZE < 8192) ? I40E_RXBUFFER_3072 :
-						       I40E_RXBUFFER_2048;
-	}
 
 	/* set up individual rings */
 	for (i = 0; i < vsi->num_queue_pairs && !err; i++)
@@ -13265,7 +13283,7 @@ static int i40e_xdp_setup(struct i40e_vsi *vsi, struct bpf_prog *prog,
 	int i;
 
 	/* Don't allow frames that span over multiple buffers */
-	if (frame_size > vsi->rx_buf_len) {
+	if (frame_size > i40e_calculate_vsi_rx_buf_len(vsi)) {
 		NL_SET_ERR_MSG_MOD(extack, "MTU too large to enable XDP");
 		return -EINVAL;
 	}
-- 
2.31.1

_______________________________________________
Intel-wired-lan mailing list
Intel-wired-lan@osuosl.org
https://lists.osuosl.org/mailman/listinfo/intel-wired-lan

^ permalink raw reply related	[flat|nested] 4+ messages in thread
* Re: [Intel-wired-lan] [PATCH net v2] i40e: Fix the inability to attach XDP program on downed interface
  2022-12-05 12:39 [Intel-wired-lan] [PATCH net v2] i40e: Fix the inability to attach XDP program on downed interface Mateusz Palczewski
@ 2022-12-05 23:09 ` Tony Nguyen
  2022-12-06 11:55 ` Maciej Fijalkowski
  1 sibling, 0 replies; 4+ messages in thread
From: Tony Nguyen @ 2022-12-05 23:09 UTC (permalink / raw)
  To: Mateusz Palczewski, intel-wired-lan, Maciej Fijalkowski, bjorn,
	Karlsson, Magnus
  Cc: Staszewski, BartoszX

+ Maciej, Bjorn, and Magnus as I presume this is the update to this patch

https://lore.kernel.org/netdev/Y3OFmgLa56rwVQ4j@boxer/


On 12/5/2022 4:39 AM, Mateusz Palczewski wrote:
> From: "Staszewski, BartoszX" <bartoszx.staszewski@intel.com>

This is incorrect; first name then last name, no 'X'.

> Whenever interface was down, function
> i40e_xdp was passing vsi->rx_buf_len field
> to i40e_xdp_setup() which was equal 0.
> i40e_open() calls i40e_vsi_configure_rx()
> which configures that field, but that only
> happens when interface is up. When it is
> down, i40e_open() is not being called, thus
> vsi->rx_buf_len is not set.
> 
> Solution for this is calculate buffer length
> in newly created function - i40e_calculate_vsi_rx_buf_len()
> that return actual buffer length. Buffer length is
> being calculated based on the same rules applied previously in
> i40e_vsi_configure_rx() function.
> 
> Fixes: 613142b0bb88 ("i40e: Log error for oversized MTU on device")
> Fixes: 0c8493d90b6b ("i40e: add XDP support for pass and drop actions")
> Signed-off-by: "Staszewski, BartoszX" <bartoszx.staszewski@intel.com>
> Signed-off-by: Mateusz Palczewski <mateusz.palczewski@intel.com>
> ---
>   v2: Fixed commit title and message, this patch is only for a case of
>       fresh start so I believe there is no need for rx_buf_len clearance
> ---
>   drivers/net/ethernet/intel/i40e/i40e_main.c | 42 +++++++++++++++------
>   1 file changed, 30 insertions(+), 12 deletions(-)
> 
> diff --git a/drivers/net/ethernet/intel/i40e/i40e_main.c b/drivers/net/ethernet/intel/i40e/i40e_main.c
> index b5dcd15ced36..2fec0cff282c 100644
> --- a/drivers/net/ethernet/intel/i40e/i40e_main.c
> +++ b/drivers/net/ethernet/intel/i40e/i40e_main.c
> @@ -3693,6 +3693,30 @@ static int i40e_vsi_configure_tx(struct i40e_vsi *vsi)
>   	return err;
>   }
>   
> +/**
> + * i40e_calculate_vsi_rx_buf_len - Calculates buffer length
> + *
> + * @vsi: VSI to calculate rx_buf_len from
> + */
> +static u16 i40e_calculate_vsi_rx_buf_len(struct i40e_vsi *vsi)
> +{
> +	u16 ret;
> +
> +	if (!vsi->netdev || (vsi->back->flags & I40E_FLAG_LEGACY_RX)) {
> +		ret = I40E_RXBUFFER_2048;
> +#if (PAGE_SIZE < 8192)
> +	} else if (!I40E_2K_TOO_SMALL_WITH_PADDING &&
> +		   (vsi->netdev->mtu <= ETH_DATA_LEN)) {
> +		ret = I40E_RXBUFFER_1536 - NET_IP_ALIGN;
> +#endif
> +	} else {
> +		ret = (PAGE_SIZE < 8192) ? I40E_RXBUFFER_3072 :
> +					   I40E_RXBUFFER_2048;
> +	}
> +
> +	return ret;
> +}
> +
>   /**
>    * i40e_vsi_configure_rx - Configure the VSI for Rx
>    * @vsi: the VSI being configured
> @@ -3704,20 +3728,14 @@ static int i40e_vsi_configure_rx(struct i40e_vsi *vsi)
>   	int err = 0;
>   	u16 i;
>   
> -	if (!vsi->netdev || (vsi->back->flags & I40E_FLAG_LEGACY_RX)) {
> -		vsi->max_frame = I40E_MAX_RXBUFFER;
> -		vsi->rx_buf_len = I40E_RXBUFFER_2048;
> +	vsi->max_frame = I40E_MAX_RXBUFFER;
> +	vsi->rx_buf_len = i40e_calculate_vsi_rx_buf_len(vsi);
> +
>   #if (PAGE_SIZE < 8192)
> -	} else if (!I40E_2K_TOO_SMALL_WITH_PADDING &&
> -		   (vsi->netdev->mtu <= ETH_DATA_LEN)) {
> +	if (vsi->netdev && !I40E_2K_TOO_SMALL_WITH_PADDING &&
> +	    vsi->netdev->mtu <= ETH_DATA_LEN)
>   		vsi->max_frame = I40E_RXBUFFER_1536 - NET_IP_ALIGN;
> -		vsi->rx_buf_len = I40E_RXBUFFER_1536 - NET_IP_ALIGN;
>   #endif
> -	} else {
> -		vsi->max_frame = I40E_MAX_RXBUFFER;
> -		vsi->rx_buf_len = (PAGE_SIZE < 8192) ? I40E_RXBUFFER_3072 :
> -						       I40E_RXBUFFER_2048;
> -	}
>   
>   	/* set up individual rings */
>   	for (i = 0; i < vsi->num_queue_pairs && !err; i++)
> @@ -13265,7 +13283,7 @@ static int i40e_xdp_setup(struct i40e_vsi *vsi, struct bpf_prog *prog,
>   	int i;
>   
>   	/* Don't allow frames that span over multiple buffers */
> -	if (frame_size > vsi->rx_buf_len) {
> +	if (frame_size > i40e_calculate_vsi_rx_buf_len(vsi)) {
>   		NL_SET_ERR_MSG_MOD(extack, "MTU too large to enable XDP");
>   		return -EINVAL;
>   	}
_______________________________________________
Intel-wired-lan mailing list
Intel-wired-lan@osuosl.org
https://lists.osuosl.org/mailman/listinfo/intel-wired-lan

^ permalink raw reply	[flat|nested] 4+ messages in thread
* Re: [Intel-wired-lan] [PATCH net v2] i40e: Fix the inability to attach XDP program on downed interface
  2022-12-05 12:39 [Intel-wired-lan] [PATCH net v2] i40e: Fix the inability to attach XDP program on downed interface Mateusz Palczewski
  2022-12-05 23:09 ` Tony Nguyen
@ 2022-12-06 11:55 ` Maciej Fijalkowski
  2022-12-06 14:37   ` Paul Menzel
  1 sibling, 1 reply; 4+ messages in thread
From: Maciej Fijalkowski @ 2022-12-06 11:55 UTC (permalink / raw)
  To: Mateusz Palczewski; +Cc: intel-wired-lan, Staszewski, BartoszX

On Mon, Dec 05, 2022 at 07:39:32AM -0500, Mateusz Palczewski wrote:
> From: "Staszewski, BartoszX" <bartoszx.staszewski@intel.com>
> 
> Whenever interface was down, function

Can you say explicitly that you were trying to load XDP prog on downed
iface?

> i40e_xdp was passing vsi->rx_buf_len field
> to i40e_xdp_setup() which was equal 0.
> i40e_open() calls i40e_vsi_configure_rx()
> which configures that field, but that only
> happens when interface is up. When it is
> down, i40e_open() is not being called, thus
> vsi->rx_buf_len is not set.

looks odd, can you set your editor to have 72 chars per line in the commit
msg? Also, would be good to include the mention that you were getting "MTU
too large to enable XDP" like you had in v1.

> 
> Solution for this is calculate buffer length
> in newly created function - i40e_calculate_vsi_rx_buf_len()
> that return actual buffer length. Buffer length is
> being calculated based on the same rules applied previously in
> i40e_vsi_configure_rx() function.

Contents of the patch looks ok to me, but still I would improve commit
msg. Would be good if you could take a look at ixgbe and ice if they have
similar issue - eazy commitz.

Reviewed-by: Maciej Fijalkowski <maciej.fijalkowski@intel.com>

> 
> Fixes: 613142b0bb88 ("i40e: Log error for oversized MTU on device")
> Fixes: 0c8493d90b6b ("i40e: add XDP support for pass and drop actions")
> Signed-off-by: "Staszewski, BartoszX" <bartoszx.staszewski@intel.com>
> Signed-off-by: Mateusz Palczewski <mateusz.palczewski@intel.com>
> ---
>  v2: Fixed commit title and message, this patch is only for a case of
>      fresh start so I believe there is no need for rx_buf_len clearance
> ---
>  drivers/net/ethernet/intel/i40e/i40e_main.c | 42 +++++++++++++++------
>  1 file changed, 30 insertions(+), 12 deletions(-)
> 
> diff --git a/drivers/net/ethernet/intel/i40e/i40e_main.c b/drivers/net/ethernet/intel/i40e/i40e_main.c
> index b5dcd15ced36..2fec0cff282c 100644
> --- a/drivers/net/ethernet/intel/i40e/i40e_main.c
> +++ b/drivers/net/ethernet/intel/i40e/i40e_main.c
> @@ -3693,6 +3693,30 @@ static int i40e_vsi_configure_tx(struct i40e_vsi *vsi)
>  	return err;
>  }
>  
> +/**
> + * i40e_calculate_vsi_rx_buf_len - Calculates buffer length
> + *
> + * @vsi: VSI to calculate rx_buf_len from
> + */
> +static u16 i40e_calculate_vsi_rx_buf_len(struct i40e_vsi *vsi)
> +{
> +	u16 ret;
> +
> +	if (!vsi->netdev || (vsi->back->flags & I40E_FLAG_LEGACY_RX)) {
> +		ret = I40E_RXBUFFER_2048;
> +#if (PAGE_SIZE < 8192)
> +	} else if (!I40E_2K_TOO_SMALL_WITH_PADDING &&
> +		   (vsi->netdev->mtu <= ETH_DATA_LEN)) {
> +		ret = I40E_RXBUFFER_1536 - NET_IP_ALIGN;
> +#endif
> +	} else {
> +		ret = (PAGE_SIZE < 8192) ? I40E_RXBUFFER_3072 :
> +					   I40E_RXBUFFER_2048;
> +	}
> +
> +	return ret;
> +}
> +
>  /**
>   * i40e_vsi_configure_rx - Configure the VSI for Rx
>   * @vsi: the VSI being configured
> @@ -3704,20 +3728,14 @@ static int i40e_vsi_configure_rx(struct i40e_vsi *vsi)
>  	int err = 0;
>  	u16 i;
>  
> -	if (!vsi->netdev || (vsi->back->flags & I40E_FLAG_LEGACY_RX)) {
> -		vsi->max_frame = I40E_MAX_RXBUFFER;
> -		vsi->rx_buf_len = I40E_RXBUFFER_2048;
> +	vsi->max_frame = I40E_MAX_RXBUFFER;
> +	vsi->rx_buf_len = i40e_calculate_vsi_rx_buf_len(vsi);
> +
>  #if (PAGE_SIZE < 8192)
> -	} else if (!I40E_2K_TOO_SMALL_WITH_PADDING &&
> -		   (vsi->netdev->mtu <= ETH_DATA_LEN)) {
> +	if (vsi->netdev && !I40E_2K_TOO_SMALL_WITH_PADDING &&
> +	    vsi->netdev->mtu <= ETH_DATA_LEN)
>  		vsi->max_frame = I40E_RXBUFFER_1536 - NET_IP_ALIGN;
> -		vsi->rx_buf_len = I40E_RXBUFFER_1536 - NET_IP_ALIGN;
>  #endif
> -	} else {
> -		vsi->max_frame = I40E_MAX_RXBUFFER;
> -		vsi->rx_buf_len = (PAGE_SIZE < 8192) ? I40E_RXBUFFER_3072 :
> -						       I40E_RXBUFFER_2048;
> -	}
>  
>  	/* set up individual rings */
>  	for (i = 0; i < vsi->num_queue_pairs && !err; i++)
> @@ -13265,7 +13283,7 @@ static int i40e_xdp_setup(struct i40e_vsi *vsi, struct bpf_prog *prog,
>  	int i;
>  
>  	/* Don't allow frames that span over multiple buffers */
> -	if (frame_size > vsi->rx_buf_len) {
> +	if (frame_size > i40e_calculate_vsi_rx_buf_len(vsi)) {
>  		NL_SET_ERR_MSG_MOD(extack, "MTU too large to enable XDP");
>  		return -EINVAL;
>  	}
> -- 
> 2.31.1
> 
> _______________________________________________
> Intel-wired-lan mailing list
> Intel-wired-lan@osuosl.org
> https://lists.osuosl.org/mailman/listinfo/intel-wired-lan
_______________________________________________
Intel-wired-lan mailing list
Intel-wired-lan@osuosl.org
https://lists.osuosl.org/mailman/listinfo/intel-wired-lan

^ permalink raw reply	[flat|nested] 4+ messages in thread
* Re: [Intel-wired-lan] [PATCH net v2] i40e: Fix the inability to attach XDP program on downed interface
  2022-12-06 11:55 ` Maciej Fijalkowski
@ 2022-12-06 14:37   ` Paul Menzel
  0 siblings, 0 replies; 4+ messages in thread
From: Paul Menzel @ 2022-12-06 14:37 UTC (permalink / raw)
  To: Mateusz Palczewski; +Cc: intel-wired-lan, Bartosz Staszewski

Dear Mateusz,


Thank you for the patch.

Am 06.12.22 um 12:55 schrieb Maciej Fijalkowski:
> On Mon, Dec 05, 2022 at 07:39:32AM -0500, Mateusz Palczewski wrote:

[…]

>> Solution for this is calculate buffer length
>> in newly created function - i40e_calculate_vsi_rx_buf_len()
>> that return actual buffer length. Buffer length is
>> being calculated based on the same rules applied previously in
>> i40e_vsi_configure_rx() function.

s/that return/that returns/

> Contents of the patch looks ok to me, but still I would improve commit
> msg.

I agree with Maciej. Please also improve the git commit message 
summary/title:

> Allow to attach XDP programs on downed interface

[…]


Kind regards,

Paul
_______________________________________________
Intel-wired-lan mailing list
Intel-wired-lan@osuosl.org
https://lists.osuosl.org/mailman/listinfo/intel-wired-lan

^ permalink raw reply	[flat|nested] 4+ messages in thread
end of thread, other threads:[~2022-12-06 14:37 UTC | newest]

Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-12-05 12:39 [Intel-wired-lan] [PATCH net v2] i40e: Fix the inability to attach XDP program on downed interface Mateusz Palczewski
2022-12-05 23:09 ` Tony Nguyen
2022-12-06 11:55 ` Maciej Fijalkowski
2022-12-06 14:37   ` Paul Menzel

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.