[PATCH net 0/2][pull request] Intel Wired LAN Driver Updates 2022-11-14 (i40e)

[PATCH net 0/2][pull request] Intel Wired LAN Driver Updates 2022-11-14 (i40e)

[Tony Nguyen]

This series contains updates to i40e driver only.

Sylwester removes attempted allocation of Rx buffers when AF_XDP is in Tx
only mode.

Bartosz adds helper to calculate Rx buffer length so that it can be
used when interface is down; before value has been set in struct.

The following are changes since commit ed1fe1bebe18884b11e5536b5ac42e3a48960835:
  net: dsa: make dsa_master_ioctl() see through port_hwtstamp_get() shims
and are available in the git repository at:
  git://git.kernel.org/pub/scm/linux/kernel/git/tnguy/net-queue 40GbE

Bartosz Staszewski (1):
  i40e: fix xdp_redirect logs error message when testing with MTU=1500

Sylwester Dziedziuch (1):
  i40e: Fix failure message when XDP is configured in TX only mode

 drivers/net/ethernet/intel/i40e/i40e_main.c | 48 +++++++++++++++------
 1 file changed, 34 insertions(+), 14 deletions(-)

-- 
2.35.1

[PATCH net 1/2] i40e: Fix failure message when XDP is configured in TX only mode

[Tony Nguyen]

From: Sylwester Dziedziuch <sylwesterx.dziedziuch@intel.com>

When starting xdpsock program in TX only mode:

samples/bpf/xdpsock -i <interface> -t

there was an error on i40e driver:

Failed to allocate some buffers on AF_XDP ZC enabled Rx ring 0 (pf_q 81)

It was caused by trying to allocate RX buffers even though
no RX buffers are available because we run in TX only mode.

Fix this by checking for number of available buffers
for RX queue when allocating buffers during XDP setup.

Fixes: 0a714186d3c0 ("i40e: add AF_XDP zero-copy Rx support")
Signed-off-by: Sylwester Dziedziuch <sylwesterx.dziedziuch@intel.com>
Signed-off-by: Mateusz Palczewski <mateusz.palczewski@intel.com>
Tested-by: Shwetha Nagaraju <Shwetha.nagaraju@intel.com>
Signed-off-by: Tony Nguyen <anthony.l.nguyen@intel.com>
---
 drivers/net/ethernet/intel/i40e/i40e_main.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/drivers/net/ethernet/intel/i40e/i40e_main.c b/drivers/net/ethernet/intel/i40e/i40e_main.c
index b5dcd15ced36..41112f92f9ef 100644
--- a/drivers/net/ethernet/intel/i40e/i40e_main.c
+++ b/drivers/net/ethernet/intel/i40e/i40e_main.c
@@ -3555,7 +3555,7 @@ static int i40e_configure_rx_ring(struct i40e_ring *ring)
 	struct i40e_hw *hw = &vsi->back->hw;
 	struct i40e_hmc_obj_rxq rx_ctx;
 	i40e_status err = 0;
-	bool ok;
+	bool ok = true;
 	int ret;
 
 	bitmap_zero(ring->state, __I40E_RING_STATE_NBITS);
@@ -3653,7 +3653,9 @@ static int i40e_configure_rx_ring(struct i40e_ring *ring)
 
 	if (ring->xsk_pool) {
 		xsk_pool_set_rxq_info(ring->xsk_pool, &ring->xdp_rxq);
-		ok = i40e_alloc_rx_buffers_zc(ring, I40E_DESC_UNUSED(ring));
+		if (ring->xsk_pool->free_list_cnt)
+			ok = i40e_alloc_rx_buffers_zc(ring,
+						      I40E_DESC_UNUSED(ring));
 	} else {
 		ok = !i40e_alloc_rx_buffers(ring, I40E_DESC_UNUSED(ring));
 	}
-- 
2.35.1

[PATCH net 2/2] i40e: fix xdp_redirect logs error message when testing with MTU=1500

[Tony Nguyen]

From: Bartosz Staszewski <bartoszx.staszewski@intel.com>

The driver is currently logging an error message "MTU too large to enable XDP"
when trying to enable XDP on totally normal MTU.

