osmo-pcu[master]: add KPI counter to count bytes for RLC and LLC frames
diff mbox

Message ID gerrit.1464693937476.I9a98a5a375d39b3f4990360056c4d6145e755f4d@gerrit.osmocom.org
State New
Headers show

Commit Message

gerrit-no-reply@lists.osmocom.org May 31, 2016, 11:25 a.m. UTC
Review at  https://gerrit.osmocom.org/145

add KPI counter to count bytes for RLC and LLC frames

rlc.dl_bytes		bytes before sending rlc
rlc.dl_payload_bytes	count data w/o LI
rlc.ul_bytes		bytes when received rlc (only valid)
rlc.ul_payload_bytes	count data fragments w/o LI
llc.dl_bytes		complete encapsulated LLC PDUs
llc.ul_bytes		complete received LLC PDUs

Change-Id: I9a98a5a375d39b3f4990360056c4d6145e755f4d
---
M src/bts.cpp
M src/bts.h
M src/gprs_rlcmac_sched.cpp
M src/tbf_dl.cpp
M src/tbf_ul.cpp
5 files changed, 41 insertions(+), 1 deletion(-)


  git pull ssh://gerrit.osmocom.org:29418/osmo-pcu refs/changes/45/145/1

Comments

gerrit-no-reply@lists.osmocom.org May 31, 2016, 11:56 a.m. UTC | #1
Patch Set 1: Code-Review+1
gerrit-no-reply@lists.osmocom.org June 1, 2016, 10:50 a.m. UTC | #2
Patch Set 2:

(3 comments)

https://gerrit.osmocom.org/#/c/145/2//COMMIT_MSG
Commit Message:

Line 9: rlc.dl_bytes		bytes before sending rlc
"before sending"? Doesn't 'dl' mean 'downloaded'?


Line 15: 
it would be good to have this explanation of counter items as comment in the code instead, because it is very helpful and should not be "hidden" in a commit log.


https://gerrit.osmocom.org/#/c/145/2/src/bts.cpp
File src/bts.cpp:

Line 79: 	{ "rlc.ul_payload_bytes",	"RLC UL Payload Bytes "},
I'd prefer if DL and UL were spelled out,
because of limited space I'd shorten 'Bytes' to 'b' instead.
If it's not enough space maybe use 'haul' instead of 'payload'?

Or use 'recvd' and 'sent' instead of DL and UL?
The item just above used 'sent', so actually we should stay with that.

It is about 'sent' and 'received' bytes, isn't it? Or did I get this wrong entirely? Anyway, I'd prefer it spelled out :)
gerrit-no-reply@lists.osmocom.org June 1, 2016, 10:57 a.m. UTC | #3
Patch Set 2:

(2 comments)

https://gerrit.osmocom.org/#/c/145/2//COMMIT_MSG
Commit Message:

Line 9: rlc.dl_bytes		bytes before sending rlc
> "before sending"? Doesn't 'dl' mean 'downloaded'?
In the GSM world, DL is downlink and UL is uplink.


Line 15: 
> it would be good to have this explanation of counter items as comment in th
It belongs into the user manual, which I have also requested lynxis to do.
gerrit-no-reply@lists.osmocom.org June 1, 2016, 1:57 p.m. UTC | #4
Patch Set 2:

(1 comment)

https://gerrit.osmocom.org/#/c/145/2/src/bts.cpp
File src/bts.cpp:

Line 79: 	{ "rlc.ul_payload_bytes",	"RLC UL Payload Bytes "},
> I'd prefer if DL and UL were spelled out,
As indicated, UL and DL have quite fixed meaning inside the cellular world, and we use them in various places. Unlike Rx and Tx, UL and DL are well-defined and don't depend on the point-of-view of the observer. Rx of the MS is different than Rx of the PCU.  PCU Rx from Um interface is different direction than Rx frm SGSN on Gb interface, so that's very confusing. UL and DL leave no room for (mis)interpretation.

Patch
diff mbox

diff --git a/src/bts.cpp b/src/bts.cpp
index 953ac4d..9e91b87 100644
--- a/src/bts.cpp
+++ b/src/bts.cpp
@@ -73,6 +73,10 @@ 
 	{ "rlc.late-block",		"RLC Late Block       "},
 	{ "rlc.sent-dummy",		"RLC Sent Dummy       "},
 	{ "rlc.sent-control",		"RLC Sent Control     "},
