Message-ID: <dfd74b29-b68e-588a-b8f5-5ca7ff216819@intel.com>
Date:   Thu, 6 Jan 2022 15:48:39 -0800
User-Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:91.0) Gecko/20100101
 Firefox/91.0 Thunderbird/91.4.0
Subject: Re: [PATCH v2 2/5] selftests/resctrl: Make resctrl_tests run using
 kselftest framework
Content-Language: en-US
To:     Shaopeng Tan <tan.shaopeng@jp.fujitsu.com>,
        Fenghua Yu <fenghua.yu@intel.com>,
        Shuah Khan <shuah@kernel.org>
CC:     <linux-kernel@vger.kernel.org>, <linux-kselftest@vger.kernel.org>
References: <20211213100154.180599-1-tan.shaopeng@jp.fujitsu.com>
 <20211213100154.180599-3-tan.shaopeng@jp.fujitsu.com>
From:   Reinette Chatre <reinette.chatre@intel.com>
In-Reply-To: <20211213100154.180599-3-tan.shaopeng@jp.fujitsu.com>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: 7bit
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0: =?utf-8?B?K1RscnBURTBvK2I1Mm1OOW5aTzQreWJOZTlFNHp2VXRGZFhTSHhHeWZuZFpE?=
 =?utf-8?B?WC9BNHhRUmk0SkJHWG11VTdqWDBBMWYvY0tkQVk1UFVubit6S08zOXlOUTg1?=
 =?utf-8?B?TC9Rd0hQK0l6NlpMYmFPVkhibmorQVgra3NnTWZhRCttYWpIWEdnNFd3dDV3?=
 =?utf-8?B?TDdvVENvUWdnamdQckNDbDBkT253VXRBOXRyb0VUTDFTUlBkM2ZQajFHM2Rv?=
 =?utf-8?B?M1VHWGVZUzFBQzByNFhvVDQ1RWFQWTF2a21lN3BVTCtZdG52VnJ6dnRZRXdC?=
 =?utf-8?B?eHZlT2F4SDdjVWdJelhleHJaS2xYblF5UFpLamJuZ25BcExJa2Q1MHNHRFVC?=
 =?utf-8?B?c1ZpYlUrZVE1bkFkTnhPUjR3SCtzNWJ6Z2Zid2g0MFl5NEhmRUdDOXBXVEJu?=
 =?utf-8?B?RkVhWnRsTG5XaFo3K2VxYXBwYmdwVnRKbWlNUDJRL0wrU0xQZ0Jna1QxNjh5?=
 =?utf-8?B?OUczREs4MThjbDMwdTdpSms0QUVCZEtndzZrQi9RTWMwem91eEg5bFBGa1VX?=
 =?utf-8?B?ZHd2bW5XT1A3UkR2NlpRYXBDeGphOVYyVmFwY2ZxSFB3d3V6TFAxdGVwTmx4?=
 =?utf-8?B?TzZlS3A1bkQ2bkVIZ2hqOGRiQnM5TzBsMFpBTktHZkpSVEVjc1J2Z2JlL3Bj?=
 =?utf-8?B?akkyQ2lhM2ZDSm1IbHhpTHNQblFNUmp0dmlJQTBTakMrb0tacFBmRkYweHFn?=
 =?utf-8?B?ek96UitEUXBVcDducS9EeWtIRGRtNGFlcWRySGRZV3ZLWFpRL21IMWVEN1A3?=
 =?utf-8?B?OUtwOHFaRjN0TmIwQkd5bmoybEhkak5DZ1pjcmlXc0RDMG5FYW42dDRrRUVu?=
 =?utf-8?B?d1JZUHdUcFZiUnl4WGh4aGl4ZVVlMXdkaFExN2xxZFBUdVJFKzFKVFMwMGJm?=
 =?utf-8?B?SUdFYXB6eDNPcXVNaEhDVnU0blVXL3hTOGZnRHphY3B4UU1wcTNyM3lPY1ZP?=
 =?utf-8?B?eC9kTFFnVU9wYkY5ZG5mZy9HaGJLSFh2NlFYenk1YkE1TVRka3pXSk55NUx2?=
 =?utf-8?B?SDRuM2txRVZSVkdRNGc3N2pwNUZFSE1SNjJySWxIb2RMMm1yc05SejA1bE9L?=
 =?utf-8?B?dnZVbWVma2duRWRnM3Zkdm8zbWRtUFRaSTN6bFVnZW9yeG5sTkFuKzZWbXFT?=
 =?utf-8?B?WXZRMHdtaDVKd0ZabTBkM1hzR3dJNDcxMnlic3U4YlZyak8vNWFWMjJwV1Uz?=
 =?utf-8?B?K2J3UkZDQ2RHaEJEclZtMkVEcFY1UUs2Tkt0L1NOVzJyeGoxbkNDN3F1RHZD?=
 =?utf-8?B?QU5EYWtPdmNFYWZsZWRKWGJYditrOHMzL1htMEVQTDRYRG9qN0JCb1ZMTkt3?=
 =?utf-8?B?Sm9sU3hFUVltZWh5OGlEazVHMjdxcFM1amptRkFFREIzZ1FuVUd6WnNFUlZ3?=
 =?utf-8?B?c0t2ck44VUEvZzVWSCtsL0xKRm5ESHVDZmhFdEdoUyt2aVF2MTJ5anhtdnVv?=
 =?utf-8?B?UzBmai84RW1TbmZ5UUc0d0lMQUJzUDI1VzI3Ym1SRlZCTDZtNjkzN1pSSUtw?=
 =?utf-8?B?N2ovcHRLZmo1Vk84UGx0MUJIYWI3WksrOU9kNFJCYWJJWE9ibldodWJXL1R2?=
 =?utf-8?B?cys2ZnYwTWltRllnV1p1R2p3NFA3WmFzUkJFRUpiRlpVTnJ5VlVkUEZUbkNh?=
 =?utf-8?B?UlRPbDZSNFJBTlBNOFlnRnE4MHBPZGN1QVlnUEkza0NKSllzN1paa2p4SEts?=
 =?utf-8?B?bmxObFUyZmJGN0k4MVhQZFIvNWhsRnBJMGx5NUdzMnJEbjN1US9RWnNaMHUv?=
 =?utf-8?B?UDk0SjNTaGZ4MitDSTExZzNncXhRK01zUkd6V01SODdEMEZpSlFTZ1FqZFFp?=
 =?utf-8?B?VXkwYnFYUnFYbEpwZ0U1K01qeENOUE9vZXZxcTA4cm9ZbVA1K3U2eVYwN09r?=
 =?utf-8?B?VmtoL01QSlRZbXFWWS9lc0o3S2drTzlBOVh5UmZpNE1RcFY1a0V6OStoY3ZK?=
 =?utf-8?B?SDhVUEJzR2hSWGVacTdtSk5YTTJXczFBdEZmdCt5ZDBwKzR2VlplMFlqZUtF?=
 =?utf-8?B?RE5peDRiR3pLdlR0eElOQnRyOG1QT25kb2p4WHpScmhNa3RpMWhvL3dCTFVH?=
 =?utf-8?B?dUVjM0g1WXdHTlVRRk9VYVhjN1NJUmhzeVpZM3htaXhFN2FBY1o4UlhuU2Za?=
 =?utf-8?B?OTJscHREcDlsK213ZEdLWTBHenJVNG5HamdDN3FSNWNlSUt1ZXFMOFJZS29G?=
 =?utf-8?B?MGJUT2daRlozTExrMWRjVXY4K3BqTVJsb1g1VlpMSjhFVmNubGhIVEhiazho?=
 =?utf-8?Q?yb+wGsgKIc85iuqWVgdMS3w0J940mLKW8U3H3O3KvE=3D?=
X-MS-Exchange-CrossTenant-Network-Message-Id: 17d5312e-8ea9-4d91-ffd3-08d9d16f1202
X-MS-Exchange-CrossTenant-AuthSource: BN0PR11MB5744.namprd11.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 06 Jan 2022 23:48:42.6209
 (UTC)
X-MS-Exchange-CrossTenant-FromEntityHeader: Hosted
X-MS-Exchange-CrossTenant-Id: 46c98d88-e344-4ed4-8496-4ed7712e255d
X-MS-Exchange-CrossTenant-MailboxType: HOSTED
X-MS-Exchange-CrossTenant-UserPrincipalName: 1sYxEDCCswjRXf92ks1K/BB50XRhpHY2Wv9V6kk9iIaOGisBSp4qDncK8+ICKzjizBzqoSAAGOz6fA3Uph01plrlHVYDWKKacMIQKNTNRKg=
X-MS-Exchange-Transport-CrossTenantHeadersStamped: BN9PR11MB5548
X-OriginatorOrg: intel.com
Precedence: bulk
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org

Hi Shaopeng Tan,

On 12/13/2021 2:01 AM, Shaopeng Tan wrote:
> This commit enables resctrl_tests to be built/run in kselftest framework.

(This commit)

> Build/run resctrl_tests by building/running all tests of kselftest, or by using
> the "TARGETS" variable on the make command line to specify resctrl_tests.

