On 2020-05-19 20:32, Anshuman Khandual wrote:
...
How about not being quite so granular on the THP config options, and
just guarding these events with the overall CONFIG_TRANSPARENT_HUGEPAGE
option, instead of the sub-option CONFIG_ARCH_ENABLE_THP_MIGRATION?
I tentatively think it's harmless and not really misleading to have
/proc/vmstat showing this in all THP-enabled configurations:
thp_pmd_migration_success 0
thp_pmd_migration_failure 0
...if THP is enabled, and *whether or not* _THP_MIGRATION is enabled.
And this simplifies things a bit. Given how the .config options can get,
I think simplifying would be nice.
However, I'm ready to be corrected on that, if it's a bad idea for
other API reasons perhaps. Can anyone please comment?
There is no THP migration events to track unless it is enabled. Why to
show these statistics (as 0) when its not even possible. If the config
simplicity is the only intended rationale here, it might not be the
case either. These events and their tracking would still need to be
wrapped with CONFIG_TRANSPARENT_HUGEPAGE otherwise.
If your concern is more towards CONFIG_ARCH_ENABLE_THP_MIGRATION being
unsuitable or with complex dependencies, then that is something how THP
migration feature itself is implemented currently and adding VM events
does not address that. A possible patch in the future patch could solve
all these (together).
But sure, let's hear it for what others have to say on this.
Well, I don't want to hold up progress. If it's not very convincing to you,
let's just drop the idea/ It was kind of weak. :)
+ THP_PMD_MIGRATION_SUCCESS,
+ THP_PMD_MIGRATION_FAILURE,
+#endif
#endif
#ifdef CONFIG_MEMORY_BALLOON
BALLOON_INFLATE,
diff --git a/mm/migrate.c b/mm/migrate.c
index 7160c1556f79..5325700a3e90 100644
--- a/mm/migrate.c
+++ b/mm/migrate.c
@@ -1170,6 +1170,18 @@ static int __unmap_and_move(struct page *page, struct page *newpage,
#define ICE_noinline
#endif
+#ifdef CONFIG_ARCH_ENABLE_THP_MIGRATION
+static inline void thp_migration_success(bool success)
I think this should be named
thp_pmd_migration_success()
, since that's what you're really counting. Or, you could
name the events THP_MIGRATION_SUCCESS|FAILURE. Either way,
just so the function name matches the events it's counting.
Makes sense but IMHO we should keep _pmd_ to be more specific.
Will change the name here as thp_pmd_migration_success().
+{
+ if (success)
+ count_vm_event(THP_PMD_MIGRATION_SUCCESS);
+ else
+ count_vm_event(THP_PMD_MIGRATION_FAILURE);
+}
+#else
+static inline void thp_migration_success(bool success) { }
This whole ifdef clause would disappear if my suggestion above is
We will have to protect these with CONFIG_TRANSPARENT_HUGEPAGE as
the events are still conditionally available.
Yes you are right, of course. And I even worked through that, but then
when I sat down to write a response my fingers typed v1 of my understanding
instead of v2. No one knows why. :) Sorry about the misinformation there.
accepted. However, if not, then I believe the convention for this
kind of situation is:
static inline void thp_migration_success(bool success)
{
}
AFAIK, we have examples both ways but will change if this is preferred.
Not worth worrying about, but I do recall a few recent code reviews that
all preferred the multi-line version, which is why I suggested it.
Anyway, either way, with the thp_pmd_migration_success() name change, you
can add:
Reviewed-by: John Hubbard <jhubbard@xxxxxxxxxx>
thanks,
--
John Hubbard
NVIDIA
