Re: [PATCH iwl-net] ice: fix incorrect counter for buffer allocation failures

[Jacob Keller <jacob.e.keller@intel.com>]

[-- Attachment #1.1: Type: text/plain, Size: 3283 bytes --]



On 8/9/2025 6:08 AM, Jason Xing wrote:
> On Sat, Aug 9, 2025 at 5:38 PM Paul Menzel <pmenzel@molgen.mpg.de> wrote:
>>
>> Dear Michal,
>>
>>
>> Thank you for your patch.
>>
>>
>> Am 08.08.25 um 17:53 schrieb Michal Kubiak:
>>> Currently, the driver increments `alloc_page_failed` when buffer allocation fails
>>> in `ice_clean_rx_irq()`. However, this counter is intended for page allocation
>>> failures, not buffer allocation issues.
>>>
>>> This patch corrects the counter by incrementing `alloc_buf_failed` instead,
>>> ensuring accurate statistics reporting for buffer allocation failures.
>>>
>>> Fixes: 2fba7dc5157b ("ice: Add support for XDP multi-buffer on Rx side")
>>> Reported-by: Jacob Keller <jacob.e.keller@intel.com>
>>> Suggested-by: Paul Menzel <pmenzel@molgen.mpg.de>
>>
>> Thank you, but I merely asked to send in the patch separately and didn’t
>> spot the error. So, I’d remove the tag, but you add the one at the end.
>>
>>> Signed-off-by: Michal Kubiak <michal.kubiak@intel.com>
> 
> Reviewed-by: Jason Xing <kerneljasonxing@gmail.com>
> 
>>> ---
>>>   drivers/net/ethernet/intel/ice/ice_txrx.c | 2 +-
>>>   1 file changed, 1 insertion(+), 1 deletion(-)
>>>
>>> diff --git a/drivers/net/ethernet/intel/ice/ice_txrx.c b/drivers/net/ethernet/intel/ice/ice_txrx.c
>>> index 93907ab2eac7..1b1ebfd347ef 100644
>>> --- a/drivers/net/ethernet/intel/ice/ice_txrx.c
>>> +++ b/drivers/net/ethernet/intel/ice/ice_txrx.c
>>> @@ -1337,7 +1337,7 @@ static int ice_clean_rx_irq(struct ice_rx_ring *rx_ring, int budget)
>>>                       skb = ice_construct_skb(rx_ring, xdp);
>>>               /* exit if we failed to retrieve a buffer */
>>>               if (!skb) {
>>> -                     rx_ring->ring_stats->rx_stats.alloc_page_failed++;
>>> +                     rx_ring->ring_stats->rx_stats.alloc_buf_failed++;
>>>                       xdp_verdict = ICE_XDP_CONSUMED;
>>>               }
>>>               ice_put_rx_mbuf(rx_ring, xdp, ntc, xdp_verdict);
>>
>> Reviewed-by: Paul Menzel <pmenzel@molgen.mpg.de>
>>
>>
>> Kind regards,
>>
>> Paul
>>
>>
>> PS: A little off-topic: As this code is present since v6.3-rc1, I
>> wonder, why this has not been causing any user visible issues in the
>> last two years. Can somebody explain this?
>>
> 
> From my limited experience, upgrading to the new kernel (like v6.x) is
> not an easy thing. Plus not that many people monitor the driver
> counter on the machine with the ice driver loaded. Sometimes we
> neglect this error because it doesn't harm the real and overall
> workload even when the allocation fails. Things like this sometimes
> happen in other areas :)
> 

Exactly this. An end user is unlikely to know the difference between the
buffer allocated vs page allocated failures. Even if they see the
counters going up, how would they know that the counter going up was a bug?

This counter really is just for user visibility into a problem
occurring. Reporting the right counter is critical from a developer
perspective as we might use the fact that one but not the other is going
up in an attempt to isolate issues.. but the end user is unlikely to
ever notice without code inspection.

> Thanks,
> Jason
> 


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 236 bytes --]