Messages in this thread Patch in this message | | | From | "Edgecombe, Rick P" <> | Subject | Re: [PATCH v2 2/8] x86/mm: Don't do a TLB flush if changing a PTE that isn't marked present | Date | Mon, 27 Nov 2023 22:21:07 +0000 |
| |
On Tue, 2023-11-21 at 13:20 -0800, mhkelley58@gmail.com wrote: > --- a/arch/x86/mm/pat/set_memory.c > +++ b/arch/x86/mm/pat/set_memory.c > @@ -1636,7 +1636,10 @@ static int __change_page_attr(struct cpa_data > *cpa, int primary) > */ > if (pte_val(old_pte) != pte_val(new_pte)) { > set_pte_atomic(kpte, new_pte); > - cpa->flags |= CPA_FLUSHTLB; > + > + /* If old_pte isn't present, it's not in the > TLB */ > + if (pte_present(old_pte)) > + cpa->flags |= CPA_FLUSHTLB; > } > cpa->numpages = 1; > return 0; >
Makes sense to me. The PMD case can be handled similarly in __should_split_large_page().
I also think it should be more robust in regards to the cache flushing changes.
If callers did: set_memory_np() set_memory_uc() set_memory_p()
Then the cache flush would be missed. I don't think anyone is, but we shouldn't introduce hidden things like that. Maybe fix it like this:
diff --git a/arch/x86/mm/pat/set_memory.c b/arch/x86/mm/pat/set_memory.c index f519e5ca543b..28ff53a4447a 100644 --- a/arch/x86/mm/pat/set_memory.c +++ b/arch/x86/mm/pat/set_memory.c @@ -1856,11 +1856,6 @@ static int change_page_attr_set_clr(unsigned long *addr, int numpages, ret = __change_page_attr_set_clr(&cpa, 1); - /* - * Check whether we really changed something: - */ - if (!(cpa.flags & CPA_FLUSHTLB)) - goto out; /* * No need to flush, when we did not set any of the caching @@ -1868,6 +1863,12 @@ static int change_page_attr_set_clr(unsigned long *addr, int numpages, */ cache = !!pgprot2cachemode(mask_set); + /* + * Check whether we really changed something: + */ + if (!(cpa.flags & CPA_FLUSHTLB) && !cache) + goto out; + /* * On error; flush everything to be sure. */ Hmm, might want to maintain the "On error; flush everything to be sure" logic in the NP->P case as well.
| |