Re: [PATCH] tracing/gpu: Add gpu_mem_imported tracepoint

[Ørjan Eide]

On Mon, Jul 26, 2021 at 04:41:31PM +0000, Kalesh Singh wrote:
> The existing gpu_mem_total tracepoint allows GPU drivers a unifrom way
> to report the per-process and system-wide GPU memory usage. This
> tracepoint reports a single total of the GPU private allocations and the
> imported memory. [1]
> 
> To allow distinguishing GPU private vs imported memory, add
> gpu_mem_imported tracepoint.
>
> GPU drivers can use this tracepoint to report the per-process and global
> GPU-imported memory in a uniform way.

Right, as imported dma-buf memory is usually shared between two or more
processes it will be hard to reconcile system memory usage with process
private GPU memory and imported dma-buf memory in a single total sum. It
will be good to improve this and having a separate imported GPU memory
size is good.

> For backward compatility with already existing implementations of
> gpu_mem_total by various Android GPU drivers, this is proposed as a new
> tracepoint rather than additional args to gpu_mem_total.

Is this for compatibility with kernel GPU drivers only, or are there
user space users of the existing tracepoint that can't cope with it
being extended?

The eBPF used by Android to track the GPU memory is the only established
user of the tracepoint that I know about. Can existing versions of the
eBPF can handle this OK?

If the only worry is GPU kernel drivers then I think that extending the
existing tracepoint, to add a new field with the imported memory sum,
will be better. There doesn't appear to be any in-kernel GPU drivers
implementing this yet, and other GPU kernel drivers can just be extended
to use the new extended tracepoint. As long as there's some mechanism to
detect the extended variant of the tracepoint for backports, if the new
tracepoint is ever backported to older kernels, that shouldn't be a
problem.

-- 
Ørjan

> [1] https://lore.kernel.org/r/20200302234840.57188-1-zzyiwei@xxxxxxxxxx/
> 
> Signed-off-by: Kalesh Singh <kaleshsingh@xxxxxxxxxx>
> ---
>  include/trace/events/gpu_mem.h | 51 ++++++++++++++++++++++++----------
>  1 file changed, 36 insertions(+), 15 deletions(-)
> 
> diff --git a/include/trace/events/gpu_mem.h b/include/trace/events/gpu_mem.h
> index 26d871f96e94..b9543abf1461 100644
> --- a/include/trace/events/gpu_mem.h
> +++ b/include/trace/events/gpu_mem.h
> @@ -13,21 +13,7 @@
>  
>  #include <linux/tracepoint.h>
>  
> -/*
> - * The gpu_memory_total event indicates that there's an update to either the
> - * global or process total gpu memory counters.
> - *
> - * This event should be emitted whenever the kernel device driver allocates,
> - * frees, imports, unimports memory in the GPU addressable space.
> - *
> - * @gpu_id: This is the gpu id.
> - *
> - * @pid: Put 0 for global total, while positive pid for process total.
> - *
> - * @size: Size of the allocation in bytes.
> - *
> - */
> -TRACE_EVENT(gpu_mem_total,
> +DECLARE_EVENT_CLASS(gpu_mem_template,
>  
>  	TP_PROTO(uint32_t gpu_id, uint32_t pid, uint64_t size),
>  
> @@ -51,6 +37,41 @@ TRACE_EVENT(gpu_mem_total,
>  		__entry->size)
>  );
>  
> +/*
> + * The gpu_memory_total event indicates that there's an update to either the
> + * global or process total gpu memory counters.
> + *
> + * This event should be emitted whenever the kernel device driver allocates,
> + * frees, imports, unimports memory in the GPU addressable space.
> + *
> + * @gpu_id: This is the gpu id.
> + *
> + * @pid: Put 0 for global total, while positive pid for process total.
> + *
> + * @size: Size of the allocation in bytes.
> + *
> + */
> +DEFINE_EVENT(gpu_mem_template, gpu_mem_total,
> +	TP_PROTO(uint32_t gpu_id, uint32_t pid, uint64_t size),
> +	TP_ARGS(gpu_id, pid, size));
> +
> +/*
> + * The gpu_mem_imported event indicates that there's an update to the
> + * global or process imported gpu memory counters.
> + *
> + * This event should be emitted whenever the kernel device driver imports
> + * or unimports memory (allocated externally) in the GPU addressable space.
> + *
> + * @gpu_id: This is the gpu id.
> + *
> + * @pid: Put 0 for global total, while positive pid for process total.
> + *
> + * @size: Size of the imported memory in bytes.
> + */
> +DEFINE_EVENT(gpu_mem_template, gpu_mem_imported,
> +	TP_PROTO(uint32_t gpu_id, uint32_t pid, uint64_t size),
> +	TP_ARGS(gpu_id, pid, size));
> +
>  #endif /* _TRACE_GPU_MEM_H */
>  
>  /* This part must be outside protection */
> 
> base-commit: ff1176468d368232b684f75e82563369208bc371
> -- 
> 2.32.0.432.gabb21c7263-goog
>