From: Steven Rostedt <rostedt@goodmis.org> To: Henrik Austad <henrik@austad.us> Cc: linux-kernel@vger.kernel.org, linux-media@vger.kernel.org, alsa-devel@vger.kernel.org, netdev@vger.kernel.org, Henrik Austad <haustad@cisco.com>, "David S. Miller" <davem@davemloft.net>, Ingo Molnar <mingo@redhat.com> Subject: Re: [very-RFC 6/8] Add TSN event-tracing Date: Sun, 12 Jun 2016 22:22:01 -0400 [thread overview] Message-ID: <20160612222201.00a8aa4c@grimm.local.home> (raw) In-Reply-To: <20160612212510.GD32724@icarus.home.austad.us> On Sun, 12 Jun 2016 23:25:10 +0200 Henrik Austad <henrik@austad.us> wrote: > > > +#include <linux/if_ether.h> > > > +#include <linux/if_vlan.h> > > > +/* #include <linux/skbuff.h> */ > > > + > > > +/* FIXME: update to TRACE_CLASS to reduce overhead */ > > > > I'm curious to why I didn't do this now. A class would make less > > duplication of typing too ;-) > > Yeah, I found this in a really great article written by some tracing-dude, > I hear he talks really, really fast! I plead the 5th! > > https://lwn.net/Articles/381064/ > > > > +TRACE_EVENT(tsn_buffer_write, > > > + > > > + TP_PROTO(struct tsn_link *link, > > > + size_t bytes), > > > + > > > + TP_ARGS(link, bytes), > > > + > > > + TP_STRUCT__entry( > > > + __field(u64, stream_id) > > > + __field(size_t, size) > > > + __field(size_t, bsize) > > > + __field(size_t, size_left) > > > + __field(void *, buffer) > > > + __field(void *, head) > > > + __field(void *, tail) > > > + __field(void *, end) > > > + ), > > > + > > > + TP_fast_assign( > > > + __entry->stream_id = link->stream_id; > > > + __entry->size = bytes; > > > + __entry->bsize = link->used_buffer_size; > > > + __entry->size_left = (link->head - link->tail) % link->used_buffer_size; > > > > Move this logic into the print statement, since you save head and tail. > > Ok, any particular reason? Because it removes calculations during the trace. The calculations done in TP_printk() are done at the time of reading the trace, and calculations done in TP_fast_assign() are done during the recording and hence adding more overhead to the trace itself. > > > > + __entry->buffer = link->buffer; > > > + __entry->head = link->head; > > > + __entry->tail = link->tail; > > > + __entry->end = link->end; > > > + ), > > > + > > > + TP_printk("stream_id=%llu, copy=%zd, buffer: %zd, avail=%zd, [buffer=%p, head=%p, tail=%p, end=%p]", > > > + __entry->stream_id, __entry->size, __entry->bsize, __entry->size_left, > > > > __entry->stream_id, __entry->size, __entry->bsize, > > (__entry->head - __entry->tail) % __entry->bsize, > > > > Ok, so is this about saving space by dropping one intermediate value, or is > it some other point I'm missing here? Nope, just moving the overhead from the recording of the trace to the reading of the trace. > > > > + __entry->buffer, __entry->head, __entry->tail, __entry->end) > > > + > > > + ); > > > + > > > + > > > + TP_fast_assign( > > > + __entry->stream_id = link->stream_id; > > > + __entry->vlan_tag = (skb_vlan_tag_present(skb) ? skb_vlan_tag_get(skb) : 0); > > > + __entry->bytes = bytes; > > > + __entry->data_len = skb->data_len; > > > + __entry->headlen = skb_headlen(skb); > > > + __entry->protocol = ntohs(vlan_get_protocol(skb)); > > > > Maybe it would be better to do the ntohs() in the TP_printk() as well. > > > > > + __entry->prot_native = ntohs(skb->protocol); > > > > here too. > > > > > + __entry->tx_idx = skb_get_queue_mapping(skb); > > > + > > > + __entry->mac_len = skb->mac_len; > > > + __entry->hdr_len = skb->hdr_len; > > > + __entry->vlan_tci = skb->vlan_tci; > > > + __entry->mac_header = skb->mac_header; > > > + __entry->tail = (unsigned int)skb->tail; > > > + __entry->end = (unsigned int)skb->end; > > > + __entry->truesize = skb->truesize; > > > + ), > > > + > > > + TP_printk("stream_id=%llu,vlan_tag=0x%04x,data_size=%zd,data_len=%zd,headlen=%u,proto=0x%04x (0x%04x),tx_idx=%d,mac_len=%u,hdr_len=%u,vlan_tci=0x%02x,mac_header=0x%02x,tail=%u,end=%u,truesize=%u", > > > + __entry->stream_id, > > > + __entry->vlan_tag, > > > + __entry->bytes, > > > + __entry->data_len, > > > + __entry->headlen, > > > + __entry->protocol, > > > + __entry->prot_native, __entry->tx_idx, > > > + __entry->mac_len, > > > + __entry->hdr_len, > > > + __entry->vlan_tci, > > > + __entry->mac_header, > > > > Is this an ether mac header? If so we support %M. But as it's defined > > as only u16, it doesn't seem like it can be. > > Actually, looking at the output, I'm not quite sure what it is that I > wanted to grab with that, the skb->mac_header should give an offset into > the header-area of skb, so it should be a constant offset from skb->head > (that is an actual pointer). > > I *think* I wanted to make sure I updated things correctly so that the > offset didn't suddenly change, but the fact that I'm no longer sure > indicates that I should just drop that one. That whole printout is too long > anyway.. > > Thanks for pointing a finger at this! > > > I'm still a bit stymied as to why logic should be in TP_printk() and not > TP_fast_assign(). Not that I really have any preferences either way, just > curious. As said above, it's simply just trying to make tracing have less of an effect on what is being traced. The more you can do in the post transactions the faster the trace becomes and less invasive the trace is on the system performance. -- Steve
WARNING: multiple messages have this Message-ID (diff)
From: Steven Rostedt <rostedt@goodmis.org> To: alsa-devel@alsa-project.org Cc: netdev@vger.kernel.org, linux-kernel@vger.kernel.org, "David S. Miller" <davem@davemloft.net>, Ingo Molnar <mingo@redhat.com>, Henrik Austad <haustad@cisco.com>, alsa-devel@vger.kernel.org, linux-media@vger.kernel.org Subject: Re: [very-RFC 6/8] Add TSN event-tracing Date: Sun, 12 Jun 2016 22:22:01 -0400 [thread overview] Message-ID: <20160612222201.00a8aa4c@grimm.local.home> (raw) In-Reply-To: <20160612212510.GD32724@icarus.home.austad.us> On Sun, 12 Jun 2016 23:25:10 +0200 Henrik Austad <henrik@austad.us> wrote: > > > +#include <linux/if_ether.h> > > > +#include <linux/if_vlan.h> > > > +/* #include <linux/skbuff.h> */ > > > + > > > +/* FIXME: update to TRACE_CLASS to reduce overhead */ > > > > I'm curious to why I didn't do this now. A class would make less > > duplication of typing too ;-) > > Yeah, I found this in a really great article written by some tracing-dude, > I hear he talks really, really fast! I plead the 5th! > > https://lwn.net/Articles/381064/ > > > > +TRACE_EVENT(tsn_buffer_write, > > > + > > > + TP_PROTO(struct tsn_link *link, > > > + size_t bytes), > > > + > > > + TP_ARGS(link, bytes), > > > + > > > + TP_STRUCT__entry( > > > + __field(u64, stream_id) > > > + __field(size_t, size) > > > + __field(size_t, bsize) > > > + __field(size_t, size_left) > > > + __field(void *, buffer) > > > + __field(void *, head) > > > + __field(void *, tail) > > > + __field(void *, end) > > > + ), > > > + > > > + TP_fast_assign( > > > + __entry->stream_id = link->stream_id; > > > + __entry->size = bytes; > > > + __entry->bsize = link->used_buffer_size; > > > + __entry->size_left = (link->head - link->tail) % link->used_buffer_size; > > > > Move this logic into the print statement, since you save head and tail. > > Ok, any particular reason? Because it removes calculations during the trace. The calculations done in TP_printk() are done at the time of reading the trace, and calculations done in TP_fast_assign() are done during the recording and hence adding more overhead to the trace itself. > > > > + __entry->buffer = link->buffer; > > > + __entry->head = link->head; > > > + __entry->tail = link->tail; > > > + __entry->end = link->end; > > > + ), > > > + > > > + TP_printk("stream_id=%llu, copy=%zd, buffer: %zd, avail=%zd, [buffer=%p, head=%p, tail=%p, end=%p]", > > > + __entry->stream_id, __entry->size, __entry->bsize, __entry->size_left, > > > > __entry->stream_id, __entry->size, __entry->bsize, > > (__entry->head - __entry->tail) % __entry->bsize, > > > > Ok, so is this about saving space by dropping one intermediate value, or is > it some other point I'm missing here? Nope, just moving the overhead from the recording of the trace to the reading of the trace. > > > > + __entry->buffer, __entry->head, __entry->tail, __entry->end) > > > + > > > + ); > > > + > > > + > > > + TP_fast_assign( > > > + __entry->stream_id = link->stream_id; > > > + __entry->vlan_tag = (skb_vlan_tag_present(skb) ? skb_vlan_tag_get(skb) : 0); > > > + __entry->bytes = bytes; > > > + __entry->data_len = skb->data_len; > > > + __entry->headlen = skb_headlen(skb); > > > + __entry->protocol = ntohs(vlan_get_protocol(skb)); > > > > Maybe it would be better to do the ntohs() in the TP_printk() as well. > > > > > + __entry->prot_native = ntohs(skb->protocol); > > > > here too. > > > > > + __entry->tx_idx = skb_get_queue_mapping(skb); > > > + > > > + __entry->mac_len = skb->mac_len; > > > + __entry->hdr_len = skb->hdr_len; > > > + __entry->vlan_tci = skb->vlan_tci; > > > + __entry->mac_header = skb->mac_header; > > > + __entry->tail = (unsigned int)skb->tail; > > > + __entry->end = (unsigned int)skb->end; > > > + __entry->truesize = skb->truesize; > > > + ), > > > + > > > + TP_printk("stream_id=%llu,vlan_tag=0x%04x,data_size=%zd,data_len=%zd,headlen=%u,proto=0x%04x (0x%04x),tx_idx=%d,mac_len=%u,hdr_len=%u,vlan_tci=0x%02x,mac_header=0x%02x,tail=%u,end=%u,truesize=%u", > > > + __entry->stream_id, > > > + __entry->vlan_tag, > > > + __entry->bytes, > > > + __entry->data_len, > > > + __entry->headlen, > > > + __entry->protocol, > > > + __entry->prot_native, __entry->tx_idx, > > > + __entry->mac_len, > > > + __entry->hdr_len, > > > + __entry->vlan_tci, > > > + __entry->mac_header, > > > > Is this an ether mac header? If so we support %M. But as it's defined > > as only u16, it doesn't seem like it can be. > > Actually, looking at the output, I'm not quite sure what it is that I > wanted to grab with that, the skb->mac_header should give an offset into > the header-area of skb, so it should be a constant offset from skb->head > (that is an actual pointer). > > I *think* I wanted to make sure I updated things correctly so that the > offset didn't suddenly change, but the fact that I'm no longer sure > indicates that I should just drop that one. That whole printout is too long > anyway.. > > Thanks for pointing a finger at this! > > > I'm still a bit stymied as to why logic should be in TP_printk() and not > TP_fast_assign(). Not that I really have any preferences either way, just > curious. As said above, it's simply just trying to make tracing have less of an effect on what is being traced. The more you can do in the post transactions the faster the trace becomes and less invasive the trace is on the system performance. -- Steve -- To unsubscribe from this list: send the line "unsubscribe alsa-devel" in
next prev parent reply other threads:[~2016-06-13 2:22 UTC|newest] Thread overview: 145+ messages / expand[flat|nested] mbox.gz Atom feed top 2016-06-11 23:01 [very-RFC 0/8] TSN driver for the kernel Henrik Austad 2016-06-11 23:01 ` [alsa-devel] " Henrik Austad 2016-06-11 23:01 ` Henrik Austad 2016-06-11 23:01 ` [very-RFC 1/8] TSN: add documentation Henrik Austad 2016-06-11 23:01 ` [alsa-devel] " Henrik Austad 2016-06-11 23:01 ` Henrik Austad 2016-06-11 23:01 ` [very-RFC 2/8] TSN: Add the standard formerly known as AVB to the kernel Henrik Austad 2016-06-11 23:01 ` Henrik Austad 2016-06-11 23:01 ` [alsa-devel] " Henrik Austad 2016-06-11 23:01 ` [very-RFC 3/8] Adding TSN-driver to Intel I210 controller Henrik Austad 2016-06-11 23:01 ` Henrik Austad 2016-06-11 23:01 ` [alsa-devel] " Henrik Austad 2016-06-11 23:01 ` [very-RFC 4/8] Add TSN header for the driver Henrik Austad 2016-06-11 23:01 ` Henrik Austad 2016-06-11 23:01 ` [alsa-devel] " Henrik Austad 2016-06-11 23:01 ` [very-RFC 5/8] Add TSN machinery to drive the traffic from a shim over the network Henrik Austad 2016-06-11 23:01 ` [alsa-devel] " Henrik Austad 2016-06-11 23:01 ` Henrik Austad 2016-06-11 23:01 ` [very-RFC 6/8] Add TSN event-tracing Henrik Austad 2016-06-11 23:01 ` Henrik Austad 2016-06-11 23:01 ` [alsa-devel] " Henrik Austad 2016-06-12 16:58 ` Steven Rostedt 2016-06-12 16:58 ` Steven Rostedt 2016-06-12 16:58 ` [alsa-devel] " Steven Rostedt 2016-06-12 21:25 ` Henrik Austad 2016-06-12 21:25 ` Henrik Austad 2016-06-12 21:25 ` [alsa-devel] " Henrik Austad 2016-06-13 2:22 ` Steven Rostedt [this message] 2016-06-13 2:22 ` Steven Rostedt 2016-06-13 7:20 ` Henrik Austad 2016-06-13 7:20 ` Henrik Austad 2016-06-11 23:01 ` [very-RFC 7/8] AVB ALSA - Add ALSA shim for TSN Henrik Austad 2016-06-11 23:01 ` Henrik Austad 2016-06-11 23:01 ` [alsa-devel] " Henrik Austad 2016-06-15 11:49 ` Richard Cochran 2016-06-15 11:49 ` [alsa-devel] " Richard Cochran 2016-06-15 12:13 ` Henrik Austad 2016-06-15 12:13 ` [alsa-devel] " Henrik Austad 2016-06-15 12:43 ` Richard Cochran 2016-06-15 12:43 ` Richard Cochran 2016-06-15 12:43 ` [alsa-devel] " Richard Cochran 2016-06-15 12:13 ` Henrik Austad 2016-06-15 11:49 ` Richard Cochran 2016-06-11 23:01 ` [very-RFC 8/8] MAINTAINERS: add TSN/AVB-entries Henrik Austad 2016-06-11 23:01 ` Henrik Austad 2016-06-11 23:01 ` [alsa-devel] " Henrik Austad 2016-06-12 3:38 ` [very-RFC 0/8] TSN driver for the kernel Takashi Sakamoto 2016-06-12 3:38 ` Takashi Sakamoto 2016-06-12 4:30 ` Takashi Sakamoto 2016-06-12 8:31 ` Henrik Austad 2016-06-12 10:43 ` Takashi Sakamoto 2016-06-12 22:06 ` Henrik Austad 2016-06-12 8:28 ` Henrik Austad 2016-06-12 8:28 ` Henrik Austad 2016-06-12 9:14 ` Takashi Sakamoto 2016-06-12 9:14 ` Takashi Sakamoto 2016-06-13 11:47 ` Richard Cochran 2016-06-13 11:47 ` [alsa-devel] " Richard Cochran 2016-06-13 13:00 ` Henrik Austad 2016-06-13 13:00 ` [alsa-devel] " Henrik Austad 2016-06-13 19:32 ` Richard Cochran 2016-06-13 19:32 ` Richard Cochran 2016-06-13 19:32 ` [alsa-devel] " Richard Cochran 2016-06-14 9:30 ` Henrik Austad 2016-06-14 9:30 ` Henrik Austad 2016-06-14 9:30 ` [alsa-devel] " Henrik Austad 2016-06-14 18:26 ` Richard Cochran 2016-06-14 18:26 ` [alsa-devel] " Richard Cochran 2016-06-14 20:38 ` Henrik Austad 2016-06-14 20:38 ` Henrik Austad 2016-06-14 20:38 ` [alsa-devel] " Henrik Austad 2016-06-15 7:04 ` Richard Cochran 2016-06-15 7:04 ` [alsa-devel] " Richard Cochran 2016-06-15 7:50 ` Henrik Austad 2016-06-15 7:50 ` [alsa-devel] " Henrik Austad 2016-06-15 7:50 ` Henrik Austad 2016-06-15 11:41 ` Richard Cochran 2016-06-15 11:41 ` Richard Cochran 2016-06-15 11:41 ` [alsa-devel] " Richard Cochran 2016-06-15 7:04 ` Richard Cochran 2016-06-15 7:11 ` Richard Cochran 2016-06-15 7:11 ` [alsa-devel] " Richard Cochran 2016-06-15 7:11 ` Richard Cochran 2016-06-14 18:26 ` Richard Cochran 2016-06-13 19:37 ` Richard Cochran 2016-06-13 19:37 ` Richard Cochran 2016-06-13 19:37 ` [alsa-devel] " Richard Cochran 2016-06-13 13:00 ` Henrik Austad 2016-06-13 13:12 ` Arnd Bergmann 2016-06-13 13:12 ` Arnd Bergmann 2016-06-13 13:12 ` [alsa-devel] " Arnd Bergmann 2016-06-13 15:56 ` John Fastabend 2016-06-13 15:56 ` John Fastabend 2016-06-13 15:56 ` [alsa-devel] " John Fastabend 2016-06-14 8:35 ` Henrik Austad 2016-06-14 8:35 ` Henrik Austad 2016-06-14 8:35 ` [alsa-devel] " Henrik Austad 2016-06-13 19:51 ` Richard Cochran 2016-06-13 19:51 ` [alsa-devel] " Richard Cochran 2016-06-14 11:18 ` One Thousand Gnomes 2016-06-14 11:18 ` [alsa-devel] " One Thousand Gnomes 2016-06-14 17:04 ` Richard Cochran 2016-06-14 17:04 ` [alsa-devel] " Richard Cochran 2016-06-14 17:04 ` Richard Cochran 2016-06-15 3:15 ` Takashi Sakamoto 2016-06-15 8:06 ` Richard Cochran 2016-06-18 5:22 ` Takashi Sakamoto 2016-06-18 5:22 ` Takashi Sakamoto 2016-06-18 22:45 ` Henrik Austad 2016-06-19 9:46 ` Richard Cochran 2016-06-20 8:05 ` Henrik Austad 2016-06-20 8:05 ` Henrik Austad 2016-06-19 14:45 ` Takashi Sakamoto 2016-06-20 9:06 ` Henrik Austad 2016-06-20 9:06 ` Henrik Austad 2016-06-21 2:22 ` Takashi Sakamoto 2016-06-21 2:22 ` Takashi Sakamoto 2016-06-20 11:08 ` [alsa-devel] " Pierre-Louis Bossart 2016-06-20 11:08 ` Pierre-Louis Bossart 2016-06-20 11:49 ` [alsa-devel] " Henrik Austad 2016-06-20 11:49 ` Henrik Austad 2016-06-20 12:18 ` [alsa-devel] " Richard Cochran 2016-06-20 12:31 ` Richard Cochran 2016-06-20 15:21 ` Richard Cochran 2016-06-20 15:21 ` Richard Cochran 2016-06-21 5:54 ` [alsa-devel] " Takashi Iwai 2016-06-21 5:54 ` Takashi Iwai 2016-06-21 6:38 ` [alsa-devel] " Richard Cochran 2016-06-21 6:38 ` Richard Cochran 2016-06-21 6:45 ` [alsa-devel] " Takashi Iwai 2016-06-21 6:45 ` Takashi Iwai 2016-06-21 17:18 ` [alsa-devel] " Pierre-Louis Bossart 2016-06-21 17:18 ` Pierre-Louis Bossart 2016-06-21 17:45 ` [alsa-devel] " Pierre-Louis Bossart 2016-06-21 19:40 ` Richard Cochran 2016-06-22 12:36 ` Pierre-Louis Bossart 2016-06-22 12:36 ` Pierre-Louis Bossart 2016-06-23 10:38 ` [alsa-devel] " Henrik Austad 2016-06-23 13:28 ` Richard Cochran 2016-06-15 3:27 ` Takashi Sakamoto 2016-06-14 11:18 ` One Thousand Gnomes 2016-06-13 19:51 ` Richard Cochran 2016-06-13 11:47 ` Richard Cochran -- strict thread matches above, loose matches on Subject: below -- 2016-06-11 22:22 Henrik Austad 2016-06-11 22:22 ` [very-RFC 6/8] Add TSN event-tracing Henrik Austad 2016-06-11 22:22 ` Henrik Austad
Reply instructions: You may reply publicly to this message via plain-text email using any one of the following methods: * Save the following mbox file, import it into your mail client, and reply-to-all from there: mbox Avoid top-posting and favor interleaved quoting: https://en.wikipedia.org/wiki/Posting_style#Interleaved_style * Reply using the --to, --cc, and --in-reply-to switches of git-send-email(1): git send-email \ --in-reply-to=20160612222201.00a8aa4c@grimm.local.home \ --to=rostedt@goodmis.org \ --cc=alsa-devel@vger.kernel.org \ --cc=davem@davemloft.net \ --cc=haustad@cisco.com \ --cc=henrik@austad.us \ --cc=linux-kernel@vger.kernel.org \ --cc=linux-media@vger.kernel.org \ --cc=mingo@redhat.com \ --cc=netdev@vger.kernel.org \ /path/to/YOUR_REPLY https://kernel.org/pub/software/scm/git/docs/git-send-email.html * If your mail client supports setting the In-Reply-To header via mailto: links, try the mailto: linkBe sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.