[PATCH] RDMA/hfi: add a judgment on the availability of cpumask

[PATCH] RDMA/hfi: add a judgment on the availability of cpumask

[xiaolinkui]

From: Linkui Xiao <xiaolinkui@kylinos.cn>

When CONFIG_CPUMASK_OFFSTACK is n, cpumask may fail to allocate, cpumask may
be NULL, and performing a bitmap operation on cpumask may cause problems at
this time.

Of course, this is a unlikely event.

Signed-off-by: Linkui Xiao <xiaolinkui@kylinos.cn>
---
 drivers/infiniband/hw/hfi1/affinity.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/drivers/infiniband/hw/hfi1/affinity.c b/drivers/infiniband/hw/hfi1/affinity.c
index 77ee77d4000f..3caa861f4d1d 100644
--- a/drivers/infiniband/hw/hfi1/affinity.c
+++ b/drivers/infiniband/hw/hfi1/affinity.c
@@ -1047,16 +1047,16 @@ int hfi1_get_proc_affinity(int node)
 	 */
 
 	ret = zalloc_cpumask_var(&diff, GFP_KERNEL);
-	if (!ret)
+	if (!ret || unlikely(!diff))
 		goto done;
 	ret = zalloc_cpumask_var(&hw_thread_mask, GFP_KERNEL);
-	if (!ret)
+	if (!ret || unlikely(!hw_thread_mask))
 		goto free_diff;
 	ret = zalloc_cpumask_var(&available_mask, GFP_KERNEL);
-	if (!ret)
+	if (!ret || unlikely(!available_mask))
 		goto free_hw_thread_mask;
 	ret = zalloc_cpumask_var(&intrs_mask, GFP_KERNEL);
-	if (!ret)
+	if (!ret || unlikely(!intrs_mask))
 		goto free_available_mask;
 
 	mutex_lock(&affinity->lock);
-- 
2.17.1

Re: [PATCH] RDMA/hfi: add a judgment on the availability of cpumask

[Leon Romanovsky]

On Tue, Apr 04, 2023 at 11:05:25AM +0800, xiaolinkui wrote:
> From: Linkui Xiao <xiaolinkui@kylinos.cn>
> 
> When CONFIG_CPUMASK_OFFSTACK is n, cpumask may fail to allocate, cpumask may
> be NULL, and performing a bitmap operation on cpumask may cause problems at
> this time.
> 
> Of course, this is a unlikely event.
> 
> Signed-off-by: Linkui Xiao <xiaolinkui@kylinos.cn>
> ---
>  drivers/infiniband/hw/hfi1/affinity.c | 8 ++++----
>  1 file changed, 4 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/infiniband/hw/hfi1/affinity.c b/drivers/infiniband/hw/hfi1/affinity.c
> index 77ee77d4000f..3caa861f4d1d 100644
> --- a/drivers/infiniband/hw/hfi1/affinity.c
> +++ b/drivers/infiniband/hw/hfi1/affinity.c
> @@ -1047,16 +1047,16 @@ int hfi1_get_proc_affinity(int node)
>  	 */
>  
>  	ret = zalloc_cpumask_var(&diff, GFP_KERNEL);
> -	if (!ret)
> +	if (!ret || unlikely(!diff))

Why do you think that check of "ret" is not enough?
"ret" will be false if diff == NULL.

Thanks

Re: [PATCH] RDMA/hfi: add a judgment on the availability of cpumask

[xiaolinkui]

Thanks for your reply.

When CONFIG_CPUMASK_OFFSTACK=y, "ret" will be false if diff==NULL.

However, when CONFIG_CPUMASK_OFFSTACK=n, these two are not necessarily 
equivalent.

Thanks

On 4/4/23 14:05, Leon Romanovsky wrote:
> On Tue, Apr 04, 2023 at 11:05:25AM +0800, xiaolinkui wrote:
>> From: Linkui Xiao <xiaolinkui@kylinos.cn>
>>
>> When CONFIG_CPUMASK_OFFSTACK is n, cpumask may fail to allocate, cpumask may
>> be NULL, and performing a bitmap operation on cpumask may cause problems at
>> this time.
>>
>> Of course, this is a unlikely event.
>>
>> Signed-off-by: Linkui Xiao <xiaolinkui@kylinos.cn>
>> ---
>>   drivers/infiniband/hw/hfi1/affinity.c | 8 ++++----
>>   1 file changed, 4 insertions(+), 4 deletions(-)
>>
>> diff --git a/drivers/infiniband/hw/hfi1/affinity.c b/drivers/infiniband/hw/hfi1/affinity.c
>> index 77ee77d4000f..3caa861f4d1d 100644
>> --- a/drivers/infiniband/hw/hfi1/affinity.c
>> +++ b/drivers/infiniband/hw/hfi1/affinity.c
>> @@ -1047,16 +1047,16 @@ int hfi1_get_proc_affinity(int node)
>>   	 */
>>   
>>   	ret = zalloc_cpumask_var(&diff, GFP_KERNEL);
>> -	if (!ret)
>> +	if (!ret || unlikely(!diff))
> Why do you think that check of "ret" is not enough?
> "ret" will be false if diff == NULL.
>
> Thanks

