Re: [PATCH v4 3/5] batman-adv: mcast: implement multicast packet reception and forwarding

["Linus Lüssing" <linus.luessing@c0d3.blue>]

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