[PATCH 0/5] batman-adv: Bitmap helper and size handling cleanup for aggregations

b.a.t.m.a.n.lists.open-mesh.org archive mirror
 help / color / mirror / Atom feed

* [PATCH 0/5] batman-adv: Bitmap helper and size handling cleanup for aggregations
@ 2025-02-02 16:04 Sven Eckelmann
  2025-02-02 16:04 ` [PATCH 1/5] batman-adv: Limit number of aggregated packets directly Sven Eckelmann
                   ` (4 more replies)
  0 siblings, 5 replies; 6+ messages in thread
From: Sven Eckelmann @ 2025-02-02 16:04 UTC (permalink / raw)
  To: b.a.t.m.a.n; +Cc: Sven Eckelmann

The B.A.T.M.A.N. IV code enforced a limit on the size of an aggregated OGM
packet due to some self written (inline) code to perform bit operations on
an simple unsigned integer. The comment which explained this was too vague
to really understand the size limitations of the u32 vs. the packet size.

By using the Linux bitmap helpers, it is possible to untangle the maximum
number of fragmented packets from the maximum size of an aggregate. Which
can make the aggregation code more flexible for tests and maybe better
airtime usage in the future.

At the same time, it was noticed that the size handling code was slightly
off. For the RX side, the size of the aggregation was limited. And for the
TX side, it didn't consider the MTU for the outgoing interface.

Signed-off-by: Sven Eckelmann <sven@narfation.org>
---
Sven Eckelmann (5):
      batman-adv: Limit number of aggregated packets directly
      batman-adv: Switch to bitmap helper for aggregation handling
      batman-adv: Use actual packet count for aggregated packets
      batman-adv: Ignore own maximum aggregation size during RX
      batman-adv: Limit aggregation size to outgoing MTU

 net/batman-adv/bat_iv_ogm.c | 43 +++++++++++++++++++++++++------------------
 net/batman-adv/bat_v_ogm.c  |  3 +--
 net/batman-adv/main.h       |  4 +---
 net/batman-adv/send.c       |  2 +-
 net/batman-adv/types.h      |  2 +-
 5 files changed, 29 insertions(+), 25 deletions(-)
---
base-commit: dfdce35d69c55b6b5d017a7e800e553089a26eb9
change-id: 20250202-bitmap_aggregation-dd6046fbc5dc

Best regards,
-- 
Sven Eckelmann <sven@narfation.org>

^ permalink raw reply	[flat|nested] 6+ messages in thread

* [PATCH 1/5] batman-adv: Limit number of aggregated packets directly
  2025-02-02 16:04 [PATCH 0/5] batman-adv: Bitmap helper and size handling cleanup for aggregations Sven Eckelmann
