On 23/08/2021 13:13, Leo Yan wrote:
On Mon, Aug 23, 2021 at 11:57:52AM +0100, James Clark wrote:
[...]
Ok thanks for the explanation, that makes sense now. I do have one other point about the documentation for the function:
Welcome!
- 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