Messages in this thread | | | Subject | Re: [PATCH v5 2/3] x86/topology: Avoid wasting 128k for package id array | From | Prarit Bhargava <> | Date | Fri, 10 Nov 2017 07:29:56 -0500 |
| |
On 11/09/2017 07:43 PM, Thomas Gleixner wrote: > 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.
There was a Intel test bot report of a failure during boot on virtual systems with Andi's patch.
> >> + /* 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 Thomas.
Andi, do you want to take a look at this?
P.
> Thanks, > > tglx >
| |