----- On Sep 26, 2017, at 10:32 AM, Michael Jeanson [email protected] 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
_______________________________________________
lttng-dev mailing list
[email protected]
https://lists.lttng.org/cgi-bin/mailman/listinfo/lttng-dev
