[Intel-wired-lan][PATCH iwl-net] idpf: set completion tag for "empty" bufs associated with a packet

[Intel-wired-lan][PATCH iwl-net] idpf: set completion tag for "empty" bufs associated with a packet

[Joshua Hay]

Commit d9028db618a6 ("idpf: convert to libeth Tx buffer completion")
inadvertently removed code that was necessary for the tx buffer cleaning
routine to iterate over all buffers associated with a packet.

When a frag is too large for a single data descriptor, it will be split
across multiple data descriptors. This means the frag will span multiple
buffers in the buffer ring in order to keep the descriptor and buffer
ring indexes aligned. The buffer entries in the ring are technically
empty and no cleaning actions need to be performed. These empty buffers
can precede other frags associated with the same packet. I.e. a single
packet on the buffer ring can look like:

	buf[0]=skb0.frag0
	buf[1]=skb0.frag1
	buf[2]=empty
	buf[3]=skb0.frag2

The cleaning routine iterates through these buffers based on a matching
completion tag. If the completion tag is not set for buf2, the loop will
end prematurely. Frag2 will be left uncleaned and next_to_clean will be
left pointing to the end of packet, which will break the cleaning logic
for subsequent cleans. This consequently leads to tx timeouts.

Assign the empty bufs the same completion tag for the packet to ensure
the cleaning routine iterates over all of the buffers associated with
the packet.

Fixes: d9028db618a6 ("idpf: convert to libeth Tx buffer completion")
Signed-off-by: Joshua Hay <joshua.a.hay@intel.com>
Acked-by: Alexander Lobakin <aleksander.lobakin@intel.com>
Reviewed-by: Madhu chittim <madhu.chittim@intel.com>
---
 drivers/net/ethernet/intel/idpf/idpf_txrx.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/net/ethernet/intel/idpf/idpf_txrx.c b/drivers/net/ethernet/intel/idpf/idpf_txrx.c
index d4e6f0e10487..60d15b3e6e2f 100644
--- a/drivers/net/ethernet/intel/idpf/idpf_txrx.c
+++ b/drivers/net/ethernet/intel/idpf/idpf_txrx.c
@@ -2448,6 +2448,7 @@ static void idpf_tx_splitq_map(struct idpf_tx_queue *tx_q,
 			 * rest of the packet.
 			 */
 			tx_buf->type = LIBETH_SQE_EMPTY;
+			idpf_tx_buf_compl_tag(tx_buf) = params->compl_tag;
 
 			/* Adjust the DMA offset and the remaining size of the
 			 * fragment.  On the first iteration of this loop,
-- 
2.39.2

Re: [Intel-wired-lan][PATCH iwl-net] idpf: set completion tag for "empty" bufs associated with a packet

[Simon Horman]

On Mon, Oct 07, 2024 at 01:24:35PM -0700, Joshua Hay wrote:
> Commit d9028db618a6 ("idpf: convert to libeth Tx buffer completion")
> inadvertently removed code that was necessary for the tx buffer cleaning
> routine to iterate over all buffers associated with a packet.
> 
> When a frag is too large for a single data descriptor, it will be split
> across multiple data descriptors. This means the frag will span multiple
> buffers in the buffer ring in order to keep the descriptor and buffer
> ring indexes aligned. The buffer entries in the ring are technically
> empty and no cleaning actions need to be performed. These empty buffers
> can precede other frags associated with the same packet. I.e. a single
> packet on the buffer ring can look like:
> 
> 	buf[0]=skb0.frag0
> 	buf[1]=skb0.frag1
> 	buf[2]=empty
> 	buf[3]=skb0.frag2
> 
> The cleaning routine iterates through these buffers based on a matching
> completion tag. If the completion tag is not set for buf2, the loop will
> end prematurely. Frag2 will be left uncleaned and next_to_clean will be
> left pointing to the end of packet, which will break the cleaning logic
> for subsequent cleans. This consequently leads to tx timeouts.
> 
> Assign the empty bufs the same completion tag for the packet to ensure
> the cleaning routine iterates over all of the buffers associated with
> the packet.
> 
> Fixes: d9028db618a6 ("idpf: convert to libeth Tx buffer completion")
> Signed-off-by: Joshua Hay <joshua.a.hay@intel.com>
> Acked-by: Alexander Lobakin <aleksander.lobakin@intel.com>
> Reviewed-by: Madhu chittim <madhu.chittim@intel.com>

Thanks for the detailed description.

Reviewed-by: Simon Horman <horms@kernel.org>

RE: [Intel-wired-lan] [PATCH iwl-net] idpf: set completion tag for "empty" bufs associated with a packet

[Singh, Krishneil K]

> -----Original Message-----
> From: Intel-wired-lan <intel-wired-lan-bounces@osuosl.org> On Behalf Of
> Simon Horman
> Sent: Thursday, October 17, 2024 2:04 AM
> To: Hay, Joshua A <joshua.a.hay@intel.com>
> Cc: intel-wired-lan@lists.osuosl.org; Lobakin, Aleksander
> <aleksander.lobakin@intel.com>; Chittim, Madhu
> <madhu.chittim@intel.com>; netdev@vger.kernel.org
> Subject: Re: [Intel-wired-lan] [PATCH iwl-net] idpf: set completion tag for
> "empty" bufs associated with a packet
> 
> On Mon, Oct 07, 2024 at 01:24:35PM -0700, Joshua Hay wrote:
> > Commit d9028db618a6 ("idpf: convert to libeth Tx buffer completion")
> > inadvertently removed code that was necessary for the tx buffer cleaning
> > routine to iterate over all buffers associated with a packet.
> >
> > When a frag is too large for a single data descriptor, it will be split
> > across multiple data descriptors. This means the frag will span multiple
> > buffers in the buffer ring in order to keep the descriptor and buffer
> > ring indexes aligned. The buffer entries in the ring are technically
> > empty and no cleaning actions need to be performed. These empty buffers
> > can precede other frags associated with the same packet. I.e. a single
> > packet on the buffer ring can look like:
> >
> > 	buf[0]=skb0.frag0
> > 	buf[1]=skb0.frag1
> > 	buf[2]=empty
> > 	buf[3]=skb0.frag2
> >
> > The cleaning routine iterates through these buffers based on a matching
> > completion tag. If the completion tag is not set for buf2, the loop will
> > end prematurely. Frag2 will be left uncleaned and next_to_clean will be
> > left pointing to the end of packet, which will break the cleaning logic
> > for subsequent cleans. This consequently leads to tx timeouts.
> >
> > Assign the empty bufs the same completion tag for the packet to ensure
> > the cleaning routine iterates over all of the buffers associated with
> > the packet.
> >
> > Fixes: d9028db618a6 ("idpf: convert to libeth Tx buffer completion")
> > Signed-off-by: Joshua Hay <joshua.a.hay@intel.com>
> > Acked-by: Alexander Lobakin <aleksander.lobakin@intel.com>
> > Reviewed-by: Madhu chittim <madhu.chittim@intel.com>
> 
> Thanks for the detailed description.
> 
> Reviewed-by: Simon Horman <horms@kernel.org>

Tested-by: Krishneil Singh <krishneil.k.singh@intel.com>