On 23/08/2021 13:13, Leo Yan wrote:

On Mon, Aug 23, 2021 at 11:57:52AM +0100, James Clark wrote:

[...]

quoted

Ok thanks for the explanation, that makes sense now. I do have one other
point about the documentation for the function:

Welcome!

quoted

quoted

+ * When update the AUX tail and detects any carrying in the high 32 bits, it
+ * means there have two store operations in user space and it cannot promise
+ * the atomicity for 64-bit write, so return '-1' in this case to tell the
+ * caller an overflow error has happened.
+ */

I couldn't see how it can ever return -1, it seems like it would loop forever
until it reads the correct value.

I use this chunk comment to address the function
compat_auxtrace_mmap__write_tail():

+int __weak compat_auxtrace_mmap__write_tail(struct auxtrace_mmap *mm, u64 tail)
+{
+	struct perf_event_mmap_page *pc = mm->userpg;
+	u64 mask = (u64)(UINT32_MAX) << 32;
+
+	if (tail & mask)
+		return -1;
+
+	/* Ensure all reads are done before we write the tail out */
+	smp_mb();
+	WRITE_ONCE(pc->aux_tail, tail);
+	return 0;
+}

Please let me know if this is okay or not?  Otherwise, if you think
the format can cause confusion, I'd like to split the comments into
two sections, one section for reading AUX head and another is for
writing AUX tail.

I see what you mean now, I think keeping it in one section is fine, I would just
replace "When update the AUX tail and detects" with "When
compat_auxtrace_mmap__write_tail() detects". If the function name is there then
it's clear that it's not the return value of compat_auxtrace_mmap__read_head()

Thanks
James

Thanks,
Leo