Re: [PATCH v22 01/18] mm: Introduce Data Access MONitor (DAMON)

[SeongJae Park <sjpark@amazon.com>]

Hi Shakeel,


Thanks for the review! :D

On Wed, 25 Nov 2020 07:29:10 -0800 Shakeel Butt <shakeelb@google.com> wrote:

> On Tue, Oct 20, 2020 at 2:01 AM SeongJae Park <sjpark@amazon.com> wrote:
> >
> > From: SeongJae Park <sjpark@amazon.de>
> >
> > DAMON is a data access monitoring framework for the Linux kernel.  The
> > core mechanisms of DAMON make it
> >
> >  - accurate (the monitoring output is useful enough for DRAM level
> >    performance-centric memory management; It might be inappropriate for
> >    CPU Cache levels, though),
> >  - light-weight (the monitoring overhead is normally low enough to be
> >    applied online), and
> >  - scalable (the upper-bound of the overhead is in constant range
> >    regardless of the size of target workloads).
> >
> > Using this framework, hence, we can easily write efficient kernel space
> > data access monitoring applications.  For example, the kernel's memory
> > management mechanisms can make advanced decisions using this.
> > Experimental data access aware optimization works that incurring high
> > access monitoring overhead could implemented again on top of this.
> >
> > Due to its simple and flexible interface, providing user space interface
> > would be also easy.  Then, user space users who have some special
> > workloads can write personalized applications for better understanding
> > and optimizations of their workloads and systems.
> >
> > That said, this commit is implementing only basic data structures and
> > simple manipulation functions of the structures.  The core mechanisms of
> > DAMON will be implemented by following commits.
> >
> > Signed-off-by: SeongJae Park <sjpark@amazon.de>
> > Reviewed-by: Leonard Foerster <foersleo@amazon.de>
> > Reviewed-by: Varad Gautam <vrd@amazon.de>
> 
> I don't see any benefit of this patch on its own. Some of this should
> be part of the main damon context patch.

Agreed.  Will change so in the next version.

> I would suggest to separate
> the core (damon context) from the target related structs (target,
> region, addr range).

To be honest, I unsure if I'm fully understanding what specific change you want
to make.  So if I'm misunderstanding your point below, please let me know.

Seems like you are concerning for future support of special kind use cases that
don't need targets/regions/addresses, such as page granularity monitoring that
having interest in only if the pages accessed or not, rather than access
frequency.  In the context, your suggestion makes sense as the region
abstraction is only burden in the case, as I also mentioned in the cover
letter.  This could be done via idle pages tracking, but DAMON will be able to
do this faster by reducing the number of user-kernel context switches.

I once considered adding some change in the core part for efficient support of
such use cases, but resulted in believing that the best way for that is
implementing another primitive for the case and use it in a controlled way.

In a high level, it should disable the 'adaptive regions adjustment' feature
and define it's own targets structs other than the damon_target.  Then, your
implementation of the primitive callbacks should use your own targets.

In more detail, the 'adaptive regions adjustment' can be disabled by setting
the min_nr_regions and max_nr_regions of the damon_ctx with same value, say, 0.
Your own targets structs could be stored in 'damon_callback->private'.  Or, we
could add another 'private' field in damon_ctx for that.

I think this will work, but I also admit that this could look like a hairy
hack to someone.  Fundamentally, this is because the region based
overhead/accuracy handling is strongly coupled in the design.  So, I think what
you are really suggesting is making DAMON more general by default and
supporting the region based overhead/accuracy handling additionally.

If I'm understanding correctly, how about below like change?  Obviously this
should be cleaned up a lot, but I just want to quickly share my idea and
discuss.  Also note that it's based on the damon/next tree[1].

[1] https://github.com/sjp38/linux/tree/damon/next

+enum damon_type {
+       ARBITRARY_TARGETS,
+       ADAPTIVE_REGIONS,
+};
+
+struct damon_adaptive_regions_ctx {
+       unsigned long min_nr_regions;
+       unsigned long max_nr_regions;
+       struct list_head targets;
+       struct list_head schemes;
+};
+
 /**
  * struct damon_ctx - Represents a context for each monitoring.  This is the
  * main interface that allows users to set the attributes and get the results
@@ -243,8 +255,6 @@ struct damon_ctx {
        unsigned long sample_interval;
        unsigned long aggr_interval;
        unsigned long regions_update_interval;
-       unsigned long min_nr_regions;
-       unsigned long max_nr_regions;

        struct timespec64 last_aggregation;
        struct timespec64 last_regions_update;
@@ -253,11 +263,14 @@ struct damon_ctx {
        bool kdamond_stop;
        struct mutex kdamond_lock;

-       struct list_head targets_list;  /* 'damon_target' objects */
-       struct list_head schemes_list;  /* 'damos' objects */
-
        struct damon_primitive primitive;
        struct damon_callback callback;
+
+       enum damon_type type;
+       union {
+               struct damon_adaptive_regions_ctx arctx;
+               void *targets;
+       };
 };

The patchset will first introduce DAMON as only ARBITRARY_TARGETS (or, would
TINY be a better name?) type supporting form.  After that, following patch will
add ADAPTIVE_REGIONS type support.  Do you think I'm correctly understanding
your point and above suggestion makes sense?

> 
> Also I would prefer the code be added with the actual usage otherwise
> it is hard to review.

Agreed.  Will do so in the next version.

> 
> > ---
> [snip]
> > +unsigned int damon_nr_regions(struct damon_target *t)
> > +{
> > +       struct damon_region *r;
> > +       unsigned int nr_regions = 0;
> > +
> > +       damon_for_each_region(r, t)
> > +               nr_regions++;
> > +
> > +       return nr_regions;
> > +}
> 
> Why not keep a count instead of traversing to get the size?

First of all, because this makes code simpler while this is not confirmed as
bottleneck, yet.  Because users can set the max_nr_regions, I think this will
not be real problem in the region based overhead/accuracy handling case.  In
case of the 'ARBITRARY_TARGETS', this will not used at all.  I also prefer this
because the user could add/remove regions on their own inside their callbacks.


Thanks,
SeongJae Park