This was caused by whenever the interface was down, function
i40e_xdp was passing vsi->rx_buf_len field to i40e_xdp_setup()
which was equal 0. i40e_open() then  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
which causes the bug 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")
Signed-off-by: Bartosz Staszewski <bartoszx.staszewski@intel.com>
Signed-off-by: Mateusz Palczewski <mateusz.palczewski@intel.com>
Tested-by: Shwetha Nagaraju <Shwetha.nagaraju@intel.com>
Signed-off-by: Tony Nguyen <anthony.l.nguyen@intel.com>
---
 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 41112f92f9ef..4b3b6e5b612d 100644
--- a/drivers/net/ethernet/intel/i40e/i40e_main.c
+++ b/drivers/net/ethernet/intel/i40e/i40e_main.c
@@ -3695,6 +3695,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
@@ -3706,20 +3730,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++)
@@ -13267,7 +13285,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.35.1

Re: [PATCH net 1/2] i40e: Fix failure message when XDP is configured in TX only mode

[Maciej Fijalkowski]

On Mon, Nov 14, 2022 at 04:03:23PM -0800, Tony Nguyen wrote:
> From: Sylwester Dziedziuch <sylwesterx.dziedziuch@intel.com>
> 
> When starting xdpsock program in TX only mode:
> 
> samples/bpf/xdpsock -i <interface> -t
> 
> there was an error on i40e driver:
> 
> Failed to allocate some buffers on AF_XDP ZC enabled Rx ring 0 (pf_q 81)
> 
> It was caused by trying to allocate RX buffers even though
> no RX buffers are available because we run in TX only mode.
> 
> Fix this by checking for number of available buffers
> for RX queue when allocating buffers during XDP setup.

I was not sure if we want to proceed with this or not. For sure it's not a
fix to me, behavior was not broken, txonly mode was working correctly.
We're only getting rid of the bogus message that caused confusion within
people.

I feel that if we want that in then we should route this via -next and
address other drivers as well. Not sure what are Magnus' thoughts on this.

> 
> Fixes: 0a714186d3c0 ("i40e: add AF_XDP zero-copy Rx support")
> Signed-off-by: Sylwester Dziedziuch <sylwesterx.dziedziuch@intel.com>
> Signed-off-by: Mateusz Palczewski <mateusz.palczewski@intel.com>
> Tested-by: Shwetha Nagaraju <Shwetha.nagaraju@intel.com>
> Signed-off-by: Tony Nguyen <anthony.l.nguyen@intel.com>
> ---
>  drivers/net/ethernet/intel/i40e/i40e_main.c | 6 ++++--
>  1 file changed, 4 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/net/ethernet/intel/i40e/i40e_main.c b/drivers/net/ethernet/intel/i40e/i40e_main.c
> index b5dcd15ced36..41112f92f9ef 100644
> --- a/drivers/net/ethernet/intel/i40e/i40e_main.c
> +++ b/drivers/net/ethernet/intel/i40e/i40e_main.c
> @@ -3555,7 +3555,7 @@ static int i40e_configure_rx_ring(struct i40e_ring *ring)
>  	struct i40e_hw *hw = &vsi->back->hw;
>  	struct i40e_hmc_obj_rxq rx_ctx;
>  	i40e_status err = 0;
> -	bool ok;
> +	bool ok = true;
>  	int ret;
>  
>  	bitmap_zero(ring->state, __I40E_RING_STATE_NBITS);
> @@ -3653,7 +3653,9 @@ static int i40e_configure_rx_ring(struct i40e_ring *ring)
>  
>  	if (ring->xsk_pool) {
>  		xsk_pool_set_rxq_info(ring->xsk_pool, &ring->xdp_rxq);
> -		ok = i40e_alloc_rx_buffers_zc(ring, I40E_DESC_UNUSED(ring));
> +		if (ring->xsk_pool->free_list_cnt)
> +			ok = i40e_alloc_rx_buffers_zc(ring,
> +						      I40E_DESC_UNUSED(ring));
>  	} else {
>  		ok = !i40e_alloc_rx_buffers(ring, I40E_DESC_UNUSED(ring));
>  	}
> -- 
> 2.35.1
> 

Re: [PATCH net 2/2] i40e: fix xdp_redirect logs error message when testing with MTU=1500

[Maciej Fijalkowski]

