Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider: - [Medium] The statistic `XE_GT_STATS_ID_DEFRAG_MB_MOVED` systematically undercounts data moved because it truncates the byte count to megabytes *before* accumulating it into the global counter. Any defragmentation move smaller than 1 MiB (e.g., re-backing a 64 KiB page) will evaluate to 0 and not be counted at all. Additionally, the sub-megabyte remainders of larger moves are permanently lost. --
--- Patch [21]: [PATCH v2 21/33] drm/xe: Add defrag GT stats --- commit a152b345237256025e6e622bf740f0941373de67 Author: Matthew Brost <[email protected]> drm/xe: Add defrag GT stats This commit introduces observability for the page defragmentation worker by adding several primary-GT statistics counters. These counters track the success, failure, and amount of data moved during defragmentation attempts. > diff --git a/drivers/gpu/drm/xe/xe_bo.c b/drivers/gpu/drm/xe/xe_bo.c > index 6a847e42946ce..c1bbcf0d21edb 100644 > --- a/drivers/gpu/drm/xe/xe_bo.c > +++ b/drivers/gpu/drm/xe/xe_bo.c [ ... ] > @@ -1264,6 +1267,20 @@ static int xe_bo_defrag_one(struct xe_device *xe, > struct xe_bo *bo, > xe_dbg(xe, "Defrag attempt on BO size=%zu: ret=%pe consumed=%llu\n", > xe_bo_size(bo), ERR_PTR(ret), *consumed); > > + if (!ret && ttm_tt_is_beneficial_order_failed(bo->ttm.ttm)) > + xe_gt_stats_incr(xe_root_mmio_gt(xe), > + XE_GT_STATS_ID_DEFRAG_PARTIAL_SUCCESS_COUNT, > + 1); > + else > + xe_gt_stats_incr(xe_root_mmio_gt(xe), > + ret ? XE_GT_STATS_ID_DEFRAG_FAILED_COUNT : > + XE_GT_STATS_ID_DEFRAG_SUCCESS_COUNT, 1); > + > + if (!ret) > + xe_gt_stats_incr(xe_root_mmio_gt(xe), > + XE_GT_STATS_ID_DEFRAG_MB_MOVED, > + *consumed >> 20); [Severity: Medium] Will this undercount the data moved if we truncate to megabytes before accumulation? If the consumed amount is less than 1 MiB (e.g. re-backing a 64 KiB page), this evaluates to 0 and nothing is added to the global counter. The sub-megabyte remainders of larger moves also appear to be permanently lost on every function call. Would it be better to accumulate the exact value in bytes (or kilobytes) and only perform the division when presenting the statistic to user-space? > unlock: > xe_bo_unlock(bo); > return ret; -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=21
