[1/2] batman-adv: Make number of (re)broadcasts configurable via sysfs

Message ID 1362764336-1591-1-git-send-email-linus.luessing@web.de (mailing list archive)
State Superseded, archived
Headers

Commit Message

Linus Lüssing March 8, 2013, 5:38 p.m. UTC
  Depending on the scenario, people might want to adjust the number of
(re)broadcast of data packets - usually higher values in sparse or lower
values in dense networks.

Signed-off-by: Linus Lüssing <linus.luessing@web.de>
---
 send.c               |    4 +++-
 soft-interface.c     |    1 +
 sysfs-class-net-mesh |    8 ++++++++
 sysfs.c              |    2 ++
 types.h              |    1 +
 5 files changed, 15 insertions(+), 1 deletion(-)
  

Comments

Matthias Schiffer March 8, 2013, 7:42 p.m. UTC | #1
On 03/08/2013 08:28 PM, Marek Lindner wrote:
> On Saturday, March 09, 2013 01:38:56 Linus Lüssing wrote:
>> From: Matthias Schiffer <mschiffer@universe-factory.net>
>>
>> In heterogenous networks, setting the number of broadcasts to differing
>> values on different interfaces can be beneficial.
>>
>> E.g., on wireless interfaces with high packet loss a higher number of
>> broadcasts may be necessary, whereas on low-bandwidth interfaces with
>> relatively high reliablily (such as VPN links over slow internet lines)
>> sending only a single packet makes more sense to preserve bandwidth.
> 
> In general, I like the idea but the approach isn't the best. Can't we automate 
> these settings instead of adding hundreds of little knobs nobody will 
> understand ? Why not detecting wifi interfaces as such and configure the 
> broadcast value accordingly ? The same goes for ethernet / vpns ?

While is makes sense to find some sensible defaults for such values, in
my opinion everything should be as configurable as possible. I don't
think it would be a problem to have some dozens knobs more if a normal
user almost never has to touch them. On the other hand, for testing,
debugging and development purposes, it can save a lot of time not having
to recompile the kernel for such changes all the time.

Also, for VPN connections, automatic detection of the link type isn't
generally possible, as we'd have to know which medium the VPN is going
over...

Regards,
Matthias

> 
> Cheers,
> Marek
>
  
Marek Lindner March 8, 2013, 8:06 p.m. UTC | #2
On Saturday, March 09, 2013 03:42:34 Matthias Schiffer wrote:
> > In general, I like the idea but the approach isn't the best. Can't we
> > automate these settings instead of adding hundreds of little knobs
> > nobody will understand ? Why not detecting wifi interfaces as such and
> > configure the broadcast value accordingly ? The same goes for ethernet /
> > vpns ?
> 
> While is makes sense to find some sensible defaults for such values, in
> my opinion everything should be as configurable as possible. I don't
> think it would be a problem to have some dozens knobs more if a normal
> user almost never has to touch them. On the other hand, for testing,
> debugging and development purposes, it can save a lot of time not having
> to recompile the kernel for such changes all the time.

Knobs only used for debugging and development should never go into sysfs. User 
space APIs are created for life time. Once it is there and we submit it to the 
kernel the API has to stay and has to be supported forever.


> Also, for VPN connections, automatic detection of the link type isn't
> generally possible, as we'd have to know which medium the VPN is going
> over...

You are running batman-adv over VPN links on top of wifi interfaces ?

Cheers,
Marek
  
Matthias Schiffer March 8, 2013, 9:50 p.m. UTC | #3
On 03/08/2013 09:06 PM, Marek Lindner wrote:
> On Saturday, March 09, 2013 03:42:34 Matthias Schiffer wrote:
>>> In general, I like the idea but the approach isn't the best. Can't we
>>> automate these settings instead of adding hundreds of little knobs
>>> nobody will understand ? Why not detecting wifi interfaces as such and
>>> configure the broadcast value accordingly ? The same goes for ethernet /
>>> vpns ?
>>
>> While is makes sense to find some sensible defaults for such values, in
>> my opinion everything should be as configurable as possible. I don't
>> think it would be a problem to have some dozens knobs more if a normal
>> user almost never has to touch them. On the other hand, for testing,
>> debugging and development purposes, it can save a lot of time not having
>> to recompile the kernel for such changes all the time.
> 
> Knobs only used for debugging and development should never go into sysfs. User 
> space APIs are created for life time. Once it is there and we submit it to the 
> kernel the API has to stay and has to be supported forever.

You have a point there... While I still think making the broadcast count
adjustable makes sense, I'd also be happy to get a patch accepted that
just sets the broadcast count depending on the interface type.

My current plan is:

* WLAN devices: 3 broadcasts
* Everything else: 1 broadcast

Any additional types that should be considered?

> 
> 
>> Also, for VPN connections, automatic detection of the link type isn't
>> generally possible, as we'd have to know which medium the VPN is going
>> over...
> 
> You are running batman-adv over VPN links on top of wifi interfaces ?
Not yet, but extrapolating from the setups I've already done, it's
probably just a matter of time until I am ;)

Matthias

> 
> Cheers,
> Marek
>
  
