Re: [PATCH 1/3] drm/xe/gt_stats: Detect and log overflow

[Matthew Auld <matthew.auld@intel.com>]

On 25/02/2025 17:34, Francois Dugast wrote:
> On Tue, Feb 25, 2025 at 09:41:15AM -0600, Lucas De Marchi wrote:
>> On Tue, Feb 25, 2025 at 02:17:04PM +0100, Francois Dugast wrote:
>>> Check if the atomic counter limit has been reached and if that is the
>>> case, set the counter value to the new increment to prevent keeping
>>> a garbage value, and notify it in the kernel log as below:
>>>
>>>     [drm:xe_gt_stats_incr [xe]] GT0: stats vma_pagefault_bytes overflow, resetting
>>>
>>> Signed-off-by: Francois Dugast <francois.dugast@intel.com>
>>> ---
>>> drivers/gpu/drm/xe/xe_gt_stats.c | 18 ++++++++++++------
>>> 1 file changed, 12 insertions(+), 6 deletions(-)
>>>
>>> diff --git a/drivers/gpu/drm/xe/xe_gt_stats.c b/drivers/gpu/drm/xe/xe_gt_stats.c
>>> index 2e9879ea4674..753c081b68a6 100644
>>> --- a/drivers/gpu/drm/xe/xe_gt_stats.c
>>> +++ b/drivers/gpu/drm/xe/xe_gt_stats.c
>>> @@ -8,8 +8,15 @@
>>> #include <drm/drm_print.h>
>>>
>>> #include "xe_gt.h"
>>> +#include "xe_gt_printk.h"
>>> #include "xe_gt_stats.h"
>>>
>>> +static const char *const stat_description[__XE_GT_STATS_NUM_IDS] = {
>>> +	"tlb_inval_count",
>>> +	"vma_pagefault_count",
>>> +	"vma_pagefault_bytes",
>>> +};
>>> +
>>> /**
>>>   * xe_gt_stats_incr - Increments the specified stats counter
>>>   * @gt: GT structure
>>> @@ -24,13 +31,12 @@ void xe_gt_stats_incr(struct xe_gt *gt, const enum xe_gt_stats_id id, int incr)
>>> 		return;
>>>
>>> 	atomic_add(incr, &gt->stats.counters[id]);
>>> -}
>>>
>>> -static const char *const stat_description[__XE_GT_STATS_NUM_IDS] = {
>>> -	"tlb_inval_count",
>>> -	"vma_pagefault_count",
>>> -	"vma_pagefault_bytes",
>>> -};
>>> +	if (atomic_read(&gt->stats.counters[id]) < 0) {
>>
>> why are we not using unsigned logic? it stops being atomic when you
>> update/read/set.
> 
> That is right, thanks for catching this.
> 
> @Matt: with the 2 other patches of the series, the counter limit was increased
> so much that the purpose of this patch is much less relevant. Is it fine with
> you if I retain your RB and merge the 2 other patches of this series, without
> this one?

Yeah, sure.

> 
> Francois
> 
>>
>> Lucas De Marchi
>>
>>> +		xe_gt_dbg(gt, "stats %s overflow, resetting\n", stat_description[id]);
>>> +		atomic_set(&gt->stats.counters[id], incr);
>>> +	}
>>> +}
>>>
>>> /**
>>>   * xe_gt_stats_print_info - Print the GT stats
>>> -- 
>>> 2.43.0
>>>