22 Mar
2016
22 Mar
'16
2:12 p.m.
Parsing the uplink data header for GPRS and EGPRS header type 3
is handled in separate functions.
This patch will enhance modularity of the code.
---
src/decoding.cpp | 140 ++++++++++++++++++++++++++++++-------------------------
src/decoding.h | 9 +++-
2 files changed, 84 insertions(+), 65 deletions(-)
diff --git a/src/decoding.cpp b/src/decoding.cpp
index f2b548c..0c81b2a 100644
--- a/src/decoding.cpp
+++ b/src/decoding.cpp
@@ -344,74 +344,14 @@ void Decoding::extract_rbb(const struct bitvec *rbb, char
*show_rbb)
int Decoding::rlc_parse_ul_data_header(struct gprs_rlc_data_info *rlc,
const uint8_t *data, GprsCodingScheme cs)
{
- const struct gprs_rlc_ul_header_egprs_3 *egprs3;
- const struct rlc_ul_header *gprs;
- unsigned int e_ti_header;
unsigned int cur_bit = 0;
- int punct, punct2, with_padding, cps;
- unsigned int offs;
-
switch(cs.headerTypeData()) {
- case GprsCodingScheme::HEADER_GPRS_DATA:
- gprs = static_cast<struct rlc_ul_header *>
- ((void *)data);
-
- gprs_rlc_data_info_init_ul(rlc, cs, false);
-
- rlc->r = gprs->r;
- rlc->si = gprs->si;
- rlc->tfi = gprs->tfi;
- rlc->cps = 0;
- rlc->rsb = 0;
-
- rlc->num_data_blocks = 1;
- rlc->block_info[0].cv = gprs->cv;
- rlc->block_info[0].pi = gprs->pi;
- rlc->block_info[0].bsn = gprs->bsn;
- rlc->block_info[0].e = gprs->e;
- rlc->block_info[0].ti = gprs->ti;
- rlc->block_info[0].spb = 0;
-
- cur_bit += rlc->data_offs_bits[0];
-
- /* skip data area */
- cur_bit += cs.maxDataBlockBytes() * 8;
+ case GprsCodingScheme::HEADER_GPRS_DATA :
+ cur_bit = rlc_parse_ul_data_header_gprs(rlc, data, cs);
break;
- case GprsCodingScheme::HEADER_EGPRS_DATA_TYPE_3:
- egprs3 = static_cast<struct gprs_rlc_ul_header_egprs_3 *>
- ((void *)data);
-
- cps = (egprs3->cps_a << 0) | (egprs3->cps_b << 2);
- gprs_rlc_mcs_cps_decode(cps, cs, &punct, &punct2, &with_padding);
- gprs_rlc_data_info_init_ul(rlc, cs, with_padding);
-
- rlc->r = egprs3->r;
- rlc->si = egprs3->si;
- rlc->tfi = (egprs3->tfi_a << 0) | (egprs3->tfi_b << 2);
- rlc->cps = cps;
- rlc->rsb = egprs3->rsb;
-
- rlc->num_data_blocks = 1;
- rlc->block_info[0].cv = egprs3->cv;
- rlc->block_info[0].pi = egprs3->pi;
- rlc->block_info[0].spb = egprs3->spb;
- rlc->block_info[0].bsn =
- (egprs3->bsn1_a << 0) | (egprs3->bsn1_b << 5);
-
- cur_bit += rlc->data_offs_bits[0] - 2;
-
- offs = rlc->data_offs_bits[0] / 8;
- OSMO_ASSERT(rlc->data_offs_bits[0] % 8 == 1);
-
- e_ti_header = (data[offs-1] + (data[offs] << 8)) >> 7;
- rlc->block_info[0].e = !!(e_ti_header & 0x01);
- rlc->block_info[0].ti = !!(e_ti_header & 0x02);
- cur_bit += 2;
-
- /* skip data area */
- cur_bit += cs.maxDataBlockBytes() * 8;
+ case GprsCodingScheme::HEADER_EGPRS_DATA_TYPE_3 :
+ cur_bit = rlc_parse_ul_data_header_egprs_type_3(rlc, data, cs);
break;
-
case GprsCodingScheme::HEADER_EGPRS_DATA_TYPE_1:
case GprsCodingScheme::HEADER_EGPRS_DATA_TYPE_2:
/* TODO: Support both header types */
@@ -426,6 +366,78 @@ int Decoding::rlc_parse_ul_data_header(struct gprs_rlc_data_info
*rlc,
return cur_bit;
}
+int Decoding::rlc_parse_ul_data_header_egprs_type_3(
+ struct gprs_rlc_data_info *rlc,
+ const uint8_t *data,
+ const GprsCodingScheme &cs)
+{
+ int punct, punct2, with_padding, cps;
+ unsigned int e_ti_header, offs, cur_bit = 0;
+ const struct gprs_rlc_ul_header_egprs_3 *egprs3;
+
+ egprs3 = static_cast < struct gprs_rlc_ul_header_egprs_3 * >
+ ((void *)data);
+
+ cps = (egprs3->cps_a << 0) | (egprs3->cps_b << 2);
+ gprs_rlc_mcs_cps_decode(cps, cs, &punct, &punct2, &with_padding);
+ gprs_rlc_data_info_init_ul(rlc, cs, with_padding);
+
+ rlc->r = egprs3->r;
+ rlc->si = egprs3->si;
+ rlc->tfi = (egprs3->tfi_a << 0) | (egprs3->tfi_b << 2);
+ rlc->cps = cps;
+ rlc->rsb = egprs3->rsb;
+
+ rlc->num_data_blocks = 1;
+ rlc->block_info[0].cv = egprs3->cv;
+ rlc->block_info[0].pi = egprs3->pi;
+ rlc->block_info[0].spb = egprs3->spb;
+ rlc->block_info[0].bsn =
+ (egprs3->bsn1_a << 0) | (egprs3->bsn1_b << 5);
+
+ cur_bit += rlc->data_offs_bits[0] - 2;
+ offs = rlc->data_offs_bits[0] / 8;
+ OSMO_ASSERT(rlc->data_offs_bits[0] % 8 == 1);
+ e_ti_header = (data[offs-1] + (data[offs] << 8)) >> 7;
+ rlc->block_info[0].e = !!(e_ti_header & 0x01);
+ rlc->block_info[0].ti = !!(e_ti_header & 0x02);
+ cur_bit += 2;
+ /* skip data area */
+ cur_bit += cs.maxDataBlockBytes() * 8;
+
+ return cur_bit;
+}
+
+int Decoding::rlc_parse_ul_data_header_gprs(struct gprs_rlc_data_info *rlc,
+ const uint8_t *data, const GprsCodingScheme &cs)
+{
+ const struct rlc_ul_header *gprs;
+ unsigned int cur_bit = 0;
+
+ gprs = static_cast < struct rlc_ul_header * >
+ ((void *)data);
+
+ gprs_rlc_data_info_init_ul(rlc, cs, false);
+
+ rlc->r = gprs->r;
+ rlc->si = gprs->si;
+ rlc->tfi = gprs->tfi;
+ rlc->cps = 0;
+ rlc->rsb = 0;
+ rlc->num_data_blocks = 1;
+ rlc->block_info[0].cv = gprs->cv;
+ rlc->block_info[0].pi = gprs->pi;
+ rlc->block_info[0].bsn = gprs->bsn;
+ rlc->block_info[0].e = gprs->e;
+ rlc->block_info[0].ti = gprs->ti;
+ rlc->block_info[0].spb = 0;
+ cur_bit += rlc->data_offs_bits[0];
+ /* skip data area */
+ cur_bit += cs.maxDataBlockBytes() * 8;
+
+ return cur_bit;
+}
+
/**
* \brief Copy LSB bitstream RLC data block to byte aligned buffer.
*
diff --git a/src/decoding.h b/src/decoding.h
index 58ecd18..1043d67 100644
--- a/src/decoding.h
+++ b/src/decoding.h
@@ -43,7 +43,14 @@ public:
static void extract_rbb(const uint8_t *rbb, char *extracted_rbb);
static void extract_rbb(const struct bitvec *rbb, char *show_rbb);
-
+ static int rlc_parse_ul_data_header_egprs_type_3(
+ struct gprs_rlc_data_info *rlc,
+ const uint8_t *data,
+ const GprsCodingScheme &cs);
+ static int rlc_parse_ul_data_header_gprs(
+ struct gprs_rlc_data_info *rlc,
+ const uint8_t *data,
+ const GprsCodingScheme &cs);
static int rlc_parse_ul_data_header(struct gprs_rlc_data_info *rlc,
const uint8_t *data, GprsCodingScheme cs);
static unsigned int rlc_copy_to_aligned_buffer(
--
2.5.0
22 Mar
22 Mar
2:12 p.m.
New subject: [PATCH 2/2] Avoid copy constructor in uplink tbf path
The copy constructor of GprsCodingScheme class object used in
uplink tbf path is replaced by the reference variable.
Appropriate function prototypes and function definition are
changed.
This patch will increase efficiency of the code.
---
src/bts.cpp | 4 ++--
src/bts.h | 4 ++--
src/decoding.cpp | 4 ++--
src/decoding.h | 4 ++--
src/gprs_coding_scheme.cpp | 4 ++--
src/gprs_coding_scheme.h | 24 ++++++++++++------------
6 files changed, 22 insertions(+), 22 deletions(-)
diff --git a/src/bts.cpp b/src/bts.cpp
index 715fb51..412d502 100644
--- a/src/bts.cpp
+++ b/src/bts.cpp
@@ -1316,7 +1316,7 @@ int gprs_rlcmac_pdch::rcv_block(uint8_t *data, uint8_t len, uint32_t
fn,
}
int gprs_rlcmac_pdch::rcv_data_block(uint8_t *data, uint32_t fn,
- struct pcu_l1_meas *meas, GprsCodingScheme cs)
+ struct pcu_l1_meas *meas, const GprsCodingScheme &cs)
{
int rc;
struct gprs_rlc_data_info rlc_dec;
@@ -1375,7 +1375,7 @@ int gprs_rlcmac_pdch::rcv_data_block(uint8_t *data, uint32_t fn,
}
int gprs_rlcmac_pdch::rcv_block_gprs(uint8_t *data, uint32_t fn,
- struct pcu_l1_meas *meas, GprsCodingScheme cs)
+ struct pcu_l1_meas *meas, const GprsCodingScheme &cs)
{
unsigned payload = data[0] >> 6;
bitvec *block;
diff --git a/src/bts.h b/src/bts.h
index c975304..812419e 100644
--- a/src/bts.h
+++ b/src/bts.h
@@ -66,9 +66,9 @@ struct gprs_rlcmac_pdch {
int rcv_block(uint8_t *data, uint8_t len, uint32_t fn,
struct pcu_l1_meas *meas);
int rcv_block_gprs(uint8_t *data, uint32_t fn,
- struct pcu_l1_meas *meas, GprsCodingScheme cs);
+ struct pcu_l1_meas *meas, const GprsCodingScheme &cs);
int rcv_data_block(uint8_t *data, uint32_t fn,
- struct pcu_l1_meas *meas, GprsCodingScheme cs);
+ struct pcu_l1_meas *meas, const GprsCodingScheme &cs);
gprs_rlcmac_bts *bts_data() const;
BTS *bts() const;
diff --git a/src/decoding.cpp b/src/decoding.cpp
index 0c81b2a..1c94b5e 100644
--- a/src/decoding.cpp
+++ b/src/decoding.cpp
@@ -181,7 +181,7 @@ static int parse_extensions_gprs(const uint8_t *data, unsigned int
data_len,
}
int Decoding::rlc_data_from_ul_data(
- const struct gprs_rlc_data_block_info *rdbi, GprsCodingScheme cs,
+ const struct gprs_rlc_data_block_info *rdbi, const GprsCodingScheme &cs,
const uint8_t *data, RlcData *chunks, unsigned int chunks_size,
uint32_t *tlli)
{
@@ -342,7 +342,7 @@ void Decoding::extract_rbb(const struct bitvec *rbb, char *show_rbb)
}
int Decoding::rlc_parse_ul_data_header(struct gprs_rlc_data_info *rlc,
- const uint8_t *data, GprsCodingScheme cs)
+ const uint8_t *data, const GprsCodingScheme &cs)
{
unsigned int cur_bit = 0;
switch(cs.headerTypeData()) {
diff --git a/src/decoding.h b/src/decoding.h
index 1043d67..efee4d2 100644
--- a/src/decoding.h
+++ b/src/decoding.h
@@ -36,7 +36,7 @@ public:
static int rlc_data_from_ul_data(
const struct gprs_rlc_data_block_info *rdbi,
- GprsCodingScheme cs, const uint8_t *data, RlcData *chunks,
+ const GprsCodingScheme &cs, const uint8_t *data, RlcData *chunks,
unsigned int chunks_size, uint32_t *tlli);
static uint8_t get_ms_class_by_capability(MS_Radio_Access_capability_t *cap);
static uint8_t get_egprs_ms_class_by_capability(MS_Radio_Access_capability_t *cap);
@@ -52,7 +52,7 @@ public:
const uint8_t *data,
const GprsCodingScheme &cs);
static int rlc_parse_ul_data_header(struct gprs_rlc_data_info *rlc,
- const uint8_t *data, GprsCodingScheme cs);
+ const uint8_t *data, const GprsCodingScheme &cs);
static unsigned int rlc_copy_to_aligned_buffer(
const struct gprs_rlc_data_info *rlc,
unsigned int data_block_idx,
diff --git a/src/gprs_coding_scheme.cpp b/src/gprs_coding_scheme.cpp
index 8601d4f..cb141c1 100644
--- a/src/gprs_coding_scheme.cpp
+++ b/src/gprs_coding_scheme.cpp
@@ -259,7 +259,7 @@ const char *GprsCodingScheme::modeName(Mode mode)
}
}
-bool GprsCodingScheme::isFamilyCompatible(GprsCodingScheme o) const
+bool GprsCodingScheme::isFamilyCompatible(const GprsCodingScheme &o) const
{
if (*this == o)
return true;
@@ -270,7 +270,7 @@ bool GprsCodingScheme::isFamilyCompatible(GprsCodingScheme o) const
return family() == o.family();
}
-bool GprsCodingScheme::isCombinable(GprsCodingScheme o) const
+bool GprsCodingScheme::isCombinable(const GprsCodingScheme &o) const
{
return numDataBlocks() == o.numDataBlocks();
}
diff --git a/src/gprs_coding_scheme.h b/src/gprs_coding_scheme.h
index aec3762..1519e12 100644
--- a/src/gprs_coding_scheme.h
+++ b/src/gprs_coding_scheme.h
@@ -64,16 +64,16 @@ public:
unsigned int to_num() const;
GprsCodingScheme& operator =(Scheme s);
- GprsCodingScheme& operator =(GprsCodingScheme o);
+ GprsCodingScheme& operator =(const GprsCodingScheme &o);
bool isValid() const {return UNKNOWN <= m_scheme && m_scheme <= MCS9;}
bool isGprs() const {return CS1 <= m_scheme && m_scheme <= CS4;}
bool isEgprs() const {return m_scheme >= MCS1;}
bool isEgprsGmsk() const {return isEgprs() && m_scheme <= MCS4;}
bool isCompatible(Mode mode) const;
- bool isCompatible(GprsCodingScheme o) const;
- bool isFamilyCompatible(GprsCodingScheme o) const;
- bool isCombinable(GprsCodingScheme o) const;
+ bool isCompatible(const GprsCodingScheme &o) const;
+ bool isFamilyCompatible(const GprsCodingScheme &o) const;
+ bool isCombinable(const GprsCodingScheme &o) const;
void inc(Mode mode);
void dec(Mode mode);
@@ -133,7 +133,7 @@ inline bool GprsCodingScheme::isCompatible(Mode mode) const
return false;
}
-inline bool GprsCodingScheme::isCompatible(GprsCodingScheme o) const
+inline bool GprsCodingScheme::isCompatible(const GprsCodingScheme &o) const
{
return (isGprs() && o.isGprs()) || (isEgprs() && o.isEgprs());
}
@@ -160,7 +160,7 @@ inline GprsCodingScheme& GprsCodingScheme::operator =(Scheme s)
return *this;
}
-inline GprsCodingScheme& GprsCodingScheme::operator =(GprsCodingScheme o)
+inline GprsCodingScheme& GprsCodingScheme::operator =(const GprsCodingScheme &o)
{
m_scheme = o.m_scheme;
return *this;
@@ -183,33 +183,33 @@ inline GprsCodingScheme GprsCodingScheme::getEgprsByNum(unsigned
num)
}
/* The coding schemes form a partial ordering */
-inline bool operator ==(GprsCodingScheme a, GprsCodingScheme b)
+inline bool operator ==(const GprsCodingScheme &a, const GprsCodingScheme &b)
{
return GprsCodingScheme::Scheme(a) == GprsCodingScheme::Scheme(b);
}
-inline bool operator !=(GprsCodingScheme a, GprsCodingScheme b)
+inline bool operator !=(const GprsCodingScheme &a, const GprsCodingScheme &b)
{
return !(a == b);
}
-inline bool operator <(GprsCodingScheme a, GprsCodingScheme b)
+inline bool operator <(const GprsCodingScheme &a, const GprsCodingScheme &b)
{
return a.isCompatible(b) &&
GprsCodingScheme::Scheme(a) < GprsCodingScheme::Scheme(b);
}
-inline bool operator >(GprsCodingScheme a, GprsCodingScheme b)
+inline bool operator >(const GprsCodingScheme &a, const GprsCodingScheme &b)
{
return b < a;
}
-inline bool operator <=(GprsCodingScheme a, GprsCodingScheme b)
+inline bool operator <=(const GprsCodingScheme &a, const GprsCodingScheme &b)
{
return a == b || a < b;
}
-inline bool operator >=(GprsCodingScheme a, GprsCodingScheme b)
+inline bool operator >=(const GprsCodingScheme &a, const GprsCodingScheme &b)
{
return a == b || a > b;
}
--
2.5.0
9:26 p.m.
New subject: [PATCH 2/2] Avoid copy constructor in uplink tbf path
On 22 Mar 2016, at 14:12, Bhargava Abhyankar
<Bhargava.Abhyankar(a)radisys.com> wrote:
The copy constructor of GprsCodingScheme class object used in
uplink tbf path is replaced by the reference variable.
Appropriate function prototypes and function definition are
changed.
yes, it is good style to mark them as const (and pass them as reference).
This patch will increase efficiency of the code.
Given the size of the member and that the constructor is inlined my first assumption is
that for this specific type the runtime cost of copying m_scheme and a pointer will be
about the same.
kind regards
holger
23 Mar
23 Mar
11:23 a.m.
New subject: [PATCH 2/2] Avoid copy constructor in uplink tbf path
Hi
On 22.03.2016 21:26, Holger Freyther wrote:
Hmm. I do not agree. Passing as reference doesn't make much sense here
(there is not performance issue and no risk of slicing). It just an
integer/enum disguised as class and it is meant be used like an enum.
But of course, if it was a reference, marking it as const would be good.
On 22 Mar 2016, at 14:12, Bhargava Abhyankar
<Bhargava.Abhyankar(a)radisys.com> wrote:
The copy constructor of GprsCodingScheme class object used in
uplink tbf path is replaced by the reference variable.
Appropriate function prototypes and function definition are
changed.
yes, it is good style to mark them as const (and pass them as reference).
> This patch will increase efficiency of the code.
No, using a reference in general (unless inlining is done) will
eventually result in a larger variable/parameter/field size (depending
on the enum size and the pointer size on the target plattform, where the
pointer size will be >= enum size on all platforms I know of). In
addition, dereferencing is needed which cost additional instructions and
memory accesses (albeit with a high probability for a cache hit, if
there is any).
So in my personal opinion I wouldn't merge this patch, BYMMV.
Kind regards
Jacob
Given the size of the member and that the constructor is inlined my first assumption is
that for this specific type the runtime cost of copying m_scheme and a pointer will be
about the same.
11:34 a.m.
New subject: [PATCH 2/2] Avoid copy constructor in uplink tbf path
On 23 Mar 2016, at 11:23, Jacob
<jacob01(a)gmx.net> wrote:
Hi
Hi,
yes, it is good style to mark them as const (and pass them as reference).
Hmm. I do not agree. Passing as reference doesn't make much sense here
(there is not performance issue and no risk of slicing). It just an
integer/enum disguised as class and it is meant be used like an enum.
But of course, if it was a reference, marking it as const would be good. > This patch
will increase efficiency of the code.
No, using a reference in general (unless inlining is done) will
eventually result in a larger variable/parameter/field size (depending
on the enum size and the pointer size on the target plattform, where the
pointer size will be >= enum size on all platforms I know of). In
addition, dereferencing is needed which cost additional instructions and
memory accesses (albeit with a high probability for a cache hit, if
there is any). 
11:46 a.m.
New subject: [PATCH 2/2] Avoid copy constructor in uplink tbf path
Dear all,
as a general rule, I suggest to refrain from optimizations, unless
a) a performance issue is immediately obvious, or
b) there is actual profiling showing an improvement comparing
before/after, or at least showing that the existing code is a CPU
hog.
Everything else is - with all respect - likely to waste time of everyone
involved in preparing the patch, reviewing it, discussing it, etc.
To say it with Donald Knuth:
"Premature optimization is the root of all evil"
Now that of course is no excuse from writing unneccesarily bloated or
inefficient code in the first place, but we generally try to keep things
simple.
Also, keep in mind that a PCU in 1999/2000 had to run on incredibly
small and inefficient hardware, while today I think the smallest system
is the sysmoBTS 1002 on an ARM926EJS processor at 405 MHz. And it only
has a single TRX at that.
All other systems it runs on have much faster CPU, memory bandwidth, RAM
size.... so let's focus on improving the code functionality and
completeness before optimizing things that are not even proven issues in
the real world
Regards,
Harald
--
- Harald Welte <laforge(a)gnumonks.org> http://laforge.gnumonks.org/
============================================================================
"Privacy in residential applications is a desirable marketing option."
(ETSI EN 300 175-7 Ch. A6)
24 Mar
24 Mar
2:23 p.m.
Hello,
Please let me know the feedback for this patch. I would like to send further patches of
mcs 5 to mcs 9 support in uplink on top of this patch.
Regards
Bhargava Abhyankar
11:45 p.m.
Hi Bhargava,
On Tue, Mar 22, 2016 at 06:42:30PM +0530, Bhargava Abhyankar wrote:
Parsing the uplink data header for GPRS and EGPRS
header type 3
is handled in separate functions.
This patch will enhance modularity of the code.
I'm not an expert on OsmoPCU, but this patch looks pretty
straight-forward and undisputable. Moving away from large switch
statements towards separate functions/methods is generally appreciated
a lot, as is increasing modularity.
--
- Harald Welte <laforge(a)gnumonks.org> http://laforge.gnumonks.org/
============================================================================
"Privacy in residential applications is a desirable marketing option."
(ETSI EN 300 175-7 Ch. A6)
30 Mar
30 Mar
10:11 p.m.
Hi Bhargava,
On Tue, Mar 22, 2016 at 06:42:30PM +0530, Bhargava Abhyankar wrote:
Parsing the uplink data header for GPRS and EGPRS
header type 3
is handled in separate functions.
This patch will enhance modularity of the code.
I've merged this to master now, thanks.
The 2/2 of this series is rejected, see the related discussion.
--
- Harald Welte <laforge(a)gnumonks.org> http://laforge.gnumonks.org/
============================================================================
"Privacy in residential applications is a desirable marketing option."
(ETSI EN 300 175-7 Ch. A6)
3366
days inactive
3374
days old
osmocom-net-gprs@lists.osmocom.org
8 comments
4 participants
participants (4)
-
Bhargava Abhyankar -
Harald Welte -
Holger Freyther -
Jacob