Hi Reinette,
On Wed, 2020-03-11 at 13:22 -0700, Reinette Chatre wrote:
Hi Sai,
On 3/11/2020 12:14 PM, Sai Praneeth Prakhya wrote:
On Wed, 2020-03-11 at 10:03 -0700, Reinette Chatre wrote:
On 3/10/2020 6:59 PM, Sai Praneeth Prakhya wrote:
On Tue, 2020-03-10 at 15:14 -0700, Reinette Chatre wrote:
Hi Sai,
[SNIP]
Please let me know if you think otherwise
I think this patch can be split up into logical changes without breaking the tests along the way. In my original review I identified two changes that can be split out. Other things that can be split out:
- have CAT test take shareable bits into account
- enable measurement of cache references (addition of this new perf
event attribute, hooks to get measurements, etc.)
- transition CAT test to use "perf rate" measurement instead of "perf
count"
- etc.
I think we could split the patch like this but I am unable to see the benefit of doing so.. (Sorry! if I misunderstood what you meant).
Separating patches into logical changes facilitates review. Please consider this huge patch from the reviewer's perspective - it consists out of many different changes and is hard to review. If instead this patch was split into logical changes it would make it easier to understand what it is trying to do/change.
Ok.. makes sense.
This is not a request that I invent but part of the established kernel development process. Please see Documentation/process/submitting-patches.rst (section is titled "Separate your changes").
Sure! will take a look at it.
As CAT and CQM test cases are buggy (CAT is not testing CAT at all) and we are not attempting to fix them by incremental changes but completely changing the test plan itself (i.e. the way the test works), so why not just remove older test cases and add new test? I thought this might be more easier for review i.e. to see the new test case all at once. Don't you think so?
From what I understand the new test continues to use many parts of the original test. Completely removing the original test would thus end up needing to add back a lot of code that was removed. Incremental changes do seem appropriate to me. The logical changes I listed above actually has nothing to do with "the way the test works". When those building blocks are in place the test can be changed in one patch and it would be much more obvious how the new test is different from the original.
Ok.. makes sense. Will split the patch as you suggested.
Regards, Sai