[ltt-dev] [PATCH take2 09/13] enable accessing struct ltt_channel_buf_struct

[Mathieu Desnoyers]

* Lai Jiangshan (laijs at cn.fujitsu.com) wrote:
> struct ltt_channel_buf_struct in ltt-relay.c and ltt-relay-locked.c
> are different to each other, we implement access-ops for access them.
> 
> Signed-off-by: Lai Jiangshan <laijs at cn.fujitsu.com>
> ---
> diff --git a/include/linux/ltt-channels.h b/include/linux/ltt-channels.h
> index 4a3924b..db513cb 100644
> --- a/include/linux/ltt-channels.h
> +++ b/include/linux/ltt-channels.h
> @@ -45,11 +48,18 @@ struct ltt_channel_struct {
>  	void (*buffer_end) (struct rchan_buf *buf,
>  			u64 tsc, unsigned int offset, unsigned int subbuf_idx);
>  	struct kref kref;	/* Channel transport reference count */
> +	struct ltt_channel_buf_access_ops *buf_access_ops;

OK, I'll make one thing clear here :

We must add a big big comment around the buf_access_ops to tell anyone
who ever consider using these in the tracing fast-path that it's _not a
good idea at all_. I know that you are just adding them so they can be
used generically be the binary-to-text converter, which is not fast path
anyway, but I foresee that people will want to misuse them and add an
unnecessary performance hit (many function calls) just for the fun of
adding another abstraction level in the tracing fast path.

So the implementation is OK, but please add a comment to that effect.
(cut and pasting text I write in these emails is OK by the way).

Mathieu


>  	unsigned int subbuf_size;
>  	unsigned int subbuf_cnt;
>  	const char *channel_name;
>  } ____cacheline_aligned;
>  
> +/* ops for accessing struct ltt_channel_struct.buf's data */
> +struct ltt_channel_buf_access_ops {
> +	unsigned long (*get_offset)(void *buf, int cpu);
> +	unsigned long (*get_consumed)(void *buf, int cpu);
> +};
> +
>  struct ltt_channel_setting {
>  	unsigned int subbuf_size;
>  	unsigned int subbuf_cnt;
> diff --git a/ltt/ltt-relay.c b/ltt/ltt-relay.c
> index 6d222c6..806558f
> --- a/ltt/ltt-relay.c
> +++ b/ltt/ltt-relay.c
> @@ -90,6 +90,24 @@ struct ltt_channel_buf_struct {
>  	atomic_t wakeup_readers;	/* Boolean : wakeup readers waiting ? */
>  } ____cacheline_aligned;
>  
> +static unsigned long get_offset(void *buf, int cpu)
> +{
> +	struct ltt_channel_buf_struct *ltt_buf = percpu_ptr(buf, cpu);
> +	return local_read(&ltt_buf->offset);
> +}
> +
> +static unsigned long get_consumed(void *buf, int cpu)
> +{
> +	struct ltt_channel_buf_struct *ltt_buf = percpu_ptr(buf, cpu);
> +	return atomic_long_read(&ltt_buf->consumed);
> +}
> +
> +static struct ltt_channel_buf_access_ops ltt_channel_buf_accesser =
> +{
> +	.get_offset   = get_offset,
> +	.get_consumed = get_consumed,
> +};
> +
>  /*
>   * Last TSC comparison functions. Check if the current TSC overflows
>   * LTT_TSC_BITS bits from the last TSC read. Reads and writes last_tsc
> diff --git a/ltt/ltt-relay-locked.c b/ltt/ltt-relay-locked.c
> index ef5f47c..a3c5bb9
> --- a/ltt/ltt-relay-locked.c
> +++ b/ltt/ltt-relay-locked.c
> @@ -81,6 +81,24 @@ struct ltt_channel_buf_struct {
>  	int wakeup_readers;		/* Boolean : wakeup readers waiting ? */
>  } ____cacheline_aligned;
>  
> +static unsigned long get_offset(void *buf, int cpu)
> +{
> +	struct ltt_channel_buf_struct *ltt_buf = percpu_ptr(buf, cpu);
> +	return ltt_buf->offset;
> +}
> +
> +static unsigned long get_consumed(void *buf, int cpu)
> +{
> +	struct ltt_channel_buf_struct *ltt_buf = percpu_ptr(buf, cpu);
> +	return ltt_buf->consumed;
> +}
> +
> +static struct ltt_channel_buf_access_ops ltt_channel_buf_accesser =
> +{
> +	.get_offset   = get_offset,
> +	.get_consumed = get_consumed,
> +};
> +
>  /*
>   * Last TSC comparison functions. Check if the current TSC overflows
>   * LTT_TSC_BITS bits from the last TSC read. Reads and writes last_tsc
> 
> 
> 
> _______________________________________________
> ltt-dev mailing list
> ltt-dev at lists.casi.polymtl.ca
> http://lists.casi.polymtl.ca/cgi-bin/mailman/listinfo/ltt-dev
> 

-- 
Mathieu Desnoyers
OpenPGP key fingerprint: 8CD5 52C3 8E3C 4140 715F  BA06 3F25 A8FE 3BAE 9A68