Re: [v3 2/2] device-dax: "Hotremove" persistent memory that is used like normal RAM

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

Hi Pavel,

Thanks for doing this!  I knew we'd have to get to it eventually, but
sounds like you needed it sooner rather than later.

...
>  static inline struct dev_dax *to_dev_dax(struct device *dev)
> diff --git a/drivers/dax/kmem.c b/drivers/dax/kmem.c
> index 4c0131857133..6f1640462df9 100644
> --- a/drivers/dax/kmem.c
> +++ b/drivers/dax/kmem.c
> @@ -71,21 +71,107 @@ int dev_dax_kmem_probe(struct device *dev)
>  		kfree(new_res);
>  		return rc;
>  	}
> +	dev_dax->dax_kmem_res = new_res;
>  
>  	return 0;
>  }
>  
> +#ifdef CONFIG_MEMORY_HOTREMOVE

Instead of this #ifdef, is there any downside to doing

	if (!IS_ENABLED(CONFIG_MEMORY_HOTREMOVE)) {
		/*
		 * Without hotremove, purposely leak ...
		 */
		return 0;
	}
		

> +/*
> + * Check that device-dax's memory_blocks are offline. If a memory_block is not
> + * offline a warning is printed and an error is returned. dax hotremove can
> + * succeed only when every memory_block is offlined beforehand.
> + */

I'd much rather see comments inline with the code than all piled at the
top of a function like this.

One thing that would be helpful, though, is a reminder about needing the
device hotplug lock.

> +static int
> +check_memblock_offlined_cb(struct memory_block *mem, void *arg)
> +{
> +	struct device *mem_dev = &mem->dev;
> +	bool is_offline;
> +
> +	device_lock(mem_dev);
> +	is_offline = mem_dev->offline;
> +	device_unlock(mem_dev);
> +
> +	if (!is_offline) {
> +		struct device *dev = (struct device *)arg;

The two devices confused me for a bit here.  Seems worth a comment to
remind the reader what this device _is_ versus 'mem_dev'.

> +		unsigned long spfn = section_nr_to_pfn(mem->start_section_nr);
> +		unsigned long epfn = section_nr_to_pfn(mem->end_section_nr);
> +		phys_addr_t spa = spfn << PAGE_SHIFT;
> +		phys_addr_t epa = epfn << PAGE_SHIFT;
> +
> +		dev_warn(dev, "memory block [%pa-%pa] is not offline\n",
> +			 &spa, &epa);

I thought we had a magic resource printk %something.  Could we just
print one of the device resources here to save all the section/pfn/paddr
calculations?

Also, should we consider a slightly scarier message?  This path has a
permanent, user-visible effect (we can never try to unbind again).

> +		return -EBUSY;
> +	}
> +
> +	return 0;
> +}

Even though they're static, I'd prefer that we not create two versions
of check_memblock_offlined_cb() in the kernel.  Can we give this a
better, non-conflicting name?

> +static int dev_dax_kmem_remove(struct device *dev)
> +{
> +	struct dev_dax *dev_dax = to_dev_dax(dev);
> +	struct resource *res = dev_dax->dax_kmem_res;
> +	resource_size_t kmem_start;
> +	resource_size_t kmem_size;
> +	unsigned long start_pfn;
> +	unsigned long end_pfn;
> +	int rc;
> +
> +	/*
> +	 * dax kmem resource does not exist, means memory was never hotplugged.
> +	 * So, nothing to do here.
> +	 */
> +	if (!res)
> +		return 0;

How could that happen?  I can't think of any obvious scenarios.

> +	kmem_start = res->start;
> +	kmem_size = resource_size(res);
> +	start_pfn = kmem_start >> PAGE_SHIFT;
> +	end_pfn = start_pfn + (kmem_size >> PAGE_SHIFT) - 1;
> +
> +	/*
> +	 * Walk and check that every singe memory_block of dax region is
> +	 * offline
> +	 */
> +	lock_device_hotplug();
> +	rc = walk_memory_range(start_pfn, end_pfn, dev,
> +			       check_memblock_offlined_cb);

Does lock_device_hotplug() also lock memory online/offline?  Otherwise,
isn't this offline check racy?  If not, can you please spell that out in
a comment?

Also, could you compare this a bit to the walk_memory_range() use in
__remove_memory()?  Why do we need two walks looking for offline blocks?