Hi Catalin
Thanks for your opinions.
在 2020/9/22 17:58, Catalin Marinas 写道:
> On Mon, Sep 21, 2020 at 02:00:05AM +0000, Chen Jun wrote:
>> From: Wei Yongjun <weiyongjun1@xxxxxxxxxx>
>>
>> Currently the reporting of the percpu chunks leaking problem
>> are not supported. This patch introduces this function.
>>
>> Since __percpu pointer is not pointing directly to the actual chunks,
>> this patch creates an object for __percpu pointer, but marks it as no
>> scan block, only check whether this pointer is referenced by other
>> blocks.
>
> OK, so you wanted NO_SCAN to not touch the block at all, not even update
> the checksum. Maybe better add a new flag, NO_ACCESS (and we could use
> it to track ioremap leaks, it's been on my wishlist for years).
>
I will add a new OBJECT_NO_ACCESS.
The checksum of the object will not be updated and its memory block will
not be scanned if the object marked with OBJECT_NO_ACCESS.
>> diff --git a/mm/kmemleak.c b/mm/kmemleak.c
>> index c09c6b59eda6..feedb72f06f2 100644
>> --- a/mm/kmemleak.c
>> +++ b/mm/kmemleak.c
>> @@ -283,6 +288,9 @@ static void hex_dump_object(struct seq_file *seq,
>> const u8 *ptr = (const u8 *)object->pointer;
>> size_t len;
>>
>> + if (object->flags & OBJECT_PERCPU)
>> + ptr = this_cpu_ptr((void __percpu *)object->pointer);
>
> You may want to print the CPU number as well since the information is
> likely different on another CPU. Also, I think this context is
> preemptable, so it's better with a get_cpu/put_cpu().
>
I will print cpu number when dump the percpu object.
>> @@ -651,6 +672,19 @@ static void create_object(unsigned long ptr, size_t size, int min_count,
>> raw_spin_unlock_irqrestore(&kmemleak_lock, flags);
>> }
>>
>> +static void create_object(unsigned long ptr, size_t size, int min_count,
>> + gfp_t gfp)
>> +{
>> + __create_object(ptr, size, min_count, 0, gfp);
>> +}
>> +
>> +static void create_object_percpu(unsigned long ptr, size_t size, int min_count,
>> + gfp_t gfp)
>> +{
>> + __create_object(ptr, size, min_count, OBJECT_PERCPU | OBJECT_NO_SCAN,
>> + gfp);
>> +}
>> +
>> /*
>> * Mark the object as not allocated and schedule RCU freeing via put_object().
>> */
>> @@ -912,10 +946,12 @@ void __ref kmemleak_alloc_percpu(const void __percpu *ptr, size_t size,
>> * Percpu allocations are only scanned and not reported as leaks
>> * (min_count is set to 0).
>> */
>> - if (kmemleak_enabled && ptr && !IS_ERR(ptr))
>> + if (kmemleak_enabled && ptr && !IS_ERR(ptr)) {
>> for_each_possible_cpu(cpu)
>> create_object((unsigned long)per_cpu_ptr(ptr, cpu),
>> size, 0, gfp);
>> + create_object_percpu((unsigned long)ptr, size, 1, gfp);
>> + }
>> }
>
> A concern I have here is that ptr may overlap with an existing object
> and the insertion in the rb tree will fail. For example, with !SMP,
> ptr == per_cpu_ptr(ptr, 0), so create_object() will fail and kmemleak
> gets disabled.
>
> An option would to figure out how to allow overlapping ranges with rb
> tree (or find a replacement for it if not possible).
>
> Another option would be to have an additional structure to track the
> __percpu pointers since they have their own range. If size is not
> relevant, maybe go for an xarray, otherwise another rb tree (do we have
> any instance of pointers referring some inner member of a __percpu
> object?). The scan_object() function will have to search two trees.
>
I would like to use CONFIG_SMP to seprate code:
if SMP, we will create some objects for per_cpu_ptr(ptr, cpu) and an
object with OBJECT_NO_ACCESS for ptr.
if !SMP, we will not create object for per_cpu_ptr(ptr,cpu), but an
object without OBJECT_NO_ACCESS for ptr will be created.
What do you think about this opinion.
Waiting for your reply
Best wishes
Jun
