[PATCH v2] ath10k: handle ieee80211 header and payload tracing separately

[PATCH v2] ath10k: handle ieee80211 header and payload tracing separately

[Rajkumar Manoharan]

For packet log, the transmitted frame 802.11 header alone is sufficient.
Recording entire packet is also consuming lot of disk space. To optimize
this, tx and rx data tracepoints are splitted into header and payload
tracepoints.

To record tx ieee80211 headers

     trace-cmd record -e ath10k_tx_hdr

To record complete packets

     trace-cmd record -e ath10k_tx_hdr -e ath10k_tx_payload

Cc: Michal Kazior <michal.kazior@tieto.com>
Signed-off-by: Rajkumar Manoharan <rmanohar@qti.qualcomm.com>
---
v2: Move header len processing inside trace
 drivers/net/wireless/ath/ath10k/htt_rx.c |  4 +-
 drivers/net/wireless/ath/ath10k/htt_tx.c |  3 +-
 drivers/net/wireless/ath/ath10k/trace.h  | 77 +++++++++++++++++++++++++++-----
 drivers/net/wireless/ath/ath10k/wmi.c    |  7 ++-
 4 files changed, 75 insertions(+), 16 deletions(-)

diff --git a/drivers/net/wireless/ath/ath10k/htt_rx.c b/drivers/net/wireless/ath/ath10k/htt_rx.c
index a691fdf..52c6306 100644
--- a/drivers/net/wireless/ath/ath10k/htt_rx.c
+++ b/drivers/net/wireless/ath/ath10k/htt_rx.c
@@ -297,8 +297,6 @@ static inline struct sk_buff *ath10k_htt_rx_netbuf_pop(struct ath10k_htt *htt)
 			 DMA_FROM_DEVICE);
 	ath10k_dbg_dump(ar, ATH10K_DBG_HTT_DUMP, NULL, "htt rx netbuf pop: ",
 			msdu->data, msdu->len + skb_tailroom(msdu));
-	trace_ath10k_htt_rx_pop_msdu(ar, msdu->data, msdu->len +
-				     skb_tailroom(msdu));
 
 	return msdu;
 }
@@ -903,6 +901,8 @@ static void ath10k_process_rx(struct ath10k *ar,
 		   !!(status->flag & RX_FLAG_AMSDU_MORE));
 	ath10k_dbg_dump(ar, ATH10K_DBG_HTT_DUMP, NULL, "rx skb: ",
 			skb->data, skb->len);
+	trace_ath10k_rx_hdr(ar, skb->data, skb->len);
+	trace_ath10k_rx_payload(ar, skb->data, skb->len);
 
 	ieee80211_rx(ar->hw, skb);
 }
diff --git a/drivers/net/wireless/ath/ath10k/htt_tx.c b/drivers/net/wireless/ath/ath10k/htt_tx.c
index b0df470..1e51249 100644
--- a/drivers/net/wireless/ath/ath10k/htt_tx.c
+++ b/drivers/net/wireless/ath/ath10k/htt_tx.c
@@ -564,7 +564,8 @@ int ath10k_htt_tx(struct ath10k_htt *htt, struct sk_buff *msdu)
 		   (u32)skb_cb->paddr, vdev_id, tid);
 	ath10k_dbg_dump(ar, ATH10K_DBG_HTT_DUMP, NULL, "htt tx msdu: ",
 			msdu->data, msdu->len);
-	trace_ath10k_htt_tx_msdu(ar, msdu->data, msdu->len);
+	trace_ath10k_tx_hdr(ar, msdu->data, msdu->len);
+	trace_ath10k_tx_payload(ar, msdu->data, msdu->len);
 
 	sg_items[0].transfer_id = 0;
 	sg_items[0].transfer_context = NULL;
