Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
Subject: Re: [PATCH 4/9] platform/surface: aggregator: Add trace points
To:     Steven Rostedt <rostedt@goodmis.org>
Cc:     linux-kernel@vger.kernel.org, Hans de Goede <hdegoede@redhat.com>,
        Mark Gross <mgross@linux.intel.com>,
        Arnd Bergmann <arnd@arndb.de>,
        Greg Kroah-Hartman <gregkh@linuxfoundation.org>,
        Ingo Molnar <mingo@redhat.com>,
        =?UTF-8?Q?Bla=c5=be_Hrastnik?= <blaz@mxxn.io>,
        Dorian Stoll <dorian.stoll@tmsp.io>,
        platform-driver-x86@vger.kernel.org
References: <20201115192143.21571-1-luzmaximilian@gmail.com>
 <20201115192143.21571-5-luzmaximilian@gmail.com>
 <20201117114411.30af7078@gandalf.local.home>
From:   Maximilian Luz <luzmaximilian@gmail.com>
Message-ID: <cff66ef4-9a26-69df-ee03-b6b36ab6943b@gmail.com>
Date:   Tue, 17 Nov 2020 18:42:20 +0100
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:68.0) Gecko/20100101
 Thunderbird/68.12.0
MIME-Version: 1.0
In-Reply-To: <20201117114411.30af7078@gandalf.local.home>
Content-Type: text/plain; charset=utf-8; format=flowed
Content-Language: en-US
Content-Transfer-Encoding: 7bit
Precedence: bulk

On 11/17/20 5:44 PM, Steven Rostedt wrote:
> On Sun, 15 Nov 2020 20:21:38 +0100
> Maximilian Luz <luzmaximilian@gmail.com> wrote:

[...]

>>   	/*
>>   	 * Lock packet and commit with memory barrier. If this packet has
>>   	 * already been locked, it's going to be removed and completed by
>> @@ -1154,6 +1167,8 @@ static void ssh_ptl_timeout_reap(struct work_struct *work)
>>   	ktime_t next = KTIME_MAX;
>>   	bool resub = false;
>>   
>> +	trace_ssam_ptl_timeout_reap("pending", atomic_read(&ptl->pending.count));
> 
> I noticed that the only two places that use timeout_reap, both have
> "pending" as a string. Is this necessary to save? Would an enum work?

I think its probably cleaner to declare an event class for those.
Currently they are using the GENERIC_UINT_EVENT class, which I intended
as mapping with string to uint, but I'm only using that in three places,
two of which are for the timeout. So dropping the GENERIC_UINT_EVENT
class is probably the best solution(?).

[...]

>> +/**
>> + * ssam_trace_ptr_uid() - Convert the pointer to a non-pointer UID string.
>> + * @ptr: The pointer to convert.
>> + * @uid_str: A buffer of length SSAM_PTR_UID_LEN where the UID will be stored.
>> + *
>> + * Converts the given pointer into a UID string that is safe to be shared
>> + * with userspace and logs, i.e. doesn't give away the real memory location.
>> + */
>> +static inline void ssam_trace_ptr_uid(const void *ptr, char *uid_str)
>> +{
>> +	char buf[2 * sizeof(void *) + 1];
>> +
>> +	snprintf(buf, ARRAY_SIZE(buf), "%p", ptr);
>> +	memcpy(uid_str, &buf[ARRAY_SIZE(buf) - SSAM_PTR_UID_LEN],
>> +	       SSAM_PTR_UID_LEN);
> 
> Is the above snprintf() to have the ptr turn into a hash?
> 
> Otherwise, couldn't you just truncate the value of ptr?

Yes, the idea is to generate the same output for a pointer as a normal
printk message would generate, i.e. generate a (truncated) hash of the
pointer. While the trace points should be enough on their own, I prefer
to have the output lining up with the kernel log, so that we can match
packets across both if ever necessary.

> In any case, you want to make sure that ARRAY_SIZE(buf) is always bigger
> than, or equal to, SSAM_PTR_UID_LEN, and you should probably stick a:
> 
> 	BUILD_BUG_ON(ARRAY_SIZE(buf) < SSAM_PTR_UID_LEN);
> 
> in there, which would cause the build to fail if that was ever the case.

Thanks! That is a good idea.

[...]

>> +#define ssam_trace_get_command_field_u8(packet, field) \
>> +	((!packet || packet->data.len < SSH_COMMAND_MESSAGE_LENGTH(0)) \
>> +	 ? 0 : p->data.ptr[SSH_MSGOFFSET_COMMAND(field)])
> 
> I think you want the above to be:
> 
> 	? 0 : packet->data.ptr[SSH_MSGOFFSET_COMMAND(field)])
> 
> The only reason it worked, is because the caller passed in "p".

Oh, right!

>> +DECLARE_EVENT_CLASS(ssam_packet_class,
>> +	TP_PROTO(const struct ssh_packet *packet),
>> +
>> +	TP_ARGS(packet),
>> +
>> +	TP_STRUCT__entry(
>> +		__field(unsigned long, state)
>> +		__array(char, uid, SSAM_PTR_UID_LEN)
>> +		__field(u8, priority)
>> +		__field(u16, length)
>> +		__field(u16, seq)
>> +	),
>> +
>> +	TP_fast_assign(
>> +		__entry->state = READ_ONCE(packet->state);
>> +		ssam_trace_ptr_uid(packet, __entry->uid);
>> +		__entry->priority = READ_ONCE(packet->priority);
> 
> I'm curious about the need for the read once here.

I generally prefer to be explicit when accessing values that can be
changed concurrently by other parties. In the very least, this should
guarantee that the value will be read as a whole in one instruction and
the compiler does not do anything unexpected (like reading it in
multiple instructions, which could lead to invalid state or priority
values).

Arguably, the latter will very likely never happen even without the
READ_ONCE, but I prefer to be on the safe side.

> The rest seems fine.

Thanks,
Max