Re: [PATCH/RFC] tracing: make trace_marker{,_raw} stream-like

[John Keeping <john@metanate.com>]

Hi Steve,

On Thu, Sep 09, 2021 at 08:35:29AM -0400, Steven Rostedt wrote:
> On Thu,  9 Sep 2021 12:57:34 +0100
> John Keeping <john@metanate.com> wrote:
> 
> > The tracing marker files are write-only streams with no meaningful
> > concept of file position.  Using stream_open() to mark them as
> > stream-link indicates this and has the added advantage that a single
> > file descriptor can now be used from multiple threads without contention
> > thanks to clearing FMODE_ATOMIC_POS.
> > 
> > Note that this has the potential to break existing userspace by since
> > both lseek(2) and pwrite(2) will now return ESPIPE when previously lseek
> > would have updated the stored offset and pwrite would have appended to
> > the trace.  A survey of libtracefs and several other projects found to
> > use trace_marker(_raw) [1][2][3] suggests that everyone limits
> > themselves to calling write(2) and close(2) on these file descriptors so
> > there is a good chance this will go unnoticed and the benefits of
> > reduced overhead and lock contention seem worth the risk.
> > 
> > [1] https://github.com/google/perfetto
> > [2] https://github.com/intel/media-driver/
> > [3] https://w1.fi/cgit/hostap/
> > 
> > Signed-off-by: John Keeping <john@metanate.com>
> > ---
> > I'm marking this as RFC because it breaks the Prime Directive of Linux
> > development, as explained above.  But I'm hoping this is one of the
> 
> The "Prime Directive of Linux development" is the tree falling in the
> forest approach. If you break user space API but there's no user space
> application around to notice the break, did you really break it? The answer
> is "No".
> 
> > cases where we get away with it because this is a huge improvement for
> > multi-threaded programs when doing the simple thing of opening a single
> > trace_marker FD at startup and just writing to it from any thread.
> 
> I like the idea too.
> 
> > 
> > After writing this, I realised that an alternative solution to my
> > original problem would have been to use pwrite instead of write!  But I
> > still think fixing this at the source would be better.
> 
> I'm fine with adding this. But I'm going to add it after the merge window
> for the next release (5.16).
> 
> If someone complains that it broke their application, we may need to revert
> it, but I doubt that will happen.

Were you expecting more input from me on this?  The above sounded like
"will be added for 5.16" but I don't see this change in v5.16-rc4 and
the patch is still marked as "New" in patchwork [1]

[1] https://patchwork.kernel.org/project/linux-trace-devel/patch/20210909115734.3818711-1-john@metanate.com/


Thanks,
John