On 06.04.23 16:16, Johannes Weiner wrote:
On Thu, Apr 06, 2023 at 03:23:11PM +0200, David Hildenbrand wrote:
Often, when you have to start making a list of things that a patch does, it
might make sense to split some of the items into separate patches such that you
can avoid lists and just explain in list-free text how the pieces in the patch
fit together.
I'd suggest splitting this patch into logical pieces. For example, separating
the general profit calculation/exposure from the per-mm profit and the per-mm
ksm type indication.
Originally these were individual patches. If I recall correctly Johannes
Weiner wanted them as one patch. I can certainly split them again.
That's why I remember that v1 contained more patches :)
Again, just my opinion on patches that require a description in form of a
list ...
My concern was the initial splitting of patch 1. I found it easier to
review with the new prctl() being one logical change, and fully
implemented in one go. The changelog is still in the form of a list,
but it essentially enumerates diff hunks that make up the interface.
No objection to splitting out the multiple sysfs knobs, though!
The strategy I usually follow is this:
1. Split out changes based on user-visible behavior (new prctl(), new
sysfs knob)
2. Extract changes made along the way that have stand-alone value in
existing code (bug fixes, simplifying/documenting tricky sections,
cleanups).
3. Split out noisy prep work such as renames and refactors that make
the user-visible functional changes more difficult to understand.
and then order the series into sections 2, 3, and 1.
I agree. The most important part is the "logical change" part. Once it's
down to a list of 3 items or so for a single commit we can usually
express it like (just an example that does no longer apply due to
pages_volatile() not being required) the following when combining items
1+2+3 from the list:
"
Let's expose the general KSM profit via sysfs and document that new
toggle. [add details about that and especially why we are doing that]
As we need the number of volatile pages to calculate the general KSM
profit, factor out existing functionality into a helper.
"
Much easier to read.
--
Thanks,
David / dhildenb