@ 2025-02-02 16:04 ` Sven Eckelmann
  2025-02-02 16:04 ` [PATCH 2/5] batman-adv: Switch to bitmap helper for aggregation handling Sven Eckelmann
                   ` (3 subsequent siblings)
  4 siblings, 0 replies; 6+ messages in thread
From: Sven Eckelmann @ 2025-02-02 16:04 UTC (permalink / raw)
  To: b.a.t.m.a.n; +Cc: Sven Eckelmann

The currently selected size in BATADV_MAX_AGGREGATION_BYTES (512) is chosen
such that the number of possible aggregated packets is lower than 32. This
number must be limited so that the type of
batadv_forw_packet->direct_link_flags has enough bits to represent each
packet (with the size of at least 24 bytes).

This requirement is better implemented in code instead of having it inside
a comment.

Signed-off-by: Sven Eckelmann <sven@narfation.org>
---
 net/batman-adv/bat_iv_ogm.c | 4 ++++
 net/batman-adv/main.h       | 3 ---
 2 files changed, 4 insertions(+), 3 deletions(-)

diff --git a/net/batman-adv/bat_iv_ogm.c b/net/batman-adv/bat_iv_ogm.c
index 07ae5dd1f150b062e97897b269d45b041e8b4dfe..b316cfc5b8769928a77ac936e5935ae780d5ad68 100644
--- a/net/batman-adv/bat_iv_ogm.c
+++ b/net/batman-adv/bat_iv_ogm.c
@@ -446,6 +446,7 @@ batadv_iv_ogm_can_aggregate(const struct batadv_ogm_packet *new_bat_ogm_packet,
 	struct batadv_ogm_packet *batadv_ogm_packet;
 	int aggregated_bytes = forw_packet->packet_len + packet_len;
 	struct batadv_hard_iface *primary_if = NULL;
+	u8 packet_num = forw_packet->num_packets + 1;
 	bool res = false;
 	unsigned long aggregation_end_time;
 
@@ -468,6 +469,9 @@ batadv_iv_ogm_can_aggregate(const struct batadv_ogm_packet *new_bat_ogm_packet,
 	if (aggregated_bytes > BATADV_MAX_AGGREGATION_BYTES)
 		return false;
 
+	if (packet_num >= BITS_PER_TYPE(forw_packet->direct_link_flags))
+		return false;
+
 	/* packet is not leaving on the same interface. */
 	if (forw_packet->if_outgoing != if_outgoing)
 		return false;
diff --git a/net/batman-adv/main.h b/net/batman-adv/main.h
index 964f3088af5b52d3ad1d709fb964dc2c3d79d7a5..485e68a06835b48923278da7b9aa79db6968945f 100644
--- a/net/batman-adv/main.h
+++ b/net/batman-adv/main.h
@@ -102,9 +102,6 @@
  */
 #define BATADV_TQ_SIMILARITY_THRESHOLD 50
 
-/* should not be bigger than 512 bytes or change the size of
- * forw_packet->direct_link_flags
- */
 #define BATADV_MAX_AGGREGATION_BYTES 512
 #define BATADV_MAX_AGGREGATION_MS 100
 

-- 
2.47.2


^ permalink raw reply related	[flat|nested] 6+ messages in thread

* [PATCH 2/5] batman-adv: Switch to bitmap helper for aggregation handling
  2025-02-02 16:04 [PATCH 0/5] batman-adv: Bitmap helper and size handling cleanup for aggregations Sven Eckelmann
  2025-02-02 16:04 ` [PATCH 1/5] batman-adv: Limit number of aggregated packets directly Sven Eckelmann
