Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.35 as permitted sender) client-ip=23.128.96.35;
Message-ID: <aceadbf1-0345-2b7d-4468-c7c42ac15e03@oracle.com>
Date:   Wed, 27 Sep 2023 15:07:02 +0530
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:102.0) Gecko/20100101
 Thunderbird/102.15.1
Subject: Re: [PATCH next] cgroup/cpuset: Cleanup signedness issue in
 cpu_exclusive_check()
To:     Harshit Mogalapalli <harshit.m.mogalapalli@oracle.com>,
        Waiman Long <longman@redhat.com>,
        Zefan Li <lizefan.x@bytedance.com>, Tejun Heo <tj@kernel.org>,
        Johannes Weiner <hannes@cmpxchg.org>, cgroups@vger.kernel.org,
        linux-kernel@vger.kernel.org
Cc:     dan.carpenter@linaro.org, kernel-janitors@vger.kernel.org,
        error27@gmail.com, kernel test robot <lkp@intel.com>
References: <20230927065801.2139969-1-harshit.m.mogalapalli@oracle.com>
Content-Language: en-US
From:   Kamalesh Babulal <kamalesh.babulal@oracle.com>
In-Reply-To: <20230927065801.2139969-1-harshit.m.mogalapalli@oracle.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?aEZ6ZTRIVUtCQWUrS0pJV2haODV6eG5USzZmTmtwdHZVM2k3WmtRbWc4Y2E0?=
 =?utf-8?B?V1IvclVYTldGTmFKRU8rM21QYzVCUVk3cVBtbTZhZnI1M2JSM016QnVmenlV?=
 =?utf-8?B?Q3JyZW5RaG5ISU04K09HemlxTk5aRHhWa1dUMXpkRHlEYlhkSUp6djZNUG5l?=
 =?utf-8?B?SCtiY2FBRUdOTXVDV0FaMzZ4Smw3SHdQemo4bXM4elJwNnZ5dHFMaGY2RUJ1?=
 =?utf-8?B?QzRuNVBuVFp3QzZPdmlHN09Gck4yeUdRdzVKSnI1Y0xJVVFNTXlTc2VYNWgr?=
 =?utf-8?B?ZVVRek1QN0wrT2xWZzRwR0pyZjIyYTdaa2NkbEd1eFkrQTd3Nm9ySDVVOGxa?=
 =?utf-8?B?aW01MllEWDVEMndwVlFQcE1tM3pVVUQ5MDMwVFFJd0lWaVg3V3dJZWhNY1g4?=
 =?utf-8?B?aml1LzJ1ZGpUaDZMbXlsN2duazVvMGp6QmRhSUE4V05TM21VNzREaTl0THlQ?=
 =?utf-8?B?VVkrODlNZmpRdDIvZEFSVCtSZTNwUm93d0xVeHBGbk9BaldvT3lydlM5LzFM?=
 =?utf-8?B?bFF2aGVsTmhYdnFzR3BOVU1HS3FjcVRUcS9yemRoajg2ckhjdW1RMlpqUEho?=
 =?utf-8?B?T245a05PS3Zhb21ISUV2RVY0azBDb1RSMndISGl5ZGpFL0lINGp4bitINUZz?=
 =?utf-8?B?ZzRRWnVOZ2hmOCt5VTFpSVVQQXl1UkJvMzNzVWhRYjRhSWg3ZHV2MUVSWDNi?=
 =?utf-8?B?Q3F0cU0zdDdoQVJPV1hVS21MZnZ5NyszV0hsYXBWa1BqUk9CV2VOaklGRytZ?=
 =?utf-8?B?MkR2aWpCdFpxd1N4U0JsYzlCTC9BSEtJY3kwZ05HRHNRNnNldmgwZlRsSmVu?=
 =?utf-8?B?SWVQRXZVYlhyc0RHUkJtSUxoMTZDVG1sZ2JQR3lDZUkwRWNiNXNrZ3h2OER6?=
 =?utf-8?B?dVRWT1FUWGFsVmV3NFloSUl4Vk0xMktWc0Z3UG01NFdzQWMwMVlYQ01sQzcz?=
 =?utf-8?B?QkQvYWpLZm85S0RzU3RGaFI3K2RKTVlUQS9Jd3dyQ0gxbTgvSzFOclRCQklE?=
 =?utf-8?B?dC9EQWd1bGdnak40bGtIdDZQUzA0UktCbCtjck1iWWp0QXE4TlVvT3NWNk40?=
 =?utf-8?B?QmdNa01jT0dvRkdmM3hVb09pZVJvRUlDL0NPQ1pDSnRqcE1XNy91N24yWCsx?=
 =?utf-8?B?bENUdnI0QWxpdFBwakpBRWpGcE1pNVVuMkszMklTYUcwMEphbHlTcUh2dXhP?=
 =?utf-8?B?REM4NWptZmhFTDgxblFSRmNqemhWbGQrTWNKY214a2ppUDl6cGphZ3Fmb1E0?=
 =?utf-8?B?SG0xQWxpVkdaVDF1M29MclFnZ2JiYSt2Wmg2dFBmT2dvbFdvKzBUcm5TaHgv?=
 =?utf-8?B?Tm1vOUlDM0xpWVdXMFp0a1dzTjlKWDZHZlhJSm1EQ3picG1KMlBpMXVzQklO?=
 =?utf-8?B?MG1pQVVuUW11MXorcVdmMTlxY1oydk5vY25wQ0JHMEJoQ2hoZ3FoS2RxcjBQ?=
 =?utf-8?B?YkVaT1B4SldNZ0NxL3F2VyttT05UTWoxdWp1ZHlCVUs3VldnL0FLcXlqZ3pp?=
 =?utf-8?B?MFFWZWhpTmZaNU5wYzJOLy8yd0xCeXYrTnVuVktBMzRnNjd0LzBzVlpyNUVl?=
 =?utf-8?B?TngvT2x4dG9mczAvQ0VnaDBtTFpIWFpYOEt2Uk9GUGIxQ2NRUVc0bkY1WXVk?=
 =?utf-8?B?Wm5ZOTEzRFhUUXpaUkd3NDdqQVJvNlVGSFZZckIzVzJaSWFJbGIzT3VZeVhW?=
 =?utf-8?B?YWpXN09JaWw3a0R4SVJDeXRzQytuL1Bjd0x4bXh6Q3pDSVZZZFB2WGRkZ2NP?=
 =?utf-8?B?bXZFOUFFZDVuTW1WNjN6T0wzVkZxM2VSVWdFeHVjbDRKRFJlV09wK05iOE8r?=
 =?utf-8?B?SU1sZkl5TDFKdDJwcWRzN3paamJlaXkySk0wSGFJL2xoMDVEMnVQVWJLdkdh?=
 =?utf-8?B?NmZGcHJSS0Y0dXBkU0tPblZ3VWlub0gwcUwzclRVMm9aMUh2L3UwSUJUVDlI?=
 =?utf-8?B?MnpRSmZWYXhyMUNRcm50c3VZTnNuNEttVkYwUXMvcVJaem5YaEZ1M2tuVGtF?=
 =?utf-8?B?ZHBGZW42amc0SS9NZlJlaitnSW8rdjNYam5rTTZSNGNBWU5LcnlvWGwvb0cw?=
 =?utf-8?B?cWIySGNKSEhRamdTL2RQNk1IUGlaZCtDVUwyZmRHbHcyaVllYTBLWis5RW8w?=
 =?utf-8?B?eUMwTmY3ZHk4R3pyT1VEcXJWNE9ybmtqcnN5TFc5d1JRTHRTTHpCT2xpdFB0?=
 =?utf-8?B?S1E9PQ==?=
