Linux-HyperV List
 help / color / mirror / Atom feed
* RE: [PATCH v6 09/10] arm64: efi: Export screen_info
From: Michael Kelley @ 2020-03-19 21:46 UTC (permalink / raw)
  To: Arnd Bergmann
  Cc: Will Deacon, Ard Biesheuvel, Catalin Marinas, Mark Rutland,
	Marc Zyngier, Linux ARM, gregkh, linux-kernel@vger.kernel.org,
	linux-hyperv@vger.kernel.org, linux-efi, linux-arch,
	olaf@aepfle.de, Andy Whitcroft, vkuznets, Jason Wang,
	marcelo.cerri@canonical.com, KY Srinivasan, Sunil Muthuswamy,
	Boqun Feng
In-Reply-To: <CAK8P3a1YCtc3LJ-_3iT90_Srehb96gLHvTXsbJ0wT6NFYCG=TQ@mail.gmail.com>

From: Arnd Bergmann <arnd@arndb.de> Sent: Wednesday, March 18, 2020 2:27 AM
> 
> On Wed, Mar 18, 2020 at 1:18 AM Michael Kelley <mikelley@microsoft.com> wrote:
> > From: Arnd Bergmann <arnd@arndb.de>
> > > On Sat, Mar 14, 2020 at 4:36 PM Michael Kelley <mikelley@microsoft.com> wrote:
> > > >
> > > > The Hyper-V frame buffer driver may be built as a module, and
> > > > it needs access to screen_info. So export screen_info.
> > > >
> > > > Signed-off-by: Michael Kelley <mikelley@microsoft.com>
> > >
> > > Is there any chance of using a more modern KMS based driver for the screen
> > > than the old fbdev subsystem? I had hoped to one day completely remove
> > > support for the old CONFIG_VIDEO_FBDEV and screen_info from modern
> > > architectures.
> > >
> >
> > The current hyperv_fb.c driver is all we have today for the synthetic Hyper-V
> > frame buffer device.  That driver builds and runs on both ARM64 and x86.
> >
> > I'm not knowledgeable about video/graphics drivers, but when you
> > say "a more modern KMS based driver", are you meaning one based on
> > DRM & KMS?  Does DRM make sense for a "dumb" frame buffer device?
> > Are there any drivers that would be a good pattern to look at?
> 
> It used to be a lot harder to write a DRM driver compared to an fbdev
> driver, but this has changed to the opposite over the years.
> 
> A fairly minimal example would be drivers/gpu/drm/pl111/pl111_drv.c
> or anything in drivers/gpu/drm/tiny/, but you may want to look at the
> other hypervisor platforms first, i.e drivers/gpu/drm/virtio/virtgpu_drv.c,
> drivers/gpu/drm/vmwgfx/vmwgfx_drv.c, drivers/gpu/drm/xen/xen_drm_front.c,
> drivers/gpu/drm/qxl/qxl_drv.c, and drivers/gpu/drm/bochs/bochs_drv.c.
> 

Thanks for the pointers, especially for the other hypervisors.

Michael

^ permalink raw reply

* RE: [PATCH v6 04/10] arm64: hyperv: Add memory alloc/free functions for Hyper-V size pages
From: Michael Kelley @ 2020-03-19 21:43 UTC (permalink / raw)
  To: Arnd Bergmann
  Cc: Marc Zyngier, gregkh, Will Deacon, Ard Biesheuvel,
	Catalin Marinas, Mark Rutland, Linux ARM,
	linux-kernel@vger.kernel.org, linux-hyperv@vger.kernel.org,
	linux-efi, linux-arch, olaf@aepfle.de, Andy Whitcroft, vkuznets,
	Jason Wang, marcelo.cerri@canonical.com, KY Srinivasan,
	Sunil Muthuswamy, Boqun Feng
In-Reply-To: <CAK8P3a2AO4k3vJ7WuJQz7ick+XPgGY3Jfk8-ROqtwyNs0nWkDA@mail.gmail.com>

