Re: [PATCH v6 3/3] batman-adv: mcast: shrink tracker packet after scrubbing

[Simon Wunderlich]

On Thursday, July 20, 2023 6:35:55 AM CEST Linus Lüssing wrote:
> Remove all zero MAC address entries (00:00:00:00:00:00) from a multicast
> packet's tracker TVLV before transmitting it and update all headers
> accordingly. This way, instead of keeping the multicast packet at a
> constant size throughout its journey through the mesh, it will become
> more lightweight, smaller with every interested receiver on the way and
> on each splitting intersection. Which can save some valuable bandwidth.
> 
> Signed-off-by: Linus Lüssing <linus.luess...@c0d3.blue>
> ---
>  net/batman-adv/multicast_forw.c | 195 ++++++++++++++++++++++++++++++++
>  1 file changed, 195 insertions(+)
> 
> diff --git a/net/batman-adv/multicast_forw.c
> b/net/batman-adv/multicast_forw.c index d0f75a63de2a..d7b1aabd4b72 100644
> --- a/net/batman-adv/multicast_forw.c
> +++ b/net/batman-adv/multicast_forw.c
> @@ -712,6 +712,200 @@ batadv_mcast_forw_scrub_dests(struct batadv_priv
> *bat_priv, }
>  }
> 
> +/**
> + * batadv_mcast_forw_shrink_pack_dests() - pack destinations of a tracker
> TVLV + * @skb: the batman-adv multicast packet to compact destinations in +
> *
> + * Compacts the originator destination MAC addresses in the multicast
> tracker + * TVLV of the given multicast packet. This is done by moving all
> non-zero + * MAC addresses in direction of the skb head and all zero MAC
> addresses in skb + * tail direction, within the multicast tracker TVLV.
> + *
> + * Return: The number of consecutive zero MAC address destinations which
> are + * now at the end of the multicast tracker TVLV.
> + */
> +static int batadv_mcast_forw_shrink_pack_dests(struct sk_buff *skb)
> +{
> +     struct batadv_tvlv_mcast_tracker *mcast_tracker;
> +     u16 num_dests_slot, num_dests_filler;
> +     unsigned char *skb_net_hdr;
> +     u8 *slot, *filler;
> +
> +     skb_net_hdr = skb_network_header(skb);
> +     mcast_tracker = (struct batadv_tvlv_mcast_tracker *)skb_net_hdr;
> +     num_dests_slot = ntohs(mcast_tracker->num_dests);
> +
> +     slot = (u8 *)mcast_tracker + sizeof(*mcast_tracker);
> +
> +     if (!num_dests_slot)
> +             return 0;
> +
> +     num_dests_filler = num_dests_slot - 1;
> +     filler = slot + ETH_ALEN;
> +
> +     batadv_mcast_forw_tracker_for_each_dest(slot, num_dests_slot) {
> +             /* find an empty slot */
> +             if (!is_zero_ether_addr(slot))
> +                     continue;
> +
> +             /* keep filler ahead of slot */
> +             if (filler <= slot) {
> +                     num_dests_filler = num_dests_slot - 1;
> +                     filler = slot + ETH_ALEN;
> +             }
> +
> +             /* find a candidate to fill the empty slot */
> +             batadv_mcast_forw_tracker_for_each_dest(filler,
> +                                                     num_dests_filler) {
> +                     if (is_zero_ether_addr(filler))
> +                             continue;
> +
> +                     ether_addr_copy(slot, filler);
> +                     eth_zero_addr(filler);
> +                     goto cont_next_slot;

goto is redundant, just continue
> +             }
> +
> +             /* could not find a filler, we can stop
> +              * - and must not advance the slot pointer!
> +              */
> +             if (!num_dests_filler)
> +                     break;
> +

This label can then be removed as well.

I'm wondering why we need to keep all those pointers and do the pointer magic 
in the first place? can't we just make two nested functions like this:

for (all entries)
    if zero-entry:
    for (all entries)
         if non-zero-entry
              swap()

(I find this current function very hard to read)

Cheers,
      Simon

signature.asc
Description: This is a digitally signed message part.