X-MS-Exchange-AntiSpam-ExternalHop-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-ExternalHop-MessageData-0: =?utf-8?B?c1cxa0o0dnlZakc4WXAxS01NQVVVN1prM0JDa1N2U01scmw5L3ExT2cvODFt?=
 =?utf-8?B?NkNKRWdleXk3QVVodGh4ckhlSjFFQzVmNHp0cHRMekFoMEVzMzA0K281MDRq?=
 =?utf-8?B?aHhEbUF3MEloOUg1bHgxN01FcFlCN3I3Z3JwZ3F2bm8rTC85bXVuYXdJbmE1?=
 =?utf-8?B?YTgyTGNCNGpFWkxxWjBrSkVkZlpORExicDZmQytEMDFxZVBhMHRFOGltTzF4?=
 =?utf-8?B?ZGY3UHdDSC94QmJQaVo2Zi9vSUJIZTNORDJubkhkNkhRemhPK3hlRkNxakt3?=
 =?utf-8?B?bmJQSU9IbEl0cEhVVkgwdzVraVhqQUNVTFYwNVFKSkJyWkM5VzlCM05GTUFS?=
 =?utf-8?B?MFdEdjN0S3RpQ2dlUmlXdm0rTjZnK1pZQkZzU0ZUbHY4SmxjemtFbkd1MGxK?=
 =?utf-8?B?VVVKZlVUVTlacm5XZzl5RjBsMVZtVHJSdTV1cnRMQ29lck0vZU53byswSGl0?=
 =?utf-8?B?Q2V6UXhGT29kSENDZ3BUeC93bEJHWFpIQmpSZ0htWWpwYzM1bytOWTZNMm9G?=
 =?utf-8?B?bkM3dHUzN2g0VTV0MGhWRnVGN3F4NCtZdzBBdDBNUFdBeVpBWnhZNzQxRk12?=
 =?utf-8?B?RSt4L0x1L1BVRTN5WXFyU2NYUGZlR1NQT3NHRzN3R3E4VDR5RHFkL3FKM3lZ?=
 =?utf-8?B?elMvR0hGa1Y4SXUrWENvR2hxSnZaZ1RSWFBuM2VrVklFeHpmV0tud3kvQ1VY?=
 =?utf-8?B?djUwZ0R1UnhGRjhuc1dkUmZaUDMydytjZ2lxSEh5V0R4ODNFaGZOUVpXSnAx?=
 =?utf-8?B?blhBWHJQWmZFSW1HbHVtMnhZTStFSkJrUzdXNU1BOXB2ckxNd2dkZm55SXpl?=
 =?utf-8?B?MThCRjFxckF5SG5mWXJiWjNpTVlESXBhZHdrVXE5em5LZjJROTRaMEgzWUFP?=
 =?utf-8?B?UWxYSC9ONWhLaHFKenZLWjRxTmtkQXJsdU5NOVJrdUhhR2pCdWVlNis3Nkgy?=
 =?utf-8?B?TnRnU3hHKzBJYlhRRDdpd0dRRFk3NG9TNFlPV3IraHlDSE5kUWtvYXdneDdu?=
 =?utf-8?B?VkRTajUzWDZtbVBvN0Y3eXZFY1lza2RQS0FDakoraEJRYmhFOWNuZVRBZDhl?=
 =?utf-8?B?ZmtRK2h1WVFDWW0rS3lkZVRGcDZUN0c5VFFjZDFDTHV4cWhPb2xVU3QwRXFZ?=
 =?utf-8?B?UHdmVnBkVUlvVjZTT2NRS24raWZWV3JEcXVLcEhWa1BDcTVyOFoyZUkzcS8z?=
 =?utf-8?B?M0lPVlk1TVJUczcwdlBlZGlOTzRwdDkzZ3RyTkZyeE5kWWF5STAzcUJmaG1K?=
 =?utf-8?B?ZmZVcW9LRzBic25Ec1RkRU01SFozQnlad1AxdzZ0OWc1ZVAxUzIrdkZqQ2Jr?=
 =?utf-8?Q?SwaIso6H22INo=3D?=