+	{ "rlc.dl_bytes",		"RLC DL Bytes         "},
+	{ "rlc.dl_payload_bytes",	"RLC DL Payload Bytes "},
+	{ "rlc.ul_bytes",		"RLC UL Bytes         "},
+	{ "rlc.ul_payload_bytes",	"RLC UL Payload Bytes "},
 	{ "decode.errors",		"Decode Errors        "},
 	{ "sba.allocated",		"SBA Allocated        "},
 	{ "sba.freed",			"SBA Freed            "},
@@ -80,6 +84,8 @@ 
 	{ "llc.timeout",		"Timedout Frames      "},
 	{ "llc.dropped",		"Dropped Frames       "},
 	{ "llc.scheduled",		"Scheduled Frames     "},
+	{ "llc.dl_bytes",               "RLC encapsulated PDUs"},
+	{ "llc.ul_bytes",               "full PDUs received   "},
 	{ "rach.requests",		"RACH requests        "},
 };
 
@@ -1302,6 +1308,8 @@ 
 		return -EINVAL;
 	}
 
+	bts()->rlc_ul_bytes(len);
+
 	LOGP(DRLCMACUL, LOGL_DEBUG, "Got RLC block, coding scheme: %s, "
 		"length: %d (%d))\n", cs.name(), len, cs.usedSizeUL());
 
diff --git a/src/bts.h b/src/bts.h
index 35f24d1..a713c46 100644
--- a/src/bts.h
+++ b/src/bts.h
@@ -235,6 +235,10 @@ 
 		CTR_RLC_LATE_BLOCK,
 		CTR_RLC_SENT_DUMMY,
 		CTR_RLC_SENT_CONTROL,
+		CTR_RLC_DL_BYTES,
+		CTR_RLC_DL_PAYLOAD_BYTES,
+		CTR_RLC_UL_BYTES,
+		CTR_RLC_UL_PAYLOAD_BYTES,
 		CTR_DECODE_ERRORS,
 		CTR_SBA_ALLOCATED,
 		CTR_SBA_FREED,
@@ -242,6 +246,8 @@ 
 		CTR_LLC_FRAME_TIMEDOUT,
 		CTR_LLC_FRAME_DROPPED,
 		CTR_LLC_FRAME_SCHED,
+		CTR_LLC_DL_BYTES,
+		CTR_LLC_UL_BYTES,
 		CTR_RACH_REQUESTS,
 	};
 
@@ -313,6 +319,10 @@ 
 	void rlc_late_block();
 	void rlc_sent_dummy();
 	void rlc_sent_control();
+	void rlc_dl_bytes(int bytes);
+	void rlc_dl_payload_bytes(int bytes);
+	void rlc_ul_bytes(int bytes);
+	void rlc_ul_payload_bytes(int bytes);
 	void decode_error();
 	void sba_allocated();
 	void sba_freed();
@@ -320,6 +330,8 @@ 
 	void llc_timedout_frame();
 	void llc_dropped_frame();
 	void llc_frame_sched();
+	void llc_dl_bytes(int bytes);
+	void llc_ul_bytes(int bytes);
 	void rach_frame();
 
 	void ms_present(int32_t n);
@@ -427,6 +439,11 @@ 
 	return m_statg;
 }
 