On Mon, Nov 14, 2022 at 04:03:24PM -0800, Tony Nguyen wrote:
> From: Bartosz Staszewski <bartoszx.staszewski@intel.com>
> 
> The driver is currently logging an error message "MTU too large to enable XDP"
> when trying to enable XDP on totally normal MTU.

Could you rephrase this to "Fix the inability to attach XDP program on
downed interface" ?

> 
> This was caused by whenever the interface was down, function
> i40e_xdp was passing vsi->rx_buf_len field to i40e_xdp_setup()
> which was equal 0. i40e_open() then  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
> which causes the bug is not set.

Where rx_buf_len is cleared though? Or is it only the case for a fresh
start?

> 
> 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")

I think the problem dates back to 2017 and:
Fixes: 0c8493d90b6b ("i40e: add XDP support for pass and drop actions")

CC: Bjorn Topel <bjorn@kernel.org>

So i'm saying let's have two fixes tags here.

> Signed-off-by: Bartosz Staszewski <bartoszx.staszewski@intel.com>
> Signed-off-by: Mateusz Palczewski <mateusz.palczewski@intel.com>
> Tested-by: Shwetha Nagaraju <Shwetha.nagaraju@intel.com>
> Signed-off-by: Tony Nguyen <anthony.l.nguyen@intel.com>
> ---
>  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 41112f92f9ef..4b3b6e5b612d 100644
> --- a/drivers/net/ethernet/intel/i40e/i40e_main.c
> +++ b/drivers/net/ethernet/intel/i40e/i40e_main.c
> @@ -3695,6 +3695,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
> @@ -3706,20 +3730,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++)
> @@ -13267,7 +13285,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.35.1
> 

Re: [PATCH net 1/2] i40e: Fix failure message when XDP is configured in TX only mode

[Saeed Mahameed]

On 15 Nov 13:13, Maciej Fijalkowski wrote:
>On Mon, Nov 14, 2022 at 04:03:23PM -0800, Tony Nguyen wrote:
>> From: Sylwester Dziedziuch <sylwesterx.dziedziuch@intel.com>
>>
>> When starting xdpsock program in TX only mode:
>>
>> samples/bpf/xdpsock -i <interface> -t
>>
>> there was an error on i40e driver:
>>
>> Failed to allocate some buffers on AF_XDP ZC enabled Rx ring 0 (pf_q 81)
>>
>> It was caused by trying to allocate RX buffers even though
>> no RX buffers are available because we run in TX only mode.
>>
>> Fix this by checking for number of available buffers
>> for RX queue when allocating buffers during XDP setup.
>
>I was not sure if we want to proceed with this or not. For sure it's not a
>fix to me, behavior was not broken, txonly mode was working correctly.
>We're only getting rid of the bogus message that caused confusion within
>people.
>
>I feel that if we want that in then we should route this via -next and
>address other drivers as well. Not sure what are Magnus' thoughts on this.
>
+1

Some other driver might not have this print message issue, but it would be
nice if the driver got some indication of the TX only nature so maybe we can
cut some corners on napi and avoid even attempting to allocate the rx zc
buffers.

Re: [PATCH net 0/2][pull request] Intel Wired LAN Driver Updates 2022-11-14 (i40e)

[Vladimir Oltean]

Hi,

On Mon, Nov 14, 2022 at 04:03:22PM -0800, Tony Nguyen wrote:
> This series contains updates to i40e driver only.
> 
> Sylwester removes attempted allocation of Rx buffers when AF_XDP is in Tx
> only mode.
> 
> Bartosz adds helper to calculate Rx buffer length so that it can be
> used when interface is down; before value has been set in struct.
> 
> The following are changes since commit ed1fe1bebe18884b11e5536b5ac42e3a48960835:
>   net: dsa: make dsa_master_ioctl() see through port_hwtstamp_get() shims
> and are available in the git repository at:
>   git://git.kernel.org/pub/scm/linux/kernel/git/tnguy/net-queue 40GbE
> 
> Bartosz Staszewski (1):
>   i40e: fix xdp_redirect logs error message when testing with MTU=1500
> 
> Sylwester Dziedziuch (1):
>   i40e: Fix failure message when XDP is configured in TX only mode
> 
>  drivers/net/ethernet/intel/i40e/i40e_main.c | 48 +++++++++++++++------
>  1 file changed, 34 insertions(+), 14 deletions(-)
> 
> -- 
> 2.35.1
> 

