Re: [PATCH RFC] mm/memory_hotplug: Introduce memory block types

[Dave Hansen <dave.hansen@linux.intel.com>]

It's really nice if these kinds of things are broken up.  First, replace
the old want_memblock parameter, then add the parameter to the
__add_page() calls.

> +/*
> + * NONE:     No memory block is to be created (e.g. device memory).
> + * NORMAL:   Memory block that represents normal (boot or hotplugged) memory
> + *           (e.g. ACPI DIMMs) that should be onlined either automatically
> + *           (memhp_auto_online) or manually by user space to select a
> + *           specific zone.
> + *           Applicable to memhp_auto_online.
> + * STANDBY:  Memory block that represents standby memory that should only
> + *           be onlined on demand by user space (e.g. standby memory on
> + *           s390x), but never automatically by the kernel.
> + *           Not applicable to memhp_auto_online.
> + * PARAVIRT: Memory block that represents memory added by
> + *           paravirtualized mechanisms (e.g. hyper-v, xen) that will
> + *           always automatically get onlined. Memory will be unplugged
> + *           using ballooning, not by relying on the MOVABLE ZONE.
> + *           Not applicable to memhp_auto_online.
> + */
> +enum {
> +	MEMORY_BLOCK_NONE,
> +	MEMORY_BLOCK_NORMAL,
> +	MEMORY_BLOCK_STANDBY,
> +	MEMORY_BLOCK_PARAVIRT,
> +};

This does not seem like the best way to expose these.

STANDBY, for instance, seems to be essentially a replacement for a check
against running on s390 in userspace to implement a _typical_ s390
policy.  It seems rather weird to try to make the userspace policy
determination easier by telling userspace about the typical s390 policy
via the kernel.

As for the OOM issues, that sounds like something we need to fix by
refusing to do (or delaying) hot-add operations once we consume too much
ZONE_NORMAL from memmap[]s rather than trying to indirectly tell
userspace to hurry thing along.

So, to my eye, we need:

 +enum {
 +	MEMORY_BLOCK_NONE,
 +	MEMORY_BLOCK_STANDBY, /* the default */
 +	MEMORY_BLOCK_AUTO_ONLINE,
 +};

and we can probably collapse NONE into AUTO_ONLINE because userspace
ends up doing the same thing for both: nothing.

>  struct memory_block {
>  	unsigned long start_section_nr;
>  	unsigned long end_section_nr;
> @@ -34,6 +58,7 @@ struct memory_block {
>  	int (*phys_callback)(struct memory_block *);
>  	struct device dev;
>  	int nid;			/* NID for this memory block */
> +	int type;			/* type of this memory block */
>  };

Shouldn't we just be creating and using an actual named enum type?