On Thu, 22 Mar 2018 12:31:12 -0700
Alexei Starovoitov [off-list ref] wrote:

On 3/22/18 11:11 AM, Steven Rostedt wrote:

quoted

On Thu, 22 Mar 2018 11:01:48 -0700
Alexei Starovoitov [off-list ref] wrote:
 

quoted

From: Alexei Starovoitov <ast@kernel.org>

Fix all tracepoint arguments to pass structures (large and small) by reference
instead of by value.
Avoiding passing large structs by value is a good coding style.
Passing small structs sometimes is beneficial, but in all cases
it makes no difference vs readability of the code.
The subsequent patch enforces that all tracepoints args are either integers
or pointers and fit into 64-bit.  

But some of these structures are used to force type checking, and are
just the same size as a number. That's why they don't have "struct" in
front of them. Like pmd_t. Will the subsequent patches really break if
the structure itself has one element that is of size long? Just seems
to add extra code to pass in an address to something that fits into a
single register.  

yeah. C doesn't allow casting of 'struct s { u64 var };' into u64
without massive hacks and aliasing warnings by compiler.
CAST_TO_U64 macro in patch 7 will prevent tracepoint arguments to be
things like pmd_t. It's not perfect, but doing & of pmd_t variable
is imo clean enough as you can see in this patch.
The macro can be tweaked to do the cast like
*(sizeof(typeof(arg))*)&arg,
but there is no way to get rid of compiler warning.

OK, but instead of changing it to pass by reference, why not just pass
the value. That is:

 static void xen_set_pte_atomic(pte_t *ptep, pte_t pte)
 {
-	trace_xen_mmu_set_pte_atomic(ptep, pte);
+	trace_xen_mmu_set_pte_atomic(ptep, native_pte_val(pte));
 	set_64bit((u64 *)ptep, native_pte_val(pte));
 }

It shouldn't add any extra code, as those helper functions are
basically just special casts.

-- Steve