From: Arnd Bergmann <arnd@arndb.de> Sent: Wednesday, March 18, 2020 2:58 AM
> 
> On Wed, Mar 18, 2020 at 1:15 AM Michael Kelley <mikelley@microsoft.com> wrote:
> > From: Arnd Bergmann <arnd@arndb.de> Sent: Monday, March 16, 2020 1:33 AM
> > >
> > > On Mon, Mar 16, 2020 at 9:30 AM Marc Zyngier <maz@kernel.org> wrote:
> > > > On 2020-03-16 08:22, Arnd Bergmann wrote:
> > > > > On Sat, Mar 14, 2020 at 4:36 PM Michael Kelley <mikelley@microsoft.com>
> > > > > wrote:
> > > > >>  /*
> > > > >> + * Functions for allocating and freeing memory with size and
> > > > >> + * alignment HV_HYP_PAGE_SIZE. These functions are needed because
> > > > >> + * the guest page size may not be the same as the Hyper-V page
> > > > >> + * size. We depend upon kmalloc() aligning power-of-two size
> > > > >> + * allocations to the allocation size boundary, so that the
> > > > >> + * allocated memory appears to Hyper-V as a page of the size
> > > > >> + * it expects.
> > > > >> + *
> > > > >> + * These functions are used by arm64 specific code as well as
> > > > >> + * arch independent Hyper-V drivers.
> > > > >> + */
> > > > >> +
> > > > >> +void *hv_alloc_hyperv_page(void)
> > > > >> +{
> > > > >> +       BUILD_BUG_ON(PAGE_SIZE <  HV_HYP_PAGE_SIZE);
> > > > >> +       return kmalloc(HV_HYP_PAGE_SIZE, GFP_KERNEL);
> > > > >> +}
> > > > >> +EXPORT_SYMBOL_GPL(hv_alloc_hyperv_page);
> > > > >
> > > > > I don't think there is any guarantee that kmalloc() returns
> > > > > page-aligned
> > > > > allocations in general.
> > > >
> > > > I believe that guarantee came with 59bb47985c1db ("mm, sl[aou]b:
> > > > guarantee
> > > > natural alignment for kmalloc(power-of-two)").
> > > >
> > > > > How about using get_free_pages() to implement this?
> > > >
> > > > This would certainly work, at the expense of a lot of wasted memory when
> > > > PAGE_SIZE isn't 4k.
> > >
> > > I'm sure this is the least of your problems when the guest runs with
> > > a large base page size, you've already wasted most of your memory
> > > otherwise then.
> > >
> >
> > I think there's value in keeping these functions.  There are 8 uses in
> > architecture independent code at the moment, which admittedly saves
> > only ~1/2 Mbyte of memory with a 64K page size, but we will have
> > additional uses with more memory savings as we get all of the
> > Hyper-V synthetic drivers to work with 64K page size.  Furthermore,
> > there's coming work that will require additional steps to share a page
> > between a guest and the Hyper-V host.  These functions are the right
> > place to put the code for the additional sharing steps.  Removing them
> > now in favor of a bare kmalloc() and then adding them back doesn't
> > seem worthwhile.
> 
> My point was to keep the functions but use alloc_pages() internally,
> so you can deal with the hypervisor having a larger page size than
> the guest, which seems to be a more important scenario if I correctly
> understand the differences between the way Windows and Linux
> deal with page cache.

OK, I see now what you are getting at.  I should spell out my
assumption, which is the opposite.  Hyper-V won't have a page
size other than 4K anytime in the foreseeable future.  The
code is too wedded to the x86 4K page size, and the host-guest
interfaces have a lot of implicit assumptions that the page size is
4K (unfortunately).  But the last time I looked, RHEL for ARM64 is
delivered with a 64K page size.  So my assumption is that the only
combination that really matters is the guest page size being larger
than the Hyper-V page size.  The other way around just won't
happen without a major overhaul to Hyper-V, including a rework
of the guest-host interface.

Michael

> 
> As far as I understand, using 64kb pages on Windows is generally
> a win as the VFS code already deals with units of that size, while
> on Linux the 4kb page size is too deeply entrenched within the
> file system code to mess with it: Whatever you gain in terms of
> TLB pressure on workloads that cannot use huge pages is all lost
> again through extra I/O and wasted physical memory.
> 
>         Arnd

^ permalink raw reply

* RE: [PATCH v6 01/10] arm64: hyperv: Add core Hyper-V include files
From: Michael Kelley @ 2020-03-19 21:31 UTC (permalink / raw)
  To: Arnd Bergmann
  Cc: Will Deacon, Ard Biesheuvel, Catalin Marinas, Mark Rutland,
	Marc Zyngier, Linux ARM, gregkh, linux-kernel@vger.kernel.org,
	linux-hyperv@vger.kernel.org, linux-efi, linux-arch,
	olaf@aepfle.de, Andy Whitcroft, vkuznets, Jason Wang,
	marcelo.cerri@canonical.com, KY Srinivasan, Sunil Muthuswamy,
	Boqun Feng
In-Reply-To: <CAK8P3a02EULGxyuKFq8YnbG8BQ_m-RKciaNEc9ZbdP2yz9dt+Q@mail.gmail.com>

From: Arnd Bergmann <arnd@arndb.de> Sent: Wednesday, March 18, 2020 3:10 AM
> 
> On Wed, Mar 18, 2020 at 1:12 AM Michael Kelley <mikelley@microsoft.com> wrote:
> > From: Arnd Bergmann <arnd@arndb.de> Sent: Monday, March 16, 2020 1:48 AM
> > > On Sat, Mar 14, 2020 at 4:36 PM Michael Kelley <mikelley@microsoft.com> wrote:
> > >
> > > > +
> > > > +/* Define input and output layout for Get VP Register hypercall */
> > > > +struct hv_get_vp_register_input {
> > > > +       u64 partitionid;
> > > > +       u32 vpindex;
> > > > +       u8  inputvtl;
> > > > +       u8  padding[3];
> > > > +       u32 name0;
> > > > +       u32 name1;
> > > > +} __packed;
> > >
> > > Are you sure these need to be made byte-aligned according to the
> > > specification? If the structure itself is aligned to 64 bit, better mark only
> > > the individual fields that are misaligned as __packed.
> > >
> > > If the structure is aligned to only 32-bit addresses instead of
> > > 64-bit, mark it as "__packed __aligned(4)" to let the compiler
> > > generate better code for accessing it.
> >
> > None of the fields are misaligned and it will always be aligned to 64-bit
> > addresses, so there should be no padding needed or added.  There was
> > a discussion of __packed and the Hyper-V data structures in general on
> > LKML here:  https://lkml.org/lkml/2018/11/30/848  Adding __packed was
> > done as a preventative measure, not because anything was actually
> > broken.  Marking as __aligned(8) here would indicate the correct intent,
> > though the use of the structure always ensures 64-bit alignment.
> 
> Just drop the __packed annotations then, they just confuse the compiler
> in this case. In particular, when the compiler thinks that a structure is
> misaligned, it tries to avoid using load/store instructions on it that are
> inefficient or trap with misaligned code, so having default alignment
> produces better object code.

So I'm confused a bit.  Were the original concerns in the above LKML
discussion bogus?  Is it legal for the compiler to reorder fields or add
padding, even if the layout of fields in the structure doesn't require it?
If the compiler *could* do such, then it seems like keeping the __packed
would be appropriate per the LKML discussion.

> 
> > > Also, in order to write portable code, it would be helpful to mark
> > > all the fields as explicitly little-endian, and use __le32_to_cpu()
> > > etc for accessing them.
> >
> > There's an opening comment in this file stating that all data
> > structures shared between Hyper-V and a guest VM are little
> > endian.  Is there some other marking to consider using?
> 
> Yes, device drivers should generally define data structures using
> the __le32, __le64 etc types, and use the conversion functions
> to access them. Building with 'make C=1' usually tells you when
> you have mismatched annotations.
> 
> > We have definitely not allowed for the case of Hyper-V running on
> > a big endian architecture.  There are a *lot* of messages and data
> > structures passed between the guest and Hyper-V, and coding
> > to handle either endianness is a big project.  I'm doubtful
> > of the value until and unless we actually have a need for it.
> 
> In general, the use of big-endian software on Linux is declining, however
> 
> - arm64 as an architecture is meant to support both endian types,
>   and we still try to ensure it works either way as long as there are
>   users that depend on it.
> 
> - The remaining users of big-endian software are probably
>   more likely to run on virtual machines than on real hardware
> 
> - Any device driver should generally be written against portable
>   interfaces, even if you think you know how it will be used. As
>   driver writers tend to look at existing code for new drivers, it's
>   better to have them all be portable. (This is a similar argument
>   to the irqchip interface).
> 
> Even if you don't convert any of the existing architecture independent
> code to run both ways, I see no reason to not do it for new drivers.

OK, let me look into this.  Given how the major Linux distros on
ARM64 have all gone little-endian, I'm a bit skeptical of the value
for the big server environments in which Hyper-V would be used.

> 
> > > > +/* Define synthetic interrupt controller message flags. */
> > > > +union hv_message_flags {
> > > > +       __u8 asu8;
> > > > +       struct {
> > > > +               __u8 msg_pending:1;
> > > > +               __u8 reserved:7;
> > > > +       } __packed;
> > > > +};
> > >
> > > For similar reasons, please avoid bit fields and just use a
> > > bit mask on the first member of the union.
> >
> > Unfortunately, changing to a bit mask ripples into
> > architecture independent code and into the x86
> > implementation.  I'd prefer not to drag that complexity
> > into this patch set.
> 
> How so? If this file is arm64 specific, there should be no need to make
> x86 do the same change.

This file, hyperv-tlfs.h, is duplicating some definitions on the x86 and
ARM64 sides that are used by arch independent code, and this is one
of those definitions.  I had held off on breaking the file into arch
independent and arch specific portions because the Hyper-V team has
left some gray areas for functionality that isn't yet used on the ARM64
side.  So in some cases, it's hard to know what functionality to put
into the arch independent portion.

But I think I'll go ahead and make the separation with reasonably good
accuracy, and update the x86 side accordingly.  That will reduce the size
of this patch set to contain only the things that we know are ARM64
specific and which are actually used by the ARM64 code.  Things like the
hv_message_flags will go into the arch independent portion so that
they can be used by the arch independent code without cluttering up
the arch specific code.  Making the change will help reduce any
confusion about what is ARM64-specific. The other core #include file,
mshyperv.h, has already been done this way.

Michael

> 
> > > > + * Use the Hyper-V provided stimer0 as the timer that is made
> > > > + * available to the architecture independent Hyper-V drivers.
> > > > + */
> > > > +#define hv_init_timer(timer, tick) \
> > > > +               hv_set_vpreg(HV_REGISTER_STIMER0_COUNT + (2*timer), tick)
> > > > +#define hv_init_timer_config(timer, val) \
> > > > +               hv_set_vpreg(HV_REGISTER_STIMER0_CONFIG + (2*timer), val)
> > > > +#define hv_get_current_tick(tick) \
> > > > +               (tick = hv_get_vpreg(HV_REGISTER_TIME_REFCOUNT))
> > >
> > > In general, we prefer inline functions over macros in header files.
> >
> > I can change the "set" calls to inline functions.  As you can see, the "get"
> > functions are coded and used in architecture independent code and on
> > the x86 side in a way that won't convert to inline functions.
> 
> Ok.
> 
>         Arnd

^ permalink raw reply

* RE: [PATCH v6 03/10] arm64: hyperv: Add hypercall and register access functions
From: Michael Kelley @ 2020-03-19 21:04 UTC (permalink / raw)
  To: Hillf Danton
  Cc: will@kernel.org, ardb@kernel.org, arnd@arndb.de,
	catalin.marinas@arm.com, mark.rutland@arm.com, maz@kernel.org,
	linux-arm-kernel@lists.infradead.org, gregkh@linuxfoundation.org,
	linux-kernel@vger.kernel.org, linux-hyperv@vger.kernel.org,
	linux-efi@vger.kernel.org, linux-arch@vger.kernel.org,
	olaf@aepfle.de, apw@canonical.com, vkuznets, jasowang@redhat.com,
	marcelo.cerri@canonical.com, KY Srinivasan, Sunil Muthuswamy,
	boqun.feng@gmail.com
In-Reply-To: <20200318031130.5476-1-hdanton@sina.com>

From: Hillf Danton <hdanton@sina.com> Sent: Tuesday, March 17, 2020 8:12 PM
> 
> On Sat, 14 Mar 2020 08:35:12 -0700 Michael Kelley wrote:
> > +/*
> > + * Get the value of a single VP register.  One version
> > + * returns just 64 bits and another returns the full 128 bits.
> > + * The two versions are separate to avoid complicating the
> > + * calling sequence for the more frequently used 64 bit version.
> > + */
> > +
> > +/*
> > + * Input and output memory allocation sizes are rounded up to a power
> > + * of 2 so kmalloc() will guarantee alignment. In turn, the alignment
> > + * ensures that the allocations don't cross a page boundary, which is
> 
> Better to specify kmalloc's current alignment and why it fails to ensure
> (4 * sizeof(u64))-sized allocations wont cross page boundary.
> 

Is your comment referring to ARCH_KMALLOC_MINALIGN?  If so, I see
how that makes sense.  BUILD_BUG_ON (sizeof (*input) >
ARCH_KMALLOC_MINALIGN) would be a cleaner solution. 

Thanks,

Michael

> > + * required by the hypercall interface.
> > + */
> > +#define INPUTSIZE (4 * sizeof(u64))
> > +#define OUTPUTSIZE (2 * sizeof(u64))
> > +
> > +static void __hv_get_vpreg_128(u32 msr, struct hv_get_vp_register_output *res)
> > +{
> > +	union hv_hypercall_status		status;
> > +	struct hv_get_vp_register_input		*input;
> > +
> > +	BUILD_BUG_ON(sizeof(*input) > INPUTSIZE);
> > +
> > +	input = kzalloc(INPUTSIZE, GFP_ATOMIC);
> > +
> > +	input->partitionid = HV_PARTITION_ID_SELF;
> > +	input->vpindex = HV_VP_INDEX_SELF;
> > +	input->inputvtl = 0;
> > +	input->name0 = msr;
> > +	input->name1 = 0;
> > +
> > +
> > +	status.as_uint64 = hv_do_hypercall(
> > +		HVCALL_GET_VP_REGISTERS | HV_HYPERCALL_REP_COUNT_1,
> > +		input, res);
> > +
> > +	/*
> > +	 * Something is fundamentally broken in the hypervisor if
> > +	 * getting a VP register fails. There's really no way to
> > +	 * continue as a guest VM, so panic.
> > +	 */
> > +	BUG_ON(status.status != HV_STATUS_SUCCESS);
> > +
> > +	kfree(input);
> > +}


^ permalink raw reply

* RE: [PATCH v8 2/5] x86/hyper-v: Add synthetic debugger definitions
From: Michael Kelley @ 2020-03-19 20:49 UTC (permalink / raw)
  To: Jon Doron, kvm@vger.kernel.org, linux-hyperv@vger.kernel.org; +Cc: vkuznets
In-Reply-To: <20200319063836.678562-3-arilou@gmail.com>

From: Jon Doron <arilou@gmail.com> Sent: Wednesday, March 18, 2020 11:39 PM
> 
> Hyper-V synthetic debugger has two modes, one that uses MSRs and
> the other that use Hypercalls.
> 
> Add all the required definitions to both types of synthetic debugger
> interface.
> 
> Some of the required new CPUIDs and MSRs are not documented in the TLFS
> so they are in hyperv.h instead.
> 
> The reason they are not documented is because they are subjected to be
> removed in future versions of Windows.
> 
> Signed-off-by: Jon Doron <arilou@gmail.com>

Reviewed-by: Michael Kelley <mikelley@microsoft.com>

> ---
>  arch/x86/include/asm/hyperv-tlfs.h |  6 ++++++
>  arch/x86/kvm/hyperv.h              | 27 +++++++++++++++++++++++++++
>  2 files changed, 33 insertions(+)
> 
> diff --git a/arch/x86/include/asm/hyperv-tlfs.h b/arch/x86/include/asm/hyperv-tlfs.h
> index 92abc1e42bfc..671ce2a39d4b 100644
> --- a/arch/x86/include/asm/hyperv-tlfs.h
> +++ b/arch/x86/include/asm/hyperv-tlfs.h
> @@ -131,6 +131,8 @@
>  #define HV_FEATURE_FREQUENCY_MSRS_AVAILABLE		BIT(8)
>  /* Crash MSR available */
>  #define HV_FEATURE_GUEST_CRASH_MSR_AVAILABLE		BIT(10)
> +/* Support for debug MSRs available */
> +#define HV_FEATURE_DEBUG_MSRS_AVAILABLE			BIT(11)
>  /* stimer Direct Mode is available */
>  #define HV_STIMER_DIRECT_MODE_AVAILABLE			BIT(19)
> 
> @@ -376,6 +378,9 @@ struct hv_tsc_emulation_status {
>  #define HVCALL_SEND_IPI_EX			0x0015
>  #define HVCALL_POST_MESSAGE			0x005c
>  #define HVCALL_SIGNAL_EVENT			0x005d
> +#define HVCALL_POST_DEBUG_DATA			0x0069
> +#define HVCALL_RETRIEVE_DEBUG_DATA		0x006a
> +#define HVCALL_RESET_DEBUG_SESSION		0x006b
>  #define HVCALL_FLUSH_GUEST_PHYSICAL_ADDRESS_SPACE 0x00af
>  #define HVCALL_FLUSH_GUEST_PHYSICAL_ADDRESS_LIST 0x00b0
> 
> @@ -419,6 +424,7 @@ enum HV_GENERIC_SET_FORMAT {
>  #define HV_STATUS_INVALID_HYPERCALL_INPUT	3
>  #define HV_STATUS_INVALID_ALIGNMENT		4
>  #define HV_STATUS_INVALID_PARAMETER		5
> +#define HV_STATUS_OPERATION_DENIED		8
>  #define HV_STATUS_INSUFFICIENT_MEMORY		11
>  #define HV_STATUS_INVALID_PORT_ID		17
>  #define HV_STATUS_INVALID_CONNECTION_ID		18
> diff --git a/arch/x86/kvm/hyperv.h b/arch/x86/kvm/hyperv.h
> index 757cb578101c..5e4780bf6dd7 100644
> --- a/arch/x86/kvm/hyperv.h
> +++ b/arch/x86/kvm/hyperv.h
> @@ -23,6 +23,33 @@
> 
>  #include <linux/kvm_host.h>
> 
> +/*
> + * The #defines related to the synthetic debugger are required by KDNet, but
> + * they are not documented in the Hyper-V TLFS because the synthetic debugger
> + * functionality has been deprecated and is subject to removal in future versions
> + * of Windows.
> + */
> +#define HYPERV_CPUID_SYNDBG_VENDOR_AND_MAX_FUNCTIONS	0x40000080
> +#define HYPERV_CPUID_SYNDBG_INTERFACE			0x40000081
> +#define HYPERV_CPUID_SYNDBG_PLATFORM_CAPABILITIES	0x40000082
> +
> +/*
> + * Hyper-V synthetic debugger platform capabilities
> + * These are HYPERV_CPUID_SYNDBG_PLATFORM_CAPABILITIES.EAX bits.
> + */
> +#define HV_X64_SYNDBG_CAP_ALLOW_KERNEL_DEBUGGING	BIT(1)
> +
> +/* Hyper-V Synthetic debug options MSR */
> +#define HV_X64_MSR_SYNDBG_CONTROL		0x400000F1
> +#define HV_X64_MSR_SYNDBG_STATUS		0x400000F2
> +#define HV_X64_MSR_SYNDBG_SEND_BUFFER		0x400000F3
> +#define HV_X64_MSR_SYNDBG_RECV_BUFFER		0x400000F4
> +#define HV_X64_MSR_SYNDBG_PENDING_BUFFER	0x400000F5
> +#define HV_X64_MSR_SYNDBG_OPTIONS		0x400000FF
> +
> +/* Hyper-V HV_X64_MSR_SYNDBG_OPTIONS bits */
> +#define HV_X64_SYNDBG_OPTION_USE_HCALLS		BIT(2)
> +
>  static inline struct kvm_vcpu_hv *vcpu_to_hv_vcpu(struct kvm_vcpu *vcpu)
>  {
>  	return &vcpu->arch.hyperv;
> --
> 2.24.1


^ permalink raw reply

* Re: [PATCH v3 8/8] mm/memory_hotplug: allow to specify a default online_type
From: Pankaj Gupta @ 2020-03-19 17:26 UTC (permalink / raw)
  To: David Hildenbrand
  Cc: linux-kernel, linux-mm, linuxppc-dev, linux-hyperv, Wei Yang,
	Michal Hocko, Greg Kroah-Hartman, Andrew Morton, Michal Hocko,
	Oscar Salvador, Rafael J. Wysocki, Baoquan He
In-Reply-To: <20200319131221.14044-9-david@redhat.com>

> For now, distributions implement advanced udev rules to essentially
> - Don't online any hotplugged memory (s390x)
> - Online all memory to ZONE_NORMAL (e.g., most virt environments like
>   hyperv)
> - Online all memory to ZONE_MOVABLE in case the zone imbalance is taken
>   care of (e.g., bare metal, special virt environments)
>
> In summary: All memory is usually onlined the same way, however, the
> kernel always has to ask user space to come up with the same answer.
> E.g., Hyper-V always waits for a memory block to get onlined before
> continuing, otherwise it might end up adding memory faster than
> onlining it, which can result in strange OOM situations. This waiting
> slows down adding of a bigger amount of memory.
>
> Let's allow to specify a default online_type, not just "online" and
> "offline". This allows distributions to configure the default online_type
> when booting up and be done with it.
>
> We can now specify "offline", "online", "online_movable" and
> "online_kernel" via
> - "memhp_default_state=" on the kernel cmdline
> - /sys/devices/system/memory/auto_online_blocks
> just like we are able to specify for a single memory block via
> /sys/devices/system/memory/memoryX/state
>
> Reviewed-by: Wei Yang <richard.weiyang@gmail.com>
> Acked-by: Michal Hocko <mhocko@suse.com>
> Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> Cc: Andrew Morton <akpm@linux-foundation.org>
> Cc: Michal Hocko <mhocko@kernel.org>
> Cc: Oscar Salvador <osalvador@suse.de>
> Cc: "Rafael J. Wysocki" <rafael@kernel.org>
> Cc: Baoquan He <bhe@redhat.com>
> Cc: Wei Yang <richard.weiyang@gmail.com>
> Signed-off-by: David Hildenbrand <david@redhat.com>
> ---
>  drivers/base/memory.c          | 11 +++++------
>  include/linux/memory_hotplug.h |  2 ++
>  mm/memory_hotplug.c            |  8 ++++----
>  3 files changed, 11 insertions(+), 10 deletions(-)
>
> diff --git a/drivers/base/memory.c b/drivers/base/memory.c
> index 8d3e16dab69f..2b09b68b9f78 100644
> --- a/drivers/base/memory.c
> +++ b/drivers/base/memory.c
> @@ -35,7 +35,7 @@ static const char *const online_type_to_str[] = {
>         [MMOP_ONLINE_MOVABLE] = "online_movable",
>  };
>
> -static int memhp_online_type_from_str(const char *str)
> +int memhp_online_type_from_str(const char *str)
>  {
>         int i;
>
> @@ -394,13 +394,12 @@ static ssize_t auto_online_blocks_store(struct device *dev,
>                                         struct device_attribute *attr,
>                                         const char *buf, size_t count)
>  {
> -       if (sysfs_streq(buf, "online"))
> -               memhp_default_online_type = MMOP_ONLINE;
> -       else if (sysfs_streq(buf, "offline"))
> -               memhp_default_online_type = MMOP_OFFLINE;
> -       else
> +       const int online_type = memhp_online_type_from_str(buf);
> +
> +       if (online_type < 0)
>                 return -EINVAL;
>
> +       memhp_default_online_type = online_type;
>         return count;
>  }
>
> diff --git a/include/linux/memory_hotplug.h b/include/linux/memory_hotplug.h
> index 6d6f85bb66e9..93d9ada74ddd 100644
> --- a/include/linux/memory_hotplug.h
> +++ b/include/linux/memory_hotplug.h
> @@ -118,6 +118,8 @@ extern int arch_add_memory(int nid, u64 start, u64 size,
>                            struct mhp_params *params);
>  extern u64 max_mem_size;
>
> +extern int memhp_online_type_from_str(const char *str);
> +
>  /* Default online_type (MMOP_*) when new memory blocks are added. */
>  extern int memhp_default_online_type;
>  /* If movable_node boot option specified */
> diff --git a/mm/memory_hotplug.c b/mm/memory_hotplug.c
> index 4efcf8cb9ac5..89197163d138 100644
> --- a/mm/memory_hotplug.c
> +++ b/mm/memory_hotplug.c
> @@ -74,10 +74,10 @@ int memhp_default_online_type = MMOP_ONLINE;
>
>  static int __init setup_memhp_default_state(char *str)
>  {
> -       if (!strcmp(str, "online"))
> -               memhp_default_online_type = MMOP_ONLINE;
> -       else if (!strcmp(str, "offline"))
> -               memhp_default_online_type = MMOP_OFFLINE;
> +       const int online_type = memhp_online_type_from_str(str);
> +
> +       if (online_type >= 0)
> +               memhp_default_online_type = online_type;
>
>         return 1;
>  }
> --
Acked-by: Pankaj Gupta <pankaj.gupta.linux@gmail.com>

> 2.24.1
>
>

^ permalink raw reply

* Re: [PATCH v3 7/8] mm/memory_hotplug: convert memhp_auto_online to store an online_type
From: Pankaj Gupta @ 2020-03-19 17:19 UTC (permalink / raw)
  To: David Hildenbrand
  Cc: linux-kernel, linux-mm, linuxppc-dev, linux-hyperv, Wei Yang,
	Michal Hocko, Greg Kroah-Hartman, Andrew Morton, Michal Hocko,
	Oscar Salvador, Rafael J. Wysocki, Baoquan He
In-Reply-To: <20200319131221.14044-8-david@redhat.com>

> ... and rename it to memhp_default_online_type. This is a preparation
> for more detailed default online behavior.
>
> Reviewed-by: Wei Yang <richard.weiyang@gmail.com>
> Acked-by: Michal Hocko <mhocko@suse.com>
> Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> Cc: Andrew Morton <akpm@linux-foundation.org>
> Cc: Michal Hocko <mhocko@kernel.org>
> Cc: Oscar Salvador <osalvador@suse.de>
> Cc: "Rafael J. Wysocki" <rafael@kernel.org>
> Cc: Baoquan He <bhe@redhat.com>
> Cc: Wei Yang <richard.weiyang@gmail.com>
> Signed-off-by: David Hildenbrand <david@redhat.com>
> ---
>  drivers/base/memory.c          | 10 ++++------
>  include/linux/memory_hotplug.h |  3 ++-
>  mm/memory_hotplug.c            | 11 ++++++-----
>  3 files changed, 12 insertions(+), 12 deletions(-)
>
> diff --git a/drivers/base/memory.c b/drivers/base/memory.c
> index 8a7f29c0bf97..8d3e16dab69f 100644
> --- a/drivers/base/memory.c
> +++ b/drivers/base/memory.c
> @@ -386,10 +386,8 @@ static DEVICE_ATTR_RO(block_size_bytes);
>  static ssize_t auto_online_blocks_show(struct device *dev,
>                                        struct device_attribute *attr, char *buf)
>  {
> -       if (memhp_auto_online)
> -               return sprintf(buf, "online\n");
> -       else
> -               return sprintf(buf, "offline\n");
> +       return sprintf(buf, "%s\n",
> +                      online_type_to_str[memhp_default_online_type]);
>  }
>
>  static ssize_t auto_online_blocks_store(struct device *dev,
> @@ -397,9 +395,9 @@ static ssize_t auto_online_blocks_store(struct device *dev,
>                                         const char *buf, size_t count)
>  {
>         if (sysfs_streq(buf, "online"))
> -               memhp_auto_online = true;
> +               memhp_default_online_type = MMOP_ONLINE;
>         else if (sysfs_streq(buf, "offline"))
> -               memhp_auto_online = false;
> +               memhp_default_online_type = MMOP_OFFLINE;
>         else
>                 return -EINVAL;
>
> diff --git a/include/linux/memory_hotplug.h b/include/linux/memory_hotplug.h
> index 76f3c617a8ab..6d6f85bb66e9 100644
> --- a/include/linux/memory_hotplug.h
> +++ b/include/linux/memory_hotplug.h
> @@ -118,7 +118,8 @@ extern int arch_add_memory(int nid, u64 start, u64 size,
>                            struct mhp_params *params);
>  extern u64 max_mem_size;
>
> -extern bool memhp_auto_online;
> +/* Default online_type (MMOP_*) when new memory blocks are added. */
> +extern int memhp_default_online_type;
>  /* If movable_node boot option specified */
>  extern bool movable_node_enabled;
>  static inline bool movable_node_is_enabled(void)
> diff --git a/mm/memory_hotplug.c b/mm/memory_hotplug.c
> index e21a7d53ade5..4efcf8cb9ac5 100644
> --- a/mm/memory_hotplug.c
> +++ b/mm/memory_hotplug.c
> @@ -67,17 +67,17 @@ void put_online_mems(void)
>  bool movable_node_enabled = false;
>
>  #ifndef CONFIG_MEMORY_HOTPLUG_DEFAULT_ONLINE
> -bool memhp_auto_online;
> +int memhp_default_online_type = MMOP_OFFLINE;
>  #else
> -bool memhp_auto_online = true;
> +int memhp_default_online_type = MMOP_ONLINE;
>  #endif
>
>  static int __init setup_memhp_default_state(char *str)
>  {
>         if (!strcmp(str, "online"))
> -               memhp_auto_online = true;
> +               memhp_default_online_type = MMOP_ONLINE;
>         else if (!strcmp(str, "offline"))
> -               memhp_auto_online = false;
> +               memhp_default_online_type = MMOP_OFFLINE;
>
>         return 1;
>  }
> @@ -993,6 +993,7 @@ static int check_hotplug_memory_range(u64 start, u64 size)
>
>  static int online_memory_block(struct memory_block *mem, void *arg)
>  {
> +       mem->online_type = memhp_default_online_type;
>         return device_online(&mem->dev);
>  }
>
> @@ -1065,7 +1066,7 @@ int __ref add_memory_resource(int nid, struct resource *res)
>         mem_hotplug_done();
>
>         /* online pages if requested */
> -       if (memhp_auto_online)
> +       if (memhp_default_online_type != MMOP_OFFLINE)
>                 walk_memory_blocks(start, size, NULL, online_memory_block);
>
>         return ret;
> --

Acked-by: Pankaj Gupta <pankaj.gupta.linux@gmail.com>

> 2.24.1
>
>

^ permalink raw reply

* Re: [PATCH v3 6/8] mm/memory_hotplug: unexport memhp_auto_online
From: Pankaj Gupta @ 2020-03-19 16:52 UTC (permalink / raw)
  To: David Hildenbrand
  Cc: linux-kernel, linux-mm, linuxppc-dev, linux-hyperv, Wei Yang,
	Michal Hocko, Andrew Morton, Michal Hocko, Oscar Salvador,
	Rafael J. Wysocki, Baoquan He
In-Reply-To: <20200319131221.14044-7-david@redhat.com>

> All in-tree users except the mm-core are gone. Let's drop the export.
>
> Reviewed-by: Wei Yang <richard.weiyang@gmail.com>
> Acked-by: Michal Hocko <mhocko@suse.com>
> Cc: Andrew Morton <akpm@linux-foundation.org>
> Cc: Michal Hocko <mhocko@kernel.org>
> Cc: Oscar Salvador <osalvador@suse.de>
> Cc: "Rafael J. Wysocki" <rafael@kernel.org>
> Cc: Baoquan He <bhe@redhat.com>
> Cc: Wei Yang <richard.weiyang@gmail.com>
> Signed-off-by: David Hildenbrand <david@redhat.com>
> ---
>  mm/memory_hotplug.c | 1 -
>  1 file changed, 1 deletion(-)
>
> diff --git a/mm/memory_hotplug.c b/mm/memory_hotplug.c
> index da6aab272c9b..e21a7d53ade5 100644
> --- a/mm/memory_hotplug.c
> +++ b/mm/memory_hotplug.c
> @@ -71,7 +71,6 @@ bool memhp_auto_online;
>  #else
>  bool memhp_auto_online = true;
>  #endif
> -EXPORT_SYMBOL_GPL(memhp_auto_online);
>
>  static int __init setup_memhp_default_state(char *str)
>  {
> --
> 2.24.1

Acked-by: Pankaj Gupta <pankaj.gupta.linux@gmail.com>
>
>

^ permalink raw reply

* Re: [PATCH v3 3/8] drivers/base/memory: store mapping between MMOP_* and string in an array
From: Pankaj Gupta @ 2020-03-19 16:33 UTC (permalink / raw)
  To: David Hildenbrand
  Cc: linux-kernel, linux-mm, linuxppc-dev, linux-hyperv, Wei Yang,
	Michal Hocko, Greg Kroah-Hartman, Andrew Morton, Michal Hocko,
	Oscar Salvador, Rafael J. Wysocki, Baoquan He
In-Reply-To: <20200319131221.14044-4-david@redhat.com>

> Let's use a simple array which we can reuse soon. While at it, move the
> string->mmop conversion out of the device hotplug lock.
>
> Reviewed-by: Wei Yang <richard.weiyang@gmail.com>
> Acked-by: Michal Hocko <mhocko@suse.com>
> Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> Cc: Andrew Morton <akpm@linux-foundation.org>
> Cc: Michal Hocko <mhocko@kernel.org>
> Cc: Oscar Salvador <osalvador@suse.de>
> Cc: "Rafael J. Wysocki" <rafael@kernel.org>
> Cc: Baoquan He <bhe@redhat.com>
> Cc: Wei Yang <richard.weiyang@gmail.com>
> Signed-off-by: David Hildenbrand <david@redhat.com>
> ---
>  drivers/base/memory.c | 38 +++++++++++++++++++++++---------------
>  1 file changed, 23 insertions(+), 15 deletions(-)
>
> diff --git a/drivers/base/memory.c b/drivers/base/memory.c
> index e7e77cafef80..8a7f29c0bf97 100644
> --- a/drivers/base/memory.c
> +++ b/drivers/base/memory.c
> @@ -28,6 +28,24 @@
>
>  #define MEMORY_CLASS_NAME      "memory"
>
> +static const char *const online_type_to_str[] = {
> +       [MMOP_OFFLINE] = "offline",
> +       [MMOP_ONLINE] = "online",
> +       [MMOP_ONLINE_KERNEL] = "online_kernel",
> +       [MMOP_ONLINE_MOVABLE] = "online_movable",
> +};
> +
> +static int memhp_online_type_from_str(const char *str)
> +{
> +       int i;
> +
> +       for (i = 0; i < ARRAY_SIZE(online_type_to_str); i++) {
> +               if (sysfs_streq(str, online_type_to_str[i]))
> +                       return i;
> +       }
> +       return -EINVAL;
> +}
> +
>  #define to_memory_block(dev) container_of(dev, struct memory_block, dev)
>
>  static int sections_per_block;
> @@ -236,26 +254,17 @@ static int memory_subsys_offline(struct device *dev)
>  static ssize_t state_store(struct device *dev, struct device_attribute *attr,
>                            const char *buf, size_t count)
>  {
> +       const int online_type = memhp_online_type_from_str(buf);
>         struct memory_block *mem = to_memory_block(dev);
> -       int ret, online_type;
> +       int ret;
> +
> +       if (online_type < 0)
> +               return -EINVAL;
>
>         ret = lock_device_hotplug_sysfs();
>         if (ret)
>                 return ret;
>
> -       if (sysfs_streq(buf, "online_kernel"))
> -               online_type = MMOP_ONLINE_KERNEL;
> -       else if (sysfs_streq(buf, "online_movable"))
> -               online_type = MMOP_ONLINE_MOVABLE;
> -       else if (sysfs_streq(buf, "online"))
> -               online_type = MMOP_ONLINE;
> -       else if (sysfs_streq(buf, "offline"))
> -               online_type = MMOP_OFFLINE;
> -       else {
> -               ret = -EINVAL;
> -               goto err;
> -       }
> -
>         switch (online_type) {
>         case MMOP_ONLINE_KERNEL:
>         case MMOP_ONLINE_MOVABLE:
> @@ -271,7 +280,6 @@ static ssize_t state_store(struct device *dev, struct device_attribute *attr,
>                 ret = -EINVAL; /* should never happen */
>         }
>
> -err:
>         unlock_device_hotplug();
>
>         if (ret < 0)
> --

Nice cleanup patch.
Acked-by: Pankaj Gupta <pankaj.gupta.linux@gmail.com>

> 2.24.1
>
>

^ permalink raw reply

* Re: [PATCH v3 2/8] drivers/base/memory: map MMOP_OFFLINE to 0
From: Pankaj Gupta @ 2020-03-19 16:29 UTC (permalink / raw)
  To: David Hildenbrand
  Cc: linux-kernel, linux-mm, linuxppc-dev, linux-hyperv, Wei Yang,
	Michal Hocko, Greg Kroah-Hartman, Andrew Morton, Michal Hocko,
	Oscar Salvador, Rafael J. Wysocki, Baoquan He
In-Reply-To: <20200319131221.14044-3-david@redhat.com>

> Historically, we used the value -1. Just treat 0 as the special
> case now. Clarify a comment (which was wrong, when we come via
> device_online() the first time, the online_type would have been 0 /
> MEM_ONLINE). The default is now always MMOP_OFFLINE. This removes the
> last user of the manual "-1", which didn't use the enum value.
>
> This is a preparation to use the online_type as an array index.
>
> Reviewed-by: Wei Yang <richard.weiyang@gmail.com>
> Acked-by: Michal Hocko <mhocko@suse.com>
> Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> Cc: Andrew Morton <akpm@linux-foundation.org>
> Cc: Michal Hocko <mhocko@kernel.org>
> Cc: Oscar Salvador <osalvador@suse.de>
> Cc: "Rafael J. Wysocki" <rafael@kernel.org>
> Cc: Baoquan He <bhe@redhat.com>
> Cc: Wei Yang <richard.weiyang@gmail.com>
> Signed-off-by: David Hildenbrand <david@redhat.com>
> ---
>  drivers/base/memory.c          | 11 ++++-------
>  include/linux/memory_hotplug.h |  2 +-
>  2 files changed, 5 insertions(+), 8 deletions(-)
>
> diff --git a/drivers/base/memory.c b/drivers/base/memory.c
> index 8c5ce42c0fc3..e7e77cafef80 100644
> --- a/drivers/base/memory.c
> +++ b/drivers/base/memory.c
> @@ -211,17 +211,14 @@ static int memory_subsys_online(struct device *dev)
>                 return 0;
>
>         /*
> -        * If we are called from state_store(), online_type will be
> -        * set >= 0 Otherwise we were called from the device online
> -        * attribute and need to set the online_type.
> +        * When called via device_online() without configuring the online_type,
> +        * we want to default to MMOP_ONLINE.
>          */
> -       if (mem->online_type < 0)
> +       if (mem->online_type == MMOP_OFFLINE)
>                 mem->online_type = MMOP_ONLINE;
>
>         ret = memory_block_change_state(mem, MEM_ONLINE, MEM_OFFLINE);
> -
> -       /* clear online_type */
> -       mem->online_type = -1;
> +       mem->online_type = MMOP_OFFLINE;
>
>         return ret;
>  }
> diff --git a/include/linux/memory_hotplug.h b/include/linux/memory_hotplug.h
> index 3aaf00db224c..76f3c617a8ab 100644
> --- a/include/linux/memory_hotplug.h
> +++ b/include/linux/memory_hotplug.h
> @@ -48,7 +48,7 @@ enum {
>  /* Types for control the zone type of onlined and offlined memory */
>  enum {
>         /* Offline the memory. */
> -       MMOP_OFFLINE = -1,
> +       MMOP_OFFLINE = 0,
>         /* Online the memory. Zone depends, see default_zone_for_pfn(). */
>         MMOP_ONLINE,
>         /* Online the memory to ZONE_NORMAL. */
> --
> 2.24.1

Looks good to me.
Acked-by: Pankaj Gupta <pankaj.gupta.linux@gmail.com>
>
>

^ permalink raw reply

* Re: [PATCH v3 1/8] drivers/base/memory: rename MMOP_ONLINE_KEEP to MMOP_ONLINE
From: Pankaj Gupta @ 2020-03-19 16:28 UTC (permalink / raw)
  To: David Hildenbrand
  Cc: linux-kernel, linux-mm, linuxppc-dev, linux-hyperv, Wei Yang,
	Greg Kroah-Hartman, Andrew Morton, Michal Hocko, Oscar Salvador,
	Rafael J. Wysocki, Baoquan He
In-Reply-To: <20200319131221.14044-2-david@redhat.com>

> The name is misleading and it's not really clear what is "kept". Let's just
> name it like the online_type name we expose to user space ("online").
>
> Add some documentation to the types.
>
> Reviewed-by: Wei Yang <richard.weiyang@gmail.com>
> Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> Cc: Andrew Morton <akpm@linux-foundation.org>
> Cc: Michal Hocko <mhocko@kernel.org>
> Cc: Oscar Salvador <osalvador@suse.de>
> Cc: "Rafael J. Wysocki" <rafael@kernel.org>
> Cc: Baoquan He <bhe@redhat.com>
> Cc: Wei Yang <richard.weiyang@gmail.com>
> Signed-off-by: David Hildenbrand <david@redhat.com>
> ---
>  drivers/base/memory.c          | 9 +++++----
>  include/linux/memory_hotplug.h | 6 +++++-
>  2 files changed, 10 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/base/memory.c b/drivers/base/memory.c
> index 6448c9ece2cb..8c5ce42c0fc3 100644
> --- a/drivers/base/memory.c
> +++ b/drivers/base/memory.c
> @@ -216,7 +216,7 @@ static int memory_subsys_online(struct device *dev)
>          * attribute and need to set the online_type.
>          */
>         if (mem->online_type < 0)
> -               mem->online_type = MMOP_ONLINE_KEEP;
> +               mem->online_type = MMOP_ONLINE;
>
>         ret = memory_block_change_state(mem, MEM_ONLINE, MEM_OFFLINE);
>
> @@ -251,7 +251,7 @@ static ssize_t state_store(struct device *dev, struct device_attribute *attr,
>         else if (sysfs_streq(buf, "online_movable"))
>                 online_type = MMOP_ONLINE_MOVABLE;
>         else if (sysfs_streq(buf, "online"))
> -               online_type = MMOP_ONLINE_KEEP;
> +               online_type = MMOP_ONLINE;
>         else if (sysfs_streq(buf, "offline"))
>                 online_type = MMOP_OFFLINE;
>         else {
> @@ -262,7 +262,7 @@ static ssize_t state_store(struct device *dev, struct device_attribute *attr,
>         switch (online_type) {
>         case MMOP_ONLINE_KERNEL:
>         case MMOP_ONLINE_MOVABLE:
> -       case MMOP_ONLINE_KEEP:
> +       case MMOP_ONLINE:
>                 /* mem->online_type is protected by device_hotplug_lock */
>                 mem->online_type = online_type;
>                 ret = device_online(&mem->dev);
> @@ -342,7 +342,8 @@ static ssize_t valid_zones_show(struct device *dev,
>         }
>
>         nid = mem->nid;
> -       default_zone = zone_for_pfn_range(MMOP_ONLINE_KEEP, nid, start_pfn, nr_pages);
> +       default_zone = zone_for_pfn_range(MMOP_ONLINE, nid, start_pfn,
> +                                         nr_pages);
>         strcat(buf, default_zone->name);
>
>         print_allowed_zone(buf, nid, start_pfn, nr_pages, MMOP_ONLINE_KERNEL,
> diff --git a/include/linux/memory_hotplug.h b/include/linux/memory_hotplug.h
> index 3195d11876ea..3aaf00db224c 100644
> --- a/include/linux/memory_hotplug.h
> +++ b/include/linux/memory_hotplug.h
> @@ -47,9 +47,13 @@ enum {
>
>  /* Types for control the zone type of onlined and offlined memory */
>  enum {
> +       /* Offline the memory. */
>         MMOP_OFFLINE = -1,
> -       MMOP_ONLINE_KEEP,
> +       /* Online the memory. Zone depends, see default_zone_for_pfn(). */
> +       MMOP_ONLINE,
> +       /* Online the memory to ZONE_NORMAL. */
>         MMOP_ONLINE_KERNEL,
> +       /* Online the memory to ZONE_MOVABLE. */
>         MMOP_ONLINE_MOVABLE,
>  };
>
> --
Looks good to me.

Acked-by: Pankaj Gupta <pankaj.gupta.linux@gmail.com>

> 2.24.1
>
>

^ permalink raw reply

* RE: [PATCH 0/4] x86/Hyper-V: Panic code path fixes
From: Michael Kelley @ 2020-03-19 16:07 UTC (permalink / raw)
  To: Michael Kelley, Tianyu Lan, KY Srinivasan, Haiyang Zhang,
	Stephen Hemminger, Wei Liu, tglx@linutronix.de, mingo@redhat.com,
	bp@alien8.de, hpa@zytor.com, x86@kernel.org
  Cc: Tianyu Lan, linux-hyperv@vger.kernel.org,
	linux-kernel@vger.kernel.org, vkuznets
In-Reply-To: <MW2PR2101MB10524F27F366005959A1007FD7F40@MW2PR2101MB1052.namprd21.prod.outlook.com>

From: Michael Kelley <mikelley@microsoft.com> Sent: Thursday, March 19, 2020 8:15 AM
> 
> From: Tianyu Lan <Tianyu.Lan@microsoft.com> Sent: Thursday, March 19, 2020 7:08 AM
> > >>
> > >> This patchset fixes some issues in the Hyper-V panic code path.
> > >> Patch 1 resolves issue that panic system still responses network
> > >> packets.
> > >> Patch 2-3 resolves crash enlightenment issues.
> > >> Patch 4 is to set crash_kexec_post_notifiers to true for Hyper-V
> > >> VM in order to report crash data or kmsg to host before running
> > >> kdump kernel.
> > >
> > > I still see an issue that isn't addressed by these patches.  The VMbus
> > > driver registers a "die notifier" and a "panic notifier".   But die() will
> > > eventually call panic() if panic_on_oops is set (which I think it typically
> > > is).  If the CRASH_NOTIFY_MSG option is *not* enabled, then
> > > hyperv_report_panic() could get called by the die notifier, and then
> > > again by the panic notifier.
> > >
> > > Do we even need the "die notifier"?  If it was removed, there would
> > > not be any notification to Hyper-V via the die() path unless panic_on_oops
> > > is set, which I think is actually the correct behavior.  I'm not
> > > completely clear on what is supposed to happen in general to the
> > > Linux kernel if panic_on_oops is not set. Does it try to continue to run?
> > > If so, then we should not be notifying Hyper-V if panic_on_oops is not
> > > set, and removing the die notifier is the right thing to do.
> > >
> >
> > hyperv_report_panic() has re-enter check inside and so kernel only
> > reports crash register data once during die().
> 
> Ah, yes, you are right.
> 
> > From comment in the
> > hyperv_report_panic(), register value reported in die chain is more
> > exact than value in panic chain. The register value in die chain is
> > passed by die() caller. Register value reported in panic chain
> > is collected in the hyperv_panic_event().
> >
> > If panic_on_oops is not set, the task should be killed and kernel
> > still runs. In this case, we may not trigger crash enlightenment.
> 
> I'm not completely clear on your last statement.   It seems like there
> is still a problem in that die() will call hyperv_report_panic() even if
> panic_on_oops is not set.  We will have reported a panic to Hyper-V
> even though the VM did not stop running.
> 
> Michael

There's one more issue to consider.  hv_kmsg_dump() skips calling
hyperv_report_panic_msg() if sysctl_record_panic_msg has been cleared
by a sysctl command.  (This sysctl option gives a customer the ability to 
increase privacy by not having the VM's dmesg contents sent to Hyper-V.)
In this case, the earlier hyperv_report_panic() call should be used.  Otherwise,
there would not be any notification to Hyper-V about the panic.

Michael

^ permalink raw reply

* RE: [PATCH 0/4] x86/Hyper-V: Panic code path fixes
From: Michael Kelley @ 2020-03-19 15:14 UTC (permalink / raw)
  To: Tianyu Lan, KY Srinivasan, Haiyang Zhang, Stephen Hemminger,
	Wei Liu, tglx@linutronix.de, mingo@redhat.com, bp@alien8.de,
	hpa@zytor.com, x86@kernel.org
  Cc: Tianyu Lan, linux-hyperv@vger.kernel.org,
	linux-kernel@vger.kernel.org, vkuznets
In-Reply-To: <1d1bc90c-7fbe-6123-eeea-5f9a5aad77e4@gmail.com>

From: Tianyu Lan <Tianyu.Lan@microsoft.com> Sent: Thursday, March 19, 2020 7:08 AM
> >>
> >> This patchset fixes some issues in the Hyper-V panic code path.
> >> Patch 1 resolves issue that panic system still responses network
> >> packets.
> >> Patch 2-3 resolves crash enlightenment issues.
> >> Patch 4 is to set crash_kexec_post_notifiers to true for Hyper-V
> >> VM in order to report crash data or kmsg to host before running
> >> kdump kernel.
> >
> > I still see an issue that isn't addressed by these patches.  The VMbus
> > driver registers a "die notifier" and a "panic notifier".   But die() will
> > eventually call panic() if panic_on_oops is set (which I think it typically
> > is).  If the CRASH_NOTIFY_MSG option is *not* enabled, then
> > hyperv_report_panic() could get called by the die notifier, and then
> > again by the panic notifier.
> >
> > Do we even need the "die notifier"?  If it was removed, there would
> > not be any notification to Hyper-V via the die() path unless panic_on_oops
> > is set, which I think is actually the correct behavior.  I'm not
> > completely clear on what is supposed to happen in general to the
> > Linux kernel if panic_on_oops is not set. Does it try to continue to run?
> > If so, then we should not be notifying Hyper-V if panic_on_oops is not
> > set, and removing the die notifier is the right thing to do.
> >
> 
> hyperv_report_panic() has re-enter check inside and so kernel only
> reports crash register data once during die().

Ah, yes, you are right.

> From comment in the
> hyperv_report_panic(), register value reported in die chain is more
> exact than value in panic chain. The register value in die chain is
> passed by die() caller. Register value reported in panic chain
> is collected in the hyperv_panic_event(). 
> 
> If panic_on_oops is not set, the task should be killed and kernel
> still runs. In this case, we may not trigger crash enlightenment.

I'm not completely clear on your last statement.   It seems like there
is still a problem in that die() will call hyperv_report_panic() even if
panic_on_oops is not set.  We will have reported a panic to Hyper-V
even though the VM did not stop running.

Michael

^ permalink raw reply

* RE: [PATCH 0/4] x86/Hyper-V: Unload vmbus channel in hv panic callback
From: Michael Kelley @ 2020-03-19 15:06 UTC (permalink / raw)
  To: vkuznets, ltykernel@gmail.com
  Cc: Tianyu Lan, linux-hyperv@vger.kernel.org,
	linux-kernel@vger.kernel.org, KY Srinivasan, Haiyang Zhang,
	Stephen Hemminger, Wei Liu, tglx@linutronix.de, mingo@redhat.com,
	bp@alien8.de, hpa@zytor.com, x86@kernel.org
In-Reply-To: <87mu8c22ky.fsf@vitty.brq.redhat.com>

From: Vitaly Kuznetsov <vkuznets@redhat.com>  Sent: Thursday, March 19, 2020 1:04 AM
> 
> Michael Kelley <mikelley@microsoft.com> writes:
> 
> >> > --- a/drivers/hv/vmbus_drv.c
> >> > +++ b/drivers/hv/vmbus_drv.c
> >> > @@ -53,9 +53,12 @@ static int hyperv_panic_event(struct notifier_block *nb,
> unsigned
> >> long val,
> >> >  {
> >> >  	struct pt_regs *regs;
> >> >
> >> > -	regs = current_pt_regs();
> >> > +	vmbus_initiate_unload(true);
> >> >
> >> > -	hyperv_report_panic(regs, val);
> >> > +	if (ms_hyperv.misc_features & HV_FEATURE_GUEST_CRASH_MSR_AVAILABLE) {
> >>
> >> With Michael's effors to make code in drivers/hv arch agnostic, I think
> >> we need a better, arch-neutral way.
> >
> > Vitaly -- could you elaborate on what part is not arch-neutral?  I don't see
> > a problem.  ms_hyperv and the misc_features field exist for both the x86
> > and ARM64 code branches.  It turns out the particular bit for
> > GUEST_CRASH_MSR_AVAILABLE is different on the two architectures, but
> > the compiler will do the right thing.
> >
> 
> Ah, apologies, missed the fact that we also call it
> 'HV_FEATURE_GUEST_CRASH_MSR_AVAILABLE' - I have to admit I was confused
> by the 'MSR' part. We can probably rename this to something like
> HV_FEATURE_GUEST_CRASH_REGS_AVAILABLE - but not as part of the series.
> 

Good point.  Agreed.

Michael

^ permalink raw reply

* Re: [PATCH 0/4] x86/Hyper-V: Panic code path fixes
From: Tianyu Lan @ 2020-03-19 14:08 UTC (permalink / raw)
  To: Michael Kelley, KY Srinivasan, Haiyang Zhang, Stephen Hemminger,
	Wei Liu, tglx@linutronix.de, mingo@redhat.com, bp@alien8.de,
	hpa@zytor.com, x86@kernel.org
  Cc: Tianyu Lan, linux-hyperv@vger.kernel.org,
	linux-kernel@vger.kernel.org, vkuznets
In-Reply-To: <MW2PR2101MB1052F185AF4134EB2BB9ECBFD7F40@MW2PR2101MB1052.namprd21.prod.outlook.com>

Hi Michael:
      Thanks for your review.

On 3/19/2020 8:57 AM, Michael Kelley wrote:
> From: ltykernel@gmail.com <ltykernel@gmail.com> Sent: Tuesday, March 17, 2020 6:25 AM
>>
>> This patchset fixes some issues in the Hyper-V panic code path.
>> Patch 1 resolves issue that panic system still responses network
>> packets.
>> Patch 2-3 resolves crash enlightenment issues.
>> Patch 4 is to set crash_kexec_post_notifiers to true for Hyper-V
>> VM in order to report crash data or kmsg to host before running
>> kdump kernel.
> 
> I still see an issue that isn't addressed by these patches.  The VMbus
> driver registers a "die notifier" and a "panic notifier".   But die() will
> eventually call panic() if panic_on_oops is set (which I think it typically
> is).  If the CRASH_NOTIFY_MSG option is *not* enabled, then
> hyperv_report_panic() could get called by the die notifier, and then
> again by the panic notifier.
> 
> Do we even need the "die notifier"?  If it was removed, there would
> not be any notification to Hyper-V via the die() path unless panic_on_oops
> is set, which I think is actually the correct behavior.  I'm not
> completely clear on what is supposed to happen in general to the
> Linux kernel if panic_on_oops is not set. Does it try to continue to run?
> If so, then we should not be notifying Hyper-V if panic_on_oops is not
> set, and removing the die notifier is the right thing to do.
> 

hyperv_report_panic() has re-enter check inside and so kernel only 
reports crash register data once during die(). From comment in the
hyperv_report_panic(), register value reported in die chain is more
exact than value in panic chain. The register value in die chain is
passed by die() caller. Register value reported in panic chain
is collected in the hyperv_panic_event().


If panic_on_oops is not set, the task should be killed and kernel
still runs. In this case, we may not trigger crash enlightenment.



> Michael
> 
>>
>> Tianyu Lan (4):
>>    x86/Hyper-V: Unload vmbus channel in hv panic callback
>>    x86/Hyper-V: Free hv_panic_page when fail to register kmsg dump
>>    x86/Hyper-V: Trigger crash enlightenment only once during  system
>>      crash.
>>    x86/Hyper-V: Report crash register data or ksmg before running crash
>>      kernel
>>
>>   arch/x86/kernel/cpu/mshyperv.c | 10 ++++++++++
>>   drivers/hv/channel_mgmt.c      |  5 +++++
>>   drivers/hv/vmbus_drv.c         | 35 +++++++++++++++++++++++++----------
>>   3 files changed, 40 insertions(+), 10 deletions(-)
>>
>> --
>> 2.14.5
> 

^ permalink raw reply

* [PATCH v3 1/8] drivers/base/memory: rename MMOP_ONLINE_KEEP to MMOP_ONLINE
From: David Hildenbrand @ 2020-03-19 13:12 UTC (permalink / raw)
  To: linux-kernel
  Cc: linux-mm, linuxppc-dev, linux-hyperv, David Hildenbrand, Wei Yang,
	Greg Kroah-Hartman, Andrew Morton, Michal Hocko, Oscar Salvador,
	Rafael J. Wysocki, Baoquan He
In-Reply-To: <20200319131221.14044-1-david@redhat.com>

The name is misleading and it's not really clear what is "kept". Let's just
name it like the online_type name we expose to user space ("online").

Add some documentation to the types.

Reviewed-by: Wei Yang <richard.weiyang@gmail.com>
Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Cc: Andrew Morton <akpm@linux-foundation.org>
Cc: Michal Hocko <mhocko@kernel.org>
Cc: Oscar Salvador <osalvador@suse.de>
Cc: "Rafael J. Wysocki" <rafael@kernel.org>
Cc: Baoquan He <bhe@redhat.com>
Cc: Wei Yang <richard.weiyang@gmail.com>
Signed-off-by: David Hildenbrand <david@redhat.com>
---
 drivers/base/memory.c          | 9 +++++----
 include/linux/memory_hotplug.h | 6 +++++-
 2 files changed, 10 insertions(+), 5 deletions(-)

diff --git a/drivers/base/memory.c b/drivers/base/memory.c
index 6448c9ece2cb..8c5ce42c0fc3 100644
--- a/drivers/base/memory.c
+++ b/drivers/base/memory.c
@@ -216,7 +216,7 @@ static int memory_subsys_online(struct device *dev)
 	 * attribute and need to set the online_type.
 	 */
 	if (mem->online_type < 0)
-		mem->online_type = MMOP_ONLINE_KEEP;
+		mem->online_type = MMOP_ONLINE;
 
 	ret = memory_block_change_state(mem, MEM_ONLINE, MEM_OFFLINE);
 
@@ -251,7 +251,7 @@ static ssize_t state_store(struct device *dev, struct device_attribute *attr,
 	else if (sysfs_streq(buf, "online_movable"))
 		online_type = MMOP_ONLINE_MOVABLE;
 	else if (sysfs_streq(buf, "online"))
-		online_type = MMOP_ONLINE_KEEP;
+		online_type = MMOP_ONLINE;
 	else if (sysfs_streq(buf, "offline"))
 		online_type = MMOP_OFFLINE;
 	else {
@@ -262,7 +262,7 @@ static ssize_t state_store(struct device *dev, struct device_attribute *attr,
 	switch (online_type) {
 	case MMOP_ONLINE_KERNEL:
 	case MMOP_ONLINE_MOVABLE:
-	case MMOP_ONLINE_KEEP:
+	case MMOP_ONLINE:
 		/* mem->online_type is protected by device_hotplug_lock */
 		mem->online_type = online_type;
 		ret = device_online(&mem->dev);
@@ -342,7 +342,8 @@ static ssize_t valid_zones_show(struct device *dev,
 	}
 
 	nid = mem->nid;
-	default_zone = zone_for_pfn_range(MMOP_ONLINE_KEEP, nid, start_pfn, nr_pages);
+	default_zone = zone_for_pfn_range(MMOP_ONLINE, nid, start_pfn,
+					  nr_pages);
 	strcat(buf, default_zone->name);
 
 	print_allowed_zone(buf, nid, start_pfn, nr_pages, MMOP_ONLINE_KERNEL,
diff --git a/include/linux/memory_hotplug.h b/include/linux/memory_hotplug.h
index 3195d11876ea..3aaf00db224c 100644
--- a/include/linux/memory_hotplug.h
+++ b/include/linux/memory_hotplug.h
@@ -47,9 +47,13 @@ enum {
 
 /* Types for control the zone type of onlined and offlined memory */
 enum {
+	/* Offline the memory. */
 	MMOP_OFFLINE = -1,
-	MMOP_ONLINE_KEEP,
+	/* Online the memory. Zone depends, see default_zone_for_pfn(). */
+	MMOP_ONLINE,
+	/* Online the memory to ZONE_NORMAL. */
 	MMOP_ONLINE_KERNEL,
+	/* Online the memory to ZONE_MOVABLE. */
 	MMOP_ONLINE_MOVABLE,
 };
 
-- 
2.24.1


^ permalink raw reply related

* [PATCH v3 0/8] mm/memory_hotplug: allow to specify a default online_type
From: David Hildenbrand @ 2020-03-19 13:12 UTC (permalink / raw)
  To: linux-kernel
  Cc: linux-mm, linuxppc-dev, linux-hyperv, David Hildenbrand,
	Vitaly Kuznetsov, Yumei Huang, Igor Mammedov, Baoquan He,
	Eduardo Habkost, Milan Zamazal, Andrew Morton,
	Benjamin Herrenschmidt, Greg Kroah-Hartman, Haiyang Zhang,
	K. Y. Srinivasan, Michael Ellerman, Michal Hocko, Michal Hocko,
	Oscar Salvador, Paul Mackerras, Rafael J. Wysocki,
	Stephen Hemminger, Wei Liu, Wei Yang

Distributions nowadays use udev rules ([1] [2]) to specify if and
how to online hotplugged memory. The rules seem to get more complex with
many special cases. Due to the various special cases,
CONFIG_MEMORY_HOTPLUG_DEFAULT_ONLINE cannot be used. All memory hotplug
is handled via udev rules.

Everytime we hotplug memory, the udev rule will come to the same
conclusion. Especially Hyper-V (but also soon virtio-mem) add a lot of
memory in separate memory blocks and wait for memory to get onlined by user
space before continuing to add more memory blocks (to not add memory faster
than it is getting onlined). This of course slows down the whole memory
hotplug process.

To make the job of distributions easier and to avoid udev rules that get
more and more complicated, let's extend the mechanism provided by
- /sys/devices/system/memory/auto_online_blocks
- "memhp_default_state=" on the kernel cmdline
to be able to specify also "online_movable" as well as "online_kernel"

v2 -> v3:
- "hv_balloon: don't check for memhp_auto_online manually"
-- init_completion() before register_memory_notifier()
- Minor typo fix

v1 -> v2:
- Tweaked some patch descriptions
- Added
-- "powernv/memtrace: always online added memory blocks"
-- "hv_balloon: don't check for memhp_auto_online manually"
-- "mm/memory_hotplug: unexport memhp_auto_online"
- "mm/memory_hotplug: convert memhp_auto_online to store an online_type"
-- No longer touches hv/memtrace code


=== Example /usr/libexec/config-memhotplug ===

#!/bin/bash

VIRT=`systemd-detect-virt --vm`
ARCH=`uname -p`

sense_virtio_mem() {
  if [ -d "/sys/bus/virtio/drivers/virtio_mem/" ]; then
    DEVICES=`find /sys/bus/virtio/drivers/virtio_mem/ -maxdepth 1 -type l | wc -l`
    if [ $DEVICES != "0" ]; then
        return 0
    fi
  fi
  return 1
}

if [ ! -e "/sys/devices/system/memory/auto_online_blocks" ]; then
  echo "Memory hotplug configuration support missing in the kernel"
  exit 1
fi

if grep "memhp_default_state=" /proc/cmdline > /dev/null; then
  echo "Memory hotplug configuration overridden in kernel cmdline (memhp_default_state=)"
  exit 1
fi

if [ $VIRT == "microsoft" ]; then
  echo "Detected Hyper-V on $ARCH"
  # Hyper-V wants all memory in ZONE_NORMAL
  ONLINE_TYPE="online_kernel"
elif sense_virtio_mem; then
  echo "Detected virtio-mem on $ARCH"
  # virtio-mem wants all memory in ZONE_NORMAL
  ONLINE_TYPE="online_kernel"
elif [ $ARCH == "s390x" ] || [ $ARCH == "s390" ]; then
  echo "Detected $ARCH"
  # standby memory should not be onlined automatically
  ONLINE_TYPE="offline"
elif [ $ARCH == "ppc64" ] || [ $ARCH == "ppc64le" ]; then
  echo "Detected" $ARCH
  # PPC64 onlines all hotplugged memory right from the kernel
  ONLINE_TYPE="offline"
elif [ $VIRT == "none" ]; then
  echo "Detected bare-metal on $ARCH"
  # Bare metal users expect hotplugged memory to be unpluggable. We assume
  # that ZONE imbalances on such enterpise servers cannot happen and is
  # properly documented
  ONLINE_TYPE="online_movable"
else
  # TODO: Hypervisors that want to unplug DIMMs and can guarantee that ZONE
  # imbalances won't happen
  echo "Detected $VIRT on $ARCH"
  # Usually, ballooning is used in virtual environments, so memory should go to
  # ZONE_NORMAL. However, sometimes "movable_node" is relevant.
  ONLINE_TYPE="online"
fi

echo "Selected online_type:" $ONLINE_TYPE

# Configure what to do with memory that will be hotplugged in the future
echo $ONLINE_TYPE 2>/dev/null > /sys/devices/system/memory/auto_online_blocks
if [ $? != "0" ]; then
  echo "Memory hotplug cannot be configured (e.g., old kernel or missing permissions)"
  # A backup udev rule should handle old kernels if necessary
  exit 1
fi

# Process all already pluggedd blocks (e.g., DIMMs, but also Hyper-V or virtio-mem)
if [ $ONLINE_TYPE != "offline" ]; then
  for MEMORY in /sys/devices/system/memory/memory*; do
    STATE=`cat $MEMORY/state`
    if [ $STATE == "offline" ]; then
        echo $ONLINE_TYPE > $MEMORY/state
    fi
  done
fi


=== Example /usr/lib/systemd/system/config-memhotplug.service ===

[Unit]
Description=Configure memory hotplug behavior
DefaultDependencies=no
Conflicts=shutdown.target
Before=sysinit.target shutdown.target
After=systemd-modules-load.service
ConditionPathExists=|/sys/devices/system/memory/auto_online_blocks

[Service]
ExecStart=/usr/libexec/config-memhotplug
Type=oneshot
TimeoutSec=0
RemainAfterExit=yes

[Install]
WantedBy=sysinit.target


=== Example modification to the 40-redhat.rules [2] ===

diff --git a/40-redhat.rules b/40-redhat.rules-new
index 2c690e5..168fd03 100644
--- a/40-redhat.rules
+++ b/40-redhat.rules-new
@@ -6,6 +6,9 @@ SUBSYSTEM=="cpu", ACTION=="add", TEST=="online", ATTR{online}=="0", ATTR{online}
 # Memory hotadd request
 SUBSYSTEM!="memory", GOTO="memory_hotplug_end"
 ACTION!="add", GOTO="memory_hotplug_end"
+# memory hotplug behavior configured
+PROGRAM=="grep online /sys/devices/system/memory/auto_online_blocks", GOTO="memory_hotplug_end"
+
 PROGRAM="/bin/uname -p", RESULT=="s390*", GOTO="memory_hotplug_end"

 ENV{.state}="online"

===


[1] https://github.com/lnykryn/systemd-rhel/pull/281
[2] https://github.com/lnykryn/systemd-rhel/blob/staging/rules/40-redhat.rules

Cc: Vitaly Kuznetsov <vkuznets@redhat.com>
Cc: Yumei Huang <yuhuang@redhat.com>
Cc: Igor Mammedov <imammedo@redhat.com>
Cc: Baoquan He <bhe@redhat.com>
Cc: Eduardo Habkost <ehabkost@redhat.com>
Cc: Milan Zamazal <mzamazal@redhat.com>

David Hildenbrand (8):
  drivers/base/memory: rename MMOP_ONLINE_KEEP to MMOP_ONLINE
  drivers/base/memory: map MMOP_OFFLINE to 0
  drivers/base/memory: store mapping between MMOP_* and string in an
    array
  powernv/memtrace: always online added memory blocks
  hv_balloon: don't check for memhp_auto_online manually
  mm/memory_hotplug: unexport memhp_auto_online
  mm/memory_hotplug: convert memhp_auto_online to store an online_type
  mm/memory_hotplug: allow to specify a default online_type

 arch/powerpc/platforms/powernv/memtrace.c | 14 ++---
 drivers/base/memory.c                     | 71 ++++++++++++-----------
 drivers/hv/hv_balloon.c                   | 25 ++++----
 include/linux/memory_hotplug.h            | 13 ++++-
 mm/memory_hotplug.c                       | 16 ++---
 5 files changed, 69 insertions(+), 70 deletions(-)

-- 
2.24.1


^ permalink raw reply related

* [PATCH v3 2/8] drivers/base/memory: map MMOP_OFFLINE to 0
From: David Hildenbrand @ 2020-03-19 13:12 UTC (permalink / raw)
  To: linux-kernel
  Cc: linux-mm, linuxppc-dev, linux-hyperv, David Hildenbrand, Wei Yang,
	Michal Hocko, Greg Kroah-Hartman, Andrew Morton, Michal Hocko,
	Oscar Salvador, Rafael J. Wysocki, Baoquan He
In-Reply-To: <20200319131221.14044-1-david@redhat.com>

Historically, we used the value -1. Just treat 0 as the special
case now. Clarify a comment (which was wrong, when we come via
device_online() the first time, the online_type would have been 0 /
MEM_ONLINE). The default is now always MMOP_OFFLINE. This removes the
last user of the manual "-1", which didn't use the enum value.

This is a preparation to use the online_type as an array index.

Reviewed-by: Wei Yang <richard.weiyang@gmail.com>
Acked-by: Michal Hocko <mhocko@suse.com>
Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Cc: Andrew Morton <akpm@linux-foundation.org>
Cc: Michal Hocko <mhocko@kernel.org>
Cc: Oscar Salvador <osalvador@suse.de>
Cc: "Rafael J. Wysocki" <rafael@kernel.org>
Cc: Baoquan He <bhe@redhat.com>
Cc: Wei Yang <richard.weiyang@gmail.com>
Signed-off-by: David Hildenbrand <david@redhat.com>
---
 drivers/base/memory.c          | 11 ++++-------
 include/linux/memory_hotplug.h |  2 +-
 2 files changed, 5 insertions(+), 8 deletions(-)

diff --git a/drivers/base/memory.c b/drivers/base/memory.c
index 8c5ce42c0fc3..e7e77cafef80 100644
--- a/drivers/base/memory.c
+++ b/drivers/base/memory.c
@@ -211,17 +211,14 @@ static int memory_subsys_online(struct device *dev)
 		return 0;
 
 	/*
-	 * If we are called from state_store(), online_type will be
-	 * set >= 0 Otherwise we were called from the device online
-	 * attribute and need to set the online_type.
+	 * When called via device_online() without configuring the online_type,
+	 * we want to default to MMOP_ONLINE.
 	 */
-	if (mem->online_type < 0)
+	if (mem->online_type == MMOP_OFFLINE)
 		mem->online_type = MMOP_ONLINE;
 
 	ret = memory_block_change_state(mem, MEM_ONLINE, MEM_OFFLINE);
-
-	/* clear online_type */
-	mem->online_type = -1;
+	mem->online_type = MMOP_OFFLINE;
 
 	return ret;
 }
diff --git a/include/linux/memory_hotplug.h b/include/linux/memory_hotplug.h
index 3aaf00db224c..76f3c617a8ab 100644
--- a/include/linux/memory_hotplug.h
+++ b/include/linux/memory_hotplug.h
@@ -48,7 +48,7 @@ enum {
 /* Types for control the zone type of onlined and offlined memory */
 enum {
 	/* Offline the memory. */
-	MMOP_OFFLINE = -1,
+	MMOP_OFFLINE = 0,
 	/* Online the memory. Zone depends, see default_zone_for_pfn(). */
 	MMOP_ONLINE,
 	/* Online the memory to ZONE_NORMAL. */
-- 
2.24.1


^ permalink raw reply related

* [PATCH v3 3/8] drivers/base/memory: store mapping between MMOP_* and string in an array
From: David Hildenbrand @ 2020-03-19 13:12 UTC (permalink / raw)
  To: linux-kernel
  Cc: linux-mm, linuxppc-dev, linux-hyperv, David Hildenbrand, Wei Yang,
	Michal Hocko, Greg Kroah-Hartman, Andrew Morton, Michal Hocko,
	Oscar Salvador, Rafael J. Wysocki, Baoquan He
In-Reply-To: <20200319131221.14044-1-david@redhat.com>

Let's use a simple array which we can reuse soon. While at it, move the
string->mmop conversion out of the device hotplug lock.

Reviewed-by: Wei Yang <richard.weiyang@gmail.com>
Acked-by: Michal Hocko <mhocko@suse.com>
Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Cc: Andrew Morton <akpm@linux-foundation.org>
Cc: Michal Hocko <mhocko@kernel.org>
Cc: Oscar Salvador <osalvador@suse.de>
Cc: "Rafael J. Wysocki" <rafael@kernel.org>
Cc: Baoquan He <bhe@redhat.com>
Cc: Wei Yang <richard.weiyang@gmail.com>
Signed-off-by: David Hildenbrand <david@redhat.com>
---
 drivers/base/memory.c | 38 +++++++++++++++++++++++---------------
 1 file changed, 23 insertions(+), 15 deletions(-)

diff --git a/drivers/base/memory.c b/drivers/base/memory.c
index e7e77cafef80..8a7f29c0bf97 100644
--- a/drivers/base/memory.c
+++ b/drivers/base/memory.c
@@ -28,6 +28,24 @@
 
 #define MEMORY_CLASS_NAME	"memory"
 
+static const char *const online_type_to_str[] = {
+	[MMOP_OFFLINE] = "offline",
+	[MMOP_ONLINE] = "online",
+	[MMOP_ONLINE_KERNEL] = "online_kernel",
+	[MMOP_ONLINE_MOVABLE] = "online_movable",
+};
+
+static int memhp_online_type_from_str(const char *str)
+{
+	int i;
+
+	for (i = 0; i < ARRAY_SIZE(online_type_to_str); i++) {
+		if (sysfs_streq(str, online_type_to_str[i]))
+			return i;
+	}
+	return -EINVAL;
+}
+
 #define to_memory_block(dev) container_of(dev, struct memory_block, dev)
 
 static int sections_per_block;
@@ -236,26 +254,17 @@ static int memory_subsys_offline(struct device *dev)
 static ssize_t state_store(struct device *dev, struct device_attribute *attr,
 			   const char *buf, size_t count)
 {
+	const int online_type = memhp_online_type_from_str(buf);
 	struct memory_block *mem = to_memory_block(dev);
-	int ret, online_type;
+	int ret;
+
+	if (online_type < 0)
+		return -EINVAL;
 
 	ret = lock_device_hotplug_sysfs();
 	if (ret)
 		return ret;
 
-	if (sysfs_streq(buf, "online_kernel"))
-		online_type = MMOP_ONLINE_KERNEL;
-	else if (sysfs_streq(buf, "online_movable"))
-		online_type = MMOP_ONLINE_MOVABLE;
-	else if (sysfs_streq(buf, "online"))
-		online_type = MMOP_ONLINE;
-	else if (sysfs_streq(buf, "offline"))
-		online_type = MMOP_OFFLINE;
-	else {
-		ret = -EINVAL;
-		goto err;
-	}
-
 	switch (online_type) {
 	case MMOP_ONLINE_KERNEL:
 	case MMOP_ONLINE_MOVABLE:
@@ -271,7 +280,6 @@ static ssize_t state_store(struct device *dev, struct device_attribute *attr,
 		ret = -EINVAL; /* should never happen */
 	}
 
-err:
 	unlock_device_hotplug();
 
 	if (ret < 0)
-- 
2.24.1


^ permalink raw reply related

* [PATCH v3 4/8] powernv/memtrace: always online added memory blocks
From: David Hildenbrand @ 2020-03-19 13:12 UTC (permalink / raw)
  To: linux-kernel
  Cc: linux-mm, linuxppc-dev, linux-hyperv, David Hildenbrand, Wei Yang,
	Michal Hocko, Michael Ellerman, Benjamin Herrenschmidt,
	Paul Mackerras, Andrew Morton, Greg Kroah-Hartman, Michal Hocko,
	Oscar Salvador, Rafael J. Wysocki, Baoquan He
In-Reply-To: <20200319131221.14044-1-david@redhat.com>

Let's always try to online the re-added memory blocks. In case add_memory()
already onlined the added memory blocks, the first device_online() call
will fail and stop processing the remaining memory blocks.

This avoids manually having to check memhp_auto_online.

Note: PPC always onlines all hotplugged memory directly from the kernel
as well - something that is handled by user space on other
architectures.

Reviewed-by: Wei Yang <richard.weiyang@gmail.com>
Acked-by: Michal Hocko <mhocko@suse.com>
Acked-by: Michael Ellerman <mpe@ellerman.id.au>
Cc: Benjamin Herrenschmidt <benh@kernel.crashing.org>
Cc: Paul Mackerras <paulus@samba.org>
Cc: Michael Ellerman <mpe@ellerman.id.au>
Cc: Andrew Morton <akpm@linux-foundation.org>
Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Cc: Michal Hocko <mhocko@kernel.org>
Cc: Oscar Salvador <osalvador@suse.de>
Cc: "Rafael J. Wysocki" <rafael@kernel.org>
Cc: Baoquan He <bhe@redhat.com>
Cc: Wei Yang <richard.weiyang@gmail.com>
Cc: linuxppc-dev@lists.ozlabs.org
Signed-off-by: David Hildenbrand <david@redhat.com>
---
 arch/powerpc/platforms/powernv/memtrace.c | 14 ++++----------
 1 file changed, 4 insertions(+), 10 deletions(-)

diff --git a/arch/powerpc/platforms/powernv/memtrace.c b/arch/powerpc/platforms/powernv/memtrace.c
index d6d64f8718e6..13b369d2cc45 100644
--- a/arch/powerpc/platforms/powernv/memtrace.c
+++ b/arch/powerpc/platforms/powernv/memtrace.c
@@ -231,16 +231,10 @@ static int memtrace_online(void)
 			continue;
 		}
 
-		/*
-		 * If kernel isn't compiled with the auto online option
-		 * we need to online the memory ourselves.
-		 */
-		if (!memhp_auto_online) {
-			lock_device_hotplug();
-			walk_memory_blocks(ent->start, ent->size, NULL,
-					   online_mem_block);
-			unlock_device_hotplug();
-		}
+		lock_device_hotplug();
+		walk_memory_blocks(ent->start, ent->size, NULL,
+				   online_mem_block);
+		unlock_device_hotplug();
 
 		/*
 		 * Memory was added successfully so clean up references to it
-- 
2.24.1


^ permalink raw reply related

* [PATCH v3 5/8] hv_balloon: don't check for memhp_auto_online manually
From: David Hildenbrand @ 2020-03-19 13:12 UTC (permalink / raw)
  To: linux-kernel
  Cc: linux-mm, linuxppc-dev, linux-hyperv, David Hildenbrand,
	Vitaly Kuznetsov, K. Y. Srinivasan, Haiyang Zhang,
	Stephen Hemminger, Wei Liu, Andrew Morton, Michal Hocko,
	Oscar Salvador, Rafael J. Wysocki, Baoquan He, Wei Yang
In-Reply-To: <20200319131221.14044-1-david@redhat.com>

We get the MEM_ONLINE notifier call if memory is added right from the
kernel via add_memory() or later from user space.

Let's get rid of the "ha_waiting" flag - the wait event has an inbuilt
mechanism (->done) for that. Initialize the wait event only once and
reinitialize before adding memory. Unconditionally call complete() and
wait_for_completion_timeout().

If there are no waiters, complete() will only increment ->done - which
will be reset by reinit_completion(). If complete() has already been
called, wait_for_completion_timeout() will not wait.

There is still the chance for a small race between concurrent
reinit_completion() and complete(). If complete() wins, we would not
wait - which is tolerable (and the race exists in current code as well).

Note: We only wait for "some" memory to get onlined, which seems to be
      good enough for now.

Reviewed-by: Vitaly Kuznetsov <vkuznets@redhat.com>
Cc: "K. Y. Srinivasan" <kys@microsoft.com>
Cc: Haiyang Zhang <haiyangz@microsoft.com>
Cc: Stephen Hemminger <sthemmin@microsoft.com>
Cc: Wei Liu <wei.liu@kernel.org>
Cc: Andrew Morton <akpm@linux-foundation.org>
Cc: Michal Hocko <mhocko@kernel.org>
Cc: Oscar Salvador <osalvador@suse.de>
Cc: "Rafael J. Wysocki" <rafael@kernel.org>
Cc: Baoquan He <bhe@redhat.com>
Cc: Wei Yang <richard.weiyang@gmail.com>
Cc: Vitaly Kuznetsov <vkuznets@redhat.com>
Cc: linux-hyperv@vger.kernel.org
Signed-off-by: David Hildenbrand <david@redhat.com>
---
 drivers/hv/hv_balloon.c | 25 ++++++++++---------------
 1 file changed, 10 insertions(+), 15 deletions(-)

diff --git a/drivers/hv/hv_balloon.c b/drivers/hv/hv_balloon.c
index a02ce43d778d..32e3bc0aa665 100644
--- a/drivers/hv/hv_balloon.c
+++ b/drivers/hv/hv_balloon.c
@@ -533,7 +533,6 @@ struct hv_dynmem_device {
 	 * State to synchronize hot-add.
 	 */
 	struct completion  ol_waitevent;
-	bool ha_waiting;
 	/*
 	 * This thread handles hot-add
 	 * requests from the host as well as notifying
@@ -634,10 +633,7 @@ static int hv_memory_notifier(struct notifier_block *nb, unsigned long val,
 	switch (val) {
 	case MEM_ONLINE:
 	case MEM_CANCEL_ONLINE:
-		if (dm_device.ha_waiting) {
-			dm_device.ha_waiting = false;
-			complete(&dm_device.ol_waitevent);
-		}
+		complete(&dm_device.ol_waitevent);
 		break;
 
 	case MEM_OFFLINE:
@@ -726,8 +722,7 @@ static void hv_mem_hot_add(unsigned long start, unsigned long size,
 		has->covered_end_pfn +=  processed_pfn;
 		spin_unlock_irqrestore(&dm_device.ha_lock, flags);
 
-		init_completion(&dm_device.ol_waitevent);
-		dm_device.ha_waiting = !memhp_auto_online;
+		reinit_completion(&dm_device.ol_waitevent);
 
 		nid = memory_add_physaddr_to_nid(PFN_PHYS(start_pfn));
 		ret = add_memory(nid, PFN_PHYS((start_pfn)),
@@ -753,15 +748,14 @@ static void hv_mem_hot_add(unsigned long start, unsigned long size,
 		}
 
 		/*
-		 * Wait for the memory block to be onlined when memory onlining
-		 * is done outside of kernel (memhp_auto_online). Since the hot
-		 * add has succeeded, it is ok to proceed even if the pages in
-		 * the hot added region have not been "onlined" within the
-		 * allowed time.
+		 * Wait for memory to get onlined. If the kernel onlined the
+		 * memory when adding it, this will return directly. Otherwise,
+		 * it will wait for user space to online the memory. This helps
+		 * to avoid adding memory faster than it is getting onlined. As
+		 * adding succeeded, it is ok to proceed even if the memory was
+		 * not onlined in time.
 		 */
-		if (dm_device.ha_waiting)
-			wait_for_completion_timeout(&dm_device.ol_waitevent,
-						    5*HZ);
+		wait_for_completion_timeout(&dm_device.ol_waitevent, 5 * HZ);
 		post_status(&dm_device);
 	}
 }
@@ -1706,6 +1700,7 @@ static int balloon_probe(struct hv_device *dev,
 
 #ifdef CONFIG_MEMORY_HOTPLUG
 	set_online_page_callback(&hv_online_page);
+	init_completion(&dm_device.ol_waitevent);
 	register_memory_notifier(&hv_memory_nb);
 #endif
 
-- 
2.24.1


^ permalink raw reply related

* [PATCH v3 6/8] mm/memory_hotplug: unexport memhp_auto_online
From: David Hildenbrand @ 2020-03-19 13:12 UTC (permalink / raw)
  To: linux-kernel
  Cc: linux-mm, linuxppc-dev, linux-hyperv, David Hildenbrand, Wei Yang,
	Michal Hocko, Andrew Morton, Michal Hocko, Oscar Salvador,
	Rafael J. Wysocki, Baoquan He
In-Reply-To: <20200319131221.14044-1-david@redhat.com>

All in-tree users except the mm-core are gone. Let's drop the export.

Reviewed-by: Wei Yang <richard.weiyang@gmail.com>
Acked-by: Michal Hocko <mhocko@suse.com>
Cc: Andrew Morton <akpm@linux-foundation.org>
Cc: Michal Hocko <mhocko@kernel.org>
Cc: Oscar Salvador <osalvador@suse.de>
Cc: "Rafael J. Wysocki" <rafael@kernel.org>
Cc: Baoquan He <bhe@redhat.com>
Cc: Wei Yang <richard.weiyang@gmail.com>
Signed-off-by: David Hildenbrand <david@redhat.com>
---
 mm/memory_hotplug.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/mm/memory_hotplug.c b/mm/memory_hotplug.c
index da6aab272c9b..e21a7d53ade5 100644
--- a/mm/memory_hotplug.c
+++ b/mm/memory_hotplug.c
@@ -71,7 +71,6 @@ bool memhp_auto_online;
 #else
 bool memhp_auto_online = true;
 #endif
-EXPORT_SYMBOL_GPL(memhp_auto_online);
 
 static int __init setup_memhp_default_state(char *str)
 {
-- 
2.24.1


^ permalink raw reply related

* [PATCH v3 7/8] mm/memory_hotplug: convert memhp_auto_online to store an online_type
From: David Hildenbrand @ 2020-03-19 13:12 UTC (permalink / raw)
  To: linux-kernel
  Cc: linux-mm, linuxppc-dev, linux-hyperv, David Hildenbrand, Wei Yang,
	Michal Hocko, Greg Kroah-Hartman, Andrew Morton, Michal Hocko,
	Oscar Salvador, Rafael J. Wysocki, Baoquan He
In-Reply-To: <20200319131221.14044-1-david@redhat.com>

... and rename it to memhp_default_online_type. This is a preparation
for more detailed default online behavior.

Reviewed-by: Wei Yang <richard.weiyang@gmail.com>
Acked-by: Michal Hocko <mhocko@suse.com>
Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Cc: Andrew Morton <akpm@linux-foundation.org>
Cc: Michal Hocko <mhocko@kernel.org>
Cc: Oscar Salvador <osalvador@suse.de>
Cc: "Rafael J. Wysocki" <rafael@kernel.org>
Cc: Baoquan He <bhe@redhat.com>
Cc: Wei Yang <richard.weiyang@gmail.com>
Signed-off-by: David Hildenbrand <david@redhat.com>
---
 drivers/base/memory.c          | 10 ++++------
 include/linux/memory_hotplug.h |  3 ++-
 mm/memory_hotplug.c            | 11 ++++++-----
 3 files changed, 12 insertions(+), 12 deletions(-)

diff --git a/drivers/base/memory.c b/drivers/base/memory.c
index 8a7f29c0bf97..8d3e16dab69f 100644
--- a/drivers/base/memory.c
+++ b/drivers/base/memory.c
@@ -386,10 +386,8 @@ static DEVICE_ATTR_RO(block_size_bytes);
 static ssize_t auto_online_blocks_show(struct device *dev,
 				       struct device_attribute *attr, char *buf)
 {
-	if (memhp_auto_online)
-		return sprintf(buf, "online\n");
-	else
-		return sprintf(buf, "offline\n");
+	return sprintf(buf, "%s\n",
+		       online_type_to_str[memhp_default_online_type]);
 }
 
 static ssize_t auto_online_blocks_store(struct device *dev,
@@ -397,9 +395,9 @@ static ssize_t auto_online_blocks_store(struct device *dev,
 					const char *buf, size_t count)
 {
 	if (sysfs_streq(buf, "online"))
-		memhp_auto_online = true;
+		memhp_default_online_type = MMOP_ONLINE;
 	else if (sysfs_streq(buf, "offline"))
-		memhp_auto_online = false;
+		memhp_default_online_type = MMOP_OFFLINE;
 	else
 		return -EINVAL;
 
diff --git a/include/linux/memory_hotplug.h b/include/linux/memory_hotplug.h
index 76f3c617a8ab..6d6f85bb66e9 100644
--- a/include/linux/memory_hotplug.h
+++ b/include/linux/memory_hotplug.h
@@ -118,7 +118,8 @@ extern int arch_add_memory(int nid, u64 start, u64 size,
 			   struct mhp_params *params);
 extern u64 max_mem_size;
 
-extern bool memhp_auto_online;
+/* Default online_type (MMOP_*) when new memory blocks are added. */
+extern int memhp_default_online_type;
 /* If movable_node boot option specified */
 extern bool movable_node_enabled;
 static inline bool movable_node_is_enabled(void)
diff --git a/mm/memory_hotplug.c b/mm/memory_hotplug.c
index e21a7d53ade5..4efcf8cb9ac5 100644
--- a/mm/memory_hotplug.c
+++ b/mm/memory_hotplug.c
@@ -67,17 +67,17 @@ void put_online_mems(void)
 bool movable_node_enabled = false;
 
 #ifndef CONFIG_MEMORY_HOTPLUG_DEFAULT_ONLINE
-bool memhp_auto_online;
+int memhp_default_online_type = MMOP_OFFLINE;
 #else
-bool memhp_auto_online = true;
+int memhp_default_online_type = MMOP_ONLINE;
 #endif
 
 static int __init setup_memhp_default_state(char *str)
 {
 	if (!strcmp(str, "online"))
-		memhp_auto_online = true;
+		memhp_default_online_type = MMOP_ONLINE;
 	else if (!strcmp(str, "offline"))
-		memhp_auto_online = false;
+		memhp_default_online_type = MMOP_OFFLINE;
 
 	return 1;
 }
@@ -993,6 +993,7 @@ static int check_hotplug_memory_range(u64 start, u64 size)
 
 static int online_memory_block(struct memory_block *mem, void *arg)
 {
+	mem->online_type = memhp_default_online_type;
 	return device_online(&mem->dev);
 }
 
@@ -1065,7 +1066,7 @@ int __ref add_memory_resource(int nid, struct resource *res)
 	mem_hotplug_done();
 
 	/* online pages if requested */
-	if (memhp_auto_online)
+	if (memhp_default_online_type != MMOP_OFFLINE)
 		walk_memory_blocks(start, size, NULL, online_memory_block);
 
 	return ret;
-- 
2.24.1


^ permalink raw reply related

* [PATCH v3 8/8] mm/memory_hotplug: allow to specify a default online_type
From: David Hildenbrand @ 2020-03-19 13:12 UTC (permalink / raw)
  To: linux-kernel
  Cc: linux-mm, linuxppc-dev, linux-hyperv, David Hildenbrand, Wei Yang,
	Michal Hocko, Greg Kroah-Hartman, Andrew Morton, Michal Hocko,
	Oscar Salvador, Rafael J. Wysocki, Baoquan He
In-Reply-To: <20200319131221.14044-1-david@redhat.com>

For now, distributions implement advanced udev rules to essentially
- Don't online any hotplugged memory (s390x)
- Online all memory to ZONE_NORMAL (e.g., most virt environments like
  hyperv)
- Online all memory to ZONE_MOVABLE in case the zone imbalance is taken
  care of (e.g., bare metal, special virt environments)

In summary: All memory is usually onlined the same way, however, the
kernel always has to ask user space to come up with the same answer.
E.g., Hyper-V always waits for a memory block to get onlined before
continuing, otherwise it might end up adding memory faster than
onlining it, which can result in strange OOM situations. This waiting
slows down adding of a bigger amount of memory.

Let's allow to specify a default online_type, not just "online" and
"offline". This allows distributions to configure the default online_type
when booting up and be done with it.

We can now specify "offline", "online", "online_movable" and
"online_kernel" via
- "memhp_default_state=" on the kernel cmdline
- /sys/devices/system/memory/auto_online_blocks
just like we are able to specify for a single memory block via
/sys/devices/system/memory/memoryX/state

Reviewed-by: Wei Yang <richard.weiyang@gmail.com>
Acked-by: Michal Hocko <mhocko@suse.com>
Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Cc: Andrew Morton <akpm@linux-foundation.org>
Cc: Michal Hocko <mhocko@kernel.org>
Cc: Oscar Salvador <osalvador@suse.de>
Cc: "Rafael J. Wysocki" <rafael@kernel.org>
Cc: Baoquan He <bhe@redhat.com>
Cc: Wei Yang <richard.weiyang@gmail.com>
Signed-off-by: David Hildenbrand <david@redhat.com>
---
 drivers/base/memory.c          | 11 +++++------
 include/linux/memory_hotplug.h |  2 ++
 mm/memory_hotplug.c            |  8 ++++----
 3 files changed, 11 insertions(+), 10 deletions(-)

diff --git a/drivers/base/memory.c b/drivers/base/memory.c
index 8d3e16dab69f..2b09b68b9f78 100644
--- a/drivers/base/memory.c
+++ b/drivers/base/memory.c
@@ -35,7 +35,7 @@ static const char *const online_type_to_str[] = {
 	[MMOP_ONLINE_MOVABLE] = "online_movable",
 };
 
-static int memhp_online_type_from_str(const char *str)
+int memhp_online_type_from_str(const char *str)
 {
 	int i;
 
@@ -394,13 +394,12 @@ static ssize_t auto_online_blocks_store(struct device *dev,
 					struct device_attribute *attr,
 					const char *buf, size_t count)
 {
-	if (sysfs_streq(buf, "online"))
-		memhp_default_online_type = MMOP_ONLINE;
-	else if (sysfs_streq(buf, "offline"))
-		memhp_default_online_type = MMOP_OFFLINE;
-	else
+	const int online_type = memhp_online_type_from_str(buf);
+
+	if (online_type < 0)
 		return -EINVAL;
 
+	memhp_default_online_type = online_type;
 	return count;
 }
 
diff --git a/include/linux/memory_hotplug.h b/include/linux/memory_hotplug.h
index 6d6f85bb66e9..93d9ada74ddd 100644
--- a/include/linux/memory_hotplug.h
+++ b/include/linux/memory_hotplug.h
@@ -118,6 +118,8 @@ extern int arch_add_memory(int nid, u64 start, u64 size,
 			   struct mhp_params *params);
 extern u64 max_mem_size;
 
+extern int memhp_online_type_from_str(const char *str);
+
 /* Default online_type (MMOP_*) when new memory blocks are added. */
 extern int memhp_default_online_type;
 /* If movable_node boot option specified */
diff --git a/mm/memory_hotplug.c b/mm/memory_hotplug.c
index 4efcf8cb9ac5..89197163d138 100644
--- a/mm/memory_hotplug.c
+++ b/mm/memory_hotplug.c
@@ -74,10 +74,10 @@ int memhp_default_online_type = MMOP_ONLINE;
 
 static int __init setup_memhp_default_state(char *str)
 {
-	if (!strcmp(str, "online"))
-		memhp_default_online_type = MMOP_ONLINE;
-	else if (!strcmp(str, "offline"))
-		memhp_default_online_type = MMOP_OFFLINE;
+	const int online_type = memhp_online_type_from_str(str);
+
+	if (online_type >= 0)
+		memhp_default_online_type = online_type;
 
 	return 1;
 }
-- 
2.24.1


^ permalink raw reply related

* Re: [PATCH v2 4/8] powernv/memtrace: always online added memory blocks
From: Michael Ellerman @ 2020-03-19  9:49 UTC (permalink / raw)
  To: David Hildenbrand, linux-kernel
  Cc: linux-mm, linuxppc-dev, linux-hyperv, David Hildenbrand,
	Benjamin Herrenschmidt, Paul Mackerras, Andrew Morton,
	Greg Kroah-Hartman, Michal Hocko, Oscar Salvador,
	Rafael J. Wysocki, Baoquan He, Wei Yang
In-Reply-To: <20200317104942.11178-5-david@redhat.com>

David Hildenbrand <david@redhat.com> writes:
> Let's always try to online the re-added memory blocks. In case add_memory()
> already onlined the added memory blocks, the first device_online() call
> will fail and stop processing the remaining memory blocks.
>
> This avoids manually having to check memhp_auto_online.
>
> Note: PPC always onlines all hotplugged memory directly from the kernel
> as well - something that is handled by user space on other
> architectures.
>
> Cc: Benjamin Herrenschmidt <benh@kernel.crashing.org>
> Cc: Paul Mackerras <paulus@samba.org>
> Cc: Michael Ellerman <mpe@ellerman.id.au>
> Cc: Andrew Morton <akpm@linux-foundation.org>
> Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> Cc: Michal Hocko <mhocko@kernel.org>
> Cc: Oscar Salvador <osalvador@suse.de>
> Cc: "Rafael J. Wysocki" <rafael@kernel.org>
> Cc: Baoquan He <bhe@redhat.com>
> Cc: Wei Yang <richard.weiyang@gmail.com>
> Cc: linuxppc-dev@lists.ozlabs.org
> Signed-off-by: David Hildenbrand <david@redhat.com>
> ---
>  arch/powerpc/platforms/powernv/memtrace.c | 14 ++++----------
>  1 file changed, 4 insertions(+), 10 deletions(-)

Fine by me.

Acked-by: Michael Ellerman <mpe@ellerman.id.au> (powerpc)

cheers

> diff --git a/arch/powerpc/platforms/powernv/memtrace.c b/arch/powerpc/platforms/powernv/memtrace.c
> index d6d64f8718e6..13b369d2cc45 100644
> --- a/arch/powerpc/platforms/powernv/memtrace.c
> +++ b/arch/powerpc/platforms/powernv/memtrace.c
> @@ -231,16 +231,10 @@ static int memtrace_online(void)
>  			continue;
>  		}
>  
> -		/*
> -		 * If kernel isn't compiled with the auto online option
> -		 * we need to online the memory ourselves.
> -		 */
> -		if (!memhp_auto_online) {
> -			lock_device_hotplug();
> -			walk_memory_blocks(ent->start, ent->size, NULL,
> -					   online_mem_block);
> -			unlock_device_hotplug();
> -		}
> +		lock_device_hotplug();
> +		walk_memory_blocks(ent->start, ent->size, NULL,
> +				   online_mem_block);
> +		unlock_device_hotplug();
>  
>  		/*
>  		 * Memory was added successfully so clean up references to it
> -- 
> 2.24.1

^ permalink raw reply


This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox