Srikar Dronamraju <sri...@linux.vnet.ibm.com> writes: > * Michael Ellerman <m...@ellerman.id.au> [2020-07-31 17:52:15]: > >> Srikar Dronamraju <sri...@linux.vnet.ibm.com> writes: >> > If allocated earlier and the search fails, then cpumask need to be >> > freed. However cpu_l1_cache_map can be allocated after we search thread >> > group. >> >> It's not freed anywhere AFAICS? > > Yes, its never freed. Infact we are never checking if > zalloc_cpumask_var_node fails. Its not just this cpumask, but historically > all the other existing cpumasks in arch/powerpc/kernel/smp.c are never > freed/checked. I did dig into this a bit and it appears that .. > (Please do correct me if I am wrong!! )
That's correct. > Powerpc using cpumask_var_t for all of the percpu variables. And it dont seem > to enable CONFIG_CPUMASK_OFFSTACK even from the MAXSMP config. I remember Rusty adding that code, but I don't know if we ever considered enabling CPUMASK_OFFSTACK. Probably we meant to but never got around to doing it. > So from include/linux/cpumask.h > > typedef struct cpumask cpumask_var_t[1]; > and > zalloc_cpumask_var_node ends up being cpumask_clear > > So I think we are historically we seem to assume we are always > !CPUMASK_OFFSTACK and hence we dont need to check for return as well as > free.. Right. > I would look forward to your comments on how we should handle this going > forward. But I would keep this the same for this patchset. Agreed, just clarify in the change log that it's not freed at the moment because of CPU_MASK_OFFSTACK=n > One of the questions that I have is if we most likely are to be in > !CONFIG_CPUMASK_OFFSTACK, then should be migrate to cpumask_t for percpu > variables. I don't think so, cpumask_t is semi-deprecated AIUI. > The reason being we end up using NR_CPU cpumask for each percpu cpumask > variable instead of using NR_CPU cpumask_t pointer. Our current defconfigs have NR_CPUS=2048, which is probably just small enough to continue using OFFSTACK=n. But we allow configuring NR_CPUS up to 8192, which surely would need OFFSTACK=y in order to work. So I think we need to stick with cpumask_var_t, but we should test with OFFSTACK=y, and should probably be a bit more careful with checking the allocations succeed. And then we should select OFFSTACK=y for NR_CPUS above some threshold. cheers