Please review the feedback I provided to your first version. I do not
see the changelog improvements that describe how a user may use the
kselftest framework to run the resctrl tests nor the requested information
on how existing workflows are impacted. Going back through my previous review
feedback to ensure that all is addressed is very time consuming and causes
considerable delay when needing to review newer versions. 

tools/testing/selftests/resctrl/README should be updated also

> This commit modified the Makefile of kernel kselftest set and
> the Makefile of resctrl_tests.

This is clear from the diffstat. You could elaborate why the changes were
needed instead and do so without the "This commit" usage.

> 
> Signed-off-by: Shaopeng Tan <tan.shaopeng@jp.fujitsu.com>
> ---
>  tools/testing/selftests/Makefile         |  1 +
>  tools/testing/selftests/resctrl/Makefile | 20 ++++++--------------
>  2 files changed, 7 insertions(+), 14 deletions(-)
> 
> diff --git a/tools/testing/selftests/Makefile b/tools/testing/selftests/Makefile
> index c852eb40c4f7..7df397c6893c 100644
> --- a/tools/testing/selftests/Makefile
> +++ b/tools/testing/selftests/Makefile
> @@ -51,6 +51,7 @@ TARGETS += proc
>  TARGETS += pstore
>  TARGETS += ptrace
>  TARGETS += openat2
> +TARGETS += resctrl
>  TARGETS += rlimits
>  TARGETS += rseq
>  TARGETS += rtc
> diff --git a/tools/testing/selftests/resctrl/Makefile b/tools/testing/selftests/resctrl/Makefile
> index 6bcee2ec91a9..c9e8540fc594 100644
> --- a/tools/testing/selftests/resctrl/Makefile
> +++ b/tools/testing/selftests/resctrl/Makefile
> @@ -1,17 +1,9 @@
> -CC = $(CROSS_COMPILE)gcc
> -CFLAGS = -g -Wall -O2 -D_FORTIFY_SOURCE=2
> -SRCS=$(wildcard *.c)
> -OBJS=$(SRCS:.c=.o)
> +CFLAGS += -g -Wall -O2 -D_FORTIFY_SOURCE=2
>  
> -all: resctrl_tests
> +TEST_GEN_PROGS := resctrl_tests
> +EXTRA_SOURCES := $(wildcard *.c)

Why is the meaning of "EXTRA_SOURCES" (i.e. what is "extra"?) and
why is "SRCS" no longer sufficient?

>  
> -$(OBJS): $(SRCS)
> -	$(CC) $(CFLAGS) -c $(SRCS)
> +all: $(TEST_GEN_PROGS)
>  
> -resctrl_tests: $(OBJS)
> -	$(CC) $(CFLAGS) -o $@ $^
> -
> -.PHONY: clean
> -
> -clean:
> -	$(RM) $(OBJS) resctrl_tests
> +$(TEST_GEN_PROGS): $(EXTRA_SOURCES)
> +include ../lib.mk

Reinette