Re: [Question] About damon_set_regions function.

[SeongJae Park <sj@kernel.org>]

Hello Levi,

On Tue, 6 Sep 2022 12:19:07 +0900 Yun Levi <ppbuk5246@gmail.com> wrote:

> Hello damon community.
> I'm a beginner of damon who want to use it well :)
> 
> While I'm reading the damon core code,
> I have a question about the "damon_set_regions" function's features.
> 
> Comment said that
> "This function adds new regions to, or modify existing regions of a
> monitoring target to fit in specific ranges".
> 
> But I don't know if it's correct in the case below:
> 
> Suppose, target already has 2 ranges one of them is 4K to 16K and the
> other 24K to 32K
> And someone calls damon_set_regions with a range 8K to 28K.
> 
> In that situation, damon_set_regions function will modify existing two
> region like
>     - first as 8K to 16K
>     - last as 24K to 28K
> 
> base on below code snippet:
>          } else {
>               /* resize intersecting regions to fit in this range */
>               first->ar.start = ALIGN_DOWN(range->start,
>                       DAMON_MIN_REGION);
>               last->ar.end = ALIGN(range->end, DAMON_MIN_REGION);
>          }
> 
> But, I don't know the reason why the region having 16K to 24K range
> isn't added to the target region list and it's correct or not.

Good finding!  Right, the code assumes the regions are always contiguous, which
is wrong!

> 
> In my thinking,  there should be the code adding the region like:
> 
> diff --git a/mm/damon/core.c b/mm/damon/core.c
> index 7d25dc582fe3..f755efca71b2 100644
> --- a/mm/damon/core.c
> +++ b/mm/damon/core.c
> @@ -225,6 +225,15 @@ int damon_set_regions(struct damon_target *t,
> struct damon_addr_range *ranges,
>                         first->ar.start = ALIGN_DOWN(range->start,
>                                         DAMON_MIN_REGION);
>                         last->ar.end = ALIGN(range->end, DAMON_MIN_REGION);
> +
> +                       if (first != last) {
> +                               newr = damon_new_region(
> +                                               first->ar.end,
> last->ar_start); /**< already aligned. */
> +                               if (!newr)
> +                                       return -ENOMEM;
> +
> +                               damon_insert_region(newr, first, last, t);
> +                       }
>                 }
>         }
>         return 0;
> 
> Am I wrong?

This would fix the specific case, but there might be more than 2 regions that
not contiguous.  I think we should also handle the case?  E.g.,

--- a/mm/damon/core.c
+++ b/mm/damon/core.c
@@ -194,6 +194,7 @@ int damon_set_regions(struct damon_target *t, struct damon_addr_range *ranges,
 {
        struct damon_region *r, *next;
        unsigned int i;
+       bool found;

        /* Remove regions which are not in the new ranges */
        damon_for_each_region_safe(r, next, t) {
@@ -235,6 +236,24 @@ int damon_set_regions(struct damon_target *t, struct damon_addr_range *ranges,
                        first->ar.start = ALIGN_DOWN(range->start,
                                        DAMON_MIN_REGION);
                        last->ar.end = ALIGN(range->end, DAMON_MIN_REGION);
+
+                       /* fill the holes between first and last */
+                       found = false;
+                       damon_for_each_region(r, t) {
+                               struct damon_region *next;
+                               if (r == first)
+                                       found = true;
+                               if (!found)
+                                       continue;
+                               if (r == last)
+                                       break;
+                               next = damon_next_region(r);
+                               if (next->ar.start != r->ar.end) {
+                                       newr = damon_new_region(r->ar.end, next->ar.start);
+                                       damon_insert_region(newr, r, next, t);
+                               }
+                       }
+
                }
        }
        return 0;

Of course, the code should be cleaned up (e.g., would better to use
list_for_each_entry_from() and factor out to a separate function).  This is
only for a PoC.  Is there anything I'm missing?


Thanks,
SJ

> 
> Thanks.
> 
> -- 
> Best regards,
> Levi