Hi Greg, You're right, it got a little stale, sorry about that. It definitely did *not* get side-tracked, though. I have the next iteration ready, minus code cleaning which I haven't found time to do this past week. I will update the PR early next week and will probably ask for a volunteer to try it (plus the code review) to address the usability within Ceph. The gist of that last email I sent was that, following Sage's suggestions, I got rid of having to define tracepoints in some external text file before using them. Mohamad On 07/06/2018 01:46 PM, Gregory Farnum wrote: > This thread is a little stale, and it looks like the PR is as well. > Did anything more happen? I was pretty happy with the direction of > work, although I haven’t fully assimilated this last email. > -Greg > > On Tue, Jun 12, 2018 at 2:02 PM, Mohamad Gebai <mgebai@xxxxxxx> wrote: >> On 06/07/2018 01:24 PM, Mohamad Gebai wrote: >>> On 06/07/2018 12:52 PM, Sage Weil wrote: >>>> On Thu, 7 Jun 2018, Mohamad Gebai wrote: >>>>> [snip] >>>>> Any thought about the approach? How can I move this forward? >>>> This is a huge improvement over manually writing the .tp files! I have >>>> one thought, though. It's still necessary both to adjust the tracepoint >>>> location, e.g., >>>> >>>> >>>> - dout(30) << __func__ << " " << *(b_it->first) >>>> - << " expected4release=" << blob_expected_for_release >>>> - << " expected_allocations=" << bi.expected_allocations >>>> - << dendl; >>>> + trace_process_protrusive_extents_expected4release(*(b_it->first), >>>> + blob_expected_for_release, bi.expected_allocations); >>>> >>>> and also to include the line in tracing/tracetool/subsys, eg., >>>> >>>> + process_protrusive_extents_expected4release(Blob blob, int64_t blob_expected_for_release, int64_t expected_allocations) "%s expected4release=%llu expected_allocations=%llu" 30 >>>> >>>> Do you think it's possible to declare these inline and generate the >>>> tracing/tracetool/subsys files from the source? For example, write >>>> something like >>>> >>>> trace(30, process_protrusive_extents_expected4release, >>>> Blob *(b_it->first), >>>> int64_t blob_expected_for_release, >>>> int64_t bi.expected_allocations, >>>> "%s expected4release=%llu expected_allocations=%llu"); >>>> >>>> A macro could expand that out to the function call, and a preprocessor >>>> pass could slurp these up and generate the file for input to >>>> tracetool (or tracetool could extract them from the source >>>> directly). >>> Good point, I definitely see that as a possibility. I'll fiddle with it >>> and report back. >> Ok, I think I got to something that is feasible and hopefully less >> awkward. Consider the following syntax in the call site: >> >> trace_<event_name>(<loglevel>, <subsys>, <type1>, <name1>, <value1> [, >> <type2>, <name2>, <value2>, ...], <format_string>); >> >> When expanded, this macro takes everything except the values out, as such: >> >> __trace_<event_name>(<value1> [, <value2>, ...]); >> >> The example you gave goes from: >> >> dout(30) << __func__ << " " << *(b_it->first) >> << " expected4release=" << blob_expected_for_release >> << " expected_allocations=" << bi.expected_allocations >> << dendl; >> >> to: >> >> trace_process_protrusive_extents_expected4release(30, bluestore_gc, >> Blob, blob, *(b_it->first), >> int64_t, blob_expected_for_release, blob_expected_for_release, >> int64_t, expected_allocations, bi.expected_allocations, >> "%s expected4release=%llu expected_allocations=%llu"); >> >> which when expanded becomes: >> >> __trace_process_protrusive_extents_expected4release( >> *(b_it->first), >> blob_expected_for_release, >> bi.expected_allocations, >> ); >> >> We'll need not only to specify the type of each field, but also the name >> we want it to have. Otherwise, what would be the name of the first >> argument of this example (*(b_it->first))? The good thing now is that >> this syntax is self-declarative, and I'm generating valid code (and tp >> files) from it. The bad thing is that it's not straightforward. There is >> also the "parsing the entire code base" part.. >> >> Thoughts? Is that something we can consider? >> >> PS: I haven't updated the PR yet. >> >> Mohamad >> >> -- >> To unsubscribe from this list: send the line "unsubscribe ceph-devel" in >> the body of a message to majordomo@xxxxxxxxxxxxxxx >> More majordomo info at http://vger.kernel.org/majordomo-info.html -- To unsubscribe from this list: send the line "unsubscribe ceph-devel" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