Sorry to interject, but there might be a potential bug I noticed a while
ago in the i40e driver, and I didn't find the occasion to bring it up,
but remembered just now.

Is it correct for i40e_run_xdp_zc() to call i40e_xmit_xdp_tx_ring() on
the XDP_TX action? If I'm reading the code correctly, this will map the
buffer to DMA before sending it. But since this is a zero-copy RX buffer,
it has already been mapped to DMA in i40e_xsk_pool_enable().

Since I don't have the hardware, I can't be 100% sure if I'm following
the code correctly. However if I compare with i40e_xmit_zc(), the latter
does not map buffers to DMA, so I think neither should the former.

Re: [PATCH net 0/2][pull request] Intel Wired LAN Driver Updates 2022-11-14 (i40e)

[Maciej Fijalkowski]

On Thu, Nov 17, 2022 at 01:21:21AM +0200, Vladimir Oltean wrote:
> Hi,
> 
> On Mon, Nov 14, 2022 at 04:03:22PM -0800, Tony Nguyen wrote:
> > This series contains updates to i40e driver only.
> > 
> > Sylwester removes attempted allocation of Rx buffers when AF_XDP is in Tx
> > only mode.
> > 
> > Bartosz adds helper to calculate Rx buffer length so that it can be
> > used when interface is down; before value has been set in struct.
> > 
> > The following are changes since commit ed1fe1bebe18884b11e5536b5ac42e3a48960835:
> >   net: dsa: make dsa_master_ioctl() see through port_hwtstamp_get() shims
> > and are available in the git repository at:
> >   git://git.kernel.org/pub/scm/linux/kernel/git/tnguy/net-queue 40GbE
> > 
> > Bartosz Staszewski (1):
> >   i40e: fix xdp_redirect logs error message when testing with MTU=1500
> > 
> > Sylwester Dziedziuch (1):
> >   i40e: Fix failure message when XDP is configured in TX only mode
> > 
> >  drivers/net/ethernet/intel/i40e/i40e_main.c | 48 +++++++++++++++------
> >  1 file changed, 34 insertions(+), 14 deletions(-)
> > 
> > -- 
> > 2.35.1
> > 
> 
> Sorry to interject, but there might be a potential bug I noticed a while
> ago in the i40e driver, and I didn't find the occasion to bring it up,
> but remembered just now.
> 
> Is it correct for i40e_run_xdp_zc() to call i40e_xmit_xdp_tx_ring() on
> the XDP_TX action? If I'm reading the code correctly, this will map the
> buffer to DMA before sending it. But since this is a zero-copy RX buffer,
> it has already been mapped to DMA in i40e_xsk_pool_enable().

Hey Vladimir,

have a look at xdp_convert_zc_to_xdp_frame() in net/core/xdp.c. For XDP_TX
on ZC Rx side we basically create new xdp_frame backed by new page and
copy the contents we had in ZC buffer. Then we give back the ZC buffer to
XSK buff pool and new xdp_frame has to be DMA mapped to HW.

> 
> Since I don't have the hardware, I can't be 100% sure if I'm following
> the code correctly. However if I compare with i40e_xmit_zc(), the latter
> does not map buffers to DMA, so I think neither should the former.

Re: [PATCH net 0/2][pull request] Intel Wired LAN Driver Updates 2022-11-14 (i40e)

[Vladimir Oltean]

Hi Maciej,

On Thu, Nov 17, 2022 at 01:03:04AM +0100, Maciej Fijalkowski wrote:
> Hey Vladimir,
> 
> have a look at xdp_convert_zc_to_xdp_frame() in net/core/xdp.c. For XDP_TX
> on ZC Rx side we basically create new xdp_frame backed by new page and
> copy the contents we had in ZC buffer. Then we give back the ZC buffer to
> XSK buff pool and new xdp_frame has to be DMA mapped to HW.

Ah, ok, I didn't notice the xdp_convert_zc_to_xdp_frame() call inside
xdp_convert_buff_to_frame(), it's quite well hidden...

