Re: [PATCH v5 2/3] x86/topology: Avoid wasting 128k for package id array

[Thomas Gleixner <tglx@linutronix.de>]

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