diff --git a/drivers/net/wireless/ath/ath10k/trace.h b/drivers/net/wireless/ath/ath10k/trace.h
index b9a2ba6..919dc7e 100644
--- a/drivers/net/wireless/ath/ath10k/trace.h
+++ b/drivers/net/wireless/ath/ath10k/trace.h
@@ -341,7 +341,10 @@ TRACE_EVENT(ath10k_txrx_tx_unref,
 	 )
 );
 
-DECLARE_EVENT_CLASS(ath10k_data_event,
+#define ATH10K_FRM_HDR_LEN  \
+	ieee80211_hdrlen(((struct ieee80211_hdr *)data)->frame_control)
+
+DECLARE_EVENT_CLASS(ath10k_hdr_event,
 		    TP_PROTO(struct ath10k *ar, void *data, size_t len),
 
 	TP_ARGS(ar, data, len),
@@ -350,14 +353,14 @@ DECLARE_EVENT_CLASS(ath10k_data_event,
 		__string(device, dev_name(ar->dev))
 		__string(driver, dev_driver_string(ar->dev))
 		__field(size_t, len)
-		__dynamic_array(u8, data, len)
+		__dynamic_array(u8, data, ATH10K_FRM_HDR_LEN)
 	),
 
 	TP_fast_assign(
 		__assign_str(device, dev_name(ar->dev));
 		__assign_str(driver, dev_driver_string(ar->dev));
-		__entry->len = len;
-		memcpy(__get_dynamic_array(data), data, len);
+		__entry->len = ATH10K_FRM_HDR_LEN;
+		memcpy(__get_dynamic_array(data), data, __entry->len);
 	),
 
 	TP_printk(
@@ -368,30 +371,82 @@ DECLARE_EVENT_CLASS(ath10k_data_event,
 	)
 );
 
-DEFINE_EVENT(ath10k_data_event, ath10k_htt_tx_msdu,
-	     TP_PROTO(struct ath10k *ar, void *data, size_t len),
-	     TP_ARGS(ar, data, len)
+DECLARE_EVENT_CLASS(ath10k_payload_event,
+		    TP_PROTO(struct ath10k *ar, void *data, size_t len),
+
+	TP_ARGS(ar, data, len),
+
+	TP_STRUCT__entry(
+		__string(device, dev_name(ar->dev))
+		__string(driver, dev_driver_string(ar->dev))
+		__field(size_t, len)
+		__dynamic_array(u8, payload, (len - ATH10K_FRM_HDR_LEN))
+	),
+
+	TP_fast_assign(
+		__assign_str(device, dev_name(ar->dev));
+		__assign_str(driver, dev_driver_string(ar->dev));
+		__entry->len = len - ATH10K_FRM_HDR_LEN;
+		memcpy(__get_dynamic_array(payload),
+		       data + ATH10K_FRM_HDR_LEN, __entry->len);
+	),
+
+	TP_printk(
+		"%s %s len %zu\n",
+		__get_str(driver),
+		__get_str(device),
+		__entry->len
+	)
 );
 
-DEFINE_EVENT(ath10k_data_event, ath10k_htt_rx_pop_msdu,
+
+DEFINE_EVENT(ath10k_hdr_event, ath10k_tx_hdr,
 	     TP_PROTO(struct ath10k *ar, void *data, size_t len),
 	     TP_ARGS(ar, data, len)
 );
 
-DEFINE_EVENT(ath10k_data_event, ath10k_wmi_mgmt_tx,
+DEFINE_EVENT(ath10k_payload_event, ath10k_tx_payload,
 	     TP_PROTO(struct ath10k *ar, void *data, size_t len),
 	     TP_ARGS(ar, data, len)
 );
 
-DEFINE_EVENT(ath10k_data_event, ath10k_wmi_bcn_tx,
+DEFINE_EVENT(ath10k_hdr_event, ath10k_rx_hdr,
 	     TP_PROTO(struct ath10k *ar, void *data, size_t len),
 	     TP_ARGS(ar, data, len)
 );
 
-DEFINE_EVENT(ath10k_data_event, ath10k_htt_rx_desc,
+DEFINE_EVENT(ath10k_payload_event, ath10k_rx_payload,
 	     TP_PROTO(struct ath10k *ar, void *data, size_t len),
 	     TP_ARGS(ar, data, len)
 );
+
+TRACE_EVENT(ath10k_htt_rx_desc,
+	    TP_PROTO(struct ath10k *ar, void *data, size_t len),
+
+	TP_ARGS(ar, data, len),
+
+	TP_STRUCT__entry(
+		__string(device, dev_name(ar->dev))
+		__string(driver, dev_driver_string(ar->dev))
+		__field(u16, len)
+		__dynamic_array(u8, rxdesc, len)
+	),
+
+	TP_fast_assign(
+		__assign_str(device, dev_name(ar->dev));
+		__assign_str(driver, dev_driver_string(ar->dev));
+		__entry->len = len;
+		memcpy(__get_dynamic_array(rxdesc), data, len);
+	),
+
+	TP_printk(
+		"%s %s rxdesc len %d",
+		__get_str(driver),
+		__get_str(device),
+		__entry->len
+	 )
+);
+
 #endif /* _TRACE_H_ || TRACE_HEADER_MULTI_READ*/
 
 /* we don't want to use include/trace/events */
diff --git a/drivers/net/wireless/ath/ath10k/wmi.c b/drivers/net/wireless/ath/ath10k/wmi.c
index ae746ce..3dcb216 100644
--- a/drivers/net/wireless/ath/ath10k/wmi.c
+++ b/drivers/net/wireless/ath/ath10k/wmi.c
@@ -834,7 +834,8 @@ int ath10k_wmi_mgmt_tx(struct ath10k *ar, struct sk_buff *skb)
 	ath10k_dbg(ar, ATH10K_DBG_WMI, "wmi mgmt tx skb %p len %d ftype %02x stype %02x\n",
 		   wmi_skb, wmi_skb->len, fc & IEEE80211_FCTL_FTYPE,
 		   fc & IEEE80211_FCTL_STYPE);
-	trace_ath10k_wmi_mgmt_tx(ar, skb->data, skb->len);
+	trace_ath10k_tx_hdr(ar, skb->data, skb->len);
+	trace_ath10k_tx_payload(ar, skb->data, skb->len);
 
 	/* Send the management frame buffer to the target */
 	ret = ath10k_wmi_cmd_send(ar, wmi_skb, ar->wmi.cmd->mgmt_tx_cmdid);
@@ -1893,7 +1894,9 @@ static void ath10k_wmi_event_host_swba(struct ath10k *ar, struct sk_buff *skb)
 		arvif->beacon = bcn;
 		arvif->beacon_sent = false;
 
-		trace_ath10k_wmi_bcn_tx(ar, bcn->data, bcn->len);
+		trace_ath10k_tx_hdr(ar, bcn->data, bcn->len);
+		trace_ath10k_tx_payload(ar, bcn->data, bcn->len);
+
 		ath10k_wmi_tx_beacon_nowait(arvif);
 skip:
 		spin_unlock_bh(&ar->data_lock);
-- 
2.1.3

Re: [PATCH v2] ath10k: handle ieee80211 header and payload tracing separately

[Kalle Valo]

Rajkumar Manoharan <rmanohar@qti.qualcomm.com> writes:

> For packet log, the transmitted frame 802.11 header alone is sufficient.
> Recording entire packet is also consuming lot of disk space. To optimize
> this, tx and rx data tracepoints are splitted into header and payload
> tracepoints.
>
> To record tx ieee80211 headers
>
>      trace-cmd record -e ath10k_tx_hdr
>
> To record complete packets
>
>      trace-cmd record -e ath10k_tx_hdr -e ath10k_tx_payload
>
> Cc: Michal Kazior <michal.kazior@tieto.com>
> Signed-off-by: Rajkumar Manoharan <rmanohar@qti.qualcomm.com>

[...]

> --- a/drivers/net/wireless/ath/ath10k/trace.h
> +++ b/drivers/net/wireless/ath/ath10k/trace.h
> @@ -341,7 +341,10 @@ TRACE_EVENT(ath10k_txrx_tx_unref,
>  	 )
>  );
>  
> -DECLARE_EVENT_CLASS(ath10k_data_event,
> +#define ATH10K_FRM_HDR_LEN  \
> +	ieee80211_hdrlen(((struct ieee80211_hdr *)data)->frame_control)

This macro does not look good. I would recommend to follow what Johannes
suggested:

"It would be worth hiding that inside the tracepoint's assign function,
so instead of passing data/len here you'd pass the full skb, or the full
skb data/skb len, like this:

ar, skb->data, skb->len

to both tracers. Then inside the tracer you can do the hdrlen check, and
that way move the code into the tracing so it's not hit when tracing is
disabled."

-- 
Kalle Valo

Re: [PATCH v2] ath10k: handle ieee80211 header and payload tracing separately

[Rajkumar Manoharan]

On Tue, Nov 04, 2014 at 01:34:37AM +0200, Kalle Valo wrote:
> Rajkumar Manoharan <rmanohar@qti.qualcomm.com> writes:
> 
> > For packet log, the transmitted frame 802.11 header alone is sufficient.
> > Recording entire packet is also consuming lot of disk space. To optimize
> > this, tx and rx data tracepoints are splitted into header and payload
> > tracepoints.
> >  
> > -DECLARE_EVENT_CLASS(ath10k_data_event,
> > +#define ATH10K_FRM_HDR_LEN  \
> > +	ieee80211_hdrlen(((struct ieee80211_hdr *)data)->frame_control)
> 
> This macro does not look good. I would recommend to follow what Johannes
> suggested:
>
> "It would be worth hiding that inside the tracepoint's assign function,
> so instead of passing data/len here you'd pass the full skb, or the full
> skb data/skb len, like this:
> 
> ar, skb->data, skb->len
> 
> to both tracers. Then inside the tracer you can do the hdrlen check, and
> that way move the code into the tracing so it's not hit when tracing is
> disabled."
v2 does the same. tracing functions just take ar, skb->data and skb->len.
header check is handled inside tracing funtions.

I do not understand your concerns. :(

-Rajkumar

Re: [PATCH v2] ath10k: handle ieee80211 header and payload tracing separately

[Kalle Valo]

Rajkumar Manoharan <rmanohar@qti.qualcomm.com> writes:

> On Tue, Nov 04, 2014 at 01:34:37AM +0200, Kalle Valo wrote:
>> Rajkumar Manoharan <rmanohar@qti.qualcomm.com> writes:
>> 
>> > For packet log, the transmitted frame 802.11 header alone is sufficient.
>> > Recording entire packet is also consuming lot of disk space. To optimize
>> > this, tx and rx data tracepoints are splitted into header and payload
>> > tracepoints.
>> >  
>> > -DECLARE_EVENT_CLASS(ath10k_data_event,
>> > +#define ATH10K_FRM_HDR_LEN  \
>> > +	ieee80211_hdrlen(((struct ieee80211_hdr *)data)->frame_control)
>> 
>> This macro does not look good. I would recommend to follow what Johannes
>> suggested:
>>
>> "It would be worth hiding that inside the tracepoint's assign function,
>> so instead of passing data/len here you'd pass the full skb, or the full
>> skb data/skb len, like this:
>> 
>> ar, skb->data, skb->len
>> 
>> to both tracers. Then inside the tracer you can do the hdrlen check, and
>> that way move the code into the tracing so it's not hit when tracing is
>> disabled."
>
> v2 does the same. tracing functions just take ar, skb->data and skb->len.
> header check is handled inside tracing funtions.
>
> I do not understand your concerns. :(

Sorry, I was confusing. I meant that wouldn't it be better to pass the
skb pointer instead of skb-data and skb->len? I understood that was what
Johannes suggested.

But ATH10K_FRM_HDR_LEN is the problem. To simplify this, what if you
just always send the first 30 bytes (or whatever would be the max header
length)? Also should you check that skb->len is at least the length
defined by ieee80211_hdrlen()?

-- 
Kalle Valo

Re: [PATCH v2] ath10k: handle ieee80211 header and payload tracing separately

[Rajkumar Manoharan]

On Wed, Nov 05, 2014 at 03:29:13AM +0200, Kalle Valo wrote:
> Rajkumar Manoharan <rmanohar@qti.qualcomm.com> writes:
> 
> > On Tue, Nov 04, 2014 at 01:34:37AM +0200, Kalle Valo wrote:
> >> Rajkumar Manoharan <rmanohar@qti.qualcomm.com> writes:
> >> 
> >> > -DECLARE_EVENT_CLASS(ath10k_data_event,
> >> > +#define ATH10K_FRM_HDR_LEN  \
> >> > +	ieee80211_hdrlen(((struct ieee80211_hdr *)data)->frame_control)
> >> 
> >> This macro does not look good. I would recommend to follow what Johannes
> >> suggested:
> >>
> >
> > v2 does the same. tracing functions just take ar, skb->data and skb->len.
> > header check is handled inside tracing funtions.
> >
> > I do not understand your concerns. :(
> 
> Sorry, I was confusing. I meant that wouldn't it be better to pass the
> skb pointer instead of skb-data and skb->len? I understood that was what
> Johannes suggested.
> 
> But ATH10K_FRM_HDR_LEN is the problem. To simplify this, what if you
> just always send the first 30 bytes (or whatever would be the max header
> length)? Also should you check that skb->len is at least the length
> defined by ieee80211_hdrlen()?
>
The goal of header & payload split is to reduce the amount of data being
collected. Sending maximum header size always will unnecessarily
increase the trace.dat size as hdr_size for mgmt and beacon always be 24.
This will also increase tracing traffic when it is being collected in
remote machine.

since tracepoints are being called only after the preprocessing,
skb->len never be lesser than 802.11 header size.

The macro is defined to ease readability and reduce # of line changes.
I originally thought of naming the macro as IEEE80211_HDR_LEN. But such
definition does not look correct inside driver.

-Rajkumar

Re: [PATCH v2] ath10k: handle ieee80211 header and payload tracing separately

[Johannes Berg]

On Wed, 2014-11-05 at 15:14 +0530, Rajkumar Manoharan wrote:

> The macro is defined to ease readability and reduce # of line changes.
> I originally thought of naming the macro as IEEE80211_HDR_LEN. But such
> definition does not look correct inside driver.

FWIW, I pointed Kalle to the macro on IRC - IMHO you should pass
arguments to the macro (and probably even make it an inline function)
rather than having it assume certain variables exist when the macro is
used.

Kalle may have misunderstood what I was trying to say :)

johannes

Re: [PATCH v2] ath10k: handle ieee80211 header and payload tracing separately

[Rajkumar Manoharan]

On Wed, Nov 05, 2014 at 11:12:01AM +0100, Johannes Berg wrote:
> On Wed, 2014-11-05 at 15:14 +0530, Rajkumar Manoharan wrote:
> 
> > The macro is defined to ease readability and reduce # of line changes.
> > I originally thought of naming the macro as IEEE80211_HDR_LEN. But such
> > definition does not look correct inside driver.
> 
> FWIW, I pointed Kalle to the macro on IRC - IMHO you should pass
> arguments to the macro (and probably even make it an inline function)
> rather than having it assume certain variables exist when the macro is
> used.
> 
> Kalle may have misunderstood what I was trying to say :)
> 
Sounds clear..Thanks for pointing out. Will change it as inline funtion..

-Rajkumar