Marek Lindner March 9, 2013, 10:07 a.m. UTC | #4
On Saturday, March 09, 2013 05:50:31 Matthias Schiffer wrote:
> You have a point there... While I still think making the broadcast count
> adjustable makes sense, I'd also be happy to get a patch accepted that
> just sets the broadcast count depending on the interface type.
> 
> My current plan is:
> 
> * WLAN devices: 3 broadcasts
> * Everything else: 1 broadcast
> 
> Any additional types that should be considered?

Sounds good to me.

Cheers,
Marek
  

Patch

diff --git a/send.c b/send.c
index 0a0bb45..4a73c37 100644
--- a/send.c
+++ b/send.c
@@ -237,12 +237,14 @@  static void batadv_send_outstanding_bcast_packet(struct work_struct *work)
 	struct sk_buff *skb1;
 	struct net_device *soft_iface;
 	struct batadv_priv *bat_priv;
+	int num_bcasts;
 
 	delayed_work = container_of(work, struct delayed_work, work);
 	forw_packet = container_of(delayed_work, struct batadv_forw_packet,
 				   delayed_work);
 	soft_iface = forw_packet->if_incoming->soft_iface;
 	bat_priv = netdev_priv(soft_iface);
+	num_bcasts = atomic_read(&bat_priv->num_bcasts);
 
 	spin_lock_bh(&bat_priv->forw_bcast_list_lock);
 	hlist_del(&forw_packet->list);
@@ -271,7 +273,7 @@  static void batadv_send_outstanding_bcast_packet(struct work_struct *work)
 	forw_packet->num_packets++;
 
 	/* if we still have some more bcasts to send */
-	if (forw_packet->num_packets < 3) {
+	if (forw_packet->num_packets < num_bcasts) {
 		_batadv_add_bcast_packet_to_list(bat_priv, forw_packet,
 						 msecs_to_jiffies(5));
 		return;
diff --git a/soft-interface.c b/soft-interface.c
index 403b8c4..53771a4 100644
--- a/soft-interface.c
+++ b/soft-interface.c
@@ -464,6 +464,7 @@  static int batadv_softif_init_late(struct net_device *dev)
 	atomic_set(&bat_priv->gw_bandwidth, 41);
 	atomic_set(&bat_priv->orig_interval, 1000);
 	atomic_set(&bat_priv->hop_penalty, 30);
+	atomic_set(&bat_priv->num_bcasts, 3);
 #ifdef CONFIG_BATMAN_ADV_DEBUG
 	atomic_set(&bat_priv->log_level, 0);
 #endif
diff --git a/sysfs-class-net-mesh b/sysfs-class-net-mesh
index bdcd8b4..2058cad 100644
--- a/sysfs-class-net-mesh
+++ b/sysfs-class-net-mesh
@@ -75,6 +75,14 @@  Description:
                 to send fewer wifi packets but still the same
                 content) is enabled or not.
 
+What:           /sys/class/net/<mesh_iface>/mesh/num_bcasts
+Date:           Mar 2013
+Contact:        Linus Lüssing <linus.luessing@web.de>
+Description:
+		Defines the number of broadcasts used to forward
+		a multicast (including broadcast) payload frame on an
+		interface.
+
 What:           /sys/class/net/<mesh_iface>/mesh/orig_interval
 Date:           May 2010
 Contact:        Marek Lindner <lindner_marek@yahoo.de>
diff --git a/sysfs.c b/sysfs.c
index 15a22ef..d166b87 100644
--- a/sysfs.c
+++ b/sysfs.c
@@ -435,6 +435,7 @@  BATADV_ATTR_SIF_UINT(orig_interval, S_IRUGO | S_IWUSR, 2 * BATADV_JITTER,
 		     INT_MAX, NULL);
 BATADV_ATTR_SIF_UINT(hop_penalty, S_IRUGO | S_IWUSR, 0, BATADV_TQ_MAX_VALUE,
 		     NULL);
+BATADV_ATTR_SIF_UINT(num_bcasts, S_IRUGO | S_IWUSR, 1, INT_MAX, NULL);
 BATADV_ATTR_SIF_UINT(gw_sel_class, S_IRUGO | S_IWUSR, 1, BATADV_TQ_MAX_VALUE,
 		     batadv_post_gw_deselect);
 static BATADV_ATTR(gw_bandwidth, S_IRUGO | S_IWUSR, batadv_show_gw_bwidth,
@@ -462,6 +463,7 @@  static struct batadv_attribute *batadv_mesh_attrs[] = {
 	&batadv_attr_gw_mode,
 	&batadv_attr_orig_interval,
 	&batadv_attr_hop_penalty,
+	&batadv_attr_num_bcasts,
 	&batadv_attr_gw_sel_class,
 	&batadv_attr_gw_bandwidth,
 #ifdef CONFIG_BATMAN_ADV_DEBUG
diff --git a/types.h b/types.h
index aba8364..41bfa0b 100644
--- a/types.h
+++ b/types.h
@@ -555,6 +555,7 @@  struct batadv_priv {
 	atomic_t gw_bandwidth;
 	atomic_t orig_interval;
 	atomic_t hop_penalty;
+	atomic_t num_bcasts;
 #ifdef CONFIG_BATMAN_ADV_DEBUG
 	atomic_t log_level;
 #endif