From mboxrd@z Thu Jan  1 00:00:00 1970
From: mathieu.desnoyers@efficios.com (Mathieu Desnoyers)
Date: Tue, 26 Sep 2017 14:57:35 +0000 (UTC)
Subject: [lttng-dev] [RFC lttng-modules v4] Add kmalloc failover to
	vmalloc
In-Reply-To: <2e9e834c-2694-f7db-c6f6-ae048dee22fe@efficios.com>
References: <1506351380-4988-1-git-send-email-mjeanson@efficios.com>
 <1217070996.17685.1506352743808.JavaMail.zimbra@efficios.com>
 <2e9e834c-2694-f7db-c6f6-ae048dee22fe@efficios.com>
Message-ID: <2105422460.18557.1506437855430.JavaMail.zimbra@efficios.com>

----- On Sep 26, 2017, at 10:32 AM, Michael Jeanson mjeanson at efficios.com wrote:

> 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.

If upstream mm code does that, then I think we're at least as safe as
the kernel itself. It's possible that given that the kernel always
builds with O2, the problem never shows up.
> 
>> 
>> 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.

I'll take your patch as is.

Thanks,

Mathieu

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

-- 
Mathieu Desnoyers
EfficiOS Inc.
http://www.efficios.com