On Sat, Jul 12, 2025 at 5:14 AM Robert Pang <robertp...@google.com> wrote:
>
> Here's a rewritten version of your email, aiming for clarity,
> conciseness, and a professional tone:
>
> Subject: Question about dm-cache promotion decision in dm-cache-policy-smq.c
> Hi,
>
> I'm currently investigating how dm-cache makes promotion decisions and
> have a question regarding a specific line in the
> update_promote_levels() function within dm-cache-policy-smq.c.
>
> Specifically, I'm looking at line [1] in the code snippet below:
>
> static void update_promote_levels(struct smq_policy *mq)
> {
> /*
> * If there are unused cache entries then we want to be really
> * eager to promote.
> */
> unsigned int threshold_level = allocator_empty(&mq->cache_alloc) ?
> default_promote_level(mq) : (NR_HOTSPOT_LEVELS / 2u);
>
> /* line [1] */
> threshold_level = max(threshold_level, NR_HOTSPOT_LEVELS);
> ...
>
> mq->read_promote_level = NR_HOTSPOT_LEVELS - threshold_level;
> mq->write_promote_level = (NR_HOTSPOT_LEVELS - threshold_level);
> }
>
> My understanding is that NR_HOTSPOT_LEVELS is defined as 64, and
> default_promote_level() returns a value between 1 and 8. If this is
> correct, then threshold_level (before line [1]) would always be less
> than or equal to NR_HOTSPOT_LEVELS (e.g., 8 or 32). Consequently,
> max(threshold_level, NR_HOTSPOT_LEVELS) would always evaluate to
> NR_HOTSPOT_LEVELS, effectively setting threshold_level to 64
> regardless of its initial calculation.
>
> I've traced line [1] back to commit b29d498 ("dm cache: significant
> rework to leverage dm-bio-prison-v2") [2]. It seems like the line's
> purpose is to cap threshold_level to prevent underflow in the
> subsequent subtractions for read_promote_level and
> write_promote_level.
>
> Given this intended purpose and the typical behavior of min() and
> max() functions, shouldn't line [1] instead be:
>
> threshold_level = min(threshold_level, NR_HOTSPOT_LEVELS);
>
> This would ensure threshold_level doesn't exceed NR_HOTSPOT_LEVELS,
> while still allowing its initial calculated value to be used if it's
> lower.
>
> Could someone please clarify if my interpretation of this logic is
> correct, or if I'm missing something?
>
> Best regards
> Robert Pang
>
> [1]
> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/md/dm-cache-policy-smq.c#n1057
> [2]
> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=b29d498
>
Hi,
Currently, dm-cache makes promotion decisions based on short-term
access patterns. The read/write_promote_level determines how
frequently a missed block must be accessed before it is promoted. If
we modify line [1] to use min(threshold_level, NR_HOTSPOT_LEVELS), it
would increase the read/write_promote_level, making the cache less
responsive to varying IO patterns. That may be why commit b29d498
changed the behavior.
I’ve noticed this ambiguity before, just haven’t submitted a patch to
clean it up. The default_promote_level() might have been kept as a
reminder of the older approach.
Thanks,
Ming-Hung Tsai
