Re: [PATCH net-next v3 03/12] iavf: optimize Rx buffer allocation a bunch

[Alexander Lobakin <aleksander.lobakin@intel.com>]

From: Maciej Fijalkowski <maciej.fijalkowski@intel.com>
Date: Wed, 31 May 2023 18:39:44 +0200

> On Tue, May 30, 2023 at 05:00:26PM +0200, Alexander Lobakin wrote:
>> The Rx hotpath code of IAVF is not well-optimized TBH. Before doing any
>> further buffer model changes, shake it up a bit. Notably:
>>
>> 1. Cache more variables on the stack.
>>    DMA device, Rx page size, NTC -- these are the most common things
>>    used all throughout the hotpath, often in loops on each iteration.
>>    Instead of fetching (or even calculating, as with the page size) them
>>    from the ring all the time, cache them on the stack at the beginning
>>    of the NAPI polling callback. NTC will be written back at the end,
>>    the rest are used read-only, so no sync needed.
> 
> I like calculating page size once per napi istance. Reduces a bunch of
> branches ;)
> 
> Yet another optimization I did on other drivers was to store rx_offset
> within ring struct. I skipped iavf for some reason. I can follow-up with
> that, but I'm bringing this up so we keep an eye on it.

rx_offset is stored as Page Pool param in its struct. So no follow-ups
here needed :)

[...]

>> 3. Don't allocate with %GPF_ATOMIC on ifup.
> 
> s/GPF/GFP

Breh :s

> 
>>    This involved introducing the @gfp parameter to a couple functions.
>>    Doesn't change anything for Rx -> softirq.
>> 4. 1 budget unit == 1 descriptor, not skb.
>>    There could be underflow when receiving a lot of fragmented frames.
>>    If each of them would consist of 2 frags, it means that we'd process
>>    64 descriptors at the point where we pass the 32th skb to the stack.
>>    But the driver would count that only as a half, which could make NAPI
>>    re-enable interrupts prematurely and create unnecessary CPU load.
> 
> How would this affect 9k MTU workloads?

Not measured =\ But I feel like I'll drop this bullet, so will see.

> 
>> 5. Shortcut !size case.
>>    It's super rare, but possible -- for example, if the last buffer of
>>    the fragmented frame contained only FCS, which was then stripped by
>>    the HW. Instead of checking for size several times when processing,
>>    quickly reuse the buffer and jump to the skb fields part.
> 
> would be good to say about pagecnt_bias handling.

?? Bias is changed only when the buffer contains data, in this case it's
not changed, so the buffer is ready to be reused.

[...]

>> Function: add/remove: 4/2 grow/shrink: 0/5 up/down: 473/-647 (-174)
>>
>> + up to 2% performance.
> 
> I am sort of not buying that. You are removing iavf_reuse_rx_page() here
> which is responsible for reusing the page, but on next patch that is
> supposed to avoid page split perf drops by 30%. A bit confusing?

Nope. reuse_rx_page() only adds overhead since it moves reusable buffers
around the ring, while without it they get reused in-place. That's why
it doesn't cause any regressions. The next patch removes page reuse
completely, hence the perf changes.

[...]

>> -static void iavf_reuse_rx_page(struct iavf_ring *rx_ring,
>> -			       struct iavf_rx_buffer *old_buff)
> 
> this is recycling logic so i feel this removal belongs to patch 04, right?

(above)

> 
>> -{
>> -	struct iavf_rx_buffer *new_buff;
>> -	u16 nta = rx_ring->next_to_alloc;


[...]

>> -static struct iavf_rx_buffer *iavf_get_rx_buffer(struct iavf_ring *rx_ring,
>> -						 const unsigned int size)
>> +static void iavf_sync_rx_buffer(struct device *dev, struct iavf_rx_buffer *buf,
>> +				u32 size)
> 
> you have peeled out all of the contents of this function, why not calling
> dma_sync_single_range_for_cpu() directly?

Pretty long line, so I decided to leave it here. It gets removed anyway
when Page Pool is here.

[...]

>>  	if (iavf_can_reuse_rx_page(rx_buffer)) {
>> -		/* hand second half of page back to the ring */
>> -		iavf_reuse_rx_page(rx_ring, rx_buffer);
>>  		rx_ring->rx_stats.page_reuse_count++;
> 
> what is the purpose of not reusing the page but bumping the meaningless
> stat? ;)

Also above. It's reused, just not moved around the ring :D

[...]

>> +		/* Very rare, but possible case. The most common reason:
>> +		 * the last fragment contained FCS only, which was then

                            ^^^^^^^^

>> +		 * stripped by the HW.
> 
> you could also mention this is happening for fragmented frames

Mmm?

> 
>> +		 */
>> +		if (unlikely(!size))
>> +			goto skip_data;
Thanks,
Olek