Re: Re: [PATCH v17 12/15] Documentation/admin-guide/mm: Add a document for DAMON

From: SeongJae Park
Date: Tue Jul 07 2020 - 04:07:47 EST


On Tue, 7 Jul 2020 10:49:06 +0300 Mike Rapoport <rppt@xxxxxxxxxx> wrote:

> Hello SeongJae,
>
> On Mon, Jul 06, 2020 at 01:53:19PM +0200, SeongJae Park wrote:
> > From: SeongJae Park <sjpark@xxxxxxxxx>
> >
> > This commit adds a document for DAMON under
> > `Documentation/admin-guide/mm/damon/`.
> >
> > Signed-off-by: SeongJae Park <sjpark@xxxxxxxxx>
> > ---
> > Documentation/admin-guide/mm/damon/api.rst | 20 ++
> > .../admin-guide/mm/damon/damon_heatmap.png | Bin 0 -> 8366 bytes
> > .../admin-guide/mm/damon/damon_wss_change.png | Bin 0 -> 7211 bytes
> > .../admin-guide/mm/damon/damon_wss_dist.png | Bin 0 -> 6173 bytes
> > Documentation/admin-guide/mm/damon/eval.rst | 222 +++++++++++++
> > Documentation/admin-guide/mm/damon/faq.rst | 59 ++++
> > .../admin-guide/mm/damon/freqmine_heatmap.png | Bin 0 -> 9999 bytes
> > .../admin-guide/mm/damon/freqmine_wss_sz.png | Bin 0 -> 5589 bytes
> > .../mm/damon/freqmine_wss_time.png | Bin 0 -> 6550 bytes
> > Documentation/admin-guide/mm/damon/guide.rst | 194 ++++++++++++
> > Documentation/admin-guide/mm/damon/index.rst | 35 +++
> > .../admin-guide/mm/damon/mechanisms.rst | 159 ++++++++++
> > Documentation/admin-guide/mm/damon/plans.rst | 29 ++
> > Documentation/admin-guide/mm/damon/start.rst | 117 +++++++
> > .../mm/damon/streamcluster_heatmap.png | Bin 0 -> 42210 bytes
> > .../mm/damon/streamcluster_wss_sz.png | Bin 0 -> 6327 bytes
> > .../mm/damon/streamcluster_wss_time.png | Bin 0 -> 8830 bytes
> > Documentation/admin-guide/mm/damon/usage.rst | 296 ++++++++++++++++++
> > Documentation/admin-guide/mm/index.rst | 1 +
>
> It's really cool to see documentation along with the code!
>
> I'd suggest to reorganize the DAMON docs to better match the current
> structure of Documentation/ directory.
>
> The description of DAMON usage from the userspace and reference for the
> userland tools does belong to Documentation/admin-guide/mm. However, the
> kernel APIs are better to be placed in Documentation/core-api or even
> Documentation/dev-tools. As for the detailed description of the internal
> DAMON operation, this would naturally belong to Documentation/vm.
>
> Another thing is that this patch is really hard to review because of the
> encoded png blobs. In general, we try to keep Documentation/ readable in
> plain text, but if you think that the images are essential and must be a
> part of DAMON in-tree docs rather than links to an external resource,
> please split them to a separate patch.

Totally agreed! Thanks for the kind and wise comments. I will try to remove
the blobs or at least separate those in the next spin.


Thanks,
SeongJae Park

>
> --
> Sincerely yours,
> Mike.
>