Re: [PATCH 02/17] prmem: write rare for static allocation

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

> +static __always_inline bool __is_wr_after_init(const void *ptr, size_t size)
> +{
> +	size_t start = (size_t)&__start_wr_after_init;
> +	size_t end = (size_t)&__end_wr_after_init;
> +	size_t low = (size_t)ptr;
> +	size_t high = (size_t)ptr + size;
> +
> +	return likely(start <= low && low < high && high <= end);
> +}

size_t is an odd type choice for doing address arithmetic.

> +/**
> + * wr_memset() - sets n bytes of the destination to the c value
> + * @dst: beginning of the memory to write to
> + * @c: byte to replicate
> + * @size: amount of bytes to copy
> + *
> + * Returns true on success, false otherwise.
> + */
> +static __always_inline
> +bool wr_memset(const void *dst, const int c, size_t n_bytes)
> +{
> +	size_t size;
> +	unsigned long flags;
> +	uintptr_t d = (uintptr_t)dst;
> +
> +	if (WARN(!__is_wr_after_init(dst, n_bytes), WR_ERR_RANGE_MSG))
> +		return false;
> +	while (n_bytes) {
> +		struct page *page;
> +		uintptr_t base;
> +		uintptr_t offset;
> +		uintptr_t offset_complement;

Again, these are really odd choices for types.  vmap() returns a void*
pointer, on which you can do arithmetic.  Why bother keeping another
type to which you have to cast to and from?

BTW, our usual "pointer stored in an integer type" is 'unsigned long',
if a pointer needs to be manipulated.

> +		local_irq_save(flags);

Why are you doing the local_irq_save()?

> +		page = virt_to_page(d);
> +		offset = d & ~PAGE_MASK;
> +		offset_complement = PAGE_SIZE - offset;
> +		size = min(n_bytes, offset_complement);
> +		base = (uintptr_t)vmap(&page, 1, VM_MAP, PAGE_KERNEL);

Can you even call vmap() (which sleeps) with interrupts off?

> +		if (WARN(!base, WR_ERR_PAGE_MSG)) {
> +			local_irq_restore(flags);
> +			return false;
> +		}

You really need some kmap_atomic()-style accessors to wrap this stuff
for you.  This little pattern is repeated over and over.

...
> +const char WR_ERR_RANGE_MSG[] = "Write rare on invalid memory range.";
> +const char WR_ERR_PAGE_MSG[] = "Failed to remap write rare page.";

Doesn't the compiler de-duplicate duplicated strings for you?  Is there
any reason to declare these like this?