@ 2025-02-02 16:04 ` Sven Eckelmann
  2025-02-02 16:04 ` [PATCH 3/5] batman-adv: Use actual packet count for aggregated packets Sven Eckelmann
                   ` (2 subsequent siblings)
  4 siblings, 0 replies; 6+ messages in thread
From: Sven Eckelmann @ 2025-02-02 16:04 UTC (permalink / raw)
  To: b.a.t.m.a.n; +Cc: Sven Eckelmann

The aggregation code duplicates code which already exists in the the bitops
and bitmap helper. By switching to the bitmap helpers, operating on larger
aggregations becomes possible without touching the different portions of
the code which read/modify direct_link_flags.

Signed-off-by: Sven Eckelmann <sven@narfation.org>
---
 net/batman-adv/bat_iv_ogm.c | 19 +++++++++----------
 net/batman-adv/main.h       |  1 +
 net/batman-adv/types.h      |  2 +-
 3 files changed, 11 insertions(+), 11 deletions(-)

diff --git a/net/batman-adv/bat_iv_ogm.c b/net/batman-adv/bat_iv_ogm.c
index b316cfc5b8769928a77ac936e5935ae780d5ad68..47967a53c416abc6ac37cdd269b5277e8e47f680 100644
--- a/net/batman-adv/bat_iv_ogm.c
+++ b/net/batman-adv/bat_iv_ogm.c
@@ -355,7 +355,7 @@ static void batadv_iv_ogm_send_to_if(struct batadv_forw_packet *forw_packet,
 		/* we might have aggregated direct link packets with an
 		 * ordinary base packet
 		 */
-		if (forw_packet->direct_link_flags & BIT(packet_num) &&
+		if (test_bit(packet_num, forw_packet->direct_link_flags) &&
 		    forw_packet->if_incoming == hard_iface)
 			batadv_ogm_packet->flags |= BATADV_DIRECTLINK;
 		else
@@ -460,6 +460,7 @@ batadv_iv_ogm_can_aggregate(const struct batadv_ogm_packet *new_bat_ogm_packet,
 	 * - the send time is within our MAX_AGGREGATION_MS time
 	 * - the resulting packet won't be bigger than
 	 *   MAX_AGGREGATION_BYTES
+	 * - the number of packets is lower than MAX_AGGREGATION_PACKETS
 	 * otherwise aggregation is not possible
 	 */
 	if (!time_before(send_time, forw_packet->send_time) ||
@@ -469,7 +470,7 @@ batadv_iv_ogm_can_aggregate(const struct batadv_ogm_packet *new_bat_ogm_packet,
 	if (aggregated_bytes > BATADV_MAX_AGGREGATION_BYTES)
 		return false;
 
-	if (packet_num >= BITS_PER_TYPE(forw_packet->direct_link_flags))
+	if (packet_num >= BATADV_MAX_AGGREGATION_PACKETS)
 		return false;
 
 	/* packet is not leaving on the same interface. */
@@ -578,12 +579,13 @@ static void batadv_iv_ogm_aggregate_new(const unsigned char *packet_buff,
 	memcpy(skb_buff, packet_buff, packet_len);
 
 	forw_packet_aggr->own = own_packet;
-	forw_packet_aggr->direct_link_flags = BATADV_NO_FLAGS;
+	bitmap_zero(forw_packet_aggr->direct_link_flags,
+		    BATADV_MAX_AGGREGATION_PACKETS);
 	forw_packet_aggr->send_time = send_time;
 
 	/* save packet direct link flag status */
 	if (direct_link)
-		forw_packet_aggr->direct_link_flags |= 1;
+		set_bit(0, forw_packet_aggr->direct_link_flags);
 
 	INIT_DELAYED_WORK(&forw_packet_aggr->delayed_work,
 			  batadv_iv_send_outstanding_bat_ogm_packet);
@@ -596,17 +598,14 @@ static void batadv_iv_ogm_aggregate(struct batadv_forw_packet *forw_packet_aggr,
 				    const unsigned char *packet_buff,
 				    int packet_len, bool direct_link)
 {
-	unsigned long new_direct_link_flag;
-
 	skb_put_data(forw_packet_aggr->skb, packet_buff, packet_len);
 	forw_packet_aggr->packet_len += packet_len;
 	forw_packet_aggr->num_packets++;
 
 	/* save packet direct link flag status */
-	if (direct_link) {
-		new_direct_link_flag = BIT(forw_packet_aggr->num_packets);
-		forw_packet_aggr->direct_link_flags |= new_direct_link_flag;
-	}
+	if (direct_link)
+		set_bit(forw_packet_aggr->num_packets,
+			forw_packet_aggr->direct_link_flags);
 }
 
 /**
diff --git a/net/batman-adv/main.h b/net/batman-adv/main.h
index 485e68a06835b48923278da7b9aa79db6968945f..541fce90ec4188c993e5df16da5b61f493b5a5e0 100644
--- a/net/batman-adv/main.h
+++ b/net/batman-adv/main.h
@@ -102,6 +102,7 @@
  */
 #define BATADV_TQ_SIMILARITY_THRESHOLD 50
 
+#define BATADV_MAX_AGGREGATION_PACKETS 32
 #define BATADV_MAX_AGGREGATION_BYTES 512
 #define BATADV_MAX_AGGREGATION_MS 100
 
diff --git a/net/batman-adv/types.h b/net/batman-adv/types.h
index fe89f08533fe694e137bf31594c60915d4783c7a..4216d01aac803339528943762b55076b5387df4a 100644
--- a/net/batman-adv/types.h
+++ b/net/batman-adv/types.h
@@ -2161,7 +2161,7 @@ struct batadv_forw_packet {
 	u16 packet_len;
 
 	/** @direct_link_flags: direct link flags for aggregated OGM packets */
-	u32 direct_link_flags;
+	DECLARE_BITMAP(direct_link_flags, BATADV_MAX_AGGREGATION_PACKETS);
 
 	/** @num_packets: counter for aggregated OGMv1 packets */
 	u8 num_packets;

-- 
2.47.2


^ permalink raw reply related	[flat|nested] 6+ messages in thread

* [PATCH 3/5] batman-adv: Use actual packet count for aggregated packets
  2025-02-02 16:04 [PATCH 0/5] batman-adv: Bitmap helper and size handling cleanup for aggregations Sven Eckelmann
  2025-02-02 16:04 ` [PATCH 1/5] batman-adv: Limit number of aggregated packets directly Sven Eckelmann
  2025-02-02 16:04 ` [PATCH 2/5] batman-adv: Switch to bitmap helper for aggregation handling Sven Eckelmann
@ 2025-02-02 16:04 ` Sven Eckelmann
  2025-02-02 16:04 ` [PATCH 4/5] batman-adv: Ignore own maximum aggregation size during RX Sven Eckelmann
  2025-02-02 16:04 ` [PATCH 5/5] batman-adv: Limit aggregation size to outgoing MTU Sven Eckelmann
  4 siblings, 0 replies; 6+ messages in thread
