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

[SeongJae Park]

On Thu, 26 Nov 2020 12:51:57 +0100 SeongJae Park <sjpark@xxxxxxxxxx> wrote:

> Hi Shakeel,
> 
> 
> Thanks for the review! :D
> 
> On Wed, 25 Nov 2020 07:29:10 -0800 Shakeel Butt <shakeelb@xxxxxxxxxx> wrote:
> 
> > On Tue, Oct 20, 2020 at 2:01 AM SeongJae Park <sjpark@xxxxxxxxxx> wrote:
> > >
> > > From: SeongJae Park <sjpark@xxxxxxxxx>
> > >
> > > 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@xxxxxxxxx>
> > > Reviewed-by: Leonard Foerster <foersleo@xxxxxxxxx>
> > > Reviewed-by: Varad Gautam <vrd@xxxxxxxxx>
> > 
[...]
> > 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?

In a private message, Shakeel confirmed I'm correnctly understanding his
intention and asked next version.  I will post next version soon.


Thanks,
SeongJae Park