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

[Francois Dugast <francois.dugast@intel.com>]

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?

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
> >