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

[Ingo Molnar]

* Oleg Nesterov <oleg@xxxxxxxxxx> wrote:

> 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".

Sorry - i mean lockups while _using_ workqueues. The workqueue code 
has been pretty OK - sans that thing with work_on_cpu() which was 
quite a pain with 4-5 regressions IIRC. So generally workqueue usage 
isnt particularly well developed - and having some basic 
instrumentation increases mindshare. IMHO.

> 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).

it's against the tracing tree - which has new/changed facilities 
which these patches are against. I think Frederic can send you an 
URI to a branch in his tree with these bits properly added in, for 
review. (a full patch in that case would be useful too i guess)

> 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.

thanks!

> 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().

If this is fixed (and no other problem surfaces), would you mind to 
ack these bits?

And if you can think of any way to make it even simpler / less 
intrusive, please let us know ...

	Ingo
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/