Re: [PATCH] RDMA/hfi: add a judgment on the availability of cpumask

[Leon Romanovsky]

On Tue, Apr 04, 2023 at 03:01:58PM +0800, xiaolinkui wrote:
> Thanks for your reply.
> 
> When CONFIG_CPUMASK_OFFSTACK=y, "ret" will be false if diff==NULL.
> 
> However, when CONFIG_CPUMASK_OFFSTACK=n, these two are not necessarily
> equivalent.

And what is the problem with that? cpumask_* API is built to handle
correctly this mode.

Reminder: please don't top post.

> 
> Thanks

Re: [PATCH] RDMA/hfi: add a judgment on the availability of cpumask

[kernel test robot]

Hi xiaolinkui,

kernel test robot noticed the following build warnings:

[auto build test WARNING on rdma/for-next]
[also build test WARNING on linus/master v6.3-rc5 next-20230406]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]

url:    https://github.com/intel-lab-lkp/linux/commits/xiaolinkui/RDMA-hfi-add-a-judgment-on-the-availability-of-cpumask/20230404-113847
base:   https://git.kernel.org/pub/scm/linux/kernel/git/rdma/rdma.git for-next
patch link:    https://lore.kernel.org/r/20230404030525.24020-1-xiaolinkui%40126.com
patch subject: [PATCH] RDMA/hfi: add a judgment on the availability of cpumask
config: x86_64-randconfig-a003-20220117 (https://download.01.org/0day-ci/archive/20230407/202304071125.xK1fezQ1-lkp@intel.com/config)
compiler: gcc-11 (Debian 11.3.0-8) 11.3.0
reproduce (this is a W=1 build):
        # https://github.com/intel-lab-lkp/linux/commit/206c6fc9aa5afd354f4201216ca8c2c0057fb49d
        git remote add linux-review https://github.com/intel-lab-lkp/linux
        git fetch --no-tags linux-review xiaolinkui/RDMA-hfi-add-a-judgment-on-the-availability-of-cpumask/20230404-113847
        git checkout 206c6fc9aa5afd354f4201216ca8c2c0057fb49d
        # save the config file
        mkdir build_dir && cp config build_dir/.config
        make W=1 O=build_dir ARCH=x86_64 olddefconfig
        make W=1 O=build_dir ARCH=x86_64 SHELL=/bin/bash drivers/infiniband/hw/hfi1/

If you fix the issue, kindly add following tag where applicable
| Reported-by: kernel test robot <lkp@intel.com>
| Link: https://lore.kernel.org/oe-kbuild-all/202304071125.xK1fezQ1-lkp@intel.com/

All warnings (new ones prefixed by >>):

   In file included from include/asm-generic/bug.h:5,
                    from arch/x86/include/asm/bug.h:87,
                    from include/linux/bug.h:5,
                    from include/linux/mmdebug.h:5,
                    from include/linux/percpu.h:5,
                    from include/linux/arch_topology.h:9,
                    from include/linux/topology.h:30,
                    from drivers/infiniband/hw/hfi1/affinity.c:6:
   drivers/infiniband/hw/hfi1/affinity.c: In function 'hfi1_get_proc_affinity':
>> drivers/infiniband/hw/hfi1/affinity.c:1050:30: warning: the address of 'diff' will always evaluate as 'true' [-Waddress]
    1050 |         if (!ret || unlikely(!diff))
         |                              ^
   include/linux/compiler.h:78:45: note: in definition of macro 'unlikely'
      78 | # define unlikely(x)    __builtin_expect(!!(x), 0)
         |                                             ^
>> drivers/infiniband/hw/hfi1/affinity.c:1053:30: warning: the address of 'hw_thread_mask' will always evaluate as 'true' [-Waddress]
    1053 |         if (!ret || unlikely(!hw_thread_mask))
         |                              ^
   include/linux/compiler.h:78:45: note: in definition of macro 'unlikely'
      78 | # define unlikely(x)    __builtin_expect(!!(x), 0)
         |                                             ^
>> drivers/infiniband/hw/hfi1/affinity.c:1056:30: warning: the address of 'available_mask' will always evaluate as 'true' [-Waddress]
    1056 |         if (!ret || unlikely(!available_mask))
         |                              ^
   include/linux/compiler.h:78:45: note: in definition of macro 'unlikely'
      78 | # define unlikely(x)    __builtin_expect(!!(x), 0)
         |                                             ^
>> drivers/infiniband/hw/hfi1/affinity.c:1059:30: warning: the address of 'intrs_mask' will always evaluate as 'true' [-Waddress]
    1059 |         if (!ret || unlikely(!intrs_mask))
         |                              ^
   include/linux/compiler.h:78:45: note: in definition of macro 'unlikely'
      78 | # define unlikely(x)    __builtin_expect(!!(x), 0)
         |                                             ^


vim +1050 drivers/infiniband/hw/hfi1/affinity.c

   995	
   996	int hfi1_get_proc_affinity(int node)
   997	{
   998		int cpu = -1, ret, i;
   999		struct hfi1_affinity_node *entry;
  1000		cpumask_var_t diff, hw_thread_mask, available_mask, intrs_mask;
  1001		const struct cpumask *node_mask,
  1002			*proc_mask = current->cpus_ptr;
  1003		struct hfi1_affinity_node_list *affinity = &node_affinity;
  1004		struct cpu_mask_set *set = &affinity->proc;
  1005	
  1006		/*
  1007		 * check whether process/context affinity has already
  1008		 * been set
  1009		 */
  1010		if (current->nr_cpus_allowed == 1) {
  1011			hfi1_cdbg(PROC, "PID %u %s affinity set to CPU %*pbl",
  1012				  current->pid, current->comm,
  1013				  cpumask_pr_args(proc_mask));
  1014			/*
  1015			 * Mark the pre-set CPU as used. This is atomic so we don't
  1016			 * need the lock
  1017			 */
  1018			cpu = cpumask_first(proc_mask);
  1019			cpumask_set_cpu(cpu, &set->used);
  1020			goto done;
  1021		} else if (current->nr_cpus_allowed < cpumask_weight(&set->mask)) {
  1022			hfi1_cdbg(PROC, "PID %u %s affinity set to CPU set(s) %*pbl",
  1023				  current->pid, current->comm,
  1024				  cpumask_pr_args(proc_mask));
  1025			goto done;
  1026		}
  1027	
  1028		/*
  1029		 * The process does not have a preset CPU affinity so find one to
  1030		 * recommend using the following algorithm:
  1031		 *
  1032		 * For each user process that is opening a context on HFI Y:
  1033		 *  a) If all cores are filled, reinitialize the bitmask
  1034		 *  b) Fill real cores first, then HT cores (First set of HT
  1035		 *     cores on all physical cores, then second set of HT core,
  1036		 *     and, so on) in the following order:
  1037		 *
  1038		 *     1. Same NUMA node as HFI Y and not running an IRQ
  1039		 *        handler
  1040		 *     2. Same NUMA node as HFI Y and running an IRQ handler
  1041		 *     3. Different NUMA node to HFI Y and not running an IRQ
  1042		 *        handler
  1043		 *     4. Different NUMA node to HFI Y and running an IRQ
  1044		 *        handler
  1045		 *  c) Mark core as filled in the bitmask. As user processes are
  1046		 *     done, clear cores from the bitmask.
  1047		 */
  1048	
  1049		ret = zalloc_cpumask_var(&diff, GFP_KERNEL);
> 1050		if (!ret || unlikely(!diff))
  1051			goto done;
  1052		ret = zalloc_cpumask_var(&hw_thread_mask, GFP_KERNEL);
> 1053		if (!ret || unlikely(!hw_thread_mask))
  1054			goto free_diff;
  1055		ret = zalloc_cpumask_var(&available_mask, GFP_KERNEL);
> 1056		if (!ret || unlikely(!available_mask))
  1057			goto free_hw_thread_mask;
  1058		ret = zalloc_cpumask_var(&intrs_mask, GFP_KERNEL);
> 1059		if (!ret || unlikely(!intrs_mask))
  1060			goto free_available_mask;
  1061	
  1062		mutex_lock(&affinity->lock);
  1063		/*
  1064		 * If we've used all available HW threads, clear the mask and start
  1065		 * overloading.
  1066		 */
  1067		_cpu_mask_set_gen_inc(set);
  1068	
  1069		/*
  1070		 * If NUMA node has CPUs used by interrupt handlers, include them in the
  1071		 * interrupt handler mask.
  1072		 */
  1073		entry = node_affinity_lookup(node);
  1074		if (entry) {
  1075			cpumask_copy(intrs_mask, (entry->def_intr.gen ?
  1076						  &entry->def_intr.mask :
  1077						  &entry->def_intr.used));
  1078			cpumask_or(intrs_mask, intrs_mask, (entry->rcv_intr.gen ?
  1079							    &entry->rcv_intr.mask :
  1080							    &entry->rcv_intr.used));
  1081			cpumask_or(intrs_mask, intrs_mask, &entry->general_intr_mask);
  1082		}
  1083		hfi1_cdbg(PROC, "CPUs used by interrupts: %*pbl",
  1084			  cpumask_pr_args(intrs_mask));
  1085	
  1086		cpumask_copy(hw_thread_mask, &set->mask);
  1087	
  1088		/*
  1089		 * If HT cores are enabled, identify which HW threads within the
  1090		 * physical cores should be used.
  1091		 */
  1092		if (affinity->num_core_siblings > 0) {
  1093			for (i = 0; i < affinity->num_core_siblings; i++) {
  1094				find_hw_thread_mask(i, hw_thread_mask, affinity);
  1095	
  1096				/*
  1097				 * If there's at least one available core for this HW
  1098				 * thread number, stop looking for a core.
  1099				 *
  1100				 * diff will always be not empty at least once in this
  1101				 * loop as the used mask gets reset when
  1102				 * (set->mask == set->used) before this loop.
  1103				 */
  1104				cpumask_andnot(diff, hw_thread_mask, &set->used);
  1105				if (!cpumask_empty(diff))
  1106					break;
  1107			}
  1108		}
  1109		hfi1_cdbg(PROC, "Same available HW thread on all physical CPUs: %*pbl",
  1110			  cpumask_pr_args(hw_thread_mask));
  1111	
  1112		node_mask = cpumask_of_node(node);
  1113		hfi1_cdbg(PROC, "Device on NUMA %u, CPUs %*pbl", node,
  1114			  cpumask_pr_args(node_mask));
  1115	
  1116		/* Get cpumask of available CPUs on preferred NUMA */
  1117		cpumask_and(available_mask, hw_thread_mask, node_mask);
  1118		cpumask_andnot(available_mask, available_mask, &set->used);
  1119		hfi1_cdbg(PROC, "Available CPUs on NUMA %u: %*pbl", node,
  1120			  cpumask_pr_args(available_mask));
  1121	
  1122		/*
  1123		 * At first, we don't want to place processes on the same
  1124		 * CPUs as interrupt handlers. Then, CPUs running interrupt
  1125		 * handlers are used.
  1126		 *
  1127		 * 1) If diff is not empty, then there are CPUs not running
  1128		 *    non-interrupt handlers available, so diff gets copied
  1129		 *    over to available_mask.
  1130		 * 2) If diff is empty, then all CPUs not running interrupt
  1131		 *    handlers are taken, so available_mask contains all
  1132		 *    available CPUs running interrupt handlers.
  1133		 * 3) If available_mask is empty, then all CPUs on the
  1134		 *    preferred NUMA node are taken, so other NUMA nodes are
  1135		 *    used for process assignments using the same method as
  1136		 *    the preferred NUMA node.
  1137		 */
  1138		cpumask_andnot(diff, available_mask, intrs_mask);
  1139		if (!cpumask_empty(diff))
  1140			cpumask_copy(available_mask, diff);
  1141	
  1142		/* If we don't have CPUs on the preferred node, use other NUMA nodes */
  1143		if (cpumask_empty(available_mask)) {
  1144			cpumask_andnot(available_mask, hw_thread_mask, &set->used);
  1145			/* Excluding preferred NUMA cores */
  1146			cpumask_andnot(available_mask, available_mask, node_mask);
  1147			hfi1_cdbg(PROC,
  1148				  "Preferred NUMA node cores are taken, cores available in other NUMA nodes: %*pbl",
  1149				  cpumask_pr_args(available_mask));
  1150	
  1151			/*
  1152			 * At first, we don't want to place processes on the same
  1153			 * CPUs as interrupt handlers.
  1154			 */
  1155			cpumask_andnot(diff, available_mask, intrs_mask);
  1156			if (!cpumask_empty(diff))
  1157				cpumask_copy(available_mask, diff);
  1158		}
  1159		hfi1_cdbg(PROC, "Possible CPUs for process: %*pbl",
  1160			  cpumask_pr_args(available_mask));
  1161	
  1162		cpu = cpumask_first(available_mask);
  1163		if (cpu >= nr_cpu_ids) /* empty */
  1164			cpu = -1;
  1165		else
  1166			cpumask_set_cpu(cpu, &set->used);
  1167	
  1168		mutex_unlock(&affinity->lock);
  1169		hfi1_cdbg(PROC, "Process assigned to CPU %d", cpu);
  1170	
  1171		free_cpumask_var(intrs_mask);
  1172	free_available_mask:
  1173		free_cpumask_var(available_mask);
  1174	free_hw_thread_mask:
  1175		free_cpumask_var(hw_thread_mask);
  1176	free_diff:
  1177		free_cpumask_var(diff);
  1178	done:
  1179		return cpu;
  1180	}
  1181	

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests