[Question] About damon_set_regions function.

[Question] About damon_set_regions function.

[Yun Levi]

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.

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?

Thanks.

-- 
Best regards,
Levi

Re: [Question] About damon_set_regions function.

[SeongJae Park]

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

Re: [Question] About damon_set_regions function.

[Yun Levi]

> 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.,

Thanks for your review! I miss this case.
In that case maybe some of former regions wouldn't be removed!


> --- 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?
>

I think it's good.
But, IMHO, I don't know if it would be useful to merge
first <= new_insert_area <= last with new inserted regions as one region.

if merging is meaningless, LGTM.

Thanks.
--

Sincerely,
Levi

Re: [Question] About damon_set_regions function.

[SeongJae Park]

Hi Levi,

On Wed, 7 Sep 2022 13:50:01 +0900 Yun Levi <ppbuk5246@gmail.com> wrote:

> > 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.,
> 
> Thanks for your review! I miss this case.
> In that case maybe some of former regions wouldn't be removed!
> 
> 
> > --- 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?
> >
> 
> I think it's good.
> But, IMHO, I don't know if it would be useful to merge
> first <= new_insert_area <= last with new inserted regions as one region.

In the case, I concern that we will lost the access monitoring information of
the first and last regions that we collected so far.

> 
> if merging is meaningless, LGTM.

Thanks.  If you don't mind, I will post a patch for this soon.


Thanks,
SJ

> 
> Thanks.
> --
> 
> Sincerely,
> Levi

Re: [Question] About damon_set_regions function.

[Yun Levi]

>
> Thanks.  If you don't mind, I will post a patch for this soon.

Never mind! I'm looking forward to seeing the patch :) Thanks!


-- 
Best regards,
Levi