X-OriginatorOrg: oracle.com
X-MS-Exchange-CrossTenant-Network-Message-Id: 5fab2e67-d46d-4da1-05ff-08dbbf3d5448
X-MS-Exchange-CrossTenant-AuthSource: DM6PR10MB3001.namprd10.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 27 Sep 2023 09:37:13.7045
 (UTC)
X-MS-Exchange-CrossTenant-FromEntityHeader: Hosted
X-MS-Exchange-CrossTenant-Id: 4e2c6054-71cb-48f1-bd6c-3a9705aca71b
X-MS-Exchange-CrossTenant-MailboxType: HOSTED
X-MS-Exchange-CrossTenant-UserPrincipalName: IogUuNzPdC2ojllJ2rwcp/foCuGReM8Da6+YRU2jPHMAQkZAhEQls6tM/0gsNdQcAGqva9tp+Qbqd9x+5N/X0a7me29C4zgXro585vhwj+Y=
X-MS-Exchange-Transport-CrossTenantHeadersStamped: DS0PR10MB7344
X-Proofpoint-Virus-Version: vendor=baseguard
 engine=ICAP:2.0.267,Aquarius:18.0.980,Hydra:6.0.619,FMLib:17.11.176.26
 definitions=2023-09-27_04,2023-09-26_01,2023-05-22_02
X-Proofpoint-Spam-Details: rule=notspam policy=default score=0 spamscore=0 suspectscore=0 phishscore=0
 mlxscore=0 malwarescore=0 mlxlogscore=999 bulkscore=0 adultscore=0
 classifier=spam adjust=0 reason=mlx scancount=1 engine=8.12.0-2309180000
 definitions=main-2309270078
