Re: [Question] About damon_set_regions function.

[SeongJae Park <sj@kernel.org>]

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