[lttng-dev] [RFC lttng-modules v4] Add kmalloc failover to vmalloc

[mjeanson@efficios.com (Michael Jeanson)]

On 2017-09-25 11:19, Mathieu Desnoyers wrote:
>> +/**
>> + * lttng_kvmalloc_node - attempt to allocate physically contiguous memory, but
>> upon
>> + * failure, fall back to non-contiguous (vmalloc) allocation.
>> + * @size: size of the request.
>> + * @flags: gfp mask for the allocation - must be compatible with GFP_KERNEL.
>> + *
>> + * Uses kmalloc to get the memory but if the allocation fails then falls back
>> + * to the vmalloc allocator. Use lttng_kvfree to free the memory.
>> + *
>> + * Reclaim modifiers - __GFP_NORETRY, __GFP_REPEAT and __GFP_NOFAIL are not
>> supported
>> + */
>> +static inline
>> +void *lttng_kvmalloc_node(unsigned long size, gfp_t flags, int node)
>> +{
>> +	void *ret;
>> +
>> +	/*
>> +	 * vmalloc uses GFP_KERNEL for some internal allocations (e.g page tables)
>> +	 * so the given set of flags has to be compatible.
>> +	 */
>> +	WARN_ON_ONCE((flags & GFP_KERNEL) != GFP_KERNEL);
>> +
>> +	/*
>> +	 * If the allocation fits in a single page, do not fallback.
>> +	 */
>> +	if (size <= PAGE_SIZE) {
>> +		return kmalloc_node(size, flags, node);
>> +	}
>> +
>> +	/*
>> +	 * Make sure that larger requests are not too disruptive - no OOM
>> +	 * killer and no allocation failure warnings as we have a fallback
>> +	 */
>> +	ret = kmalloc_node(size, flags | __GFP_NOWARN | __GFP_NORETRY, node);
>> +	if (!ret) {
>> +		if (node == NUMA_NO_NODE) {
>> +			/*
>> +			 * If no node was specified, use __vmalloc which is
>> +			 * always exported.
>> +			 */
>> +			ret = __vmalloc(size, flags | __GFP_HIGHMEM, PAGE_KERNEL);
>> +		} else {
>> +			/*
>> +			 * Otherwise, we need to select a node but __vmalloc_node
>> +			 * is not exported, use this fallback wrapper which uses
>> +			 * kallsyms if available or falls back to kmalloc_node.
>> +			 */
>> +			ret = __lttng_vmalloc_node_fallback(size, 1,
>> +					flags | __GFP_HIGHMEM, PAGE_KERNEL, node,
>> +					__builtin_return_address(0));
> 
> I try to never use __builtin_return_address(0) directly. It's buggy on powerpc32,
> and causes stack corruption.
> 
> The kernel exposes _RET_IP_ nowadays. Can we use it instead ?

That part was taken from the upstream mm code, I don't have a strong
opinion on that. _RET_IP_ points to the same function, I'd have to check
when it was introduced.

> 
> And I'm tempted to do a trick similar to lttng-ust there, e.g.:
> 
> /*
>  * Use of __builtin_return_address(0) sometimes seems to cause stack
>  * corruption on 32-bit PowerPC. Disable this feature on that
>  * architecture for now by always using the NULL value for the ip
>  * context.
>  */
> #if defined(__PPC__) && !defined(__PPC64__)
> #define LTTNG_UST_CALLER_IP()           NULL
> #else /* #if defined(__PPC__) && !defined(__PPC64__) */
> #define LTTNG_UST_CALLER_IP()           __builtin_return_address(0)
> #endif /* #else #if defined(__PPC__) && !defined(__PPC64__) */

I don't know what the side effects of setting the caller to NULL would
be here, you're the kernel developer I'll trust you on that one.

So whichever solution you prefer.

> 
> Thanks,
> 
> Mathieu
> 
> 
>> +		}
>> +
>> +		/*
>> +		 * Make sure we don't trigger recursive page faults in the
>> +		 * tracing fast path.
>> +		 */
>> +		wrapper_vmalloc_sync_all();
>> +	}
>> +	return ret;
>> +}