Date:   Wed, 5 Oct 2022 11:47:28 -0700
From:   Joe Damato <jdamato@...tly.com>
To:     Jesse Brandeburg <jesse.brandeburg@...el.com>
Cc:     Maciej Fijalkowski <maciej.fijalkowski@...el.com>,
        intel-wired-lan@...ts.osuosl.org, netdev@...r.kernel.org,
        kuba@...nel.org, davem@...emloft.net, anthony.l.nguyen@...el.com
Subject: Re: [next-queue 2/3] i40e: i40e_clean_tx_irq returns work done

On Wed, Oct 05, 2022 at 11:33:23AM -0700, Jesse Brandeburg wrote:
> On 10/5/2022 10:50 AM, Joe Damato wrote:
> >On Wed, Oct 05, 2022 at 12:46:31PM +0200, Maciej Fijalkowski wrote:
> >>On Wed, Oct 05, 2022 at 01:31:42AM -0700, Joe Damato wrote:
> >>>Adjust i40e_clean_tx_irq to return the actual number of packets cleaned
> >>>and adjust the logic in i40e_napi_poll to check this value.
> 
> it's fine to return the number cleaned but let's keep that data and changes
> to itself instead of changing the flow of the routine.
> 
> 
> >>>
> >>>Signed-off-by: Joe Damato <jdamato@...tly.com>
> >>>---
> >>>  drivers/net/ethernet/intel/i40e/i40e_txrx.c | 24 +++++++++++++-----------
> >>>  drivers/net/ethernet/intel/i40e/i40e_xsk.c  | 12 ++++++------
> >>>  drivers/net/ethernet/intel/i40e/i40e_xsk.h  |  2 +-
> >>>  3 files changed, 20 insertions(+), 18 deletions(-)
> >>>
> >>>diff --git a/drivers/net/ethernet/intel/i40e/i40e_txrx.c b/drivers/net/ethernet/intel/i40e/i40e_txrx.c
> >>>index b97c95f..ed88309 100644
> >>>--- a/drivers/net/ethernet/intel/i40e/i40e_txrx.c
> >>>+++ b/drivers/net/ethernet/intel/i40e/i40e_txrx.c
> >>>@@ -924,10 +924,10 @@ void i40e_detect_recover_hung(struct i40e_vsi *vsi)
> >>>   * @tx_ring: Tx ring to clean
> >>>   * @napi_budget: Used to determine if we are in netpoll
> >>>   *
> >>>- * Returns true if there's any budget left (e.g. the clean is finished)
> >>>+ * Returns the number of packets cleaned
> >>>   **/
> >>>-static bool i40e_clean_tx_irq(struct i40e_vsi *vsi,
> >>>-			      struct i40e_ring *tx_ring, int napi_budget)
> >>>+static int i40e_clean_tx_irq(struct i40e_vsi *vsi,
> >>>+			     struct i40e_ring *tx_ring, int napi_budget)
> >>>  {
> >>>  	int i = tx_ring->next_to_clean;
> >>>  	struct i40e_tx_buffer *tx_buf;
> >>>@@ -1026,7 +1026,7 @@ static bool i40e_clean_tx_irq(struct i40e_vsi *vsi,
> >>>  	i40e_arm_wb(tx_ring, vsi, budget);
> >>>  	if (ring_is_xdp(tx_ring))
> >>>-		return !!budget;
> >>>+		return total_packets;
> >>>  	/* notify netdev of completed buffers */
> >>>  	netdev_tx_completed_queue(txring_txq(tx_ring),
> >>>@@ -1048,7 +1048,7 @@ static bool i40e_clean_tx_irq(struct i40e_vsi *vsi,
> >>>  		}
> >>>  	}
> >>>-	return !!budget;
> >>>+	return total_packets;
> >>>  }
> >>>  /**
> >>>@@ -2689,10 +2689,12 @@ int i40e_napi_poll(struct napi_struct *napi, int budget)
> >>>  			       container_of(napi, struct i40e_q_vector, napi);
> >>>  	struct i40e_vsi *vsi = q_vector->vsi;
> >>>  	struct i40e_ring *ring;
> >>>+	bool tx_clean_complete = true;
> >>>  	bool clean_complete = true;
> >>>  	bool arm_wb = false;
> >>>  	int budget_per_ring;
> >>>  	int work_done = 0;
> >>>+	int tx_wd = 0;
> >>>  	if (test_bit(__I40E_VSI_DOWN, vsi->state)) {
> >>>  		napi_complete(napi);
> >>>@@ -2703,12 +2705,12 @@ int i40e_napi_poll(struct napi_struct *napi, int budget)
> >>>  	 * budget and be more aggressive about cleaning up the Tx descriptors.
> >>>  	 */
> >>>  	i40e_for_each_ring(ring, q_vector->tx) {
> >>>-		bool wd = ring->xsk_pool ?
> >>>-			  i40e_clean_xdp_tx_irq(vsi, ring) :
> >>>-			  i40e_clean_tx_irq(vsi, ring, budget);
> >>>+		tx_wd = ring->xsk_pool ?
> >>>+			i40e_clean_xdp_tx_irq(vsi, ring) :
> >>>+			i40e_clean_tx_irq(vsi, ring, budget);
> >>>-		if (!wd) {
> >>>-			clean_complete = false;
> >>>+		if (tx_wd >= budget) {
> >>>+			tx_clean_complete = false;
> >>
> >>This will break for AF_XDP Tx ZC. AF_XDP Tx ZC in intel drivers ignores
> >>budget given by NAPI. If you look at i40e_xmit_zc():
> >>
> >>func def:
> >>static bool i40e_xmit_zc(struct i40e_ring *xdp_ring, unsigned int budget)
> >>
> >>callsite:
> >>	return i40e_xmit_zc(tx_ring, I40E_DESC_UNUSED(tx_ring));
> >>
> >>we give free ring space as a budget and with your change we would be
> >>returning the amount of processed tx descriptors which you will be
> >>comparing against NAPI budget (64, unless you have busy poll enabled with
> >>a different batch size). Say you start with empty ring and your HW rings
> >>are sized to 1k but there was only 512 AF_XDP descriptors ready for Tx.
> >>You produced all of them successfully to ring and you return 512 up to
> >>i40e_napi_poll.
> >
> >Good point, my bad.
> >
> >I've reworked this for the v2 and have given i40e_clean_tx_irq,
> >and i40e_clean_xdp_tx_irq an out parameter which will record the number
> >TXes cleaned.
> >
> >I tweaked i40e_xmit_zc to return the number of packets (nb_pkts) and moved
> >the boolean to check if that's under the "budget"
> >(I40E_DESC_UNUSED(tx_ring)) into i40e_clean_xdp_tx_irq.
> >
> >I think that might solve the issues you've described.
> 
> Please don't change the flow of this function, transmit clean ups are so
> cheap that we don't bother counting them or limiting them beyond a maximum
> (so they don't clean forever)
> 
> Basically transmits should not be counted when exiting NAPI, besides that we
> did "at least one". The only thing that matters to the budget is that we
> "finished" transmit cleanup or not, which would make sure we rescheduled
> napi if we weren't finished cleaning (for instance on a 8160 entry tx ring)
> transmits.
> 
> I'd much rather you kept this series to a simple return count of tx cleaned
> in "out" as you've said you'd do in v2, and then use that data *only* in the
> context of the new trace event.
> 
> That way you're not changing the flow and introducing tough to debug issues
> in the hot path.

In the v2 I've been hacking on I've added out params to i40e_clean_tx_irq and
i40e_clean_xdp_tx_irq, but I avoided adding an out param in i40e_xmit_zc,
since lifting the boolean out seemed pretty straightforward.

I'll drop that though in favor of an out param in i40e_xmit_zc, as well, to
avoid changing the flow of the code.

Thanks for taking a look.

Powered by blists - more mailing lists