On Fri, 17 May 2024 11:51:51 +0300 Alex Rusuf <yorha.op@xxxxxxxxx> wrote: > Hi SJ > > > Hello Alex, > > > > > > Adding high level comments first. I will try to read each patch and add > > detailed comments to those as soon as I get some time. > > > > Also, please Cc linux-mm@ for DAMON patches. I'd also recommend cc-ing > > linux-kernel@. > > Thank you for clarification, I'll add them! > > > > > On Wed, 15 May 2024 18:24:50 +0300 Alex Rusuf <yorha.op@xxxxxxxxx> wrote: > > > Currently kdamond uses only one context per kthread > > > and most of its time it sleeps, so utilizing several > > > contexts can scale kdamond and allow it to use > > > another set of operations. [...] > > > [3] https://github.com/onlyoneofme/damon-multi-contexts-tests.git > > > > Do you have a plan to integrate this into DAMON selftests or damon-tests? > > Not in the form they are for time being. These are just simple shell scripts > that set up kdamond to use multiple contexts and these scripts only > collect information like DAMO (but only as perf.data). > > Anyway, I think we can integrate them with damon-tests with some modifications > to be able to actually _test_ if multiple contexts work. > > As for DAMON selftests, I didn't touch them for time being, so they truly > need to be modified and expanded, but before doing that I would like > you to look at implementation first, because changes in implementation > could affect selftests, so once we agree on that I will implement selftests. > > Also note, that I didn't integrate changes with debugfs. I remember this is > deprecated interface, but I'm not sure if compatibility need to be preseved > with it, so do we need to expand debugfs for this? I think we should keep debugfs work as was. So I think we should ensure it's not broken, but don't need to expand it to support this new feature. > > > > > > Alex Rusuf (7): > > > mm/damon/core: kdamond_struct abstraction layer > > > > Let's make the subjects clear what it does. For example, this patch's > > subject could be "add kdamonds_struct abstraction layer". Similar comment > > for other patches. Also, I think '_struct' suffix of 'kdamond_struct' is > > not really needed. Let's remove it if there is no special reason to add > > it. > > Sure, I'll change that in next version, thanks! > > > > > > mm/damon/core: list-based contexts organization > > > > I think this can be squashed into the first patch? If not, could you please > > let clarify? > > I just tried to separate those patches for them to be as simple as > possible (actually I failed at that...), but sure, we can squash them. > > > > > > mm/damon/lru_sort: kdamond_struct abstraction layer > > > mm/damon/reclaim: kdamon_struct abstraction layer > > > > Does these two patches mean lru_sort and reclaim are broken by the first > > patch? Let's keep everything unbroken in middle of the patchset, to help > > bisect. > > Yes, they're broken by the first patch, I'll squash them, thanks! > > > > mm/damon/core: rename nr_running_ctxs -> nr_running_kdamonds > > > > I think this would also better to be together with the first patch? I know > > this does not break something, but makes reading patch bit complex. > > No problem, thanks! > > > > > > mm/damon/core: multi-context support > > > mm/damon/core: multi-context awarness for trace events > > > > I think these two patches should be squashed into one patch. Otherwise, the > > trace point is broken in the middle of the patch series, right? > > You're right, I'll squash them, thanks! Cool, I'm looking forward to the next version of this great patchset! Thanks, SJ [...]