X-Proofpoint-GUID: offEK1AqN4ciYNhVdZj7S1dKOXxakYTH
X-Proofpoint-ORIG-GUID: offEK1AqN4ciYNhVdZj7S1dKOXxakYTH
X-Spam-Status: No, score=-2.3 required=5.0 tests=DKIM_SIGNED,DKIM_VALID,
	DKIM_VALID_AU,HEADER_FROM_DIFFERENT_DOMAINS,MAILING_LIST_MULTI,
	NICE_REPLY_A,SPF_HELO_NONE,SPF_PASS autolearn=unavailable
	autolearn_force=no version=3.4.6
X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on groat.vger.email
Precedence: bulk
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org
X-Greylist: Sender passed SPF test, not delayed by milter-greylist-4.6.4 (groat.vger.email [0.0.0.0]); Wed, 27 Sep 2023 02:39:02 -0700 (PDT)


On 9/27/23 12:28, Harshit Mogalapalli wrote:
> Smatch complains about returning negative error codes from a type
> bool function.
> 
> kernel/cgroup/cpuset.c:705 cpu_exclusive_check() warn:
> 	signedness bug returning '(-22)'
> 
> The code works correctly, but it is confusing.  The current behavior is
> that cpu_exclusive_check() returns true if it's *NOT* exclusive.  Rename
> it to cpusets_are_exclusive() and reverse the returns so it returns true
> if it is exclusive and false if it's not.  Update both callers as well.
> 
> Reported-by: kernel test robot <lkp@intel.com>
> Reported-by: Dan Carpenter <error27@gmail.com>
> Closes: https://lore.kernel.org/r/202309201706.2LhKdM6o-lkp@intel.com/
> Signed-off-by: Harshit Mogalapalli <harshit.m.mogalapalli@oracle.com>

The patch looks good to me, returning true on exclusive cpusets is more
intuitive. Renaming cpu_exclusive_check() to is_cpuset_exclusive() is
one other option, though, there is is_cpu_exclusive() function, which
sounds similar, and tests for the cpu exclusive bit in a given cpuset's
flag.

I don't have a strong opinion between the original function name and
the proposed rename.

Reviewed-by: Kamalesh Babulal <kamalesh.babulal@oracle.com>

> ---
> This is based on sattic analysis, only compile tested
> ---
>  kernel/cgroup/cpuset.c | 14 +++++++-------
>  1 file changed, 7 insertions(+), 7 deletions(-)
> 
> diff --git a/kernel/cgroup/cpuset.c b/kernel/cgroup/cpuset.c
> index 15f399153a2e..afefddd33c3e 100644
> --- a/kernel/cgroup/cpuset.c
> +++ b/kernel/cgroup/cpuset.c
> @@ -719,18 +719,18 @@ static inline struct cpumask *fetch_xcpus(struct cpuset *cs)
>  }
>  
>  /*
> - * cpu_exclusive_check() - check if two cpusets are exclusive
> + * cpusets_are_exclusive() - check if two cpusets are exclusive
>   *
> - * Return 0 if exclusive, -EINVAL if not
> + * Return true if exclusive, false if not
>   */
> -static inline bool cpu_exclusive_check(struct cpuset *cs1, struct cpuset *cs2)
> +static inline bool cpusets_are_exclusive(struct cpuset *cs1, struct cpuset *cs2)
>  {
>  	struct cpumask *xcpus1 = fetch_xcpus(cs1);
>  	struct cpumask *xcpus2 = fetch_xcpus(cs2);
>  
>  	if (cpumask_intersects(xcpus1, xcpus2))
> -		return -EINVAL;
> -	return 0;
> +		return false;
> +	return true;
>  }
>  
>  /*
> @@ -833,7 +833,7 @@ static int validate_change(struct cpuset *cur, struct cpuset *trial)
>  	cpuset_for_each_child(c, css, par) {
>  		if ((is_cpu_exclusive(trial) || is_cpu_exclusive(c)) &&
>  		    c != cur) {
> -			if (cpu_exclusive_check(trial, c))
> +			if (!cpusets_are_exclusive(trial, c))
>  				goto out;
>  		}
>  		if ((is_mem_exclusive(trial) || is_mem_exclusive(c)) &&
> @@ -1864,7 +1864,7 @@ static int update_parent_effective_cpumask(struct cpuset *cs, int cmd,
>  			cpuset_for_each_child(child, css, parent) {
>  				if (child == cs)
>  					continue;
> -				if (cpu_exclusive_check(cs, child)) {
> +				if (!cpusets_are_exclusive(cs, child)) {
>  					exclusive = false;
>  					break;
>  				}

-- 
Thanks,
Kamalesh