Re: [PATCH] sched/numa: Fix scan period for remote private faults

在 2026年08月04日 11:44, Zhan Xusheng 写道:
> From: Zhan Xusheng <[email protected]>
>
> On Tue, Aug 04, 2026 at 11:07:31AM +0800, Hongling Zeng wrote:
>> This is wrong because for remote private memory, we should continue
>> to the ratio calculation which can speed up scanning to migrate the
>> memory to the local node.
> I don't think the ratio calculation actually speeds scanning up in that
> case, though. For the pure remote-private accesses you describe
> (shared == 0, private > 0):
>
> 	ps_ratio = private * NUMA_PERIOD_SLOTS / (private + shared)
> 	         = private * 10 / (private + 0)
> 	         = 10
>
> which is >= NUMA_PERIOD_THRESHOLD (7), so it takes the first branch:
>
> 	int slot = ps_ratio - NUMA_PERIOD_THRESHOLD;   /* 3 */
> 	diff = slot * period_slot;                     /* > 0 */
>
> and numa_scan_period is *increased* (scan slower), not decreased. The
> speed-up (else) branch is only reached when both ps_ratio < 7 and
> lr_ratio < 7, which pure-private accesses (ps_ratio == 10) never satisfy.
>
> So dropping the early return here doesn't speed scanning up; it just
> grows the period by ~3 slots instead of doubling it. That might still be
> a reasonable change, but the justification as written describes an effect
> that doesn't seem to happen. Could you double-check, and share some
> before/after numbers on a remote-private workload? A scan-rate change
> like this really wants data behind it.
Thank you for the detailed review. Your analysis is completely correct.
   You're right. The original commit message incorrectly claimed this would
   "speed up" scanning. The actual effect is changing from unconditional
   doubling to a ratio-based adjustment (which still slows scanning, but 
less
   aggressively).

   I've updated the patch based on your feedback:
[PATCH v2] sched/numa: avoid doubling scan period for remote private faults

>
> Two smaller things:
>
>    - The comment rewrites (ps_ratio -> "private", lr_ratio -> "local") look
>      like a reasonable cleanup on their own, but folding them into a
>      behavioural change makes the patch harder to review -- perhaps split
>      them out. (The lr_ratio branch also keeps the "shared ... moved by
>      other tasks" sentence, which no longer fits a local-dominant branch.)
  This is a good point. For now I've kept them together since both changes
   address the same underlying issue (misleading comments about what the 
ratios
   represent). If this version is acceptable, I can submit a separate 
cleanup
   patch in the future if needed.

>
>    - This is the same early return that other in-flight patches touch (the
>      numa_faults_locality reset thread, where Peter suggested sharing the
>      tail with the normal path). It may be worth coordinating so the
>      changes don't collide
>
> Thanks,
> Zhan Xusheng
   I'll check for coordination with other in-flight patches before the next
   submission.

   Thank you again for catching the fundamental issue with the original
   justification and for the detailed suggestions on how to improve it.

   Best regards,
   Hongling