[PATCH 1/2 v2] tracing/events: provide string with undefined size support

[PATCH 1/2 v2] tracing/events: provide string with undefined size support

[Frederic Weisbecker]

Impact: less memory usage for tracing

This patch provides the support for dynamic size strings on
event tracing.

The key concept is to use a structure with an ending char array field of
undefined size and use such ability to allocate the minimal size on the ring
buffer to make the entry fit inside as opposite to a fixed length strings with
upper bound.

This patch provides one new macro:

-__ending_string(name, src)

This one declares the string to the structure inside TP_STRUCT__entry.
You need to provide the name of the string field and the source that will be
copied inside.
Two constraints: only one __ending_string() per TRACE_EVENT can be added and
it must be the last field to be declared. Hence the __ending prefix.

This macro will declare the necessary field and will also add the dynamic
size of the string needed for the ring buffer entry allocation.

It also support filtering because these strings behave essentially
like usual fixed length string.

Changes in v2:

Address the suggestion of Steven Rostedt: drop the opening_string() macro
and redefine __ending_string() to get the size of the string to be copied
instead of overwritting the whole ring buffer allocation.

Signed-off-by: Frederic Weisbecker <fweisbec@gmail.com>
---
 include/trace/ftrace.h |   42 +++++++++++++++++++++++++++++++++++++++---
 1 files changed, 39 insertions(+), 3 deletions(-)

diff --git a/include/trace/ftrace.h b/include/trace/ftrace.h
index 6fb06bd..7d29cf5 100644
--- a/include/trace/ftrace.h
+++ b/include/trace/ftrace.h
@@ -27,6 +27,9 @@
 #undef __field
 #define __field(type, item)		type	item;
 
+#undef __ending_string
+#define __ending_string(item, src)	char	item[];
+
 #undef TP_STRUCT__entry
 #define TP_STRUCT__entry(args...) args
 
@@ -150,6 +153,14 @@ ftrace_raw_output_##call(struct trace_iterator *iter, int flags)	\
 	if (!ret)							\
 		return 0;
 
+#undef __ending_string
+#define __ending_string(item, src)					      \
+	ret = trace_seq_printf(s, "\tfield: char " #item "[];\t"	      \
+			       "offset:%u;\n",				      \
+			       (unsigned int)offsetof(typeof(field), item));  \
+	if (!ret)							      \
+		return 0;
+
 #undef __entry
 #define __entry REC
 
@@ -193,6 +204,18 @@ ftrace_format_##call(struct trace_seq *s)				\
 	if (ret)							\
 		return ret;
 
+/*
+ * We choose a size of MAX_FILTER_STR_VAL, then we behave like
+ * a usual string with the maximum size to keep being filterable.
+ */
+#undef __ending_string
+#define __ending_string(item, src)					\
+	ret = trace_define_field(event_call, "char []", #item,		\
+				 offsetof(typeof(field), item),		\
+				 MAX_FILTER_STR_VAL);			\
+	if (ret)							\
+		return ret;
+
 #undef TRACE_EVENT
 #define TRACE_EVENT(call, proto, args, tstruct, func, print)		\
 int									\
@@ -413,6 +436,15 @@ __attribute__((section("_ftrace_events"))) event_##call = {		\
 #undef __entry
 #define __entry entry
 
+#undef __field
+#define __field(type, item)
+
+#undef __array
+#define __array(type, item, len)
+
+#undef __ending_string
+#define __ending_string(item, src)	__str_size = strlen(src) + 1
+
 #undef TRACE_EVENT
 #define TRACE_EVENT(call, proto, args, tstruct, assign, print)		\
 _TRACE_PROFILE(call, PARAMS(proto), PARAMS(args))			\
@@ -425,14 +457,18 @@ static void ftrace_raw_event_##call(proto)				\
 	struct ring_buffer_event *event;				\
 	struct ftrace_raw_##call *entry;				\
 	unsigned long irq_flags;					\
+	int __str_size = 0;						\
 	int pc;								\
 									\
 	local_save_flags(irq_flags);					\
 	pc = preempt_count();						\
 									\