From: Sven Eckelmann @ 2025-02-02 16:04 UTC (permalink / raw)
  To: b.a.t.m.a.n; +Cc: Sven Eckelmann

The batadv_forw_packet->num_packets didn't store the number of packets but
the the number of packets - 1. This didn't had any effects on the actual
handling of aggregates but can easily be a source of confusion when reading
the code.

Signed-off-by: Sven Eckelmann <sven@narfation.org>
---
 net/batman-adv/bat_iv_ogm.c | 5 +++--
 net/batman-adv/send.c       | 2 +-
 2 files changed, 4 insertions(+), 3 deletions(-)

diff --git a/net/batman-adv/bat_iv_ogm.c b/net/batman-adv/bat_iv_ogm.c
index 47967a53c416abc6ac37cdd269b5277e8e47f680..a43c268b2a3028c9a36fe8ce060cfb23ac5f9381 100644
--- a/net/batman-adv/bat_iv_ogm.c
+++ b/net/batman-adv/bat_iv_ogm.c
@@ -446,7 +446,7 @@ batadv_iv_ogm_can_aggregate(const struct batadv_ogm_packet *new_bat_ogm_packet,
 	struct batadv_ogm_packet *batadv_ogm_packet;
 	int aggregated_bytes = forw_packet->packet_len + packet_len;
 	struct batadv_hard_iface *primary_if = NULL;
-	u8 packet_num = forw_packet->num_packets + 1;
+	u8 packet_num = forw_packet->num_packets;
 	bool res = false;
 	unsigned long aggregation_end_time;
 
@@ -600,12 +600,13 @@ static void batadv_iv_ogm_aggregate(struct batadv_forw_packet *forw_packet_aggr,
 {
 	skb_put_data(forw_packet_aggr->skb, packet_buff, packet_len);
 	forw_packet_aggr->packet_len += packet_len;
-	forw_packet_aggr->num_packets++;
 
 	/* save packet direct link flag status */
 	if (direct_link)
 		set_bit(forw_packet_aggr->num_packets,
 			forw_packet_aggr->direct_link_flags);
+
+	forw_packet_aggr->num_packets++;
 }
 
 /**
diff --git a/net/batman-adv/send.c b/net/batman-adv/send.c
index 0379b126865d7c07b84c08c93eca38279fe6df9c..39a13664dc4dae46f49851af738bca4c3b8891ef 100644
--- a/net/batman-adv/send.c
+++ b/net/batman-adv/send.c
@@ -532,7 +532,7 @@ batadv_forw_packet_alloc(struct batadv_hard_iface *if_incoming,
 	forw_packet->queue_left = queue_left;
 	forw_packet->if_incoming = if_incoming;
 	forw_packet->if_outgoing = if_outgoing;
-	forw_packet->num_packets = 0;
+	forw_packet->num_packets = 1;
 
 	return forw_packet;
 

-- 
2.47.2


^ permalink raw reply related	[flat|nested] 6+ messages in thread

* [PATCH 4/5] batman-adv: Ignore own maximum aggregation size during RX
  2025-02-02 16:04 [PATCH 0/5] batman-adv: Bitmap helper and size handling cleanup for aggregations Sven Eckelmann
                   ` (2 preceding siblings ...)
  2025-02-02 16:04 ` [PATCH 3/5] batman-adv: Use actual packet count for aggregated packets Sven Eckelmann
@ 2025-02-02 16:04 ` Sven Eckelmann
  2025-02-02 16:04 ` [PATCH 5/5] batman-adv: Limit aggregation size to outgoing MTU Sven Eckelmann
  4 siblings, 0 replies; 6+ messages in thread
From: Sven Eckelmann @ 2025-02-02 16:04 UTC (permalink / raw)
  To: b.a.t.m.a.n; +Cc: Sven Eckelmann

An OGMv1 and OGMv2 packet receive processing were not only limited by the
number of bytes in the received packet but also by the nodes maximum
aggregation packet size limit. But this limit is relevant for TX and not
for RX. It should thefore not be enforced by batadv_(i)v_ogm_aggr_packet

This has a minor side effect for B.A.T.M.A.N. IV because the
batadv_iv_ogm_aggr_packet is also used for the preprocessing for the TX.
But since the aggregation code itself will not allow more than
BATADV_MAX_AGGREGATION_BYTES bytes, this check was never triggering prior
of removing it.

Signed-off-by: Sven Eckelmann <sven@narfation.org>
---
 net/batman-adv/bat_iv_ogm.c | 3 +--
 net/batman-adv/bat_v_ogm.c  | 3 +--
 2 files changed, 2 insertions(+), 4 deletions(-)

diff --git a/net/batman-adv/bat_iv_ogm.c b/net/batman-adv/bat_iv_ogm.c
index a43c268b2a3028c9a36fe8ce060cfb23ac5f9381..769d5ff779eb88de8b9d889bb3e4a3304875ea32 100644
--- a/net/batman-adv/bat_iv_ogm.c
+++ b/net/batman-adv/bat_iv_ogm.c
@@ -325,8 +325,7 @@ batadv_iv_ogm_aggr_packet(int buff_pos, int packet_len,
 	/* check if there is enough space for the optional TVLV */
 	next_buff_pos += ntohs(ogm_packet->tvlv_len);
 
-	return (next_buff_pos <= packet_len) &&
-	       (next_buff_pos <= BATADV_MAX_AGGREGATION_BYTES);
+	return next_buff_pos <= packet_len;
 }
 
 /* send a batman ogm to a given interface */
diff --git a/net/batman-adv/bat_v_ogm.c b/net/batman-adv/bat_v_ogm.c
index e503ee0d896bd52bb5827102a7a57fc07279f0bd..8f89ffe6020ced5668dc90ac438ecd6dcd1d74b6 100644
--- a/net/batman-adv/bat_v_ogm.c
+++ b/net/batman-adv/bat_v_ogm.c
@@ -839,8 +839,7 @@ batadv_v_ogm_aggr_packet(int buff_pos, int packet_len,
 	/* check if there is enough space for the optional TVLV */
 	next_buff_pos += ntohs(ogm2_packet->tvlv_len);
 
-	return (next_buff_pos <= packet_len) &&
-	       (next_buff_pos <= BATADV_MAX_AGGREGATION_BYTES);
+	return next_buff_pos <= packet_len;
 }
 
 /**

-- 
2.47.2


^ permalink raw reply related	[flat|nested] 6+ messages in thread

* [PATCH 5/5] batman-adv: Limit aggregation size to outgoing MTU
  2025-02-02 16:04 [PATCH 0/5] batman-adv: Bitmap helper and size handling cleanup for aggregations Sven Eckelmann
                   ` (3 preceding siblings ...)
  2025-02-02 16:04 ` [PATCH 4/5] batman-adv: Ignore own maximum aggregation size during RX Sven Eckelmann
@ 2025-02-02 16:04 ` Sven Eckelmann
  4 siblings, 0 replies; 6+ messages in thread
From: Sven Eckelmann @ 2025-02-02 16:04 UTC (permalink / raw)
  To: b.a.t.m.a.n; +Cc: Sven Eckelmann

If a B.A.T.M.A.N. IV aggregated OGM was prepared, it was always assumed
that 512 bytes (BATADV_MAX_AGGREGATION_BYTES) can be transmitted. But the
outgoing MTU might be too small for these 512 bytes and the aggregation
size must be adjusted in this case. Otherwise, the aggregates will cause
unnecessary packet loss.

For now, the non-aggregated packet length is not touched.

Signed-off-by: Sven Eckelmann <sven@narfation.org>
---
 net/batman-adv/bat_iv_ogm.c | 16 ++++++++++------
 1 file changed, 10 insertions(+), 6 deletions(-)

diff --git a/net/batman-adv/bat_iv_ogm.c b/net/batman-adv/bat_iv_ogm.c
index 769d5ff779eb88de8b9d889bb3e4a3304875ea32..dbad6007871a97ee01f7295c005ab3deeff86efd 100644
--- a/net/batman-adv/bat_iv_ogm.c
+++ b/net/batman-adv/bat_iv_ogm.c
@@ -443,22 +443,26 @@ batadv_iv_ogm_can_aggregate(const struct batadv_ogm_packet *new_bat_ogm_packet,
 			    const struct batadv_forw_packet *forw_packet)
 {
 	struct batadv_ogm_packet *batadv_ogm_packet;
-	int aggregated_bytes = forw_packet->packet_len + packet_len;
+	unsigned int aggregated_bytes = forw_packet->packet_len + packet_len;
 	struct batadv_hard_iface *primary_if = NULL;
 	u8 packet_num = forw_packet->num_packets;
 	bool res = false;
 	unsigned long aggregation_end_time;
+	unsigned int max_bytes;
 
 	batadv_ogm_packet = (struct batadv_ogm_packet *)forw_packet->skb->data;
 	aggregation_end_time = send_time;
 	aggregation_end_time += msecs_to_jiffies(BATADV_MAX_AGGREGATION_MS);
 
+	max_bytes = min_t(unsigned int, if_outgoing->net_dev->mtu,
+			  BATADV_MAX_AGGREGATION_BYTES);
+
 	/* we can aggregate the current packet to this aggregated packet
 	 * if:
 	 *
 	 * - the send time is within our MAX_AGGREGATION_MS time
 	 * - the resulting packet won't be bigger than
-	 *   MAX_AGGREGATION_BYTES
+	 *   MAX_AGGREGATION_BYTES and MTU of the outgoing interface
 	 * - the number of packets is lower than MAX_AGGREGATION_PACKETS
 	 * otherwise aggregation is not possible
 	 */
@@ -466,7 +470,7 @@ batadv_iv_ogm_can_aggregate(const struct batadv_ogm_packet *new_bat_ogm_packet,
 	    !time_after_eq(aggregation_end_time, forw_packet->send_time))
 		return false;
 
-	if (aggregated_bytes > BATADV_MAX_AGGREGATION_BYTES)
+	if (aggregated_bytes > max_bytes)
 		return false;
 
 	if (packet_num >= BATADV_MAX_AGGREGATION_PACKETS)
@@ -551,9 +555,9 @@ static void batadv_iv_ogm_aggregate_new(const unsigned char *packet_buff,
 	unsigned int skb_size;
 	atomic_t *queue_left = own_packet ? NULL : &bat_priv->batman_queue_left;
 
-	if (atomic_read(&bat_priv->aggregated_ogms) &&
-	    packet_len < BATADV_MAX_AGGREGATION_BYTES)
-		skb_size = BATADV_MAX_AGGREGATION_BYTES;
+	if (atomic_read(&bat_priv->aggregated_ogms))
+		skb_size = max_t(unsigned int, BATADV_MAX_AGGREGATION_BYTES,
+				 packet_len);
 	else
 		skb_size = packet_len;
 

-- 
2.47.2


^ permalink raw reply related	[flat|nested] 6+ messages in thread

end of thread, other threads:[~2025-02-02 16:08 UTC | newest]

Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2025-02-02 16:04 [PATCH 0/5] batman-adv: Bitmap helper and size handling cleanup for aggregations Sven Eckelmann
2025-02-02 16:04 ` [PATCH 1/5] batman-adv: Limit number of aggregated packets directly Sven Eckelmann
2025-02-02 16:04 ` [PATCH 2/5] batman-adv: Switch to bitmap helper for aggregation handling Sven Eckelmann
2025-02-02 16:04 ` [PATCH 3/5] batman-adv: Use actual packet count for aggregated packets Sven Eckelmann
2025-02-02 16:04 ` [PATCH 4/5] batman-adv: Ignore own maximum aggregation size during RX Sven Eckelmann
2025-02-02 16:04 ` [PATCH 5/5] batman-adv: Limit aggregation size to outgoing MTU Sven Eckelmann

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).