On Mon, Jul 22, 2024 at 3:47 PM Waiman Long longman@redhat.com wrote:
On 7/22/24 15:30, David Finkel wrote:
diff --git a/mm/page_counter.c b/mm/page_counter.c index db20d6452b71..40d5f4990218 100644 --- a/mm/page_counter.c +++ b/mm/page_counter.c @@ -82,6 +82,8 @@ void page_counter_charge(struct page_counter *counter, unsigned long nr_pages) */ if (new > READ_ONCE(c->watermark)) WRITE_ONCE(c->watermark, new);
if (new > READ_ONCE(c->local_watermark))WRITE_ONCE(c->local_watermark, new);Hm, can't we have a single comparison on the hot path? Also, we read and write c->local_watermark speculatively here, Idk if it's still acceptable with an ability to reset watermarks "locally". Maybe it is, but it definitely deserves at least a comment with an explanation.
Unfortunately, since the two watermarks may be reset at different times I don't think we can consolidate. e.g. I think that if the usage peaked, dropped down a bit and then was going back up again when the "local_watermark" was reset, we'll continue only bumping local_watermark, but we don't want to touch "watermark" until we hit that watermark again.
If we make page_counter_reset_watermark() reset the local_watermark as well, we can guarantee "local_watermark <= watermark" and wrap one check inside the other.
if (new > READ_ONCE(c->local_watermark)) { WRITE_ONCE(c->local_watermark, new); if (new > READ_ONCE(c->watermark)) WRITE_ONCE(c->watermark, new); }Cheers, Longman
Hmm, yeah, given that we'll only be resetting one of the two, I think I'll use this option. The branch predictor should make that second check pretty much a noop in the common-case when we enter the outer if, too.
Thanks!