Re: [PATCH 0/4] workqueue_tracepoint: Add worklet tracepoints for worklet lifecycle tracing

[Oleg Nesterov <oleg@redhat.com>]

On 04/26, Andrew Morton wrote:
>
> Most workqueue work lately has come from Oleg.  I'm unaware that he has
> expressed an interest in this feature?  Oleg, would it have been useful
> in any of the work you've done?

Well. Probably not. But I don't think this matters. Other people (and
not only kernel developers) can find this useful.

I _think_ that if you are going to hack workqueue.c itself, it is
more easy to just add some printks, may be I am wrong. But, probably
tracepoints can help to write/debug, say, device drivers. Or admins
can use debugfs to see whats going on, or to provide more info for
the bugreports.

I try to avoid "do we need this feauture" discussions as much as
possible. Because I never know. My usage of kernel is very, very
limited, I never do something "interesting" on my machine. This reminds
me the discussion about the ability to trace /sbin/init. Some developers
were unhappy with the trivial patch I sent. They said it is trivial
to change your kernel if you need this. But no, it was not trivial
to me when I was admin. So, I just can't judge.

> > And the thing is, the workqueue code has been pretty problematic
> > lately - with lockups and other regressions.

Hmm. Perhaps I missed some bug-reports... But I don't remember any
recent problems with workueues except the "usual" bugs like "flush
shares the lock with work->func".


As for the patches, I can't review them now. They are on top of
some other changes which I didn't see (or perhaps I lost the patches
I was cc'ed? sorry in this case).

But at least the change in workqueue.c looks very simple, and do
not complicate the code for readers. And, if we add any tracing
to workqueues, then it is very natural to add entry/exit handlers.


I must admit, I don't really understand why trace_workqueue.c uses
cwq->thread as a "primary key". I have the feeling we can simplify
this code if we pass "struct workqueue_struct *" instead, but I am
not sure.

In particular, trace_workqueue_flush(cwq->thread) looks a bit strange
to me. I can't imagine how it can be useful per-thread and without
trace_workqueue_flush_end() or something. I mean, if we need to trace
flushes, then imho it makes much more sense to add flush_start/flush_end
handlers into flush_workqueue().

Oleg.