So it's clear now from a correctness point of view, thanks for clarifying.
This could spark a separate discussion about whether there is any better
alternative to copying the RX buffer for XDP_TX and re-mapping to DMA
something that was already mapped. But I'm not interested in that, since
I believe who wrote the code probably thought about the high costs too.
Anyway, I believe that in the general case (meaning from the perspective
of the XSK API) it's perfectly fine to keep the RX buffer around for a
while, nobody forces you to copy the frame out of it for XDP_TX.

Re: [PATCH net 0/2][pull request] Intel Wired LAN Driver Updates 2022-11-14 (i40e)

[Maciej Fijalkowski]

On Thu, Nov 17, 2022 at 02:24:33AM +0200, Vladimir Oltean wrote:
> Hi Maciej,
> 
> On Thu, Nov 17, 2022 at 01:03:04AM +0100, Maciej Fijalkowski wrote:
> > Hey Vladimir,
> > 
> > have a look at xdp_convert_zc_to_xdp_frame() in net/core/xdp.c. For XDP_TX
> > on ZC Rx side we basically create new xdp_frame backed by new page and
> > copy the contents we had in ZC buffer. Then we give back the ZC buffer to
> > XSK buff pool and new xdp_frame has to be DMA mapped to HW.
> 
> Ah, ok, I didn't notice the xdp_convert_zc_to_xdp_frame() call inside
> xdp_convert_buff_to_frame(), it's quite well hidden...
> 
> So it's clear now from a correctness point of view, thanks for clarifying.
> This could spark a separate discussion about whether there is any better
> alternative to copying the RX buffer for XDP_TX and re-mapping to DMA
> something that was already mapped. But I'm not interested in that, since
> I believe who wrote the code probably thought about the high costs too.
> Anyway, I believe that in the general case (meaning from the perspective
> of the XSK API) it's perfectly fine to keep the RX buffer around for a
> while, nobody forces you to copy the frame out of it for XDP_TX.

I sort of agree but I will get back to you after getting some sleep.
Basically I am observing better perf when I decide not to convert buff to
frame for XDP_TX (in this case I'm referring to standard data path of
Intel drivers, not the ZC data path). For ZC I am thinking about
converting ZC Rx buff to xdp_buff, but maybe we need to revisit the idea
behind that copy altogether. It was developed way before the times when
XSK buffer pool got introduced.

Re: [PATCH net 1/2] i40e: Fix failure message when XDP is configured in TX only mode

[Magnus Karlsson]

On Wed, Nov 16, 2022 at 11:30 PM Saeed Mahameed <saeed@kernel.org> wrote:
>
> On 15 Nov 13:13, Maciej Fijalkowski wrote:
> >On Mon, Nov 14, 2022 at 04:03:23PM -0800, Tony Nguyen wrote:
> >> From: Sylwester Dziedziuch <sylwesterx.dziedziuch@intel.com>
> >>
> >> When starting xdpsock program in TX only mode:
> >>
> >> samples/bpf/xdpsock -i <interface> -t
> >>
> >> there was an error on i40e driver:
> >>
> >> Failed to allocate some buffers on AF_XDP ZC enabled Rx ring 0 (pf_q 81)
> >>
> >> It was caused by trying to allocate RX buffers even though
> >> no RX buffers are available because we run in TX only mode.
> >>
> >> Fix this by checking for number of available buffers
> >> for RX queue when allocating buffers during XDP setup.
> >
> >I was not sure if we want to proceed with this or not. For sure it's not a
> >fix to me, behavior was not broken, txonly mode was working correctly.
> >We're only getting rid of the bogus message that caused confusion within
> >people.
> >
> >I feel that if we want that in then we should route this via -next and
> >address other drivers as well. Not sure what are Magnus' thoughts on this.
> >
> +1
>
> Some other driver might not have this print message issue, but it would be
> nice if the driver got some indication of the TX only nature so maybe we can
> cut some corners on napi and avoid even attempting to allocate the rx zc
> buffers.

This would save some resources for sure. We could add a field to the
XDP_SETUP_XSK_POOL command struct that is passed to the driver to
indicate if the socket has an Rx component and if it has a Tx
component. Just do not know how common it is to have a Tx only xsk
socket. Packet generators come to my mind, but what more?