On Thu, Jan 19, 2023 at 01:47:25PM +0100, Simon Wunderlich wrote:
On Tuesday, December 27, 2022 8:34:07 PM CET Linus Lüssing wrote:
+/**
- struct batadv_tvlv_mcast_tracker - payload of a multicast tracker tvlv
- @num_dests: number of subsequent destination originator MAC addresses
- @align: (optional) alignment bytes to make the tracker TVLV 4 bytes
aligned, + * present if num_dests are even, not present if odd
- */
+struct batadv_tvlv_mcast_tracker {
__be16 num_dests;__u8 align[2];+};
The one thing which I really don't like is to have the alignment in the beginning, and depending on the number of entries. Normally, such alignments should be at the end of the structure so it is straight forward for a parser to omit it.
My understanding is that the alignment is due to technical reasons (mac address list is assembled by pushing the data to the front), perhaps to save another memove/memcpy. However, the data is collected by traversing various lists, and if performance would be a concern, then this data should be cached and this "technicality" wouldn't be needed either.
The technical reason was to have the payload data from the IP header 4 bytes aligned. To allow efficient access in there once the payload is decapsulated.
So please, skip the alignment in the front and have it in the back.
The reasons to put the aligment at the front are/were the following two:
1) We do keep a count of receiving nodes, however it might change while we are pushing destinations. So it is at the moment easier to push the padding to the front, when we know exactly how many destinations we have added to the packet. As discussed offline, I guess that is what you were refering to above.
2) Alignment changes during routing everytime a router removes an odd number of destinations from the tracker TVLV. Changing alignment more to the front would in theory less memory movements. Example for an ideal case:
[MC-HDR][pad][dest1][dest2]...[dest100][PAYLOAD] -> dest1 is deleted [MC-HDR][pad][00000][dest2]...[dest100][PAYLOAD] -> move only MC-HDR: [MC-HDR][dest2]...[dest100][PAYLOAD]
If the alignment were behind dest100 then everything would need to be moved.
Even though it's unconventional to have the padding in the front, I found the idea quite tempting and useful to have the padding in front, saves quite a bit of code and potential bugs I would hope :D. And would hopefully (potentially allow to) make the packet routing a bit faster. So from a "purely technical" point-of-view it seemed advantageous to me :D.
Another idea would be to have the padding at the end by adding and keeping a zero-MAC destination entry if the number of destinations were otherwise even. Disadvantage: Might waste 4 more bytes than necessary.
The rest of the packet format looks good from what I've seen.
Thanks for having a look at it!
Regards, Linus