Re: [RFC PATCH 2/6] Driver Tracing Interface (DTI) code

2007-07-15 Thread Randy Dunlap
On Fri, 29 Jun 2007 22:23:53 -0500 Tom Zanussi wrote:

> The Driver Tracing Interface (DTI) code.
> 
> Signed-off-by: Tom Zanussi <[EMAIL PROTECTED]>
> Signed-off-by: David Wilder <[EMAIL PROTECTED]>
> ---
>  drivers/base/Kconfig   |   11 
>  drivers/base/Makefile  |1 
>  drivers/base/dti.c |  836 
> +
>  drivers/base/dti_merged_view.c |  332 
>  include/linux/dti.h|  293 ++
>  5 files changed, 1473 insertions(+)
> 
> diff --git a/drivers/base/Kconfig b/drivers/base/Kconfig
> index 5d6312e..fbc9c0e 100644
> --- a/drivers/base/Kconfig
> +++ b/drivers/base/Kconfig
> @@ -49,6 +49,17 @@ config DEBUG_DEVRES
>  
> If you are unsure about this, Say N here.
>  
> +config DTI
> + bool "Driver Tracing Interface (DTI)"
> + select GTSC
> + help
> + Provides  functions to write variable length trace records
> + into a wraparound memory trace buffer. One purpose of
> + this is to inspect the debug traces after a system crash in order to
> + analyze the reason for the failure. The traces are accessable from
> + system dumps via dump analysis tools like crash or lcrash. In live
> + systems the traces can be read via a debugfs interface.
> +

Indent help text by 2 spaces, i.e., tab + 2 spaces.

>  config SYS_HYPERVISOR
>   bool
>   default n

> diff --git a/drivers/base/dti.c b/drivers/base/dti.c
> new file mode 100644
> index 000..2feec11
> --- /dev/null
> +++ b/drivers/base/dti.c
> @@ -0,0 +1,836 @@
> +
> +extern int dti_create_merged_views(struct dti_info *dti);
> +extern void dti_remove_merged_views(struct dti_info *dti);

externs need to be in some header file.

> +struct file_operations level_fops;
> +

> +/**
> + * dti_register_level: create trace dir and level ctrl file

s/: / - / to be consistent with rest (or most) of kernel.  I.e.,
kernel-doc function format is

 * @func - short description

(throughout source file(s))
OK, the : is optional, but the - is needed.

> + *
> + * Internal - exported for setup macros.
> + */
> +struct dti_info *dti_register_level(const char *name, int level,
> + struct dti_handle *handle)
> +{
> + return __dti_register_level(name, level, sub_size(handle->size),
> + nr_sub(handle->size), handle);
> +}
> +EXPORT_SYMBOL_GPL(dti_register_level);
> +

> diff --git a/drivers/base/dti_merged_view.c b/drivers/base/dti_merged_view.c
> new file mode 100644
> index 000..b3fee0f
> --- /dev/null
> +++ b/drivers/base/dti_merged_view.c
> @@ -0,0 +1,332 @@
> +
> +struct dti_merged_view_info {
> + void *next_event; /* NULL if at end of buffer */
> + struct timeval last_read;
> + int cpu;
> + unsigned long long bytes_left;
> + void *buf;
> +loff_t pos;

use tab above instead of spaces.

> +};
> +
> +struct dti_merged_view {
> + void *current_header; /* header currently being read */
> + ssize_t header_bytes_left;
> + char header[80];
> + void *current_event; /* record currently being read */
> + ssize_t event_bytes_left;
> + struct rchan *chan;
> + int show_timestamps;
> + struct dti_merged_view_info info[NR_CPUS]; /* per-cpu buffer info */
> +} __attribute__ ((packed));
> +
> +struct file_operations dti_merged_view_fops;
> +struct file_operations dti_merged_ts_view_fops;
> +
> +/**
> + * dti_create_merged_views:
> + * Creates merged view files in the trace's parent.

Short description needs to be on same line as function name.

> + *
> + * @trace: trace handle to create view of
> + *
> + * returns 0 on sucess.
> + */
> +int dti_create_merged_views(struct dti_info *dti)
> +{
> + struct dentry *parent = dti->trace->dir;
> +
> + dti->merged_view = debugfs_create_file("merged", 0, parent, dti,
> +_merged_view_fops);
> +
> + if (dti->merged_view == NULL ||
> + dti->merged_view == (struct dentry *)-ENODEV) 
> + goto cleanup;
> +
> + dti->merged_ts_view = debugfs_create_file("merged-ts",
> +   0, parent, dti,
> +   _merged_ts_view_fops);
> +
> + if (dti->merged_ts_view == NULL ||
> + dti->merged_ts_view == (struct dentry *)-ENODEV) 
> + goto cleanup;
> +
> + return 0;
> +cleanup:
> + dti_remove_merged_views(dti);
> +
> + return -ENOMEM;
> +}
> +
> +static int dti_merged_view_close(struct inode *inode, struct file *filp)
> +{
> + int i;
> + struct dti_merged_view *view = filp->private_data;
> + 
> + for_each_possible_cpu(i)
> +if (view->info[i].buf)
> + free_page((unsigned long)view->info[i].buf);
> +kfree(view);

use tab instead of spaces.

> +
> + return 0;
> +}
> +

> +static void *next_smallest(int *smallest_cpu, struct dti_merged_view *view )
> 

Re: [RFC PATCH 2/6] Driver Tracing Interface (DTI) code

2007-07-15 Thread Randy Dunlap
On Fri, 29 Jun 2007 22:23:53 -0500 Tom Zanussi wrote:

 The Driver Tracing Interface (DTI) code.
 
 Signed-off-by: Tom Zanussi [EMAIL PROTECTED]
 Signed-off-by: David Wilder [EMAIL PROTECTED]
 ---
  drivers/base/Kconfig   |   11 
  drivers/base/Makefile  |1 
  drivers/base/dti.c |  836 
 +
  drivers/base/dti_merged_view.c |  332 
  include/linux/dti.h|  293 ++
  5 files changed, 1473 insertions(+)
 
 diff --git a/drivers/base/Kconfig b/drivers/base/Kconfig
 index 5d6312e..fbc9c0e 100644
 --- a/drivers/base/Kconfig
 +++ b/drivers/base/Kconfig
 @@ -49,6 +49,17 @@ config DEBUG_DEVRES
  
 If you are unsure about this, Say N here.
  
 +config DTI
 + bool Driver Tracing Interface (DTI)
 + select GTSC
 + help
 + Provides  functions to write variable length trace records
 + into a wraparound memory trace buffer. One purpose of
 + this is to inspect the debug traces after a system crash in order to
 + analyze the reason for the failure. The traces are accessable from
 + system dumps via dump analysis tools like crash or lcrash. In live
 + systems the traces can be read via a debugfs interface.
 +

