Staging: batman-adv: bonding and interface alternating
Commit Message
This patch adds interface alternating to the new bonding feature.
Instead of only sending in a round robin fashion on the usable interfaces,
we can also attempt to use a different interface for sending than for
receiving (interface alternating). This should reduce problems of the
half-duplex nature of WiFi Hardware and thus increase performance. The
bonding modes are now enhanced from two modes (disabled/enabled) to the
following 4 modes:
* 0 - bonding off
* 1 - round robin (as before)
* 2 - interface alternating
* 3 - smart bonding (round robin + interface alternating)
This is an experimental patch and targeted for upcoming experiments at
Wireless Battle Mesh v3 in Bracciano/Italy.
Feedback, comments and reviews appreciated!
Signed-off-by: Simon Wunderlich <siwu@hrz.tu-chemnitz.de>
---
Comments
Only some non-technical comments about the patch for now:
I thought we decided that the "Staging: " part in the title will be appended
when the stuff will be send to Greg and should be omitted inside our source
trees. Also your patch wouldn't apply in linux-next because the bonding stuff
is in master and not in maint (only maint will be send to the kernel folks).
> This is an experimental patch and targeted for upcoming experiments at
> Wireless Battle Mesh v3 in Bracciano/Italy.
>
> Feedback, comments and reviews appreciated!
That part is quite interesting for the mailinglist but should not be part of
the commit message, or am I wrong? In that situation such information should
be appended after the "---". git-am or similar tools should automatically
ignore that parts, but the receiver can still read that comment.
> Index: a/batman-adv-kernelland/main.h
> ===================================================================
> --- a/batman-adv-kernelland/main.h (revision 1679)
> +++ a/batman-adv-kernelland/main.h (working copy)
> @@ -63,6 +63,10 @@
> * to be considered as bonding candidates */
>
> #define BONDING_TQ_THRESHOLD 50
> +#define BONDING_NONE 0
> +#define BONDING_ROUNDROBIN 1
You've wrote something like "space""tab""1" here. Can you please remove the
space before the tab.
Have you already painted some nice graphics or wrote some text to explain the
modes further (yes, code is the best documentation, but a bad specification)?
The rest looks like it should be tested in italy :)
Best regards,
Sven
Hey,
i've checked in a changed and fixed up version of this patch in r1686.
This version has the interface alternating enabled by default, so
we don't need to change the "bonding_enabled" to "bonding_mode" anymore.
We have decided here in Bracciano to have alternating as default as it
appears to be beneficial most of the times, but we couldn't really think
of a scenario where this might be a problem.
I've also added some documentation at
http://www.open-mesh.net/wiki/bonding-alternating
and i'm looking for comments and proofreading and comments. ;)
I've also followed your other comments (hopefully correctly), so thank
you very much for the review.
best regards
Simon
On Wed, May 26, 2010 at 12:39:22PM +0200, Sven Eckelmann wrote:
> Only some non-technical comments about the patch for now:
>
> I thought we decided that the "Staging: " part in the title will be appended
> when the stuff will be send to Greg and should be omitted inside our source
> trees. Also your patch wouldn't apply in linux-next because the bonding stuff
> is in master and not in maint (only maint will be send to the kernel folks).
>
> > This is an experimental patch and targeted for upcoming experiments at
> > Wireless Battle Mesh v3 in Bracciano/Italy.
> >
> > Feedback, comments and reviews appreciated!
>
> That part is quite interesting for the mailinglist but should not be part of
> the commit message, or am I wrong? In that situation such information should
> be appended after the "---". git-am or similar tools should automatically
> ignore that parts, but the receiver can still read that comment.
>
> > Index: a/batman-adv-kernelland/main.h
> > ===================================================================
> > --- a/batman-adv-kernelland/main.h (revision 1679)
> > +++ a/batman-adv-kernelland/main.h (working copy)
> > @@ -63,6 +63,10 @@
> > * to be considered as bonding candidates */
> >
> > #define BONDING_TQ_THRESHOLD 50
> > +#define BONDING_NONE 0
> > +#define BONDING_ROUNDROBIN 1
>
> You've wrote something like "space""tab""1" here. Can you please remove the
> space before the tab.
>
> Have you already painted some nice graphics or wrote some text to explain the
> modes further (yes, code is the best documentation, but a bad specification)?
>
> The rest looks like it should be tested in italy :)
>
> Best regards,
> Sven
Simon Wunderlich wrote:
> I've also followed your other comments (hopefully correctly), so thank
> you very much for the review.
[...]
> > > This is an experimental patch and targeted for upcoming experiments at
> > > Wireless Battle Mesh v3 in Bracciano/Italy.
> > >
> > > Feedback, comments and reviews appreciated!
> >
> > That part is quite interesting for the mailinglist but should not be part
> > of the commit message, or am I wrong? In that situation such information
> > should be appended after the "---". git-am or similar tools should
> > automatically ignore that parts, but the receiver can still read that
> > comment.
That "---" part was actually meant for the mail and not for the svn commit
messages. It will not be stripped from commit messages by git-svn - so it
remains in the git master branch. I have removed it manually from my rebase
branch - please tell me if that wasn't what you wanted and I will re-add it.
I noticed the "The patch also includes a bug fix...". What exactly do you mean
here? I would guess that it is only this part:
> + /* we only care if the other candidate is even
> + * considered as candidate. */
> + if (tmp_neigh_node2->next_bond_candidate == NULL)
> + continue;
> +
> +
But nice to see that the testing resulted in an even more compact patch which
is better readable. :)
Best regards,
Sven
Hey Sven,
On Fri, Jun 04, 2010 at 10:56:50PM +0200, Sven Eckelmann wrote:
> Simon Wunderlich wrote:
> > I've also followed your other comments (hopefully correctly), so thank
> > you very much for the review.
> [...]
> > > > This is an experimental patch and targeted for upcoming experiments at
> > > > Wireless Battle Mesh v3 in Bracciano/Italy.
> > > >
> > > > Feedback, comments and reviews appreciated!
> > >
> > > That part is quite interesting for the mailinglist but should not be part
> > > of the commit message, or am I wrong? In that situation such information
> > > should be appended after the "---". git-am or similar tools should
> > > automatically ignore that parts, but the receiver can still read that
> > > comment.
>
> That "---" part was actually meant for the mail and not for the svn commit
> messages. It will not be stripped from commit messages by git-svn - so it
> remains in the git master branch. I have removed it manually from my rebase
> branch - please tell me if that wasn't what you wanted and I will re-add it.
Oh, okay. We don't actually need it in the commit message, this was more an
internal notice, so it is fine to keep it out.
>
> I noticed the "The patch also includes a bug fix...". What exactly do you mean
> here? I would guess that it is only this part:
>
> > + /* we only care if the other candidate is even
> > + * considered as candidate. */
> > + if (tmp_neigh_node2->next_bond_candidate == NULL)
> > + continue;
> > +
> > +
Yep, that is the bug fix. ;)
>
> But nice to see that the testing resulted in an even more compact patch which
> is better readable. :)
thank you. I'm also happier with this version. ;)
best regards,
Simon
===================================================================
@@ -119,7 +119,7 @@
struct bat_priv {
struct net_device_stats stats;
atomic_t aggregation_enabled;
- atomic_t bonding_enabled;
+ atomic_t bonding_mode;
atomic_t vis_mode;
atomic_t gw_mode;
atomic_t gw_class;
===================================================================
@@ -250,7 +250,7 @@
if (!orig_node)
orig_node = transtable_search(ethhdr->h_dest);
- router = find_router(orig_node);
+ router = find_router(orig_node, NULL);
if (!router)
goto unlock;
===================================================================
@@ -514,7 +514,7 @@
/* unicast packet */
case BAT_UNICAST:
- ret = recv_unicast_packet(skb);
+ ret = recv_unicast_packet(skb, batman_if);
break;
/* broadcast packet */
===================================================================
@@ -417,7 +417,7 @@
{
/* don't care if bonding is not enabled */
- if (!atomic_read(&bat_priv->bonding_enabled)) {
+ if (!atomic_read(&bat_priv->bonding_mode)) {
orig_node->bond.candidates = 0;
return;
}
@@ -429,7 +429,7 @@
return;
}
-/* mark possible bonding candidates in the neighbor list */
+/* mark possible bond.candidates in the neighbor list */
void update_bonding_candidates(struct bat_priv *bat_priv,
struct orig_node *orig_node)
{
@@ -440,7 +440,7 @@
struct neigh_node *first_candidate, *last_candidate;
/* don't care if bonding is not enabled */
- if (!atomic_read(&bat_priv->bonding_enabled)) {
+ if (!atomic_read(&bat_priv->bonding_mode)) {
orig_node->bond.candidates = 0;
return;
}
@@ -453,7 +453,7 @@
best_tq = orig_node->router->tq_avg;
- /* update bonding candidates */
+ /* update bond.candidates */
candidates = 0;
@@ -1007,14 +1007,16 @@
/* find a suitable router for this originator, and use
* bonding if possible. */
-struct neigh_node *find_router(struct orig_node *orig_node)
+struct neigh_node *find_router(struct orig_node *orig_node,
+ struct batman_if *recv_if)
{
/* FIXME: each orig_node->batman_if will be attached to a softif */
struct bat_priv *bat_priv = netdev_priv(soft_device);
struct orig_node *primary_orig_node;
struct orig_node *router_orig;
- struct neigh_node *router;
+ struct neigh_node *router, *first_candidate;
static uint8_t zero_mac[ETH_ALEN] = {0, 0, 0, 0, 0, 0};
+ int bonding_mode;
if (!orig_node)
return NULL;
@@ -1022,8 +1024,9 @@
if (!orig_node->router)
return NULL;
- /* don't care if bonding is not enabled */
- if (!atomic_read(&bat_priv->bonding_enabled))
+ /* just return default router if bonding is not enabled */
+ bonding_mode = atomic_read(&bat_priv->bonding_mode);
+ if (!bonding_mode)
return orig_node->router;
router_orig = orig_node->router->orig_node;
@@ -1052,19 +1055,50 @@
if (primary_orig_node->bond.candidates < 2)
return orig_node->router;
- router = primary_orig_node->bond.selected;
+ switch (bonding_mode) {
+ case BONDING_ROUNDROBIN:
+ router = primary_orig_node->bond.selected;
- /* sanity check - this should never happen. */
- if (!router)
- return orig_node->router;
+ /* sanity check - this should never happen. */
+ if (!router)
+ return orig_node->router;
- /* select the next bonding partner ... */
- primary_orig_node->bond.selected = router->next_bond_candidate;
+ /* select the next bonding partner ... */
+ primary_orig_node->bond.selected = router->next_bond_candidate;
+ break;
+ case BONDING_ALTERNATE:
+ /* in alternate mode, the first node should
+ * always choose the default router. */
+ if (recv_if == NULL)
+ return orig_node->router;
+ /* all nodes between should choose a candidate which
+ * is is not on the interface where the packet came
+ * in. There might be more than one alternative
+ * interface, and we send the packet in a round robin
+ * fashion on these interfaces in this case. */
+ case BONDING_SMART:
+ first_candidate = primary_orig_node->bond.selected;
+ router = first_candidate;
+
+ do {
+ /* recv_if == NULL on the first node. */
+ if (router->if_incoming != recv_if)
+ break;
+ router = router->next_bond_candidate;
+ } while (router != first_candidate);
+
+ /* select the next to have some diversity here. */
+ primary_orig_node->bond.selected = router->next_bond_candidate;
+ break;
+ default:
+ router = orig_node->router;
+ }
+
return router;
}
-int recv_unicast_packet(struct sk_buff *skb)
+int recv_unicast_packet(struct sk_buff *skb, struct batman_if *recv_if)
{
struct unicast_packet *unicast_packet;
struct orig_node *orig_node;
@@ -1116,7 +1150,7 @@
orig_node = ((struct orig_node *)
hash_find(orig_hash, unicast_packet->dest));
- router = find_router(orig_node);
+ router = find_router(orig_node, recv_if);
if (!router) {
spin_unlock_irqrestore(&orig_hash_lock, flags);
===================================================================
@@ -63,6 +63,10 @@
* to be considered as bonding candidates */
#define BONDING_TQ_THRESHOLD 50
+#define BONDING_NONE 0
+#define BONDING_ROUNDROBIN 1
+#define BONDING_ALTERNATE 2
+#define BONDING_SMART 3
#define MAX_AGGREGATION_BYTES 512 /* should not be bigger than 512 bytes or
* change the size of
===================================================================
@@ -32,11 +32,12 @@
struct neigh_node *neigh_node,
unsigned char *hna_buff, int hna_buff_len);
int recv_icmp_packet(struct sk_buff *skb);
-int recv_unicast_packet(struct sk_buff *skb);
+int recv_unicast_packet(struct sk_buff *skb, struct batman_if *recv_if);
int recv_bcast_packet(struct sk_buff *skb);
int recv_vis_packet(struct sk_buff *skb);
int recv_bat_packet(struct sk_buff *skb,
struct batman_if *batman_if);
-struct neigh_node *find_router(struct orig_node *orig_node);
+struct neigh_node *find_router(struct orig_node *orig_node,
+ struct batman_if *recv_if);
void update_bonding_candidates(struct bat_priv *bat_priv,
struct orig_node *orig_node);
===================================================================
@@ -92,10 +92,9 @@
{
struct device *dev = to_dev(kobj->parent);
struct bat_priv *bat_priv = netdev_priv(to_net_dev(dev));
- int bond_status = atomic_read(&bat_priv->bonding_enabled);
+ int bonding_mode = atomic_read(&bat_priv->bonding_mode);
- return sprintf(buff, "%s\n",
- bond_status == 0 ? "disabled" : "enabled");
+ return sprintf(buff, "%d\n", bonding_mode);
}
static ssize_t store_bond(struct kobject *kobj, struct attribute *attr,
@@ -104,17 +103,25 @@
struct device *dev = to_dev(kobj->parent);
struct net_device *net_dev = to_net_dev(dev);
struct bat_priv *bat_priv = netdev_priv(net_dev);
- int bonding_enabled_tmp = -1;
+ int bonding_mode_tmp = -1;
- if (((count == 2) && (buff[0] == '1')) ||
- (strncmp(buff, "enable", 6) == 0))
- bonding_enabled_tmp = 1;
+ if (count == 2)
+ switch (buff[0]) {
+ case '0':
+ bonding_mode_tmp = 0;
+ break;
+ case '1':
+ bonding_mode_tmp = BONDING_ROUNDROBIN;
+ break;
+ case '2':
+ bonding_mode_tmp = BONDING_ALTERNATE;
+ break;
+ case '3':
+ bonding_mode_tmp = BONDING_SMART;
+ break;
+ }
- if (((count == 2) && (buff[0] == '0')) ||
- (strncmp(buff, "disable", 7) == 0))
- bonding_enabled_tmp = 0;
-
- if (bonding_enabled_tmp < 0) {
+ if (bonding_mode_tmp < 0) {
if (buff[count - 1] == '\n')
buff[count - 1] = '\0';
@@ -123,16 +130,14 @@
return -EINVAL;
}
- if (atomic_read(&bat_priv->bonding_enabled) == bonding_enabled_tmp)
+ if (atomic_read(&bat_priv->bonding_mode) == bonding_mode_tmp)
return count;
- printk(KERN_INFO "batman-adv:Changing bonding from: %s to: %s on mesh: %s\n",
- atomic_read(&bat_priv->bonding_enabled) == 1 ?
- "enabled" : "disabled",
- bonding_enabled_tmp == 1 ? "enabled" : "disabled",
+ printk(KERN_INFO "batman-adv:Changing bonding from: %d to: %d on mesh: %s\n",
+ atomic_read(&bat_priv->bonding_mode), bonding_mode_tmp,
net_dev->name);
- atomic_set(&bat_priv->bonding_enabled, (unsigned)bonding_enabled_tmp);
+ atomic_set(&bat_priv->bonding_mode, (unsigned)bonding_mode_tmp);
return count;
}
@@ -303,7 +308,7 @@
/* FIXME: should be done in the general mesh setup
routine as soon as we have it */
atomic_set(&bat_priv->aggregation_enabled, 1);
- atomic_set(&bat_priv->bonding_enabled, 0);
+ atomic_set(&bat_priv->bonding_mode, 0);
atomic_set(&bat_priv->vis_mode, VIS_TYPE_CLIENT_UPDATE);
atomic_set(&bat_priv->gw_mode, GW_MODE_OFF);
atomic_set(&bat_priv->gw_class, 0);