Date:   Wed, 5 Oct 2022 11:33:23 -0700
From:   Jesse Brandeburg <jesse.brandeburg@...el.com>
To:     Joe Damato <jdamato@...tly.com>,
        Maciej Fijalkowski <maciej.fijalkowski@...el.com>
CC:     <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 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.

Also, mixing new features and introducing refactors makes it very hard 
to unwind if something goes wrong in the future.

Thanks,
Jesse

Powered by blists - more mailing lists