-	event = trace_current_buffer_lock_reserve(event_##call.id,	\
-				  sizeof(struct ftrace_raw_##call),	\
-				  irq_flags, pc);			\
+	tstruct;							\
+									\
+	event = trace_current_buffer_lock_reserve(			\
+			event_##call.id,				\
+			sizeof(struct ftrace_raw_##call) + __str_size,	\
+			irq_flags, pc);					\
 	if (!event)							\
 		return;							\
 	entry	= ring_buffer_event_data(event);			\
-- 
1.6.1

[PATCH 2/2 v2] tracing/lock: provide lock_acquired event support for dynamic size string

[Frederic Weisbecker]

Now that we can support the dynamic sized string, make the lock tracing
able to use it, making it safe against modules removal and consuming
the right amount of memory needed for each lock name

Changes in v2:
adapt to the __ending_string() updates and the opening_string() removal.

Signed-off-by: Frederic Weisbecker <fweisbec@gmail.com>
---
 include/trace/events/lockdep.h |    4 ++--
 1 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/include/trace/events/lockdep.h b/include/trace/events/lockdep.h
index 45e326b..e649fbc 100644
--- a/include/trace/events/lockdep.h
+++ b/include/trace/events/lockdep.h
@@ -38,14 +38,14 @@ TRACE_EVENT(lock_acquired,
 	TP_ARGS(lock, ip, waittime),
 
 	TP_STRUCT__entry(
-		__field(const char *, name)
 		__field(unsigned long, wait_usec)
 		__field(unsigned long, wait_nsec_rem)
+		__ending_string(name, lock->name)
 	),
 	TP_fast_assign(
-		__entry->name = lock->name;
 		__entry->wait_nsec_rem = do_div(waittime, NSEC_PER_USEC);
 		__entry->wait_usec = (unsigned long) waittime;
+		strcpy(__entry->name, lock->name);
 	),
 	TP_printk("%s (%lu.%03lu us)", __entry->name, __entry->wait_usec,
 				       __entry->wait_nsec_rem)
-- 
1.6.1

Re: [PATCH 1/2 v2] tracing/events: provide string with undefined size support

[Peter Zijlstra]

On Thu, 2009-04-16 at 22:00 +0200, Frederic Weisbecker wrote:
> Impact: less memory usage for tracing
> 
> This patch provides the support for dynamic size strings on
> event tracing.
> 
> The key concept is to use a structure with an ending char array field of
> undefined size and use such ability to allocate the minimal size on the ring
> buffer to make the entry fit inside as opposite to a fixed length strings with
> upper bound.
> 
> This patch provides one new macro:
> 
> -__ending_string(name, src)
> 
> This one declares the string to the structure inside TP_STRUCT__entry.
> You need to provide the name of the string field and the source that will be
> copied inside.
> Two constraints: only one __ending_string() per TRACE_EVENT can be added and
> it must be the last field to be declared. Hence the __ending prefix.
> 
> This macro will declare the necessary field and will also add the dynamic
> size of the string needed for the ring buffer entry allocation.
> 
> It also support filtering because these strings behave essentially
> like usual fixed length string.

can't we simply do __string(name, src) and output something like:

 struct {
   u16 size;
   char str[0];
 } name;

That would get rid of this __ending_ wart.

Re: [PATCH 1/2 v2] tracing/events: provide string with undefined size support

[Frédéric Weisbecker]

2009/4/16 Peter Zijlstra <a.p.zijlstra@chello.nl>:
> On Thu, 2009-04-16 at 22:00 +0200, Frederic Weisbecker wrote:
>> Impact: less memory usage for tracing
>>
>> This patch provides the support for dynamic size strings on
>> event tracing.
>>
>> The key concept is to use a structure with an ending char array field of
>> undefined size and use such ability to allocate the minimal size on the ring
>> buffer to make the entry fit inside as opposite to a fixed length strings with
>> upper bound.
>>
>> This patch provides one new macro:
>>
>> -__ending_string(name, src)
>>
>> This one declares the string to the structure inside TP_STRUCT__entry.
>> You need to provide the name of the string field and the source that will be
>> copied inside.
>> Two constraints: only one __ending_string() per TRACE_EVENT can be added and
>> it must be the last field to be declared. Hence the __ending prefix.
>>
>> This macro will declare the necessary field and will also add the dynamic
>> size of the string needed for the ring buffer entry allocation.
>>
>> It also support filtering because these strings behave essentially
>> like usual fixed length string.
>
> can't we simply do __string(name, src) and output something like:
>
>  struct {
>   u16 size;
>   char str[0];
>  } name;
>
> That would get rid of this __ending_ wart.
>
>

Hmm, I don't understand.
Such a thing doesn't seem to work. Once we fill the string it would
override the fields that
follow it if it's not at the end.

Re: [PATCH 1/2 v2] tracing/events: provide string with undefined size support

[Peter Zijlstra]

On Thu, 2009-04-16 at 22:28 +0200, Frédéric Weisbecker wrote:
> 2009/4/16 Peter Zijlstra <a.p.zijlstra@chello.nl>:
> > On Thu, 2009-04-16 at 22:00 +0200, Frederic Weisbecker wrote:
> >> Impact: less memory usage for tracing
> >>
> >> This patch provides the support for dynamic size strings on
> >> event tracing.
> >>
> >> The key concept is to use a structure with an ending char array field of
> >> undefined size and use such ability to allocate the minimal size on the ring
> >> buffer to make the entry fit inside as opposite to a fixed length strings with
> >> upper bound.
> >>
> >> This patch provides one new macro:
> >>
> >> -__ending_string(name, src)
> >>
> >> This one declares the string to the structure inside TP_STRUCT__entry.
> >> You need to provide the name of the string field and the source that will be
> >> copied inside.
> >> Two constraints: only one __ending_string() per TRACE_EVENT can be added and
> >> it must be the last field to be declared. Hence the __ending prefix.
> >>
> >> This macro will declare the necessary field and will also add the dynamic
> >> size of the string needed for the ring buffer entry allocation.
> >>
> >> It also support filtering because these strings behave essentially
> >> like usual fixed length string.
> >
> > can't we simply do __string(name, src) and output something like:
> >
> >  struct {
> >   u16 size;
> >   char str[0];
> >  } name;
> >
> > That would get rid of this __ending_ wart.
> >
> >
> 
> Hmm, I don't understand.
> Such a thing doesn't seem to work. Once we fill the string it would
> override the fields that
> follow it if it's not at the end.

Just grow the thing to fit whatever string length -- rather common
pattern:

struct foo {
  int length;
  char data[0];
};

struct foo *bar = kmalloc(sizeof(struct foo) + data_size);

and bob's your uncle.

Re: [PATCH 1/2 v2] tracing/events: provide string with undefined size support

[Frédéric Weisbecker]

Le 17 avril 2009 08:22, Peter Zijlstra <a.p.zijlstra@chello.nl> a écrit :
> On Thu, 2009-04-16 at 22:28 +0200, Frédéric Weisbecker wrote:
>> 2009/4/16 Peter Zijlstra <a.p.zijlstra@chello.nl>:
>> > On Thu, 2009-04-16 at 22:00 +0200, Frederic Weisbecker wrote:
>> >> Impact: less memory usage for tracing
>> >>
>> >> This patch provides the support for dynamic size strings on
>> >> event tracing.
>> >>
>> >> The key concept is to use a structure with an ending char array field of
>> >> undefined size and use such ability to allocate the minimal size on the ring
>> >> buffer to make the entry fit inside as opposite to a fixed length strings with
>> >> upper bound.
>> >>
>> >> This patch provides one new macro:
>> >>
>> >> -__ending_string(name, src)
>> >>
>> >> This one declares the string to the structure inside TP_STRUCT__entry.
>> >> You need to provide the name of the string field and the source that will be
>> >> copied inside.
>> >> Two constraints: only one __ending_string() per TRACE_EVENT can be added and
>> >> it must be the last field to be declared. Hence the __ending prefix.
>> >>
>> >> This macro will declare the necessary field and will also add the dynamic
>> >> size of the string needed for the ring buffer entry allocation.
>> >>
>> >> It also support filtering because these strings behave essentially
>> >> like usual fixed length string.
>> >
>> > can't we simply do __string(name, src) and output something like:
>> >
>> >  struct {
>> >   u16 size;
>> >   char str[0];
>> >  } name;
>> >
>> > That would get rid of this __ending_ wart.
>> >
>> >
>>
>> Hmm, I don't understand.
>> Such a thing doesn't seem to work. Once we fill the string it would
>> override the fields that
>> follow it if it's not at the end.
>
> Just grow the thing to fit whatever string length -- rather common
> pattern:
>
> struct foo {
>  int length;
>  char data[0];
> };
>
> struct foo *bar = kmalloc(sizeof(struct foo) + data_size);
>
> and bob's your uncle.
>
>


Ok, but I can't do that.
What we do is allocating towards the ring buffer before filling:

struct foo {
   field1;
   field2;
   ...
};

struct foo *f;

event = ring_buffer_lock_reserve(sizeof(*f), ...);
f = ring_buffer_event_data(event);

I can't use a kmalloc here. We are tracing a random event which can happen
at a random frequency, random context, etc...

Re: [PATCH 1/2 v2] tracing/events: provide string with undefined size support

[Peter Zijlstra]

On Fri, 2009-04-17 at 10:59 +0200, Frédéric Weisbecker wrote:

> struct foo {
>    field1;
>    field2;
>    ...
> };
> 
> struct foo *f;
> 
> event = ring_buffer_lock_reserve(sizeof(*f), ...);
> f = ring_buffer_event_data(event);
> 
> I can't use a kmalloc here. We are tracing a random event which can happen
> at a random frequency, random context, etc...

Can't you add a bit to that ring_buffer_lock_reserve() thing?

The thing I do for perf_counters is I first iterate all the output, then
make the reserve large enough to fit all the output in, then copy the
bits into the output buffer.

The result is that the output cannot be interpreted as a fixed offset
struct, but that's not much of an issue anyway.

Another possibility is using relative pointers for strings that point
beyond the tail of the fixed offset struct.

So something like:

  __field(int, foo);
  __string(bar);
  __field(int, foo2);
  __string(bar2);
  __field(int, foo3);

would look like:

 struct plop {
   int foo;
   char *bar;
   int foo2;
   char *bar2;
   int foo3;

   char data[0];
 }

and you'd do something like:

 size = sizeof(struct plop);
 size += strlen(bar) + 1;
 size += strlen(bar2) + 1;

 event = ring_buffer_lock_reserve(size);
 offset = sizeof(struct plop);
 my_plop.bar = (char *)offset;
 offset += strlen(bar) + 1;
 my_plop.bar2 = (char *)offset;
 memcpy(&event, &my_plop, sizeof(struct plop));
 memcpy(&event + my_plop.bar, bar, strlen(bar)+1);
 memcpy(&event + my_plop.bar2, bar2, strlen(bar2)+1);
 ring_buffer_unlock();

Then on reading, you'd get a variable sized entry, with a fixed size
fixed offset struct, that contains relative offset character pointers.

Re: [PATCH 1/2 v2] tracing/events: provide string with undefined size support

[Frédéric Weisbecker]

Le 17 avril 2009 11:29, Peter Zijlstra <a.p.zijlstra@chello.nl> a écrit :
> On Fri, 2009-04-17 at 10:59 +0200, Frédéric Weisbecker wrote:
>
>> struct foo {
>>    field1;
>>    field2;
>>    ...
>> };
>>
>> struct foo *f;
>>
>> event = ring_buffer_lock_reserve(sizeof(*f), ...);
>> f = ring_buffer_event_data(event);
>>
>> I can't use a kmalloc here. We are tracing a random event which can happen
>> at a random frequency, random context, etc...
>
> Can't you add a bit to that ring_buffer_lock_reserve() thing?
>
> The thing I do for perf_counters is I first iterate all the output, then
> make the reserve large enough to fit all the output in, then copy the
> bits into the output buffer.
>
> The result is that the output cannot be interpreted as a fixed offset
> struct, but that's not much of an issue anyway.
>
> Another possibility is using relative pointers for strings that point
> beyond the tail of the fixed offset struct.
>
> So something like:
>
>  __field(int, foo);
>  __string(bar);
>  __field(int, foo2);
>  __string(bar2);
>  __field(int, foo3);
>
> would look like:
>
>  struct plop {
>   int foo;
>   char *bar;
>   int foo2;
>   char *bar2;
>   int foo3;
>
>   char data[0];
>  }
>
> and you'd do something like:
>
>  size = sizeof(struct plop);
>  size += strlen(bar) + 1;
>  size += strlen(bar2) + 1;
>
>  event = ring_buffer_lock_reserve(size);
>  offset = sizeof(struct plop);
>  my_plop.bar = (char *)offset;
>  offset += strlen(bar) + 1;
>  my_plop.bar2 = (char *)offset;
>  memcpy(&event, &my_plop, sizeof(struct plop));
>  memcpy(&event + my_plop.bar, bar, strlen(bar)+1);
>  memcpy(&event + my_plop.bar2, bar2, strlen(bar2)+1);
>  ring_buffer_unlock();
>
> Then on reading, you'd get a variable sized entry, with a fixed size
> fixed offset struct, that contains relative offset character pointers.



I like much this idea.
Just a small change on it: I could use absolute addresses

offset = sizeof(struct plop);
my_plop.bar = &entry + offset;
offset += strlen(str1) + 1;
my_plop.bar2 = &entry + offset;
strcpy(my_plop.bar, str1);
strcpy(my_plop.bar2, str2);

So that its integration will need very few changes to support printing
and filtering. It will just behave like usual char [..].

Nice! Thanks Peter.

Re: [PATCH 1/2 v2] tracing/events: provide string with undefined size support

[Frédéric Weisbecker]

Le 17 avril 2009 12:04, Frédéric Weisbecker <fweisbec@gmail.com> a écrit :
> Le 17 avril 2009 11:29, Peter Zijlstra <a.p.zijlstra@chello.nl> a écrit :
>> On Fri, 2009-04-17 at 10:59 +0200, Frédéric Weisbecker wrote:
>>
>>> struct foo {
>>>    field1;
>>>    field2;
>>>    ...
>>> };
>>>
>>> struct foo *f;
>>>
>>> event = ring_buffer_lock_reserve(sizeof(*f), ...);
>>> f = ring_buffer_event_data(event);
>>>
>>> I can't use a kmalloc here. We are tracing a random event which can happen
>>> at a random frequency, random context, etc...
>>
>> Can't you add a bit to that ring_buffer_lock_reserve() thing?
>>
>> The thing I do for perf_counters is I first iterate all the output, then
>> make the reserve large enough to fit all the output in, then copy the
>> bits into the output buffer.
>>
>> The result is that the output cannot be interpreted as a fixed offset
>> struct, but that's not much of an issue anyway.
>>
>> Another possibility is using relative pointers for strings that point
>> beyond the tail of the fixed offset struct.
>>
>> So something like:
>>
>>  __field(int, foo);
>>  __string(bar);
>>  __field(int, foo2);
>>  __string(bar2);
>>  __field(int, foo3);
>>
>> would look like:
>>
>>  struct plop {
>>   int foo;
>>   char *bar;
>>   int foo2;
>>   char *bar2;
>>   int foo3;
>>
>>   char data[0];
>>  }
>>
>> and you'd do something like:
>>
>>  size = sizeof(struct plop);
>>  size += strlen(bar) + 1;
>>  size += strlen(bar2) + 1;
>>
>>  event = ring_buffer_lock_reserve(size);
>>  offset = sizeof(struct plop);
>>  my_plop.bar = (char *)offset;
>>  offset += strlen(bar) + 1;
>>  my_plop.bar2 = (char *)offset;
>>  memcpy(&event, &my_plop, sizeof(struct plop));
>>  memcpy(&event + my_plop.bar, bar, strlen(bar)+1);
>>  memcpy(&event + my_plop.bar2, bar2, strlen(bar2)+1);
>>  ring_buffer_unlock();
>>
>> Then on reading, you'd get a variable sized entry, with a fixed size
>> fixed offset struct, that contains relative offset character pointers.
>
>
>
> I like much this idea.
> Just a small change on it: I could use absolute addresses
>

I messed up it below, but I guess you understood it.
Well just some fixes to be sure everyone understood the idea:

entry = get_entry_from_buffer();
offset = sizeof(struct plop);
entry->bar = entry + offset;
offset += strlen(str1) + 1;
entry->bar2 = entry + offset;
strcpy(entry->bar, str1);
strcpy(entry->bar2, str2);

> So that its integration will need very few changes to support printing
> and filtering. It will just behave like usual char [..].
>
> Nice! Thanks Peter.
>

Re: [PATCH 1/2 v2] tracing/events: provide string with undefined size support

[Steven Rostedt]

[ /me can't sleep, trying to get tired again ]


On Fri, 17 Apr 2009, Peter Zijlstra wrote:

> On Fri, 2009-04-17 at 10:59 +0200, Fr?d?ric Weisbecker wrote:
> 
> > struct foo {
> >    field1;
> >    field2;
> >    ...
> > };
> > 
> > struct foo *f;
> > 
> > event = ring_buffer_lock_reserve(sizeof(*f), ...);
> > f = ring_buffer_event_data(event);
> > 
> > I can't use a kmalloc here. We are tracing a random event which can happen
> > at a random frequency, random context, etc...
> 
> Can't you add a bit to that ring_buffer_lock_reserve() thing?
> 
> The thing I do for perf_counters is I first iterate all the output, then
> make the reserve large enough to fit all the output in, then copy the
> bits into the output buffer.
> 
> The result is that the output cannot be interpreted as a fixed offset
> struct, but that's not much of an issue anyway.
> 
> Another possibility is using relative pointers for strings that point
> beyond the tail of the fixed offset struct.
> 
> So something like:
> 
>   __field(int, foo);
>   __string(bar);
>   __field(int, foo2);
>   __string(bar2);
>   __field(int, foo3);
> 
> would look like:
> 
>  struct plop {
>    int foo;
>    char *bar;
>    int foo2;
>    char *bar2;
>    int foo3;
> 
>    char data[0];
>  }
> 
> and you'd do something like:
> 
>  size = sizeof(struct plop);
>  size += strlen(bar) + 1;
>  size += strlen(bar2) + 1;
> 
>  event = ring_buffer_lock_reserve(size);
>  offset = sizeof(struct plop);
>  my_plop.bar = (char *)offset;
>  offset += strlen(bar) + 1;
>  my_plop.bar2 = (char *)offset;
>  memcpy(&event, &my_plop, sizeof(struct plop));
>  memcpy(&event + my_plop.bar, bar, strlen(bar)+1);
>  memcpy(&event + my_plop.bar2, bar2, strlen(bar2)+1);
>  ring_buffer_unlock();
> 
> Then on reading, you'd get a variable sized entry, with a fixed size
> fixed offset struct, that contains relative offset character pointers.

When I replied to Frederic, I thought I could come up with a way to do 
something like you are proposing. Instead, I only ended up with the 
variant that Frederic implemented.

I've done what you are suggesting several times in tracing. Logdev does 
this in its tracing.

The problem that we have, is that we don't have actual code. We have a 
TRACE_EVENT macro that is doing the work for us. This, unfortunately, 
limits what we can do.

One response is to just free code it. Well, then all users of TRACE_EVENT 
would end up doing a lot more work too.

The more power we put into TRACE_EVENT, the uglier and more complex it 
ends up.

But that said, one could still do what you are suggesting. The "__ending" 
wart still needs to be there, since it must happen at the end of the 
struct.


        TP_STRUCT__entry(
                __field(int, str2loc)
                __field(int, str3loc)
		__string(str1)
		__string(str2)
                __ending_string(data, str3)
        ),
        TP_fast_assign(
		__entry->str2loc = strlen(str1);
		__entry->str3loc = strlen(str2) + __entry->str2loc;
                strcpy(__entry->data, str1);
		strcpy(__entry->data + __entry->str2loc, str2)
		strcpy(__entry->data + __entry->str3loc, str3)
        ),
        TP_printk("str1:%s str2:%s str3:%s\n",
		__entry->data,
		__entry->data + __entry->str2loc,
		__entry->date + __entry->str3loc)


For this to work, we need to just add a define for the __string macro for 
the reservation:

#define __string(x)  __str_size_ += strlen(x) + 1;

-- Steve

Re: [PATCH 1/2 v2] tracing/events: provide string with undefined size support

[Frédéric Weisbecker]

2009/4/17 Steven Rostedt <rostedt@goodmis.org>:
>
> [ /me can't sleep, trying to get tired again ]
>
>
> On Fri, 17 Apr 2009, Peter Zijlstra wrote:
>
>> On Fri, 2009-04-17 at 10:59 +0200, Fr?d?ric Weisbecker wrote:
>>
>> > struct foo {
>> >    field1;
>> >    field2;
>> >    ...
>> > };
>> >
>> > struct foo *f;
>> >
>> > event = ring_buffer_lock_reserve(sizeof(*f), ...);
>> > f = ring_buffer_event_data(event);
>> >
>> > I can't use a kmalloc here. We are tracing a random event which can happen
>> > at a random frequency, random context, etc...
>>
>> Can't you add a bit to that ring_buffer_lock_reserve() thing?
>>
>> The thing I do for perf_counters is I first iterate all the output, then
>> make the reserve large enough to fit all the output in, then copy the
>> bits into the output buffer.
>>
>> The result is that the output cannot be interpreted as a fixed offset
>> struct, but that's not much of an issue anyway.
>>
>> Another possibility is using relative pointers for strings that point
>> beyond the tail of the fixed offset struct.
>>
>> So something like:
>>
>>   __field(int, foo);
>>   __string(bar);
>>   __field(int, foo2);
>>   __string(bar2);
>>   __field(int, foo3);
>>
>> would look like:
>>
>>  struct plop {
>>    int foo;
>>    char *bar;
>>    int foo2;
>>    char *bar2;
>>    int foo3;
>>
>>    char data[0];
>>  }
>>
>> and you'd do something like:
>>
>>  size = sizeof(struct plop);
>>  size += strlen(bar) + 1;
>>  size += strlen(bar2) + 1;
>>
>>  event = ring_buffer_lock_reserve(size);
>>  offset = sizeof(struct plop);
>>  my_plop.bar = (char *)offset;
>>  offset += strlen(bar) + 1;
>>  my_plop.bar2 = (char *)offset;
>>  memcpy(&event, &my_plop, sizeof(struct plop));
>>  memcpy(&event + my_plop.bar, bar, strlen(bar)+1);
>>  memcpy(&event + my_plop.bar2, bar2, strlen(bar2)+1);
>>  ring_buffer_unlock();
>>
>> Then on reading, you'd get a variable sized entry, with a fixed size
>> fixed offset struct, that contains relative offset character pointers.
>
> When I replied to Frederic, I thought I could come up with a way to do
> something like you are proposing. Instead, I only ended up with the
> variant that Frederic implemented.
>
> I've done what you are suggesting several times in tracing. Logdev does
> this in its tracing.
>
> The problem that we have, is that we don't have actual code. We have a
> TRACE_EVENT macro that is doing the work for us. This, unfortunately,
> limits what we can do.
>
> One response is to just free code it. Well, then all users of TRACE_EVENT
> would end up doing a lot more work too.
>
> The more power we put into TRACE_EVENT, the uglier and more complex it
> ends up.
>
> But that said, one could still do what you are suggesting. The "__ending"
> wart still needs to be there, since it must happen at the end of the
> struct.
>
>
>        TP_STRUCT__entry(
>                __field(int, str2loc)
>                __field(int, str3loc)
>                __string(str1)
>                __string(str2)
>                __ending_string(data, str3)
>        ),


Or something like:

 TP_STRUCT__ending_string(
)

that would do the __ending_string itself...



>        TP_fast_assign(
>                __entry->str2loc = strlen(str1);
>                __entry->str3loc = strlen(str2) + __entry->str2loc;
>                strcpy(__entry->data, str1);
>                strcpy(__entry->data + __entry->str2loc, str2)
>                strcpy(__entry->data + __entry->str3loc, str3)
>        ),
>        TP_printk("str1:%s str2:%s str3:%s\n",
>                __entry->data,
>                __entry->data + __entry->str2loc,
>                __entry->date + __entry->str3loc)
>
>
> For this to work, we need to just add a define for the __string macro for
> the reservation:
>
> #define __string(x)  __str_size_ += strlen(x) + 1;
>
> -- Steve
>
>

Re: [PATCH 1/2 v2] tracing/events: provide string with undefined size support

[Peter Zijlstra]

On Fri, 2009-04-17 at 06:10 -0400, Steven Rostedt wrote:

> When I replied to Frederic, I thought I could come up with a way to do 
> something like you are proposing. Instead, I only ended up with the 
> variant that Frederic implemented.
> 
> I've done what you are suggesting several times in tracing. Logdev does 
> this in its tracing.
> 
> The problem that we have, is that we don't have actual code. We have a 
> TRACE_EVENT macro that is doing the work for us. This, unfortunately, 
> limits what we can do.

Can't you do things like:

#define __string(x) unsigned long length_##x, offset_##x;

  TP_STRUCT__entry

  size = sizeof(struct foo);

#define __string(x) length_##x = strlen(x) + 1; size += length_##x;

  TP_STRUCT__entry

  entry = ring_buffer_lock_reserve(size);
  offset = sizeof(stuct foo);

#define __string(x) \
	__entry->x = offset_##x = offset; offset += length_##x;

  TP_STRUCT__entry

  TP_fast_assign

#define _string(x) strcpy(&entry->data[offset_##x], x);

  TP_STRUCT__entry

  ring_buffer_unlock();

also, you don't need that __ending_string() thing, you can always end a
struct with char data[0], its 0 size ;-)

Re: [PATCH 1/2 v2] tracing/events: provide string with undefined size support

[Steven Rostedt]

On Fri, 17 Apr 2009, Steven Rostedt wrote:
> 
> But that said, one could still do what you are suggesting. The "__ending" 
> wart still needs to be there, since it must happen at the end of the 
> struct.
> 
> 
>         TP_STRUCT__entry(
>                 __field(int, str2loc)
>                 __field(int, str3loc)
> 		__string(str1)
> 		__string(str2)
>                 __ending_string(data, str3)
>         ),
>         TP_fast_assign(
> 		__entry->str2loc = strlen(str1);
> 		__entry->str3loc = strlen(str2) + __entry->str2loc;
>                 strcpy(__entry->data, str1);
> 		strcpy(__entry->data + __entry->str2loc, str2)
> 		strcpy(__entry->data + __entry->str3loc, str3)
>         ),
>         TP_printk("str1:%s str2:%s str3:%s\n",
> 		__entry->data,
> 		__entry->data + __entry->str2loc,
> 		__entry->date + __entry->str3loc)
> 
> 
> For this to work, we need to just add a define for the __string macro for 
> the reservation:
> 
> #define __string(x)  __str_size_ += strlen(x) + 1;

Hmm, thinking about this more, we could do...


Make all structs end with:

	char __str_data__[0];


then we could just have __string(dec, param), ie.


	__string(str1, param_str1)
	__string(str2, param_str2)
	__string(srt3, param_str3)

But instead of defining string as a character, it would just be an index 
into the __str_data_.

#define __string(dec, param)	int dec;


We can break up the TRACE_EVENT macro again to create the declaration:

#undef everything and make empty macros of all we don't use

#define __string(dec, str)	int __str_loc_##dec;

#define TRACE_EVENT(...)	\
static void ftrace_raw_event_##call(proto)	\
{	\
	struct ring_buffer_event *event;	\
	struct ftrace_raw_##call *entry;	\
	unsigned long flags;			\
	int pc;					\
	int _str_size_ = 0;			\
						\
	tstruct

#include the file

#undef __string
#define __string(str, param)	__str_loc_##str = _str_size_; \
				_str_size_ += strlen(param) + 1;

#define TRACE_EVENT(...)
	tstruct

#include the file again

Then we can do the assignment for the user:

#undef everything again
#define the usual suspects

#define __string(str, param)	__entry->str = __str_loc_##str;

#define TRACE_EVENT(...)
        local_save_flags(irq_flags);                                    \
        pc = preempt_count();                                           \
                                                                        \
        event = trace_current_buffer_lock_reserve(event_##call.id,      \
                                  sizeof(struct ftrace_raw_##call),     \
                                  irq_flags, pc);                       \
        if (!event)                                                     \
                return;                                                 \
        entry   = ring_buffer_event_data(event);                        \
                                                                        \
	tstruct								\
        assign;                                                         \

#include the file again

#undef everything again,

#define __string(str, param)	strcpy(__entry->__str_data__ + 		\
				__str_loc_##str, param);

#define TRACE_EVENT(...) \
	tstruct		\
	trace_current_buffer_unlock_commit(event, irq_flags, pc);	\
}


The tstruct would do the assign for the user.

Then this is what a TRACE_EVENT would look like:

TRACE_EVENT(my_event,

	TP_PROTO(char *str1, char *str2, char *str3),

	TP_ARGS(str1, str2, str3),

	TP_STRUCT__entry(
		__string(str1, str1)
		__string(str2, str2)
		__string(str3, str3)
	),

	TP_fast_assign(
		/* empty, strings are automated */
	),

	TP_printk("str1:%s str2:%s str3:%s\n",
		__entry->__str_data__ + __entry->str1,
		__entry->__str_data__ + __entry->str2,
		__entry->__str_data__ + __entry->str3)
);


/me feels more evil than ever ;-)

-- Steve

Re: [PATCH 1/2 v2] tracing/events: provide string with undefined size support

[Steven Rostedt]

On Fri, 17 Apr 2009, Peter Zijlstra wrote:

> On Fri, 2009-04-17 at 06:10 -0400, Steven Rostedt wrote:
> 
> > When I replied to Frederic, I thought I could come up with a way to do 
> > something like you are proposing. Instead, I only ended up with the 
> > variant that Frederic implemented.
> > 
> > I've done what you are suggesting several times in tracing. Logdev does 
> > this in its tracing.
> > 
> > The problem that we have, is that we don't have actual code. We have a 
> > TRACE_EVENT macro that is doing the work for us. This, unfortunately, 
> > limits what we can do.
> 
> Can't you do things like:
> 
> #define __string(x) unsigned long length_##x, offset_##x;
> 
>   TP_STRUCT__entry
> 
>   size = sizeof(struct foo);
> 
> #define __string(x) length_##x = strlen(x) + 1; size += length_##x;
> 
>   TP_STRUCT__entry
> 
>   entry = ring_buffer_lock_reserve(size);
>   offset = sizeof(stuct foo);
> 
> #define __string(x) \
> 	__entry->x = offset_##x = offset; offset += length_##x;
> 
>   TP_STRUCT__entry
> 
>   TP_fast_assign
> 
> #define _string(x) strcpy(&entry->data[offset_##x], x);
> 
>   TP_STRUCT__entry
> 
>   ring_buffer_unlock();
> 
> also, you don't need that __ending_string() thing, you can always end a
> struct with char data[0], its 0 size ;-)
> 

This looks very similar to what I suggested in the cross email.

Frederic,

You want to take a crack at implementing this. I think my description had 
a bit more details, but is basically the same as what Peter is describing 
here.

I'd just make __string become an offset (as described in my email)

Then the user could just index directly. They only need to worry about the 
print, the assignment would be automated.

-- Steve

Re: [PATCH 1/2 v2] tracing/events: provide string with undefined size support

[Frédéric Weisbecker]

2009/4/17 Steven Rostedt <rostedt@goodmis.org>:
>
> On Fri, 17 Apr 2009, Peter Zijlstra wrote:
>
>> On Fri, 2009-04-17 at 06:10 -0400, Steven Rostedt wrote:
>>
>> > When I replied to Frederic, I thought I could come up with a way to do
>> > something like you are proposing. Instead, I only ended up with the
>> > variant that Frederic implemented.
>> >
>> > I've done what you are suggesting several times in tracing. Logdev does
>> > this in its tracing.
>> >
>> > The problem that we have, is that we don't have actual code. We have a
>> > TRACE_EVENT macro that is doing the work for us. This, unfortunately,
>> > limits what we can do.
>>
>> Can't you do things like:
>>
>> #define __string(x) unsigned long length_##x, offset_##x;
>>
>>   TP_STRUCT__entry
>>
>>   size = sizeof(struct foo);
>>
>> #define __string(x) length_##x = strlen(x) + 1; size += length_##x;
>>
>>   TP_STRUCT__entry
>>
>>   entry = ring_buffer_lock_reserve(size);
>>   offset = sizeof(stuct foo);
>>
>> #define __string(x) \
>>       __entry->x = offset_##x = offset; offset += length_##x;
>>
>>   TP_STRUCT__entry
>>
>>   TP_fast_assign
>>
>> #define _string(x) strcpy(&entry->data[offset_##x], x);
>>
>>   TP_STRUCT__entry
>>
>>   ring_buffer_unlock();
>>
>> also, you don't need that __ending_string() thing, you can always end a
>> struct with char data[0], its 0 size ;-)
>>
>
> This looks very similar to what I suggested in the cross email.
>
> Frederic,
>
> You want to take a crack at implementing this. I think my description had
> a bit more details, but is basically the same as what Peter is describing
> here.
>
> I'd just make __string become an offset (as described in my email)
>
> Then the user could just index directly. They only need to worry about the
> print, the assignment would be automated.


Nice!
Ok, I will restart with it!


>
> -- Steve
>
>

Re: [PATCH 1/2 v2] tracing/events: provide string with undefined size support

[Frederic Weisbecker]

On Fri, Apr 17, 2009 at 06:39:56AM -0400, Steven Rostedt wrote:
> On Fri, 17 Apr 2009, Steven Rostedt wrote:
> > 
> > But that said, one could still do what you are suggesting. The "__ending" 
> > wart still needs to be there, since it must happen at the end of the 
> > struct.
> > 
> > 
> >         TP_STRUCT__entry(
> >                 __field(int, str2loc)
> >                 __field(int, str3loc)
> > 		__string(str1)
> > 		__string(str2)
> >                 __ending_string(data, str3)
> >         ),
> >         TP_fast_assign(
> > 		__entry->str2loc = strlen(str1);
> > 		__entry->str3loc = strlen(str2) + __entry->str2loc;
> >                 strcpy(__entry->data, str1);
> > 		strcpy(__entry->data + __entry->str2loc, str2)
> > 		strcpy(__entry->data + __entry->str3loc, str3)
> >         ),
> >         TP_printk("str1:%s str2:%s str3:%s\n",
> > 		__entry->data,
> > 		__entry->data + __entry->str2loc,
> > 		__entry->date + __entry->str3loc)
> > 
> > 
> > For this to work, we need to just add a define for the __string macro for 
> > the reservation:
> > 
> > #define __string(x)  __str_size_ += strlen(x) + 1;
> 
> Hmm, thinking about this more, we could do...
> 
> 
> Make all structs end with:

> 
> 	char __str_data__[0];
> 
> 
> then we could just have __string(dec, param), ie.
> 
> 
> 	__string(str1, param_str1)
> 	__string(str2, param_str2)
> 	__string(srt3, param_str3)
> 
> But instead of defining string as a character, it would just be an index 
> into the __str_data_.
> 
> #define __string(dec, param)	int dec;
> 
> 
> We can break up the TRACE_EVENT macro again to create the declaration:
> 
> #undef everything and make empty macros of all we don't use
> 
> #define __string(dec, str)	int __str_loc_##dec;
> 
> #define TRACE_EVENT(...)	\
> static void ftrace_raw_event_##call(proto)	\
> {	\
> 	struct ring_buffer_event *event;	\
> 	struct ftrace_raw_##call *entry;	\
> 	unsigned long flags;			\
> 	int pc;					\
> 	int _str_size_ = 0;			\
> 						\
> 	tstruct
> 
> #include the file
> 
> #undef __string
> #define __string(str, param)	__str_loc_##str = _str_size_; \
> 				_str_size_ += strlen(param) + 1;
> 
> #define TRACE_EVENT(...)
> 	tstruct
> 
> #include the file again


While I'm trying to implement this, I'm discovering that it's
impossible, unless we have only one TRACE_EVENT in the file we are
including, otherwise we would end up with interleaving functions:

 static void ftrace_raw_event_event1(proto)
 {
       struct ring_buffer_event *event;
       struct ftrace_raw_event1 *entry;
       unsigned long flags;
       int pc;
       int _str_size_ = 0;

       tstruct

 static void ftrace_raw_event_event2(proto)     
 {      
       struct ring_buffer_event *event;         
       struct ftrace_raw_event2 *entry;   
       unsigned long flags;                     
       int pc;                                  
       int _str_size_ = 0;                      

       tstruct


So we still need something inside the fast_assign to do the
assignment job.

Something like the following macro will suffice:

#define	__assign_string(field, src)	\
	strcpy(entry->__ending_str + __str_loc_##field, src);

That's not a big deal, we just actually need this in TP__fast_assign()
and then we are done.

And also:


> The tstruct would do the assign for the user.
> 
> Then this is what a TRACE_EVENT would look like:
> 
> TRACE_EVENT(my_event,
> 
> 	TP_PROTO(char *str1, char *str2, char *str3),
> 
> 	TP_ARGS(str1, str2, str3),
> 
> 	TP_STRUCT__entry(
> 		__string(str1, str1)
> 		__string(str2, str2)
> 		__string(str3, str3)
> 	),
> 
> 	TP_fast_assign(
> 		/* empty, strings are automated */
> 	),
> 
> 	TP_printk("str1:%s str2:%s str3:%s\n",
> 		__entry->__str_data__ + __entry->str1,
> 		__entry->__str_data__ + __entry->str2,
> 		__entry->__str_data__ + __entry->str3)



A simple macro here to simplify that for the users:

#define __get_str(item)		\
	__entry->__ending_str + __entry->__str_loc_#item

would be much more convenient.

Anyway, I'll submit it and will wait for your comments.

Frederic.


> );
> 
> 
> /me feels more evil than ever ;-)
> 
> -- Steve
> 
> 

Re: [PATCH 1/2 v2] tracing/events: provide string with undefined size support

[Steven Rostedt]

On Sat, 18 Apr 2009, Frederic Weisbecker wrote:
> > 
> > #undef __string
> > #define __string(str, param)	__str_loc_##str = _str_size_; \
> > 				_str_size_ += strlen(param) + 1;
> > 
> > #define TRACE_EVENT(...)
> > 	tstruct
> > 
> > #include the file again
> 
> 
> While I'm trying to implement this, I'm discovering that it's
> impossible, unless we have only one TRACE_EVENT in the file we are
> including, otherwise we would end up with interleaving functions:
> 
>  static void ftrace_raw_event_event1(proto)
>  {
>        struct ring_buffer_event *event;
>        struct ftrace_raw_event1 *entry;
>        unsigned long flags;
>        int pc;
>        int _str_size_ = 0;
> 
>        tstruct
> 
>  static void ftrace_raw_event_event2(proto)     
>  {      
>        struct ring_buffer_event *event;         
>        struct ftrace_raw_event2 *entry;   
>        unsigned long flags;                     
>        int pc;                                  
>        int _str_size_ = 0;                      
> 
>        tstruct

Ug, that's right.

> 
> 
> So we still need something inside the fast_assign to do the
> assignment job.
> 
> Something like the following macro will suffice:
> 
> #define	__assign_string(field, src)	\
> 	strcpy(entry->__ending_str + __str_loc_##field, src);
> 
> That's not a big deal, we just actually need this in TP__fast_assign()
> and then we are done.
> 
> And also:
> 
> 
> > The tstruct would do the assign for the user.
> > 
> > Then this is what a TRACE_EVENT would look like:
> > 
> > TRACE_EVENT(my_event,
> > 
> > 	TP_PROTO(char *str1, char *str2, char *str3),
> > 
> > 	TP_ARGS(str1, str2, str3),
> > 
> > 	TP_STRUCT__entry(
> > 		__string(str1, str1)
> > 		__string(str2, str2)
> > 		__string(str3, str3)
> > 	),
> > 
> > 	TP_fast_assign(
> > 		/* empty, strings are automated */
> > 	),
> > 
> > 	TP_printk("str1:%s str2:%s str3:%s\n",
> > 		__entry->__str_data__ + __entry->str1,
> > 		__entry->__str_data__ + __entry->str2,
> > 		__entry->__str_data__ + __entry->str3)
> 
> 
> 
> A simple macro here to simplify that for the users:
> 
> #define __get_str(item)		\
> 	__entry->__ending_str + __entry->__str_loc_#item
> 
> would be much more convenient.
> 
> Anyway, I'll submit it and will wait for your comments.

Sure, do what you think is needed. Make sure the change log has an 
example that includes multiple strings.

Thanks,

-- Steve