+#define CREATE_COUNT_ADD_INLINE(func_name, ctr_name) \
+	inline void BTS::func_name(int inc) {\
+		rate_ctr_add(&m_ratectrs->ctr[ctr_name], inc); \
+	}
+
 #define CREATE_COUNT_INLINE(func_name, ctr_name) \
 	inline void BTS::func_name() {\
 		rate_ctr_inc(&m_ratectrs->ctr[ctr_name]); \
@@ -455,6 +472,10 @@ 
 CREATE_COUNT_INLINE(rlc_late_block, CTR_RLC_LATE_BLOCK);
 CREATE_COUNT_INLINE(rlc_sent_dummy, CTR_RLC_SENT_DUMMY);
 CREATE_COUNT_INLINE(rlc_sent_control, CTR_RLC_SENT_CONTROL);
+CREATE_COUNT_ADD_INLINE(rlc_dl_bytes, CTR_RLC_DL_BYTES);
+CREATE_COUNT_ADD_INLINE(rlc_dl_payload_bytes, CTR_RLC_DL_PAYLOAD_BYTES);
+CREATE_COUNT_ADD_INLINE(rlc_ul_bytes, CTR_RLC_UL_BYTES);
+CREATE_COUNT_ADD_INLINE(rlc_ul_payload_bytes, CTR_RLC_UL_PAYLOAD_BYTES);
 CREATE_COUNT_INLINE(decode_error, CTR_DECODE_ERRORS)
 CREATE_COUNT_INLINE(sba_allocated, CTR_SBA_ALLOCATED)
 CREATE_COUNT_INLINE(sba_freed, CTR_SBA_FREED)
@@ -462,6 +483,8 @@ 
 CREATE_COUNT_INLINE(llc_timedout_frame, CTR_LLC_FRAME_TIMEDOUT);
 CREATE_COUNT_INLINE(llc_dropped_frame, CTR_LLC_FRAME_DROPPED);
 CREATE_COUNT_INLINE(llc_frame_sched, CTR_LLC_FRAME_SCHED);
+CREATE_COUNT_ADD_INLINE(llc_dl_bytes, CTR_LLC_DL_BYTES);
+CREATE_COUNT_ADD_INLINE(llc_ul_bytes, CTR_LLC_UL_BYTES);
 CREATE_COUNT_INLINE(rach_frame, CTR_RACH_REQUESTS);
 
 #undef CREATE_COUNT_INLINE
diff --git a/src/gprs_rlcmac_sched.cpp b/src/gprs_rlcmac_sched.cpp
index fce3aaf..0367ad0 100644
--- a/src/gprs_rlcmac_sched.cpp
+++ b/src/gprs_rlcmac_sched.cpp
@@ -360,6 +360,7 @@ 
 	if (!msg)
 		return -ENOMEM;
 	/* msg is now available */
+	bts->bts->rlc_dl_bytes(msg->data_len);
 
 	/* set USF */
 	OSMO_ASSERT(msgb_length(msg) > 0);
diff --git a/src/tbf_dl.cpp b/src/tbf_dl.cpp
index 66f747b..5931676 100644
--- a/src/tbf_dl.cpp
+++ b/src/tbf_dl.cpp
@@ -513,6 +513,7 @@ 
 
 	do {
 		bool is_final;
+		int payload_written = 0;
 
 		if (m_llc.frame_length() == 0) {
 			/* nothing to sent - delay the release of the TBF */
@@ -540,7 +541,10 @@ 
 		is_final = llc_queue()->size() == 0 && !keep_open(fn);
 
 		ar = Encoding::rlc_data_to_dl_append(rdbi, cs,
-			&m_llc, &write_offset, &num_chunks, data, is_final, NULL);
+			&m_llc, &write_offset, &num_chunks, data, is_final, &payload_written);
+
+		if (payload_written > 0)
+			bts->rlc_dl_payload_bytes(payload_written);
 
 		if (ar == Encoding::AR_NEED_MORE_BLOCKS)
 			break;
@@ -548,6 +552,7 @@ 
 		LOGP(DRLCMACDL, LOGL_INFO, "Complete DL frame for %s"
 			"len=%d\n", tbf_name(this), m_llc.frame_length());
 		gprs_rlcmac_dl_bw(this, m_llc.frame_length());
+		bts->llc_dl_bytes(m_llc.frame_length());
 		m_llc.reset();
 
 		if (is_final) {
diff --git a/src/tbf_ul.cpp b/src/tbf_ul.cpp
index e7c64ad..98962d1 100644
--- a/src/tbf_ul.cpp
+++ b/src/tbf_ul.cpp
@@ -67,6 +67,8 @@ 
 	for (i = 0; i < num_frames; i++) {
 		frame = frames + i;
 
+		bts->rlc_ul_payload_bytes(frame->length);
+
 		LOGP(DRLCMACUL, LOGL_DEBUG, "-- Frame %d starts at offset %d, "
 			"length=%d, is_complete=%d\n",
 			i + 1, frame->offset, frame->length, frame->is_complete);
@@ -79,6 +81,7 @@ 
 			LOGP(DRLCMACUL, LOGL_INFO, "%s complete UL frame len=%d\n",
 				tbf_name(this) , m_llc.frame_length());
 			snd_ul_ud();
+			bts->llc_ul_bytes(m_llc.frame_length());
 			m_llc.reset();
 		}
 	}