Re: [PATCH v3 4/5] s390/mm: implement MEM_PREPARE_ONLINE/MEM_FINISH_OFFLINE notifiers

[David Hildenbrand]

On 27.11.23 17:12, Sumanth Korikkar wrote:
> On Mon, Nov 27, 2023 at 04:11:05PM +0100, David Hildenbrand wrote:
> > > diff --git a/drivers/s390/char/sclp_cmd.c b/drivers/s390/char/sclp_cmd.c
> > > index 355e63e44e95..30b829e4c052 100644
> > > --- a/drivers/s390/char/sclp_cmd.c
> > > +++ b/drivers/s390/char/sclp_cmd.c
> > > @@ -18,6 +18,7 @@
> > >    #include <linux/mm.h>
> > >    #include <linux/mmzone.h>
> > >    #include <linux/memory.h>
> > > +#include <linux/memory_hotplug.h>
> > >    #include <linux/module.h>
> > >    #include <asm/ctlreg.h>
> > >    #include <asm/chpid.h>
> > > @@ -26,6 +27,7 @@
> > >    #include <asm/sclp.h>
> > >    #include <asm/numa.h>
> > >    #include <asm/facility.h>
> > > +#include <asm/page-states.h>
> > >    #include "sclp.h"
> > > @@ -319,6 +321,7 @@ static bool contains_standby_increment(unsigned long start, unsigned long end)
> > >    static int sclp_mem_notifier(struct notifier_block *nb,
> > >    			     unsigned long action, void *data)
> > >    {
> > > +	struct memory_block *memory_block;
> > >    	unsigned long start, size;
> > >    	struct memory_notify *arg;
> > >    	unsigned char id;
> > > @@ -340,18 +343,29 @@ static int sclp_mem_notifier(struct notifier_block *nb,
> > >    		if (contains_standby_increment(start, start + size))
> > >    			rc = -EPERM;
> > >    		break;
> > > -	case MEM_GOING_ONLINE:
> > > +	case MEM_PREPARE_ONLINE:
> > > +		memory_block = find_memory_block(pfn_to_section_nr(arg->start_pfn));
> > > +		if (!memory_block) {
> > > +			rc = -EINVAL;
> > > +			goto out;
> > > +		}
> > >    		rc = sclp_mem_change_state(start, size, 1);
> > > +		if (rc || !memory_block->altmap)
> > > +			goto out;
> > > +		/*
> > > +		 * Set CMMA state to nodat here, since the struct page memory
> > > +		 * at the beginning of the memory block will not go through the
> > > +		 * buddy allocator later.
> > > +		 */
> > > +		__arch_set_page_nodat((void *)__va(start), memory_block->altmap->free);
> >
> > Looking up the memory block and grabbing the altmap from there is a bit
> > unfortunate.
> >
> > Why can't we do that when adding the altmap? Will the hypervisor scream at
> > us?
> calling __arch_set_page_nodat() before making memory block accessible
> will lead to crash. Hence, we think this is the only safe location to
> place it.
>
> > ... would we want to communicate any altmap start+size via the memory
> > notifier instead?
>
> Passing start, size  of memory range via memory notifier looks correct
> approach to me, as we try to make the specified range accessible.
>
> If we want to pass altmap size (nr_vmemmap_pages), then we might need a
> new field in struct memory_notify, which would prevent access of
> memory_block->altmap->free in the notifier.
>
> Do you want to take this approach instead?
>
> If yes, Then I could add a new field nr_vmemmap_pages in struct
> memory_notify and place it in PATCH : introduce
> MEM_PREPARE_ONLINE/MEM_FINISH_OFFLINE notifiers.

Yes, see my other mail. That's probably cleanest!

--
Cheers,

David / dhildenb