Messages in this thread | | | Subject | Re: [PATCH v1 2/3] perf auxtrace: Add compat_auxtrace_mmap__{read_head|write_tail} | From | James Clark <> | Date | Mon, 23 Aug 2021 17:00:14 +0100 |
| |
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 >
| |