Hi, Dave
On 2021/6/9 11:00, Dave Chinner wrote:
> On Thu, May 27, 2021 at 09:56:38PM +0800, Zhang Yi wrote:
>> +/**
>> + * jbd2_journal_shrink_scan()
>> + *
>> + * Scan the checkpointed buffer on the checkpoint list and release the
>> + * journal_head.
>> + */
>> +unsigned long jbd2_journal_shrink_scan(struct shrinker *shrink,
>> + struct shrink_control *sc)
>> +{
>> + journal_t *journal = container_of(shrink, journal_t, j_shrinker);
>> + unsigned long nr_to_scan = sc->nr_to_scan;
>> + unsigned long nr_shrunk;
>> + unsigned long count;
>> +
>> + count = percpu_counter_sum_positive(&journal->j_jh_shrink_count);
>> + trace_jbd2_shrink_scan_enter(journal, sc->nr_to_scan, count);
>> +
>> + nr_shrunk = jbd2_journal_shrink_checkpoint_list(journal, &nr_to_scan);
>> +
>> + count = percpu_counter_sum_positive(&journal->j_jh_shrink_count);
>> + trace_jbd2_shrink_scan_exit(journal, nr_to_scan, nr_shrunk, count);
>> +
>> + return nr_shrunk;
>> +}
>> +
>> +/**
>> + * jbd2_journal_shrink_scan()
>> + *
>> + * Count the number of checkpoint buffers on the checkpoint list.
>> + */
>> +unsigned long jbd2_journal_shrink_count(struct shrinker *shrink,
>> + struct shrink_control *sc)
>> +{
>> + journal_t *journal = container_of(shrink, journal_t, j_shrinker);
>> + unsigned long count;
>> +
>> + count = percpu_counter_sum_positive(&journal->j_jh_shrink_count);
>> + trace_jbd2_shrink_count(journal, sc->nr_to_scan, count);
>> +
>> + return count;
>> +}
>
> NACK.
>
> You should not be putting an expensive percpu counter sum in a
> shrinker object count routine. These gets called a *lot* under
> memory pressure, and summing over hundreds/thousands of CPUs on
> every direct reclaim instance over every mounted filesystem
> instance that uses jbd2 is really, really going to hurt system
> performance under memory pressure.
>
> And, quite frankly, summing twice in the scan routine just to trace
> the result with no other purpose is unnecessary and excessive CPU
> overhead for a shrinker.
>
> Just use percpu_counter_read() in all cases here - it is more than
> accurate enough for the purposes of both tracing and memory reclaim.
OK, I missed this point, thanks for the comments, I will use
percpu_counter_read_positive() in the next iteration.
Thanks,
Yi.