Indent help text by 2 spaces, i.e., tab + 2 spaces.

  config SYS_HYPERVISOR
   bool
   default n

 diff --git a/drivers/base/dti.c b/drivers/base/dti.c
 new file mode 100644
 index 000..2feec11
 --- /dev/null
 +++ b/drivers/base/dti.c
 @@ -0,0 +1,836 @@
 +
 +extern int dti_create_merged_views(struct dti_info *dti);
 +extern void dti_remove_merged_views(struct dti_info *dti);

externs need to be in some header file.

 +struct file_operations level_fops;
 +

 +/**
 + * dti_register_level: create trace dir and level ctrl file

s/: / - / to be consistent with rest (or most) of kernel.  I.e.,
kernel-doc function format is

 * @func - short description

(throughout source file(s))
OK, the : is optional, but the - is needed.

 + *
 + * Internal - exported for setup macros.
 + */
 +struct dti_info *dti_register_level(const char *name, int level,
 + struct dti_handle *handle)
 +{
 + return __dti_register_level(name, level, sub_size(handle-size),
 + nr_sub(handle-size), handle);
 +}
 +EXPORT_SYMBOL_GPL(dti_register_level);
 +

 diff --git a/drivers/base/dti_merged_view.c b/drivers/base/dti_merged_view.c
 new file mode 100644
 index 000..b3fee0f
 --- /dev/null
 +++ b/drivers/base/dti_merged_view.c
 @@ -0,0 +1,332 @@
 +
 +struct dti_merged_view_info {
 + void *next_event; /* NULL if at end of buffer */
 + struct timeval last_read;
 + int cpu;
 + unsigned long long bytes_left;
 + void *buf;
 +loff_t pos;

use tab above instead of spaces.

 +};
 +
 +struct dti_merged_view {
 + void *current_header; /* header currently being read */
 + ssize_t header_bytes_left;
 + char header[80];
 + void *current_event; /* record currently being read */
 + ssize_t event_bytes_left;
 + struct rchan *chan;
 + int show_timestamps;
 + struct dti_merged_view_info info[NR_CPUS]; /* per-cpu buffer info */
 +} __attribute__ ((packed));
 +
 +struct file_operations dti_merged_view_fops;
 +struct file_operations dti_merged_ts_view_fops;
 +
 +/**
 + * dti_create_merged_views:
 + * Creates merged view files in the trace's parent.

Short description needs to be on same line as function name.

 + *
 + * @trace: trace handle to create view of
 + *
 + * returns 0 on sucess.
 + */
 +int dti_create_merged_views(struct dti_info *dti)
 +{
 + struct dentry *parent = dti-trace-dir;
 +
 + dti-merged_view = debugfs_create_file(merged, 0, parent, dti,
 +dti_merged_view_fops);
 +
 + if (dti-merged_view == NULL ||
 + dti-merged_view == (struct dentry *)-ENODEV) 
 + goto cleanup;
 +
 + dti-merged_ts_view = debugfs_create_file(merged-ts,
 +   0, parent, dti,
 +   dti_merged_ts_view_fops);
 +
 + if (dti-merged_ts_view == NULL ||
 + dti-merged_ts_view == (struct dentry *)-ENODEV) 
 + goto cleanup;
 +
 + return 0;
 +cleanup:
 + dti_remove_merged_views(dti);
 +
 + return -ENOMEM;
 +}
 +
 +static int dti_merged_view_close(struct inode *inode, struct file *filp)
 +{
 + int i;
 + struct dti_merged_view *view = filp-private_data;
 + 
 + for_each_possible_cpu(i)
 +if (view-info[i].buf)
 + free_page((unsigned long)view-info[i].buf);
 +kfree(view);

use tab instead of spaces.

 +
 + return 0;
 +}
 +

 +static void *next_smallest(int *smallest_cpu, struct dti_merged_view *view )
 +{
 +int i;
 +void *next, *smallest = NULL;

use tab instead of spaces

 +
 + for_each_possible_cpu(i) {
 + if 

Re: [RFC PATCH 2/6] Driver Tracing Interface (DTI) code

2007-07-05 Thread Mathieu Desnoyers
* Tom Zanussi ([EMAIL PROTECTED]) wrote:
> On Tue, 2007-07-03 at 23:13 -0400, Mathieu Desnoyers wrote:
> > > +void *__dti_reserve(struct dti_info *dti, int prio, size_t len)
> > > +{
> > > + struct dti_event *event;
> > > + int event_len;
> > > +
> > > + if (!dti)
> > > + return NULL;
> > > +
> > > + if (prio > dti->level)
> > > + return NULL;
> > > +
> > > + event_len = len + sizeof(*event);
> > > +
> > > + event = relay_reserve(dti->trace->rchan, event_len);
> > > + if (!event)
> > > + return NULL;
> > > +
> > > + event->time = sched_clock();
> > 
> > sched_clock() is dangerous.. you have to make sure you provide correct
> > clock errors in the trace parsing.. not exactly something interesting
> > when you are looking at the "one" case over 1 that was faulty. See
> > the i386 sched-clock.c:
> > 
> >  * Scheduler clock - returns current time in nanosec units.
> >  * All data is local to the CPU.
> >  * The values are approximately[1] monotonic local to a CPU, but not
> >  * between CPUs.   There might be also an occasionally random error,
> >  * but not too bad. Between CPUs the values can be non monotonic.
> >  *
> >  * [1] no attempt to stop CPU instruction reordering, which can hit
> >  * in a 100 instruction window or so.
> > 
> > How do you plan to use these timestamps to reorder events across CPUs ?
> > 
> 
> lttng_timestamp()? ;-)
> 

Yeah, this problem is not easy.. I have a set of per architecture
timestamping implementations that detects the limitations of the
hardware and overcomes them (32 bits TSC -> 64 bits extension that
supports read from NMI context, detect TSCs not in sync (AMDs and some
intels) that falls back to what is more or less a logical clock then,
printing warnings. I also keep a recipe cookbook about what people
should disable on these architectures to get precise timestamps.
> 
> > > +
> > > +static int vprintk_normal(struct dti_info *dti, int prio, const char* 
> > > fmt,
> > > +   va_list args)
> > > +{
> > > + struct dti_event *event;
> > > + void *buf;
> > > + int len, event_len, rc = -1;
> > > + unsigned long flags;
> > > +
> > > + if (!dti)
> > > + return -1;
> > > +
> > > + if (prio > dti->level)
> > > + return -1;
> > > +
> > > + local_irq_save(flags);
> > > + buf = dti_printf_tmpbuf[smp_processor_id()];
> > > + len = vsnprintf(buf, DTI_PRINTF_TMPBUF_SIZE, fmt, args);
> > 
> > As I said, why not put the result of the vsnprintf directly in the
> > buffer after the relay_reserve ?
> > 
> 
> Because I don't know how much to reserve until I've done the
> vsnprintf().  I think that to have vsnprintf() print directly into the
> relay buffer, I'd need a relay_unreserve() to remove the unused portion.
> 

This is why I use a 2 pass approach in LTTng: the first one, given a
NULL buffer, calculates the reserve size, and then the second pass does
the copy.

> > > +
> > > +/**
> > > + *   dti_relog_initbuf - re-log static initbuf into real relay 
> > > channel
> > > + *   @work: work struct that contains the the dti handle
> > > + *
> > > + *   The global initbuf may contain events from multiple cpus.  These
> > > + *   events must be put into their respective cpu buffers once the
> > > + *   per-cpu channel is available.
> > > + *
> > > + *   Internal - exported for setup macros.
> > > + */
> > > +int dti_relog_initbuf(struct dti_handle *handle)
> > > +{
> > 
> > What do you do with the data recorded by the system while you do this
> > buffer copy? Is the timestamp ordering kept? Why do you copy the static
> > buffers into other buffers at all anyway ? They could be exported
> > through relay (if it supported exporting statically mapped buffers).
> > 
> 
> You're right, we need to do something about that.  As mentioned before,
> we copy the events from the static buffer into the relay channel (the
> timestamp ordering is kept) after it becomes available because that
> seems like the simplest thing to do.  We could add support to relay to
> export another set of (static) buffers, or add the relogging code to
> relay if other tracers want to use it.
> 
> 

The problem with relogging is that it will be a huge timestamping mess.
I doubt that we can reliably continue tracing while we do this copy.
Therefore, we will end up losing events or having timestamps jumping
backward.

A solution that would export the static buffers separately to user-space
and (maybe) allows them to be (somehow?) de-allocated once they have
been read by user-space seems cleaner to me. The switch between the
static buffers and the normal relay buffers could then be done
atomically in a simple pointer overwrite.

Mathieu

-- 
Mathieu Desnoyers
Computer Engineering Ph.D. Student, Ecole Polytechnique de Montreal
OpenPGP key fingerprint: 8CD5 52C3 8E3C 4140 715F  BA06 3F25 A8FE 3BAE 9A68
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  

Re: [RFC PATCH 2/6] Driver Tracing Interface (DTI) code

2007-07-05 Thread Tom Zanussi
On Tue, 2007-07-03 at 23:13 -0400, Mathieu Desnoyers wrote:
> > +void *__dti_reserve(struct dti_info *dti, int prio, size_t len)
> > +{
> > +   struct dti_event *event;
> > +   int event_len;
> > +
> > +   if (!dti)
> > +   return NULL;
> > +
> > +   if (prio > dti->level)
> > +   return NULL;
> > +
> > +   event_len = len + sizeof(*event);
> > +
> > +   event = relay_reserve(dti->trace->rchan, event_len);
> > +   if (!event)
> > +   return NULL;
> > +
> > +   event->time = sched_clock();
> 
> sched_clock() is dangerous.. you have to make sure you provide correct
> clock errors in the trace parsing.. not exactly something interesting
> when you are looking at the "one" case over 1 that was faulty. See
> the i386 sched-clock.c:
> 
>  * Scheduler clock - returns current time in nanosec units.
>  * All data is local to the CPU.
>  * The values are approximately[1] monotonic local to a CPU, but not
>  * between CPUs.   There might be also an occasionally random error,
>  * but not too bad. Between CPUs the values can be non monotonic.
>  *
>  * [1] no attempt to stop CPU instruction reordering, which can hit
>  * in a 100 instruction window or so.
> 
> How do you plan to use these timestamps to reorder events across CPUs ?
> 

lttng_timestamp()? ;-)


> > +
> > +static int vprintk_normal(struct dti_info *dti, int prio, const char* fmt,
> > + va_list args)
> > +{
> > +   struct dti_event *event;
> > +   void *buf;
> > +   int len, event_len, rc = -1;
> > +   unsigned long flags;
> > +
> > +   if (!dti)
> > +   return -1;
> > +
> > +   if (prio > dti->level)
> > +   return -1;
> > +
> > +   local_irq_save(flags);
> > +   buf = dti_printf_tmpbuf[smp_processor_id()];
> > +   len = vsnprintf(buf, DTI_PRINTF_TMPBUF_SIZE, fmt, args);
> 
> As I said, why not put the result of the vsnprintf directly in the
> buffer after the relay_reserve ?
> 

Because I don't know how much to reserve until I've done the
vsnprintf().  I think that to have vsnprintf() print directly into the
relay buffer, I'd need a relay_unreserve() to remove the unused portion.

> > +
> > +/**
> > + * dti_relog_initbuf - re-log static initbuf into real relay channel
> > + * @work: work struct that contains the the dti handle
> > + *
> > + * The global initbuf may contain events from multiple cpus.  These
> > + * events must be put into their respective cpu buffers once the
> > + * per-cpu channel is available.
> > + *
> > + * Internal - exported for setup macros.
> > + */
> > +int dti_relog_initbuf(struct dti_handle *handle)
> > +{
> 
> What do you do with the data recorded by the system while you do this
> buffer copy? Is the timestamp ordering kept? Why do you copy the static
> buffers into other buffers at all anyway ? They could be exported
> through relay (if it supported exporting statically mapped buffers).
> 

You're right, we need to do something about that.  As mentioned before,
we copy the events from the static buffer into the relay channel (the
timestamp ordering is kept) after it becomes available because that
seems like the simplest thing to do.  We could add support to relay to
export another set of (static) buffers, or add the relogging code to
relay if other tracers want to use it.

Tom





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


Re: [RFC PATCH 2/6] Driver Tracing Interface (DTI) code

2007-07-05 Thread Tom Zanussi
On Tue, 2007-07-03 at 23:13 -0400, Mathieu Desnoyers wrote:
  +void *__dti_reserve(struct dti_info *dti, int prio, size_t len)
  +{
  +   struct dti_event *event;
  +   int event_len;
  +
  +   if (!dti)
  +   return NULL;
  +
  +   if (prio  dti-level)
  +   return NULL;
  +
  +   event_len = len + sizeof(*event);
  +
  +   event = relay_reserve(dti-trace-rchan, event_len);
  +   if (!event)
  +   return NULL;
  +
  +   event-time = sched_clock();
 
 sched_clock() is dangerous.. you have to make sure you provide correct
 clock errors in the trace parsing.. not exactly something interesting
 when you are looking at the one case over 1 that was faulty. See
 the i386 sched-clock.c:
 
  * Scheduler clock - returns current time in nanosec units.
  * All data is local to the CPU.
  * The values are approximately[1] monotonic local to a CPU, but not
  * between CPUs.   There might be also an occasionally random error,
  * but not too bad. Between CPUs the values can be non monotonic.
  *
  * [1] no attempt to stop CPU instruction reordering, which can hit
  * in a 100 instruction window or so.
 
 How do you plan to use these timestamps to reorder events across CPUs ?
 

lttng_timestamp()? ;-)


  +
  +static int vprintk_normal(struct dti_info *dti, int prio, const char* fmt,
  + va_list args)
  +{
  +   struct dti_event *event;
  +   void *buf;
  +   int len, event_len, rc = -1;
  +   unsigned long flags;
  +
  +   if (!dti)
  +   return -1;
  +
  +   if (prio  dti-level)
  +   return -1;
  +
  +   local_irq_save(flags);
  +   buf = dti_printf_tmpbuf[smp_processor_id()];
  +   len = vsnprintf(buf, DTI_PRINTF_TMPBUF_SIZE, fmt, args);
 
 As I said, why not put the result of the vsnprintf directly in the
 buffer after the relay_reserve ?
 

Because I don't know how much to reserve until I've done the
vsnprintf().  I think that to have vsnprintf() print directly into the
relay buffer, I'd need a relay_unreserve() to remove the unused portion.

  +
  +/**
  + * dti_relog_initbuf - re-log static initbuf into real relay channel
  + * @work: work struct that contains the the dti handle
  + *
  + * The global initbuf may contain events from multiple cpus.  These
  + * events must be put into their respective cpu buffers once the
  + * per-cpu channel is available.
  + *
  + * Internal - exported for setup macros.
  + */
  +int dti_relog_initbuf(struct dti_handle *handle)
  +{
 
 What do you do with the data recorded by the system while you do this
 buffer copy? Is the timestamp ordering kept? Why do you copy the static
 buffers into other buffers at all anyway ? They could be exported
 through relay (if it supported exporting statically mapped buffers).
 

You're right, we need to do something about that.  As mentioned before,
we copy the events from the static buffer into the relay channel (the
timestamp ordering is kept) after it becomes available because that
seems like the simplest thing to do.  We could add support to relay to
export another set of (static) buffers, or add the relogging code to
relay if other tracers want to use it.

Tom





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


Re: [RFC PATCH 2/6] Driver Tracing Interface (DTI) code

2007-07-05 Thread Mathieu Desnoyers
* Tom Zanussi ([EMAIL PROTECTED]) wrote:
 On Tue, 2007-07-03 at 23:13 -0400, Mathieu Desnoyers wrote:
   +void *__dti_reserve(struct dti_info *dti, int prio, size_t len)
   +{
   + struct dti_event *event;
   + int event_len;
   +
   + if (!dti)
   + return NULL;
   +
   + if (prio  dti-level)
   + return NULL;
   +
   + event_len = len + sizeof(*event);
   +
   + event = relay_reserve(dti-trace-rchan, event_len);
   + if (!event)
   + return NULL;
   +
   + event-time = sched_clock();
  
  sched_clock() is dangerous.. you have to make sure you provide correct
  clock errors in the trace parsing.. not exactly something interesting
  when you are looking at the one case over 1 that was faulty. See
  the i386 sched-clock.c:
  
   * Scheduler clock - returns current time in nanosec units.
   * All data is local to the CPU.
   * The values are approximately[1] monotonic local to a CPU, but not
   * between CPUs.   There might be also an occasionally random error,
   * but not too bad. Between CPUs the values can be non monotonic.
   *
   * [1] no attempt to stop CPU instruction reordering, which can hit
   * in a 100 instruction window or so.
  
  How do you plan to use these timestamps to reorder events across CPUs ?
  
 
 lttng_timestamp()? ;-)
 

Yeah, this problem is not easy.. I have a set of per architecture
timestamping implementations that detects the limitations of the
hardware and overcomes them (32 bits TSC - 64 bits extension that
supports read from NMI context, detect TSCs not in sync (AMDs and some
intels) that falls back to what is more or less a logical clock then,
printing warnings. I also keep a recipe cookbook about what people
should disable on these architectures to get precise timestamps.
 
   +
   +static int vprintk_normal(struct dti_info *dti, int prio, const char* 
   fmt,
   +   va_list args)
   +{
   + struct dti_event *event;
   + void *buf;
   + int len, event_len, rc = -1;
   + unsigned long flags;
   +
   + if (!dti)
   + return -1;
   +
   + if (prio  dti-level)
   + return -1;
   +
   + local_irq_save(flags);
   + buf = dti_printf_tmpbuf[smp_processor_id()];
   + len = vsnprintf(buf, DTI_PRINTF_TMPBUF_SIZE, fmt, args);
  
  As I said, why not put the result of the vsnprintf directly in the
  buffer after the relay_reserve ?
  
 
 Because I don't know how much to reserve until I've done the
 vsnprintf().  I think that to have vsnprintf() print directly into the
 relay buffer, I'd need a relay_unreserve() to remove the unused portion.
 

This is why I use a 2 pass approach in LTTng: the first one, given a
NULL buffer, calculates the reserve size, and then the second pass does
the copy.

   +
   +/**
   + *   dti_relog_initbuf - re-log static initbuf into real relay 
   channel
   + *   @work: work struct that contains the the dti handle
   + *
   + *   The global initbuf may contain events from multiple cpus.  These
   + *   events must be put into their respective cpu buffers once the
   + *   per-cpu channel is available.
   + *
   + *   Internal - exported for setup macros.
   + */
   +int dti_relog_initbuf(struct dti_handle *handle)
   +{
  
  What do you do with the data recorded by the system while you do this
  buffer copy? Is the timestamp ordering kept? Why do you copy the static
  buffers into other buffers at all anyway ? They could be exported
  through relay (if it supported exporting statically mapped buffers).
  
 
 You're right, we need to do something about that.  As mentioned before,
 we copy the events from the static buffer into the relay channel (the
 timestamp ordering is kept) after it becomes available because that
 seems like the simplest thing to do.  We could add support to relay to
 export another set of (static) buffers, or add the relogging code to
 relay if other tracers want to use it.
 
 

The problem with relogging is that it will be a huge timestamping mess.
I doubt that we can reliably continue tracing while we do this copy.
Therefore, we will end up losing events or having timestamps jumping
backward.

A solution that would export the static buffers separately to user-space
and (maybe) allows them to be (somehow?) de-allocated once they have
been read by user-space seems cleaner to me. The switch between the
static buffers and the normal relay buffers could then be done
atomically in a simple pointer overwrite.

Mathieu

-- 
Mathieu Desnoyers
Computer Engineering Ph.D. Student, Ecole Polytechnique de Montreal
OpenPGP key fingerprint: 8CD5 52C3 8E3C 4140 715F  BA06 3F25 A8FE 3BAE 9A68
-
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/


Re: [RFC PATCH 2/6] Driver Tracing Interface (DTI) code

2007-07-03 Thread Mathieu Desnoyers
Please see comments below,

* Tom Zanussi ([EMAIL PROTECTED]) wrote:
> The Driver Tracing Interface (DTI) code.
> 
> Signed-off-by: Tom Zanussi <[EMAIL PROTECTED]>
> Signed-off-by: David Wilder <[EMAIL PROTECTED]>
> ---
>  drivers/base/Kconfig   |   11 
>  drivers/base/Makefile  |1 
>  drivers/base/dti.c |  836 
> +
>  drivers/base/dti_merged_view.c |  332 
>  include/linux/dti.h|  293 ++
>  5 files changed, 1473 insertions(+)
> 
> diff --git a/drivers/base/Kconfig b/drivers/base/Kconfig
> index 5d6312e..fbc9c0e 100644
> --- a/drivers/base/Kconfig
> +++ b/drivers/base/Kconfig
> @@ -49,6 +49,17 @@ config DEBUG_DEVRES
>  
> If you are unsure about this, Say N here.
>  
> +config DTI
> + bool "Driver Tracing Interface (DTI)"
> + select GTSC
> + help
> + Provides  functions to write variable length trace records
> + into a wraparound memory trace buffer. One purpose of
> + this is to inspect the debug traces after a system crash in order to
> + analyze the reason for the failure. The traces are accessable from
> + system dumps via dump analysis tools like crash or lcrash. In live
> + systems the traces can be read via a debugfs interface.
> +
>  config SYS_HYPERVISOR
>   bool
>   default n
> diff --git a/drivers/base/Makefile b/drivers/base/Makefile
> index b39ea3f..7caa5f5 100644
> --- a/drivers/base/Makefile
> +++ b/drivers/base/Makefile
> @@ -12,6 +12,7 @@ obj-$(CONFIG_NUMA)  += node.o
>  obj-$(CONFIG_MEMORY_HOTPLUG_SPARSE) += memory.o
>  obj-$(CONFIG_SMP)+= topology.o
>  obj-$(CONFIG_SYS_HYPERVISOR) += hypervisor.o
> +obj-$(CONFIG_DTI) += dti.o dti_merged_view.o
>  
>  ifeq ($(CONFIG_DEBUG_DRIVER),y)
>  EXTRA_CFLAGS += -DDEBUG
> diff --git a/drivers/base/dti.c b/drivers/base/dti.c
> new file mode 100644
> index 000..2feec11
> --- /dev/null
> +++ b/drivers/base/dti.c
> @@ -0,0 +1,836 @@
> +/*
> + *Linux Driver Tracing Interface.
> + *
> + *Copyright (C) IBM Corp. 2007
> + *Author(s): Tom Zanussi <[EMAIL PROTECTED]>
> + *   Dave Wilder <[EMAIL PROTECTED]>
> + *   Michael Holzheu <[EMAIL PROTECTED]>
> + */
> +
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +
> +extern int dti_create_merged_views(struct dti_info *dti);
> +extern void dti_remove_merged_views(struct dti_info *dti);
> +struct file_operations level_fops;
> +
> +static inline int nr_sub(int size)
> +{
> + if (size < 4)
> + return 0;
> + 
> + if (size >= 8 * PAGE_SIZE)
> + return 8;
> + else
> + return 4;
> +}
> +
> +static inline int sub_size(int size)
> +{
> + if (size < 4)
> + return 0;
> + 
> + if (size >= 8 * PAGE_SIZE)
> + return size / 8;
> + else
> + return size / 4;
> +}
> +
> +/*
> + * For dti_printk, maximum size of klog formatting buffer beyond which
> + * truncation will occur
> + */
> +#define DTI_PRINTF_TMPBUF_SIZE (1024)
> +
> +/* per-cpu dti_printf formatting temporary buffer */
> +static char dti_printf_tmpbuf[NR_CPUS][DTI_PRINTF_TMPBUF_SIZE];
> +

Why do you do a supplementary copy of this data? It could be written
directly to the buffers.

> +/*
> + * Low-level registration functions
> + */
> +
> +static struct dti_info *__dti_register_level(const char *name, int level,
> +  int sub_size, int nr_sub,
> +  struct dti_handle *handle)
> +{
> + struct dti_info *dti;
> +
> + dti = kzalloc(sizeof(*dti), GFP_KERNEL);
> + if(!dti)
> + return NULL;
> +
> + dti->trace = trace_setup("dti", name, sub_size, nr_sub,
> +  TRACE_FLIGHT_CHANNEL | TRACE_DISABLE_STATE);
> + if (!dti->trace)
> + goto setup_failed;
> + 
> + dti->handle = handle;
> + dti->level = level;
> + dti->level_ctrl = debugfs_create_file("level", 0,
> +   dti->trace->dir, dti,
> +   _fops);
> + if (!dti->level_ctrl) {
> + printk("Couldn't create level control file\n");
> + goto level_failed;
> + }
> +
> + strncpy(dti->name, name, NAME_MAX);
> +
> + return dti;
> +
> +level_failed:
> + trace_cleanup(dti->trace);
> +setup_failed:
> + kfree(dti);
> +
> + return NULL;
> +}
> +
> +/**
> + * dti_register_level: create trace dir and level ctrl file
> + *
> + * Internal - exported for setup macros.
> + */
> +struct dti_info *dti_register_level(const char *name, int level,
> + struct dti_handle *handle)
> +{
> + return __dti_register_level(name, level, sub_size(handle->size),
> + nr_sub(handle->size), handle);
> +}
> +EXPORT_SYMBOL_GPL(dti_register_level);
> +
> +static void 

Re: [RFC PATCH 2/6] Driver Tracing Interface (DTI) code

2007-07-03 Thread Mathieu Desnoyers
Please see comments below,

* Tom Zanussi ([EMAIL PROTECTED]) wrote:
 The Driver Tracing Interface (DTI) code.
 
 Signed-off-by: Tom Zanussi [EMAIL PROTECTED]
 Signed-off-by: David Wilder [EMAIL PROTECTED]
 ---
  drivers/base/Kconfig   |   11 
  drivers/base/Makefile  |1 
  drivers/base/dti.c |  836 
 +
  drivers/base/dti_merged_view.c |  332 
  include/linux/dti.h|  293 ++
  5 files changed, 1473 insertions(+)
 
 diff --git a/drivers/base/Kconfig b/drivers/base/Kconfig
 index 5d6312e..fbc9c0e 100644
 --- a/drivers/base/Kconfig
 +++ b/drivers/base/Kconfig
 @@ -49,6 +49,17 @@ config DEBUG_DEVRES
  
 If you are unsure about this, Say N here.
  
 +config DTI
 + bool Driver Tracing Interface (DTI)
 + select GTSC
 + help
 + Provides  functions to write variable length trace records
 + into a wraparound memory trace buffer. One purpose of
 + this is to inspect the debug traces after a system crash in order to
 + analyze the reason for the failure. The traces are accessable from
 + system dumps via dump analysis tools like crash or lcrash. In live
 + systems the traces can be read via a debugfs interface.
 +
  config SYS_HYPERVISOR
   bool
   default n
 diff --git a/drivers/base/Makefile b/drivers/base/Makefile
 index b39ea3f..7caa5f5 100644
 --- a/drivers/base/Makefile
 +++ b/drivers/base/Makefile
 @@ -12,6 +12,7 @@ obj-$(CONFIG_NUMA)  += node.o
  obj-$(CONFIG_MEMORY_HOTPLUG_SPARSE) += memory.o
  obj-$(CONFIG_SMP)+= topology.o
  obj-$(CONFIG_SYS_HYPERVISOR) += hypervisor.o
 +obj-$(CONFIG_DTI) += dti.o dti_merged_view.o
  
  ifeq ($(CONFIG_DEBUG_DRIVER),y)
  EXTRA_CFLAGS += -DDEBUG
 diff --git a/drivers/base/dti.c b/drivers/base/dti.c
 new file mode 100644
 index 000..2feec11
 --- /dev/null
 +++ b/drivers/base/dti.c
 @@ -0,0 +1,836 @@
 +/*
 + *Linux Driver Tracing Interface.
 + *
 + *Copyright (C) IBM Corp. 2007
 + *Author(s): Tom Zanussi [EMAIL PROTECTED]
 + *   Dave Wilder [EMAIL PROTECTED]
 + *   Michael Holzheu [EMAIL PROTECTED]
 + */
 +
 +#include linux/fs.h
 +#include linux/debugfs.h
 +#include linux/relay.h
 +#include linux/module.h
 +#include linux/hardirq.h
 +#include linux/dti.h
 +
 +extern int dti_create_merged_views(struct dti_info *dti);
 +extern void dti_remove_merged_views(struct dti_info *dti);
 +struct file_operations level_fops;
 +
 +static inline int nr_sub(int size)
 +{
 + if (size  4)
 + return 0;
 + 
 + if (size = 8 * PAGE_SIZE)
 + return 8;
 + else
 + return 4;
 +}
 +
 +static inline int sub_size(int size)
 +{
 + if (size  4)
 + return 0;
 + 
 + if (size = 8 * PAGE_SIZE)
 + return size / 8;
 + else
 + return size / 4;
 +}
 +
 +/*
 + * For dti_printk, maximum size of klog formatting buffer beyond which
 + * truncation will occur
 + */
 +#define DTI_PRINTF_TMPBUF_SIZE (1024)
 +
 +/* per-cpu dti_printf formatting temporary buffer */
 +static char dti_printf_tmpbuf[NR_CPUS][DTI_PRINTF_TMPBUF_SIZE];
 +

Why do you do a supplementary copy of this data? It could be written
directly to the buffers.

 +/*
 + * Low-level registration functions
 + */
 +
 +static struct dti_info *__dti_register_level(const char *name, int level,
 +  int sub_size, int nr_sub,
 +  struct dti_handle *handle)
 +{
 + struct dti_info *dti;
 +
 + dti = kzalloc(sizeof(*dti), GFP_KERNEL);
 + if(!dti)
 + return NULL;
 +
 + dti-trace = trace_setup(dti, name, sub_size, nr_sub,
 +  TRACE_FLIGHT_CHANNEL | TRACE_DISABLE_STATE);
 + if (!dti-trace)
 + goto setup_failed;
 + 
 + dti-handle = handle;
 + dti-level = level;
 + dti-level_ctrl = debugfs_create_file(level, 0,
 +   dti-trace-dir, dti,
 +   level_fops);
 + if (!dti-level_ctrl) {
 + printk(Couldn't create level control file\n);
 + goto level_failed;
 + }
 +
 + strncpy(dti-name, name, NAME_MAX);
 +
 + return dti;
 +
 +level_failed:
 + trace_cleanup(dti-trace);
 +setup_failed:
 + kfree(dti);
 +
 + return NULL;
 +}
 +
 +/**
 + * dti_register_level: create trace dir and level ctrl file
 + *
 + * Internal - exported for setup macros.
 + */
 +struct dti_info *dti_register_level(const char *name, int level,
 + struct dti_handle *handle)
 +{
 + return __dti_register_level(name, level, sub_size(handle-size),
 + nr_sub(handle-size), handle);
 +}
 +EXPORT_SYMBOL_GPL(dti_register_level);
 +
 +static void dti_unregister_level(struct dti_info *dti)
 +{
 + debugfs_remove(dti-level_ctrl);
 + trace_cleanup(dti-trace);
 + 

[RFC PATCH 2/6] Driver Tracing Interface (DTI) code

2007-06-29 Thread Tom Zanussi
The Driver Tracing Interface (DTI) code.

Signed-off-by: Tom Zanussi <[EMAIL PROTECTED]>
Signed-off-by: David Wilder <[EMAIL PROTECTED]>
---
 drivers/base/Kconfig   |   11 
 drivers/base/Makefile  |1 
 drivers/base/dti.c |  836 +
 drivers/base/dti_merged_view.c |  332 
 include/linux/dti.h|  293 ++
 5 files changed, 1473 insertions(+)

diff --git a/drivers/base/Kconfig b/drivers/base/Kconfig
index 5d6312e..fbc9c0e 100644
--- a/drivers/base/Kconfig
+++ b/drivers/base/Kconfig
@@ -49,6 +49,17 @@ config DEBUG_DEVRES
 
  If you are unsure about this, Say N here.
 
+config DTI
+   bool "Driver Tracing Interface (DTI)"
+   select GTSC
+   help
+   Provides  functions to write variable length trace records
+   into a wraparound memory trace buffer. One purpose of
+   this is to inspect the debug traces after a system crash in order to
+   analyze the reason for the failure. The traces are accessable from
+   system dumps via dump analysis tools like crash or lcrash. In live
+   systems the traces can be read via a debugfs interface.
+
 config SYS_HYPERVISOR
bool
default n
diff --git a/drivers/base/Makefile b/drivers/base/Makefile
index b39ea3f..7caa5f5 100644
--- a/drivers/base/Makefile
+++ b/drivers/base/Makefile
@@ -12,6 +12,7 @@ obj-$(CONFIG_NUMA)+= node.o
 obj-$(CONFIG_MEMORY_HOTPLUG_SPARSE) += memory.o
 obj-$(CONFIG_SMP)  += topology.o
 obj-$(CONFIG_SYS_HYPERVISOR) += hypervisor.o
+obj-$(CONFIG_DTI) += dti.o dti_merged_view.o
 
 ifeq ($(CONFIG_DEBUG_DRIVER),y)
 EXTRA_CFLAGS += -DDEBUG
diff --git a/drivers/base/dti.c b/drivers/base/dti.c
new file mode 100644
index 000..2feec11
--- /dev/null
+++ b/drivers/base/dti.c
@@ -0,0 +1,836 @@
+/*
+ *Linux Driver Tracing Interface.
+ *
+ *Copyright (C) IBM Corp. 2007
+ *Author(s): Tom Zanussi <[EMAIL PROTECTED]>
+ *   Dave Wilder <[EMAIL PROTECTED]>
+ *   Michael Holzheu <[EMAIL PROTECTED]>
+ */
+
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+
+extern int dti_create_merged_views(struct dti_info *dti);
+extern void dti_remove_merged_views(struct dti_info *dti);
+struct file_operations level_fops;
+
+static inline int nr_sub(int size)
+{
+   if (size < 4)
+   return 0;
+   
+   if (size >= 8 * PAGE_SIZE)
+   return 8;
+   else
+   return 4;
+}
+
+static inline int sub_size(int size)
+{
+   if (size < 4)
+   return 0;
+   
+   if (size >= 8 * PAGE_SIZE)
+   return size / 8;
+   else
+   return size / 4;
+}
+
+/*
+ * For dti_printk, maximum size of klog formatting buffer beyond which
+ * truncation will occur
+ */
+#define DTI_PRINTF_TMPBUF_SIZE (1024)
+
+/* per-cpu dti_printf formatting temporary buffer */
+static char dti_printf_tmpbuf[NR_CPUS][DTI_PRINTF_TMPBUF_SIZE];
+
+/*
+ * Low-level registration functions
+ */
+
+static struct dti_info *__dti_register_level(const char *name, int level,
+int sub_size, int nr_sub,
+struct dti_handle *handle)
+{
+   struct dti_info *dti;
+
+   dti = kzalloc(sizeof(*dti), GFP_KERNEL);
+   if(!dti)
+   return NULL;
+
+   dti->trace = trace_setup("dti", name, sub_size, nr_sub,
+TRACE_FLIGHT_CHANNEL | TRACE_DISABLE_STATE);
+   if (!dti->trace)
+   goto setup_failed;
+   
+   dti->handle = handle;
+   dti->level = level;
+   dti->level_ctrl = debugfs_create_file("level", 0,
+ dti->trace->dir, dti,
+ _fops);
+   if (!dti->level_ctrl) {
+   printk("Couldn't create level control file\n");
+   goto level_failed;
+   }
+
+   strncpy(dti->name, name, NAME_MAX);
+
+   return dti;
+
+level_failed:
+   trace_cleanup(dti->trace);
+setup_failed:
+   kfree(dti);
+
+   return NULL;
+}
+
+/**
+ * dti_register_level: create trace dir and level ctrl file
+ *
+ * Internal - exported for setup macros.
+ */
+struct dti_info *dti_register_level(const char *name, int level,
+   struct dti_handle *handle)
+{
+   return __dti_register_level(name, level, sub_size(handle->size),
+   nr_sub(handle->size), handle);
+}
+EXPORT_SYMBOL_GPL(dti_register_level);
+
+static void dti_unregister_level(struct dti_info *dti)
+{
+   debugfs_remove(dti->level_ctrl);
+   trace_cleanup(dti->trace);
+   kfree(dti);
+}
+
+/**
+ * dti_register_channel: create channel part of new trace
+ */
+static int dti_register_channel(struct dti_info *dti)
+{
+   int rc = 0;
+   
+   rc = trace_start(dti->trace);
+   if (rc)
+   return rc;

[RFC PATCH 2/6] Driver Tracing Interface (DTI) code

2007-06-29 Thread Tom Zanussi
The Driver Tracing Interface (DTI) code.

Signed-off-by: Tom Zanussi [EMAIL PROTECTED]
Signed-off-by: David Wilder [EMAIL PROTECTED]
---
 drivers/base/Kconfig   |   11 
 drivers/base/Makefile  |1 
 drivers/base/dti.c |  836 +
 drivers/base/dti_merged_view.c |  332 
 include/linux/dti.h|  293 ++
 5 files changed, 1473 insertions(+)

diff --git a/drivers/base/Kconfig b/drivers/base/Kconfig
index 5d6312e..fbc9c0e 100644
--- a/drivers/base/Kconfig
+++ b/drivers/base/Kconfig
@@ -49,6 +49,17 @@ config DEBUG_DEVRES
 
  If you are unsure about this, Say N here.
 
+config DTI
+   bool Driver Tracing Interface (DTI)
+   select GTSC
+   help
+   Provides  functions to write variable length trace records
+   into a wraparound memory trace buffer. One purpose of
+   this is to inspect the debug traces after a system crash in order to
+   analyze the reason for the failure. The traces are accessable from
+   system dumps via dump analysis tools like crash or lcrash. In live
+   systems the traces can be read via a debugfs interface.
+
 config SYS_HYPERVISOR
bool
default n
diff --git a/drivers/base/Makefile b/drivers/base/Makefile
index b39ea3f..7caa5f5 100644
--- a/drivers/base/Makefile
+++ b/drivers/base/Makefile
@@ -12,6 +12,7 @@ obj-$(CONFIG_NUMA)+= node.o
 obj-$(CONFIG_MEMORY_HOTPLUG_SPARSE) += memory.o
 obj-$(CONFIG_SMP)  += topology.o
 obj-$(CONFIG_SYS_HYPERVISOR) += hypervisor.o
+obj-$(CONFIG_DTI) += dti.o dti_merged_view.o
 
 ifeq ($(CONFIG_DEBUG_DRIVER),y)
 EXTRA_CFLAGS += -DDEBUG
diff --git a/drivers/base/dti.c b/drivers/base/dti.c
new file mode 100644
index 000..2feec11
--- /dev/null
+++ b/drivers/base/dti.c
@@ -0,0 +1,836 @@
+/*
+ *Linux Driver Tracing Interface.
+ *
+ *Copyright (C) IBM Corp. 2007
+ *Author(s): Tom Zanussi [EMAIL PROTECTED]
+ *   Dave Wilder [EMAIL PROTECTED]
+ *   Michael Holzheu [EMAIL PROTECTED]
+ */
+
+#include linux/fs.h
+#include linux/debugfs.h
+#include linux/relay.h
+#include linux/module.h
+#include linux/hardirq.h
+#include linux/dti.h
+
+extern int dti_create_merged_views(struct dti_info *dti);
+extern void dti_remove_merged_views(struct dti_info *dti);
+struct file_operations level_fops;
+
+static inline int nr_sub(int size)
+{
+   if (size  4)
+   return 0;
+   
+   if (size = 8 * PAGE_SIZE)
+   return 8;
+   else
+   return 4;
+}
+
+static inline int sub_size(int size)
+{
+   if (size  4)
+   return 0;
+   
+   if (size = 8 * PAGE_SIZE)
+   return size / 8;
+   else
+   return size / 4;
+}
+
+/*
+ * For dti_printk, maximum size of klog formatting buffer beyond which
+ * truncation will occur
+ */
+#define DTI_PRINTF_TMPBUF_SIZE (1024)
+
+/* per-cpu dti_printf formatting temporary buffer */
+static char dti_printf_tmpbuf[NR_CPUS][DTI_PRINTF_TMPBUF_SIZE];
+
+/*
+ * Low-level registration functions
+ */
+
+static struct dti_info *__dti_register_level(const char *name, int level,
+int sub_size, int nr_sub,
+struct dti_handle *handle)
+{
+   struct dti_info *dti;
+
+   dti = kzalloc(sizeof(*dti), GFP_KERNEL);
+   if(!dti)
+   return NULL;
+
+   dti-trace = trace_setup(dti, name, sub_size, nr_sub,
+TRACE_FLIGHT_CHANNEL | TRACE_DISABLE_STATE);
+   if (!dti-trace)
+   goto setup_failed;
+   
+   dti-handle = handle;
+   dti-level = level;
+   dti-level_ctrl = debugfs_create_file(level, 0,
+ dti-trace-dir, dti,
+ level_fops);
+   if (!dti-level_ctrl) {
+   printk(Couldn't create level control file\n);
+   goto level_failed;
+   }
+
+   strncpy(dti-name, name, NAME_MAX);
+
+   return dti;
+
+level_failed:
+   trace_cleanup(dti-trace);
+setup_failed:
+   kfree(dti);
+
+   return NULL;
+}
+
+/**
+ * dti_register_level: create trace dir and level ctrl file
+ *
+ * Internal - exported for setup macros.
+ */
+struct dti_info *dti_register_level(const char *name, int level,
+   struct dti_handle *handle)
+{
+   return __dti_register_level(name, level, sub_size(handle-size),
+   nr_sub(handle-size), handle);
+}
+EXPORT_SYMBOL_GPL(dti_register_level);
+
+static void dti_unregister_level(struct dti_info *dti)
+{
+   debugfs_remove(dti-level_ctrl);
+   trace_cleanup(dti-trace);
+   kfree(dti);
+}
+
+/**
+ * dti_register_channel: create channel part of new trace
+ */
+static int dti_register_channel(struct dti_info *dti)
+{
+   int rc = 0;
+   
+   rc =