From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1755389AbdKJAnl (ORCPT ); Thu, 9 Nov 2017 19:43:41 -0500 Received: from Galois.linutronix.de ([146.0.238.70]:53177 "EHLO Galois.linutronix.de" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752060AbdKJAnk (ORCPT ); Thu, 9 Nov 2017 19:43:40 -0500 Date: Fri, 10 Nov 2017 01:43:23 +0100 (CET) From: Thomas Gleixner To: Prarit Bhargava cc: linux-kernel@vger.kernel.org, Andi Kleen , Ingo Molnar , "H. Peter Anvin" , x86@kernel.org, Peter Zijlstra , Dave Hansen , Piotr Luc , Kan Liang , Borislav Petkov , Stephane Eranian , Arvind Yadav , Andy Lutomirski , Christian Borntraeger , "Kirill A. Shutemov" , Tom Lendacky , He Chen , Mathias Krause , Tim Chen , Vitaly Kuznetsov Subject: Re: [PATCH v5 2/3] x86/topology: Avoid wasting 128k for package id array In-Reply-To: <20171105165428.32108-3-prarit@redhat.com> Message-ID: References: <20171105165428.32108-1-prarit@redhat.com> <20171105165428.32108-3-prarit@redhat.com> User-Agent: Alpine 2.20 (DEB 67 2015-01-07) MIME-Version: 1.0 Content-Type: text/plain; charset=US-ASCII X-Linutronix-Spam-Score: -1.0 X-Linutronix-Spam-Level: - X-Linutronix-Spam-Status: No , -1.0 points, 5.0 required, ALL_TRUSTED=-1,SHORTCIRCUIT=-0.0001 Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Sun, 5 Nov 2017, Prarit Bhargava wrote: > [v5]: Change kmalloc to GFP_ATOMIC to fix "sleeping function" warning on > virtual machines. What has this to do with virtual machines? The very same issue is on physcial hardware because this is called from the early CPU bringup code with interrupts and preemption disabled. > + /* Allocate and copy a new array */ > + ltp_pkg_map_new = kmalloc(logical_packages * sizeof(u16), GFP_ATOMIC); > + BUG_ON(!ltp_pkg_map_new); Having an allocation in that code path is a bad idea. First of all the error handling in there is just crap, because the only thing you can do is panic. Aside of that atomic allocations should be avoided when we can and we can. Sorry I missed that when looking at the patch earlier. Something along this makes it work proper: struct pkg_map { unsigned int size; unsigned int used; unsigned int map[0]; }; static struct pkg_map *logical_to_physical_pkg_map __read_mostly; static int resize_pkg_map(void) { struct pkg_map *newmap, *oldmap = logical_to_physical_pkg_map; int size; if (oldmap->size > oldmap->used) return 0; size = sizeof(*oldmap) + sizeof(unsigned int) * oldmap->size; newmap = kzalloc(size + sizeof(unsigned int)); if (!newmap) return -ENOMEM; memcpy(newmap, oldmap, size); newmap->size++; logical_to_physical_pkg_map = newmap; kfree(oldmap); return 0; } int __cpu_up(....) { if (resize_pkg_map()) return -ENOMEM; return smp_ops.cpu_up(....); } static void update_map(....) { if (find_map()) return; map->map[map->used] = physid; map->used++; } static void smp_init_package_map() { struct pkg_map *map; map = kzalloc(sizeof(*newmap) + sizeof(unsigned int)); map->size = 1; } See? No BUG_ON() in the early secondary cpu boot code. If memory allocation fails the thing goes back gracefully. Locking/barriers omitted as you have choices here: 1) RCU Needs the proper RCU magic for the lookup and the pointer swap. That requires also a proper barrier between the assignement of the new id and the increment of the used count plus the corresponding one on the read side. 2) mutex Must be held when swapping the pointers and across lookup Same barrier requirement as RCU 3) raw_spinlock Must be held when swapping the pointers and across lookup No barriers as long as you hold the lock across the assignement and increment. All of that works. There is no way to make sure that a lookup is fully serialized against a concurrent update. Even if the lookup holds cpu_read_lock() the new package might arrive right after the unlock. Thanks, tglx