| Message ID | 20230707053331.510041-1-anshuman.khandual@arm.com (mailing list archive) |
|---|---|
| Headers | show |
| Series | arm64/mm: Clean up pte_dirty() state management | expand |
On 07.07.23 07:33, Anshuman Khandual wrote: > These pte_dirty() changes make things explicitly clear, while improving the > code readability. This optimizes HW dirty state transfer into SW dirty bit. > This also adds a new arm64 documentation explaining overall pte dirty state > management in detail. This series applies on the latest mainline kernel. > > I skimmed over most of the series, and I am not convinced that this is actually a cleanup. If we cannot really always differentiate between sw/hw clearing, why have separate primitives that give one the illusion that it could be done and that they are two different concepts? Maybe there are other opinions, but at least for me this made the code harder to grasp. I do appreciate a doc update, though :)
On 7/7/23 17:41, David Hildenbrand wrote: > On 07.07.23 07:33, Anshuman Khandual wrote: >> These pte_dirty() changes make things explicitly clear, while improving the >> code readability. This optimizes HW dirty state transfer into SW dirty bit. >> This also adds a new arm64 documentation explaining overall pte dirty state >> management in detail. This series applies on the latest mainline kernel. >> >> > > I skimmed over most of the series, and I am not convinced that this is actually a cleanup. If we cannot really always differentiate between sw/hw clearing, why have separate primitives that give one the illusion that it could be done and that they are two different concepts? These are indeed two different concepts working together, the current code just obscures that. Without these primitives it's even hard to follow how the SW and HW dirty parts are intertwined in implementing the generic pte_dirty() state. The current code acknowledges these two different concepts in identifying them i.e via pte_hw_dirty() and pte_sw_dirty(). #define pte_hw_dirty(pte) (pte_write(pte) && !(pte_val(pte) & PTE_RDONLY)) #define pte_sw_dirty(pte) (!!(pte_val(pte) & PTE_DIRTY)) But then falls short in demonstrating how these two states are being managed i.e created and cleared. The first patch tries to clear the situation to some extent. > > Maybe there are other opinions, but at least for me this made the code harder to grasp. > > I do appreciate a doc update, though :) These two changes also streamline HW dirty bit saving in SW dirty bit efficiently, skipping the re-doing of HW dirty bits setting which might be cleared off. That is the primary reason for saving it off in SW dirty bit in the first place. arm64/mm: Call pte_sw_mkdirty() while preserving the HW dirty state arm64/mm: Add pte_preserve_hw_dirty( Regardless, I am still trying to understand how the first patch does not improve the clarity in modifying the PTE dirty state.
On 10.07.23 04:20, Anshuman Khandual wrote: > > > On 7/7/23 17:41, David Hildenbrand wrote: >> On 07.07.23 07:33, Anshuman Khandual wrote: >>> These pte_dirty() changes make things explicitly clear, while improving the >>> code readability. This optimizes HW dirty state transfer into SW dirty bit. >>> This also adds a new arm64 documentation explaining overall pte dirty state >>> management in detail. This series applies on the latest mainline kernel. >>> >>> >> >> I skimmed over most of the series, and I am not convinced that this is actually a cleanup. If we cannot really always differentiate between sw/hw clearing, why have separate primitives that give one the illusion that it could be done and that they are two different concepts? > > These are indeed two different concepts working together, the current code just > obscures that. Without these primitives it's even hard to follow how the SW and > HW dirty parts are intertwined in implementing the generic pte_dirty() state. > > The current code acknowledges these two different concepts in identifying them > i.e via pte_hw_dirty() and pte_sw_dirty(). > > #define pte_hw_dirty(pte) (pte_write(pte) && !(pte_val(pte) & PTE_RDONLY)) > #define pte_sw_dirty(pte) (!!(pte_val(pte) & PTE_DIRTY)) > ^ these primitives make sense to me, but not the clearing part. If there is only one way to clear both, then only have one primitive to clear both and state there, that separate clearing is impossible because both are intertwined.
On Fri, Jul 07, 2023 at 11:03:27AM +0530, Anshuman Khandual wrote: > These pte_dirty() changes make things explicitly clear, while improving the > code readability. This optimizes HW dirty state transfer into SW dirty bit. > This also adds a new arm64 documentation explaining overall pte dirty state > management in detail. This series applies on the latest mainline kernel. TBH, I think this is all swings and roundabouts, and I'm not sure this is worthwhile. I appreciate that as-is some people find this confusing, but I don't think the end result of this series is actually better, and it adds more code/documentation to maintain. In particular, I don't think that we should add Documentation/ files for this, as it's very likely that won't be updated together with the code, and I think it's more of a maintenance burden than a help. If we want some introductory text to explain how the HW/SW dirty bits work, I think that should be a comment block in <asm/pgtable.h>, clearly associated with the code. Overall, I'd prefer to leave the code as-is. Thanks, Mark. > > Cc: Catalin Marinas <catalin.marinas@arm.com> > Cc: Will Deacon <will@kernel.org> > Cc: Ryan Roberts <ryan.roberts@arm.com> > Cc: Mark Rutland <mark.rutland@arm.com> > Cc: Andrew Morton <akpm@linux-foundation.org> > Cc: David Hildenbrand <david@redhat.com> > Cc: Jonathan Corbet <corbet@lwn.net> > Cc: linux-arm-kernel@lists.infradead.org > Cc: linux-kernel@vger.kernel.org > Cc: linux-doc@vger.kernel.org > > Anshuman Khandual (4): > arm64/mm: Add SW and HW dirty state helpers > arm64/mm: Call pte_sw_mkdirty() while preserving the HW dirty state > arm64/mm: Add pte_preserve_hw_dirty() > docs: arm64: Add help document for pte dirty state management > > Documentation/arch/arm64/index.rst | 1 + > Documentation/arch/arm64/pte-dirty.rst | 95 ++++++++++++++++++++++++++ > arch/arm64/include/asm/pgtable.h | 66 ++++++++++++++---- > 3 files changed, 147 insertions(+), 15 deletions(-) > create mode 100644 Documentation/arch/arm64/pte-dirty.rst > > -- > 2.30.2 >
On 7/10/23 16:55, Mark Rutland wrote: > On Fri, Jul 07, 2023 at 11:03:27AM +0530, Anshuman Khandual wrote: >> These pte_dirty() changes make things explicitly clear, while improving the >> code readability. This optimizes HW dirty state transfer into SW dirty bit. >> This also adds a new arm64 documentation explaining overall pte dirty state >> management in detail. This series applies on the latest mainline kernel. > TBH, I think this is all swings and roundabouts, and I'm not sure this is > worthwhile. I appreciate that as-is some people find this confusing, but I Current situation for pte_dirty() management is confusing when there are two distinct mechanisms to track PTE dirty states, but both are forced to work together because - HW DBM cannot track non-writable dirty state (PTE_DBM == PTE_WRITE) - Runtime check for HW DBM is avoided > don't think the end result of this series is actually better, and it adds more > code/documentation to maintain. Agreed, it does add more code and documentation but still trying to understand why it is not worthwhile. Regardless, following patch does optimize a situation where we dont need to call pte_mkdirty() knowing it will be cleared afterwards. [RFC 2/4] arm64/mm: Call pte_sw_mkdirty() while preserving the HW dirty state > > In particular, I don't think that we should add Documentation/ files for this, > as it's very likely that won't be updated together with the code, and I think > it's more of a maintenance burden than a help. If we want some introductory There are many documentation files such as Documentation/arm64/memory.rst which needs to be updated when kernel virtual address layout changes again. I am just wondering - should not there be any documentation for internal implementation details, just because they need updating with code changes. > text to explain how the HW/SW dirty bits work, I think that should be a comment > block in <asm/pgtable.h>, clearly associated with the code. Sure, will add that. > > Overall, I'd prefer to leave the code as-is. Even if we discount individual dirty clearing functions, why should not HW dirty bit transfer to SW dirty be optimized and wrapped around in a helper.
On Wed, Jul 12, 2023 at 09:31:39AM +0530, Anshuman Khandual wrote: > On 7/10/23 16:55, Mark Rutland wrote: > > On Fri, Jul 07, 2023 at 11:03:27AM +0530, Anshuman Khandual wrote: > >> These pte_dirty() changes make things explicitly clear, while improving the > >> code readability. This optimizes HW dirty state transfer into SW dirty bit. > >> This also adds a new arm64 documentation explaining overall pte dirty state > >> management in detail. This series applies on the latest mainline kernel. > > > > TBH, I think this is all swings and roundabouts, and I'm not sure this is > > worthwhile. I appreciate that as-is some people find this confusing, but I I'm pretty much on the same lines, though maybe I looked too much at this code that I don't like any further changes to it ;). > Current situation for pte_dirty() management is confusing when there are two > distinct mechanisms to track PTE dirty states, but both are forced to work > together because > > - HW DBM cannot track non-writable dirty state (PTE_DBM == PTE_WRITE) > - Runtime check for HW DBM is avoided Depending on how you look at it, we can say that any writeable PTE (as in page table permission, PTE_RDONLY cleared) is dirty and we only have a software mechanism for tracking the dirty state. The DBM feature is not actually giving us a dirty bit but an automated way to make a PTE writeable on access (for some historical reasons like the SMMU not having such mechanism in place). Maybe we can clean the code a bit based on the above perspective. E.g. instead of pte_hw_dirty() just have a !pte_hw_rdonly() macro. It may help with the confusion of having two mechanisms. OTOH, with PIE, we can have a true dirty bit but at that point we can eliminate the pte_sw_dirty() use entirely and allow soft-dirty using the current PTE_DIRTY (with some static labels based on the feature). > > don't think the end result of this series is actually better, and it adds more > > code/documentation to maintain. > > Agreed, it does add more code and documentation but still trying to understand > why it is not worthwhile. Regardless, following patch does optimize a situation > where we dont need to call pte_mkdirty() knowing it will be cleared afterwards. > > [RFC 2/4] arm64/mm: Call pte_sw_mkdirty() while preserving the HW dirty state I wonder whether the compiler eliminates much of this duplication since there are some checks for pte_write() before. We may be able to remove some checks. For example, does pte_hw_dirty() actually need to check pte_write()? A !PTE_RDONLY entry is dirty automatically since we can't trap any write access to it (prior to PIE; I need to check Joey's patches on how it treats writeable+clean PTEs; still on holiday). As for the fourth patch, I'd rather add documentation in the header file, it's more likely to be looked at and updated.