Hi.
In "struct mframe_sched_item" in target/firmware/layer1/mframe_sched.c both 'modulo' and 'frame_nr' defined as uint16_t. This seems like big space waste considering tight memory environment we're working in.
On irc it was suggested that the reason for this is either data alignment or safety concerns.
Does structure alignment matters in this case? If so - how exactly? Is it possible for 'modulo' to be bigger than 255? Is it possible for 'frame_nr' to be bigger than 255?
Hi Max,
On Wed, Oct 17, 2012 at 08:21:59PM +0200, ☎ wrote:
In "struct mframe_sched_item" in target/firmware/layer1/mframe_sched.c both 'modulo' and 'frame_nr' defined as uint16_t. This seems like big space waste considering tight memory environment we're working in.
On ARM, it doesn't matter. even if you made those fields uint8_t in the struct mframe_sched_item, they would still be aligned to 32bit boundaries. So you wouldn't change the memory layout at all based on your change.
If you really wanted to save memory, then you would have to make the struct packed. This in turn would mean that accesses to the structure members would be unaligned accesses, around which the compiler has to work around by bit-shifting.
So you save some memory at the expense of increasing code size (and execution time).
Given that the CPU speed is more limited than RAM, I think the current approach makes sense.
Does structure alignment matters in this case? If so - how exactly?
see above.
Is it possible for 'modulo' to be bigger than 255?
I don't think so, at least not for standard GSM operation.
Is it possible for 'frame_nr' to be bigger than 255?
frame_nr can never be bigger than 'modulo'.
18.10.2012 12:15, Harald Welte пишет:
On ARM, it doesn't matter. even if you made those fields uint8_t in the struct mframe_sched_item, they would still be aligned to 32bit boundaries. So you wouldn't change the memory layout at all based on your change.
That's odd - after I've changed it to uint8_t the "board/compal_e88/rssi.compalram.elf section `.data' will not fit in region `LRAM'" error is gone. I'm no expert in compiler internals but I think that indicates the change in memory layout.
Could you please comment?
Given that the CPU speed is more limited than RAM, I think the current approach makes sense.
I agree.
Is it possible for 'modulo' to be bigger than 255?
I don't think so, at least not for standard GSM operation.
As Sylvain explained on irc having modulo and frame number bigger than 256 might be useful for experimentation with packets which are spread across several frames - to represent them as huge 'fake' multiframe.
At least that's how I've understood it. He's surely doing some creepy magic with GSM :-)
On Thu, Oct 18, 2012 at 01:48:15PM +0200, ☎ wrote:
18.10.2012 12:15, Harald Welte пишет:
Could you please comment?
You want to install a utility called pahole and then inspect the .o files that use the multiframe scheduler.
The pahole output looks like this:
struct gsm_time { uint32_t fn; /* 0 4 */ uint16_t t1; /* 4 2 */ uint8_t t2; /* 6 1 */ uint8_t t3; /* 7 1 */ uint8_t tc; /* 8 1 */
/* size: 12, cachelines: 1, members: 5 */ /* padding: 3 */ /* last cacheline: 12 bytes */ };
He's talking about this structure :
struct mframe_sched_item { const struct tdma_sched_item *sched_set; uint16_t modulo; uint16_t frame_nr; uint16_t flags; };
Which currently is most likely 12 bytes long and if you make modulo and frame_nr to 8 bits, it becomes 8 bytes long and shouldn't generate any unaligned access.
However I don't see the gain of space as really that much of an advantage vs the potential time lost looking for bugs when playing with non-standard multiframes.
The fact that we are currently almost full memory is just because we still use the compal loader. We should just deprecate that and oompletely and only compile the chain loader and loader with that constrained memory layout.
Cheers,
Sylvain
19.10.2012 09:57, Sylvain Munaut пишет:
He's talking about this structure :
struct mframe_sched_item { const struct tdma_sched_item *sched_set; uint16_t modulo; uint16_t frame_nr; uint16_t flags; };
Yes, exactly. Pardon for being unclear.
However I don't see the gain of space as really that much of an advantage vs the potential time lost looking for bugs when playing with non-standard multiframes.
I agree. Btw, is this non-standard multiframes experimentation available in some public repo? Would love to have a look at the code.
The fact that we are currently almost full memory is just because we still use the compal loader. We should just deprecate that and oompletely and only compile the chain loader and loader with that constrained memory layout.
Indeed - I've got error about rssi.compalram.elf file which is too big to be used without chainloading anyway.
Maybe we can make this configurable via something like ./configure --enable-compalram-rssi ./configure --enable-chainload-rssi etc.
19.10.2012 09:42, Holger Hans Peter Freyther пишет:
You want to install a utility called pahole and then inspect the .o files that use the multiframe scheduler.
Awesome utility, thanks! Just in case somebody would google it - in ubuntu you can obtain it via:
sudo aptitude install dwarves
baseband-devel@lists.osmocom.org
-
Harald Welte -
Holger Hans Peter Freyther -
Sylvain Munaut -
☎