| Message ID | 20250407092243.2207837-1-xavier_qy@163.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | [v1] mm/contpte: Optimize loop to reduce redundant operations | expand |
Thanks for the patch. Would the following change be better? diff --git a/arch/arm64/mm/contpte.c b/arch/arm64/mm/contpte.c index 55107d27d3f8..64eb3b2fbf06 100644 --- a/arch/arm64/mm/contpte.c +++ b/arch/arm64/mm/contpte.c @@ -174,6 +174,9 @@ pte_t contpte_ptep_get(pte_t *ptep, pte_t orig_pte) if (pte_young(pte)) orig_pte = pte_mkyoung(orig_pte); + + if (pte_young(orig_pte) && pte_dirty(orig_pte)) + break; } return orig_pte;
On Mon, Apr 7, 2025 at 8:56 PM Xavier <xavier_qy@163.com> wrote: > > > > Hi Lance, > > Thanks for your feedback, my response is as follows. > > -- > Thanks, > Xavier > > > > > > At 2025-04-07 19:29:22, "Lance Yang" <ioworker0@gmail.com> wrote: > >Thanks for the patch. Would the following change be better? > > > >diff --git a/arch/arm64/mm/contpte.c b/arch/arm64/mm/contpte.c > >index 55107d27d3f8..64eb3b2fbf06 100644 > >--- a/arch/arm64/mm/contpte.c > >+++ b/arch/arm64/mm/contpte.c > >@@ -174,6 +174,9 @@ pte_t contpte_ptep_get(pte_t *ptep, pte_t orig_pte) > > > > if (pte_young(pte)) > > orig_pte = pte_mkyoung(orig_pte); > >+ > >+ if (pte_young(orig_pte) && pte_dirty(orig_pte)) > >+ break; > > } > > > > return orig_pte; > >-- > > > >We can check the orig_pte flags directly instead of using extra boolean > >variables, which gives us an early-exit when both dirty and young flags > >are set. > Your way of writing the code is indeed more concise. However, I think > using boolean variables might be more efficient. Although it introduces > additional variables, comparing boolean values is likely to be more > efficient than checking bit settings. > > > > >Also, is this optimization really needed for the common case? > This function is on a high-frequency execution path. During debugging, > I found that in most cases, the first few pages are already marked as > both dirty and young. But currently, the program still has to complete > the entire loop of 16 ptep iterations, which seriously reduces the efficiency. Hmm... agreed that this patch helps when early PTEs are dirty/young, but for late-ones-only cases, it only introduces overhead with no benefit, IIUC. So, let's wait for folks to take a look ;) Thanks, Lance > > > >Thanks, > >Lance
diff --git a/arch/arm64/mm/contpte.c b/arch/arm64/mm/contpte.c index bcac4f55f9c1..ca15d8f52d14 100644 --- a/arch/arm64/mm/contpte.c +++ b/arch/arm64/mm/contpte.c @@ -163,17 +163,26 @@ pte_t contpte_ptep_get(pte_t *ptep, pte_t orig_pte) pte_t pte; int i; + bool dirty = false; + bool young = false; ptep = contpte_align_down(ptep); for (i = 0; i < CONT_PTES; i++, ptep++) { pte = __ptep_get(ptep); - if (pte_dirty(pte)) + if (!dirty && pte_dirty(pte)) { + dirty = true; orig_pte = pte_mkdirty(orig_pte); + } - if (pte_young(pte)) + if (!young && pte_young(pte)) { + young = true; orig_pte = pte_mkyoung(orig_pte); + } + + if (dirty && young) + break; } return orig_pte;
This commit optimizes the contpte_ptep_get function by adding early termination logic. It checks if the dirty and young bits of orig_pte are already set and skips redundant bit-setting operations during the loop. This reduces unnecessary iterations and improves performance. Signed-off-by: Xavier <xavier_qy@163.com> --- arch/arm64/mm/contpte.c | 13 +++++++++++-- 1 file changed, 11 insertions(+), 2 deletions(-)